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

["Daniel Schaefer" <daniel.schaefer@hpe.com>]

On 5/20/20 2:00 PM, Leif Lindholm wrote:
> (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/ .
> 
Meaning if the commit says Signed-off-by: Abner, I can't send it to the 
list?
>> 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
>> +
I talked to Abner about this and we're going to remove this 
LibraryClasses section. It's not needed.
>> +
>> +
>> 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.
Oh, I squashed it into the other commit:
ProcessorPkg/Library: Add RiscVEdk2SbiLib
> 
>> +
>> +#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
> 
In addition to the cover letter, I also responded to your previous 
review, see:
https://edk2.groups.io/g/devel/message/59681
TLDR; we need to keep it.