Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 03/20] MdePkg: Add BaseRiscVSbiLib Library for RISC-V

["Michael D Kinney" <michael.d.kinney@intel.com>]

I agree with keeping the .S file in this component.

I was only referring to BaseLib as other examples that used
.S instead of inline assembly.

Mike

> -----Original Message-----
> From: Sunil V L <sunilvl@ventanamicro.com>
> Sent: Wednesday, February 8, 2023 11:19 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Abner Chang <abner.chang@amd.com>
> Subject: Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 03/20] MdePkg: Add BaseRiscVSbiLib Library for RISC-V
> 
> Hi Mike,
> 
> On Thu, Feb 09, 2023 at 01:45:48AM +0000, Michael D Kinney wrote:
> > One comment below.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Sunil V L <sunilvl@ventanamicro.com>
> > > Sent: Saturday, January 28, 2023 11:18 AM
> > > To: devel@edk2.groups.io
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>;
> > > Abner Chang <abner.chang@amd.com>
> > > Subject: [edk2-staging/RiscV64QemuVirt PATCH V7 03/20] MdePkg: Add BaseRiscVSbiLib Library for RISC-V
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4076
> > >
> > > This library is required to make SBI ecalls from the S-mode EDK2.
> > > This is mostly copied from
> > > edk2-platforms/Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > Acked-by: Abner Chang <abner.chang@amd.com>
> > > ---
> > >  MdePkg/MdePkg.dec                                  |   4 +
> > >  MdePkg/MdePkg.dsc                                  |   3 +
> > >  MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf |  25 +++
> > >  MdePkg/Include/Library/BaseRiscVSbiLib.h           | 127 +++++++++++
> > >  MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.c   | 227 ++++++++++++++++++++
> > >  5 files changed, 386 insertions(+)
> > >
> > > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > > index 3d08f20d15b0..ca2e4dcf815c 100644
> > > --- a/MdePkg/MdePkg.dec
> > > +++ b/MdePkg/MdePkg.dec
> > > @@ -316,6 +316,10 @@ [LibraryClasses.IA32, LibraryClasses.X64]
> > >    ##  @libraryclass  Provides function to support TDX processing.
> > >    TdxLib|Include/Library/TdxLib.h
> > >
> > > +[LibraryClasses.RISCV64]
> > > +  ##  @libraryclass  Provides function to make ecalls to SBI
> > > +  BaseRiscVSbiLib|Include/Library/BaseRiscVSbiLib.h
> > > +
> > >  [Guids]
> > >    #
> > >    # GUID defined in UEFI2.1/UEFI2.0/EFI1.1
> > > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> > > index 32a852dc466e..0ac7618b4623 100644
> > > --- a/MdePkg/MdePkg.dsc
> > > +++ b/MdePkg/MdePkg.dsc
> > > @@ -190,4 +190,7 @@ [Components.ARM, Components.AARCH64]
> > >    MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf
> > >    MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
> > >
> > > +[Components.RISCV64]
> > > +  MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf
> > > +
> > >  [BuildOptions]
> > > diff --git a/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf b/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf
> > > new file mode 100644
> > > index 000000000000..d03132bf01c1
> > > --- /dev/null
> > > +++ b/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf
> > > @@ -0,0 +1,25 @@
> > > +## @file
> > > +# RISC-V Library to call SBI ecalls
> > > +#
> > > +#  Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> > > +#
> > > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > > +##
> > > +
> > > +[Defines]
> > > +  INF_VERSION         = 0x0001001b
> > > +  BASE_NAME           = BaseRiscVSbiLib
> > > +  FILE_GUID           = D742CF3D-E600-4009-8FB5-318073008508
> > > +  MODULE_TYPE         = BASE
> > > +  VERSION_STRING      = 1.0
> > > +  LIBRARY_CLASS       = RiscVSbiLib
> > > +
> > > +[Sources]
> > > +  BaseRiscVSbiLib.c
> > > +
> > > +[Packages]
> > > +  MdePkg/MdePkg.dec
> > > +
> > > +[LibraryClasses]
> > > +  BaseLib
> > > diff --git a/MdePkg/Include/Library/BaseRiscVSbiLib.h b/MdePkg/Include/Library/BaseRiscVSbiLib.h
> > > new file mode 100644
> > > index 000000000000..3a3cbfb879f1
> > > --- /dev/null
> > > +++ b/MdePkg/Include/Library/BaseRiscVSbiLib.h
> > > @@ -0,0 +1,127 @@
> > > +/** @file
> > > +  Library to call the RISC-V SBI ecalls
> > > +
> > > +  Copyright (c) 2021-2022, Hewlett Packard Development LP. All rights reserved.<BR>
> > > +
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +  @par Glossary:
> > > +    - Hart - Hardware Thread, similar to a CPU core
> > > +
> > > +  Currently, EDK2 needs to call SBI only to set the time and to do system reset.
> > > +
> > > +**/
> > > +
> > > +#ifndef RISCV_SBI_LIB_H_
> > > +#define RISCV_SBI_LIB_H_
> > > +
> > > +#include <Uefi.h>
> > > +
> > > +/* SBI Extension IDs */
> > > +#define SBI_EXT_TIME  0x54494D45
> > > +#define SBI_EXT_SRST  0x53525354
> > > +
> > > +/* SBI function IDs for TIME extension*/
> > > +#define SBI_EXT_TIME_SET_TIMER  0x0
> > > +
> > > +/* SBI function IDs for SRST extension */
> > > +#define SBI_EXT_SRST_RESET  0x0
> > > +
> > > +#define SBI_SRST_RESET_TYPE_SHUTDOWN     0x0
> > > +#define SBI_SRST_RESET_TYPE_COLD_REBOOT  0x1
> > > +#define SBI_SRST_RESET_TYPE_WARM_REBOOT  0x2
> > > +
> > > +#define SBI_SRST_RESET_REASON_NONE     0x0
> > > +#define SBI_SRST_RESET_REASON_SYSFAIL  0x1
> > > +
> > > +/* SBI return error codes */
> > > +#define SBI_SUCCESS                0
> > > +#define SBI_ERR_FAILED             -1
> > > +#define SBI_ERR_NOT_SUPPORTED      -2
> > > +#define SBI_ERR_INVALID_PARAM      -3
> > > +#define SBI_ERR_DENIED             -4
> > > +#define SBI_ERR_INVALID_ADDRESS    -5
> > > +#define SBI_ERR_ALREADY_AVAILABLE  -6
> > > +#define SBI_ERR_ALREADY_STARTED    -7
> > > +#define SBI_ERR_ALREADY_STOPPED    -8
> > > +
> > > +#define SBI_LAST_ERR  SBI_ERR_ALREADY_STOPPED
> > > +
> > > +typedef struct {
> > > +  UINT64    BootHartId;
> > > +  VOID      *PeiServiceTable;    // PEI Service table
> > > +  VOID      *PrePiHobList;       // Pre PI Hob List
> > > +  UINT64    FlattenedDeviceTree; // Pointer to Flattened Device tree
> > > +} EFI_RISCV_FIRMWARE_CONTEXT;
> > > +
> > > +//
> > > +// EDK2 OpenSBI firmware extension return status.
> > > +//
> > > +typedef struct {
> > > +  UINTN    Error; ///< SBI status code
> > > +  UINTN    Value; ///< Value returned
> > > +} SBI_RET;
> > > +
> > > +VOID
> > > +EFIAPI
> > > +SbiSetTimer (
> > > +  IN  UINT64  Time
> > > +  );
> > > +
> > > +EFI_STATUS
> > > +EFIAPI
> > > +SbiSystemReset (
> > > +  IN  UINTN  ResetType,
> > > +  IN  UINTN  ResetReason
> > > +  );
> > > +
> > > +/**
> > > +  Get firmware context of the calling hart.
> > > +
> > > +  @param[out] FirmwareContext      The firmware context pointer.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +GetFirmwareContext (
> > > +  OUT EFI_RISCV_FIRMWARE_CONTEXT  **FirmwareContext
> > > +  );
> > > +
> > > +/**
> > > +  Set firmware context of the calling hart.
> > > +
> > > +  @param[in] FirmwareContext       The firmware context pointer.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +SetFirmwareContext (
> > > +  IN EFI_RISCV_FIRMWARE_CONTEXT  *FirmwareContext
> > > +  );
> > > +
> > > +/**
> > > +  Get pointer to OpenSBI Firmware Context
> > > +
> > > +  Get the pointer of firmware context.
> > > +
> > > +  @param    FirmwareContextPtr   Pointer to retrieve pointer to the
> > > +                                 Firmware Context.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +GetFirmwareContextPointer (
> > > +  IN OUT EFI_RISCV_FIRMWARE_CONTEXT  **FirmwareContextPtr
> > > +  );
> > > +
> > > +/**
> > > +  Set pointer to OpenSBI Firmware Context
> > > +
> > > +  Set the pointer of firmware context.
> > > +
> > > +  @param    FirmwareContextPtr   Pointer to Firmware Context.
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +SetFirmwareContextPointer (
> > > +  IN EFI_RISCV_FIRMWARE_CONTEXT  *FirmwareContextPtr
> > > +  );
> > > +
> > > +#endif
> > > diff --git a/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.c b/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.c
> > > new file mode 100644
> > > index 000000000000..15222a528753
> > > --- /dev/null
> > > +++ b/MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.c
> > > @@ -0,0 +1,227 @@
> > > +/** @file
> > > +  Instance of the SBI ecall library.
> > > +
> > > +  It allows calling an SBI function via an ecall from S-Mode.
> > > +
> > > +  Copyright (c) 2021-2022, Hewlett Packard Development LP. All rights reserved.<BR>
> > > +
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#include <Library/BaseLib.h>
> > > +#include <Library/BaseMemoryLib.h>
> > > +#include <Library/DebugLib.h>
> > > +#include <Library/BaseRiscVSbiLib.h>
> > > +
> > > +//
> > > +// Maximum arguments for SBI ecall
> > > +#define SBI_CALL_MAX_ARGS  6
> > > +
> > > +/**
> > > +  Call SBI call using ecall instruction.
> > > +
> > > +  Asserts when NumArgs exceeds SBI_CALL_MAX_ARGS.
> > > +
> > > +  @param[in] ExtId    SBI extension ID.
> > > +  @param[in] FuncId   SBI function ID.
> > > +  @param[in] NumArgs  Number of arguments to pass to the ecall.
> > > +  @param[in] ...      Argument list for the ecall.
> > > +
> > > +  @retval  Returns SBI_RET structure with value and error code.
> > > +
> > > +**/
> > > +STATIC
> > > +SBI_RET
> > > +EFIAPI
> > > +SbiCall (
> > > +  IN  UINTN  ExtId,
> > > +  IN  UINTN  FuncId,
> > > +  IN  UINTN  NumArgs,
> > > +  ...
> > > +  )
> > > +{
> > > +  UINTN    I;
> > > +  SBI_RET  Ret;
> > > +  UINTN    Args[SBI_CALL_MAX_ARGS];
> > > +  VA_LIST  ArgList;
> > > +
> > > +  VA_START (ArgList, NumArgs);
> > > +
> > > +  if (NumArgs > SBI_CALL_MAX_ARGS) {
> > > +    Ret.Error = SBI_ERR_INVALID_PARAM;
> > > +    Ret.Value = -1;
> > > +    return Ret;
> > > +  }
> > > +
> > > +  for (I = 0; I < SBI_CALL_MAX_ARGS; I++) {
> > > +    if (I < NumArgs) {
> > > +      Args[I] = VA_ARG (ArgList, UINTN);
> > > +    } else {
> > > +      // Default to 0 for all arguments that are not given
> > > +      Args[I] = 0;
> > > +    }
> > > +  }
> > > +
> > > +  VA_END (ArgList);
> > > +
> > > +  register UINTN  a0 asm ("a0") = Args[0];
> > > +  register UINTN  a1 asm ("a1") = Args[1];
> > > +  register UINTN  a2 asm ("a2") = Args[2];
> > > +  register UINTN  a3 asm ("a3") = Args[3];
> > > +  register UINTN  a4 asm ("a4") = Args[4];
> > > +  register UINTN  a5 asm ("a5") = Args[5];
> > > +  register UINTN  a6 asm ("a6") = (UINTN)(FuncId);
> > > +  register UINTN  a7 asm ("a7") = (UINTN)(ExtId);
> > > +
> > > +  asm volatile ("ecall" \
> > > +       : "+r" (a0), "+r" (a1) \
> > > +       : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7) \
> > > +       : "memory"); \
> > > +  Ret.Error = a0;
> > > +  Ret.Value = a1;
> >
> > Why is inline assembly used here?  I see use of .S files in BaseLib
> > Can this be converted to .S file?
> >
> It was ported as is from edk2-platforms. I can convert into .S file and
> I prefer to add it in this library itself than BaseLib.
> 
> Thanks!,
> Sunil