From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web11.3337.1585823287971442443 for ; Thu, 02 Apr 2020 03:28:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=qWdhJjeP; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id m17so3486384wrw.11 for ; Thu, 02 Apr 2020 03:28:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=/tmzFkBrIeuwyC+1aOWF557ZXPAkclP2P+1Mie5t7Tw=; b=qWdhJjePp3I0ttF7ysgYiR9fkMEU8sVQl+4ZcYq40lHmKKWALvTRZQyQyq/TwM/oHM rlUr7cvZ4YwiLI9/PRGQ5J+L37ZFS7TnXbC1+xPdYiScUogLqx862Siq5Hg7hIwWNTRX 4nBAMG7xoHppU0AkVMp6rcyWfgnEvDLX3HvoOROhfBIQuTTsmTXEoHlOOs0Vwl2rcYmW p1q8fY2Xqc9MHS57fuv2HPNj8/7Gyh0qyxBqgK8euwaRHYg4gNhVFn2KBBJ8OP0nIhLj P2JSRJ7l/j8e5JB4V6KTHHFcIWxUh8Ejg1ZMEAfETNUNW/FAXD+wcq7HnEyWdASaZq77 oXQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=/tmzFkBrIeuwyC+1aOWF557ZXPAkclP2P+1Mie5t7Tw=; b=dkZ2ukYDKT1RdirV/bNAbxKXhUfzEfsCtKLziBKXU+f9PolKoa0MjYfcqE6eSQfFpG gC7QuwA8ZJraQiSwNAfDvG1Pc6laYCfN3tqb/sU8MaTI3+es5dVP287JrUyfy+CQxbIF 5BFt7WfuW3KshPVFApRwAUnCODUkQj6loB2m7+eS9FEHRDgUm9Xt3ZbQHWmAqMbIlXNl jrbFJwsvCPOggg9kr5XQmb+YODZcp0HDfWk8YyPDYVUr5YKav5quG9h6dpR+TQ0Sx6HQ W6qCWYud0E8SFD7Xx+BuckWEXdfP46YM/OX7eZqT1cToCGd5luNNQX/VKE2n6xMMIXZq dtXA== X-Gm-Message-State: AGi0PuaVep58/5akiJ8JI8XxKIIGTjRQY4Fzizf17W3OEUrM8VLe9l6H /NEYN3k28fbxF/X2Q504h33+n83WnUQ= X-Google-Smtp-Source: APiQypIuCnFF819iGJraEQywed7XA/5IwZH6ZeVhsTOCiD+SzaEAk+3ekXxFRAabb8m2P4xh0VqRiw== X-Received: by 2002:adf:8187:: with SMTP id 7mr2926354wra.358.1585823286512; Thu, 02 Apr 2020 03:28:06 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id r17sm6819572wrx.46.2020.04.02.03.28.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Apr 2020 03:28:06 -0700 (PDT) Date: Thu, 2 Apr 2020 11:28:04 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, Ard Biesheuvel Subject: Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo() Message-ID: <20200402102804.GN7468@vanye> References: <20200328104321.8668-1-ard.biesheuvel@linaro.org> <20200328104321.8668-2-ard.biesheuvel@linaro.org> <20200402101610.GL7468@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote: > 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. > > >=20 > > > 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(-) > > >=20 > > > diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/Cp= uDxe/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 =3D (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL; > > > + *RootTableEntryCount =3D TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % B= ITS_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 ( > >=20 > > 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). > >=20 > > 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: > >=20 > > Reviewed-by: Leif Lindholm > >=20 > > regardless, but I'd appreciate if you could sling a STATIC into > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before > > pushing. > >=20 >=20 > 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. What I'm saying is that with -Wmissing-prototypes added, building after applying this patch (but before applying the one that deletes the function) gives: "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-aliasi= ng -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -inc= lude AutoGen.h -fno-common -DSTRING_ARRAY_NAME=3DArmMmuBaseLibStrings -g -O= s -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-= bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fve= rbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address = -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixe= d-x18 -mcmodel=3Dsmall -flto -Wno-unused-but-set-variable -Wno-unused-const= -variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-alig= n -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/R= ELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/A= rmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/= git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-A= ARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I= /work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/Embe= ddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/wor= k/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git= /edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wchar= -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffuncti= on-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_N= AME=3DArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-al= iasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlit= tle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sectio= ns -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind= -tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=3Dsmall -flto -Wno-unused-bu= t-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEP= RECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tiano= core/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLi= b/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Li= brary/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/g= it/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/= ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmP= kg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Includ= e -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/= MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmM= muLibCore.c -Wmissing-prototypes /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: err= or: no previous prototype for =E2=80=98GetRootTranslationTableInfo=E2=80=99= [-Werror=3Dmissing-prototypes] GetRootTranslationTableInfo ( ^~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors So it breaks bisect if you're experimenting with hardening. As I said, I don't insist, but I want to be sure you realise that bit before you decide. / Leif >=20 >=20 > > > - IN UINTN T0SZ, > > > - OUT UINTN *TableLevel, > > > - OUT UINTN *TableEntryCount > > > - ); > > > - > > > EFI_STATUS > > > SetGcdMemorySpaceAttributes ( > > > IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap, > > > --=20 > > > 2.17.1 > > >=20 > >=20 > >=20 > >=20 >=20