Re: [PATCH 1/1] BaseLib:Fix RISC-V Supervisor mode (S-Mode) trap handler reentry issue.

["Daniel Schaefer" <daniel.schaefer@hpe.com>]

On 7/16/20 4:10 PM, Abner Chang wrote:
> While RISC-V hart is trapped into S-Mode, the S-Mode interrupt
> CSR (SIE) is disabled by RISC-V hart. However the (SIE) is enabled
> again by RestoreTPL, this causes the second S-Mode trap is triggered
> by the machine mode (M-Mode)timer interrupt redirection. The SRET
> instruction clear Supervisor Previous Privilege (SPP) to zero
> (User mode) in the second S-Mode interrupt according to the RISC-V
> spec. Above brings hart to the user mode (U-Mode) when execute
> SRET in the nested S-Mode interrupt handler because SPP is set to
> User Mode in the second interrupt. Afterward, system runs in U-Mode
> and any accesses to S-Mode CSR causes the invalid instruction exception.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> ---
>   .../Library/BaseLib/RiscV64/RiscVInterrupt.S  | 45 ++++++++++++++++---
>   1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S b/MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S
> index 766fcfb9cb..87b3468fc7 100644
> --- a/MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S
> +++ b/MdePkg/Library/BaseLib/RiscV64/RiscVInterrupt.S
> @@ -12,21 +12,52 @@ ASM_GLOBAL ASM_PFX(RiscVDisableSupervisorModeInterrupts)
>   ASM_GLOBAL ASM_PFX(RiscVEnableSupervisorModeInterrupt)
>   ASM_GLOBAL ASM_PFX(RiscVGetSupervisorModeInterrupts)
>   
> -# define  MSTATUS_SIE    0x00000002
> -# define  CSR_SSTATUS    0x100
> +#define  SSTATUS_SIE                 0x00000002
> +#define  CSR_SSTATUS                 0x100
> +  #define  SSTATUS_SPP_BIT_POSITION  8

Please start this define at the beginning of the line.

>   
> +//
> +// This routine disables supervisor mode interrupt
> +//
>   ASM_PFX(RiscVDisableSupervisorModeInterrupts):
> -  li   a1, MSTATUS_SIE
> -  csrc CSR_SSTATUS, a1
> +  add   sp, sp, -(__SIZEOF_POINTER__)
> +  sd    a1, (sp)
> +  li    a1, SSTATUS_SIE
> +  csrc  CSR_SSTATUS, a1
> +  ld    a1, (sp)
> +  add   sp, sp, (__SIZEOF_POINTER__)
>     ret

Good! Let's not lose those register values.

> +//
> +// This routine enables supervisor mode interrupt
> +//
>   ASM_PFX(RiscVEnableSupervisorModeInterrupt):
> -  li   a1, MSTATUS_SIE
> -  csrs CSR_SSTATUS, a1
> +  add   sp, sp, -2*(__SIZEOF_POINTER__)
> +  sd    a0, (0*__SIZEOF_POINTER__)(sp)
> +  sd    a1, (1*__SIZEOF_POINTER__)(sp)
> +
> +  csrr  a0, CSR_SSTATUS
> +  and   a0, a0, (1 << SSTATUS_SPP_BIT_POSITION)
> +  bnez  a0, InTrap      // We are in supervisor mode (SMode)
> +                        // trap handler.
> +                        // Skip enabling SIE becasue SIE

                                                 because

> +                        // is set to disabled by RISC-V hart
> +                        // when the trap takes hart to SMode.
> +
> +  li    a1, SSTATUS_SIE
> +  csrs  CSR_SSTATUS, a1
> +InTrap:
> +  ld    a0, (0*__SIZEOF_POINTER__)(sp)
> +  ld    a1, (1*__SIZEOF_POINTER__)(sp)
> +  add   sp, sp, 2*(__SIZEOF_POINTER__)
>     ret
>   
> +//
> +// This routine returns supervisor mode interrupt
> +// status.
> +//
>   ASM_PFX(RiscVGetSupervisorModeInterrupts):
>     csrr a0, CSR_SSTATUS
> -  andi a0, a0, MSTATUS_SIE
> +  andi a0, a0, SSTATUS_SIE
>     ret
>   
> 

Looks good otherwise :) With the the small changes:
Reviewed-by: Daniel Schaefer <daniel.schaefer@hpe.com>