Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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 <ard.biesheuvel@linaro.org>
>>>> ---
>>>>    ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/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 <Library/BaseLib.h>
>>>>    #include <Library/DebugLib.h>
>>>> -// 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 (£"$%^£"$?!?!?)
>>> 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 not.
>>
>> 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.
> 
> Hmm, ok. So all of those definitions should really move *out* of
> ArmPkg/Include/Chipset/AArch64Mmu.h?
> 
> No, they can't, because some of the others are used in
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> 
> 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 <leif@nuviainc.com>)
> should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
> moved to ArmPkg/Include/Chipset/AArch64Mmu.h?
> 

It should simply be renamed. They are not part of the same namespace, it 
is simply a local hack in CpuDxe to distinguish between valid indexes 
into the MAIR table and an 'unknown' placeholder value.

CpuDxe is on my list of things I'd like to clean up as well.