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