Re: [edk2-devel] [RFC PATCH V2 10/19] UefiCpuPkg: Add RISC-V support in DxeCpuExceptionHandlerLib

["Ni, Ray" <ray.ni@intel.com>]

Sunil,
Is there any source code sharing between Risc-V and IA32X64?
>From the patch, I cannot see any.
If that's the case, I would prefer we don't mix the two separate implementations together into one component.
Can you please create a new CpuExceptionHandlerLib instance for Risc-V instead of including the Risc-V sources into the existing INF?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sunil V L
> Sent: Wednesday, September 7, 2022 7:36 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; De, Debkumar <debkumar.de@intel.com>; West,
> Catharine <catharine.west@intel.com>; Daniel Schaefer <git@danielschaefer.me>; Abner Chang
> <Abner.Chang@amd.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> <ardb@kernel.org>; Heinrich Schuchardt <heinrich.schuchardt@canonical.com>; Anup Patel <apatel@ventanamicro.com>;
> Sunil V L <sunilvl@ventanamicro.com>
> Subject: [edk2-devel] [RFC PATCH V2 10/19] UefiCpuPkg: Add RISC-V support in DxeCpuExceptionHandlerLib
> 
> Add low level interfaces to register exception and interrupt handlers
> for RISC-V.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf    |   7 +-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.h | 112 ++++++++++++++++
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.c | 136 ++++++++++++++++++++
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/SupervisorTrapHandler.S  | 105 +++++++++++++++
>  4 files changed, 359 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> index bf33d54bae13..8570b83e8afb 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> @@ -18,7 +18,7 @@ [Defines]
>  #
>  # The following information is for reference only and not required by the build tools.
>  #
> -#  VALID_ARCHITECTURES           = IA32 X64
> +#  VALID_ARCHITECTURES           = IA32 X64 RISCV64
>  #
> 
>  [Sources.Ia32]
> @@ -38,6 +38,11 @@ [Sources.IA32, Sources.X64]
>    PeiDxeSmmCpuException.c
>    DxeException.c
> 
> +[Sources.RISCV64]
> +  RiscV64/SupervisorTrapHandler.S
> +  RiscV64/CpuExceptionHandlerLib.c
> +  RiscV64/CpuExceptionHandlerLib.h
> +
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.h
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.h
> new file mode 100644
> index 000000000000..1cc6dbe73420
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.h
> @@ -0,0 +1,112 @@
> +/** @file
> +
> +  RISC-V Exception Handler library definition file.
> +
> +  Copyright (c) 2019-2022, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef RISCV_CPU_EXECPTION_HANDLER_LIB_H_
> +#define RISCV_CPU_EXECPTION_HANDLER_LIB_H_
> +
> +#include <Register/RiscV64/RiscVAsm.h>
> +#include <Register/RiscV64/RiscVImpl.h>
> +extern void
> +SupervisorModeTrap (
> +  void
> +  );
> +
> +//
> +// Index of SMode trap register
> +//
> +#define SMODE_TRAP_REGS_zero     0
> +#define SMODE_TRAP_REGS_ra       1
> +#define SMODE_TRAP_REGS_sp       2
> +#define SMODE_TRAP_REGS_gp       3
> +#define SMODE_TRAP_REGS_tp       4
> +#define SMODE_TRAP_REGS_t0       5
> +#define SMODE_TRAP_REGS_t1       6
> +#define SMODE_TRAP_REGS_t2       7
> +#define SMODE_TRAP_REGS_s0       8
> +#define SMODE_TRAP_REGS_s1       9
> +#define SMODE_TRAP_REGS_a0       10
> +#define SMODE_TRAP_REGS_a1       11
> +#define SMODE_TRAP_REGS_a2       12
> +#define SMODE_TRAP_REGS_a3       13
> +#define SMODE_TRAP_REGS_a4       14
> +#define SMODE_TRAP_REGS_a5       15
> +#define SMODE_TRAP_REGS_a6       16
> +#define SMODE_TRAP_REGS_a7       17
> +#define SMODE_TRAP_REGS_s2       18
> +#define SMODE_TRAP_REGS_s3       19
> +#define SMODE_TRAP_REGS_s4       20
> +#define SMODE_TRAP_REGS_s5       21
> +#define SMODE_TRAP_REGS_s6       22
> +#define SMODE_TRAP_REGS_s7       23
> +#define SMODE_TRAP_REGS_s8       24
> +#define SMODE_TRAP_REGS_s9       25
> +#define SMODE_TRAP_REGS_s10      26
> +#define SMODE_TRAP_REGS_s11      27
> +#define SMODE_TRAP_REGS_t3       28
> +#define SMODE_TRAP_REGS_t4       29
> +#define SMODE_TRAP_REGS_t5       30
> +#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_last     35
> +
> +#define SMODE_TRAP_REGS_OFFSET(x)  ((SMODE_TRAP_REGS_##x) * __SIZEOF_POINTER__)
> +#define SMODE_TRAP_REGS_SIZE  SMODE_TRAP_REGS_OFFSET(last)
> +
> +#pragma pack(1)
> +typedef struct {
> +  //
> +  // Below are follow the format of EFI_SYSTEM_CONTEXT
> +  //
> +  UINT64    zero;
> +  UINT64    ra;
> +  UINT64    sp;
> +  UINT64    gp;
> +  UINT64    tp;
> +  UINT64    t0;
> +  UINT64    t1;
> +  UINT64    t2;
> +  UINT64    s0;
> +  UINT64    s1;
> +  UINT64    a0;
> +  UINT64    a1;
> +  UINT64    a2;
> +  UINT64    a3;
> +  UINT64    a4;
> +  UINT64    a5;
> +  UINT64    a6;
> +  UINT64    a7;
> +  UINT64    s2;
> +  UINT64    s3;
> +  UINT64    s4;
> +  UINT64    s5;
> +  UINT64    s6;
> +  UINT64    s7;
> +  UINT64    s8;
> +  UINT64    s9;
> +  UINT64    s10;
> +  UINT64    s11;
> +  UINT64    t3;
> +  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;
> +} SMODE_TRAP_REGISTERS;
> +#pragma pack()
> +
> +#endif
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.c
> new file mode 100644
> index 000000000000..f4427599a6b7
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/CpuExceptionHandlerLib.c
> @@ -0,0 +1,136 @@
> +/** @file
> +  RISC-V Exception Handler library implementation.
> +
> +  Copyright (c) 2016 - 2022, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +  Copyright (c) 2022, Ventana Micro Systems Inc. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <PiPei.h>
> +#include <Library/CpuExceptionHandlerLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BaseLib.h>
> +#include <Register/RiscV64/RiscVAsm.h>
> +
> +#include "CpuExceptionHandlerLib.h"
> +
> +STATIC EFI_CPU_INTERRUPT_HANDLER  mInterruptHandlers[2];
> +
> +/**
> +  Initializes all CPU exceptions entries and provides the default exception handlers.
> +
> +  Caller should try to get an array of interrupt and/or exception vectors that are in use and need to
> +  persist by EFI_VECTOR_HANDOFF_INFO defined in PI 1.3 specification.
> +  If caller cannot get reserved vector list or it does not exists, set VectorInfo to NULL.
> +  If VectorInfo is not NULL, the exception vectors will be initialized per vector attribute accordingly.
> +
> +  @param[in]  VectorInfo    Pointer to reserved vector list.
> +
> +  @retval EFI_SUCCESS           CPU Exception Entries have been successfully initialized
> +                                with default exception handlers.
> +  @retval EFI_INVALID_PARAMETER VectorInfo includes the invalid content if VectorInfo is not NULL.
> +  @retval EFI_UNSUPPORTED       This function is not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +InitializeCpuExceptionHandlers (
> +  IN EFI_VECTOR_HANDOFF_INFO  *VectorInfo OPTIONAL
> +  )
> +{
> +  csr_write (CSR_STVEC, SupervisorModeTrap);
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  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.
> +  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]  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
> +                                previously installed.
> +  @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for InterruptType was not
> +                                previously installed.
> +  @retval EFI_UNSUPPORTED       The interrupt specified by InterruptType is not supported,
> +                                or this function is not supported.
> +**/
> +EFI_STATUS
> +EFIAPI
> +RegisterCpuInterruptHandler (
> +  IN EFI_EXCEPTION_TYPE         InterruptType,
> +  IN EFI_CPU_INTERRUPT_HANDLER  InterruptHandler
> +  )
> +{
> +  DEBUG ((DEBUG_INFO, "%a: Type:%x Handler: %x\n", __FUNCTION__, InterruptType, InterruptHandler));
> +  mInterruptHandlers[InterruptType] = InterruptHandler;
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Setup separate stacks for certain exception handlers.
> +  If the input Buffer and BufferSize are both NULL, use global variable if possible.
> +
> +  @param[in]       Buffer        Point to buffer used to separate exception stack.
> +  @param[in, out]  BufferSize    On input, it indicates the byte size of Buffer.
> +                                 If the size is not enough, the return status will
> +                                 be EFI_BUFFER_TOO_SMALL, and output BufferSize
> +                                 will be the size it needs.
> +
> +  @retval EFI_SUCCESS             The stacks are assigned successfully.
> +  @retval EFI_UNSUPPORTED         This function is not supported.
> +  @retval EFI_BUFFER_TOO_SMALL    This BufferSize is too small.
> +**/
> +EFI_STATUS
> +EFIAPI
> +InitializeSeparateExceptionStacks (
> +  IN     VOID   *Buffer,
> +  IN OUT UINTN  *BufferSize
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Supervisor mode trap handler.
> +
> +  @param[in]  SmodeTrapReg     Registers before trap occurred.
> +
> +**/
> +VOID
> +RiscVSupervisorModeTrapHandler (
> +  SMODE_TRAP_REGISTERS  *SmodeTrapReg
> +  )
> +{
> +  UINTN               SCause;
> +  EFI_SYSTEM_CONTEXT  RiscVSystemContext;
> +
> +  RiscVSystemContext.SystemContextRiscV64 = (EFI_SYSTEM_CONTEXT_RISCV64 *)SmodeTrapReg;
> +  //
> +  // Check scasue register.
> +  //
> +  SCause = (UINTN)csr_read (CSR_SCAUSE);
> +  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);
> +    }
> +  }
> +}
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/SupervisorTrapHandler.S
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/SupervisorTrapHandler.S
> new file mode 100644
> index 000000000000..649c4c5becf4
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/RiscV64/SupervisorTrapHandler.S
> @@ -0,0 +1,105 @@
> +/** @file
> +  RISC-V Processor supervisor mode trap handler
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include "CpuExceptionHandlerLib.h"
> +
> +  .align 3
> +  .section .entry, "ax", %progbits
> +  .globl SupervisorModeTrap
> +SupervisorModeTrap:
> +  addi sp, sp, -SMODE_TRAP_REGS_SIZE
> +
> +  /* Save all general regisers except SP */
> +  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)
> +  ld    t0, SMODE_TRAP_REGS_OFFSET(t0)(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)
> +  sd    t1, SMODE_TRAP_REGS_OFFSET(t1)(sp)
> +  sd    t2, SMODE_TRAP_REGS_OFFSET(t2)(sp)
> +  sd    s0, SMODE_TRAP_REGS_OFFSET(s0)(sp)
> +  sd    s1, SMODE_TRAP_REGS_OFFSET(s1)(sp)
> +  sd    a0, SMODE_TRAP_REGS_OFFSET(a0)(sp)
> +  sd    a1, SMODE_TRAP_REGS_OFFSET(a1)(sp)
> +  sd    a2, SMODE_TRAP_REGS_OFFSET(a2)(sp)
> +  sd    a3, SMODE_TRAP_REGS_OFFSET(a3)(sp)
> +  sd    a4, SMODE_TRAP_REGS_OFFSET(a4)(sp)
> +  sd    a5, SMODE_TRAP_REGS_OFFSET(a5)(sp)
> +  sd    a6, SMODE_TRAP_REGS_OFFSET(a6)(sp)
> +  sd    a7, SMODE_TRAP_REGS_OFFSET(a7)(sp)
> +  sd    s2, SMODE_TRAP_REGS_OFFSET(s2)(sp)
> +  sd    s3, SMODE_TRAP_REGS_OFFSET(s3)(sp)
> +  sd    s4, SMODE_TRAP_REGS_OFFSET(s4)(sp)
> +  sd    s5, SMODE_TRAP_REGS_OFFSET(s5)(sp)
> +  sd    s6, SMODE_TRAP_REGS_OFFSET(s6)(sp)
> +  sd    s7, SMODE_TRAP_REGS_OFFSET(s7)(sp)
> +  sd    s8, SMODE_TRAP_REGS_OFFSET(s8)(sp)
> +  sd    s9, SMODE_TRAP_REGS_OFFSET(s9)(sp)
> +  sd    s10, SMODE_TRAP_REGS_OFFSET(s10)(sp)
> +  sd    s11, SMODE_TRAP_REGS_OFFSET(s11)(sp)
> +  sd    t3, SMODE_TRAP_REGS_OFFSET(t3)(sp)
> +  sd    t4, SMODE_TRAP_REGS_OFFSET(t4)(sp)
> +  sd    t5, SMODE_TRAP_REGS_OFFSET(t5)(sp)
> +  sd    t6, SMODE_TRAP_REGS_OFFSET(t6)(sp)
> +
> +  /* Call to Supervisor mode trap handler in CpuExceptionHandlerLib.c */
> +  call  RiscVSupervisorModeTrapHandler
> +
> +  /* Restore all general regisers except SP */
> +  ld    ra, SMODE_TRAP_REGS_OFFSET(ra)(sp)
> +  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    s0, SMODE_TRAP_REGS_OFFSET(s0)(sp)
> +  ld    s1, SMODE_TRAP_REGS_OFFSET(s1)(sp)
> +  ld    a0, SMODE_TRAP_REGS_OFFSET(a0)(sp)
> +  ld    a1, SMODE_TRAP_REGS_OFFSET(a1)(sp)
> +  ld    a2, SMODE_TRAP_REGS_OFFSET(a2)(sp)
> +  ld    a3, SMODE_TRAP_REGS_OFFSET(a3)(sp)
> +  ld    a4, SMODE_TRAP_REGS_OFFSET(a4)(sp)
> +  ld    a5, SMODE_TRAP_REGS_OFFSET(a5)(sp)
> +  ld    a6, SMODE_TRAP_REGS_OFFSET(a6)(sp)
> +  ld    a7, SMODE_TRAP_REGS_OFFSET(a7)(sp)
> +  ld    s2, SMODE_TRAP_REGS_OFFSET(s2)(sp)
> +  ld    s3, SMODE_TRAP_REGS_OFFSET(s3)(sp)
> +  ld    s4, SMODE_TRAP_REGS_OFFSET(s4)(sp)
> +  ld    s5, SMODE_TRAP_REGS_OFFSET(s5)(sp)
> +  ld    s6, SMODE_TRAP_REGS_OFFSET(s6)(sp)
> +  ld    s7, SMODE_TRAP_REGS_OFFSET(s7)(sp)
> +  ld    s8, SMODE_TRAP_REGS_OFFSET(s8)(sp)
> +  ld    s9, SMODE_TRAP_REGS_OFFSET(s9)(sp)
> +  ld    s10, SMODE_TRAP_REGS_OFFSET(s10)(sp)
> +  ld    s11, SMODE_TRAP_REGS_OFFSET(s11)(sp)
> +  ld    t3, SMODE_TRAP_REGS_OFFSET(t3)(sp)
> +  ld    t4, SMODE_TRAP_REGS_OFFSET(t4)(sp)
> +  ld    t5, SMODE_TRAP_REGS_OFFSET(t5)(sp)
> +  ld    t6, SMODE_TRAP_REGS_OFFSET(t6)(sp)
> +
> +  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
> +  csrw  CSR_SSTATUS, t0
> +  ld    t1, SMODE_TRAP_REGS_OFFSET(t1)(sp)
> +  ld    t0, SMODE_TRAP_REGS_OFFSET(t0)(sp)
> +  addi  sp, sp, SMODE_TRAP_REGS_SIZE
> +  sret
> --
> 2.25.1
> 
> 
> 
> 
>