From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web11.4037.1585825400194188136 for ; Thu, 02 Apr 2020 04:03:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=PfsT5A1Q; spf=pass (domain: nuviainc.com, ip: 209.85.128.66, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f66.google.com with SMTP id t128so2946276wma.0 for ; Thu, 02 Apr 2020 04:03:19 -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=5E34e7T6gWgSsvVizpIqrXSskXsBamwkw+XkzDr453E=; b=PfsT5A1QNA6BeEfvBqzG1vT5X7N3DLW+QoUvT7xdfn1+C0eP5G5KW0Zvu1Hhv16ttu PlominbpVFMeLv04R0II8XeSAR9I/tmaN/8nctbFIxQvhsRzmvWEcu5vt0zuX6aGfOoG PQGXZTBIQndZBS9s8wsRSRL7NElT3KnqZbdZXYu7J2xOx0j2gbn8Ytq8gDBL+5kTfhHx FXXwBNVsc6XGWynjUizNoyvupw2P8szNGTaFOHC+ljoVz91mfzhwcCtpBYW764MZe7vP Nw+aEtSu/vmztWoCbArgJ3i6QWwLKzqF1UWjhqH5Ylzl4Bj6gkj8eMNlBw28+kjBHQ1k S74A== 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=5E34e7T6gWgSsvVizpIqrXSskXsBamwkw+XkzDr453E=; b=ZOOHOmaatLprNLa/jAVwCfU4LabHYou7BXUb5ttvgWdenE4M+6qUyLHWG7vqRjPvBE JkkXMEa7qGTSUpGbCkyfgrW4tloKbMfwQnJVp3RIzlEYVoEEqal1ZLJY2Q8baQlDhCpT aj2bk8SUfgcV4Ry1+8/YQHyIVXADvZkEXXggKnylDQG4w94jn1fxBWR9IWIzBW4AodvF m+2/hJdUSqqJzPhlk5EOSUQ2hhfk4vOtIJyJZnCQA+mO3+lwpJuAu+5EGTdk1zZXUHBh 5HSBEnukSjVaADakZfAL+4+cUldEvuR6AJU761IXzR6nfZ3v905bULZMc/fW1rwiiQ3s inHw== X-Gm-Message-State: AGi0Pub5uGmSNt+VGRJvwESmJE9qs2LBX6n38JlW0Fj1/++UizYzF+qT hGwrbHKxykXZYFH24mJOsRLIVg== X-Google-Smtp-Source: APiQypKVtFJRkrKyNRVh4cgb1Tkbn+SRMbei1CmecwAZrNHAq/Uvs1mNVvlKR3MBkRG7ck4ED5yhhQ== X-Received: by 2002:a1c:7d88:: with SMTP id y130mr3058853wmc.5.1585825398672; Thu, 02 Apr 2020 04:03:18 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id y16sm6912479wrp.78.2020.04.02.04.03.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Apr 2020 04:03:18 -0700 (PDT) Date: Thu, 2 Apr 2020 12:03:16 +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: <20200402110316.GP7468@vanye> References: <20200328104321.8668-1-ard.biesheuvel@linaro.org> <20200328104321.8668-2-ard.biesheuvel@linaro.org> <20200402101610.GL7468@vanye> <20200402102804.GN7468@vanye> <0e6a6c40-1a48-a5cb-563f-4f5b39c41fef@arm.com> MIME-Version: 1.0 In-Reply-To: <0e6a6c40-1a48-a5cb-563f-4f5b39c41fef@arm.com> 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:29:30 +0200, Ard Biesheuvel wrote: > On 4/2/20 12:28 PM, Leif Lindholm wrote: > > 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 relat= ed > > > > > 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. Summary after discussing off-list: This patch isn't *just* moving a public function private, but it's replacing use of a function that was never meant to be public and had a completely bonkers forward-declaration in a different module. Ard has agreed to make the commit message more explicit on that point, and I'm happy with that. So with that change, for the series: Reviewed-by: Leif Lindholm > > > > So, this may be super picky, but: > > > > Deleting the prototype without making the definition also STATIC wo= uld > > > > 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 o= ur > > > > 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 exi= sts in > > > a local header file that only gets included by CpuDxe, and not by any= of the > > > other consumers of ArmMmuLib. > >=20 > > 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: > >=20 > > "aarch64-linux-gnu-gcc" -g -fshort-wchar -fno-builtin -fno-strict-ali= asing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -= include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=3DArmMmuBaseLibStrings -g= -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-arr= ay-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -= fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-addre= ss -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ff= ixed-x18 -mcmodel=3Dsmall -flto -Wno-unused-but-set-variable -Wno-unused-co= nst-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-a= lign -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH6= 4/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch6= 4/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/wo= rk/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQem= u-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG= -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/E= mbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -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/ArmMmuLibCore.c > > leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc" -g -fshort-wc= har -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffun= ction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRA= Y_NAME=3DArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict= -aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -m= little-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sec= tions -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unw= ind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=3Dsmall -flto -Wno-unused= -but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_= DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/ti= anocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMm= uLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg= /Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/wor= k/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Libra= ry/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/A= rmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Inc= lude -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/ed= k2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/A= rmMmuLibCore.c -Wmissing-prototypes > > /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: = error: no previous prototype for =E2=80=98GetRootTranslationTableInfo=E2=80= =99 [-Werror=3Dmissing-prototypes] > > GetRootTranslationTableInfo ( > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > >=20 > > So it breaks bisect if you're experimenting with hardening. > >=20 > > As I said, I don't insist, but I want to be sure you realise that bit > > before you decide. > >=20 >=20 > But none of the other consumers of ArmMmuLib do an #include of > ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this > result. >