* [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements
@ 2023-05-23 13:04 Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
` (11 more replies)
0 siblings, 12 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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)
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)'
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:29 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
` (10 subsequent siblings)
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:35 ` Pedro Falcato
2023-05-23 13:36 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 03/12] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
` (9 subsequent siblings)
11 siblings, 2 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 03/12] ArmPkg: Fix ArmGicSendSgiTo() parameters
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
` (8 subsequent siblings)
11 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (2 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 03/12] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:32 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 05/12] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
` (7 subsequent siblings)
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 05/12] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (3 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 06/12] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
` (6 subsequent siblings)
11 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 06/12] ArmPkg: Make variables used for GicInterrupt UINTN
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (4 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 05/12] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt Sami Mujawar
` (5 subsequent siblings)
11 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (5 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 06/12] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:32 ` Pedro Falcato
2023-05-23 13:04 ` [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
` (4 subsequent siblings)
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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 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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (6 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:26 ` [edk2-devel] " Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 09/12] ArmPkg: Remove unused function declarations Sami Mujawar
` (3 subsequent siblings)
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 09/12] ArmPkg: Remove unused function declarations
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (7 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
` (2 subsequent siblings)
11 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (8 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 09/12] ArmPkg: Remove unused function declarations Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:37 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 11/12] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 11/12] ArmPkg: Adjust variable type and cast for RegShift & RegOffset
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (9 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
11 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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)'
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
` (10 preceding siblings ...)
2023-05-23 13:04 ` [PATCH v1 11/12] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
@ 2023-05-23 13:04 ` Sami Mujawar
2023-05-23 13:38 ` Ard Biesheuvel
11 siblings, 1 reply; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 13:04 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>
---
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] 25+ messages in thread
* Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt
2023-05-23 13:04 ` [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
@ 2023-05-23 13:26 ` Ard Biesheuvel
2023-05-23 15:16 ` Sami Mujawar
0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:26 UTC (permalink / raw)
To: devel, sami.mujawar
Cc: ardb+tianocore, quic_llindhol, neil.jones, pedro.falcato,
pierre.gondois, Matteo.Carlini, Akanksha.Jain2, Ben.Adderson,
Sibel.Allinson, nd
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)'
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase
2023-05-23 13:04 ` [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
@ 2023-05-23 13:29 ` Ard Biesheuvel
0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:29 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 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)'
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor
2023-05-23 13:04 ` [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
@ 2023-05-23 13:32 ` Ard Biesheuvel
2023-05-23 15:16 ` Sami Mujawar
0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:32 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 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)'
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt
2023-05-23 13:04 ` [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt Sami Mujawar
@ 2023-05-23 13:32 ` Pedro Falcato
2023-05-23 15:17 ` Sami Mujawar
0 siblings, 1 reply; 25+ messages in thread
From: Pedro Falcato @ 2023-05-23 13:32 UTC (permalink / raw)
To: Sami Mujawar
Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pierre.gondois,
Matteo.Carlini, Akanksha.Jain2, Ben.Adderson, Sibel.Allinson, nd
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase
2023-05-23 13:04 ` [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
@ 2023-05-23 13:35 ` Pedro Falcato
2023-05-23 15:16 ` Sami Mujawar
2023-05-23 13:36 ` Ard Biesheuvel
1 sibling, 1 reply; 25+ messages in thread
From: Pedro Falcato @ 2023-05-23 13:35 UTC (permalink / raw)
To: Sami Mujawar
Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pierre.gondois,
Matteo.Carlini, Akanksha.Jain2, Ben.Adderson, Sibel.Allinson, nd
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase
2023-05-23 13:04 ` [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
2023-05-23 13:35 ` Pedro Falcato
@ 2023-05-23 13:36 ` Ard Biesheuvel
1 sibling, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:36 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 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)'
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR
2023-05-23 13:04 ` [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
@ 2023-05-23 13:37 ` Ard Biesheuvel
0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:37 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 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)'
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3
2023-05-23 13:04 ` [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
@ 2023-05-23 13:38 ` Ard Biesheuvel
0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2023-05-23 13:38 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 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)'
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt
2023-05-23 13:26 ` [edk2-devel] " Ard Biesheuvel
@ 2023-05-23 15:16 ` Sami Mujawar
0 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 15:16 UTC (permalink / raw)
To: Ard Biesheuvel, devel
Cc: ardb+tianocore, quic_llindhol, neil.jones, pedro.falcato,
pierre.gondois, Matteo.Carlini, Akanksha.Jain2, Ben.Adderson,
Sibel.Allinson, nd
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)'
>>
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor
2023-05-23 13:32 ` Ard Biesheuvel
@ 2023-05-23 15:16 ` Sami Mujawar
0 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 15:16 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pedro.falcato,
pierre.gondois, Matteo.Carlini, Akanksha.Jain2, Ben.Adderson,
Sibel.Allinson, nd
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)'
>>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase
2023-05-23 13:35 ` Pedro Falcato
@ 2023-05-23 15:16 ` Sami Mujawar
0 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 15:16 UTC (permalink / raw)
To: Pedro Falcato
Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pierre.gondois,
Matteo.Carlini, Akanksha.Jain2, Ben.Adderson, Sibel.Allinson, nd
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)'
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt
2023-05-23 13:32 ` Pedro Falcato
@ 2023-05-23 15:17 ` Sami Mujawar
0 siblings, 0 replies; 25+ messages in thread
From: Sami Mujawar @ 2023-05-23 15:17 UTC (permalink / raw)
To: Pedro Falcato
Cc: devel, ardb+tianocore, quic_llindhol, neil.jones, pierre.gondois,
Matteo.Carlini, Akanksha.Jain2, Ben.Adderson, Sibel.Allinson, nd
[-- 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 --]
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2023-05-23 15:18 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-23 13:04 [PATCH v1 00/12] ArmPkg: Arm GIC Library and Driver improvements Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 01/12] ArmPkg: Fix data type used for GicDistributorBase Sami Mujawar
2023-05-23 13:29 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase Sami Mujawar
2023-05-23 13:35 ` Pedro Falcato
2023-05-23 15:16 ` Sami Mujawar
2023-05-23 13:36 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 03/12] ArmPkg: Fix ArmGicSendSgiTo() parameters Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor Sami Mujawar
2023-05-23 13:32 ` Ard Biesheuvel
2023-05-23 15:16 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 05/12] ArmPkg: Fix return type for ArmGicGetInterfaceIdentification Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 06/12] ArmPkg: Make variables used for GicInterrupt UINTN Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 07/12] ArmPkg: Fix return value for ArmGicV2AcknowledgeInterrupt Sami Mujawar
2023-05-23 13:32 ` Pedro Falcato
2023-05-23 15:17 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 08/12] ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt Sami Mujawar
2023-05-23 13:26 ` [edk2-devel] " Ard Biesheuvel
2023-05-23 15:16 ` Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 09/12] ArmPkg: Remove unused function declarations Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 10/12] ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR Sami Mujawar
2023-05-23 13:37 ` Ard Biesheuvel
2023-05-23 13:04 ` [PATCH v1 11/12] ArmPkg: Adjust variable type and cast for RegShift & RegOffset Sami Mujawar
2023-05-23 13:04 ` [PATCH v1 12/12] ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 Sami Mujawar
2023-05-23 13:38 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox