From: "Sunil V L" <sunilvl@ventanamicro.com>
To: Andrei Warkentin <andrei.warkentin@intel.com>
Cc: devel@edk2.groups.io, Daniel Schaefer <git@danielschaefer.me>
Subject: Re: [edk2 1/1] RISCV: clean up exception handling
Date: Tue, 21 Feb 2023 12:31:31 +0530 [thread overview]
Message-ID: <Y/RsS9KOkjAvt6oE@sunil-laptop> (raw)
In-Reply-To: <20230218042809.851-1-andrei.warkentin@intel.com>
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/CpuExceptionHandlerLib.c | 215 +++++++++++++++++---
> UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/SupervisorTrapHandler.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/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 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/CpuExceptionHandlerLib.c b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c
> index f1ee7d236aec..7e8be41cfb9b 100644
> --- a/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscV64CpuExceptionHandlerLib/CpuExceptionHandlerLib.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
next prev parent reply other threads:[~2023-02-21 7:01 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-18 4:28 [edk2 1/1] RISCV: clean up exception handling Andrei Warkentin
2023-02-21 7:01 ` Sunil V L [this message]
2023-02-22 7:54 ` [edk2-devel] " Andrei Warkentin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y/RsS9KOkjAvt6oE@sunil-laptop \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox