| Message ID | CAKv+Gu8Bg6o=MLk+tGsYBsUwhtzi+NFQVAZY0wGAvMzpAxqPVg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 23 February 2016 at 08:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 23 February 2016 at 00:08, Cohen, Eugene <eugene@hp.com> 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 <eugene@hp.com> > > Oops. I wonder how we tested that code (rhetorical question) > > Anyway, thanks for fixing this. > > Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > @Leif: mind if I fold in the follow change when applying? > Pushed as e3aa7252ba58 ArmPkg: CpuDxe: don't track interrupt state in a global variable 4af3dd80abb7 ArmPkg: CpuDxe: fix AArch64 interrupt read masks Thanks Eugene. Please, could you split patches up if you are fixing more than one thing at a time? > 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): > > > >> --- >> 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 <Guid/IdleLoopEvent.h> >> >> -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):