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 <ard.biesheu...@linaro.org>
> ---
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47040): https://edk2.groups.io/g/devel/message/47040
Mute This Topic: https://groups.io/mt/33207946/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
