From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 1F8C3803B1 for ; Wed, 22 Mar 2017 06:18:38 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id n11so36413966wma.1 for ; Wed, 22 Mar 2017 06:18:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=MyJ/BhtasTPJq2Aipj0kSNNNltz+PuyYreJTBEKWvms=; b=dIX4TuEheXXyMbkocuDw89mK7Ve4bKZt8lQL/OSfalrUbV0MM6T0XitXH6tVMSNBx0 QSDwKFSdAvat5O1WLVMuAqhQVtZ6O0Zs12FX423dt+wKcsRb1im71cJITFp1zG9p5ENU fmQ0urgZHjKZRL0/wFQ78v/UDMa3FQnqNjox0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=MyJ/BhtasTPJq2Aipj0kSNNNltz+PuyYreJTBEKWvms=; b=XKZlvaNTi1t11PpwobHpW38u1o1a5+70XzhZnkICmPKHmf0PDd6oFHYXEDNE2IP7et mJrx5hZSIhiPZjPujoPeHwAljjC6lIeX8gCaqVRpNZGWg48CepCqGMQ0tL+mA0yJGJt1 KTk2dHG/eP1EjOYCwEcDSgFg3y7N3Yp1IWItWbmJfOrjupNUIuA221eJtNL9As/iKIoV mjDfCbQ2SLSMRemPxLHtihUZHylpFSkNUQBu2FgYJjE0XaOGEFP6WFpn7iiz/0Iud34q xJbkdUbBKXww+DQBuyN9BEBNvU8wxVbioQWabeRt+7SpyU/36kePCd4EDixny1xSp7Dt +5iA== X-Gm-Message-State: AFeK/H2Ky/nQJG8Th5TLgJM+zCFmxZgrjwifFg+vn4Ho/YZlBRL8CFhlaBKsyUB0sAG0ayzD X-Received: by 10.28.156.140 with SMTP id f134mr7519687wme.40.1490188716279; Wed, 22 Mar 2017 06:18:36 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id b91sm1831141wrd.29.2017.03.22.06.18.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 22 Mar 2017 06:18:35 -0700 (PDT) Date: Wed, 22 Mar 2017 13:18:33 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, ryan.harkin@linaro.org, eugene@hp.com Message-ID: <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> MIME-Version: 1.0 In-Reply-To: <1490043181-20031-4-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) 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:18:38 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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