Re: [PATCH v2 2/3] ProcessorPkg/Library: Add RiscVOpensbiLib

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

(Fixing Mike's email in reply)

On Fri, May 15, 2020 at 15:39:36 +0200, Daniel Schaefer wrote:
> From: Abner Chang <abner.chang@hpe.com>
> 
> EDK2 RISC-V OpenSBI library which pull in external source files under
> RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi to the build process.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> Co-authored-by: Daniel Schaefer <daniel.schaefer@hpe.com>

These two fields have flipped contents since v1 (without being
mentioned in either cover letter or below --- of this one).

The v1 form was correct - only the contributor can certify the
adherence of the contribution to https://developercertificate.org/.

> Co-authored-by: Gilbert Chen <gilbert.chen@hpe.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> 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/RiscVOpensbiLib/RiscVOpensbiLib.inf | 60 +++++++++++++++
>  Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h     | 79 ++++++++++++++++++++
>  Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h                      | 73 ++++++++++++++++++
>  3 files changed, 212 insertions(+)
> 
> diff --git a/Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf b/Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf
> new file mode 100644
> index 000000000000..59dbd67d8e03
> --- /dev/null
> +++ b/Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf
> @@ -0,0 +1,60 @@
> +## @file
> +# RISC-V Opensbi Library Instance.
> +#
> +#  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      = RiscVOpensbiLib
> +  FILE_GUID      = 6EF0C812-66F6-11E9-93CE-3F5D5F0DF0A7
> +  MODULE_TYPE    = BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = RiscVOpensbiLib
> +
> +[Sources]
> +  opensbi/lib/sbi/riscv_asm.c
> +  opensbi/lib/sbi/riscv_atomic.c
> +  opensbi/lib/sbi/riscv_hardfp.S
> +  opensbi/lib/sbi/riscv_locks.c
> +  opensbi/lib/sbi/sbi_console.c
> +  opensbi/lib/sbi/sbi_ecall.c
> +  opensbi/lib/sbi/sbi_ecall_vendor.c
> +  opensbi/lib/sbi/sbi_ecall_replace.c
> +  opensbi/lib/sbi/sbi_ecall_legacy.c
> +  opensbi/lib/sbi/sbi_ecall_base.c
> +  opensbi/lib/sbi/sbi_emulate_csr.c
> +  opensbi/lib/sbi/sbi_fifo.c
> +  opensbi/lib/sbi/sbi_hart.c
> +  opensbi/lib/sbi/sbi_hfence.S
> +  opensbi/lib/sbi/sbi_illegal_insn.c
> +  opensbi/lib/sbi/sbi_init.c
> +  opensbi/lib/sbi/sbi_ipi.c
> +  opensbi/lib/sbi/sbi_misaligned_ldst.c
> +  opensbi/lib/sbi/sbi_scratch.c
> +  opensbi/lib/sbi/sbi_string.c
> +  opensbi/lib/sbi/sbi_system.c
> +  opensbi/lib/sbi/sbi_timer.c
> +  opensbi/lib/sbi/sbi_tlb.c
> +  opensbi/lib/sbi/sbi_trap.c
> +  opensbi/lib/sbi/sbi_unpriv.c
> +  opensbi/lib/utils/sys/clint.c
> +  opensbi/lib/utils/irqchip/plic.c
> +  opensbi/lib/utils/serial/sifive-uart.c
> +  opensbi/lib/utils/serial/uart8250.c
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec   # For libfdt.
> +  MdePkg/MdePkg.dec
> +  Silicon/RISC-V/ProcessorPkg/RiscVProcessorPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  PcdLib
> +  RiscVCpuLib
> +
> +
> +
> diff --git a/Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h b/Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
> new file mode 100644
> index 000000000000..c5c0bd6d9b01
> --- /dev/null
> +++ b/Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
> @@ -0,0 +1,79 @@
> +/** @file
> +  SBI inline function calls.
> +
> +  Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef EDK2_SBI_H_
> +#define EDK2_SBI_H_
> +
> +#include <include/sbi/riscv_asm.h> // Reference to header file in opensbi
> +#include <RiscVImpl.h>
> +#include <sbi/sbi_types.h>  // Reference to header file wrapper
> +
> +#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

Did the cover-letter changelog not suggest these had been changed from
local definitions to redefining the existing ones from Opensbi?
If this was "not possible" (the exception stated in cover letter), I
would have expected a comment above (below ---) on why. Or a reply to
my feedback on v1.

> +
> +#define SBI_BASE_EXT                   0x10
> +#define SBI_HSM_EXT                    0x48534D
> +#define SBI_TIME_EXT                   0x54494D45
> +#define SBI_IPI_EXT                    0x735049
> +#define SBI_RFNC_EXT                   0x52464E43
> +
> +//
> +// Below two definitions should be defined in OpenSBI.
> +//
> +#define SBI_EXT_FIRMWARE_CODE_BASE_START 0x0A000000
> +#define SBI_EXT_FIRMWARE_CODE_BASE_END   0x0AFFFFFF
> +
> +#define SBI_GET_SPEC_VERSION_FUNC      0
> +#define SBI_GET_IMPL_ID_FUNC           1
> +#define SBI_GET_IMPL_VERSION_FUNC      2
> +#define SBI_PROBE_EXTENSION_FUNC       3
> +#define SBI_GET_MVENDORID_FUNC         4
> +#define SBI_GET_MARCHID_FUNC           5
> +#define SBI_GET_MIMPID_FUNC            6
> +
> +#define SBI_HART_START_FUNC            0
> +#define SBI_HART_STOP_FUNC             1
> +#define SBI_HART_GET_STATUS_FUNC       2
> +
> +#define RISC_V_MAX_HART_SUPPORTED 16
> +
> +typedef
> +VOID
> +(EFIAPI *RISCV_HART_SWITCH_MODE)(
> +  IN  UINTN   FuncArg0,
> +  IN  UINTN   FuncArg1,
> +  IN  UINTN   NextAddr,
> +  IN  UINTN   NextMode,
> +  IN  BOOLEAN NextVirt
> +  );
> +
> +//
> +// Keep the structure member in 64-bit alignment.
> +//
> +typedef struct {
> +    UINT64                 IsaExtensionSupported;  // The ISA extension this core supported.
> +    RISCV_UINT128          MachineVendorId;        // Machine vendor ID
> +    RISCV_UINT128          MachineArchId;          // Machine Architecture ID
> +    RISCV_UINT128          MachineImplId;          // Machine Implementation ID
> +    RISCV_HART_SWITCH_MODE HartSwitchMode;         // OpenSBI's function to switch the mode of a hart
> +} EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC;
> +#define FIRMWARE_CONTEXT_HART_SPECIFIC_SIZE  (64 * 8) // This is the size of EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC
> +                                                      // structure. Referred by both C code and assembly code.
> +
> +typedef struct {
> +  VOID            *PeiServiceTable;       // PEI Service table
> +  EFI_RISCV_FIRMWARE_CONTEXT_HART_SPECIFIC  *HartSpecific[RISC_V_MAX_HART_SUPPORTED];
> +} EFI_RISCV_OPENSBI_FIRMWARE_CONTEXT;
> +
> +#endif
> diff --git a/Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h b/Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
> new file mode 100644
> index 000000000000..5f3278e8461f
> --- /dev/null
> +++ b/Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
> @@ -0,0 +1,73 @@
> +/** @file
> +  RISC-V OpesbSBI header file reference.
> +
> +  Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef EDK2_SBI_TYPES_H_
> +#define EDK2_SBI_TYPES_H_
> +
> +#include <Base.h>
> +
> +typedef INT8    s8;
> +typedef UINT8   u8;
> +typedef UINT8   uint8_t;
> +
> +typedef INT16   s16;
> +typedef UINT16  u16;
> +typedef INT16   int16_t;
> +typedef UINT16  uint16_t;
> +
> +typedef INT32   s32;
> +typedef UINT32  u32;
> +typedef INT32   int32_t;
> +typedef UINT32  uint32_t;
> +
> +typedef INT64   s64;
> +typedef UINT64  u64;
> +typedef INT64   int64_t;
> +typedef UINT64  uint64_t;
> +
> +#define PRILX   "016lx"

Feedback on PRILX not addressed, or commented on.

/
    Leif

> +
> +typedef BOOLEAN  bool;
> +typedef unsigned long   ulong;
> +typedef UINT64   uintptr_t;
> +typedef UINT64   size_t;
> +typedef INT64    ssize_t;
> +typedef UINT64   virtual_addr_t;
> +typedef UINT64   virtual_size_t;
> +typedef UINT64   physical_addr_t;
> +typedef UINT64   physical_size_t;
> +
> +#define __packed        __attribute__((packed))
> +#define __noreturn      __attribute__((noreturn))
> +
> +#if defined(__GNUC__) || defined(__clang__)
> +  #define likely(x) __builtin_expect((x), 1)
> +  #define unlikely(x) __builtin_expect((x), 0)
> +#else
> +  #define likely(x) (x)
> +  #define unlikely(x) (x)
> +#endif
> +
> +#undef offsetof
> +#ifdef __compiler_offsetof
> +#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE,MEMBER)
> +#else
> +#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> +#endif
> +
> +#define container_of(ptr, type, member) ({            \
> +  const typeof(((type *)0)->member) * __mptr = (ptr); \
> +  (type *)((char *)__mptr - offsetof(type, member)); })
> +
> +#define CLAMP(a, lo, hi) MIN(MAX(a, lo), hi)
> +#define ROUNDUP(a, b) ((((a)-1) / (b) + 1) * (b))
> +#define ROUNDDOWN(a, b) ((a) / (b) * (b))
> +
> +/* clang-format on */
> +
> +#endif
> -- 
> 2.26.1
>