[PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements

[PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements

[Sami Mujawar]

Bugzilla: Bug 3399 (https://bugzilla.tianocore.org/show_bug.cgi?id=3399)

This patch series address the issues reported in
https://bugzilla.tianocore.org/show_bug.cgi?id=3399
and also has general improvements and fixes for other
issues in the Arm GIC Library and driver.

This patch series is expected to be applied on top of
the patch at:
  ArmPkg: Fix GicV2 BaseAddress types
  (https://edk2.groups.io/g/devel/message/104721)

The changes can be seen at:
https://github.com/samimujawar/edk2/tree/1751_arm_giclib_v1

Sami Mujawar (12):
  ArmPkg: Fix data type used for GicDistributorBase
  ArmPkg: Fix data type used for GicInterruptInterfaceBase
  ArmPkg: Fix ArmGicSendSgiTo() parameters
  ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor
  ArmPkg: Fix return type for ArmGicGetInterfaceIdentification
  ArmPkg: Make variables used for GicInterrupt UINTN
  ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt
  ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt
  ArmPkg: Remove unused function declarations
  ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR
  ArmPkg: Adjust variable type and cast for RegShift & RegOffset
  ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3

 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c         | 18 +----
 ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 73 ++++++++++++--------
 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c         |  9 ++-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       | 18 ++---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c       |  6 +-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +-
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c       |  4 +-
 ArmPkg/Include/Library/ArmGicLib.h              | 38 +++++-----
 8 files changed, 88 insertions(+), 84 deletions(-)

-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase

[Sami Mujawar]

The data type used by variables representing the GicDistributorBase
has been inconsistently used in the ArmGic driver and the library.
The PCD defined for the GIC Distributor base address is UINT64.
However, the data types for the variables used is UINTN, INTN, and
at some places UINT32.

Therefore, update the data types to use UINTN and add necessary
typecasts when reading values from the PCD. This should then be
consistent across AArch32 and AArch64 builds.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  4 ++--
 ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 12 ++++++------
 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c   |  4 ++--
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c |  4 ++--
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c |  4 ++--
 ArmPkg/Include/Library/ArmGicLib.h        | 18 +++++++++---------
 6 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index d560c42fc9f3d5e86c2aece504102f43cb841877..9ac073db36ce92fc14de71e9a264059afd63d729 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
@@ -1,6 +1,6 @@
 /*++
 
-Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2013-2021, Arm Ltd. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -61,7 +61,7 @@ GicGetDistributorIcfgBaseAndBit (
 
   RegIndex    = Source / ARM_GIC_ICDICFR_F_STRIDE; // NOTE: truncation is significant
   Field       = Source % ARM_GIC_ICDICFR_F_STRIDE;
-  *RegAddress = PcdGet64 (PcdGicDistributorBase)
+  *RegAddress = (UINTN)PcdGet64 (PcdGicDistributorBase)
                 + ARM_GIC_ICDICFR
                 + (ARM_GIC_ICDICFR_BYTES * RegIndex);
   *Config1Bit = ((Field * ARM_GIC_ICDICFR_F_WIDTH)
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index dd3670c7ccbb18586bb28f4ac02514055471529f..6e44e89390fcdaa89302d6505f75c43c84ce3535 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -117,7 +117,7 @@ ArmGicGetInterfaceIdentification (
 UINTN
 EFIAPI
 ArmGicGetMaxNumInterrupts (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   )
 {
   UINTN  ItLines;
@@ -133,10 +133,10 @@ ArmGicGetMaxNumInterrupts (
 VOID
 EFIAPI
 ArmGicSendSgiTo (
-  IN  INTN  GicDistributorBase,
-  IN  INTN  TargetListFilter,
-  IN  INTN  CPUTargetList,
-  IN  INTN  SgiId
+  IN  UINTN  GicDistributorBase,
+  IN  INTN   TargetListFilter,
+  IN  INTN   CPUTargetList,
+  IN  INTN   SgiId
   )
 {
   MmioWrite32 (
@@ -390,7 +390,7 @@ ArmGicIsInterruptEnabled (
 VOID
 EFIAPI
 ArmGicDisableDistributor (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   )
 {
   // Disable Gic Distributor
diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
index aa4f0e2123929e0a86626b0f068d474065ca67fb..c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -13,7 +13,7 @@
 VOID
 EFIAPI
 ArmGicEnableDistributor (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   )
 {
   ARM_GIC_ARCH_REVISION  Revision;
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index 25290342bde4de907bef050d6f1bdd6e03f8dccc..b7d67d830e46b663e4054990e7456660fb22cda9 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -2,7 +2,7 @@
 
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
-Portions copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
+Portions copyright (c) 2011-2021, Arm Ltd. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -401,7 +401,7 @@ GicV2DxeInitialize (
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
   mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
-  mGicDistributorBase        = PcdGet64 (PcdGicDistributorBase);
+  mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
   mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
 
   for (Index = 0; Index < mGicNumInterrupts; Index++) {
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index b1f0cd48c752666e8b01eb5a25f8639e49213119..30c3fcbc3eee8e4f41f68a669cbc59650c12ca89 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -381,7 +381,7 @@ GicV3DxeInitialize (
   // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
-  mGicDistributorBase    = PcdGet64 (PcdGicDistributorBase);
+  mGicDistributorBase    = (UINTN)PcdGet64 (PcdGicDistributorBase);
   mGicRedistributorsBase = PcdGet64 (PcdGicRedistributorsBase);
   mGicNumInterrupts      = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
 
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 4ab670967598f21852e46f72116bf4c78ca7dd44..72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -121,7 +121,7 @@ VOID
 EFIAPI
 ArmGicSetupNonSecure (
   IN  UINTN  MpId,
-  IN  INTN   GicDistributorBase,
+  IN  UINTN  GicDistributorBase,
   IN  INTN   GicInterruptInterfaceBase
   );
 
@@ -148,28 +148,28 @@ ArmGicDisableInterruptInterface (
 VOID
 EFIAPI
 ArmGicEnableDistributor (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   );
 
 VOID
 EFIAPI
 ArmGicDisableDistributor (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   );
 
 UINTN
 EFIAPI
 ArmGicGetMaxNumInterrupts (
-  IN  INTN  GicDistributorBase
+  IN  UINTN  GicDistributorBase
   );
 
 VOID
 EFIAPI
 ArmGicSendSgiTo (
-  IN  INTN  GicDistributorBase,
-  IN  INTN  TargetListFilter,
-  IN  INTN  CPUTargetList,
-  IN  INTN  SgiId
+  IN  UINTN  GicDistributorBase,
+  IN  INTN   TargetListFilter,
+  IN  INTN   CPUTargetList,
+  IN  INTN   SgiId
   );
 
 /*
@@ -251,7 +251,7 @@ VOID
 EFIAPI
 ArmGicV2SetupNonSecure (
   IN  UINTN  MpId,
-  IN  INTN   GicDistributorBase,
+  IN  UINTN  GicDistributorBase,
   IN  INTN   GicInterruptInterfaceBase
   );
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The data type used by variables representing the GicDistributorBase
> has been inconsistently used in the ArmGic driver and the library.
> The PCD defined for the GIC Distributor base address is UINT64.
> However, the data types for the variables used is UINTN, INTN, and
> at some places UINT32.
>
> Therefore, update the data types to use UINTN and add necessary
> typecasts when reading values from the PCD. This should then be
> consistent across AArch32 and AArch64 builds.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  4 ++--
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 12 ++++++------
>  ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c   |  4 ++--
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c |  4 ++--
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c |  4 ++--
>  ArmPkg/Include/Library/ArmGicLib.h        | 18 +++++++++---------
>  6 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> index d560c42fc9f3d5e86c2aece504102f43cb841877..9ac073db36ce92fc14de71e9a264059afd63d729 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
> @@ -1,6 +1,6 @@
>  /*++
>
> -Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2013-2021, Arm Ltd. All rights reserved.<BR>

off by 2 ?

>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -61,7 +61,7 @@ GicGetDistributorIcfgBaseAndBit (
>
>    RegIndex    = Source / ARM_GIC_ICDICFR_F_STRIDE; // NOTE: truncation is significant
>    Field       = Source % ARM_GIC_ICDICFR_F_STRIDE;
> -  *RegAddress = PcdGet64 (PcdGicDistributorBase)
> +  *RegAddress = (UINTN)PcdGet64 (PcdGicDistributorBase)
>                  + ARM_GIC_ICDICFR
>                  + (ARM_GIC_ICDICFR_BYTES * RegIndex);
>    *Config1Bit = ((Field * ARM_GIC_ICDICFR_F_WIDTH)
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index dd3670c7ccbb18586bb28f4ac02514055471529f..6e44e89390fcdaa89302d6505f75c43c84ce3535 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -117,7 +117,7 @@ ArmGicGetInterfaceIdentification (
>  UINTN
>  EFIAPI
>  ArmGicGetMaxNumInterrupts (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    )
>  {
>    UINTN  ItLines;
> @@ -133,10 +133,10 @@ ArmGicGetMaxNumInterrupts (
>  VOID
>  EFIAPI
>  ArmGicSendSgiTo (
> -  IN  INTN  GicDistributorBase,
> -  IN  INTN  TargetListFilter,
> -  IN  INTN  CPUTargetList,
> -  IN  INTN  SgiId
> +  IN  UINTN  GicDistributorBase,
> +  IN  INTN   TargetListFilter,
> +  IN  INTN   CPUTargetList,
> +  IN  INTN   SgiId
>    )
>  {
>    MmioWrite32 (
> @@ -390,7 +390,7 @@ ArmGicIsInterruptEnabled (
>  VOID
>  EFIAPI
>  ArmGicDisableDistributor (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    )
>  {
>    // Disable Gic Distributor
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> index aa4f0e2123929e0a86626b0f068d474065ca67fb..c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -13,7 +13,7 @@
>  VOID
>  EFIAPI
>  ArmGicEnableDistributor (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    )
>  {
>    ARM_GIC_ARCH_REVISION  Revision;
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index 25290342bde4de907bef050d6f1bdd6e03f8dccc..b7d67d830e46b663e4054990e7456660fb22cda9 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -2,7 +2,7 @@
>
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
> -Portions copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
> +Portions copyright (c) 2011-2021, Arm Ltd. All rights reserved.<BR>
>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -401,7 +401,7 @@ GicV2DxeInitialize (
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>
>    mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
> -  mGicDistributorBase        = PcdGet64 (PcdGicDistributorBase);
> +  mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
>    mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>
>    for (Index = 0; Index < mGicNumInterrupts; Index++) {
> diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> index b1f0cd48c752666e8b01eb5a25f8639e49213119..30c3fcbc3eee8e4f41f68a669cbc59650c12ca89 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2018, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -381,7 +381,7 @@ GicV3DxeInitialize (
>    // the system.
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>
> -  mGicDistributorBase    = PcdGet64 (PcdGicDistributorBase);
> +  mGicDistributorBase    = (UINTN)PcdGet64 (PcdGicDistributorBase);
>    mGicRedistributorsBase = PcdGet64 (PcdGicRedistributorsBase);
>    mGicNumInterrupts      = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index 4ab670967598f21852e46f72116bf4c78ca7dd44..72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -121,7 +121,7 @@ VOID
>  EFIAPI
>  ArmGicSetupNonSecure (
>    IN  UINTN  MpId,
> -  IN  INTN   GicDistributorBase,
> +  IN  UINTN  GicDistributorBase,
>    IN  INTN   GicInterruptInterfaceBase
>    );
>
> @@ -148,28 +148,28 @@ ArmGicDisableInterruptInterface (
>  VOID
>  EFIAPI
>  ArmGicEnableDistributor (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicDisableDistributor (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    );
>
>  UINTN
>  EFIAPI
>  ArmGicGetMaxNumInterrupts (
> -  IN  INTN  GicDistributorBase
> +  IN  UINTN  GicDistributorBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicSendSgiTo (
> -  IN  INTN  GicDistributorBase,
> -  IN  INTN  TargetListFilter,
> -  IN  INTN  CPUTargetList,
> -  IN  INTN  SgiId
> +  IN  UINTN  GicDistributorBase,
> +  IN  INTN   TargetListFilter,
> +  IN  INTN   CPUTargetList,
> +  IN  INTN   SgiId
>    );
>
>  /*
> @@ -251,7 +251,7 @@ VOID
>  EFIAPI
>  ArmGicV2SetupNonSecure (
>    IN  UINTN  MpId,
> -  IN  INTN   GicDistributorBase,
> +  IN  UINTN  GicDistributorBase,
>    IN  INTN   GicInterruptInterfaceBase
>    );
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

[PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase

[Sami Mujawar]

The data type used by variables representing the
GicInterruptInterfaceBase has been inconsistently
used in the ArmGic driver and the library.
The PCD defined for the GIC Interrupt interface
base address is UINT64. However, the data types
for the variables used is UINTN, INTN, and at
some places UINT32.

Therefore, update the data types to use UINTN and
add necessary typecasts when reading values from
the PCD. This should then be consistent across
AArch32 and AArch64 builds.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 13 ++++++++++---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       |  2 +-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +++---
 ArmPkg/Include/Library/ArmGicLib.h              | 18 +++++++++---------
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 6e44e89390fcdaa89302d6505f75c43c84ce3535..78edc7e76a087caa5b91d896f9bd316d6530a668 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -104,10 +104,17 @@ GicGetCpuRedistributorBase (
   return 0;
 }
 
+/**
+  Return the GIC CPU Interrupt Interface ID.
+
+  @param GicInterruptInterfaceBase  Base address of the GIC Interrupt Interface.
+
+  @retval CPU Interface Identification information.
+**/
 UINTN
 EFIAPI
 ArmGicGetInterfaceIdentification (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   )
 {
   // Read the GIC Identification Register
@@ -400,7 +407,7 @@ ArmGicDisableDistributor (
 VOID
 EFIAPI
 ArmGicEnableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   )
 {
   ARM_GIC_ARCH_REVISION  Revision;
@@ -418,7 +425,7 @@ ArmGicEnableInterruptInterface (
 VOID
 EFIAPI
 ArmGicDisableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   )
 {
   ARM_GIC_ARCH_REVISION  Revision;
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index b7d67d830e46b663e4054990e7456660fb22cda9..b952c3ae31c060ecbb43c0800d34e57664a8262a 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -400,7 +400,7 @@ GicV2DxeInitialize (
   // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
-  mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
+  mGicInterruptInterfaceBase = (UINTN)PcdGet64 (PcdGicInterruptInterfaceBase);
   mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
   mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
 
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
index 85c2a920a54a1acaccb98a94b5591ce36d20697c..832f21644233655ef2f359f1e175071d2a493b7c 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -13,7 +13,7 @@
 VOID
 EFIAPI
 ArmGicV2EnableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   )
 {
   /*
@@ -26,7 +26,7 @@ ArmGicV2EnableInterruptInterface (
 VOID
 EFIAPI
 ArmGicV2DisableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   )
 {
   // Disable Gic Interface
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28..41bbf1da6a6cbb683df4bb30c4b1a1762dc7814f 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -113,7 +113,7 @@
 UINTN
 EFIAPI
 ArmGicGetInterfaceIdentification (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 // GIC Secure interfaces
@@ -122,7 +122,7 @@ EFIAPI
 ArmGicSetupNonSecure (
   IN  UINTN  MpId,
   IN  UINTN  GicDistributorBase,
-  IN  INTN   GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 VOID
@@ -136,13 +136,13 @@ ArmGicSetSecureInterrupts (
 VOID
 EFIAPI
 ArmGicEnableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 VOID
 EFIAPI
 ArmGicDisableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 VOID
@@ -203,8 +203,8 @@ ArmGicEndOfInterrupt (
 UINTN
 EFIAPI
 ArmGicSetPriorityMask (
-  IN  INTN  GicInterruptInterfaceBase,
-  IN  INTN  PriorityMask
+  IN  UINTN  GicInterruptInterfaceBase,
+  IN  INTN   PriorityMask
   );
 
 VOID
@@ -252,19 +252,19 @@ EFIAPI
 ArmGicV2SetupNonSecure (
   IN  UINTN  MpId,
   IN  UINTN  GicDistributorBase,
-  IN  INTN   GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 VOID
 EFIAPI
 ArmGicV2EnableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 VOID
 EFIAPI
 ArmGicV2DisableInterruptInterface (
-  IN  INTN  GicInterruptInterfaceBase
+  IN  UINTN  GicInterruptInterfaceBase
   );
 
 UINTN
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase

[Pedro Falcato]

On Tue, May 23, 2023 at 2:04 PM Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The data type used by variables representing the
> GicInterruptInterfaceBase has been inconsistently
> used in the ArmGic driver and the library.
> The PCD defined for the GIC Interrupt interface
> base address is UINT64. However, the data types
> for the variables used is UINTN, INTN, and at
> some places UINT32.
>
> Therefore, update the data types to use UINTN and
> add necessary typecasts when reading values from
> the PCD. This should then be consistent across
> AArch32 and AArch64 builds.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 13 ++++++++++---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       |  2 +-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +++---
>  ArmPkg/Include/Library/ArmGicLib.h              | 18 +++++++++---------
>  4 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 6e44e89390fcdaa89302d6505f75c43c84ce3535..78edc7e76a087caa5b91d896f9bd316d6530a668 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -104,10 +104,17 @@ GicGetCpuRedistributorBase (
>    return 0;
>  }
>
> +/**
> +  Return the GIC CPU Interrupt Interface ID.
> +
> +  @param GicInterruptInterfaceBase  Base address of the GIC Interrupt Interface.
> +
> +  @retval CPU Interface Identification information.
> +**/
>  UINTN
>  EFIAPI
>  ArmGicGetInterfaceIdentification (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    // Read the GIC Identification Register
> @@ -400,7 +407,7 @@ ArmGicDisableDistributor (
>  VOID
>  EFIAPI
>  ArmGicEnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    ARM_GIC_ARCH_REVISION  Revision;
> @@ -418,7 +425,7 @@ ArmGicEnableInterruptInterface (
>  VOID
>  EFIAPI
>  ArmGicDisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    ARM_GIC_ARCH_REVISION  Revision;
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index b7d67d830e46b663e4054990e7456660fb22cda9..b952c3ae31c060ecbb43c0800d34e57664a8262a 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -400,7 +400,7 @@ GicV2DxeInitialize (
>    // the system.
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>
> -  mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
> +  mGicInterruptInterfaceBase = (UINTN)PcdGet64 (PcdGicInterruptInterfaceBase);
>    mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
>    mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>
ASSERT'ing for PCD <= MAX_UINTN may be desirable here, for both bases?

> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> index 85c2a920a54a1acaccb98a94b5591ce36d20697c..832f21644233655ef2f359f1e175071d2a493b7c 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -13,7 +13,7 @@
>  VOID
>  EFIAPI
>  ArmGicV2EnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    /*
> @@ -26,7 +26,7 @@ ArmGicV2EnableInterruptInterface (
>  VOID
>  EFIAPI
>  ArmGicV2DisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    // Disable Gic Interface
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index 72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28..41bbf1da6a6cbb683df4bb30c4b1a1762dc7814f 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -113,7 +113,7 @@
>  UINTN
>  EFIAPI
>  ArmGicGetInterfaceIdentification (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  // GIC Secure interfaces
> @@ -122,7 +122,7 @@ EFIAPI
>  ArmGicSetupNonSecure (
>    IN  UINTN  MpId,
>    IN  UINTN  GicDistributorBase,
> -  IN  INTN   GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
> @@ -136,13 +136,13 @@ ArmGicSetSecureInterrupts (
>  VOID
>  EFIAPI
>  ArmGicEnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicDisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
> @@ -203,8 +203,8 @@ ArmGicEndOfInterrupt (
>  UINTN
>  EFIAPI
>  ArmGicSetPriorityMask (
> -  IN  INTN  GicInterruptInterfaceBase,
> -  IN  INTN  PriorityMask
> +  IN  UINTN  GicInterruptInterfaceBase,
> +  IN  INTN   PriorityMask
>    );
>
>  VOID
> @@ -252,19 +252,19 @@ EFIAPI
>  ArmGicV2SetupNonSecure (
>    IN  UINTN  MpId,
>    IN  UINTN  GicDistributorBase,
> -  IN  INTN   GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicV2EnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicV2DisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  UINTN
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>


-- 
Pedro

Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase

[Sami Mujawar]

Hi Pedro,

Thank you for the feedback.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 23/05/2023 02:35 pm, Pedro Falcato wrote:
> On Tue, May 23, 2023 at 2:04 PM Sami Mujawar <sami.mujawar@arm.com> wrote:
>> The data type used by variables representing the
>> GicInterruptInterfaceBase has been inconsistently
>> used in the ArmGic driver and the library.
>> The PCD defined for the GIC Interrupt interface
>> base address is UINT64. However, the data types
>> for the variables used is UINTN, INTN, and at
>> some places UINT32.
>>
>> Therefore, update the data types to use UINTN and
>> add necessary typecasts when reading values from
>> the PCD. This should then be consistent across
>> AArch32 and AArch64 builds.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 13 ++++++++++---
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       |  2 +-
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +++---
>>   ArmPkg/Include/Library/ArmGicLib.h              | 18 +++++++++---------
>>   4 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> index 6e44e89390fcdaa89302d6505f75c43c84ce3535..78edc7e76a087caa5b91d896f9bd316d6530a668 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
>> @@ -104,10 +104,17 @@ GicGetCpuRedistributorBase (
>>     return 0;
>>   }
>>
>> +/**
>> +  Return the GIC CPU Interrupt Interface ID.
>> +
>> +  @param GicInterruptInterfaceBase  Base address of the GIC Interrupt Interface.
>> +
>> +  @retval CPU Interface Identification information.
>> +**/
>>   UINTN
>>   EFIAPI
>>   ArmGicGetInterfaceIdentification (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     )
>>   {
>>     // Read the GIC Identification Register
>> @@ -400,7 +407,7 @@ ArmGicDisableDistributor (
>>   VOID
>>   EFIAPI
>>   ArmGicEnableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     )
>>   {
>>     ARM_GIC_ARCH_REVISION  Revision;
>> @@ -418,7 +425,7 @@ ArmGicEnableInterruptInterface (
>>   VOID
>>   EFIAPI
>>   ArmGicDisableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     )
>>   {
>>     ARM_GIC_ARCH_REVISION  Revision;
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> index b7d67d830e46b663e4054990e7456660fb22cda9..b952c3ae31c060ecbb43c0800d34e57664a8262a 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
>> @@ -400,7 +400,7 @@ GicV2DxeInitialize (
>>     // the system.
>>     ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>>
>> -  mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
>> +  mGicInterruptInterfaceBase = (UINTN)PcdGet64 (PcdGicInterruptInterfaceBase);
>>     mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
>>     mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>>
> ASSERT'ing for PCD <= MAX_UINTN may be desirable here, for both bases?
[SAMI] I will address this in the v2 series.
>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
>> index 85c2a920a54a1acaccb98a94b5591ce36d20697c..832f21644233655ef2f359f1e175071d2a493b7c 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
>> @@ -1,6 +1,6 @@
>>   /** @file
>>   *
>> -*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
>> +*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>>   *
>>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>>   *
>> @@ -13,7 +13,7 @@
>>   VOID
>>   EFIAPI
>>   ArmGicV2EnableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     )
>>   {
>>     /*
>> @@ -26,7 +26,7 @@ ArmGicV2EnableInterruptInterface (
>>   VOID
>>   EFIAPI
>>   ArmGicV2DisableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     )
>>   {
>>     // Disable Gic Interface
>> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
>> index 72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28..41bbf1da6a6cbb683df4bb30c4b1a1762dc7814f 100644
>> --- a/ArmPkg/Include/Library/ArmGicLib.h
>> +++ b/ArmPkg/Include/Library/ArmGicLib.h
>> @@ -113,7 +113,7 @@
>>   UINTN
>>   EFIAPI
>>   ArmGicGetInterfaceIdentification (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   // GIC Secure interfaces
>> @@ -122,7 +122,7 @@ EFIAPI
>>   ArmGicSetupNonSecure (
>>     IN  UINTN  MpId,
>>     IN  UINTN  GicDistributorBase,
>> -  IN  INTN   GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   VOID
>> @@ -136,13 +136,13 @@ ArmGicSetSecureInterrupts (
>>   VOID
>>   EFIAPI
>>   ArmGicEnableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   VOID
>>   EFIAPI
>>   ArmGicDisableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   VOID
>> @@ -203,8 +203,8 @@ ArmGicEndOfInterrupt (
>>   UINTN
>>   EFIAPI
>>   ArmGicSetPriorityMask (
>> -  IN  INTN  GicInterruptInterfaceBase,
>> -  IN  INTN  PriorityMask
>> +  IN  UINTN  GicInterruptInterfaceBase,
>> +  IN  INTN   PriorityMask
>>     );
>>
>>   VOID
>> @@ -252,19 +252,19 @@ EFIAPI
>>   ArmGicV2SetupNonSecure (
>>     IN  UINTN  MpId,
>>     IN  UINTN  GicDistributorBase,
>> -  IN  INTN   GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   VOID
>>   EFIAPI
>>   ArmGicV2EnableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   VOID
>>   EFIAPI
>>   ArmGicV2DisableInterruptInterface (
>> -  IN  INTN  GicInterruptInterfaceBase
>> +  IN  UINTN  GicInterruptInterfaceBase
>>     );
>>
>>   UINTN
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>

Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The data type used by variables representing the
> GicInterruptInterfaceBase has been inconsistently
> used in the ArmGic driver and the library.
> The PCD defined for the GIC Interrupt interface
> base address is UINT64. However, the data types
> for the variables used is UINTN, INTN, and at
> some places UINT32.
>
> Therefore, update the data types to use UINTN and
> add necessary typecasts when reading values from
> the PCD. This should then be consistent across
> AArch32 and AArch64 builds.
>

OK, so in general, I would prefer EFI_PHYSICAL_ADDRESS to represent
MMIO register addresses, given that those are physical addresses.

However, as nobody in their right mind would create a 32-bit CPU with
its GIC registers located at an address that is not 32-bit
addressable, using UINTN is acceptable here, as the codegen will be
much nicer on 32-bit ARM.

And INTN is obviously not the right type here.

> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

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

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 13 ++++++++++---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       |  2 +-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +++---
>  ArmPkg/Include/Library/ArmGicLib.h              | 18 +++++++++---------
>  4 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index 6e44e89390fcdaa89302d6505f75c43c84ce3535..78edc7e76a087caa5b91d896f9bd316d6530a668 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -104,10 +104,17 @@ GicGetCpuRedistributorBase (
>    return 0;
>  }
>
> +/**
> +  Return the GIC CPU Interrupt Interface ID.
> +
> +  @param GicInterruptInterfaceBase  Base address of the GIC Interrupt Interface.
> +
> +  @retval CPU Interface Identification information.
> +**/
>  UINTN
>  EFIAPI
>  ArmGicGetInterfaceIdentification (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    // Read the GIC Identification Register
> @@ -400,7 +407,7 @@ ArmGicDisableDistributor (
>  VOID
>  EFIAPI
>  ArmGicEnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    ARM_GIC_ARCH_REVISION  Revision;
> @@ -418,7 +425,7 @@ ArmGicEnableInterruptInterface (
>  VOID
>  EFIAPI
>  ArmGicDisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    ARM_GIC_ARCH_REVISION  Revision;
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> index b7d67d830e46b663e4054990e7456660fb22cda9..b952c3ae31c060ecbb43c0800d34e57664a8262a 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
> @@ -400,7 +400,7 @@ GicV2DxeInitialize (
>    // the system.
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
>
> -  mGicInterruptInterfaceBase = PcdGet64 (PcdGicInterruptInterfaceBase);
> +  mGicInterruptInterfaceBase = (UINTN)PcdGet64 (PcdGicInterruptInterfaceBase);
>    mGicDistributorBase        = (UINTN)PcdGet64 (PcdGicDistributorBase);
>    mGicNumInterrupts          = ArmGicGetMaxNumInterrupts (mGicDistributorBase);
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> index 85c2a920a54a1acaccb98a94b5591ce36d20697c..832f21644233655ef2f359f1e175071d2a493b7c 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -13,7 +13,7 @@
>  VOID
>  EFIAPI
>  ArmGicV2EnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    /*
> @@ -26,7 +26,7 @@ ArmGicV2EnableInterruptInterface (
>  VOID
>  EFIAPI
>  ArmGicV2DisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    )
>  {
>    // Disable Gic Interface
> diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
> index 72dbd1ca8d626c69d9bb8727d77fd34b4ab3af28..41bbf1da6a6cbb683df4bb30c4b1a1762dc7814f 100644
> --- a/ArmPkg/Include/Library/ArmGicLib.h
> +++ b/ArmPkg/Include/Library/ArmGicLib.h
> @@ -113,7 +113,7 @@
>  UINTN
>  EFIAPI
>  ArmGicGetInterfaceIdentification (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  // GIC Secure interfaces
> @@ -122,7 +122,7 @@ EFIAPI
>  ArmGicSetupNonSecure (
>    IN  UINTN  MpId,
>    IN  UINTN  GicDistributorBase,
> -  IN  INTN   GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
> @@ -136,13 +136,13 @@ ArmGicSetSecureInterrupts (
>  VOID
>  EFIAPI
>  ArmGicEnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicDisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
> @@ -203,8 +203,8 @@ ArmGicEndOfInterrupt (
>  UINTN
>  EFIAPI
>  ArmGicSetPriorityMask (
> -  IN  INTN  GicInterruptInterfaceBase,
> -  IN  INTN  PriorityMask
> +  IN  UINTN  GicInterruptInterfaceBase,
> +  IN  INTN   PriorityMask
>    );
>
>  VOID
> @@ -252,19 +252,19 @@ EFIAPI
>  ArmGicV2SetupNonSecure (
>    IN  UINTN  MpId,
>    IN  UINTN  GicDistributorBase,
> -  IN  INTN   GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicV2EnableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  VOID
>  EFIAPI
>  ArmGicV2DisableInterruptInterface (
> -  IN  INTN  GicInterruptInterfaceBase
> +  IN  UINTN  GicInterruptInterfaceBase
>    );
>
>  UINTN
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

[PATCH v1 03/12] ArmPkg: Fix ArmGicSendSgiTo() parameters

[Sami Mujawar]

The Software Generated Interrupt Register (GICD_SGIR) is a
32 bit register with the following bit assignment:
  TargetListFilter, bits [25:24]
  CPUTargetList, bits [23:16]
  NSATT, bit [15]
  SGIINTID, bits [3:0]

Therefore, modify the TargetListFilter, CPUTargetList,
SGI Interrupt ID parameters of the ArmGicSendSgiTo ()
to use UINT8 instead of INTN.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c  | 6 +++---
 ArmPkg/Include/Library/ArmGicLib.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 78edc7e76a087caa5b91d896f9bd316d6530a668..2a5e22e7b68f7c44adbf8a3f26b2b7ec04849b96 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -141,9 +141,9 @@ VOID
 EFIAPI
 ArmGicSendSgiTo (
   IN  UINTN  GicDistributorBase,
-  IN  INTN   TargetListFilter,
-  IN  INTN   CPUTargetList,
-  IN  INTN   SgiId
+  IN  UINT8  TargetListFilter,
+  IN  UINT8  CPUTargetList,
+  IN  UINT8  SgiId
   )
 {
   MmioWrite32 (
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 41bbf1da6a6cbb683df4bb30c4b1a1762dc7814f..1b879708f84315035723d77c5301279c8130bd51 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -167,9 +167,9 @@ VOID
 EFIAPI
 ArmGicSendSgiTo (
   IN  UINTN  GicDistributorBase,
-  IN  INTN   TargetListFilter,
-  IN  INTN   CPUTargetList,
-  IN  INTN   SgiId
+  IN  UINT8  TargetListFilter,
+  IN  UINT8  CPUTargetList,
+  IN  UINT8  SgiId
   );
 
 /*
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

[Sami Mujawar]

According to edk2 coding standard specification, Non-Boolean
comparisons must use a compare operator (==, !=, >, < >=, <=).
See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
edk-ii-c-coding-standards-specification/5_source_files/
57_c_programming

Therefore, fix the comparison in ArmGicEnableDistributor()

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
@@ -26,7 +26,10 @@ ArmGicEnableDistributor (
   if (Revision == ARM_GIC_ARCH_REVISION_2) {
     MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
   } else {
-    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
+    if ((MmioRead32 (
+           GicDistributorBase + ARM_GIC_ICDDCR
+           ) & ARM_GIC_ICDDCR_ARE) != 0)
+    {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
     } else {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> According to edk2 coding standard specification, Non-Boolean
> comparisons must use a compare operator (==, !=, >, < >=, <=).
> See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
> edk-ii-c-coding-standards-specification/5_source_files/
> 57_c_programming
>
> Therefore, fix the comparison in ArmGicEnableDistributor()
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> @@ -26,7 +26,10 @@ ArmGicEnableDistributor (
>    if (Revision == ARM_GIC_ARCH_REVISION_2) {
>      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>    } else {
> -    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
> +    if ((MmioRead32 (
> +           GicDistributorBase + ARM_GIC_ICDDCR
> +           ) & ARM_GIC_ICDDCR_ARE) != 0)
> +    {

This coding style is terrible. Mind using a temporary variable for the
result of the MmioRead32() instead?

>        MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
>      } else {
>        MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

Re: [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

[Sami Mujawar]

Hi Ard,

Thank you for the feedback.

I will fix this in the v2 series.

Regards,

Sami Mujawar

On 23/05/2023 02:32 pm, Ard Biesheuvel wrote:
> On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> According to edk2 coding standard specification, Non-Boolean
>> comparisons must use a compare operator (==, !=, >, < >=, <=).
>> See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
>> edk-ii-c-coding-standards-specification/5_source_files/
>> 57_c_programming
>>
>> Therefore, fix the comparison in ArmGicEnableDistributor()
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> @@ -26,7 +26,10 @@ ArmGicEnableDistributor (
>>     if (Revision == ARM_GIC_ARCH_REVISION_2) {
>>       MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>>     } else {
>> -    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
>> +    if ((MmioRead32 (
>> +           GicDistributorBase + ARM_GIC_ICDDCR
>> +           ) & ARM_GIC_ICDDCR_ARE) != 0)
>> +    {
> This coding style is terrible. Mind using a temporary variable for the
> result of the MmioRead32() instead?
>
>>         MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
>>       } else {
>>         MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>

[PATCH v1 05/12] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification

[Sami Mujawar]

The CPU Interface Identification Register (GICC_IIDR) is a 32-bit
register. Since ArmGicGetInterfaceIdentification () returns the
value read from the GICC_IIDR register, update the return type
for this function to UINT32.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c  | 2 +-
 ArmPkg/Include/Library/ArmGicLib.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 2a5e22e7b68f7c44adbf8a3f26b2b7ec04849b96..df61e3aad4a7899eaa888cb248ad2a285c7f317d 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -111,7 +111,7 @@ GicGetCpuRedistributorBase (
 
   @retval CPU Interface Identification information.
 **/
-UINTN
+UINT32
 EFIAPI
 ArmGicGetInterfaceIdentification (
   IN  UINTN  GicInterruptInterfaceBase
diff --git a/ArmPkg/Include/Library/ArmGicLib.h b/ArmPkg/Include/Library/ArmGicLib.h
index 1b879708f84315035723d77c5301279c8130bd51..49ea3422270bec90fa161593c14c7095b7598c53 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -110,7 +110,7 @@
 // Bit Mask for
 #define ARM_GIC_ICCIAR_ACKINTID  0x3FF
 
-UINTN
+UINT32
 EFIAPI
 ArmGicGetInterfaceIdentification (
   IN  UINTN  GicInterruptInterfaceBase
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 06/12] ArmPkg: Make variables used for GicInterrupt UINTN

[Sami Mujawar]

Although the maximum interrupt ID on GicV2 is 10bit
and for GicV3/4 is 24bit, and that the IAR and EOIR
registers of the Gic CPU interface are 32 bit; the
typedef HARDWARE_INTERRUPT_SOURCE is defined as
UINTN in
EmbeddedPkg\Include\Protocol\HardwareInterrupt.h

Therefore, use UINTN for Gic Interrupt variables
and use appropriate typecasts wherever needed.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index b952c3ae31c060ecbb43c0800d34e57664a8262a..fb40f56ff9231f3a28c3d90939bfd25ac3432f89 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -162,7 +162,7 @@ GicV2IrqInterruptHandler (
   IN EFI_SYSTEM_CONTEXT  SystemContext
   )
 {
-  UINT32                      GicInterrupt;
+  UINTN                       GicInterrupt;
   HARDWARE_INTERRUPT_HANDLER  InterruptHandler;
 
   GicInterrupt = ArmGicV2AcknowledgeInterrupt (mGicInterruptInterfaceBase);
@@ -349,8 +349,8 @@ GicV2ExitBootServicesEvent (
   IN VOID       *Context
   )
 {
-  UINTN   Index;
-  UINT32  GicInterrupt;
+  UINTN  Index;
+  UINTN  GicInterrupt;
 
   // Disable all the interrupts
   for (Index = 0; Index < mGicNumInterrupts; Index++) {
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

[Sami Mujawar]

The IAR register of the Gic CPU interface is 32 bit, while
the value returned by ArmGicV2AcknowledgeInterrupt() is
UINTN. Therefore, typecast the return value to UINTN before
returning.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
@@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
   )
 {
   // Read the Interrupt Acknowledge Register
-  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
+  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
 }
 
 VOID
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

[Pedro Falcato]

On Tue, May 23, 2023 at 2:04 PM Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The IAR register of the Gic CPU interface is 32 bit, while
> the value returned by ArmGicV2AcknowledgeInterrupt() is
> UINTN. Therefore, typecast the return value to UINTN before
> returning.

Since IAR is 32-bit, doesn't it make sense to return UINT32 here? Is
this for compatibility with GICv3 or...?

>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> @@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
>    )
>  {
>    // Read the Interrupt Acknowledge Register
> -  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
> +  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>  }
You don't need to cast from UINT32 to UINTN IMO. UINTN is, as far as I
know, always going to be at least 32-bits, so casting makes little
sense here, in my view, as UINTN >= UINT32.

-- 
Pedro

Re: [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

Hi Pedro,

Thank you for the feedback.

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 23/05/2023 02:32 pm, Pedro Falcato wrote:
> On Tue, May 23, 2023 at 2:04 PM Sami Mujawar<sami.mujawar@arm.com>  wrote:
>> The IAR register of the Gic CPU interface is 32 bit, while
>> the value returned by ArmGicV2AcknowledgeInterrupt() is
>> UINTN. Therefore, typecast the return value to UINTN before
>> returning.
> Since IAR is 32-bit, doesn't it make sense to return UINT32 here? Is
> this for compatibility with GICv3 or...?

[SAMI] IAR in GICv3 is 32-bit as well. I think this is done to keep 
compatibility with HARDWARE_INTERRUPT_SOURCE which is UINTN.

>> Signed-off-by: Sami Mujawar<sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> index f403bec367b5254c248e620e56471904e520f9f2..80115b243afabd5e4faad88089af738b19ce4cd1 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> @@ -16,7 +16,7 @@ ArmGicV2AcknowledgeInterrupt (
>>     )
>>   {
>>     // Read the Interrupt Acknowledge Register
>> -  return MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>> +  return (UINTN)MmioRead32 (GicInterruptInterfaceBase + ARM_GIC_ICCIAR);
>>   }
> You don't need to cast from UINT32 to UINTN IMO. UINTN is, as far as I
> know, always going to be at least 32-bits, so casting makes little
> sense here, in my view, as UINTN >= UINT32.
[SAMI] Ack. I will drop this patch.
>

[-- Attachment #2: Type: text/html, Size: 2897 bytes --]

[PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

[Sami Mujawar]

The EIOR register of the Gic CPU interface is a 32 bit register.
However, the HARDWARE_INTERRUPT_SOURCE used to represent the
interrupt source (Interrupt ID) is typedefed as UINTN, see
EmbeddedPkg\Include\Protocol\HardwareInterrupt.h

Therfore, typecast the interrupt ID (Source) value to UINT32
before setting the EOIR register. Also, add an assert to check
that the value does not exceed 32 bits.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
@@ -7,6 +7,7 @@
 **/
 
 #include <Library/ArmGicLib.h>
+#include <Library/DebugLib.h>
 #include <Library/IoLib.h>
 
 UINTN
@@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
   IN UINTN   Source
   )
 {
-  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
+  ASSERT (Source < MAX_UINT32);
+  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
 }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> The EIOR register of the Gic CPU interface is a 32 bit register.
> However, the HARDWARE_INTERRUPT_SOURCE used to represent the
> interrupt source (Interrupt ID) is typedefed as UINTN, see
> EmbeddedPkg\Include\Protocol\HardwareInterrupt.h
>
> Therfore, typecast the interrupt ID (Source) value to UINT32
> before setting the EOIR register. Also, add an assert to check
> that the value does not exceed 32 bits.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
> @@ -7,6 +7,7 @@
>  **/
>
>  #include <Library/ArmGicLib.h>
> +#include <Library/DebugLib.h>
>  #include <Library/IoLib.h>
>
>  UINTN
> @@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
>    IN UINTN   Source
>    )
>  {
> -  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
> +  ASSERT (Source < MAX_UINT32);

Should this be <= ?

> +  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
>
> 
>
>

Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt

[Sami Mujawar]

Hi Ard,

Thank you for the feedback.

I will fix this in the next series.

Regards,

Sami Mujawar

On 23/05/2023 02:26 pm, Ard Biesheuvel wrote:
> On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> The EIOR register of the Gic CPU interface is a 32 bit register.
>> However, the HARDWARE_INTERRUPT_SOURCE used to represent the
>> interrupt source (Interrupt ID) is typedefed as UINTN, see
>> EmbeddedPkg\Include\Protocol\HardwareInterrupt.h
>>
>> Therfore, typecast the interrupt ID (Source) value to UINT32
>> before setting the EOIR register. Also, add an assert to check
>> that the value does not exceed 32 bits.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> index 80115b243afabd5e4faad88089af738b19ce4cd1..e98cd9705616e7a8dfc7aaba7c80b176f8f6d0c9 100644
>> --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
>> @@ -7,6 +7,7 @@
>>   **/
>>
>>   #include <Library/ArmGicLib.h>
>> +#include <Library/DebugLib.h>
>>   #include <Library/IoLib.h>
>>
>>   UINTN
>> @@ -26,5 +27,6 @@ ArmGicV2EndOfInterrupt (
>>     IN UINTN   Source
>>     )
>>   {
>> -  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, Source);
>> +  ASSERT (Source < MAX_UINT32);
> Should this be <= ?
>
>> +  MmioWrite32 (GicInterruptInterfaceBase + ARM_GIC_ICCEIOR, (UINT32)Source);
>>   }
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>>
>>
>> 
>>
>>

[PATCH v1 09/12] ArmPkg: Remove unused function declarations

[Sami Mujawar]

The IrqInterruptHandler () and ExitBootServicesEvent () function
declarations were unused. Therefore, remove these declarations.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index 9ac073db36ce92fc14de71e9a264059afd63d729..26c3a06761d197c0047f9eee35725b488ec2a132 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
@@ -8,20 +8,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "ArmGicDxe.h"
 
-VOID
-EFIAPI
-IrqInterruptHandler (
-  IN EFI_EXCEPTION_TYPE  InterruptType,
-  IN EFI_SYSTEM_CONTEXT  SystemContext
-  );
-
-VOID
-EFIAPI
-ExitBootServicesEvent (
-  IN EFI_EVENT  Event,
-  IN VOID       *Context
-  );
-
 // Making this global saves a few bytes in image size
 EFI_HANDLE  gHardwareInterruptHandle = NULL;
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR

[Sami Mujawar]

GICD_SGIR is a 32-bit register, of which INTID is bits [3:0]
and Bits [14:4] is RES0. Since SgiId parameter in the function
ArmGicSendSgiTo () is UINT8, mask unused bits of SgiId before
writing to the GICD_SGIR register to prevent accidental setting
of the RES0 bits.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index df61e3aad4a7899eaa888cb248ad2a285c7f317d..0127cca3bf0567bc80702f415e9cbb9bd2709fbc 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
+*  Copyright (c) 2011-2023, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -148,7 +148,9 @@ ArmGicSendSgiTo (
 {
   MmioWrite32 (
     GicDistributorBase + ARM_GIC_ICDSGIR,
-    ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId
+    ((TargetListFilter & 0x3) << 24) |
+    ((CPUTargetList & 0xFF) << 16)   |
+    (SgiId & 0xF)
     );
 }
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR

[Ard Biesheuvel]

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> GICD_SGIR is a 32-bit register, of which INTID is bits [3:0]
> and Bits [14:4] is RES0. Since SgiId parameter in the function
> ArmGicSendSgiTo () is UINT8, mask unused bits of SgiId before
> writing to the GICD_SGIR register to prevent accidental setting
> of the RES0 bits.
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

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

> ---
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> index df61e3aad4a7899eaa888cb248ad2a285c7f317d..0127cca3bf0567bc80702f415e9cbb9bd2709fbc 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
> +*  Copyright (c) 2011-2023, Arm Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -148,7 +148,9 @@ ArmGicSendSgiTo (
>  {
>    MmioWrite32 (
>      GicDistributorBase + ARM_GIC_ICDSGIR,
> -    ((TargetListFilter & 0x3) << 24) | ((CPUTargetList & 0xFF) << 16) | SgiId
> +    ((TargetListFilter & 0x3) << 24) |
> +    ((CPUTargetList & 0xFF) << 16)   |
> +    (SgiId & 0xF)
>      );
>  }
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

[PATCH v1 11/12] ArmPkg: Adjust variable type and cast for RegShift & RegOffset

[Sami Mujawar]

According to the GIC architecture version 3 and 4 specification,
the maximum number of INTID bits supported in the CPU interface
is 24.

Considering this the RegShift variable is not required to be more
than 8 bits. Therefore, make the RegShift variable type to UINT8.
Also add necessary typecasts when calculating the RegOffset and
RegShift values.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 24 ++++++++++----------
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c |  8 +++----
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index 0127cca3bf0567bc80702f415e9cbb9bd2709fbc..8f3315d76f6f2b28a551d73400938430ff3e23c7 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -228,13 +228,13 @@ ArmGicSetInterruptPriority (
   )
 {
   UINT32                 RegOffset;
-  UINTN                  RegShift;
+  UINT8                  RegShift;
   ARM_GIC_ARCH_REVISION  Revision;
   UINTN                  GicCpuRedistributorBase;
 
   // Calculate register offset and bit position
-  RegOffset = Source / 4;
-  RegShift  = (Source % 4) * 8;
+  RegOffset = (UINT32)(Source / 4);
+  RegShift  = (UINT8)((Source % 4) * 8);
 
   Revision = ArmGicGetSupportedArchRevision ();
   if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
@@ -272,13 +272,13 @@ ArmGicEnableInterrupt (
   )
 {
   UINT32                 RegOffset;
-  UINTN                  RegShift;
+  UINT8                  RegShift;
   ARM_GIC_ARCH_REVISION  Revision;
   UINTN                  GicCpuRedistributorBase;
 
   // Calculate enable register offset and bit position
-  RegOffset = Source / 32;
-  RegShift  = Source % 32;
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = (UINT8)(Source % 32);
 
   Revision = ArmGicGetSupportedArchRevision ();
   if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
@@ -317,13 +317,13 @@ ArmGicDisableInterrupt (
   )
 {
   UINT32                 RegOffset;
-  UINTN                  RegShift;
+  UINT8                  RegShift;
   ARM_GIC_ARCH_REVISION  Revision;
   UINTN                  GicCpuRedistributorBase;
 
   // Calculate enable register offset and bit position
-  RegOffset = Source / 32;
-  RegShift  = Source % 32;
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = (UINT8)(Source % 32);
 
   Revision = ArmGicGetSupportedArchRevision ();
   if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
@@ -361,14 +361,14 @@ ArmGicIsInterruptEnabled (
   )
 {
   UINT32                 RegOffset;
-  UINTN                  RegShift;
+  UINT8                  RegShift;
   ARM_GIC_ARCH_REVISION  Revision;
   UINTN                  GicCpuRedistributorBase;
   UINT32                 Interrupts;
 
   // Calculate enable register offset and bit position
-  RegOffset = Source / 32;
-  RegShift  = Source % 32;
+  RegOffset = (UINT32)(Source / 32);
+  RegShift  = (UINT8)(Source % 32);
 
   Revision = ArmGicGetSupportedArchRevision ();
   if ((Revision == ARM_GIC_ARCH_REVISION_2) ||
diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index fb40f56ff9231f3a28c3d90939bfd25ac3432f89..ba43150fe57015994573d82c88d6a8d9f3174533 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -2,7 +2,7 @@
 
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
-Portions copyright (c) 2011-2021, Arm Ltd. All rights reserved.<BR>
+Portions copyright (c) 2011-2023, Arm Ltd. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -393,7 +393,7 @@ GicV2DxeInitialize (
   EFI_STATUS  Status;
   UINTN       Index;
   UINT32      RegOffset;
-  UINTN       RegShift;
+  UINT8       RegShift;
   UINT32      CpuTarget;
 
   // Make sure the Interrupt Controller Protocol is not already installed in
@@ -408,8 +408,8 @@ GicV2DxeInitialize (
     GicV2DisableInterruptSource (&gHardwareInterruptV2Protocol, Index);
 
     // Set Priority
-    RegOffset = Index / 4;
-    RegShift  = (Index % 4) * 8;
+    RegOffset = (UINT32)(Index / 4);
+    RegShift  = (UINT8)((Index % 4) * 8);
     MmioAndThenOr32 (
       mGicDistributorBase + ARM_GIC_ICDIPR + (4 * RegOffset),
       ~(0xff << RegShift),
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Sami Mujawar]

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)'

Re: [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3

[Ard Biesheuvel]

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)'
>