public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements
@ 2023-05-24 13:38 Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 01/11] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
                   ` (11 more replies)
  0 siblings, 12 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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)

This v2 series address the feedback received for the
v1 series and additionally updates the patch 6/11
'ArmPkg: Make variables used for GicInterrupt UINTN'
to extend the changes to GicV3IrqInterruptHandler ().

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

Sami Mujawar (11):
  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: 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         |  8 ++-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       | 23 +++---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c       |  6 +-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +-
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c       |  8 +--
 ArmPkg/Include/Library/ArmGicLib.h              | 40 +++++------
 8 files changed, 94 insertions(+), 88 deletions(-)

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 01/11] ArmPkg: Fix data type used for GicDistributorBase
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 02/11] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - Update Copyright year                               [Ard]
     - Assert if PcdGicDistributorBase <= UINTN            [Pedro]
     - Updated to add copyright year and assert            [Sami]
     - Ref: https://edk2.groups.io/g/devel/message/105188
            https://edk2.groups.io/g/devel/message/105191

 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c   |  4 ++--
 ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 14 +++++++-------
 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c   |  4 ++--
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c |  6 ++++--
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c |  4 ++--
 ArmPkg/Include/Library/ArmGicLib.h        | 20 ++++++++++----------
 6 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c
index d560c42fc9f3d5e86c2aece504102f43cb841877..8461fb7927eaf97c75135205f444d33c205910db 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-2023, 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..e26035a90201a7cd3025537d9351cc30019090b6 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
 *
@@ -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..38bbf2e9f81527b2545a0116120ceee56af17808 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-2023, 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..1c2061181e83bcf3f91d7bd13056f0413e212c37 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-2023, Arm Ltd. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -400,8 +400,10 @@ GicV2DxeInitialize (
   // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
+  ASSERT (PcdGet64 (PcdGicDistributorBase) <= MAX_UINTN);
+
   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..719701a67e756c2d2aeae8fc23d50b519a1997aa 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-2023, 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..928d1541d9d6bd603ea687a7814fb31c35e14a8d 100644
--- a/ArmPkg/Include/Library/ArmGicLib.h
+++ b/ArmPkg/Include/Library/ArmGicLib.h
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2021, Arm Limited. All rights reserved.<BR>
+*  Copyright (c) 2011-2023, Arm Limited. All rights reserved.<BR>
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 02/11] ArmPkg: Fix data type used for GicInterruptInterfaceBase
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 01/11] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 03/11] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
---

Notes:
    v2:
     - Assert if PcdGicInterruptInterfaceBase <= UINTN     [Pedro]
     - Updated to add copyright year and assert            [Sami]
     - Ref: https://edk2.groups.io/g/devel/message/105191

 ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 13 ++++++++++---
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       |  3 ++-
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +++---
 ArmPkg/Include/Library/ArmGicLib.h              | 18 +++++++++---------
 4 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index e26035a90201a7cd3025537d9351cc30019090b6..2432715e43fa40ba6780909a83eaf7a6f8e791fc 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 1c2061181e83bcf3f91d7bd13056f0413e212c37..a1670021c30f05707d7c37a789fdbacc4ffa9140 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -400,9 +400,10 @@ GicV2DxeInitialize (
   // the system.
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gHardwareInterruptProtocolGuid);
 
+  ASSERT (PcdGet64 (PcdGicInterruptInterfaceBase) <= MAX_UINTN);
   ASSERT (PcdGet64 (PcdGicDistributorBase) <= MAX_UINTN);
 
-  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..781645e8ea68dfcbf83edeb63823605ede2bc067 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-2023, 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 928d1541d9d6bd603ea687a7814fb31c35e14a8d..7253cda5b8f01193d3439061ccd903868ed2e145 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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 03/11] ArmPkg: Fix ArmGicSendSgiTo() parameters
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 01/11] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 02/11] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 04/11] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - no code change since v1 series                    [Sami]

 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 2432715e43fa40ba6780909a83eaf7a6f8e791fc..eca4ddb7d3a40d5ef2fc2d0433c50afeb64c4b24 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 7253cda5b8f01193d3439061ccd903868ed2e145..cede7a24b7126a90d0c1118b687e77ea7b57bf97 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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 04/11] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (2 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 03/11] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 05/11] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - Improve coding style by using temporary variable      [Ard]
     - Updated based on review feedback                      [Sami]
     - Ref: https://edk2.groups.io/g/devel/message/105189

 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
index 38bbf2e9f81527b2545a0116120ceee56af17808..1a6ad48d1ff6c3eb5c8c375606c1600e42db42d6 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
@@ -17,6 +17,7 @@ ArmGicEnableDistributor (
   )
 {
   ARM_GIC_ARCH_REVISION  Revision;
+  UINT32                 GicDistributorCtl;
 
   /*
    * Enable GIC distributor in Non-Secure world.
@@ -26,7 +27,8 @@ 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) {
+    GicDistributorCtl = MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR);
+    if ((GicDistributorCtl & ARM_GIC_ICDDCR_ARE) != 0) {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
     } else {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 05/11] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (3 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 04/11] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 06/11] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - No code change since v1 series                [Sami]

 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 eca4ddb7d3a40d5ef2fc2d0433c50afeb64c4b24..eefe6350eb804bf9b2727b605d59035877ffb817 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 cede7a24b7126a90d0c1118b687e77ea7b57bf97..93ce8aeb199418c70367ad2386be427340aa28cf 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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 06/11] ArmPkg: Make variables used for GicInterrupt UINTN
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (4 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 05/11] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 07/11] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - Updated GicV3IrqInterruptHandler () to change       [Sami]
       GicInterrupt to UINTN and added typecast before
       printing GicInterrupt in both
       GicV[2|3]IrqInterruptHandler ().

 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 8 ++++----
 ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
index a1670021c30f05707d7c37a789fdbacc4ffa9140..cd371cab2a0159b54d8e6f0f5d3930e1276cbf7e 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);
@@ -179,7 +179,7 @@ GicV2IrqInterruptHandler (
     // Call the registered interrupt handler.
     InterruptHandler (GicInterrupt, SystemContext);
   } else {
-    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
+    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", (UINT32)GicInterrupt));
     GicV2EndOfInterrupt (&gHardwareInterruptV2Protocol, GicInterrupt);
   }
 }
@@ -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++) {
diff --git a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
index 719701a67e756c2d2aeae8fc23d50b519a1997aa..41aec70481e05594628fe01cf9ad5626e4ddbf9b 100644
--- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c
@@ -156,7 +156,7 @@ GicV3IrqInterruptHandler (
   IN EFI_SYSTEM_CONTEXT  SystemContext
   )
 {
-  UINT32                      GicInterrupt;
+  UINTN                       GicInterrupt;
   HARDWARE_INTERRUPT_HANDLER  InterruptHandler;
 
   GicInterrupt = ArmGicV3AcknowledgeInterrupt ();
@@ -173,7 +173,7 @@ GicV3IrqInterruptHandler (
     // Call the registered interrupt handler.
     InterruptHandler (GicInterrupt, SystemContext);
   } else {
-    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", GicInterrupt));
+    DEBUG ((DEBUG_ERROR, "Spurious GIC interrupt: 0x%x\n", (UINT32)GicInterrupt));
     GicV3EndOfInterrupt (&gHardwareInterruptV3Protocol, GicInterrupt);
   }
 }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 07/11] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (5 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 06/11] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 08/11] ArmPkg: Remove unused function declarations Sami Mujawar
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - Assert condition should be <=                        [Ard]
     - Fixed assert condition as per feedback and           [Sami]
       also updated copyright year.
     - Ref: https://edk2.groups.io/g/devel/message/105187

 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
index f403bec367b5254c248e620e56471904e520f9f2..d21caa90e5def04ff9666939c879de4aa772f97e 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c
@@ -1,12 +1,13 @@
 /** @file
 *
-*  Copyright (c) 2013-2014, ARM Limited. All rights reserved.
+*  Copyright (c) 2013-2023, ARM Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
 **/
 
 #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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 08/11] ArmPkg: Remove unused function declarations
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (6 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 07/11] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 09/11] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

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

Notes:
    v2:
     - No updates since v1 series.                     [Sami]

 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 8461fb7927eaf97c75135205f444d33c205910db..c285c27fdde4f1bd1d04068d47990442a6bdf20e 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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 09/11] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (7 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 08/11] ArmPkg: Remove unused function declarations Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 10/11] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - Updated copyright year                             [Sami]

 ArmPkg/Drivers/ArmGic/ArmGicLib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicLib.c b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
index eefe6350eb804bf9b2727b605d59035877ffb817..0127cca3bf0567bc80702f415e9cbb9bd2709fbc 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c
@@ -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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 10/11] ArmPkg: Adjust variable type and cast for RegShift & RegOffset
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (8 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 09/11] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-05-24 13:38 ` [PATCH v2 11/11] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
  2023-06-01 16:09 ` [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Ard Biesheuvel
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - updated copyright year                          [Sami]

 ArmPkg/Drivers/ArmGic/ArmGicLib.c         | 24 ++++++++++----------
 ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c |  6 ++---
 2 files changed, 15 insertions(+), 15 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 cd371cab2a0159b54d8e6f0f5d3930e1276cbf7e..6300a2a54b194cb8c874a4e7db9051b9732d8be2 100644
--- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
+++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c
@@ -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
@@ -411,8 +411,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)'


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 11/11] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (9 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 10/11] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
@ 2023-05-24 13:38 ` Sami Mujawar
  2023-06-01 16:09 ` [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Ard Biesheuvel
  11 siblings, 0 replies; 13+ messages in thread
From: Sami Mujawar @ 2023-05-24 13:38 UTC (permalink / raw)
  To: devel
  Cc: Sami Mujawar, ardb+tianocore, quic_llindhol, neil.jones,
	pedro.falcato, pierre.gondois, Matteo.Carlini, Akanksha.Jain2,
	Ben.Adderson, Sibel.Allinson, nd

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

Notes:
    v2:
     - No updates since v1 series                         [Sami]

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements
  2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
                   ` (10 preceding siblings ...)
  2023-05-24 13:38 ` [PATCH v2 11/11] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
@ 2023-06-01 16:09 ` Ard Biesheuvel
  11 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2023-06-01 16:09 UTC (permalink / raw)
  To: Sami Mujawar
  Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pedro.falcato,
	pierre.gondois, Matteo.Carlini, Akanksha.Jain2, Ben.Adderson,
	Sibel.Allinson, nd

