From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.3963.1585825555694162109 for ; Thu, 02 Apr 2020 04:05:55 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5E57530E; Thu, 2 Apr 2020 04:05:55 -0700 (PDT) Received: from [10.37.8.12] (unknown [10.37.8.12]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 64F0E3F71E; Thu, 2 Apr 2020 04:05:54 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol To: Leif Lindholm Cc: devel@edk2.groups.io, Ard Biesheuvel References: <20200328104321.8668-1-ard.biesheuvel@linaro.org> <20200328104321.8668-6-ard.biesheuvel@linaro.org> <20200402102308.GM7468@vanye> <24502c11-cab8-f00b-6c3f-e4705f315aea@arm.com> <20200402105740.GO7468@vanye> From: "Ard Biesheuvel" Message-ID: <2d4b2726-0992-2ac1-b3ff-3b48cd0182d5@arm.com> Date: Thu, 2 Apr 2020 13:05:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200402105740.GO7468@vanye> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 4/2/20 12:57 PM, Leif Lindholm wrote: > On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote: >> On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote: >>> On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote: >>>> TT_ATTR_INDX_INVALID is #define'd but never used so drop it. >>>> >>>> Signed-off-by: Ard Biesheuvel >>>> --- >>>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 --- >>>> 1 file changed, 3 deletions(-) >>>> >>>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmP= kg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >>>> index a82596d290f1..222ff817956f 100644 >>>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >>>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >>>> @@ -19,9 +19,6 @@ >>>> #include >>>> #include >>>> -// We use this index definition to define an invalid block entry >>>> -#define TT_ATTR_INDX_INVALID ((UINT32)~0) >>>> - >>> >>> Since this is separately defined also in >>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (=C2=A3"$%^=C2=A3"$?!?!?) >>> could this patch be tweaked to instead "consolidate" the definitions >>> to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where >>> all of the other TT_ATTR_INDX_ definitions live? >>> >> >> That would imply that this value is somehow architected, which it is n= ot. >> >> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this >> constant, and it only has meaning within the context of the routines >> therein. >=20 > Hmm, ok. So all of those definitions should really move *out* of > ArmPkg/Include/Chipset/AArch64Mmu.h? >=20 > No, they can't, because some of the others are used in > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >=20 > So I'm still feeling that #defines using the same namespace should > live together in order to reduce risk of future screwups. > So, unrelated to this patch > (Reviewed-by: Leif Lindholm ) > should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be > moved to ArmPkg/Include/Chipset/AArch64Mmu.h? >=20 It should simply be renamed. They are not part of the same namespace, it=20 is simply a local hack in CpuDxe to distinguish between valid indexes=20 into the MAIR table and an 'unknown' placeholder value. CpuDxe is on my list of things I'd like to clean up as well.