From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.22759.1684848980127638060 for ; Tue, 23 May 2023 06:36:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=B7ydiB5J; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 835E4631CD for ; Tue, 23 May 2023 13:36:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E57F4C4339B for ; Tue, 23 May 2023 13:36:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1684848978; bh=o16sElNZw6GjFV9r/PM9DZ1j56GxlX7fpfXZTibCn2k=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=B7ydiB5J8I1vzKyx/9DS0DHjcbI1EacQOK8bnVVIzJ1+IZDfGPvrL/A5AdxUrRqaH s7NsuJqpzxjseVKmmK2s7Yx6KS5Pyc2LlvKeBIqD6GUrLzyrgkk/ntrNZ+H55oR/tn QFs5Kps328jwmFGNOhJAw0q1P5l4vjm+kMesn77TnaMXqnEM3YFqlGVHoNvTR8r9/h eyzGiQu/wo2BQGrS4Wst7DGG6eNyZNGMNQnAbf9/uu+koll5prVaxNbfmeVuqgT2xW MuJ7X1NLj36AwT6XNwJE3B/sEbCjhXxAwqR/DKeV2iXgElRxmD0QmKJub/QWPrdcYy c8KqilvUbMPlw== Received: by mail-lf1-f42.google.com with SMTP id 2adb3069b0e04-4f3bb61f860so3412998e87.3 for ; Tue, 23 May 2023 06:36:18 -0700 (PDT) X-Gm-Message-State: AC+VfDwKa8EzxoJqFwB0+vTVFt9P4QisRWx67pQas3ovH6QVHJLmgIvi 58KKRttw93th9lMLr4LwEOOcZitp6QcgzELTzwc= X-Google-Smtp-Source: ACHHUZ5V4+yrwIaQTOORuI0oZer+9Jmon291wPFu0tvkLF6D5LdmlRo6girno6E0NNKMEneWB/LoU8/S/3kfrVIyR9o= X-Received: by 2002:ac2:4ac5:0:b0:4f3:afcc:e1c8 with SMTP id m5-20020ac24ac5000000b004f3afcce1c8mr4538658lfp.33.1684848976828; Tue, 23 May 2023 06:36:16 -0700 (PDT) MIME-Version: 1.0 References: <20230523130421.10804-1-sami.mujawar@arm.com> <20230523130421.10804-3-sami.mujawar@arm.com> In-Reply-To: <20230523130421.10804-3-sami.mujawar@arm.com> From: "Ard Biesheuvel" Date: Tue, 23 May 2023 15:36:05 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v1 02/12] ArmPkg: Fix data type used for GicInterruptInterfaceBase To: Sami Mujawar Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, quic_llindhol@quicinc.com, neil.jones@blaize.com, pedro.falcato@gmail.com, pierre.gondois@arm.com, Matteo.Carlini@arm.com, Akanksha.Jain2@arm.com, Ben.Adderson@arm.com, Sibel.Allinson@arm.com, nd@arm.com Content-Type: text/plain; charset="UTF-8" On Tue, 23 May 2023 at 15:04, Sami Mujawar 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 Reviewed-by: Ard Biesheuvel > --- > 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)' >