Re: [PATCH v2] ArmVirtPkg: increase FD/FV size for NOOPT builds

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Thu, 12 Sep 2019 at 15:46, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 09/11/19 18:23, 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 <ard.biesheuvel@linaro.org>
> > ---
> > v2: implement suggestions by Laszlo on 1) how to parameterize this further,
> >     and b) to avoid adding another .inc file
> >     update kernel header field, as pointed out by Philippe
> >
> >  ArmVirtPkg/ArmVirt.dsc.inc       | 28 ++++++++++++++++++++
> >  ArmVirtPkg/ArmVirtQemu.fdf       | 14 +++++++---
> >  ArmVirtPkg/ArmVirtQemuKernel.fdf | 19 ++++++++++---
> >  ArmVirtPkg/ArmVirtXen.fdf        | 14 +++++++---
> >  4 files changed, 66 insertions(+), 9 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> > index a4ae25d982a2..d6b58e5c018b 100644
> > --- a/ArmVirtPkg/ArmVirt.dsc.inc
> > +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> > @@ -10,6 +10,34 @@
> >  [Defines]
> >    DEFINE DEBUG_PRINT_ERROR_LEVEL = 0x8000004F
> >
> > +  #
> > +  # 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.
> > +  #
> > +!if $(TARGET) == NOOPT
> > +  DEFINE FD_SIZE_3MB      = TRUE
> > +!endif
>
> Sorry, I must have been unclear -- I meant the macros FD_SIZE_2MB /
> FD_SIZE_3MB / FD_SIZE_IN_KB as a replacement for the $(TARGET) based
> logic, not as an addition to it.
>
> Is it important to select the 3MB build by default, in case "-b NOOPT"
> is given on the build command line?
>

Yes.

> My thinking was that, in your CI env, you could pass -D FD_SIZE_3MB
> specifically, whenever you build for -b NOOPT. Other users building for
> -b NOOPT would see no change.
>
> My suggestion would be:
>
> (1) s/for NOOPT builds/with -D FD_SIZE_3MB/ in the commit message
>
> (2) drop the above three lines (the TARGET-based conditional)
>
> With those:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> and you could go ahead and push the patch (no repost needed).
>
> OTOH, if you'd really like to set 3MB for NOOPT automatically, then I
> think we'll need a way for letting users override that new default, back
> to 2MB.
>
> ... Or is such an override possible with this patch already, perhaps?
>

Yes. If you set -b NOOPT -D FD_SIZE_2MB (in any order), you will get a
2 MB image.