Re: [edk2-devel] [PATCH v2 3/3] ProcessorPkg/Library: Add RiscVEdk2SbiLib

["Leif Lindholm" <leif@nuviainc.com>]

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.schaefer@hpe.com>
> > > 
> > > Cc: Abner Chang <abner.chang@hpe.com>
> > > Cc: Gilbert Chen <gilbert.chen@hpe.com>
> > > Cc: Michael D Kinney <michael.k.kinney@intel.com>
> > > Cc: Leif Lindholm <leif@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