Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

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

On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>> Before getting rid of GetRootTranslationTableInfo() and the related
>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>> version of the former to CpuDxe, which will be its only remaining
>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>> both OUT arguments are always provided.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>>   ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
>>   2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> index 3b6c5e733709..24eb1c4221e3 100644
>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>   
>>   #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>>   
>> +#define MIN_T0SZ        16
>> +#define BITS_PER_LEVEL  9
>> +
>> +STATIC
>> +VOID
>> +GetRootTranslationTableInfo (
>> +  IN  UINTN     T0SZ,
>> +  OUT UINTN     *RootTableLevel,
>> +  OUT UINTN     *RootTableEntryCount
>> +  )
>> +{
>> +  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
>> +  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
>> +}
>> +
>>   STATIC
>>   UINT64
>>   GetFirstPageAttribute (
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> index b627c3c50ff8..3fe5c24d5e5b 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> @@ -134,13 +134,6 @@ GetMemoryRegion (
>>     OUT    UINTN                   *RegionAttributes
>>     );
>>   
>> -VOID
>> -GetRootTranslationTableInfo (
> 
> So, this may be super picky, but:
> Deleting the prototype without making the definition also STATIC would
> cause build errors with -Wmissing-prototypes (which someone might be
> enabling explicitly or implicitly if say doing some code hardening on
> the side).
> 
> Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> current CFLAGS, but I think it would be a good habit to get into.
> So you get a:
> 
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 
> regardless, but I'd appreciate if you could sling a STATIC into
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> pushing.
> 

Well, while I see your point, please note that the prototype only exists 
in a local header file that only gets included by CpuDxe, and not by any 
of the other consumers of ArmMmuLib.


>> -  IN  UINTN    T0SZ,
>> -  OUT UINTN   *TableLevel,
>> -  OUT UINTN   *TableEntryCount
>> -  );
>> -
>>   EFI_STATUS
>>   SetGcdMemorySpaceAttributes (
>>     IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR    *MemorySpaceMap,
>> -- 
>> 2.17.1
>>
> 
> 
>