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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105173): https://edk2.groups.io/g/devel/message/105173 Mute This Topic: https://groups.io/mt/99086446/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c @@ -XXX,XX +XXX,XX @@ /*++ -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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ ArmGicGetInterfaceIdentification ( UINTN EFIAPI ArmGicGetMaxNumInterrupts ( - IN INTN GicDistributorBase + IN UINTN GicDistributorBase ) { UINTN ItLines; @@ -XXX,XX +XXX,XX @@ 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 ( @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicSetupNonSecure ( IN UINTN MpId, - IN INTN GicDistributorBase, + IN UINTN GicDistributorBase, IN INTN GicInterruptInterfaceBase ); @@ -XXX,XX +XXX,XX @@ 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 ); /* @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicV2SetupNonSecure ( IN UINTN MpId, - IN INTN GicDistributorBase, + IN UINTN GicDistributorBase, IN INTN GicInterruptInterfaceBase ); -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105176): https://edk2.groups.io/g/devel/message/105176 Mute This Topic: https://groups.io/mt/99086452/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ ArmGicDisableDistributor ( VOID EFIAPI ArmGicEnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ) { ARM_GIC_ARCH_REVISION Revision; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicV2EnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ) { /* @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ UINTN EFIAPI ArmGicGetInterfaceIdentification ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); // GIC Secure interfaces @@ -XXX,XX +XXX,XX @@ EFIAPI ArmGicSetupNonSecure ( IN UINTN MpId, IN UINTN GicDistributorBase, - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID @@ -XXX,XX +XXX,XX @@ ArmGicSetSecureInterrupts ( VOID EFIAPI ArmGicEnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID EFIAPI ArmGicDisableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID @@ -XXX,XX +XXX,XX @@ ArmGicEndOfInterrupt ( UINTN EFIAPI ArmGicSetPriorityMask ( - IN INTN GicInterruptInterfaceBase, - IN INTN PriorityMask + IN UINTN GicInterruptInterfaceBase, + IN INTN PriorityMask ); VOID @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105179): https://edk2.groups.io/g/devel/message/105179 Mute This Topic: https://groups.io/mt/99086460/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105180): https://edk2.groups.io/g/devel/message/105180 Mute This Topic: https://groups.io/mt/99086461/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105184): https://edk2.groups.io/g/devel/message/105184 Mute This Topic: https://groups.io/mt/99086468/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ // Bit Mask for #define ARM_GIC_ICCIAR_ACKINTID 0x3FF -UINTN +UINT32 EFIAPI ArmGicGetInterfaceIdentification ( IN UINTN GicInterruptInterfaceBase -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105185): https://edk2.groups.io/g/devel/message/105185 Mute This Topic: https://groups.io/mt/99086469/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ GicV2IrqInterruptHandler ( IN EFI_SYSTEM_CONTEXT SystemContext ) { - UINT32 GicInterrupt; + UINTN GicInterrupt; HARDWARE_INTERRUPT_HANDLER InterruptHandler; GicInterrupt = ArmGicV2AcknowledgeInterrupt (mGicInterruptInterfaceBase); @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105174): https://edk2.groups.io/g/devel/message/105174 Mute This Topic: https://groups.io/mt/99086448/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105177): https://edk2.groups.io/g/devel/message/105177 Mute This Topic: https://groups.io/mt/99086456/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c @@ -XXX,XX +XXX,XX @@ **/ #include <Library/ArmGicLib.h> +#include <Library/DebugLib.h> #include <Library/IoLib.h> UINTN @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105175): https://edk2.groups.io/g/devel/message/105175 Mute This Topic: https://groups.io/mt/99086450/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105181): https://edk2.groups.io/g/devel/message/105181 Mute This Topic: https://groups.io/mt/99086464/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105183): https://edk2.groups.io/g/devel/message/105183 Mute This Topic: https://groups.io/mt/99086466/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105178): https://edk2.groups.io/g/devel/message/105178 Mute This Topic: https://groups.io/mt/99086457/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ ArmGicAcknowledgeInterrupt ( Value = 1023; } + if (InterruptId != NULL) { + // InterruptId is required for the caller to know if a valid or spurious + // interrupt has been read + *InterruptId = IntId; + } + return Value; } -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105182): https://edk2.groups.io/g/devel/message/105182 Mute This Topic: https://groups.io/mt/99086465/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- Bugzilla: Bug 3399 (https://bugzilla.tianocore.org/show_bug.cgi?id=3399) This patch series address the issues reported in https://bugzilla.tianocore.org/show_bug.cgi?id=3399 and also has general improvements and fixes for other issues in the Arm GIC Library and driver. This patch series is expected to be applied on top of the patch at: ArmPkg: Fix GicV2 BaseAddress types (https://edk2.groups.io/g/devel/message/104721) This v2 series address the feedback received for the v1 series and additionally updates the patch 6/11 'ArmPkg: Make variables used for GicInterrupt UINTN' to extend the changes to GicV3IrqInterruptHandler (). The changes can be seen at: https://github.com/samimujawar/edk2/tree/1751_arm_giclib_v2 Sami Mujawar (11): ArmPkg: Fix data type used for GicDistributorBase ArmPkg: Fix data type used for GicInterruptInterfaceBase ArmPkg: Fix ArmGicSendSgiTo() parameters ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor ArmPkg: Fix return type for ArmGicGetInterfaceIdentification ArmPkg: Make variables used for GicInterrupt UINTN ArmPkg: Typecast IntID to UINT32 in ArmGicV2EndOfInterrupt ArmPkg: Remove unused function declarations ArmPkg: Prevent SgiId from setting RES0 bits of GICD_SGIR ArmPkg: Adjust variable type and cast for RegShift & RegOffset ArmPkg: Fix ArmGicAcknowledgeInterrupt () for GICv3 ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c | 18 +---- ArmPkg/Drivers/ArmGic/ArmGicLib.c | 73 ++++++++++++-------- ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 8 ++- ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c | 23 +++--- ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c | 6 +- ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c | 6 +- ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c | 8 +-- ArmPkg/Include/Library/ArmGicLib.h | 40 +++++------ 8 files changed, 94 insertions(+), 88 deletions(-) -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105239): https://edk2.groups.io/g/devel/message/105239 Mute This Topic: https://groups.io/mt/99108663/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c @@ -XXX,XX +XXX,XX @@ /*++ -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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ ArmGicGetInterfaceIdentification ( UINTN EFIAPI ArmGicGetMaxNumInterrupts ( - IN INTN GicDistributorBase + IN UINTN GicDistributorBase ) { UINTN ItLines; @@ -XXX,XX +XXX,XX @@ 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 ( @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicSetupNonSecure ( IN UINTN MpId, - IN INTN GicDistributorBase, + IN UINTN GicDistributorBase, IN INTN GicInterruptInterfaceBase ); @@ -XXX,XX +XXX,XX @@ 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 ); /* @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicV2SetupNonSecure ( IN UINTN MpId, - IN INTN GicDistributorBase, + IN UINTN GicDistributorBase, IN INTN GicInterruptInterfaceBase ); -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105244): https://edk2.groups.io/g/devel/message/105244 Mute This Topic: https://groups.io/mt/99108679/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ ArmGicDisableDistributor ( VOID EFIAPI ArmGicEnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ) { ARM_GIC_ARCH_REVISION Revision; @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2NonSecLib.c @@ -XXX,XX +XXX,XX @@ /** @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 * @@ -XXX,XX +XXX,XX @@ VOID EFIAPI ArmGicV2EnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ) { /* @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ UINTN EFIAPI ArmGicGetInterfaceIdentification ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); // GIC Secure interfaces @@ -XXX,XX +XXX,XX @@ EFIAPI ArmGicSetupNonSecure ( IN UINTN MpId, IN UINTN GicDistributorBase, - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID @@ -XXX,XX +XXX,XX @@ ArmGicSetSecureInterrupts ( VOID EFIAPI ArmGicEnableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID EFIAPI ArmGicDisableInterruptInterface ( - IN INTN GicInterruptInterfaceBase + IN UINTN GicInterruptInterfaceBase ); VOID @@ -XXX,XX +XXX,XX @@ ArmGicEndOfInterrupt ( UINTN EFIAPI ArmGicSetPriorityMask ( - IN INTN GicInterruptInterfaceBase, - IN INTN PriorityMask + IN UINTN GicInterruptInterfaceBase, + IN INTN PriorityMask ); VOID @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105238): https://edk2.groups.io/g/devel/message/105238 Mute This Topic: https://groups.io/mt/99108661/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105242): https://edk2.groups.io/g/devel/message/105242 Mute This Topic: https://groups.io/mt/99108674/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c @@ -XXX,XX +XXX,XX @@ ArmGicEnableDistributor ( ) { ARM_GIC_ARCH_REVISION Revision; + UINT32 GicDistributorCtl; /* * Enable GIC distributor in Non-Secure world. @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105245): https://edk2.groups.io/g/devel/message/105245 Mute This Topic: https://groups.io/mt/99108682/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Include/Library/ArmGicLib.h +++ b/ArmPkg/Include/Library/ArmGicLib.h @@ -XXX,XX +XXX,XX @@ // Bit Mask for #define ARM_GIC_ICCIAR_ACKINTID 0x3FF -UINTN +UINT32 EFIAPI ArmGicGetInterfaceIdentification ( IN UINTN GicInterruptInterfaceBase -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105243): https://edk2.groups.io/g/devel/message/105243 Mute This Topic: https://groups.io/mt/99108675/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ GicV2IrqInterruptHandler ( IN EFI_SYSTEM_CONTEXT SystemContext ) { - UINT32 GicInterrupt; + UINTN GicInterrupt; HARDWARE_INTERRUPT_HANDLER InterruptHandler; GicInterrupt = ArmGicV2AcknowledgeInterrupt (mGicInterruptInterfaceBase); @@ -XXX,XX +XXX,XX @@ 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); } } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV3/ArmGicV3Dxe.c @@ -XXX,XX +XXX,XX @@ GicV3IrqInterruptHandler ( IN EFI_SYSTEM_CONTEXT SystemContext ) { - UINT32 GicInterrupt; + UINTN GicInterrupt; HARDWARE_INTERRUPT_HANDLER InterruptHandler; GicInterrupt = ArmGicV3AcknowledgeInterrupt (); @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105240): https://edk2.groups.io/g/devel/message/105240 Mute This Topic: https://groups.io/mt/99108670/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Lib.c @@ -XXX,XX +XXX,XX @@ /** @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 @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105247): https://edk2.groups.io/g/devel/message/105247 Mute This Topic: https://groups.io/mt/99108685/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicCommonDxe.c @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105246): https://edk2.groups.io/g/devel/message/105246 Mute This Topic: https://groups.io/mt/99108683/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105241): https://edk2.groups.io/g/devel/message/105241 Mute This Topic: https://groups.io/mt/99108672/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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) || @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c +++ b/ArmPkg/Drivers/ArmGic/GicV2/ArmGicV2Dxe.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105249): https://edk2.groups.io/g/devel/message/105249 Mute This Topic: https://groups.io/mt/99108693/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- 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 XXXXXXX..XXXXXXX 100644 --- a/ArmPkg/Drivers/ArmGic/ArmGicLib.c +++ b/ArmPkg/Drivers/ArmGic/ArmGicLib.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ ArmGicAcknowledgeInterrupt ( Value = 1023; } + if (InterruptId != NULL) { + // InterruptId is required for the caller to know if a valid or spurious + // interrupt has been read + *InterruptId = IntId; + } + return Value; } -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105248): https://edk2.groups.io/g/devel/message/105248 Mute This Topic: https://groups.io/mt/99108687/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-