From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 09 Sep 2019 06:42:11 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 14C1B18CB51C; Mon, 9 Sep 2019 13:42:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-196.ams2.redhat.com [10.36.116.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0B22B5C22C; Mon, 9 Sep 2019 13:42:08 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg: increase FD/FV size for NOOPT builds To: Ard Biesheuvel , devel@edk2.groups.io Cc: leif.lindholm@linaro.org References: <20190906222004.30161-1-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <642dad32-75d4-f605-650d-a4daf63a6474@redhat.com> Date: Mon, 9 Sep 2019 15:42:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190906222004.30161-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.62]); Mon, 09 Sep 2019 13:42:10 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/07/19 00:20, Ard Biesheuvel wrote: > After upgrading the CI system we use for building the ArmVirtPkg > targets, we started seeing failures due to the NOOPT build running > out of space when using the CLANG38 toolchain definition combined > with clang 7. > > We really don't want to increase the FD/FV sizes in general to > accommodate this, so parameterize the relevant quantities and > increase them by 50% for NOOPT builds. > > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirt.fdf.inc | 13 +++++++++++++ > ArmVirtPkg/ArmVirtQemu.fdf | 14 +++++++++++--- > ArmVirtPkg/ArmVirtQemuKernel.fdf | 14 +++++++++++--- > ArmVirtPkg/ArmVirtXen.fdf | 14 +++++++++++--- > 4 files changed, 46 insertions(+), 9 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.fdf.inc b/ArmVirtPkg/ArmVirt.fdf.inc > new file mode 100644 > index 000000000000..5fdebcf5dc93 > --- /dev/null > +++ b/ArmVirtPkg/ArmVirt.fdf.inc > @@ -0,0 +1,13 @@ > +# > +# Copyright (c) 2019, Linaro Limited. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > + > +!if $(TARGET) != NOOPT > +DEFINE FD_SIZE = 0x200000 > +DEFINE FD_NUM_BLOCKS = 0x200 > +!else > +DEFINE FD_SIZE = 0x300000 > +DEFINE FD_NUM_BLOCKS = 0x300 > +!endif > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf > index c2169cb7964b..27dd5bf09a91 100644 > --- a/ArmVirtPkg/ArmVirtQemu.fdf > +++ b/ArmVirtPkg/ArmVirtQemu.fdf > @@ -20,14 +20,22 @@ > # > ################################################################################ > > +[Defines] > +!include ArmVirt.fdf.inc > +!if $(TARGET) != NOOPT > +DEFINE FVMAIN_COMPACT_SIZE = 0x1ff000 > +!else > +DEFINE FVMAIN_COMPACT_SIZE = 0x2ff000 > +!endif > + > [FD.QEMU_EFI] > BaseAddress = 0x00000000|gArmTokenSpaceGuid.PcdFdBaseAddress # QEMU assigns 0 - 0x8000000 for a BootROM > -Size = 0x00200000|gArmTokenSpaceGuid.PcdFdSize # The size in bytes of the FLASH Device > +Size = $(FD_SIZE)|gArmTokenSpaceGuid.PcdFdSize # The size in bytes of the FLASH Device > ErasePolarity = 1 > > # This one is tricky, it must be: BlockSize * NumBlocks = Size > BlockSize = 0x00001000 > -NumBlocks = 0x200 > +NumBlocks = $(FD_NUM_BLOCKS) > > ################################################################################ > # > @@ -59,7 +67,7 @@ DATA = { > !endif > } > > -0x00001000|0x001ff000 > +0x00001000|$(FVMAIN_COMPACT_SIZE) > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > FV = FVMAIN_COMPACT > > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.fdf b/ArmVirtPkg/ArmVirtQemuKernel.fdf > index f675b6d65ee1..1836697a0a90 100644 > --- a/ArmVirtPkg/ArmVirtQemuKernel.fdf > +++ b/ArmVirtPkg/ArmVirtQemuKernel.fdf > @@ -20,14 +20,22 @@ > # > ################################################################################ > > +[Defines] > +!include ArmVirt.fdf.inc > +!if $(TARGET) != NOOPT > +DEFINE FVMAIN_COMPACT_SIZE = 0x1f8000 > +!else > +DEFINE FVMAIN_COMPACT_SIZE = 0x2f8000 > +!endif > + > [FD.QEMU_EFI] > BaseAddress = 0x00000000|gArmTokenSpaceGuid.PcdFdBaseAddress # QEMU assigns 0 - 0x8000000 for a BootROM > -Size = 0x00200000|gArmTokenSpaceGuid.PcdFdSize # The size in bytes of the FLASH Device > +Size = $(FD_SIZE)|gArmTokenSpaceGuid.PcdFdSize # The size in bytes of the FLASH Device > ErasePolarity = 1 > > # This one is tricky, it must be: BlockSize * NumBlocks = Size > BlockSize = 0x00001000 > -NumBlocks = 0x200 > +NumBlocks = $(FD_NUM_BLOCKS) > > ################################################################################ > # > @@ -81,7 +89,7 @@ DATA = { > !endif > } > > -0x00008000|0x001f8000 > +0x00008000|$(FVMAIN_COMPACT_SIZE) > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > FV = FVMAIN_COMPACT > > diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf > index 79f681cde028..4007f49a08fb 100644 > --- a/ArmVirtPkg/ArmVirtXen.fdf > +++ b/ArmVirtPkg/ArmVirtXen.fdf > @@ -20,14 +20,22 @@ > # > ################################################################################ > > +[Defines] > +!include ArmVirt.fdf.inc > +!if $(TARGET) != NOOPT > +DEFINE FVMAIN_COMPACT_SIZE = 0x1fe000 > +!else > +DEFINE FVMAIN_COMPACT_SIZE = 0x2fe000 > +!endif > + > [FD.XEN_EFI] > BaseAddress = 0x00000000|gArmTokenSpaceGuid.PcdFdBaseAddress > -Size = 0x00200000|gArmTokenSpaceGuid.PcdFdSize > +Size = $(FD_SIZE)|gArmTokenSpaceGuid.PcdFdSize > ErasePolarity = 1 > > # This one is tricky, it must be: BlockSize * NumBlocks = Size > BlockSize = 0x00001000 > -NumBlocks = 0x200 > +NumBlocks = $(FD_NUM_BLOCKS) > > ################################################################################ > # > @@ -81,7 +89,7 @@ DATA = { > !endif > } > > -0x00002000|0x001fe000 > +0x00002000|$(FVMAIN_COMPACT_SIZE) > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize > FV = FVMAIN_COMPACT > > (1) I suggest not introducing the new file "ArmVirt.fdf.inc". Instead, the same DEFINEs could go into the [Defines] section of "ArmVirt.dsc.inc". This is because the platform build is rooted in the DSC file, and the FDF file is only reached because the DSC has a pointer to it (the FLASH_DEFINITION define). Therefore, whatever DEFINE we add to the DSC, the FDF should see it. And, all platform DSCs include "ArmVirt.dsc.inc". Furthermore, we already have a (small) [Defines] section in "ArmVirt.dsc.inc". (2) Can we control this feature with an orthogonal flag? -D FD_SIZE_3MB In fact, tailoring the pattern seen under OvmfPkg, we could add the following to "ArmVirt.dsc.inc": # # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to # one of the supported values, in place of any of the convenience macros, is # permitted. # !ifdef $(FD_SIZE_2MB) DEFINE FD_SIZE_IN_KB = 2048 !else !ifdef $(FD_SIZE_3MB) DEFINE FD_SIZE_IN_KB = 3072 !else DEFINE FD_SIZE_IN_KB = 2048 !endif !endif And then the conditionals would change from !if $(TARGET) != NOOPT ... !else ... !endif to !if $(FD_SIZE_IN_KB) == 2048 ... !endif !if $(FD_SIZE_IN_KB) == 3072 ... !endif I would like that approach because: - you could add the new flag in your CI env, without changing behavior for other NOOPT builds (-D FD_SIZE_3MB, or -D FD_SIZE_IN_KB=3072) - adding further FD size options would be quite clean (only one place to mess with "!else"). Otherwise, the patch looks fine to me. Thanks! Laszlo