[PATCH] ArmPkg/Drivers: ArmGicIsInterruptEnabled returns incorrect value

[PATCH] ArmPkg/Drivers: ArmGicIsInterruptEnabled returns incorrect value

[Robbie King]

The issue appears to have been introduced by:

41fb5d46 : ArmPkg/ArmGic: Use the GIC Redistributor instead of GIC Distributor for GICv3

The changes to ArmGicIsInterruptEnabled() introduced the error where the Boolean
result is assigned to Interrupts, but then the bit position check is performed
again (against the computed Boolean result instead of the interrupt mask) during
the return statement.

Fix removes erroneous test and relies on boolean test made at return.

Cc: Leif Lindholm <quic_llindhol@quicinc.com> 
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> 
Cc: Sami Mujawar <sami.mujawar@arm.com> 
Signed-off-by: Robbie King <robbiek@xsightlabs.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 58ab45f812..dd3670c7cc 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -366,10 +366,9 @@ ArmGicIsInterruptEnabled (
       FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
       SourceIsSpi (Source))
   {
-    Interrupts = ((MmioRead32 (
-                     GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
-                     )
-                   & (1 << RegShift)) != 0);
+    Interrupts = MmioRead32 (
+                   GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
+                   );
   } else {
     GicCpuRedistributorBase = GicGetCpuRedistributorBase (
                                 GicRedistributorBase,
-- 
2.17.1

Re: [PATCH] ArmPkg/Drivers: ArmGicIsInterruptEnabled returns incorrect value

[Ard Biesheuvel]

On Fri, 1 Jul 2022 at 17:07, Robbie King <robbiek@xsightlabs.com> wrote:
>
> The issue appears to have been introduced by:
>
> 41fb5d46 : ArmPkg/ArmGic: Use the GIC Redistributor instead of GIC Distributor for GICv3
>
> The changes to ArmGicIsInterruptEnabled() introduced the error where the Boolean
> result is assigned to Interrupts, but then the bit position check is performed
> again (against the computed Boolean result instead of the interrupt mask) during
> the return statement.
>
> Fix removes erroneous test and relies on boolean test made at return.
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Robbie King <robbiek@xsightlabs.com>

Nice find! How did you spot this? Through inspection? Or due to an
actual failure?

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

I'll go and queue this up, thanks.

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 58ab45f812..dd3670c7cc 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -366,10 +366,9 @@ ArmGicIsInterruptEnabled (
>        FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
>        SourceIsSpi (Source))
>    {
> -    Interrupts = ((MmioRead32 (
> -                     GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
> -                     )
> -                   & (1 << RegShift)) != 0);
> +    Interrupts = MmioRead32 (
> +                   GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
> +                   );
>    } else {
>      GicCpuRedistributorBase = GicGetCpuRedistributorBase (
>                                  GicRedistributorBase,
> --
> 2.17.1
>

Re: [PATCH] ArmPkg/Drivers: ArmGicIsInterruptEnabled returns incorrect value

[Robbie King]

Thanks Ard, this was due to an actual failure that was a bit of a challenge to debug.

A test in the ARM SBSA test suite installs the ISR first (which enables the interrupt) and then calls in to UEFI
to change the interrupt type (EDGE vs LEVEL) as specified in the ACPI table.  The UEFI code calls this failing
routine to determine if it needs to disable the interrupt before changing the interrupt type.  Since it received a bad
result, it did not disable the interrupt before changing the type.  The GICV3 says the behavior is
undefined if you do this, and the GIC ignored changing the type, and the test would fail due to the
interrupt not firing.  

Had our TF-A code set the interrupt type to the correct value, or if the test had changed type first and then installed
the ISR, we would not have found it.

Hope this makes sense.

-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org> 
Sent: Friday, July 1, 2022 11:16 AM
To: Robbie King <robbiek@xsightlabs.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [PATCH] ArmPkg/Drivers: ArmGicIsInterruptEnabled returns incorrect value

On Fri, 1 Jul 2022 at 17:07, Robbie King <robbiek@xsightlabs.com> wrote:
>
> The issue appears to have been introduced by:
>
> 41fb5d46 : ArmPkg/ArmGic: Use the GIC Redistributor instead of GIC 
> Distributor for GICv3
>
> The changes to ArmGicIsInterruptEnabled() introduced the error where 
> the Boolean result is assigned to Interrupts, but then the bit 
> position check is performed again (against the computed Boolean result 
> instead of the interrupt mask) during the return statement.
>
> Fix removes erroneous test and relies on boolean test made at return.
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Robbie King <robbiek@xsightlabs.com>

Nice find! How did you spot this? Through inspection? Or due to an actual failure?

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

I'll go and queue this up, thanks.

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c 
> b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 58ab45f812..dd3670c7cc 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -366,10 +366,9 @@ ArmGicIsInterruptEnabled (
>        FeaturePcdGet (PcdArmGicV3WithV2Legacy) ||
>        SourceIsSpi (Source))
>    {
> -    Interrupts = ((MmioRead32 (
> -                     GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
> -                     )
> -                   & (1 << RegShift)) != 0);
> +    Interrupts = MmioRead32 (
> +                   GicDistributorBase + ARM_GIC_ICDISER + (4 * RegOffset)
> +                   );
>    } else {
>      GicCpuRedistributorBase = GicGetCpuRedistributorBase (
>                                  GicRedistributorBase,
> --
> 2.17.1
>