Re: [edk2-devel] [edk2 1/1] RISCV: clean up exception handling

["Andrei Warkentin" <andrei.warkentin@intel.com>]

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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunil V L
Sent: Tuesday, February 21, 2023 1:02 AM
To: Warkentin, Andrei <andrei.warkentin@intel.com>
Cc: devel@edk2.groups.io; Daniel Schaefer <git@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@ventanamicro.com>
> Cc: Daniel Schaefer <git@danielschaefer.me>
> Signed-off-by: Andrei Warkentin <andrei.warkentin@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