From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (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 243F8803B1 for ; Wed, 22 Mar 2017 06:20:33 -0700 (PDT) Received: by mail-io0-x229.google.com with SMTP id f84so65238549ioj.0 for ; Wed, 22 Mar 2017 06:20:33 -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=IAJYTOgpGM5sq+yJTxR8mo2oKSWHKdqcbuTcjcY9D80=; b=E6sdLqixHsLjyalKah+QgAFPg3/xskLm4qPLLDv6PE17QAivrLhsMvPq9uQIIEkFZw XyL+dfBepugkjJZ2B///hlkca0/DvGg85d+nekTYlu+LuJ3hT6rpDEQ0rUjHj6alB+Ku MF8REjcUk4knG6CdijPdeYIxYiQxh3/Inpwtg= 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=IAJYTOgpGM5sq+yJTxR8mo2oKSWHKdqcbuTcjcY9D80=; b=iiyMEQOvz+OvazdJ/9UCREG+VwWP2C35CY22pbQEq1eQPAqNgDen4+yNwixIrIYBMX uLN8nsFXjxDc66dGqEs71LqVY4qwQMS0kky8cNsJoQBaPEnauF85n/blGwpAgdMatDX2 Ep2JDZT6GLVCt8SF7d76AaAA6zrwHOx9RJB1YpCBk6sZq9bPrpdx7o7MypZg+ll+TE8n Ena+hF04QCRzL5IbWjsB/SfDlE9qJtMvCnbXJAgOTnkcuZsaifygk1aKWxAdzfGoyXB+ 3SJ5HCCdrjIb7deEv65oeRQ84HJVm8sWU8XfL/c6/EfyuOg5J5GXNrJPNGdmDkl/epW/ oyPw== X-Gm-Message-State: AFeK/H0eXCG/UAZHhjn6T+hURGviV4PiOmmQqr1ImFYwey1zZPTr0O9o0bFBpeXnO1cuZ0wen1q0OINRXEv/hKA+ X-Received: by 10.107.168.21 with SMTP id r21mr34410608ioe.45.1490188832357; Wed, 22 Mar 2017 06:20:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Wed, 22 Mar 2017 06:20:31 -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: Wed, 22 Mar 2017 13:20:31 +0000 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: Wed, 22 Mar 2017 13:20:33 -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? > Yeah, good point. This looks like a suitable spot to dedicate some space to explain what's going on >> + >> // 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