* [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt
@ 2023-02-23 23:54 Andrei Warkentin
2023-02-23 23:54 ` [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw)
To: devel; +Cc: Andrei Warkentin
Dear all,
Please find the following patches for your reviewing
pleasure. Sunil suggested I put these into a single patch
set, instead of sending out these separately.
This patch set is also available at https://github.com/andreiw/edk2-rv-wip
Andrei Warkentin (7):
OvmfPkg: RiscVVirt: add SATA support
MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in
ImageFormatSupported
MdePkg: BaseLib: don't log in RISCV InternalSwitchStack
MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name.
MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo
UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting
UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception
handling
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 7 +
OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf | 7 +
MdePkg/Include/Protocol/DebugSupport.h | 32 ++-
.../CpuExceptionHandlerLib.h | 11 +-
MdeModulePkg/Core/Dxe/Image/Image.c | 3 +-
.../BaseLib/RiscV64/InternalSwitchStack.c | 8 -
.../BasePeCoffLib/RiscV/PeCoffLoaderEx.c | 9 +-
UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 39 +--
.../CpuExceptionHandlerLib.c | 232 ++++++++++++++++--
MdePkg/Library/BaseCpuLib/RiscV/Cpu.S | 4 +-
.../SupervisorTrapHandler.S | 17 +-
11 files changed, 299 insertions(+), 70 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-24 10:00 ` [edk2-devel] " Sunil V L 2023-02-23 23:54 ` [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported Andrei Warkentin ` (5 subsequent siblings) 6 siblings, 1 reply; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel; +Cc: Andrei Warkentin, Sunil V L, Jiewen Yao, Ard Biesheuvel Tested with a PCIe pass-thru'd AHCI controller. Cc: Sunil V L <sunilvl@ventanamicro.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 7 +++++++ OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc index 054d511138c3..28d9af4d79b9 100644 --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc @@ -426,6 +426,13 @@ [Components] MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf + # + # SATA + # + MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf + MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf + MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf + # # NVME Driver # diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf b/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf index 6072c37af03d..354c9271d10c 100644 --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf @@ -137,6 +137,13 @@ [FV.DXEFV] INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf +# +# SATA +# +INF MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf +INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf +INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf + # # NVME Driver # -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support 2023-02-23 23:54 ` [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin @ 2023-02-24 10:00 ` Sunil V L 0 siblings, 0 replies; 15+ messages in thread From: Sunil V L @ 2023-02-24 10:00 UTC (permalink / raw) To: devel, andrei.warkentin; +Cc: Jiewen Yao, Ard Biesheuvel On Thu, Feb 23, 2023 at 05:54:48PM -0600, Andrei Warkentin wrote: > Tested with a PCIe pass-thru'd AHCI controller. > > Cc: Sunil V L <sunilvl@ventanamicro.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> > Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> > --- > OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 7 +++++++ > OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf | 7 +++++++ > 2 files changed, 14 insertions(+) > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin 2023-02-23 23:54 ` [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-24 10:02 ` Sunil V L 2023-02-23 23:54 ` [PATCH 3/7] MdePkg: BaseLib: don't log in RISCV InternalSwitchStack Andrei Warkentin ` (4 subsequent siblings) 6 siblings, 1 reply; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel Cc: Andrei Warkentin, Sunil V L, Daniel Schaefer, Michael D Kinney, Liming Gao, Zhiguang Liu ARM64 and X64 may allow such foreign images to be used when driver implementing EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL is present. Cc: Sunil V L <sunilvl@ventanamicro.com> Cc: Daniel Schaefer <git@danielschaefer.me> 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: Andrei Warkentin <andrei.warkentin@intel.com> --- MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c b/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c index adbfe9ccf580..0e9ee395dc26 100644 --- a/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c +++ b/MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c @@ -104,7 +104,14 @@ PeCoffLoaderImageFormatSupported ( IN UINT16 Machine ) { - if (Machine == IMAGE_FILE_MACHINE_RISCV64) { + /* + * ARM64 and X64 may allow such foreign images to be used when + * a driver implementing EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL is + * present. + */ + if (Machine == IMAGE_FILE_MACHINE_RISCV64 || + Machine == IMAGE_FILE_MACHINE_ARM64 || + Machine == IMAGE_FILE_MACHINE_X64) { return TRUE; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported 2023-02-23 23:54 ` [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported Andrei Warkentin @ 2023-02-24 10:02 ` Sunil V L 0 siblings, 0 replies; 15+ messages in thread From: Sunil V L @ 2023-02-24 10:02 UTC (permalink / raw) To: Andrei Warkentin Cc: devel, Daniel Schaefer, Michael D Kinney, Liming Gao, Zhiguang Liu On Thu, Feb 23, 2023 at 05:54:49PM -0600, Andrei Warkentin wrote: > ARM64 and X64 may allow such foreign images to be used when > driver implementing EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL is > present. > > Cc: Sunil V L <sunilvl@ventanamicro.com> > Cc: Daniel Schaefer <git@danielschaefer.me> > 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: Andrei Warkentin <andrei.warkentin@intel.com> > --- > MdePkg/Library/BasePeCoffLib/RiscV/PeCoffLoaderEx.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/7] MdePkg: BaseLib: don't log in RISCV InternalSwitchStack 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin 2023-02-23 23:54 ` [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin 2023-02-23 23:54 ` [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-23 23:54 ` [PATCH 4/7] MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name Andrei Warkentin ` (3 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel Cc: Andrei Warkentin, Daniel Schaefer, Michael D Kinney, Liming Gao, Zhiguang Liu, Sunil V L InternalSwitchStack may be called with a TPL high enough for a DebugLib implementation to assert. Other arch implementations don't log either. Cc: Daniel Schaefer <git@danielschaefer.me> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c index cf646e498aba..b78424c16383 100644 --- a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c +++ b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c @@ -44,14 +44,6 @@ InternalSwitchStack ( { BASE_LIBRARY_JUMP_BUFFER JumpBuffer; - DEBUG (( - DEBUG_INFO, - "RISC-V InternalSwitchStack Entry:%x Context1:%x Context2:%x NewStack%x\n", \ - EntryPoint, - Context1, - Context2, - NewStack - )); JumpBuffer.RA = (UINTN)EntryPoint; JumpBuffer.SP = (UINTN)NewStack - sizeof (VOID *); JumpBuffer.S0 = (UINT64)(UINTN)Context1; -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/7] MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name. 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin ` (2 preceding siblings ...) 2023-02-23 23:54 ` [PATCH 3/7] MdePkg: BaseLib: don't log in RISCV InternalSwitchStack Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-23 23:54 ` [PATCH 5/7] MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo Andrei Warkentin ` (2 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel Cc: Andrei Warkentin, Daniel Schaefer, Michael D Kinney, Liming Gao, Zhiguang Liu, Sunil V L CpuSleep, not _CpuSleep. Cc: Daniel Schaefer <git@danielschaefer.me> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- MdePkg/Library/BaseCpuLib/RiscV/Cpu.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MdePkg/Library/BaseCpuLib/RiscV/Cpu.S b/MdePkg/Library/BaseCpuLib/RiscV/Cpu.S index 375b91d31427..d6560087e080 100644 --- a/MdePkg/Library/BaseCpuLib/RiscV/Cpu.S +++ b/MdePkg/Library/BaseCpuLib/RiscV/Cpu.S @@ -10,9 +10,9 @@ .align 3 .section .text -.global ASM_PFX(_CpuSleep) +.global ASM_PFX(CpuSleep) -ASM_PFX(_CpuSleep): +ASM_PFX(CpuSleep): wfi ret -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/7] MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin ` (3 preceding siblings ...) 2023-02-23 23:54 ` [PATCH 4/7] MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-23 23:54 ` [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting Andrei Warkentin 2023-02-23 23:54 ` [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin 6 siblings, 0 replies; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel; +Cc: Andrei Warkentin, Daniel Schaefer, Liming Gao, Jian J Wang, Sunil V L This fixes messages like: "Image type AARCH64 can't be loaded on <Unknown> UEFI system" Cc: Daniel Schaefer <git@danielschaefer.me> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Jian J Wang <jian.j.wang@intel.com> Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- MdeModulePkg/Core/Dxe/Image/Image.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c index 06cc6744b8c6..8704ebea9a7c 100644 --- a/MdeModulePkg/Core/Dxe/Image/Image.c +++ b/MdeModulePkg/Core/Dxe/Image/Image.c @@ -81,7 +81,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED MACHINE_TYPE_INFO mMachineTypeInfo[] = { { EFI_IMAGE_MACHINE_IA64, L"IA64" }, { EFI_IMAGE_MACHINE_X64, L"X64" }, { EFI_IMAGE_MACHINE_ARMTHUMB_MIXED, L"ARM" }, - { EFI_IMAGE_MACHINE_AARCH64, L"AARCH64" } + { EFI_IMAGE_MACHINE_AARCH64, L"AARCH64" }, + { EFI_IMAGE_MACHINE_RISCV64, L"RISCV64" }, }; UINT16 mDxeCoreImageMachineType = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin ` (4 preceding siblings ...) 2023-02-23 23:54 ` [PATCH 5/7] MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-24 10:45 ` Sunil V L 2023-02-23 23:54 ` [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin 6 siblings, 1 reply; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel; +Cc: Andrei Warkentin, Sunil V L, Daniel Schaefer The TimerDxe implementation doesn't account for the physical time passed due to timer handler execution or (perhaps even more importantly) time spent with interrupts masked. Other implementations (e.g. like the Arm one) do. If the timer tick is always incremented at a fixed rate, then you can slow down UEFI's perception of time by running long sections of code in a critical section. Cc: Sunil V L <sunilvl@ventanamicro.com> Cc: Daniel Schaefer <git@danielschaefer.me> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 33 +++++++++++++++------------ 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c index db153f715e60..50f57a9780f0 100644 --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c @@ -40,7 +40,8 @@ STATIC EFI_TIMER_NOTIFY mTimerNotifyFunction; // // The current period of the timer interrupt // -STATIC UINT64 mTimerPeriod = 0; +STATIC UINT64 mTimerPeriod = 0; +STATIC UINT64 mLastPeriodStart = 0; /** Timer Interrupt Handler. @@ -56,25 +57,31 @@ TimerInterruptHandler ( ) { EFI_TPL OriginalTPL; - UINT64 RiscvTimer; + UINT64 PeriodStart = RiscVReadTimer (); OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); if (mTimerNotifyFunction != NULL) { - mTimerNotifyFunction (mTimerPeriod); + // + // For any number of reasons, the ticks could be coming + // in slower than mTimerPeriod. For example, some code + // is doing a *lot* of stuff inside an EFI_TPL_HIGH + // critical section, and this should not cause the EFI + // time to increment slower. So when we take an interrupt, + // account for the actual time passed. + // + mTimerNotifyFunction (PeriodStart - mLastPeriodStart); } - RiscVDisableTimerInterrupt (); // Disable SMode timer int - RiscVClearPendingTimerInterrupt (); if (mTimerPeriod == 0) { + RiscVDisableTimerInterrupt (); gBS->RestoreTPL (OriginalTPL); - RiscVDisableTimerInterrupt (); // Disable SMode timer int return; } - RiscvTimer = RiscVReadTimer (); - SbiSetTimer (RiscvTimer += mTimerPeriod); + mLastPeriodStart = PeriodStart; + SbiSetTimer (PeriodStart += mTimerPeriod); + RiscVEnableTimerInterrupt (); // enable SMode timer int gBS->RestoreTPL (OriginalTPL); - RiscVEnableTimerInterrupt (); // enable SMode timer int } /** @@ -154,8 +161,6 @@ TimerDriverSetTimerPeriod ( IN UINT64 TimerPeriod ) { - UINT64 RiscvTimer; - DEBUG ((DEBUG_INFO, "TimerDriverSetTimerPeriod(0x%lx)\n", TimerPeriod)); if (TimerPeriod == 0) { @@ -164,9 +169,9 @@ TimerDriverSetTimerPeriod ( return EFI_SUCCESS; } - mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us - RiscvTimer = RiscVReadTimer (); - SbiSetTimer (RiscvTimer + mTimerPeriod); + mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us + mLastPeriodStart = RiscVReadTimer (); + SbiSetTimer (mLastPeriodStart + mTimerPeriod); mCpu->EnableInterrupt (mCpu); RiscVEnableTimerInterrupt (); // enable SMode timer int -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting 2023-02-23 23:54 ` [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting Andrei Warkentin @ 2023-02-24 10:45 ` Sunil V L 0 siblings, 0 replies; 15+ messages in thread From: Sunil V L @ 2023-02-24 10:45 UTC (permalink / raw) To: Andrei Warkentin; +Cc: devel, Daniel Schaefer On Thu, Feb 23, 2023 at 05:54:53PM -0600, Andrei Warkentin wrote: > The TimerDxe implementation doesn't account for the physical > time passed due to timer handler execution or (perhaps even > more importantly) time spent with interrupts masked. > > Other implementations (e.g. like the Arm one) do. If the > timer tick is always incremented at a fixed rate, then > you can slow down UEFI's perception of time by running > long sections of code in a critical section. > > Cc: Sunil V L <sunilvl@ventanamicro.com> > Cc: Daniel Schaefer <git@danielschaefer.me> > Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> > --- > UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 33 +++++++++++++++------------ > 1 file changed, 19 insertions(+), 14 deletions(-) > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling 2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin ` (5 preceding siblings ...) 2023-02-23 23:54 ` [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting Andrei Warkentin @ 2023-02-23 23:54 ` Andrei Warkentin 2023-02-24 10:44 ` Sunil V L ` (2 more replies) 6 siblings, 3 replies; 15+ messages in thread From: Andrei Warkentin @ 2023-02-23 23:54 UTC (permalink / raw) To: devel; +Cc: Andrei Warkentin, Sunil V L, Daniel Schaefer RegisterCpuInterruptHandler did not allow setting exception handlers for anything beyond the timer IRQ. Beyond that, it didn't meet the spec around handling of inputs. RiscVSupervisorModeTrapHandler now will invoke set handlers for both exceptions and interrupts. Two arrays of handlers are maintained - one for exceptions and one for interrupts. For unhandled traps, RiscVSupervisorModeTrapHandler dumps state using the now implemented DumpCpuContext. For EFI_SYSTEM_CONTEXT_RISCV64, extend this with the trapped PC address (SEPC), just like on AArch64 (ELR). This is necessary for X86EmulatorPkg to work as it allows a trap handler to return execution to a different place. Add SSTATUS/STVAL as well, at least for debugging purposes. There is no value in hiding this. Fix nested exception handling. Handler code should not be saving SIE (the value is saved in SSTATUS.SPIE) or directly restored (that's done by SRET). Save and restore the entire SSTATUS and STVAL, too. Cc: Sunil V L <sunilvl@ventanamicro.com> Cc: Daniel Schaefer <git@danielschaefer.me> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> --- MdePkg/Include/Protocol/DebugSupport.h | 32 ++- .../CpuExceptionHandlerLib.h | 11 +- UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 6 +- .../CpuExceptionHandlerLib.c | 232 ++++++++++++++++-- .../SupervisorTrapHandler.S | 17 +- 5 files changed, 254 insertions(+), 44 deletions(-) diff --git a/MdePkg/Include/Protocol/DebugSupport.h b/MdePkg/Include/Protocol/DebugSupport.h index 2b0ae2d1577b..9742663619c5 100644 --- a/MdePkg/Include/Protocol/DebugSupport.h +++ b/MdePkg/Include/Protocol/DebugSupport.h @@ -613,11 +613,34 @@ typedef struct { #define EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT 7 #define EXCEPT_RISCV_ENV_CALL_FROM_UMODE 8 #define EXCEPT_RISCV_ENV_CALL_FROM_SMODE 9 -#define EXCEPT_RISCV_ENV_CALL_FROM_HMODE 10 +#define EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE 10 #define EXCEPT_RISCV_ENV_CALL_FROM_MMODE 11 +#define EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT 12 +#define EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT 13 +#define EXCEPT_RISCV_14 14 +#define EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT 15 +#define EXCEPT_RISCV_16 16 +#define EXCEPT_RISCV_17 17 +#define EXCEPT_RISCV_18 18 +#define EXCEPT_RISCV_19 19 +#define EXCEPT_RISCV_INST_GUEST_PAGE_FAULT 20 +#define EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT 21 +#define EXCEPT_RISCV_VIRTUAL_INSTRUCTION 22 +#define EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT 23 +#define EXCEPT_RISCV_MAX_EXCEPTIONS (EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT) -#define EXCEPT_RISCV_SOFTWARE_INT 0x0 -#define EXCEPT_RISCV_TIMER_INT 0x1 +/// +/// RISC-V processor exception types for interrupts. +/// +#define EXCEPT_RISCV_IS_IRQ(x) ((x & 0x8000000000000000UL) != 0) +#define EXCEPT_RISCV_IRQ_INDEX(x) (x & 0x7FFFFFFFFFFFFFFFUL) +#define EXCEPT_RISCV_IRQ_0 0x8000000000000000UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE 0x8000000000000001UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE 0x8000000000000002UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE 0x8000000000000003UL +#define EXCEPT_RISCV_IRQ_4 0x8000000000000004UL +#define EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE 0x8000000000000005UL +#define EXCEPT_RISCV_MAX_IRQS (EXCEPT_RISCV_IRQ_INDEX(EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE)) typedef struct { UINT64 X0; @@ -652,6 +675,9 @@ typedef struct { UINT64 X29; UINT64 X30; UINT64 X31; + UINT64 SEPC; + UINT32 SSTATUS; + UINT32 STVAL; } EFI_SYSTEM_CONTEXT_RISCV64; // diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h index 30f47e87552b..9b7e1304dd3b 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h @@ -59,7 +59,7 @@ SupervisorModeTrap ( #define SMODE_TRAP_REGS_t6 31 #define SMODE_TRAP_REGS_sepc 32 #define SMODE_TRAP_REGS_sstatus 33 -#define SMODE_TRAP_REGS_sie 34 +#define SMODE_TRAP_REGS_stval 34 #define SMODE_TRAP_REGS_last 35 #define SMODE_TRAP_REGS_OFFSET(x) ((SMODE_TRAP_REGS_##x) * __SIZEOF_POINTER__) @@ -68,7 +68,7 @@ SupervisorModeTrap ( #pragma pack(1) typedef struct { // - // Below are follow the format of EFI_SYSTEM_CONTEXT + // Below follow the format of EFI_SYSTEM_CONTEXT. // UINT64 zero; UINT64 ra; @@ -102,14 +102,9 @@ typedef struct { UINT64 t4; UINT64 t5; UINT64 t6; - // - // Below are the additional information to - // EFI_SYSTEM_CONTEXT, private to supervisor mode trap - // and not public to EFI environment. - // UINT64 sepc; UINT64 sstatus; - UINT64 sie; + UINT64 stval; } SMODE_TRAP_REGISTERS; #pragma pack() diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c index 50f57a9780f0..ab280d3b2d7d 100644 --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c @@ -276,7 +276,11 @@ TimerDriverInitialize ( // // Install interrupt handler for RISC-V Timer. // - Status = mCpu->RegisterInterruptHandler (mCpu, EXCEPT_RISCV_TIMER_INT, TimerInterruptHandler); + Status = mCpu->RegisterInterruptHandler ( + mCpu, + EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE, + TimerInterruptHandler + ); ASSERT_EFI_ERROR (Status); // diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c index f1ee7d236aec..1beaf5ac513e 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c @@ -11,11 +11,162 @@ #include <Library/CpuExceptionHandlerLib.h> #include <Library/DebugLib.h> #include <Library/BaseLib.h> +#include <Library/SerialPortLib.h> +#include <Library/PrintLib.h> #include <Register/RiscV64/RiscVEncoding.h> - #include "CpuExceptionHandlerLib.h" -STATIC EFI_CPU_INTERRUPT_HANDLER mInterruptHandlers[2]; +// +// Define the maximum message length +// +#define MAX_DEBUG_MESSAGE_LENGTH 0x100 + +STATIC EFI_CPU_INTERRUPT_HANDLER mExceptionHandlers[EXCEPT_RISCV_MAX_EXCEPTIONS + 1]; +STATIC EFI_CPU_INTERRUPT_HANDLER mIrqHandlers[EXCEPT_RISCV_MAX_IRQS + 1]; + +STATIC CONST CHAR8 mExceptionOrIrqUnknown[] = "Unknown"; +STATIC CONST CHAR8 *mExceptionNameStr[EXCEPT_RISCV_MAX_EXCEPTIONS + 1] = { + "EXCEPT_RISCV_INST_MISALIGNED", + "EXCEPT_RISCV_INST_ACCESS_FAULT", + "EXCEPT_RISCV_ILLEGAL_INST", + "EXCEPT_RISCV_BREAKPOINT", + "EXCEPT_RISCV_LOAD_ADDRESS_MISALIGNED", + "EXCEPT_RISCV_LOAD_ACCESS_FAULT", + "EXCEPT_RISCV_STORE_AMO_ADDRESS_MISALIGNED", + "EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT", + "EXCEPT_RISCV_ENV_CALL_FROM_UMODE", + "EXCEPT_RISCV_ENV_CALL_FROM_SMODE", + "EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE", + "EXCEPT_RISCV_ENV_CALL_FROM_MMODE", + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_14", + "EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_16", + "EXCEPT_RISCV_17", + "EXCEPT_RISCV_18", + "EXCEPT_RISCV_19", + "EXCEPT_RISCV_INST_GUEST_PAGE_FAULT", + "EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT", + "EXCEPT_RISCV_VIRTUAL_INSTRUCTION", + "EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT" +}; + +STATIC CONST CHAR8 *mIrqNameStr[EXCEPT_RISCV_MAX_IRQS + 1] = { + "EXCEPT_RISCV_IRQ_0", + "EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE", + "EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE", + "EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE", + "EXCEPT_RISCV_IRQ_4", + "EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE", +}; + +/** + Prints a message to the serial port. + + @param Format Format string for the message to print. + @param ... Variable argument list whose contents are accessed + based on the format string specified by Format. + +**/ +STATIC +VOID +EFIAPI +InternalPrintMessage ( + IN CONST CHAR8 *Format, + ... + ) +{ + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; + VA_LIST Marker; + + // + // Convert the message to an ASCII String + // + VA_START (Marker, Format); + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker); + VA_END (Marker); + + // + // Send the print string to a Serial Port + // + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); +} + +/** + Get ASCII format string exception name by exception type. + + @param ExceptionType Exception type. + + @return ASCII format string exception name. +**/ +STATIC +CONST CHAR8 * +GetExceptionNameStr ( + IN EFI_EXCEPTION_TYPE ExceptionType + ) +{ + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { + return mExceptionOrIrqUnknown; + } + + return mIrqNameStr[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)]; + } + + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { + return mExceptionOrIrqUnknown; + } + + return mExceptionNameStr[ExceptionType]; +} + +/** + Display CPU information. This can be called by 3rd-party handlers + set by RegisterCpuInterruptHandler. + + @param ExceptionType Exception type. + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT. +**/ +VOID +EFIAPI +DumpCpuContext ( + IN EFI_EXCEPTION_TYPE ExceptionType, + IN EFI_SYSTEM_CONTEXT SystemContext + ) +{ + UINTN Printed = 0; + SMODE_TRAP_REGISTERS *Regs = (SMODE_TRAP_REGISTERS *) + SystemContext.SystemContextRiscV64; + + InternalPrintMessage ( + "!!!! RISCV64 Exception Type - %016x(%a) !!!!\n", + ExceptionType, + GetExceptionNameStr (ExceptionType) + ); + + #define REGS() \ + REG (t0); REG (t1); REG (t2); REG (t3); REG (t4); REG (t5); REG (t6); \ + REG (s0); REG (s1); REG (s2); REG (s3); REG (s4); REG (s5); REG (s6); \ + REG (s7); REG (s8); REG (s9); REG (s10); REG (s11); \ + REG (a0); REG (a1); REG (a2); REG (a3); REG (a4); REG (a5); REG (a6); \ + REG (a7); \ + REG (zero); REG (ra); REG (sp); REG (gp); REG (tp); \ + REG (sepc); REG (sstatus); REG (stval); + + #define REG(x) do { \ + InternalPrintMessage ("%7a = 0x%017lx%c", #x, Regs->x, \ + (++Printed % 2) ? L'\t' : L'\n'); \ + } while (0); + + REGS (); + if (Printed % 2) { + InternalPrintMessage ("\n"); + } + + #undef REG + #undef REGS +} /** Initializes all CPU exceptions entries and provides the default exception handlers. @@ -47,34 +198,59 @@ InitializeCpuExceptionHandlers ( Registers a function to be called from the processor interrupt handler. This function registers and enables the handler specified by InterruptHandler for a processor - interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the - handler for the processor interrupt or exception type specified by InterruptType is uninstalled. + interrupt or exception type specified by ExceptionType. If InterruptHandler is NULL, then the + handler for the processor interrupt or exception type specified by ExceptionType is uninstalled. The installed handler is called once for each processor interrupt or exception. NOTE: This function should be invoked after InitializeCpuExceptionHandlers() or InitializeCpuInterruptHandlers() invoked, otherwise EFI_UNSUPPORTED returned. - @param[in] InterruptType Defines which interrupt or exception to hook. + @param[in] ExceptionType Defines which interrupt or exception to hook. @param[in] InterruptHandler A pointer to a function of type EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs. If this parameter is NULL, then the handler will be uninstalled. @retval EFI_SUCCESS The handler for the processor interrupt was successfully installed or uninstalled. - @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a handler for InterruptType was + @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a handler for ExceptionType was previously installed. - @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for InterruptType was not + @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for ExceptionType was not previously installed. - @retval EFI_UNSUPPORTED The interrupt specified by InterruptType is not supported, + @retval EFI_UNSUPPORTED The interrupt specified by ExceptionType is not supported, or this function is not supported. **/ EFI_STATUS EFIAPI RegisterCpuInterruptHandler ( - IN EFI_EXCEPTION_TYPE InterruptType, + IN EFI_EXCEPTION_TYPE ExceptionType, IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler ) { - DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, InterruptType, InterruptHandler)); - mInterruptHandlers[InterruptType] = InterruptHandler; + DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, ExceptionType, InterruptHandler)); + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { + return EFI_UNSUPPORTED; + } + + if (mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] != NULL) { + return EFI_ALREADY_STARTED; + } else if (InterruptHandler == NULL) { + return EFI_INVALID_PARAMETER; + } + + mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] = InterruptHandler; + } else { + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { + return EFI_UNSUPPORTED; + } + + if (mExceptionHandlers[ExceptionType] != NULL) { + return EFI_ALREADY_STARTED; + } else if (InterruptHandler == NULL) { + return EFI_INVALID_PARAMETER; + } + + mExceptionHandlers[ExceptionType] = InterruptHandler; + } + return EFI_SUCCESS; } @@ -113,21 +289,31 @@ RiscVSupervisorModeTrapHandler ( SMODE_TRAP_REGISTERS *SmodeTrapReg ) { - UINTN SCause; + EFI_EXCEPTION_TYPE ExceptionType; EFI_SYSTEM_CONTEXT RiscVSystemContext; RiscVSystemContext.SystemContextRiscV64 = (EFI_SYSTEM_CONTEXT_RISCV64 *)SmodeTrapReg; - // - // Check scasue register. - // - SCause = (UINTN)RiscVGetSupervisorTrapCause (); - if ((SCause & (1UL << (sizeof (UINTN) * 8- 1))) != 0) { - // - // This is interrupt event. - // - SCause &= ~(1UL << (sizeof (UINTN) * 8- 1)); - if ((SCause == IRQ_S_TIMER) && (mInterruptHandlers[EXCEPT_RISCV_TIMER_INT] != NULL)) { - mInterruptHandlers[EXCEPT_RISCV_TIMER_INT](EXCEPT_RISCV_TIMER_INT, RiscVSystemContext); + ExceptionType = (UINTN)RiscVGetSupervisorTrapCause (); + + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + UINTN IrqIndex = EXCEPT_RISCV_IRQ_INDEX (ExceptionType); + + if ((IrqIndex <= EXCEPT_RISCV_MAX_IRQS) && + (mIrqHandlers[IrqIndex] != NULL)) + { + mIrqHandlers[IrqIndex](ExceptionType, RiscVSystemContext); + return; } + } else { + if ((ExceptionType <= EXCEPT_RISCV_MAX_EXCEPTIONS) && + (mExceptionHandlers[ExceptionType] != 0)) + { + mExceptionHandlers[ExceptionType](ExceptionType, RiscVSystemContext); + return; + } + } + + DumpCpuContext (ExceptionType, RiscVSystemContext); + while (1) { } } diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S index 649c4c5becf4..45070b5cda04 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S @@ -20,14 +20,14 @@ SupervisorModeTrap: sd t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) csrr t0, CSR_SSTATUS - and t0, t0, (SSTATUS_SIE | SSTATUS_SPIE) sd t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) csrr t0, CSR_SEPC sd t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) - csrr t0, CSR_SIE - sd t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) + csrr t0, CSR_STVAL + sd t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) + sd zero, SMODE_TRAP_REGS_OFFSET(zero)(sp) sd ra, SMODE_TRAP_REGS_OFFSET(ra)(sp) sd gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) sd tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) @@ -59,6 +59,7 @@ SupervisorModeTrap: sd t6, SMODE_TRAP_REGS_OFFSET(t6)(sp) /* Call to Supervisor mode trap handler in CpuExceptionHandlerLib.c */ + mv a0, sp call RiscVSupervisorModeTrapHandler /* Restore all general regisers except SP */ @@ -66,6 +67,7 @@ SupervisorModeTrap: ld gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) ld tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) ld t2, SMODE_TRAP_REGS_OFFSET(t2)(sp) + ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) ld s0, SMODE_TRAP_REGS_OFFSET(s0)(sp) ld s1, SMODE_TRAP_REGS_OFFSET(s1)(sp) ld a0, SMODE_TRAP_REGS_OFFSET(a0)(sp) @@ -93,13 +95,10 @@ SupervisorModeTrap: ld t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) csrw CSR_SEPC, t0 - ld t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) - csrw CSR_SIE, t0 - csrr t0, CSR_SSTATUS - ld t1, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) - or t0, t0, t1 + ld t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) csrw CSR_SSTATUS, t0 - ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) + ld t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) + csrw CSR_STVAL, t0 ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) addi sp, sp, SMODE_TRAP_REGS_SIZE sret -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling 2023-02-23 23:54 ` [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin @ 2023-02-24 10:44 ` Sunil V L 2023-03-02 4:59 ` [edk2-devel] " Tuan Phan [not found] ` <17488173C4DD581F.9697@groups.io> 2 siblings, 0 replies; 15+ messages in thread From: Sunil V L @ 2023-02-24 10:44 UTC (permalink / raw) To: Andrei Warkentin; +Cc: devel, Daniel Schaefer On Thu, Feb 23, 2023 at 05:54:54PM -0600, Andrei Warkentin wrote: > RegisterCpuInterruptHandler did not allow setting > exception handlers for anything beyond the timer IRQ. > Beyond that, it didn't meet the spec around handling > of inputs. > > RiscVSupervisorModeTrapHandler now will invoke > set handlers for both exceptions and interrupts. > Two arrays of handlers are maintained - one for exceptions > and one for interrupts. > > For unhandled traps, RiscVSupervisorModeTrapHandler dumps > state using the now implemented DumpCpuContext. > > For EFI_SYSTEM_CONTEXT_RISCV64, extend this with the trapped > PC address (SEPC), just like on AArch64 (ELR). This is > necessary for X86EmulatorPkg to work as it allows a trap > handler to return execution to a different place. Add > SSTATUS/STVAL as well, at least for debugging purposes. There > is no value in hiding this. > > Fix nested exception handling. Handler code should not > be saving SIE (the value is saved in SSTATUS.SPIE) or > directly restored (that's done by SRET). Save and > restore the entire SSTATUS and STVAL, too. > > Cc: Sunil V L <sunilvl@ventanamicro.com> > Cc: Daniel Schaefer <git@danielschaefer.me> > Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> > --- > MdePkg/Include/Protocol/DebugSupport.h | 32 ++- > .../CpuExceptionHandlerLib.h | 11 +- > UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 6 +- > .../CpuExceptionHandlerLib.c | 232 ++++++++++++++++-- > .../SupervisorTrapHandler.S | 17 +- > 5 files changed, 254 insertions(+), 44 deletions(-) > Reviewed-by: Sunil V L <sunilvl@ventanamicro.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling 2023-02-23 23:54 ` [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin 2023-02-24 10:44 ` Sunil V L @ 2023-03-02 4:59 ` Tuan Phan [not found] ` <17488173C4DD581F.9697@groups.io> 2 siblings, 0 replies; 15+ messages in thread From: Tuan Phan @ 2023-03-02 4:59 UTC (permalink / raw) To: devel, andrei.warkentin; +Cc: Sunil V L, Daniel Schaefer [-- Attachment #1: Type: text/plain, Size: 20363 bytes --] On Thu, Feb 23, 2023 at 3:55 PM Andrei Warkentin <andrei.warkentin@intel.com> wrote: > RegisterCpuInterruptHandler did not allow setting > exception handlers for anything beyond the timer IRQ. > Beyond that, it didn't meet the spec around handling > of inputs. > > RiscVSupervisorModeTrapHandler now will invoke > set handlers for both exceptions and interrupts. > Two arrays of handlers are maintained - one for exceptions > and one for interrupts. > > For unhandled traps, RiscVSupervisorModeTrapHandler dumps > state using the now implemented DumpCpuContext. > > For EFI_SYSTEM_CONTEXT_RISCV64, extend this with the trapped > PC address (SEPC), just like on AArch64 (ELR). This is > necessary for X86EmulatorPkg to work as it allows a trap > handler to return execution to a different place. Add > SSTATUS/STVAL as well, at least for debugging purposes. There > is no value in hiding this. > > Fix nested exception handling. Handler code should not > be saving SIE (the value is saved in SSTATUS.SPIE) or > directly restored (that's done by SRET). Save and > restore the entire SSTATUS and STVAL, too. > > Cc: Sunil V L <sunilvl@ventanamicro.com> > Cc: Daniel Schaefer <git@danielschaefer.me> > Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> > --- > MdePkg/Include/Protocol/DebugSupport.h | 32 ++- > .../CpuExceptionHandlerLib.h | 11 +- > UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 6 +- > .../CpuExceptionHandlerLib.c | 232 ++++++++++++++++-- > .../SupervisorTrapHandler.S | 17 +- > 5 files changed, 254 insertions(+), 44 deletions(-) > > diff --git a/MdePkg/Include/Protocol/DebugSupport.h > b/MdePkg/Include/Protocol/DebugSupport.h > index 2b0ae2d1577b..9742663619c5 100644 > --- a/MdePkg/Include/Protocol/DebugSupport.h > +++ b/MdePkg/Include/Protocol/DebugSupport.h > @@ -613,11 +613,34 @@ typedef struct { > #define EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT 7 > #define EXCEPT_RISCV_ENV_CALL_FROM_UMODE 8 > #define EXCEPT_RISCV_ENV_CALL_FROM_SMODE 9 > -#define EXCEPT_RISCV_ENV_CALL_FROM_HMODE 10 > +#define EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE 10 > #define EXCEPT_RISCV_ENV_CALL_FROM_MMODE 11 > +#define EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT 12 > +#define EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT 13 > +#define EXCEPT_RISCV_14 14 > +#define EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT 15 > +#define EXCEPT_RISCV_16 16 > +#define EXCEPT_RISCV_17 17 > +#define EXCEPT_RISCV_18 18 > +#define EXCEPT_RISCV_19 19 > +#define EXCEPT_RISCV_INST_GUEST_PAGE_FAULT 20 > +#define EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT 21 > +#define EXCEPT_RISCV_VIRTUAL_INSTRUCTION 22 > +#define EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT 23 > +#define EXCEPT_RISCV_MAX_EXCEPTIONS > (EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT) > > -#define EXCEPT_RISCV_SOFTWARE_INT 0x0 > -#define EXCEPT_RISCV_TIMER_INT 0x1 > +/// > +/// RISC-V processor exception types for interrupts. > +/// > +#define EXCEPT_RISCV_IS_IRQ(x) ((x & 0x8000000000000000UL) != 0) > +#define EXCEPT_RISCV_IRQ_INDEX(x) (x & 0x7FFFFFFFFFFFFFFFUL) > +#define EXCEPT_RISCV_IRQ_0 0x8000000000000000UL > +#define EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE 0x8000000000000001UL > +#define EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE 0x8000000000000002UL > +#define EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE 0x8000000000000003UL > +#define EXCEPT_RISCV_IRQ_4 0x8000000000000004UL > +#define EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE 0x8000000000000005UL > +#define EXCEPT_RISCV_MAX_IRQS > (EXCEPT_RISCV_IRQ_INDEX(EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE)) > > There is no difference in terms of processing interrupt or exception type. It is cleaner if we treat interrupts and exceptions the same. So no need to detect if an event is interrupt or exception. > typedef struct { > UINT64 X0; > @@ -652,6 +675,9 @@ typedef struct { > UINT64 X29; > UINT64 X30; > UINT64 X31; > + UINT64 SEPC; > + UINT32 SSTATUS; > + UINT32 STVAL; > } EFI_SYSTEM_CONTEXT_RISCV64; > > // > diff --git > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h > index 30f47e87552b..9b7e1304dd3b 100644 > --- > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h > +++ > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h > @@ -59,7 +59,7 @@ SupervisorModeTrap ( > #define SMODE_TRAP_REGS_t6 31 > #define SMODE_TRAP_REGS_sepc 32 > #define SMODE_TRAP_REGS_sstatus 33 > -#define SMODE_TRAP_REGS_sie 34 > +#define SMODE_TRAP_REGS_stval 34 > #define SMODE_TRAP_REGS_last 35 > > #define SMODE_TRAP_REGS_OFFSET(x) ((SMODE_TRAP_REGS_##x) * > __SIZEOF_POINTER__) > @@ -68,7 +68,7 @@ SupervisorModeTrap ( > #pragma pack(1) > typedef struct { > // > - // Below are follow the format of EFI_SYSTEM_CONTEXT > + // Below follow the format of EFI_SYSTEM_CONTEXT. > // > UINT64 zero; > UINT64 ra; > @@ -102,14 +102,9 @@ typedef struct { > UINT64 t4; > UINT64 t5; > UINT64 t6; > - // > - // Below are the additional information to > - // EFI_SYSTEM_CONTEXT, private to supervisor mode trap > - // and not public to EFI environment. > - // > UINT64 sepc; > UINT64 sstatus; > - UINT64 sie; > + UINT64 stval; > } SMODE_TRAP_REGISTERS; > #pragma pack() > > diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c > b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c > index 50f57a9780f0..ab280d3b2d7d 100644 > --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c > +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c > @@ -276,7 +276,11 @@ TimerDriverInitialize ( > // > // Install interrupt handler for RISC-V Timer. > // > - Status = mCpu->RegisterInterruptHandler (mCpu, EXCEPT_RISCV_TIMER_INT, > TimerInterruptHandler); > + Status = mCpu->RegisterInterruptHandler ( > + mCpu, > + EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE, > + TimerInterruptHandler > + ); > ASSERT_EFI_ERROR (Status); > > // > diff --git > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c > index f1ee7d236aec..1beaf5ac513e 100644 > --- > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c > +++ > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c > @@ -11,11 +11,162 @@ > #include <Library/CpuExceptionHandlerLib.h> > #include <Library/DebugLib.h> > #include <Library/BaseLib.h> > +#include <Library/SerialPortLib.h> > +#include <Library/PrintLib.h> > #include <Register/RiscV64/RiscVEncoding.h> > - > #include "CpuExceptionHandlerLib.h" > > -STATIC EFI_CPU_INTERRUPT_HANDLER mInterruptHandlers[2]; > +// > +// Define the maximum message length > +// > +#define MAX_DEBUG_MESSAGE_LENGTH 0x100 > + > +STATIC EFI_CPU_INTERRUPT_HANDLER > mExceptionHandlers[EXCEPT_RISCV_MAX_EXCEPTIONS + 1]; > +STATIC EFI_CPU_INTERRUPT_HANDLER mIrqHandlers[EXCEPT_RISCV_MAX_IRQS + 1]; > + > +STATIC CONST CHAR8 mExceptionOrIrqUnknown[] = > "Unknown"; > +STATIC CONST CHAR8 *mExceptionNameStr[EXCEPT_RISCV_MAX_EXCEPTIONS + 1] = > { > + "EXCEPT_RISCV_INST_MISALIGNED", > + "EXCEPT_RISCV_INST_ACCESS_FAULT", > + "EXCEPT_RISCV_ILLEGAL_INST", > + "EXCEPT_RISCV_BREAKPOINT", > + "EXCEPT_RISCV_LOAD_ADDRESS_MISALIGNED", > + "EXCEPT_RISCV_LOAD_ACCESS_FAULT", > + "EXCEPT_RISCV_STORE_AMO_ADDRESS_MISALIGNED", > + "EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT", > + "EXCEPT_RISCV_ENV_CALL_FROM_UMODE", > + "EXCEPT_RISCV_ENV_CALL_FROM_SMODE", > + "EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE", > + "EXCEPT_RISCV_ENV_CALL_FROM_MMODE", > + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT", > + "EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT", > + "EXCEPT_RISCV_14", > + "EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT", > + "EXCEPT_RISCV_16", > + "EXCEPT_RISCV_17", > + "EXCEPT_RISCV_18", > + "EXCEPT_RISCV_19", > + "EXCEPT_RISCV_INST_GUEST_PAGE_FAULT", > + "EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT", > + "EXCEPT_RISCV_VIRTUAL_INSTRUCTION", > + "EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT" > +}; > + > +STATIC CONST CHAR8 *mIrqNameStr[EXCEPT_RISCV_MAX_IRQS + 1] = { > + "EXCEPT_RISCV_IRQ_0", > + "EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE", > + "EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE", > + "EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE", > + "EXCEPT_RISCV_IRQ_4", > + "EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE", > +}; > + > +/** > + Prints a message to the serial port. > + > + @param Format Format string for the message to print. > + @param ... Variable argument list whose contents are accessed > + based on the format string specified by Format. > + > +**/ > +STATIC > +VOID > +EFIAPI > +InternalPrintMessage ( > + IN CONST CHAR8 *Format, > + ... > + ) > +{ > + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; > + VA_LIST Marker; > + > + // > + // Convert the message to an ASCII String > + // > + VA_START (Marker, Format); > + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker); > + VA_END (Marker); > + > + // > + // Send the print string to a Serial Port > + // > + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); > +} > + > +/** > + Get ASCII format string exception name by exception type. > + > + @param ExceptionType Exception type. > + > + @return ASCII format string exception name. > +**/ > +STATIC > +CONST CHAR8 * > +GetExceptionNameStr ( > + IN EFI_EXCEPTION_TYPE ExceptionType > + ) > +{ > + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { > + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { > + return mExceptionOrIrqUnknown; > + } > + > + return mIrqNameStr[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)]; > + } > + > + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { > + return mExceptionOrIrqUnknown; > + } > + > + return mExceptionNameStr[ExceptionType]; > +} > + > +/** > + Display CPU information. This can be called by 3rd-party handlers > + set by RegisterCpuInterruptHandler. > + > + @param ExceptionType Exception type. > + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT. > +**/ > +VOID > +EFIAPI > +DumpCpuContext ( > + IN EFI_EXCEPTION_TYPE ExceptionType, > + IN EFI_SYSTEM_CONTEXT SystemContext > + ) > +{ > + UINTN Printed = 0; > + SMODE_TRAP_REGISTERS *Regs = (SMODE_TRAP_REGISTERS *) > + SystemContext.SystemContextRiscV64; > + > + InternalPrintMessage ( > + "!!!! RISCV64 Exception Type - %016x(%a) !!!!\n", > + ExceptionType, > + GetExceptionNameStr (ExceptionType) > + ); > + > + #define REGS() > \ > + REG (t0); REG (t1); REG (t2); REG (t3); REG (t4); REG (t5); REG (t6); \ > + REG (s0); REG (s1); REG (s2); REG (s3); REG (s4); REG (s5); REG (s6); \ > + REG (s7); REG (s8); REG (s9); REG (s10); REG (s11); \ > + REG (a0); REG (a1); REG (a2); REG (a3); REG (a4); REG (a5); REG (a6); \ > + REG (a7); \ > + REG (zero); REG (ra); REG (sp); REG (gp); REG (tp); \ > + REG (sepc); REG (sstatus); REG (stval); > + > + #define REG(x) do { \ > + InternalPrintMessage ("%7a = 0x%017lx%c", #x, Regs->x, \ > + (++Printed % 2) ? L'\t' : L'\n'); \ > + } while (0); > + > + REGS (); > + if (Printed % 2) { > + InternalPrintMessage ("\n"); > + } > + > I would like to only print registers at DEBUG build only. If you look at ARM implementation, register and stack dump are only enabled at DEBUG build. Printing them all at release version can be at security risk. Anyone wanting to debug an issue likely builds a debug version. + #undef REG > + #undef REGS > +} > > /** > Initializes all CPU exceptions entries and provides the default > exception handlers. > @@ -47,34 +198,59 @@ InitializeCpuExceptionHandlers ( > Registers a function to be called from the processor interrupt handler. > > This function registers and enables the handler specified by > InterruptHandler for a processor > - interrupt or exception type specified by InterruptType. If > InterruptHandler is NULL, then the > - handler for the processor interrupt or exception type specified by > InterruptType is uninstalled. > + interrupt or exception type specified by ExceptionType. If > InterruptHandler is NULL, then the > + handler for the processor interrupt or exception type specified by > ExceptionType is uninstalled. > The installed handler is called once for each processor interrupt or > exception. > NOTE: This function should be invoked after > InitializeCpuExceptionHandlers() or > InitializeCpuInterruptHandlers() invoked, otherwise EFI_UNSUPPORTED > returned. > > - @param[in] InterruptType Defines which interrupt or exception to > hook. > + @param[in] ExceptionType Defines which interrupt or exception to > hook. > @param[in] InterruptHandler A pointer to a function of type > EFI_CPU_INTERRUPT_HANDLER that is called > when a processor interrupt occurs. If > this parameter is NULL, then the handler > will be uninstalled. > > @retval EFI_SUCCESS The handler for the processor interrupt > was successfully installed or uninstalled. > - @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a > handler for InterruptType was > + @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a > handler for ExceptionType was > previously installed. > - @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler > for InterruptType was not > + @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler > for ExceptionType was not > previously installed. > - @retval EFI_UNSUPPORTED The interrupt specified by InterruptType > is not supported, > + @retval EFI_UNSUPPORTED The interrupt specified by ExceptionType > is not supported, > or this function is not supported. > **/ > EFI_STATUS > EFIAPI > RegisterCpuInterruptHandler ( > - IN EFI_EXCEPTION_TYPE InterruptType, > + IN EFI_EXCEPTION_TYPE ExceptionType, > IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler > ) > { > - DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, > InterruptType, InterruptHandler)); > - mInterruptHandlers[InterruptType] = InterruptHandler; > + DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, > ExceptionType, InterruptHandler)); > + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { > + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { > + return EFI_UNSUPPORTED; > + } > + > + if (mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] != NULL) { > + return EFI_ALREADY_STARTED; > + } else if (InterruptHandler == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] = > InterruptHandler; > + } else { > + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { > + return EFI_UNSUPPORTED; > + } > + > + if (mExceptionHandlers[ExceptionType] != NULL) { > + return EFI_ALREADY_STARTED; > + } else if (InterruptHandler == NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + mExceptionHandlers[ExceptionType] = InterruptHandler; > + } > + > As my first comment, this code can be clearer if the interrupt and exception are the same. return EFI_SUCCESS; > } > > @@ -113,21 +289,31 @@ RiscVSupervisorModeTrapHandler ( > SMODE_TRAP_REGISTERS *SmodeTrapReg > ) > { > - UINTN SCause; > + EFI_EXCEPTION_TYPE ExceptionType; > EFI_SYSTEM_CONTEXT RiscVSystemContext; > > RiscVSystemContext.SystemContextRiscV64 = (EFI_SYSTEM_CONTEXT_RISCV64 > *)SmodeTrapReg; > - // > - // Check scasue register. > - // > - SCause = (UINTN)RiscVGetSupervisorTrapCause (); > - if ((SCause & (1UL << (sizeof (UINTN) * 8- 1))) != 0) { > - // > - // This is interrupt event. > - // > - SCause &= ~(1UL << (sizeof (UINTN) * 8- 1)); > - if ((SCause == IRQ_S_TIMER) && > (mInterruptHandlers[EXCEPT_RISCV_TIMER_INT] != NULL)) { > - mInterruptHandlers[EXCEPT_RISCV_TIMER_INT](EXCEPT_RISCV_TIMER_INT, > RiscVSystemContext); > + ExceptionType = > (UINTN)RiscVGetSupervisorTrapCause (); > + > + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { > + UINTN IrqIndex = EXCEPT_RISCV_IRQ_INDEX (ExceptionType); > + > + if ((IrqIndex <= EXCEPT_RISCV_MAX_IRQS) && > + (mIrqHandlers[IrqIndex] != NULL)) > + { > + mIrqHandlers[IrqIndex](ExceptionType, RiscVSystemContext); > + return; > } > + } else { > + if ((ExceptionType <= EXCEPT_RISCV_MAX_EXCEPTIONS) && > + (mExceptionHandlers[ExceptionType] != 0)) > + { > + mExceptionHandlers[ExceptionType](ExceptionType, > RiscVSystemContext); > + return; > + } > + } > + > + DumpCpuContext (ExceptionType, RiscVSystemContext); > + while (1) { > Would it be better with "do{} while (1)"? > } > } > diff --git > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S > index 649c4c5becf4..45070b5cda04 100644 > --- > a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S > +++ > b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S > @@ -20,14 +20,14 @@ SupervisorModeTrap: > sd t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) > > csrr t0, CSR_SSTATUS > - and t0, t0, (SSTATUS_SIE | SSTATUS_SPIE) > sd t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) > csrr t0, CSR_SEPC > sd t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) > - csrr t0, CSR_SIE > - sd t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) > + csrr t0, CSR_STVAL > + sd t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) > ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) > > + sd zero, SMODE_TRAP_REGS_OFFSET(zero)(sp) > sd ra, SMODE_TRAP_REGS_OFFSET(ra)(sp) > sd gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) > sd tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) > @@ -59,6 +59,7 @@ SupervisorModeTrap: > sd t6, SMODE_TRAP_REGS_OFFSET(t6)(sp) > > /* Call to Supervisor mode trap handler in CpuExceptionHandlerLib.c */ > + mv a0, sp > call RiscVSupervisorModeTrapHandler > > /* Restore all general regisers except SP */ > @@ -66,6 +67,7 @@ SupervisorModeTrap: > ld gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) > ld tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) > ld t2, SMODE_TRAP_REGS_OFFSET(t2)(sp) > + ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) > ld s0, SMODE_TRAP_REGS_OFFSET(s0)(sp) > ld s1, SMODE_TRAP_REGS_OFFSET(s1)(sp) > ld a0, SMODE_TRAP_REGS_OFFSET(a0)(sp) > @@ -93,13 +95,10 @@ SupervisorModeTrap: > > ld t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) > csrw CSR_SEPC, t0 > - ld t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) > - csrw CSR_SIE, t0 > - csrr t0, CSR_SSTATUS > - ld t1, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) > - or t0, t0, t1 > + ld t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) > csrw CSR_SSTATUS, t0 > - ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) > + ld t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) > + csrw CSR_STVAL, t0 > ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) > addi sp, sp, SMODE_TRAP_REGS_SIZE > sret > -- > 2.25.1 > > > > > > > [-- Attachment #2: Type: text/html, Size: 24090 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <17488173C4DD581F.9697@groups.io>]
* Re: [edk2-devel] [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling [not found] ` <17488173C4DD581F.9697@groups.io> @ 2023-03-02 5:17 ` Tuan Phan 2023-03-02 5:26 ` Andrei Warkentin 0 siblings, 1 reply; 15+ messages in thread From: Tuan Phan @ 2023-03-02 5:17 UTC (permalink / raw) To: devel, tphan; +Cc: andrei.warkentin, Sunil V L, Daniel Schaefer [-- Attachment #1: Type: text/plain, Size: 21063 bytes --] On Wed, Mar 1, 2023 at 8:59 PM Tuan Phan via groups.io <tphan= ventanamicro.com@groups.io> wrote: > > > On Thu, Feb 23, 2023 at 3:55 PM Andrei Warkentin < > andrei.warkentin@intel.com> wrote: > >> RegisterCpuInterruptHandler did not allow setting >> exception handlers for anything beyond the timer IRQ. >> Beyond that, it didn't meet the spec around handling >> of inputs. >> >> RiscVSupervisorModeTrapHandler now will invoke >> set handlers for both exceptions and interrupts. >> Two arrays of handlers are maintained - one for exceptions >> and one for interrupts. >> >> For unhandled traps, RiscVSupervisorModeTrapHandler dumps >> state using the now implemented DumpCpuContext. >> >> For EFI_SYSTEM_CONTEXT_RISCV64, extend this with the trapped >> PC address (SEPC), just like on AArch64 (ELR). This is >> necessary for X86EmulatorPkg to work as it allows a trap >> handler to return execution to a different place. Add >> SSTATUS/STVAL as well, at least for debugging purposes. There >> is no value in hiding this. >> >> Fix nested exception handling. Handler code should not >> be saving SIE (the value is saved in SSTATUS.SPIE) or >> directly restored (that's done by SRET). Save and >> restore the entire SSTATUS and STVAL, too. >> >> Cc: Sunil V L <sunilvl@ventanamicro.com> >> Cc: Daniel Schaefer <git@danielschaefer.me> >> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com> >> --- >> MdePkg/Include/Protocol/DebugSupport.h | 32 ++- >> .../CpuExceptionHandlerLib.h | 11 +- >> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 6 +- >> .../CpuExceptionHandlerLib.c | 232 ++++++++++++++++-- >> .../SupervisorTrapHandler.S | 17 +- >> 5 files changed, 254 insertions(+), 44 deletions(-) >> >> diff --git a/MdePkg/Include/Protocol/DebugSupport.h >> b/MdePkg/Include/Protocol/DebugSupport.h >> index 2b0ae2d1577b..9742663619c5 100644 >> --- a/MdePkg/Include/Protocol/DebugSupport.h >> +++ b/MdePkg/Include/Protocol/DebugSupport.h >> @@ -613,11 +613,34 @@ typedef struct { >> #define EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT 7 >> #define EXCEPT_RISCV_ENV_CALL_FROM_UMODE 8 >> #define EXCEPT_RISCV_ENV_CALL_FROM_SMODE 9 >> -#define EXCEPT_RISCV_ENV_CALL_FROM_HMODE 10 >> +#define EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE 10 >> #define EXCEPT_RISCV_ENV_CALL_FROM_MMODE 11 >> +#define EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT 12 >> +#define EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT 13 >> +#define EXCEPT_RISCV_14 14 >> +#define EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT 15 >> +#define EXCEPT_RISCV_16 16 >> +#define EXCEPT_RISCV_17 17 >> +#define EXCEPT_RISCV_18 18 >> +#define EXCEPT_RISCV_19 19 >> +#define EXCEPT_RISCV_INST_GUEST_PAGE_FAULT 20 >> +#define EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT 21 >> +#define EXCEPT_RISCV_VIRTUAL_INSTRUCTION 22 >> +#define EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT 23 >> +#define EXCEPT_RISCV_MAX_EXCEPTIONS >> (EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT) >> >> -#define EXCEPT_RISCV_SOFTWARE_INT 0x0 >> -#define EXCEPT_RISCV_TIMER_INT 0x1 >> +/// >> +/// RISC-V processor exception types for interrupts. >> +/// >> +#define EXCEPT_RISCV_IS_IRQ(x) ((x & 0x8000000000000000UL) != 0) >> +#define EXCEPT_RISCV_IRQ_INDEX(x) (x & 0x7FFFFFFFFFFFFFFFUL) >> +#define EXCEPT_RISCV_IRQ_0 0x8000000000000000UL >> +#define EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE 0x8000000000000001UL >> +#define EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE 0x8000000000000002UL >> +#define EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE 0x8000000000000003UL >> +#define EXCEPT_RISCV_IRQ_4 0x8000000000000004UL >> +#define EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE 0x8000000000000005UL >> +#define EXCEPT_RISCV_MAX_IRQS >> (EXCEPT_RISCV_IRQ_INDEX(EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE)) >> >> There is no difference in terms of processing interrupt or exception > type. It is cleaner if we treat interrupts and exceptions the same. So no > need to detect if an event is interrupt or exception. > > >> typedef struct { >> UINT64 X0; >> @@ -652,6 +675,9 @@ typedef struct { >> UINT64 X29; >> UINT64 X30; >> UINT64 X31; >> + UINT64 SEPC; >> + UINT32 SSTATUS; >> + UINT32 STVAL; >> } EFI_SYSTEM_CONTEXT_RISCV64; >> >> // >> diff --git >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h >> index 30f47e87552b..9b7e1304dd3b 100644 >> --- >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h >> +++ >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h >> @@ -59,7 +59,7 @@ SupervisorModeTrap ( >> #define SMODE_TRAP_REGS_t6 31 >> #define SMODE_TRAP_REGS_sepc 32 >> #define SMODE_TRAP_REGS_sstatus 33 >> -#define SMODE_TRAP_REGS_sie 34 >> +#define SMODE_TRAP_REGS_stval 34 >> #define SMODE_TRAP_REGS_last 35 >> >> #define SMODE_TRAP_REGS_OFFSET(x) ((SMODE_TRAP_REGS_##x) * >> __SIZEOF_POINTER__) >> @@ -68,7 +68,7 @@ SupervisorModeTrap ( >> #pragma pack(1) >> typedef struct { >> // >> - // Below are follow the format of EFI_SYSTEM_CONTEXT >> + // Below follow the format of EFI_SYSTEM_CONTEXT. >> // >> UINT64 zero; >> UINT64 ra; >> @@ -102,14 +102,9 @@ typedef struct { >> UINT64 t4; >> UINT64 t5; >> UINT64 t6; >> - // >> - // Below are the additional information to >> - // EFI_SYSTEM_CONTEXT, private to supervisor mode trap >> - // and not public to EFI environment. >> - // >> UINT64 sepc; >> UINT64 sstatus; >> - UINT64 sie; >> + UINT64 stval; >> } SMODE_TRAP_REGISTERS; >> #pragma pack() >> >> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c >> b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c >> index 50f57a9780f0..ab280d3b2d7d 100644 >> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c >> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c >> @@ -276,7 +276,11 @@ TimerDriverInitialize ( >> // >> // Install interrupt handler for RISC-V Timer. >> // >> - Status = mCpu->RegisterInterruptHandler (mCpu, EXCEPT_RISCV_TIMER_INT, >> TimerInterruptHandler); >> + Status = mCpu->RegisterInterruptHandler ( >> + mCpu, >> + EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE, >> + TimerInterruptHandler >> + ); >> ASSERT_EFI_ERROR (Status); >> >> // >> diff --git >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c >> index f1ee7d236aec..1beaf5ac513e 100644 >> --- >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c >> +++ >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c >> @@ -11,11 +11,162 @@ >> #include <Library/CpuExceptionHandlerLib.h> >> #include <Library/DebugLib.h> >> #include <Library/BaseLib.h> >> +#include <Library/SerialPortLib.h> >> +#include <Library/PrintLib.h> >> #include <Register/RiscV64/RiscVEncoding.h> >> - >> #include "CpuExceptionHandlerLib.h" >> >> -STATIC EFI_CPU_INTERRUPT_HANDLER mInterruptHandlers[2]; >> +// >> +// Define the maximum message length >> +// >> +#define MAX_DEBUG_MESSAGE_LENGTH 0x100 >> + >> +STATIC EFI_CPU_INTERRUPT_HANDLER >> mExceptionHandlers[EXCEPT_RISCV_MAX_EXCEPTIONS + 1]; >> +STATIC EFI_CPU_INTERRUPT_HANDLER mIrqHandlers[EXCEPT_RISCV_MAX_IRQS + >> 1]; >> + >> +STATIC CONST CHAR8 mExceptionOrIrqUnknown[] >> = "Unknown"; >> +STATIC CONST CHAR8 *mExceptionNameStr[EXCEPT_RISCV_MAX_EXCEPTIONS + 1] >> = { >> + "EXCEPT_RISCV_INST_MISALIGNED", >> + "EXCEPT_RISCV_INST_ACCESS_FAULT", >> + "EXCEPT_RISCV_ILLEGAL_INST", >> + "EXCEPT_RISCV_BREAKPOINT", >> + "EXCEPT_RISCV_LOAD_ADDRESS_MISALIGNED", >> + "EXCEPT_RISCV_LOAD_ACCESS_FAULT", >> + "EXCEPT_RISCV_STORE_AMO_ADDRESS_MISALIGNED", >> + "EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT", >> + "EXCEPT_RISCV_ENV_CALL_FROM_UMODE", >> + "EXCEPT_RISCV_ENV_CALL_FROM_SMODE", >> + "EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE", >> + "EXCEPT_RISCV_ENV_CALL_FROM_MMODE", >> + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT", >> + "EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT", >> + "EXCEPT_RISCV_14", >> + "EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT", >> + "EXCEPT_RISCV_16", >> + "EXCEPT_RISCV_17", >> + "EXCEPT_RISCV_18", >> + "EXCEPT_RISCV_19", >> + "EXCEPT_RISCV_INST_GUEST_PAGE_FAULT", >> + "EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT", >> + "EXCEPT_RISCV_VIRTUAL_INSTRUCTION", >> + "EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT" >> +}; >> + >> +STATIC CONST CHAR8 *mIrqNameStr[EXCEPT_RISCV_MAX_IRQS + 1] = { >> + "EXCEPT_RISCV_IRQ_0", >> + "EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE", >> + "EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE", >> + "EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE", >> + "EXCEPT_RISCV_IRQ_4", >> + "EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE", >> +}; >> + >> +/** >> + Prints a message to the serial port. >> + >> + @param Format Format string for the message to print. >> + @param ... Variable argument list whose contents are accessed >> + based on the format string specified by Format. >> + >> +**/ >> +STATIC >> +VOID >> +EFIAPI >> +InternalPrintMessage ( >> + IN CONST CHAR8 *Format, >> + ... >> + ) >> +{ >> + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; >> + VA_LIST Marker; >> + >> + // >> + // Convert the message to an ASCII String >> + // >> + VA_START (Marker, Format); >> + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker); >> + VA_END (Marker); >> + >> + // >> + // Send the print string to a Serial Port >> + // >> + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); >> +} >> + >> +/** >> + Get ASCII format string exception name by exception type. >> + >> + @param ExceptionType Exception type. >> + >> + @return ASCII format string exception name. >> +**/ >> +STATIC >> +CONST CHAR8 * >> +GetExceptionNameStr ( >> + IN EFI_EXCEPTION_TYPE ExceptionType >> + ) >> +{ >> + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { >> + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { >> + return mExceptionOrIrqUnknown; >> + } >> + >> + return mIrqNameStr[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)]; >> + } >> + >> + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { >> + return mExceptionOrIrqUnknown; >> + } >> + >> + return mExceptionNameStr[ExceptionType]; >> +} >> + >> +/** >> + Display CPU information. This can be called by 3rd-party handlers >> + set by RegisterCpuInterruptHandler. >> + >> + @param ExceptionType Exception type. >> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT. >> +**/ >> +VOID >> +EFIAPI >> +DumpCpuContext ( >> + IN EFI_EXCEPTION_TYPE ExceptionType, >> + IN EFI_SYSTEM_CONTEXT SystemContext >> + ) >> +{ >> + UINTN Printed = 0; >> + SMODE_TRAP_REGISTERS *Regs = (SMODE_TRAP_REGISTERS *) >> + SystemContext.SystemContextRiscV64; >> + >> + InternalPrintMessage ( >> + "!!!! RISCV64 Exception Type - %016x(%a) !!!!\n", >> + ExceptionType, >> + GetExceptionNameStr (ExceptionType) >> + ); >> + >> + #define REGS() >> \ >> + REG (t0); REG (t1); REG (t2); REG (t3); REG (t4); REG (t5); REG (t6); \ >> + REG (s0); REG (s1); REG (s2); REG (s3); REG (s4); REG (s5); REG (s6); \ >> + REG (s7); REG (s8); REG (s9); REG (s10); REG (s11); \ >> + REG (a0); REG (a1); REG (a2); REG (a3); REG (a4); REG (a5); REG (a6); \ >> + REG (a7); \ >> + REG (zero); REG (ra); REG (sp); REG (gp); REG (tp); \ >> + REG (sepc); REG (sstatus); REG (stval); >> + >> + #define REG(x) do { \ >> + InternalPrintMessage ("%7a = 0x%017lx%c", #x, Regs->x, \ >> + (++Printed % 2) ? L'\t' : L'\n'); \ >> + } while (0); >> + >> + REGS (); >> + if (Printed % 2) { >> + InternalPrintMessage ("\n"); >> + } >> + >> > I would like to only print registers at DEBUG build only. If you look at > ARM implementation, register and stack > dump are only enabled at DEBUG build. > Printing them all at release version can be at security risk. Anyone > wanting to debug an issue likely builds a debug version. > > + #undef REG >> + #undef REGS >> +} >> >> /** >> Initializes all CPU exceptions entries and provides the default >> exception handlers. >> @@ -47,34 +198,59 @@ InitializeCpuExceptionHandlers ( >> Registers a function to be called from the processor interrupt handler. >> >> This function registers and enables the handler specified by >> InterruptHandler for a processor >> - interrupt or exception type specified by InterruptType. If >> InterruptHandler is NULL, then the >> - handler for the processor interrupt or exception type specified by >> InterruptType is uninstalled. >> + interrupt or exception type specified by ExceptionType. If >> InterruptHandler is NULL, then the >> + handler for the processor interrupt or exception type specified by >> ExceptionType is uninstalled. >> The installed handler is called once for each processor interrupt or >> exception. >> NOTE: This function should be invoked after >> InitializeCpuExceptionHandlers() or >> InitializeCpuInterruptHandlers() invoked, otherwise EFI_UNSUPPORTED >> returned. >> >> - @param[in] InterruptType Defines which interrupt or exception to >> hook. >> + @param[in] ExceptionType Defines which interrupt or exception to >> hook. >> @param[in] InterruptHandler A pointer to a function of type >> EFI_CPU_INTERRUPT_HANDLER that is called >> when a processor interrupt occurs. If >> this parameter is NULL, then the handler >> will be uninstalled. >> >> @retval EFI_SUCCESS The handler for the processor interrupt >> was successfully installed or uninstalled. >> - @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a >> handler for InterruptType was >> + @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a >> handler for ExceptionType was >> previously installed. >> - @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler >> for InterruptType was not >> + @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler >> for ExceptionType was not >> previously installed. >> - @retval EFI_UNSUPPORTED The interrupt specified by InterruptType >> is not supported, >> + @retval EFI_UNSUPPORTED The interrupt specified by ExceptionType >> is not supported, >> or this function is not supported. >> **/ >> EFI_STATUS >> EFIAPI >> RegisterCpuInterruptHandler ( >> - IN EFI_EXCEPTION_TYPE InterruptType, >> + IN EFI_EXCEPTION_TYPE ExceptionType, >> IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler >> ) >> { >> - DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, >> InterruptType, InterruptHandler)); >> - mInterruptHandlers[InterruptType] = InterruptHandler; >> + DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, >> ExceptionType, InterruptHandler)); >> + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { >> + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { >> + return EFI_UNSUPPORTED; >> + } >> + >> + if (mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] != NULL) { >> + return EFI_ALREADY_STARTED; >> + } else if (InterruptHandler == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] = >> InterruptHandler; >> + } else { >> + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { >> + return EFI_UNSUPPORTED; >> + } >> + >> + if (mExceptionHandlers[ExceptionType] != NULL) { >> + return EFI_ALREADY_STARTED; >> + } else if (InterruptHandler == NULL) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + mExceptionHandlers[ExceptionType] = InterruptHandler; >> + } >> + >> > As my first comment, this code can be clearer if the interrupt and > exception are the same. > > return EFI_SUCCESS; >> } >> >> @@ -113,21 +289,31 @@ RiscVSupervisorModeTrapHandler ( >> SMODE_TRAP_REGISTERS *SmodeTrapReg >> ) >> { >> - UINTN SCause; >> + EFI_EXCEPTION_TYPE ExceptionType; >> EFI_SYSTEM_CONTEXT RiscVSystemContext; >> >> RiscVSystemContext.SystemContextRiscV64 = (EFI_SYSTEM_CONTEXT_RISCV64 >> *)SmodeTrapReg; >> - // >> - // Check scasue register. >> - // >> - SCause = (UINTN)RiscVGetSupervisorTrapCause (); >> - if ((SCause & (1UL << (sizeof (UINTN) * 8- 1))) != 0) { >> - // >> - // This is interrupt event. >> - // >> - SCause &= ~(1UL << (sizeof (UINTN) * 8- 1)); >> - if ((SCause == IRQ_S_TIMER) && >> (mInterruptHandlers[EXCEPT_RISCV_TIMER_INT] != NULL)) { >> - mInterruptHandlers[EXCEPT_RISCV_TIMER_INT](EXCEPT_RISCV_TIMER_INT, >> RiscVSystemContext); >> + ExceptionType = >> (UINTN)RiscVGetSupervisorTrapCause (); >> + >> + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { >> + UINTN IrqIndex = EXCEPT_RISCV_IRQ_INDEX (ExceptionType); >> + >> + if ((IrqIndex <= EXCEPT_RISCV_MAX_IRQS) && >> + (mIrqHandlers[IrqIndex] != NULL)) >> + { >> + mIrqHandlers[IrqIndex](ExceptionType, RiscVSystemContext); >> + return; >> } >> + } else { >> + if ((ExceptionType <= EXCEPT_RISCV_MAX_EXCEPTIONS) && >> + (mExceptionHandlers[ExceptionType] != 0)) >> + { >> + mExceptionHandlers[ExceptionType](ExceptionType, >> RiscVSystemContext); >> + return; >> + } >> + } >> + >> + DumpCpuContext (ExceptionType, RiscVSystemContext); >> + while (1) { >> > Would it be better with "do{} while (1)"? > CpuDeadLoop() even better. > } >> } >> diff --git >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S >> index 649c4c5becf4..45070b5cda04 100644 >> --- >> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S >> +++ >> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S >> @@ -20,14 +20,14 @@ SupervisorModeTrap: >> sd t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) >> >> csrr t0, CSR_SSTATUS >> - and t0, t0, (SSTATUS_SIE | SSTATUS_SPIE) >> sd t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) >> csrr t0, CSR_SEPC >> sd t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) >> - csrr t0, CSR_SIE >> - sd t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) >> + csrr t0, CSR_STVAL >> + sd t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) >> ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) >> >> + sd zero, SMODE_TRAP_REGS_OFFSET(zero)(sp) >> sd ra, SMODE_TRAP_REGS_OFFSET(ra)(sp) >> sd gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) >> sd tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) >> @@ -59,6 +59,7 @@ SupervisorModeTrap: >> sd t6, SMODE_TRAP_REGS_OFFSET(t6)(sp) >> >> /* Call to Supervisor mode trap handler in CpuExceptionHandlerLib.c */ >> + mv a0, sp >> call RiscVSupervisorModeTrapHandler >> >> /* Restore all general regisers except SP */ >> @@ -66,6 +67,7 @@ SupervisorModeTrap: >> ld gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) >> ld tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) >> ld t2, SMODE_TRAP_REGS_OFFSET(t2)(sp) >> + ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) >> ld s0, SMODE_TRAP_REGS_OFFSET(s0)(sp) >> ld s1, SMODE_TRAP_REGS_OFFSET(s1)(sp) >> ld a0, SMODE_TRAP_REGS_OFFSET(a0)(sp) >> @@ -93,13 +95,10 @@ SupervisorModeTrap: >> >> ld t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) >> csrw CSR_SEPC, t0 >> - ld t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) >> - csrw CSR_SIE, t0 >> - csrr t0, CSR_SSTATUS >> - ld t1, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) >> - or t0, t0, t1 >> + ld t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) >> csrw CSR_SSTATUS, t0 >> - ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) >> + ld t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) >> + csrw CSR_STVAL, t0 >> ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) >> addi sp, sp, SMODE_TRAP_REGS_SIZE >> sret >> -- >> 2.25.1 >> >> >> >> >> >> >> > > [-- Attachment #2: Type: text/html, Size: 24797 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling 2023-03-02 5:17 ` Tuan Phan @ 2023-03-02 5:26 ` Andrei Warkentin 0 siblings, 0 replies; 15+ messages in thread From: Andrei Warkentin @ 2023-03-02 5:26 UTC (permalink / raw) To: Tuan Phan, devel@edk2.groups.io; +Cc: Sunil V L, Daniel Schaefer [-- Attachment #1: Type: text/plain, Size: 20407 bytes --] Hi, Thanks for your review. In principle you’re right about exceptions/interrupts not being different… but in practice their numerical values are disjoint, which makes handling them the same way be awkward (not impossible, but let’s not do a linear search on exception entry!). Interrupts are distinguished by having bit 63 set (on RV64)… A From: Tuan Phan <tphan@ventanamicro.com> Sent: Wednesday, March 1, 2023 11:18 PM To: devel@edk2.groups.io; tphan@ventanamicro.com Cc: Warkentin, Andrei <andrei.warkentin@intel.com>; Sunil V L <sunilvl@ventanamicro.com>; Daniel Schaefer <git@danielschaefer.me> Subject: Re: [edk2-devel] [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling On Wed, Mar 1, 2023 at 8:59 PM Tuan Phan via groups.io<http://groups.io> <tphan=ventanamicro.com@groups.io<mailto:ventanamicro.com@groups.io>> wrote: On Thu, Feb 23, 2023 at 3:55 PM Andrei Warkentin <andrei.warkentin@intel.com<mailto:andrei.warkentin@intel.com>> wrote: RegisterCpuInterruptHandler did not allow setting exception handlers for anything beyond the timer IRQ. Beyond that, it didn't meet the spec around handling of inputs. RiscVSupervisorModeTrapHandler now will invoke set handlers for both exceptions and interrupts. Two arrays of handlers are maintained - one for exceptions and one for interrupts. For unhandled traps, RiscVSupervisorModeTrapHandler dumps state using the now implemented DumpCpuContext. For EFI_SYSTEM_CONTEXT_RISCV64, extend this with the trapped PC address (SEPC), just like on AArch64 (ELR). This is necessary for X86EmulatorPkg to work as it allows a trap handler to return execution to a different place. Add SSTATUS/STVAL as well, at least for debugging purposes. There is no value in hiding this. Fix nested exception handling. Handler code should not be saving SIE (the value is saved in SSTATUS.SPIE) or directly restored (that's done by SRET). Save and restore the entire SSTATUS and STVAL, too. Cc: Sunil V L <sunilvl@ventanamicro.com<mailto:sunilvl@ventanamicro.com>> Cc: Daniel Schaefer <git@danielschaefer.me<mailto:git@danielschaefer.me>> Signed-off-by: Andrei Warkentin <andrei.warkentin@intel.com<mailto:andrei.warkentin@intel.com>> --- MdePkg/Include/Protocol/DebugSupport.h | 32 ++- .../CpuExceptionHandlerLib.h | 11 +- UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 6 +- .../CpuExceptionHandlerLib.c | 232 ++++++++++++++++-- .../SupervisorTrapHandler.S | 17 +- 5 files changed, 254 insertions(+), 44 deletions(-) diff --git a/MdePkg/Include/Protocol/DebugSupport.h b/MdePkg/Include/Protocol/DebugSupport.h index 2b0ae2d1577b..9742663619c5 100644 --- a/MdePkg/Include/Protocol/DebugSupport.h +++ b/MdePkg/Include/Protocol/DebugSupport.h @@ -613,11 +613,34 @@ typedef struct { #define EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT 7 #define EXCEPT_RISCV_ENV_CALL_FROM_UMODE 8 #define EXCEPT_RISCV_ENV_CALL_FROM_SMODE 9 -#define EXCEPT_RISCV_ENV_CALL_FROM_HMODE 10 +#define EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE 10 #define EXCEPT_RISCV_ENV_CALL_FROM_MMODE 11 +#define EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT 12 +#define EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT 13 +#define EXCEPT_RISCV_14 14 +#define EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT 15 +#define EXCEPT_RISCV_16 16 +#define EXCEPT_RISCV_17 17 +#define EXCEPT_RISCV_18 18 +#define EXCEPT_RISCV_19 19 +#define EXCEPT_RISCV_INST_GUEST_PAGE_FAULT 20 +#define EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT 21 +#define EXCEPT_RISCV_VIRTUAL_INSTRUCTION 22 +#define EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT 23 +#define EXCEPT_RISCV_MAX_EXCEPTIONS (EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT) -#define EXCEPT_RISCV_SOFTWARE_INT 0x0 -#define EXCEPT_RISCV_TIMER_INT 0x1 +/// +/// RISC-V processor exception types for interrupts. +/// +#define EXCEPT_RISCV_IS_IRQ(x) ((x & 0x8000000000000000UL) != 0) +#define EXCEPT_RISCV_IRQ_INDEX(x) (x & 0x7FFFFFFFFFFFFFFFUL) +#define EXCEPT_RISCV_IRQ_0 0x8000000000000000UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE 0x8000000000000001UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE 0x8000000000000002UL +#define EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE 0x8000000000000003UL +#define EXCEPT_RISCV_IRQ_4 0x8000000000000004UL +#define EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE 0x8000000000000005UL +#define EXCEPT_RISCV_MAX_IRQS (EXCEPT_RISCV_IRQ_INDEX(EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE)) There is no difference in terms of processing interrupt or exception type. It is cleaner if we treat interrupts and exceptions the same. So no need to detect if an event is interrupt or exception. typedef struct { UINT64 X0; @@ -652,6 +675,9 @@ typedef struct { UINT64 X29; UINT64 X30; UINT64 X31; + UINT64 SEPC; + UINT32 SSTATUS; + UINT32 STVAL; } EFI_SYSTEM_CONTEXT_RISCV64; // diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h index 30f47e87552b..9b7e1304dd3b 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h @@ -59,7 +59,7 @@ SupervisorModeTrap ( #define SMODE_TRAP_REGS_t6 31 #define SMODE_TRAP_REGS_sepc 32 #define SMODE_TRAP_REGS_sstatus 33 -#define SMODE_TRAP_REGS_sie 34 +#define SMODE_TRAP_REGS_stval 34 #define SMODE_TRAP_REGS_last 35 #define SMODE_TRAP_REGS_OFFSET(x) ((SMODE_TRAP_REGS_##x) * __SIZEOF_POINTER__) @@ -68,7 +68,7 @@ SupervisorModeTrap ( #pragma pack(1) typedef struct { // - // Below are follow the format of EFI_SYSTEM_CONTEXT + // Below follow the format of EFI_SYSTEM_CONTEXT. // UINT64 zero; UINT64 ra; @@ -102,14 +102,9 @@ typedef struct { UINT64 t4; UINT64 t5; UINT64 t6; - // - // Below are the additional information to - // EFI_SYSTEM_CONTEXT, private to supervisor mode trap - // and not public to EFI environment. - // UINT64 sepc; UINT64 sstatus; - UINT64 sie; + UINT64 stval; } SMODE_TRAP_REGISTERS; #pragma pack() diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c index 50f57a9780f0..ab280d3b2d7d 100644 --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c @@ -276,7 +276,11 @@ TimerDriverInitialize ( // // Install interrupt handler for RISC-V Timer. // - Status = mCpu->RegisterInterruptHandler (mCpu, EXCEPT_RISCV_TIMER_INT, TimerInterruptHandler); + Status = mCpu->RegisterInterruptHandler ( + mCpu, + EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE, + TimerInterruptHandler + ); ASSERT_EFI_ERROR (Status); // diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c index f1ee7d236aec..1beaf5ac513e 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c @@ -11,11 +11,162 @@ #include <Library/CpuExceptionHandlerLib.h> #include <Library/DebugLib.h> #include <Library/BaseLib.h> +#include <Library/SerialPortLib.h> +#include <Library/PrintLib.h> #include <Register/RiscV64/RiscVEncoding.h> - #include "CpuExceptionHandlerLib.h" -STATIC EFI_CPU_INTERRUPT_HANDLER mInterruptHandlers[2]; +// +// Define the maximum message length +// +#define MAX_DEBUG_MESSAGE_LENGTH 0x100 + +STATIC EFI_CPU_INTERRUPT_HANDLER mExceptionHandlers[EXCEPT_RISCV_MAX_EXCEPTIONS + 1]; +STATIC EFI_CPU_INTERRUPT_HANDLER mIrqHandlers[EXCEPT_RISCV_MAX_IRQS + 1]; + +STATIC CONST CHAR8 mExceptionOrIrqUnknown[] = "Unknown"; +STATIC CONST CHAR8 *mExceptionNameStr[EXCEPT_RISCV_MAX_EXCEPTIONS + 1] = { + "EXCEPT_RISCV_INST_MISALIGNED", + "EXCEPT_RISCV_INST_ACCESS_FAULT", + "EXCEPT_RISCV_ILLEGAL_INST", + "EXCEPT_RISCV_BREAKPOINT", + "EXCEPT_RISCV_LOAD_ADDRESS_MISALIGNED", + "EXCEPT_RISCV_LOAD_ACCESS_FAULT", + "EXCEPT_RISCV_STORE_AMO_ADDRESS_MISALIGNED", + "EXCEPT_RISCV_STORE_AMO_ACCESS_FAULT", + "EXCEPT_RISCV_ENV_CALL_FROM_UMODE", + "EXCEPT_RISCV_ENV_CALL_FROM_SMODE", + "EXCEPT_RISCV_ENV_CALL_FROM_VS_MODE", + "EXCEPT_RISCV_ENV_CALL_FROM_MMODE", + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_14", + "EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT", + "EXCEPT_RISCV_16", + "EXCEPT_RISCV_17", + "EXCEPT_RISCV_18", + "EXCEPT_RISCV_19", + "EXCEPT_RISCV_INST_GUEST_PAGE_FAULT", + "EXCEPT_RISCV_LOAD_GUEST_PAGE_FAULT", + "EXCEPT_RISCV_VIRTUAL_INSTRUCTION", + "EXCEPT_RISCV_STORE_GUEST_PAGE_FAULT" +}; + +STATIC CONST CHAR8 *mIrqNameStr[EXCEPT_RISCV_MAX_IRQS + 1] = { + "EXCEPT_RISCV_IRQ_0", + "EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE", + "EXCEPT_RISCV_IRQ_SOFT_FROM_VSMODE", + "EXCEPT_RISCV_IRQ_SOFT_FROM_MMODE", + "EXCEPT_RISCV_IRQ_4", + "EXCEPT_RISCV_IRQ_TIMER_FROM_SMODE", +}; + +/** + Prints a message to the serial port. + + @param Format Format string for the message to print. + @param ... Variable argument list whose contents are accessed + based on the format string specified by Format. + +**/ +STATIC +VOID +EFIAPI +InternalPrintMessage ( + IN CONST CHAR8 *Format, + ... + ) +{ + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH]; + VA_LIST Marker; + + // + // Convert the message to an ASCII String + // + VA_START (Marker, Format); + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker); + VA_END (Marker); + + // + // Send the print string to a Serial Port + // + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer)); +} + +/** + Get ASCII format string exception name by exception type. + + @param ExceptionType Exception type. + + @return ASCII format string exception name. +**/ +STATIC +CONST CHAR8 * +GetExceptionNameStr ( + IN EFI_EXCEPTION_TYPE ExceptionType + ) +{ + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { + return mExceptionOrIrqUnknown; + } + + return mIrqNameStr[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)]; + } + + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { + return mExceptionOrIrqUnknown; + } + + return mExceptionNameStr[ExceptionType]; +} + +/** + Display CPU information. This can be called by 3rd-party handlers + set by RegisterCpuInterruptHandler. + + @param ExceptionType Exception type. + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT. +**/ +VOID +EFIAPI +DumpCpuContext ( + IN EFI_EXCEPTION_TYPE ExceptionType, + IN EFI_SYSTEM_CONTEXT SystemContext + ) +{ + UINTN Printed = 0; + SMODE_TRAP_REGISTERS *Regs = (SMODE_TRAP_REGISTERS *) + SystemContext.SystemContextRiscV64; + + InternalPrintMessage ( + "!!!! RISCV64 Exception Type - %016x(%a) !!!!\n", + ExceptionType, + GetExceptionNameStr (ExceptionType) + ); + + #define REGS() \ + REG (t0); REG (t1); REG (t2); REG (t3); REG (t4); REG (t5); REG (t6); \ + REG (s0); REG (s1); REG (s2); REG (s3); REG (s4); REG (s5); REG (s6); \ + REG (s7); REG (s8); REG (s9); REG (s10); REG (s11); \ + REG (a0); REG (a1); REG (a2); REG (a3); REG (a4); REG (a5); REG (a6); \ + REG (a7); \ + REG (zero); REG (ra); REG (sp); REG (gp); REG (tp); \ + REG (sepc); REG (sstatus); REG (stval); + + #define REG(x) do { \ + InternalPrintMessage ("%7a = 0x%017lx%c", #x, Regs->x, \ + (++Printed % 2) ? L'\t' : L'\n'); \ + } while (0); + + REGS (); + if (Printed % 2) { + InternalPrintMessage ("\n"); + } + I would like to only print registers at DEBUG build only. If you look at ARM implementation, register and stack dump are only enabled at DEBUG build. Printing them all at release version can be at security risk. Anyone wanting to debug an issue likely builds a debug version. + #undef REG + #undef REGS +} /** Initializes all CPU exceptions entries and provides the default exception handlers. @@ -47,34 +198,59 @@ InitializeCpuExceptionHandlers ( Registers a function to be called from the processor interrupt handler. This function registers and enables the handler specified by InterruptHandler for a processor - interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the - handler for the processor interrupt or exception type specified by InterruptType is uninstalled. + interrupt or exception type specified by ExceptionType. If InterruptHandler is NULL, then the + handler for the processor interrupt or exception type specified by ExceptionType is uninstalled. The installed handler is called once for each processor interrupt or exception. NOTE: This function should be invoked after InitializeCpuExceptionHandlers() or InitializeCpuInterruptHandlers() invoked, otherwise EFI_UNSUPPORTED returned. - @param[in] InterruptType Defines which interrupt or exception to hook. + @param[in] ExceptionType Defines which interrupt or exception to hook. @param[in] InterruptHandler A pointer to a function of type EFI_CPU_INTERRUPT_HANDLER that is called when a processor interrupt occurs. If this parameter is NULL, then the handler will be uninstalled. @retval EFI_SUCCESS The handler for the processor interrupt was successfully installed or uninstalled. - @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a handler for InterruptType was + @retval EFI_ALREADY_STARTED InterruptHandler is not NULL, and a handler for ExceptionType was previously installed. - @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for InterruptType was not + @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for ExceptionType was not previously installed. - @retval EFI_UNSUPPORTED The interrupt specified by InterruptType is not supported, + @retval EFI_UNSUPPORTED The interrupt specified by ExceptionType is not supported, or this function is not supported. **/ EFI_STATUS EFIAPI RegisterCpuInterruptHandler ( - IN EFI_EXCEPTION_TYPE InterruptType, + IN EFI_EXCEPTION_TYPE ExceptionType, IN EFI_CPU_INTERRUPT_HANDLER InterruptHandler ) { - DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, InterruptType, InterruptHandler)); - mInterruptHandlers[InterruptType] = InterruptHandler; + DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, ExceptionType, InterruptHandler)); + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + if (EXCEPT_RISCV_IRQ_INDEX (ExceptionType) > EXCEPT_RISCV_MAX_IRQS) { + return EFI_UNSUPPORTED; + } + + if (mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] != NULL) { + return EFI_ALREADY_STARTED; + } else if (InterruptHandler == NULL) { + return EFI_INVALID_PARAMETER; + } + + mIrqHandlers[EXCEPT_RISCV_IRQ_INDEX (ExceptionType)] = InterruptHandler; + } else { + if (ExceptionType > EXCEPT_RISCV_MAX_EXCEPTIONS) { + return EFI_UNSUPPORTED; + } + + if (mExceptionHandlers[ExceptionType] != NULL) { + return EFI_ALREADY_STARTED; + } else if (InterruptHandler == NULL) { + return EFI_INVALID_PARAMETER; + } + + mExceptionHandlers[ExceptionType] = InterruptHandler; + } + As my first comment, this code can be clearer if the interrupt and exception are the same. return EFI_SUCCESS; } @@ -113,21 +289,31 @@ RiscVSupervisorModeTrapHandler ( SMODE_TRAP_REGISTERS *SmodeTrapReg ) { - UINTN SCause; + EFI_EXCEPTION_TYPE ExceptionType; EFI_SYSTEM_CONTEXT RiscVSystemContext; RiscVSystemContext.SystemContextRiscV64 = (EFI_SYSTEM_CONTEXT_RISCV64 *)SmodeTrapReg; - // - // Check scasue register. - // - SCause = (UINTN)RiscVGetSupervisorTrapCause (); - if ((SCause & (1UL << (sizeof (UINTN) * 8- 1))) != 0) { - // - // This is interrupt event. - // - SCause &= ~(1UL << (sizeof (UINTN) * 8- 1)); - if ((SCause == IRQ_S_TIMER) && (mInterruptHandlers[EXCEPT_RISCV_TIMER_INT] != NULL)) { - mInterruptHandlers[EXCEPT_RISCV_TIMER_INT](EXCEPT_RISCV_TIMER_INT, RiscVSystemContext); + ExceptionType = (UINTN)RiscVGetSupervisorTrapCause (); + + if (EXCEPT_RISCV_IS_IRQ (ExceptionType)) { + UINTN IrqIndex = EXCEPT_RISCV_IRQ_INDEX (ExceptionType); + + if ((IrqIndex <= EXCEPT_RISCV_MAX_IRQS) && + (mIrqHandlers[IrqIndex] != NULL)) + { + mIrqHandlers[IrqIndex](ExceptionType, RiscVSystemContext); + return; } + } else { + if ((ExceptionType <= EXCEPT_RISCV_MAX_EXCEPTIONS) && + (mExceptionHandlers[ExceptionType] != 0)) + { + mExceptionHandlers[ExceptionType](ExceptionType, RiscVSystemContext); + return; + } + } + + DumpCpuContext (ExceptionType, RiscVSystemContext); + while (1) { Would it be better with "do{} while (1)"? CpuDeadLoop() even better. } } diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S index 649c4c5becf4..45070b5cda04 100644 --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.S @@ -20,14 +20,14 @@ SupervisorModeTrap: sd t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) csrr t0, CSR_SSTATUS - and t0, t0, (SSTATUS_SIE | SSTATUS_SPIE) sd t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) csrr t0, CSR_SEPC sd t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) - csrr t0, CSR_SIE - sd t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) + csrr t0, CSR_STVAL + sd t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) + sd zero, SMODE_TRAP_REGS_OFFSET(zero)(sp) sd ra, SMODE_TRAP_REGS_OFFSET(ra)(sp) sd gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) sd tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) @@ -59,6 +59,7 @@ SupervisorModeTrap: sd t6, SMODE_TRAP_REGS_OFFSET(t6)(sp) /* Call to Supervisor mode trap handler in CpuExceptionHandlerLib.c */ + mv a0, sp call RiscVSupervisorModeTrapHandler /* Restore all general regisers except SP */ @@ -66,6 +67,7 @@ SupervisorModeTrap: ld gp, SMODE_TRAP_REGS_OFFSET(gp)(sp) ld tp, SMODE_TRAP_REGS_OFFSET(tp)(sp) ld t2, SMODE_TRAP_REGS_OFFSET(t2)(sp) + ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) ld s0, SMODE_TRAP_REGS_OFFSET(s0)(sp) ld s1, SMODE_TRAP_REGS_OFFSET(s1)(sp) ld a0, SMODE_TRAP_REGS_OFFSET(a0)(sp) @@ -93,13 +95,10 @@ SupervisorModeTrap: ld t0, SMODE_TRAP_REGS_OFFSET(sepc)(sp) csrw CSR_SEPC, t0 - ld t0, SMODE_TRAP_REGS_OFFSET(sie)(sp) - csrw CSR_SIE, t0 - csrr t0, CSR_SSTATUS - ld t1, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) - or t0, t0, t1 + ld t0, SMODE_TRAP_REGS_OFFSET(sstatus)(sp) csrw CSR_SSTATUS, t0 - ld t1, SMODE_TRAP_REGS_OFFSET(t1)(sp) + ld t0, SMODE_TRAP_REGS_OFFSET(stval)(sp) + csrw CSR_STVAL, t0 ld t0, SMODE_TRAP_REGS_OFFSET(t0)(sp) addi sp, sp, SMODE_TRAP_REGS_SIZE sret -- 2.25.1 [-- Attachment #2: Type: text/html, Size: 33299 bytes --] ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-03-02 5:26 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-23 23:54 [PATCH 0/7] Assorted fixes to core RISC-V and RiscVVirt Andrei Warkentin
2023-02-23 23:54 ` [PATCH 1/7] OvmfPkg: RiscVVirt: add SATA support Andrei Warkentin
2023-02-24 10:00 ` [edk2-devel] " Sunil V L
2023-02-23 23:54 ` [PATCH 2/7] MdePkg: BasePeCoffLib: Allow AArch64 and x64 images in ImageFormatSupported Andrei Warkentin
2023-02-24 10:02 ` Sunil V L
2023-02-23 23:54 ` [PATCH 3/7] MdePkg: BaseLib: don't log in RISCV InternalSwitchStack Andrei Warkentin
2023-02-23 23:54 ` [PATCH 4/7] MdePkg: BaseCpuLib: Fix RISCV CpuSleep symbol name Andrei Warkentin
2023-02-23 23:54 ` [PATCH 5/7] MdeModulePkg: Dxe: add RISCV64 to mMachineTypeInfo Andrei Warkentin
2023-02-23 23:54 ` [PATCH 6/7] [PATCH v2] UefiCpuPkg: CpuTimerDxeRiscV64: fix tick duration accounting Andrei Warkentin
2023-02-24 10:45 ` Sunil V L
2023-02-23 23:54 ` [PATCH 7/7] [PATCH v2] UefiCpuPkg: BaseRiscV64CpuExceptionHandlerLib: clean up exception handling Andrei Warkentin
2023-02-24 10:44 ` Sunil V L
2023-03-02 4:59 ` [edk2-devel] " Tuan Phan
[not found] ` <17488173C4DD581F.9697@groups.io>
2023-03-02 5:17 ` Tuan Phan
2023-03-02 5:26 ` Andrei Warkentin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox