Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 16/29] RiscVPkg/Library: Add RISC-V exception library

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Sep 23, 2019 at 08:31:42AM +0800, Abner Chang wrote:
> Initial RISC-V Supervisor Mode trap handler
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> ---
>  .../RiscVExceptionLib/CpuExceptionHandler.S        |  88 ++++++++++
>  .../CpuExceptionHandlerDxeLib.inf                  |  42 +++++
>  .../RiscVExceptionLib/CpuExceptionHandlerLib.c     | 182 +++++++++++++++++++++
>  .../RiscVExceptionLib/CpuExceptionHandlerLib.uni   |  13 ++
>  4 files changed, 325 insertions(+)
>  create mode 100644 RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandler.S
>  create mode 100644 RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerDxeLib.inf
>  create mode 100644 RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.c
>  create mode 100644 RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.uni
> 
> diff --git a/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandler.S b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandler.S
> new file mode 100644
> index 0000000..cffe485
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandler.S

Since the actual handling is done elsewhere, would CpuExceptionEntry.S
be a more descriptive filename?

> @@ -0,0 +1,88 @@
> +/** @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 <RiscV.h>
> +#include <sbi/riscv_asm.h>
> +#include <sbi/riscv_encoding.h>
> +#include <sbi/sbi_platform.h>
> +#include <sbi/sbi_scratch.h>
> +#include <sbi/sbi_trap.h>
> +
> +  .align 3
> +  .section .entry, "ax", %progbits
> +  .globl _strap_handler
> +_strap_handler:

Please rename this one:
- Drop _ from function name.
- Rename to proper CamelCase. Suggest: SupervisorTrapHandler.

> +  addi sp, sp, -34*8

Please add spaces around '*', throughout.

> + /* Save all general regisers except SP and T0 */
> +  sd    ra, 1*8(sp)
> +  sd    gp, 2*8(sp)
> +  sd    tp, 3*8(sp)
> +  sd    t1, 4*8(sp)
> +  sd    t2, 5*8(sp)
> +  sd    s0, 6*8(sp)
> +  sd    s1, 7*8(sp)
> +  sd    a0, 8*8(sp)
> +  sd    a1, 9*8(sp)
> +  sd    a2, 10*8(sp)
> +  sd    a3, 11*8(sp)
> +  sd    a4, 12*8(sp)
> +  sd    a5, 13*8(sp)
> +  sd    a6, 14*8(sp)
> +  sd    a7, 15*8(sp)
> +  sd    s2, 16*8(sp)
> +  sd    s3, 17*8(sp)
> +  sd    s4, 18*8(sp)
> +  sd    s5, 19*8(sp)
> +  sd    s6, 20*8(sp)
> +  sd    s7, 21*8(sp)
> +  sd    s8, 22*8(sp)
> +  sd    s9, 23*8(sp)
> +  sd    s10, 24*8(sp)
> +  sd    s11, 25*8(sp)
> +  sd    t3, 26*8(sp)
> +  sd    t4, 27*8(sp)
> +  sd    t5, 28*8(sp)
> +  sd    t6, 29*8(sp)
> +
> +  /* Call C routine */

Something like "Call main trap handling routine in x.c" would be more
helpful.

> +  call  RiscVSupervisorModeTrapHandler
> +
> +  /* Restore all general regisers except SP and T0 */
> +  ld    ra, 1*8(sp)
> +  ld    gp, 2*8(sp)
> +  ld    tp, 3*8(sp)
> +  ld    t1, 4*8(sp)
> +  ld    t2, 5*8(sp)
> +  ld    s0, 6*8(sp)
> +  ld    s1, 7*8(sp)
> +  ld    a0, 8*8(sp)
> +  ld    a1, 9*8(sp)
> +  ld    a2, 10*8(sp)
> +  ld    a3, 11*8(sp)
> +  ld    a4, 12*8(sp)
> +  ld    a5, 13*8(sp)
> +  ld    a6, 14*8(sp)
> +  ld    a7, 15*8(sp)
> +  ld    s2, 16*8(sp)
> +  ld    s3, 17*8(sp)
> +  ld    s4, 18*8(sp)
> +  ld    s5, 19*8(sp)
> +  ld    s6, 20*8(sp)
> +  ld    s7, 21*8(sp)
> +  ld    s8, 22*8(sp)
> +  ld    s9, 23*8(sp)
> +  ld    s10, 24*8(sp)
> +  ld    s11, 25*8(sp)
> +  ld    t3, 26*8(sp)
> +  ld    t4, 27*8(sp)
> +  ld    t5, 28*8(sp)
> +  ld    t6, 29*8(sp)
> +  addi sp, sp, 34*8
> +  sret
> diff --git a/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerDxeLib.inf b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerDxeLib.inf
> new file mode 100644
> index 0000000..e5871dc
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerDxeLib.inf
> @@ -0,0 +1,42 @@
> +## @file
> +#  RISC-V CPU Exception Handler Library
> +#
> +#  Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001b
> +  BASE_NAME                      = CpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = CpuExceptionHandlerLib.uni
> +  FILE_GUID                      = 16309FCF-E900-459C-B071-052118394D11
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib
> +  CONSTRUCTOR                    = CpuExceptionHandlerLibConstructor
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = RISCV64
> +#
> +
> +[Sources.RISCV64]
> +  CpuExceptionHandler.S
> +
> +[Sources.common]
> +  CpuExceptionHandlerLib.c
> +
> +[LibraryClasses]
> +  UefiBootServicesTableLib
> +  BaseLib
> +  DebugLib
> +  RiscVCpuLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  RiscVPkg/RiscVPkg.dec

Please sort LibraryClasses and Packages alphabetically.

> +
> diff --git a/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.c b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.c
> new file mode 100644
> index 0000000..8c75be0
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.c
> @@ -0,0 +1,182 @@
> +/** @file
> +  RISC-V Exception Handler library implementition.
> +
> +  Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <PiPei.h>
> +#include <Library/CpuExceptionHandlerLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/RiscVCpuLib.h>
> +#include <sbi/sbi_types.h>
> +#include <sbi/riscv_asm.h>
> +#include <sbi/riscv_encoding.h>

Please sort sbi includes alphabetically.

> +
> +
> +extern void _strap_handler(void);

Please add this to a local include file.

> +EFI_CPU_INTERRUPT_HANDLER gInterruptHandlers[2];

This (thankfully) seems to be global to the local file only.
Could it be renamed with a 'm' prefix and given STATIC attribute?

> +/**
> +  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
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Initializes all CPU interrupt/exceptions entries and provides the default interrupt/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           All CPU interrupt/exception entries have been successfully initialized
> +                                with default interrupt/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
> +InitializeCpuInterruptHandlers (
> +  IN EFI_VECTOR_HANDOFF_INFO       *VectorInfo OPTIONAL
> +  )
> +{
> +  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, "RegisterCpuInterruptHandler: Type:%x Handler: %x\n", InterruptType, InterruptHandler));
> +  gInterruptHandlers[InterruptType] = InterruptHandler;
> +  return EFI_SUCCESS;
> +}
> +/**
> +  Machine mode trap handler.
> +
> +**/
> +VOID
> +RiscVSupervisorModeTrapHandler (
> +  VOID
> +  )
> +{
> +  EFI_SYSTEM_CONTEXT RiscVSystemContext;
> +
> +  //
> +  // Check scasue register.
> +  //
> +  if(gInterruptHandlers[EXCEPT_RISCV_TIMER_INT] != NULL) {
> +    gInterruptHandlers[EXCEPT_RISCV_TIMER_INT](EXCEPT_RISCV_TIMER_INT, (CONST EFI_SYSTEM_CONTEXT)RiscVSystemContext);
> +  }
> +}
> +
> +/**
> +  Initializes all CPU exceptions entries with optional extra initializations.
> +
> +  By default, this method should include all functionalities implemented by
> +  InitializeCpuExceptionHandlers(), plus extra initialization works, if any.
> +  This could be done by calling InitializeCpuExceptionHandlers() directly
> +  in this method besides the extra works.
> +
> +  InitData is optional and its use and content are processor arch dependent.
> +  The typical usage of it is to convey resources which have to be reserved
> +  elsewhere and are necessary for the extra initializations of exception.
> +
> +  @param[in]  VectorInfo    Pointer to reserved vector list.
> +  @param[in]  InitData      Pointer to data optional for extra initializations
> +                            of exception.
> +
> +  @retval EFI_SUCCESS             The exceptions have been successfully
> +                                  initialized.
> +  @retval EFI_INVALID_PARAMETER   VectorInfo or InitData contains invalid
> +                                  content.
> +  @retval EFI_UNSUPPORTED         This function is not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +InitializeCpuExceptionHandlersEx (
> +  IN EFI_VECTOR_HANDOFF_INFO            *VectorInfo OPTIONAL,
> +  IN CPU_EXCEPTION_INIT_DATA            *InitData OPTIONAL
> +  )
> +{
> +  return InitializeCpuExceptionHandlers (VectorInfo);
> +}
> +
> +/**
> +  The constructor function to initial interrupt handlers in
> +  RISCV_MACHINE_MODE_CONTEXT.
> +
> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
> +  @param  SystemTable   A pointer to the EFI System Table.
> +
> +  @retval EFI_SUCCESS   The destructor completed successfully.
> +  @retval Other value   The destructor did not complete successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CpuExceptionHandlerLibConstructor (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  //
> +  // Set Superviosr mode trap handler.
> +  //
> +  csr_write(CSR_STVEC, _strap_handler);
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.uni b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.uni
> new file mode 100644
> index 0000000..00cca22
> --- /dev/null
> +++ b/RiscVPkg/Library/RiscVExceptionLib/CpuExceptionHandlerLib.uni
> @@ -0,0 +1,13 @@
> +// /** @file
> +//
> +// Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "RISC-V CPU Exception Handler Librarys."
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "RISC-V CPU Exception Handler Librarys."

Library.

/
    Leif

> +
> -- 
> 2.7.4
> 
> 
> 
>