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

["Ard Biesheuvel" <ardb@kernel.org>]

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
>