ArmPkg/Drivers/ArmGic/ArmGicLib.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91016): https://edk2.groups.io/g/devel/message/91016
Mute This Topic: https://groups.io/mt/92157475/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90991): https://edk2.groups.io/g/devel/message/90991 Mute This Topic: https://groups.io/mt/92111808/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#91017): https://edk2.groups.io/g/devel/message/91017 Mute This Topic: https://groups.io/mt/92111808/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.