From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f179.google.com (mail-pf1-f179.google.com [209.85.210.179]) by mx.groups.io with SMTP id smtpd.web11.37161.1676962897428580833 for ; Mon, 20 Feb 2023 23:01:37 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=W5Y7ue3p; spf=pass (domain: ventanamicro.com, ip: 209.85.210.179, mailfrom: sunilvl@ventanamicro.com) Received: by mail-pf1-f179.google.com with SMTP id a26so1883141pfo.9 for ; Mon, 20 Feb 2023 23:01:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=6q0oYp/MPPRxnYyp1yQ/fZorHVRUDUDPDef74xkQZak=; b=W5Y7ue3p01cSArC6F5V6x4ii64jNvDHOAIBIVfvj0u4LhcFxz34TXs1XzT04KwitKZ 45OgzELIjs9G3ialnktyCg7Z5kfGM28XhXrdyDrf+b+DakWk0iaEhJK2LRfCxQsVcPlY fhJjGhLSNa44JGCoZZCTeGAhBjH1s3BAABq8RaVYD8dgwgJXgOGMx85/laWNWPAa2Ulq qYPm2HtKzQbAfgpEuz1rB3D4dzITFatbiahhc/NGN6Nc6FLWlgi6R3YD19ffEtkeYDDZ 08CplvyGduAalMt7+W/V9uSu/R8bcOz2j/5OS0CL7mn0JsU6qzXGmdur60nRz2HNZk+V v5Ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=6q0oYp/MPPRxnYyp1yQ/fZorHVRUDUDPDef74xkQZak=; b=ACyr2J97Lvm0RDJHfsVhrG4pWCE7L13xLHtntthlaU123g96cwjPNr/wWrpomWCOrR 2KFYoxtJz0gRhjGN5aijupOk+8oRoTFDrvpwoSfQ+A7v3ao4bVxn0cTMU2iGl7PbCXmQ dXRdwNNYaXNjlO05YIXwDzHf5/pRdu5dy7qxwtmNzmHAf6KOeg51LC5VV11SJpS3+QMP ybfSj53adx9w2uZZvFVPhBXV1G/g3fwjoDnmj3dQUsQAVaQb2cD1/Bzp4+otefUS8qB9 /dH82tz39ZdAUK71Z8om/8YM0XayuN1nZvM1fjNmtIfZ6rFbvV5Ko+AV14au2KvhUAtR lJMw== X-Gm-Message-State: AO0yUKUf1fMCIiaN8W2jbk7pHw+iGfCfP1shNgmj01Jewgn8EqudP2jR UO/FuTkmZJPOyPdAxyoe6AWIlQ== X-Google-Smtp-Source: AK7set896lZxJRMh5eEm76ise5jzy57CS+sHFzuCGDjChk8mmEibiPTZ3HOAni+XMry80t2WPkVLfw== X-Received: by 2002:a62:1dd0:0:b0:5aa:6125:dbf4 with SMTP id d199-20020a621dd0000000b005aa6125dbf4mr2696844pfd.11.1676962896726; Mon, 20 Feb 2023 23:01:36 -0800 (PST) Return-Path: Received: from sunil-laptop ([49.206.14.226]) by smtp.gmail.com with ESMTPSA id y21-20020aa78055000000b005a83129caeasm1171531pfm.185.2023.02.20.23.01.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Feb 2023 23:01:36 -0800 (PST) Date: Tue, 21 Feb 2023 12:31:31 +0530 From: "Sunil V L" To: Andrei Warkentin Cc: devel@edk2.groups.io, Daniel Schaefer Subject: Re: [edk2 1/1] RISCV: clean up exception handling Message-ID: References: <20230218042809.851-1-andrei.warkentin@intel.com> MIME-Version: 1.0 In-Reply-To: <20230218042809.851-1-andrei.warkentin@intel.com> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > Cc: Daniel Schaefer > Signed-off-by: Andrei Warkentin > --- > 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 > #include > #include > +#include > +#include > #include > - > #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