From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d44; helo=mail-io1-xd44.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8ED222118F779 for ; Tue, 11 Dec 2018 05:53:58 -0800 (PST) Received: by mail-io1-xd44.google.com with SMTP id l3so602973ioc.10 for ; Tue, 11 Dec 2018 05:53:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=g4bEttYe+KCONeSG+gu666PhwnIdrJhttTIFNaRjXJc=; b=WWYQtwSFpT2oTq0CqUdrILnkNp9rwaQ/cKAG/1m2iG2UDMAah9pT0kewi6vC8Psfsr IiG6zZexHji5f+3PGBEL6A4UEoRtOkFDhz2GbwUDbCBZrUgv6CoHeI2zlQwTXzFqvxD4 RARxEINgr8X0S+aXNbjLg3azg0ppctALXXONk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=g4bEttYe+KCONeSG+gu666PhwnIdrJhttTIFNaRjXJc=; b=fkw5iYaQFrlTCLBytsl7khjc8CaguRF+Mf3mmDAvmOH8qfuPU3sUw/uunIh6VMHy6e LNupdmPLYMkfwXiejonrhQwJPngvop1/0s8YpHhA8DBSprDuqbFzeY7kjPuNlC5lB0DW S9JZuAHLM29Lb+ktqwfXo63j9e/iaxVsea56+yeNrfD5j4ykkGful9bv65WQ8v/isp8d K1cT5V8OaZcEp/e52+B+TLTY7eUTSYk1/FALEupm9oPzo7o0zDkG4YVYO6RckOjxMNFF pqBCMQ/K5YUgz28x4noHzNDH/6PNtcFM/L4FqdQGBrQuCxkleNmLvoOF3d7cruYcujgO cSIA== X-Gm-Message-State: AA+aEWada29EfACWD/pwxXV6JERrkc+UfmnVbxgWhjiCuVY8MPpYsfBm cWzduRW2FkKVEZjq07K4vZhsLwhNcrIuH4ogpizfsvnSFNs= X-Google-Smtp-Source: AFSGD/XNE1N33dqRHbjTGviOW833nq5KZMPPNJS8lK2ItNPfgGKZyOP2Ykf64gQRmXw66wT3Cvyhmohp0CiqycMXIAY= X-Received: by 2002:a5e:cb42:: with SMTP id h2mr13971918iok.60.1544536437773; Tue, 11 Dec 2018 05:53:57 -0800 (PST) MIME-Version: 1.0 References: <20181210141339.17108-1-ard.biesheuvel@linaro.org> <530714fc-c9f2-d38e-0ad1-0a67b967c52a@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E38A23B@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E38A23B@SHSMSX104.ccr.corp.intel.com> From: Ard Biesheuvel Date: Tue, 11 Dec 2018 14:53:46 +0100 Message-ID: To: "Gao, Liming" Cc: Laszlo Ersek , "edk2-devel@lists.01.org" Subject: Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Dec 2018 13:53:58 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 11 Dec 2018 at 14:34, Gao, Liming wrote: > > I have no comments for this change. Reviewed-by: Liming Gao > Thanks all Pushed as 765fb87c2b70..de3c440e8a54 > > -----Original Message----- > > From: Laszlo Ersek [mailto:lersek@redhat.com] > > Sent: Tuesday, December 11, 2018 2:19 AM > > To: Ard Biesheuvel > > Cc: edk2-devel@lists.01.org; Gao, Liming > > Subject: Re: [edk2] [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment > > > > On 12/10/18 19:12, Ard Biesheuvel wrote: > > > On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek wrote: > > >> > > >> On 12/10/18 15:13, Ard Biesheuvel wrote: > > >>> Since 4 KB section alignment is required when mapping PE/COFF images > > >>> with strict permissions, update the default section alignment when > > >>> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as > > >>> SEC, PEIMs or PEI core are not affected by this change, since the > > >>> override to 32 byte aligment remains in effect. > > >>> > > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > > >>> Signed-off-by: Ard Biesheuvel > > >>> --- > > >>> BaseTools/Conf/tools_def.template | 24 +++++++++++--------- > > >>> 1 file changed, 13 insertions(+), 11 deletions(-) > > >>> > > >>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > > >>> index e0e68fd7fb49..5d34333dc54f 100755 > > >>> --- a/BaseTools/Conf/tools_def.template > > >>> +++ b/BaseTools/Conf/tools_def.template > > >>> @@ -4263,11 +4263,11 @@ DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS) > > >>> DEFINE GCC49_AARCH64_ASM_FLAGS = DEF(GCC48_AARCH64_ASM_FLAGS) > > >>> DEFINE GCC49_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) > > >>> DEFINE GCC49_ARM_CC_XIPFLAGS = DEF(GCC48_ARM_CC_XIPFLAGS) > > >>> -DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) > > DEF(GCC_AARCH64_CC_FLAGS) > > >>> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) > > DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small > > >>> DEFINE GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC48_AARCH64_CC_XIPFLAGS) > > >>> DEFINE GCC49_ARM_DLINK_FLAGS = DEF(GCC48_ARM_DLINK_FLAGS) > > >>> DEFINE GCC49_ARM_DLINK2_FLAGS = DEF(GCC48_ARM_DLINK2_FLAGS) > > >>> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) > > >>> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 > > >>> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS) > > >>> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS) > > >>> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS) > > >>> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v > > >>> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS) > > >>> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS) > > >>> > > >>> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small > > >>> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 > > >>> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > > >>> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) > > >>> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 > > >>> > > >>> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable > > -Wno-unused-const-variable -mcmodel=tiny > > >>> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable > > -Wno-unused-const-variable > > >>> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) > > >>> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 > > >>> > > >>> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small > > >>> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0 > > >>> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > > >>> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0 > > >>> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0 > > >>> > > >>> #################################################################################### > > >>> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS > > >>> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS) > > >>> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS) > > >>> > > >>> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable > > -Wno-unused-const-variable -mcmodel=small > > >>> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os > > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch > > -mcmodel=small > > >>> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable > > -Wno-unused-const-variable > > >>> + DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os > > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch > > >>> DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 > > >>> > > >>> -RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable > > -Wno-unused-const-variable -mcmodel=tiny > > >>> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable > > -Wno-unused-const-variable > > >>> RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os > > -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch > > >>> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 > > >>> > > >>> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small > > >>> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 > > >>> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0 > > >>> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0 > > >>> > > >>> > > >> > > >> Looking at this patch with "--word-diff" helps a little (it's too bad we > > >> can't break these mile long option sequences to multiple lines). So: > > >> > > >> * For GCC49: > > >> > > >> - The compiler option "-mcmodel=small" is upstreamed from both > > >> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to > > >> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the > > >> option will now also apply to RELEASE. > > >> > > >> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is > > >> removed. > > >> > > >> - The same upstreaming occurs for "-z common-page-size=0x1000", in > > >> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits > > >> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an > > >> override, to stick with the original 0x20 alignment. > > >> > > >> * For GCC5: > > >> > > >> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore > > >> the "-mcmodel=small" inheritance from the above is automatic. Only the > > >> DEBUG/NOOPT removals, and the RELEASE fixup are needed. > > >> > > >> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS, > > >> therefore the same inheritance pattern applies to "-z > > >> common-page-size=0x1000". I'm noticing an omission here however: "-z > > >> common-page-size=0x1000" should have been removed from > > >> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well. > > >> > > >> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same. > > >> > > >> > > >> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been > > >> trimmed as well? > > >> > > > > > > Yes, well spotted. > > > > > >> Furthermore, I suggest squeezing the word RELEASE into the subject, > > >> given that the change is only observable in RELEASE builds. > > >> > > > > > > Sure, I'll add that as well. > > > > > > > Thanks! Personally I don't need to see a repost to believe you. :) > > > > With those updates: > > > > Reviewed-by: Laszlo Ersek > > > > Thanks! > > Laszlo