[edk2-devel] [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar at arm.com> wrote:
>
> The ArmGicAcknowledgeInterrupt () returns the value
> returned by the Interrupt Acknowledge Register and
> the InterruptID separately in an out parameter.
>
> The function documents the following:
> 'InterruptId is returned separately from the register
> value because in the GICv2 the register value contains
> the CpuId and InterruptId while in the GICv3 the
> register value is only the InterruptId.'
>
> This function skips setting the InterruptId in the
> out parameter for GICv3. Although the return value
> from the function is the InterruptId for GICv3, this
> breaks the function usage model as the caller expects
> the InterruptId in the out parameter for the function.
> e.g. The caller may end up using the InterruptID which
> could potentially be an uninitialised variable value.
>
> Therefore, set the InterruptID in the function out
> parameter for GICv3 as well.
>
> Signed-off-by: Sami Mujawar <sami.mujawar at arm.com>

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

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 8f3315d76f6f2b28a551d73400938430ff3e23c7..7f4bb248fc7225bf63f0aea720486092b30ced10 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -176,19 +176,17 @@ ArmGicAcknowledgeInterrupt (
>    )
>  {
>    UINTN                  Value;
> +  UINTN                  IntId;
>    ARM_GIC_ARCH_REVISION  Revision;
>
> +  ASSERT (InterruptId != NULL);
>    Revision = ArmGicGetSupportedArchRevision ();
>    if (Revision == ARM_GIC_ARCH_REVISION_2) {
>      Value = ArmGicV2AcknowledgeInterrupt (GicInterruptInterfaceBase);
> -    // InterruptId is required for the caller to know if a valid or spurious
> -    // interrupt has been read
> -    ASSERT (InterruptId != NULL);
> -    if (InterruptId != NULL) {
> -      *InterruptId = Value & ARM_GIC_ICCIAR_ACKINTID;
> -    }
> +    IntId = Value & ARM_GIC_ICCIAR_ACKINTID;
>    } else if (Revision == ARM_GIC_ARCH_REVISION_3) {
>      Value = ArmGicV3AcknowledgeInterrupt ();
> +    IntId = Value;
>    } else {
>      ASSERT_EFI_ERROR (EFI_UNSUPPORTED);
>      // Report Spurious interrupt which is what the above controllers would
> @@ -196,6 +194,12 @@ ArmGicAcknowledgeInterrupt (
>      Value = 1023;
>    }
>
> +  if (InterruptId != NULL) {
> +    // InterruptId is required for the caller to know if a valid or spurious
> +    // interrupt has been read
> +    *InterruptId = IntId;
> +  }
> +
>    return Value;
>  }
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105194): https://edk2.groups.io/g/devel/message/105194
Mute This Topic: https://groups.io/mt/99086465/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-