On 27 March 2017 at 13:54, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 22 March 2017 at 13:18, Leif Lindholm <leif.lindh...@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.biesheu...@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.lindh...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel