[edk2-devel] [edk2 1/1] RISCV: clean up exception handling
Andrei Warkentin
andrei.warkentin at intel.com
Wed Feb 22 07:54:03 UTC 2023
Hi Sunil,
Thank you for the thorough review!
- Thanks for catching EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT not being 15
- The EXCEPT_RISCV_XXX defines are needed for other drivers to pass as EFI_EXCEPTION_TYPE to RegisterCpuInterruptHandler. For example, this is used by our (soon to be made available) port of the X86EmulatorPkg to RISC-V.
- RegisterCpuInterruptHandler should be a STATIC function
- DumpCpuContext is not a STATIC function - it's actually declared in MdeModulePkg/Include/Library/CpuExceptionHandlerLib.h and is for use in exception handlers set by RegisterCpuInterruptHandler.
Will resend shortly.
A
-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Sunil V L
Sent: Tuesday, February 21, 2023 1:02 AM
To: Warkentin, Andrei <andrei.warkentin at intel.com>
Cc: devel at edk2.groups.io; Daniel Schaefer <git at danielschaefer.me>
Subject: Re: [edk2-devel] [edk2 1/1] RISCV: clean up exception handling
Hi Andrei,
Happy to see this patch!
On Fri, Feb 17, 2023 at 10:28:09PM -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 at ventanamicro.com>
> Cc: Daniel Schaefer <git at danielschaefer.me>
> Signed-off-by: Andrei Warkentin <andrei.warkentin at intel.com>
> ---
> MdePkg/Include/Protocol/DebugSupport.h | 23 ++-
> UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.h | 11 +-
> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 3 +-
>
> UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandl
> erLib.c | 215 +++++++++++++++++---
> UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHan
> dler.S | 17 +-
> 5 files changed, 225 insertions(+), 44 deletions(-)
>
> diff --git a/MdePkg/Include/Protocol/DebugSupport.h
> b/MdePkg/Include/Protocol/DebugSupport.h
> index 2b0ae2d1577b..0d94c409448c 100644
> --- a/MdePkg/Include/Protocol/DebugSupport.h
> +++ b/MdePkg/Include/Protocol/DebugSupport.h
> @@ -613,11 +613,25 @@ 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_10 10
Why do we need this change? mExceptionNameStr has a string. Also, should we name these with exact words from the spec like VSMODE?
> #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_STORE_ACCESS_PAGE_FAULT 14
Shouldn't this be 15?
> +#define EXCEPT_RISCV_MAX_EXCEPTIONS (EXCEPT_RISCV_STORE_ACCESS_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)
Please run uncrustify.
> +#define EXCEPT_RISCV_IRQ_0 0x8000000000000000UL
> +#define EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE 0x8000000000000001UL
> +#define EXCEPT_RISCV_IRQ_2 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 +666,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/CpuExceptionHan
> dlerLib.h
> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHan
> dlerLib.h index 30f47e87552b..9b7e1304dd3b 100644
> ---
> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHan
> dlerLib.h
> +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptio
> +++ nHandlerLib.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 db153f715e60..0ecefddf1f18 100644
> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
> @@ -271,7 +271,8 @@ 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/CpuExceptionHan
> dlerLib.c
> b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHan
> dlerLib.c index f1ee7d236aec..7e8be41cfb9b 100644
> ---
> a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHan
> dlerLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptio
> +++ nHandlerLib.c
> @@ -11,11 +11,151 @@
> #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_HMODE",
> + "EXCEPT_RISCV_ENV_CALL_FROM_MMODE",
> + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT",
> + "EXCEPT_RISCV_INST_ACCESS_PAGE_FAULT",
> + "EXCEPT_RISCV_STORE_ACCESS_PAGE_FAULT"
> +};
> +
> +STATIC CONST CHAR8 *mIrqNameStr[EXCEPT_RISCV_MAX_IRQS + 1] = {
> + "EXCEPT_RISCV_IRQ_0",
> + "EXCEPT_RISCV_IRQ_SOFT_FROM_SMODE",
> + "EXCEPT_RISCV_IRQ_2",
> + "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.
> +
> +**/
> +VOID
> +EFIAPI
> +InternalPrintMessage (
> + IN CONST CHAR8 *Format,
> + ...
> + )
Should this be a static function?
> +{
> + 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.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +**/
> +VOID
> +EFIAPI
> +DumpCpuContext (
Should this be a static function?
Thanks,
Sunil
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100436): https://edk2.groups.io/g/devel/message/100436
Mute This Topic: https://groups.io/mt/97044152/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list