From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x234.google.com (mail-it0-x234.google.com [IPv6:2607:f8b0:4001:c0b::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6309921DFA7BB for ; Mon, 27 Mar 2017 05:54:02 -0700 (PDT) Received: by mail-it0-x234.google.com with SMTP id y18so51232265itc.1 for ; Mon, 27 Mar 2017 05:54:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=gAu8/6e7oeYDwdZc/S8EOQc6PQSaIHn8/X4DythNS0c=; b=bcOm+s7IsYRGA8JGpegBkNzfykOasQsaI8aadWlEqS58T+utar111kFApVOIXsVnkj vT+n9cZHH7SjkEtWpguFiq1dgEomoGoI1TefXhGp3YUVb8uZWrKfRbzNCzE60DY4Z1VV kebKFRmV6wofgnOMuDspL2fcUFeKYQfMxHFvE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=gAu8/6e7oeYDwdZc/S8EOQc6PQSaIHn8/X4DythNS0c=; b=JSLcwc7PmpEGYJ/yrCHZ88vyMh4rWA5EnuBc3XMC7Q+oLM1gZBui4A6ljyAZlHXxBr ohtyU6mGpV6dhoduHoQ6NigIpNqwXuJxt0m5/byc1we6sqd7UT9IwmXyd/l2PYJ8G5Ra PocD+qUrxCo7GlXBHnABJDnOWoOPQESp8fauP/BhSRDrAcctfA1u5S73nlaBgQXlfyJA 4A74o6yGRENzNAwlzALuinha7OGkSSTuN724Lih2FI8EHkkQe8KEtwZXI84rNQ3AKlPl zgY57fPgPjQfAAE3gOMovUPXHw3iG6DvFZaOC8Juv2Ck8ep7w3JRe0bfP1i16H6Gv3Wj X/rw== X-Gm-Message-State: AFeK/H1zIZ3hTpvlpUZX7+zMsJF3kwcF85hvZTJUkoIcZ0IPHsea/k8ofWYlfUS6zQny//RPEpUJMtNPW30SLmTj X-Received: by 10.107.132.155 with SMTP id o27mr20018824ioi.138.1490619241372; Mon, 27 Mar 2017 05:54:01 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 27 Mar 2017 05:54:00 -0700 (PDT) In-Reply-To: <20170322131833.GY16034@bivouac.eciton.net> References: <1490043181-20031-1-git-send-email-ard.biesheuvel@linaro.org> <1490043181-20031-4-git-send-email-ard.biesheuvel@linaro.org> <20170322131833.GY16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 27 Mar 2017 13:54:00 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Ryan Harkin , "Cohen, Eugene" Subject: Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Mar 2017 12:54:02 -0000 Content-Type: text/plain; charset=UTF-8 On 22 March 2017 at 13:18, Leif Lindholm 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 >> --- >> >> 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 >> >> #include >> - >> +#include >> #include // 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 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