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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 March 2017 at 13:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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>

No word back from Eugene, so I am going to assume that he is on board with this.

I have added the following comment

//
// Our backtrace and register dump code is written in C and so it requires
// a stack. This makes it difficult to produce meaningful diagnostics when
// the stack pointer has been corrupted. So in such cases (i.e., when taking
// synchronous exceptions), this macro is expanded with \sp set to SP0, in
// which case we switch to the SP_EL0 stack pointer, which has been
// initialized to point to a buffer that has been set aside for this purpose.
//
// Since 'sp' may no longer refer to the stack frame that was active when
// the exception was taken, we may have to switch back and forth between
// SP_EL0 and SP_ELx to record the correct value for SP in the context struct.
//

Pushed as 21dbcff5be3c