On Fri, May 29, 2020 at 14:43:43 +0200, Daniel Schaefer wrote:
> Hi Leif,
>
> thanks for this super careful review!
> Comments and one question inline.
>
> - Daniel
>
> On 5/20/20 8:27 PM, Leif Lindholm wrote:
> > On Fri, May 15, 2020 at 15:39:37 +0200, Daniel Schaefer wrote:
> > > Library provides interfaces to invoke SBI extensions.
> > >
> > > Signed-off-by: Daniel Schaefer <daniel.schae...@hpe.com>
> > >
> > > Cc: Abner Chang <abner.ch...@hpe.com>
> > > Cc: Gilbert Chen <gilbert.c...@hpe.com>
> > > Cc: Michael D Kinney <michael.k.kin...@intel.com>
> > > Cc: Leif Lindholm <l...@nuviainc.com>
> > > ---
> > > Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf
> > > | 28 +
> > > Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
> > > | 43 +-
> > > Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
> > > | 631 ++++++++++++++++
> > > Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
> > > | 789 ++++++++++++++++++++
> > > 4 files changed, 1466 insertions(+), 25 deletions(-)
> > >
> > > diff --git
> > > a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
> > > b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
> > > new file mode 100644
> > > index 000000000000..cf77814e3bbc
> > > --- /dev/null
> > > +++ b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
> >
> > > + long error; ///< SBI status code
> > > + long value; ///< Value returned
> >
> > This looks like a maintenance hazard (means different things to GCC
> > and VS for example). Can we use UINT32?
>
> I'll use UINTN because it's bigger than 32bits on RISCV64
Oh, then definitely that, yes.
> >
> > > +};
> > > +
> > > +#define SbiCall0(ext_id, func_id) \
> > > + SbiCall(ext_id, func_id, 0, 0, 0, 0, 0, 0)
> > > +#define SbiCall1(ext_id, func_id, arg0) \
> > > + SbiCall(ext_id, func_id, arg0, 0, 0, 0, 0, 0)
> > > +#define SbiCall2(ext_id, func_id, arg0, arg1) \
> > > + SbiCall(ext_id, func_id, arg0, arg1, 0, 0, 0, 0)
> > > +#define SbiCall3(ext_id, func_id, arg0, arg1, arg2) \
> > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, 0, 0, 0)
> > > +#define SbiCall4(ext_id, func_id, arg0, arg1, arg2, arg3) \
> > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, 0, 0)
> > > +#define SbiCall5(ext_id, func_id, arg0, arg1, arg2, arg3, arg4) \
> > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, 0)
> > > +#define SbiCall6(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, arg5) \
> > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, arg5)
> >
> > Ugh. This looks way too much like pre-EFIAPI x86 code.
> >
> > Is this a pattern used across multiple codebases?
> > If not, could we make this a simple argc/argv instead and do the
> > unpacking inside SbiCall()? Hmm, maybe that would make the call sites
> > even worse.
> >
> > If we need to keep these, coding style says all macros should use
> > UPPERCASE_AND_UNDERSCORES.
> >
>
> Yeah, I think argc/argv is going to make the callsites worse. What about
> vararg, like I used in SbiVendorCall in
> Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
> ? Or does that have a performance impact? Maybe an architecture specific
> one?
It *might* have a performance impact, but I'd be astonished if it is
noticeable.
Yes please, if you don't mind, I think that would really improve the
readability.
> > > +/**
> > > + Get the CPU's vendor ID
> > > +
> > > + Reads the mvendorid CSR.
> >
> > What is an MvendorId? MachineVendorId?
> > Even if an official register name, I would prefer function and
> > arguments to be given proper descriptive CamelCase names.
>
> Yes, it's the official register name. Alright, will change it.
If it *is* the official register name, adding it to the glossary
section is also OK.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +SbiGetMarchId (
> > > + OUT UINTN *MarchId
> > > + );
> > > +
> > > +/**
> > > + Get the CPU's implementation ID
> > > +
> > > + Reads the mimpid CSR.
> > > +
> > > + @param[out] MimpId The CPU's implementation ID.
> >
> > Above "Impl" is used for "Impelentation". *Strictly* speaking, "Impl"
> > doesn't fall in the pretty small group of abbreviations permitted
> > without a glossary section in the source file, but it's clear enough
> > to me I'll let it slip.
> > The same cannot be said for "Imp".
> > MachineImplId?
>
> Sounds good. Should it be added to the permitted abbreviations?
> If I spell it out fully here, it becomes quite long.
I'm OK with Impl. We should consider adding it to the list.
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +SbiHartGetStatus (
> > > + IN UINTN HartId,
> > > + OUT UINTN *HartStatus
> > > + );
> > > +
> > > +///
> > > +/// Timer extension
> > > +///
> > > +
> > > +/**
> > > + Clear pending timer interrupt bit and set timer for next event after
> > > StimeValue.
> >
> > What does the S stand for in Stime?
>
> That's what they call it in the spec: stime_value.
> I guess it stands for supervisor. Should we change it to just `Time`?
That would be a valid thing to do.
Another would be to add it to the glossary.
> > > + if (!ret.error) {
> > > + ScratchSpace = (struct sbi_scratch *) ret.value;
> > > + SbiPlatform = (struct sbi_platform *) sbi_platform_ptr(ScratchSpace);
> > > + SbiPlatform->firmware_context = (long unsigned int) FirmwareContext;
> >
> > UINT64?
>
> UINTN for compatibility with 32bits.
Of course.
Thanks!
/
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60454): https://edk2.groups.io/g/devel/message/60454
Mute This Topic: https://groups.io/mt/74227141/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
