From patchwork Tue Feb 23 07:09:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 62670 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp1678913lbl; Mon, 22 Feb 2016 23:09:39 -0800 (PST) X-Received: by 10.66.65.109 with SMTP id w13mr43973775pas.142.1456211379271; Mon, 22 Feb 2016 23:09:39 -0800 (PST) Return-Path: Received: from ml01.01.org (ml01.01.org. [2001:19d0:306:5::1]) by mx.google.com with ESMTPS id us7si45322399pac.204.2016.02.22.23.09.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 22 Feb 2016 23:09:39 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) client-ip=2001:19d0:306:5::1; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 2001:19d0:306:5::1 as permitted sender) smtp.mailfrom=edk2-devel-bounces@lists.01.org; dkim=neutral (body hash did not verify) header.i=@linaro.org Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 26E351A202D; Mon, 22 Feb 2016 23:09:41 -0800 (PST) X-Original-To: edk2-devel@lists.01.org Delivered-To: edk2-devel@lists.01.org Received: from mail-ig0-x236.google.com (mail-ig0-x236.google.com [IPv6:2607:f8b0:4001:c05::236]) (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 764D81A202C for ; Mon, 22 Feb 2016 23:09:40 -0800 (PST) Received: by mail-ig0-x236.google.com with SMTP id z8so1774566ige.0 for ; Mon, 22 Feb 2016 23:09:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=7rPjQ0QaU+UgkWssBN+qkB54ULfUY7bPxUwykPmYVj0=; b=SJgPZ4rWfKtofXh+uNvZ/wM96WVYvdu4rNLKhgIDRg9oZ3V5HUPAIZFrJ/y6FKYar3 Kuy6pfMeXvOqVDuQTrIP51weE3cmF+Nmqmbs4O5pGacmOTGrlUyzb720x0AKJAHmb1vW Tf6woj7kAKPg4puB1vvkAJeWIpaC+OBgPX0AM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=7rPjQ0QaU+UgkWssBN+qkB54ULfUY7bPxUwykPmYVj0=; b=IzlrN3aXi+LJEWLvQktZSpYx7QbeS4hSbe3y3n8oDFyfTgZbgZ6j7GBkla7cgAL/jd qzj/eA2zwBASPV472OaR3fYxkbIGfTrm4vRslhmFqVIlABfl4aELmpPzQuhpFCgCfiX9 vk4iTb/CNZyuQ1sfIbgn26NX9cSxF81gSi0Lb93yvMbNnQhigeIVoM5UypHDNj+BHc1g cnwk4+sAdVOqbmOOAyiEnCfFnViLQ3NL3gar8TU1S/ABPHLYWsLPlHRejiKDF5CoX16J 4+4tl3QKa9/gGgS3G6ZzKlxkJoUit0B0rBp2E4JdKKuVp+1qgYdMGGkePe3c46V1KExI teYg== X-Gm-Message-State: AG10YOT9SQSTeTf1CXaNbUsE29pQarcEpF7No43rNxDMs2P1wLH3Enhy4+3vnw4u6Je6580k/piSweNlI9CDcEaL MIME-Version: 1.0 X-Received: by 10.50.78.130 with SMTP id b2mr14823196igx.71.1456211377094; Mon, 22 Feb 2016 23:09:37 -0800 (PST) Received: by 10.36.29.6 with HTTP; Mon, 22 Feb 2016 23:09:37 -0800 (PST) In-Reply-To: References: Date: Tue, 23 Feb 2016 08:09:37 +0100 Message-ID: From: Ard Biesheuvel To: "Cohen, Eugene" Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [edk2] [PATCH] ArmPkg: Fix AArch64 interrupt read masks X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" On 23 February 2016 at 00:08, Cohen, Eugene wrote: > The AArch64 DAIF bits are different for reading (mrs) versus writing > (msr). The bitmask definitions assumed they were the same causing > incorrect results when trying to determine the current interrupt > state through ArmGetInterruptState. > > The logic for interpreting the DAIF read data using the csel instruction > was also incorrect and is fixed. > > Lastly, the CpuDxe driver was updated to remove an assumption that it > was the only component modifying interrupt state since this can be done > through BaseLib as well. Instead of using a global variable for last > interrupt state we now check the current PSTATE value directly. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Eugene Cohen Oops. I wonder how we tested that code (rhetorical question) Anyway, thanks for fixing this. Reviewed-by: Ard Biesheuvel @Leif: mind if I fold in the follow change when applying? > --- > ArmPkg/Drivers/CpuDxe/CpuDxe.c | 6 +--- > ArmPkg/Library/ArmLib/AArch64/ArmLibSupportV8.S | 32 ++++++++++++---------- > .../Library/ArmLib/Common/AArch64/ArmLibSupport.S | 12 ++++---- > 3 files changed, 24 insertions(+), 26 deletions(-) > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > index 0c49acb..b1cac31 100644 > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c > @@ -17,8 +17,6 @@ > > #include > > -BOOLEAN mInterruptState = FALSE; > - > > /** > This function flushes the range of addresses from Start to Start+Length > @@ -92,7 +90,6 @@ CpuEnableInterrupt ( > { > ArmEnableInterrupts (); > > - mInterruptState = TRUE; > return EFI_SUCCESS; > } > > @@ -114,7 +111,6 @@ CpuDisableInterrupt ( > { > ArmDisableInterrupts (); > > - mInterruptState = FALSE; > return EFI_SUCCESS; > } > > @@ -143,7 +139,7 @@ CpuGetInterruptState ( > return EFI_INVALID_PARAMETER; > } > > - *State = mInterruptState; > + *State = ArmGetInterruptState(); > return EFI_SUCCESS; > } > > diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupportV8.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupportV8.S > index 8fd4194..341bbce 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupportV8.S > +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupportV8.S > @@ -35,12 +35,14 @@ GCC_ASM_EXPORT (ReadCLIDR) > > .set MPIDR_U_BIT, (30) > .set MPIDR_U_MASK, (1 << MPIDR_U_BIT) > -.set DAIF_FIQ_BIT, (1 << 0) > -.set DAIF_IRQ_BIT, (1 << 1) > -.set DAIF_ABORT_BIT, (1 << 2) > -.set DAIF_DEBUG_BIT, (1 << 3) > -.set DAIF_INT_BITS, (DAIF_FIQ_BIT | DAIF_IRQ_BIT) > -.set DAIF_ALL, (DAIF_DEBUG_BIT | DAIF_ABORT_BIT | DAIF_INT_BITS) > + > +// DAIF bit definitions for writing through msr daifclr/sr daifset > +.set DAIF_WR_FIQ_BIT, (1 << 0) > +.set DAIF_WR_IRQ_BIT, (1 << 1) > +.set DAIF_WR_ABORT_BIT, (1 << 2) > +.set DAIF_WR_DEBUG_BIT, (1 << 3) > +.set DAIF_WR_INT_BITS, (DAIF_WR_FIQ_BIT | DAIF_WR_IRQ_BIT) > +.set DAIF_WR_ALL, (DAIF_WR_DEBUG_BIT | DAIF_WR_ABORT_BIT | DAIF_WR_INT_BITS) > > > ASM_PFX(ArmIsMpCore): > @@ -52,55 +54,55 @@ ASM_PFX(ArmIsMpCore): > > > ASM_PFX(ArmEnableAsynchronousAbort): > - msr daifclr, #DAIF_ABORT_BIT > + msr daifclr, #DAIF_WR_ABORT_BIT > isb > ret > > > ASM_PFX(ArmDisableAsynchronousAbort): > - msr daifset, #DAIF_ABORT_BIT > + msr daifset, #DAIF_WR_ABORT_BIT > isb > ret > > > ASM_PFX(ArmEnableIrq): > - msr daifclr, #DAIF_IRQ_BIT > + msr daifclr, #DAIF_WR_IRQ_BIT > isb > ret > > > ASM_PFX(ArmDisableIrq): > - msr daifset, #DAIF_IRQ_BIT > + msr daifset, #DAIF_WR_IRQ_BIT > isb > ret > > > ASM_PFX(ArmEnableFiq): > - msr daifclr, #DAIF_FIQ_BIT > + msr daifclr, #DAIF_WR_FIQ_BIT > isb > ret > > > ASM_PFX(ArmDisableFiq): > - msr daifset, #DAIF_FIQ_BIT > + msr daifset, #DAIF_WR_FIQ_BIT > isb > ret > > > ASM_PFX(ArmEnableInterrupts): > - msr daifclr, #DAIF_INT_BITS > + msr daifclr, #DAIF_WR_INT_BITS > isb > ret > > > ASM_PFX(ArmDisableInterrupts): > - msr daifset, #DAIF_INT_BITS > + msr daifset, #DAIF_WR_INT_BITS > isb > ret > > > ASM_PFX(ArmDisableAllExceptions): > - msr daifset, #DAIF_ALL > + msr daifset, #DAIF_WR_ALL > isb > ret > > diff --git a/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S > index 50faef4..fc782dc 100644 > --- a/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S > +++ b/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S > @@ -42,8 +42,8 @@ GCC_ASM_EXPORT (ArmWriteCpuActlr) > > #------------------------------------------------------------------------------ > > -.set DAIF_FIQ_BIT, (1 << 0) > -.set DAIF_IRQ_BIT, (1 << 1) > +.set DAIF_RD_FIQ_BIT, (1 << 6) > +.set DAIF_RD_IRQ_BIT, (1 << 7) > > ASM_PFX(ArmReadMidr): > mrs x0, midr_el1 // Read from Main ID Register (MIDR) > @@ -55,18 +55,18 @@ ASM_PFX(ArmCacheInfo): > > ASM_PFX(ArmGetInterruptState): > mrs x0, daif > - tst w0, #DAIF_IRQ_BIT // Check if IRQ is enabled. Enabled if 0. > + tst w0, #DAIF_RD_IRQ_BIT // Check if IRQ is enabled. Enabled if 0 (Z=1) > mov w0, #0 > mov w1, #1 > - csel w0, w1, w0, ne > + csel w0, w1, w0, eq // if Z=1 return 1, else 0 > ret > > ASM_PFX(ArmGetFiqState): > mrs x0, daif > - tst w0, #DAIF_FIQ_BIT // Check if FIQ is enabled. Enabled if 0. > + tst w0, #DAIF_RD_FIQ_BIT // Check if FIQ is enabled. Enabled if 0 (Z=1) > mov w0, #0 > mov w1, #1 > - csel w0, w1, w0, ne > + csel w0, w1, w0, eq // if Z=1 return 1, else 0 > ret > > ASM_PFX(ArmWriteCpacr): > -- > 1.9.5.msysgit.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel diff --git a/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S index fc782dcc1864..48b118317fca 100644 --- a/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S +++ b/ArmPkg/Library/ArmLib/Common/AArch64/ArmLibSupport.S @@ -56,17 +56,13 @@ ASM_PFX(ArmCacheInfo): ASM_PFX(ArmGetInterruptState): mrs x0, daif tst w0, #DAIF_RD_IRQ_BIT // Check if IRQ is enabled. Enabled if 0 (Z=1) - mov w0, #0 - mov w1, #1 - csel w0, w1, w0, eq // if Z=1 return 1, else 0 + cinc w0, wzr, eq // if Z=1 return 1, else 0 ret ASM_PFX(ArmGetFiqState): mrs x0, daif tst w0, #DAIF_RD_FIQ_BIT // Check if FIQ is enabled. Enabled if 0 (Z=1) - mov w0, #0 - mov w1, #1 - csel w0, w1, w0, eq // if Z=1 return 1, else 0 + cinc w0, wzr, eq // if Z=1 return 1, else 0 ret ASM_PFX(ArmWriteCpacr):