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.web10.3278.1585822857698408630 for ; Thu, 02 Apr 2020 03:20:57 -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 313EF31B; Thu, 2 Apr 2020 03:20:57 -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 DB1363F71E; Thu, 2 Apr 2020 03:20:55 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo() To: devel@edk2.groups.io, leif@nuviainc.com, Ard Biesheuvel References: <20200328104321.8668-1-ard.biesheuvel@linaro.org> <20200328104321.8668-2-ard.biesheuvel@linaro.org> <20200402101610.GL7468@vanye> From: "Ard Biesheuvel" Message-ID: Date: Thu, 2 Apr 2020 12:20:54 +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: <20200402101610.GL7468@vanye> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> --- >> 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 > > 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 >> > > >