1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable
  4. View patch

[Qemu-devel] [PATCH for-4.0 v2 2/2] roms: Allow the EDK2_EFIROM variable to be overridden

Philippe Mathieu-Daudé posted 2 patches 6 years, 8 months ago
[Qemu-devel] [PATCH for-4.0 v2 2/2] roms: Allow the EDK2_EFIROM variable to be overridden
Posted by Philippe Mathieu-Daudé 6 years, 8 months ago 
Since commit f590a812c210 we build the EDK2 EfiRom utility
unconditionally. This has been tested on all the Linux
distribution providing continuous integration (namely Debian
and Fedora). Not all distributions are able to build the
EfiRom without specific patches (In particular SUSE which
enforces the PIE protection, see [*]).

Restore the possibility to other distributions to override
the EDK2_EFIROM variable.

[*] https://lists.opensuse.org/opensuse-factory/2017-06/msg00403.html

Reported-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 roms/Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/roms/Makefile b/roms/Makefile
index d28252dafdf..ea19aa9b33c 100644
--- a/roms/Makefile
+++ b/roms/Makefile
@@ -47,7 +47,11 @@ SEABIOS_EXTRAVERSION="-prebuilt.qemu.org"
 # We need that to combine multiple images (legacy bios,
 # efi ia32, efi x64) into a single rom binary.
 #
-EDK2_EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
+# By default we build the latest EDK2 stable EfiRom utility.
+# If you have to use another one, you can also pass the location on
+# the command line, i.e. "make EDK2_EFIROM=$(type -P EfiRom) efirom"
+#
+EDK2_EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom
 
 default:
 	@echo "nothing is build by default"
@@ -120,8 +124,11 @@ build-efi-roms: build-pxe-roms
 		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
 		$(patsubst %,bin-x86_64-efi/%.efidrv,$(pxerom_targets))
 
+# Do not compile $(EDK2_EFIROM) if the variable is overridden
+ifeq "$(origin EDK2_EFIROM)" "file"
 $(EDK2_EFIROM):
 	$(MAKE) -C edk2/BaseTools
+endif
 
 slof:
 	$(MAKE) -C SLOF CROSS=$(powerpc64_cross_prefix) qemu
-- 
2.20.1
Re: [Qemu-devel] [PATCH for-4.0 v2 2/2] roms: Allow the EDK2_EFIROM variable to be overridden
Posted by Laszlo Ersek 6 years, 8 months ago 
On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
> Since commit f590a812c210 we build the EDK2 EfiRom utility
> unconditionally. This has been tested on all the Linux
> distribution providing continuous integration (namely Debian
> and Fedora). Not all distributions are able to build the
> EfiRom without specific patches (In particular SUSE which
> enforces the PIE protection, see [*]).
> 
> Restore the possibility to other distributions to override
> the EDK2_EFIROM variable.
> 
> [*] https://lists.opensuse.org/opensuse-factory/2017-06/msg00403.html
> 
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  roms/Makefile | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/roms/Makefile b/roms/Makefile
> index d28252dafdf..ea19aa9b33c 100644
> --- a/roms/Makefile
> +++ b/roms/Makefile
> @@ -47,7 +47,11 @@ SEABIOS_EXTRAVERSION="-prebuilt.qemu.org"
>  # We need that to combine multiple images (legacy bios,
>  # efi ia32, efi x64) into a single rom binary.
>  #
> -EDK2_EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
> +# By default we build the latest EDK2 stable EfiRom utility.
> +# If you have to use another one, you can also pass the location on
> +# the command line, i.e. "make EDK2_EFIROM=$(type -P EfiRom) efirom"
> +#
> +EDK2_EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom
>  
>  default:
>  	@echo "nothing is build by default"
> @@ -120,8 +124,11 @@ build-efi-roms: build-pxe-roms
>  		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
>  		$(patsubst %,bin-x86_64-efi/%.efidrv,$(pxerom_targets))
>  
> +# Do not compile $(EDK2_EFIROM) if the variable is overridden
> +ifeq "$(origin EDK2_EFIROM)" "file"
>  $(EDK2_EFIROM):
>  	$(MAKE) -C edk2/BaseTools
> +endif
>  
>  slof:
>  	$(MAKE) -C SLOF CROSS=$(powerpc64_cross_prefix) qemu
> 

I agree with the problem statement, from Olaf's message in the

  [PATCH for-4.0] roms: Allow the EFIROM variable to be overridden

thread: "roms/ seems to be unable to receive configure options".

That's the issue in need of a fix.


The present patch would break what was fixed by f590a812c210. In other
words, we should stick with using a single EfiRom for all purposes (of
roms/Makefile), but we should let the caller easily inject compiler &
linker options.

So I think the recipe should do something like:

	$(MAKE) -C edk2/BaseTools \
		EXTRA_OPTFLAGS='$(EDK2_BASETOOLS_OPTFLAGS)' \
		EXTRA_LDFLAGS='$(EDK2_BASETOOLS_LDFLAGS)'

