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

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

On 27 March 2017 at 13:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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

.. or rather, 2d120489583a