Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions

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

On Mon, Mar 20, 2017 at 08:53:01PM +0000, Ard Biesheuvel wrote:
> In order to be able to produce meaningful diagnostic output when taking
> synchronous exceptions that have been caused by corruption of the stack
> pointer, prepare the EL0 stack pointer and switch to it when handling the
> 'Sync exception using SPx' exception class. Other exception classes (of
> which we really only care about IrqSPx) are left functionally intact.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> 
> Note that some code has been moved around so that the macro doesn't grow too
> big to fit in a 128 byte slot, while keeping the code logically consistent.
> 
>  ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
>  ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
>  ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf        |  5 +-
>  3 files changed, 70 insertions(+), 38 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> index 3d6eb4974d74..bd307628af87 100644
> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> @@ -16,7 +16,7 @@
>  #include <Uefi.h>
>  
>  #include <Chipset/AArch64.h>
> -
> +#include <Library/MemoryAllocationLib.h>
>  #include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
>  
>  UINTN                   gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
> @@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK  gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
>  PHYSICAL_ADDRESS        gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
>  UINTN                   gDebuggerNoHandlerValue = 0; // todo: define for AArch64
>  
> +#define EL0_STACK_PAGES   2
> +
> +VOID
> +RegisterEl0Stack (
> +  IN  VOID    *Stack
> +  );
> +
>  RETURN_STATUS ArchVectorConfig(
>    IN  UINTN       VectorBaseAddress
>    )
>  {
>    UINTN             HcrReg;
> +  UINT8             *Stack;
> +
> +  Stack = AllocatePages (EL0_STACK_PAGES);
> +  if (Stack == NULL) {
> +    return RETURN_OUT_OF_RESOURCES;
> +  }
> +
> +  RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
>  
>    if (ArmReadCurrentEL() == AARCH64_EL2) {
>      HcrReg = ArmReadHcr();
> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> index ff1f5fc81316..ac426d72a150 100644
> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> @@ -100,6 +100,7 @@
>  
>  GCC_ASM_EXPORT(ExceptionHandlersEnd)
>  GCC_ASM_EXPORT(CommonCExceptionHandler)
> +GCC_ASM_EXPORT(RegisterEl0Stack)
>  
>  .text
>  
> @@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
>  VECTOR_BASE(ExceptionHandlersStart)
>  #endif
>  
> -  .macro  ExceptionEntry, val
> +  .macro  ExceptionEntry, val, sp=SPx
> +  .ifnc   \sp, SPx
> +  msr     SPsel, xzr
> +  .endif

Is this esoteric enough to motivate a comment?

> +
>    // Move the stackpointer so we can reach our structure with the str instruction.
>    sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>  
> -  // Push some GP registers so we can record the exception context
> +  // Push the GP registers so we can record the exception context
>    stp      x0, x1, [sp, #-GP_CONTEXT_SIZE]!
>    stp      x2, x3, [sp, #0x10]
>    stp      x4, x5, [sp, #0x20]
>    stp      x6, x7, [sp, #0x30]
> +  stp      x8,  x9,  [sp, #0x40]
> +  stp      x10, x11, [sp, #0x50]
> +  stp      x12, x13, [sp, #0x60]
> +  stp      x14, x15, [sp, #0x70]
> +  stp      x16, x17, [sp, #0x80]
> +  stp      x18, x19, [sp, #0x90]
> +  stp      x20, x21, [sp, #0xa0]
> +  stp      x22, x23, [sp, #0xb0]
> +  stp      x24, x25, [sp, #0xc0]
> +  stp      x26, x27, [sp, #0xd0]
> +  stp      x28, x29, [sp, #0xe0]
> +  add      x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>  
> -  EL1_OR_EL2_OR_EL3(x1)
> -1:mrs      x2, elr_el1   // Exception Link Register
> -  mrs      x3, spsr_el1  // Saved Processor Status Register 32bit
> -  mrs      x5, esr_el1   // EL1 Exception syndrome register 32bit
> -  mrs      x6, far_el1   // EL1 Fault Address Register
> -  b        4f
> -
> -2:mrs      x2, elr_el2   // Exception Link Register
> -  mrs      x3, spsr_el2  // Saved Processor Status Register 32bit
> -  mrs      x5, esr_el2   // EL2 Exception syndrome register 32bit
> -  mrs      x6, far_el2   // EL2 Fault Address Register
> -  b        4f
> -
> -3:mrs      x2, elr_el3   // Exception Link Register
> -  mrs      x3, spsr_el3  // Saved Processor Status Register 32bit
> -  mrs      x5, esr_el3   // EL3 Exception syndrome register 32bit
> -  mrs      x6, far_el3   // EL3 Fault Address Register
> +  .ifnc    \sp, SPx
> +  msr      SPsel, #1     // Switch back to read the SP value upon entry 
> +  mov      x7, sp
> +  msr      SPsel, xzr
> +  .else
> +  mov      x7, x28       // x28 contains the SP value upon entry
> +  .endif
>  
> -4:mrs      x4, fpsr      // Floating point Status Register  32bit
> +  stp      x30,  x7, [sp, #0xf0]
>  
>    // Record the type of exception that occurred.
>    mov       x0, #\val
> @@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
>  //
>  VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
>  ASM_PFX(SynchronousExceptionSPx):
> -  ExceptionEntry  EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> +  ExceptionEntry  EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
>  
>  VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
>  ASM_PFX(IrqSPx):
> @@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
>  
>  ASM_PFX(CommonExceptionEntry):
>  
> -  // Stack the remaining GP registers
> -  stp      x8,  x9,  [sp, #0x40]
> -  stp      x10, x11, [sp, #0x50]
> -  stp      x12, x13, [sp, #0x60]
> -  stp      x14, x15, [sp, #0x70]
> -  stp      x16, x17, [sp, #0x80]
> -  stp      x18, x19, [sp, #0x90]
> -  stp      x20, x21, [sp, #0xa0]
> -  stp      x22, x23, [sp, #0xb0]
> -  stp      x24, x25, [sp, #0xc0]
> -  stp      x26, x27, [sp, #0xd0]
> -  stp      x28, x29, [sp, #0xe0]
> -  add      x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
> -  stp      x30, x28, [sp, #0xf0]
> +  EL1_OR_EL2_OR_EL3(x1)
> +1:mrs      x2, elr_el1   // Exception Link Register
> +  mrs      x3, spsr_el1  // Saved Processor Status Register 32bit
> +  mrs      x5, esr_el1   // EL1 Exception syndrome register 32bit
> +  mrs      x6, far_el1   // EL1 Fault Address Register
> +  b        4f
> +
> +2:mrs      x2, elr_el2   // Exception Link Register
> +  mrs      x3, spsr_el2  // Saved Processor Status Register 32bit
> +  mrs      x5, esr_el2   // EL2 Exception syndrome register 32bit
> +  mrs      x6, far_el2   // EL2 Fault Address Register
> +  b        4f
> +
> +3:mrs      x2, elr_el3   // Exception Link Register
> +  mrs      x3, spsr_el3  // Saved Processor Status Register 32bit
> +  mrs      x5, esr_el3   // EL3 Exception syndrome register 32bit
> +  mrs      x6, far_el3   // EL3 Fault Address Register
> +
> +4:mrs      x4, fpsr      // Floating point Status Register  32bit
>  
>    // Save the SYS regs
>    stp      x2,  x3,  [x28, #-SYS_CONTEXT_SIZE]!
> @@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
>    add      sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>  
>    eret
> +
> +ASM_PFX(RegisterEl0Stack):
> +  msr     sp_el0, x0
> +  ret
> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> index 10d9ae0f4afc..cd9149cf76c6 100644
> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> @@ -53,10 +53,11 @@ [Packages]
>  
>  [LibraryClasses]
>    ArmLib
> -  DebugLib
> -  DefaultExceptionHandlerLib
>    BaseMemoryLib
>    CacheMaintenanceLib
> +  DebugLib
> +  DefaultExceptionHandlerLib
> +  MemoryAllocationLib
>  
>  [Pcd]
>    gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
> -- 
> 2.7.4
> 

Not tested, but it looks quite plausible, so if Eugene agrees:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>