[Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Philippe Mathieu-Daudé]

Hi,

Two trivial fixes to avoid the latest EDK2 testing series to
cause trouble to downstream distributions (in particular if
they have PIE enforced).

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  roms: Rename the EFIROM variable to avoid clashing with iPXE
  roms: Allow the EDK2_EFIROM variable to be overridden

 roms/Makefile | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Philippe Mathieu-Daudé]

Hi,

Two trivial fixes to avoid the latest EDK2 testing series to
cause trouble to downstream distributions (in particular if
they have PIE enforced).

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  roms: Rename the EFIROM variable to avoid clashing with iPXE
  roms: Allow the EDK2_EFIROM variable to be overridden

 roms/Makefile | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH for-4.0 v2 1/2] roms: Rename the EFIROM variable to avoid clashing with iPXE

[Philippe Mathieu-Daudé]

The iPXE project already uses the EFIROM for a tool named 'efirom'
which is not the Intel EfiRom used by the EDK2 project. To make
the difference obvious, rename the variable.

This fixes a long standing issue which is now masked since commit
f590a812c21 "roms: build the EfiRom utility from the roms/edk2
submodule" (but would reappear in the next commit):

  $ git checkout f590a812c21~
  $ make -C roms clean EFIROM=$(type -P EfiRom)
  make: Entering directory '/source/qemu/roms'
  [...]
  make -C ipxe/src veryclean
  make[1]: Entering directory '/source/qemu/roms/ipxe/src'
  rm -f bin{,-*}/*.* bin{,-*}/.certificate.* bin{,-*}/.certificates.* bin{,-*}/.private_key.* bin{,-*}/errors bin{,-*}/NIC ./util/zbin ./util/elf2efi32 ./util/elf2efi64 /usr/bin/EfiRom ./util/efifatbin ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
  rm: cannot remove '/usr/bin/EfiRom': Permission denied
  make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
  make[1]: Leaving directory '/source/qemu/roms/ipxe/src'
  make: *** [Makefile:152: clean] Error 2
  make: Leaving directory '/source/qemu/roms'

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

diff --git a/roms/Makefile b/roms/Makefile
index 78d5dd18c30..d28252dafdf 100644
--- a/roms/Makefile
+++ b/roms/Makefile
@@ -47,7 +47,7 @@ SEABIOS_EXTRAVERSION="-prebuilt.qemu.org"
 # We need that to combine multiple images (legacy bios,
 # efi ia32, efi x64) into a single rom binary.
 #
-EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
+EDK2_EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
 
 default:
 	@echo "nothing is build by default"
@@ -102,8 +102,8 @@ pxe-rom-%: build-pxe-roms
 
 efirom: $(patsubst %,efi-rom-%,$(pxerom_variants))
 
-efi-rom-%: build-pxe-roms build-efi-roms $(EFIROM)
-	$(EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
+efi-rom-%: build-pxe-roms build-efi-roms $(EDK2_EFIROM)
+	$(EDK2_EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
 		-b ipxe/src/bin/$(VID)$(DID).rom \
 		-ec ipxe/src/bin-i386-efi/$(VID)$(DID).efidrv \
 		-ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \
@@ -120,7 +120,7 @@ build-efi-roms: build-pxe-roms
 		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
 		$(patsubst %,bin-x86_64-efi/%.efidrv,$(pxerom_targets))
 
-$(EFIROM):
+$(EDK2_EFIROM):
 	$(MAKE) -C edk2/BaseTools
 
 slof:
-- 
2.20.1

[Qemu-devel] [PATCH for-4.0 v2 1/2] roms: Rename the EFIROM variable to avoid clashing with iPXE

[Philippe Mathieu-Daudé]

The iPXE project already uses the EFIROM for a tool named 'efirom'
which is not the Intel EfiRom used by the EDK2 project. To make
the difference obvious, rename the variable.

This fixes a long standing issue which is now masked since commit
f590a812c21 "roms: build the EfiRom utility from the roms/edk2
submodule" (but would reappear in the next commit):

  $ git checkout f590a812c21~
  $ make -C roms clean EFIROM=$(type -P EfiRom)
  make: Entering directory '/source/qemu/roms'
  [...]
  make -C ipxe/src veryclean
  make[1]: Entering directory '/source/qemu/roms/ipxe/src'
  rm -f bin{,-*}/*.* bin{,-*}/.certificate.* bin{,-*}/.certificates.* bin{,-*}/.private_key.* bin{,-*}/errors bin{,-*}/NIC ./util/zbin ./util/elf2efi32 ./util/elf2efi64 /usr/bin/EfiRom ./util/efifatbin ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
  rm: cannot remove '/usr/bin/EfiRom': Permission denied
  make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
  make[1]: Leaving directory '/source/qemu/roms/ipxe/src'
  make: *** [Makefile:152: clean] Error 2
  make: Leaving directory '/source/qemu/roms'

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

diff --git a/roms/Makefile b/roms/Makefile
index 78d5dd18c30..d28252dafdf 100644
--- a/roms/Makefile
+++ b/roms/Makefile
@@ -47,7 +47,7 @@ SEABIOS_EXTRAVERSION="-prebuilt.qemu.org"
 # We need that to combine multiple images (legacy bios,
 # efi ia32, efi x64) into a single rom binary.
 #
-EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
+EDK2_EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
 
 default:
 	@echo "nothing is build by default"
@@ -102,8 +102,8 @@ pxe-rom-%: build-pxe-roms
 
 efirom: $(patsubst %,efi-rom-%,$(pxerom_variants))
 
-efi-rom-%: build-pxe-roms build-efi-roms $(EFIROM)
-	$(EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
+efi-rom-%: build-pxe-roms build-efi-roms $(EDK2_EFIROM)
+	$(EDK2_EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
 		-b ipxe/src/bin/$(VID)$(DID).rom \
 		-ec ipxe/src/bin-i386-efi/$(VID)$(DID).efidrv \
 		-ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \
@@ -120,7 +120,7 @@ build-efi-roms: build-pxe-roms
 		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
 		$(patsubst %,bin-x86_64-efi/%.efidrv,$(pxerom_targets))
 
-$(EFIROM):
+$(EDK2_EFIROM):
 	$(MAKE) -C edk2/BaseTools
 
 slof:
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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 0/2] roms: Rename the EFIROM variable and let it be overridable

[Laszlo Ersek]

On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
> Hi,
>
> Two trivial fixes to avoid the latest EDK2 testing series to
> cause trouble to downstream distributions (in particular if
> they have PIE enforced).

I disgree with this.

(1) In the first commit message, you say,

"The iPXE project already uses the EFIROM for a tool named 'efirom'
which is not the Intel EfiRom used by the EDK2 project".

That's wrong. For building the combined (UEFI+BIOS) iPXE oprom binaries,
the efirom tool that is invoked is *most definitely* the tool from edk2.

What changes is that we now build efirom directly from the edk2
submodule, rather than using a binary that could possibly be found on a
GNU/Linux system from another package.

This is entirely aligned with the addition of the edk2 submodule. The
source for the efirom tool is now directly available, so in a
*maintainer* build -- i.e., when you decide to rebuild iPXE binaries --
we should certainly prefer to build everything from source.

Again, this is a *maintainer* build (which also covers downstream
package builds), not end-user build. If you decide to rebuild artifacts
that are otherwise offered in binary form to end-users, you commit to
building everything from source that goes into (or is necessary for)
producing those artifacts.

In the thread "edk2 fails to compile in v4.0.0-rc2", Olaf wrote,

"It is in ovmf-tools.rpm, which comes from ovmf."

That only proves my point.

(2) For a while now, it has been possible for downstream build scripts
to inject their preferred build flags into the BaseTools build recipes
(makefiles) themselves. Please see
<https://bugzilla.redhat.com/show_bug.cgi?id=1540244>. This is the
relevant upstream commit list:

  1  67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last
                  for C files too
  2  03252ae287c4 BaseTools/header.makefile: remove "-c" from
                  BUILD_CFLAGS
  3  b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
  4  b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the
                  caller
  5  81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
  6  aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS

Build BaseTools as follows:

  make -C "$EDK_TOOLS_PATH" EXTRA_OPTFLAGS="..."  EXTRA_LDFLAGS="..."

If you need to inject PIC/PIE-related flags into the BaseTools
compilation/linking, please use the above facility.

I think it's pretty usual that new upstream releases (of any open source
project) bring some changes for downstream packaging scripts.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Laszlo Ersek]

On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
> Hi,
>
> Two trivial fixes to avoid the latest EDK2 testing series to
> cause trouble to downstream distributions (in particular if
> they have PIE enforced).

I disgree with this.

(1) In the first commit message, you say,

"The iPXE project already uses the EFIROM for a tool named 'efirom'
which is not the Intel EfiRom used by the EDK2 project".

That's wrong. For building the combined (UEFI+BIOS) iPXE oprom binaries,
the efirom tool that is invoked is *most definitely* the tool from edk2.

What changes is that we now build efirom directly from the edk2
submodule, rather than using a binary that could possibly be found on a
GNU/Linux system from another package.

This is entirely aligned with the addition of the edk2 submodule. The
source for the efirom tool is now directly available, so in a
*maintainer* build -- i.e., when you decide to rebuild iPXE binaries --
we should certainly prefer to build everything from source.

Again, this is a *maintainer* build (which also covers downstream
package builds), not end-user build. If you decide to rebuild artifacts
that are otherwise offered in binary form to end-users, you commit to
building everything from source that goes into (or is necessary for)
producing those artifacts.

In the thread "edk2 fails to compile in v4.0.0-rc2", Olaf wrote,

"It is in ovmf-tools.rpm, which comes from ovmf."

That only proves my point.

(2) For a while now, it has been possible for downstream build scripts
to inject their preferred build flags into the BaseTools build recipes
(makefiles) themselves. Please see
<https://bugzilla.redhat.com/show_bug.cgi?id=1540244>. This is the
relevant upstream commit list:

  1  67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last
                  for C files too
  2  03252ae287c4 BaseTools/header.makefile: remove "-c" from
                  BUILD_CFLAGS
  3  b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
  4  b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the
                  caller
  5  81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
  6  aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS

Build BaseTools as follows:

  make -C "$EDK_TOOLS_PATH" EXTRA_OPTFLAGS="..."  EXTRA_LDFLAGS="..."

If you need to inject PIC/PIE-related flags into the BaseTools
compilation/linking, please use the above facility.

I think it's pretty usual that new upstream releases (of any open source
project) bring some changes for downstream packaging scripts.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Laszlo Ersek]

On 04/08/19 11:02, Laszlo Ersek wrote:
> On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> Two trivial fixes to avoid the latest EDK2 testing series to
>> cause trouble to downstream distributions (in particular if
>> they have PIE enforced).
> 
> I disgree with this.
> 
> (1) In the first commit message, you say,
> 
> "The iPXE project already uses the EFIROM for a tool named 'efirom'
> which is not the Intel EfiRom used by the EDK2 project".
> 
> That's wrong. For building the combined (UEFI+BIOS) iPXE oprom binaries,
> the efirom tool that is invoked is *most definitely* the tool from edk2.
> 
> What changes is that we now build efirom directly from the edk2
> submodule, rather than using a binary that could possibly be found on a
> GNU/Linux system from another package.
> 
> This is entirely aligned with the addition of the edk2 submodule. The
> source for the efirom tool is now directly available, so in a
> *maintainer* build -- i.e., when you decide to rebuild iPXE binaries --
> we should certainly prefer to build everything from source.
> 
> Again, this is a *maintainer* build (which also covers downstream
> package builds), not end-user build. If you decide to rebuild artifacts
> that are otherwise offered in binary form to end-users, you commit to
> building everything from source that goes into (or is necessary for)
> producing those artifacts.
> 
> In the thread "edk2 fails to compile in v4.0.0-rc2", Olaf wrote,
> 
> "It is in ovmf-tools.rpm, which comes from ovmf."
> 
> That only proves my point.

Of course, ipxe.git itself has a macro called EFIROM:

src/Makefile:EFIROM             := ./util/efirom

but that has *zero* relevance for QEMU commit f590a812c210 ("roms: build
the EfiRom utility from the roms/edk2 submodule", 2019-02-21).

Because, the "efirom" utility from inside ipxe.git is not used for
anything at all in the *QEMU recipe* for building the combined iPXE
option ROMs.

Look at the diff in f590a812c210:

-EFIROM ?= $(shell which EfiRom 2>/dev/null)
+EFIROM = edk2/BaseTools/Source/C/bin/EfiRom

If the way we build the combined iPXE oproms, from QEMU's ipxe.git
submodule, using "roms/Makefile", had *anything* to do with iPXE's own
"efirom", then the pre-patch state would have been wrong *already*.

Laszlo

> (2) For a while now, it has been possible for downstream build scripts
> to inject their preferred build flags into the BaseTools build recipes
> (makefiles) themselves. Please see
> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>. This is the
> relevant upstream commit list:
> 
>   1  67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last
>                   for C files too
>   2  03252ae287c4 BaseTools/header.makefile: remove "-c" from
>                   BUILD_CFLAGS
>   3  b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
>   4  b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the
>                   caller
>   5  81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
>   6  aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS
> 
> Build BaseTools as follows:
> 
>   make -C "$EDK_TOOLS_PATH" EXTRA_OPTFLAGS="..."  EXTRA_LDFLAGS="..."
> 
> If you need to inject PIC/PIE-related flags into the BaseTools
> compilation/linking, please use the above facility.
> 
> I think it's pretty usual that new upstream releases (of any open source
> project) bring some changes for downstream packaging scripts.
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH for-4.0 v2 0/2] roms: Rename the EFIROM variable and let it be overridable

[Laszlo Ersek]

On 04/08/19 11:02, Laszlo Ersek wrote:
> On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> Two trivial fixes to avoid the latest EDK2 testing series to
>> cause trouble to downstream distributions (in particular if
>> they have PIE enforced).
> 
> I disgree with this.
> 
> (1) In the first commit message, you say,
> 
> "The iPXE project already uses the EFIROM for a tool named 'efirom'
> which is not the Intel EfiRom used by the EDK2 project".
> 
> That's wrong. For building the combined (UEFI+BIOS) iPXE oprom binaries,
> the efirom tool that is invoked is *most definitely* the tool from edk2.
> 
> What changes is that we now build efirom directly from the edk2
> submodule, rather than using a binary that could possibly be found on a
> GNU/Linux system from another package.
> 
> This is entirely aligned with the addition of the edk2 submodule. The
> source for the efirom tool is now directly available, so in a
> *maintainer* build -- i.e., when you decide to rebuild iPXE binaries --
> we should certainly prefer to build everything from source.
> 
> Again, this is a *maintainer* build (which also covers downstream
> package builds), not end-user build. If you decide to rebuild artifacts
> that are otherwise offered in binary form to end-users, you commit to
> building everything from source that goes into (or is necessary for)
> producing those artifacts.
> 
> In the thread "edk2 fails to compile in v4.0.0-rc2", Olaf wrote,
> 
> "It is in ovmf-tools.rpm, which comes from ovmf."
> 
> That only proves my point.

Of course, ipxe.git itself has a macro called EFIROM:

src/Makefile:EFIROM             := ./util/efirom

but that has *zero* relevance for QEMU commit f590a812c210 ("roms: build
the EfiRom utility from the roms/edk2 submodule", 2019-02-21).

Because, the "efirom" utility from inside ipxe.git is not used for
anything at all in the *QEMU recipe* for building the combined iPXE
option ROMs.

Look at the diff in f590a812c210:

-EFIROM ?= $(shell which EfiRom 2>/dev/null)
+EFIROM = edk2/BaseTools/Source/C/bin/EfiRom

If the way we build the combined iPXE oproms, from QEMU's ipxe.git
submodule, using "roms/Makefile", had *anything* to do with iPXE's own
"efirom", then the pre-patch state would have been wrong *already*.

Laszlo

> (2) For a while now, it has been possible for downstream build scripts
> to inject their preferred build flags into the BaseTools build recipes
> (makefiles) themselves. Please see
> <https://bugzilla.redhat.com/show_bug.cgi?id=1540244>. This is the
> relevant upstream commit list:
> 
>   1  67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last
>                   for C files too
>   2  03252ae287c4 BaseTools/header.makefile: remove "-c" from
>                   BUILD_CFLAGS
>   3  b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
>   4  b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the
>                   caller
>   5  81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
>   6  aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS
> 
> Build BaseTools as follows:
> 
>   make -C "$EDK_TOOLS_PATH" EXTRA_OPTFLAGS="..."  EXTRA_LDFLAGS="..."
> 
> If you need to inject PIC/PIE-related flags into the BaseTools
> compilation/linking, please use the above facility.
> 
> I think it's pretty usual that new upstream releases (of any open source
> project) bring some changes for downstream packaging scripts.
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH for-4.0 v2 1/2] roms: Rename the EFIROM variable to avoid clashing with iPXE

[Laszlo Ersek]

On 04/05/19 17:33, Philippe Mathieu-Daudé wrote:
> The iPXE project already uses the EFIROM for a tool named 'efirom'
> which is not the Intel EfiRom used by the EDK2 project. To make
> the difference obvious, rename the variable.
> 
> This fixes a long standing issue which is now masked since commit
> f590a812c21 "roms: build the EfiRom utility from the roms/edk2
> submodule" (but would reappear in the next commit):

[*]

> 
>   $ git checkout f590a812c21~
>   $ make -C roms clean EFIROM=$(type -P EfiRom)
>   make: Entering directory '/source/qemu/roms'
>   [...]
>   make -C ipxe/src veryclean
>   make[1]: Entering directory '/source/qemu/roms/ipxe/src'
>   rm -f bin{,-*}/*.* bin{,-*}/.certificate.* bin{,-*}/.certificates.* bin{,-*}/.private_key.* bin{,-*}/errors bin{,-*}/NIC ./util/zbin ./util/elf2efi32 ./util/elf2efi64 /usr/bin/EfiRom ./util/efifatbin ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
>   rm: cannot remove '/usr/bin/EfiRom': Permission denied
>   make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
>   make[1]: Leaving directory '/source/qemu/roms/ipxe/src'
>   make: *** [Makefile:152: clean] Error 2
>   make: Leaving directory '/source/qemu/roms'
> 
> Reported-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  roms/Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/roms/Makefile b/roms/Makefile
> index 78d5dd18c30..d28252dafdf 100644
> --- a/roms/Makefile
> +++ b/roms/Makefile
> @@ -47,7 +47,7 @@ SEABIOS_EXTRAVERSION="-prebuilt.qemu.org"
>  # We need that to combine multiple images (legacy bios,
>  # efi ia32, efi x64) into a single rom binary.
>  #
> -EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
> +EDK2_EFIROM = edk2/BaseTools/Source/C/bin/EfiRom
>  
>  default:
>  	@echo "nothing is build by default"
> @@ -102,8 +102,8 @@ pxe-rom-%: build-pxe-roms
>  
>  efirom: $(patsubst %,efi-rom-%,$(pxerom_variants))
>  
> -efi-rom-%: build-pxe-roms build-efi-roms $(EFIROM)
> -	$(EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
> +efi-rom-%: build-pxe-roms build-efi-roms $(EDK2_EFIROM)
> +	$(EDK2_EFIROM) -f "0x$(VID)" -i "0x$(DID)" -l 0x02 \
>  		-b ipxe/src/bin/$(VID)$(DID).rom \
>  		-ec ipxe/src/bin-i386-efi/$(VID)$(DID).efidrv \
>  		-ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \
> @@ -120,7 +120,7 @@ build-efi-roms: build-pxe-roms
>  		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
>  		$(patsubst %,bin-x86_64-efi/%.efidrv,$(pxerom_targets))
>  
> -$(EFIROM):
> +$(EDK2_EFIROM):
>  	$(MAKE) -C edk2/BaseTools
>  
>  slof:
> 

Upon reading your and Olaf's comments in the v1 thread:

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

I've changed my mind on this series.

Please insert a paragraph at [*] in the commit message:

"iPXE's 'veryclean' recipe removes $(EFIROM) even if the EFIROM macro
originates from elsewhere."

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I'll comment under patch#2 separately.)

Thank you for picking this up.
Laszlo

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

[Laszlo Ersek]

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

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

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

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

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

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Laszlo Ersek]

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

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

[Laszlo Ersek]

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