[edk2-devel] [platform/devel-riscvplatforms PATCHv1 2/2] ProcessorPkg/Library: Add EDK2 RISC-V OpenSBI library.
Daniel Schaefer
daniel.schaefer at hpe.com
Fri May 15 13:38:44 UTC 2020
On 5/12/20 1:17 AM, Leif Lindholm wrote:
> On Mon, May 11, 2020 at 19:20:15 +0200, Daniel Schaefer wrote:
>> 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: Daniel Schaefer <daniel.schaefer at hpe.com>
>> Co-authored-by: Abner Chang <abner.chang at hpe.com>
>> Co-authored-by: Gilbert Chen <gilbert.chen at hpe.com>
>> Reviewed-by: Leif Lindholm <leif at nuviainc.com>
>>
>> Cc: Leif Lindholm <leif.lindholm at nuviainc.com>
>> Cc: Gilbert Chen <gilbert.chen at hpe.com>
>> Cc: Michael D Kinney <michael.d.kinney at intel.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 | 66 ++++++++++++++++
>> 3 files changed, 205 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
Did you want to leave a comment here?
It does seem like this Lib is not needed. Why did you add it, Abner?
Is PcdLib necessary?
>> +
>> +
>> +
>> 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
>
> These #defines are translations of what exists in
> opensbi/include/sbi/sbi_error.h - can we use the definitions there
> directly (redefining them here)?
Yes, you're right. One is improperly defined in OpenSBI but I'll use the
other ones here.
>
>> +
>> +#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..3964bf00ea88
>> --- /dev/null
>> +++ b/Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
>> @@ -0,0 +1,66 @@
>> +/** @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_
>> +
> I think this header needs to include either Base.h or ProcessorBind.h?
>
> I am also somewhat confused by this file existing both here and in
> edk2-staging RISC-V-V2. Am I on the wrong timeline?
Yes, that's a good idea. I'll include Base.h
We have abandoned the branch RISC-V-V2 on edk2-staging. Like mentioned
in the cover letter,
we only merged the bare minimum to edk2 and will include all of the
remaining code in edk2-platforms.
Thus this file will only remain here.
>
>> +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"
> Please add a comment about what PRILX is for, given this isn't
> something I can even grep for in this repo. Maybe consider also moving
> it after all the typedefs, so it isn't as undexpected?
Yes, we don't use it currently but we need to define it, because in the
OpenSBI header
it's guarded by #ifndef OPENSBI_EXTERNAL_SBI_TYPES, which we define to
override
their types.
>
>> +
>> +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))
>> +
>> +#define likely(x) __builtin_expect((x), 1)
>> +#define unlikely(x) __builtin_expect((x), 0)
> It may be premature to start worrying about support in non-gcc-like
> toolchains, but if that *does* happen, it might be nicer if it fails
> with a clear error message about "unsupported toolchain" (Base.h uses
> #if defined(__GNUC__) || defined(__clang__)
> ).
Good catch!
However it seems that both GCC and Clang support this builtin
>
> /
> Leif
Thanks for the review. I'll send an updated patch.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59681): https://edk2.groups.io/g/devel/message/59681
Mute This Topic: https://groups.io/mt/74140933/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list