On Wed, 24 May 2023 at 15:39, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> 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)
>
> This v2 series address the feedback received for the
> v1 series and additionally updates the patch 6/11
> 'ArmPkg: Make variables used for GicInterrupt UINTN'
> to extend the changes to GicV3IrqInterruptHandler ().
>
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/1751_arm_giclib_v2
>
> Sami Mujawar (11):
>   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: 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
>

Merged as #4465

Thanks!

>  ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c         | 18 +----
>  ArmPkg/Drivers/ArmGic/ArmGicLib.c               | 73 ++++++++++++--------
>  ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c         |  8 ++-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c       | 23 +++---
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c       |  6 +-
>  ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c |  6 +-
>  ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c       |  8 +--
>  ArmPkg/Include/Library/ArmGicLib.h              | 40 +++++------
>  8 files changed, 94 insertions(+), 88 deletions(-)
>
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-06-01 16:09 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-24 13:38 [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 01/11] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 02/11] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 03/11] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 04/11] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 05/11] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 06/11] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 07/11] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 08/11] ArmPkg: Remove unused function declarations Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 09/11] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 10/11] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
2023-05-24 13:38 ` [PATCH v2 11/11] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
2023-06-01 16:09 ` [PATCH v2 00/11] ArmPkg: Arm GIC Library and Driver improvements Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox