From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (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 7D7E421DFA917 for ; Mon, 27 Mar 2017 05:55:31 -0700 (PDT) Received: by mail-it0-x230.google.com with SMTP id y18so71574919itc.0 for ; Mon, 27 Mar 2017 05:55:31 -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=7+vQcsZNvAIHJgZ58peviz+STiP4SoihHKhjsTG/2Yc=; b=jGqIwTdsd5pJmQNOZ3Z+EByiZseju0GWgNUASobIT1BbtFRP6RCQEoSPsbYikqk5+z Xxp2yu1FdV/ZnN++PPV9wnlB/TBiFl2WYLH8AyMZx+uql+0TXvUdvU12FgKX2fGN7/8d 6xGwhCZ/dSIZP5QkvXSTb6pVSJt9gsZBUyA9I= 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=7+vQcsZNvAIHJgZ58peviz+STiP4SoihHKhjsTG/2Yc=; b=QoPg3our1m/3orp1VUQ+PJAut0lJ3wmD3ysaBphQKJhEsAMFTaRjR/Z1LpzfRO6VR3 0f5PYEPKyyKqIhlVBzrC8T/hYh7Rou82VuqokrxVCNSsAyEyS8ZEryjp+bdQY1WAgMh2 M0se36rsWVCNDWUnyxj0mtwCVO88Op2h+0W7GhUvme3UaEiGHh8C5oLlnDXWYRHwFlde hLdGLzjOr1Tfix0Txac9SHIWyMhiXmHLWXWxDhLfK/dj0X8AvNn58JS/zcxS2hRdGPja 5JJVlwUV58jvmajwV9s3tmp0YNsg6MrMtMrc3YtPgxh1/dIdBDGZ+RsGlVYGbFIK4YsU 5YkQ== X-Gm-Message-State: AFeK/H0MDoxd7KiGS3sCiC0ybC9CESUQqg9SeA3lJNPdBLohCMzqhjpPb+qN5A7rkpo5SixdkeBOPjrl2/mPXiq2 X-Received: by 10.107.168.21 with SMTP id r21mr19700513ioe.45.1490619330632; Mon, 27 Mar 2017 05:55:30 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 27 Mar 2017 05:55:30 -0700 (PDT) In-Reply-To: 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:55:30 +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:55:31 -0000 Content-Type: text/plain; charset=UTF-8 On 27 March 2017 at 13:54, Ard Biesheuvel wrote: > 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 .. or rather, 2d120489583a