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

Sami Mujawar posted 12 patches 1 year ago
There is a newer version of this series
[edk2-devel] [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3
Posted by Sami Mujawar 1 year ago
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@arm.com>
---
 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 (#105182): https://edk2.groups.io/g/devel/message/105182
Mute This Topic: https://groups.io/mt/99086465/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3
Posted by Ard Biesheuvel 1 year ago
On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@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@arm.com>

Reviewed-by: Ard Biesheuvel <ardb@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-