Then build scripts could be updated to invoke:

  make -C roms \
    EDK2_BASETOOLS_OPTFLAGS='...' \
    EDK2_BASETOOLS_LDFLAGS='...' \
    efirom

Thanks,
Laszlo
Re: [Qemu-devel] [PATCH for-4.0 v2 2/2] roms: Allow the EDK2_EFIROM variable to be overridden
Posted by Olaf Hering 6 years, 8 months ago 
Am Mon, 8 Apr 2019 13:05:07 +0200
schrieb Laszlo Ersek <lersek@redhat.com>:

> Then build scripts could be updated to invoke:
> 
>   make -C roms \
>     EDK2_BASETOOLS_OPTFLAGS='...' \
>     EDK2_BASETOOLS_LDFLAGS='...' \
>     efirom

The question remains: 'But why?'.
Why can edk2 not be built with '-fno-pie' right away?
Did noone approach the edk2 developers yet that their buildsystem is broken in that regard?

Olaf
Re: [Qemu-devel] [PATCH for-4.0 v2 2/2] roms: Allow the EDK2_EFIROM variable to be overridden
Posted by Laszlo Ersek 6 years, 8 months ago 
On 04/10/19 08:25, Olaf Hering wrote:
> Am Mon, 8 Apr 2019 13:05:07 +0200
> schrieb Laszlo Ersek <lersek@redhat.com>:
>
>> Then build scripts could be updated to invoke:
>>
>>   make -C roms \
>>     EDK2_BASETOOLS_OPTFLAGS='...' \
>>     EDK2_BASETOOLS_LDFLAGS='...' \
>>     efirom
>
> The question remains: 'But why?'.

Because it lets you pass "-fno-pie" & friends.

> Why can edk2 not be built with '-fno-pie' right away?

Those flags are not universal across gcc/binutils versions.

Some gcc/binutils versions don't enable PIE to begin with.

Some gcc/binutils versions take different PIE-disablement flags
(considering both the compiler and the linker) from other gcc/binutils
versions.

For example, please refer to the following *iPXE* commit:

> commit 7c395b0e21806b946fe944a27fc273407f357ea1
> Author: Michael Brown <mcb30@ipxe.org>
> Date:   Wed Jun 14 12:33:16 2017 +0100
>
>     [build] Use -no-pie on newer versions of gcc
>
>     Some distributions patch gcc to generate position independent
>     executables by default.  We currently include a workaround to check
>     for this and to add -fno-PIE -nopie to CFLAGS if required.
>
>     Newer patched versions of gcc require -fno-PIE -no-pie instead.  Check
>     for both variants.
>
>     Reported-by: Nathan Rennie-Waldock <nathan.renniewaldock@gmail.com>
>     Originally-fixed-by: Markos Chandras <mchandras@suse.de>
>     Signed-off-by: Michael Brown <mcb30@ipxe.org>

(I remember this commit because the logic it had added actually failed
on a system that I used once for building iPXE -- it was an x86_64
system with both a native gcc, and a *different version*
x86_64-to-x86_64 *cross* gcc. The selection logic determined the flags
for the one compiler toolchain, but then passed the flags to the other
compiler toolchain. The build broke. I narrowed it down to the above
commit, and then shrugged it off; it wasn't important enough to spend
more time on it.)

I also refer you to the following *edk2* commit:

> commit 11d0cd23dd1bc15a6e6a1598250ea2e0c4c36e9a
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Mon Jun 18 10:23:49 2018 +0200
>
>     BaseTools/tools_def IA32: drop -no-pie linker option for GCC49
>
>     As reported by Liming, GCC 4.9.2 does not support the -no-pie
>     linker option that we added to the GCC49 and GCC5 toolchain
>     profiles in commit c25d3905523a ("BaseTools/tools_def IA32:
>     disable PIE code generation explicitly") to work around issues
>     with recent distro toolchains that enable PIE code generation
>     by default.
>
>     So rollback the changes for GCC49 but preserve them for GCC5
>
>     Contributed-under: TianoCore Contribution Agreement 1.1
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     Acked-by: Laszlo Ersek <lersek@redhat.com>

The above facility will let you stuff the options into the edk2
BaseTools build, in your downstream qemu.spec file, that match your
downstream gcc/binutils version.

Most build hosts will need no flags.

> Did noone approach the edk2 developers yet that their buildsystem is
> broken in that regard?

Well, have you?

I don't remember anyone else reporting such an issue yet, on edk2-devel
or elsewhere, with building BaseTools. I certainly remember
collaborating with Gary Lin from SUSE on various stuff, but not this.

You are welcome to file a bug at <https://bugzilla.tianocore.org/>.
Please pick "EDK2" for "Product", "Code" for "Component", and
"BaseTools" for "Package".

... Please don't try to force a "zero build-interface changes" policy on
upstream, just so you can avoid a small tweak in a downstream build
script when you rebase. Not even the gcc/binutils command lines conform
to that. We all hope downstream rebases to be painless, and upstreams do
strive to help with that, but the downstream rebase experience is never
*entirely* painless (or automated).

Thanks,
Laszlo

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.