[edk2] ArmPkg: Fix AArch64 interrupt read masks

Message ID CAKv+Gu8Bg6o=MLk+tGsYBsUwhtzi+NFQVAZY0wGAvMzpAxqPVg@mail.gmail.com
State New
Headers show

Commit Message

Ard Biesheuvel Feb. 23, 2016, 7:09 a.m.
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?




> ---

>  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

Comments

Ard Biesheuvel Feb. 23, 2016, 11:10 a.m. | #1
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

Patch

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):