roms: Allow the EFIROM variable to be overridden

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

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

Since commit f590a812c210 we build the 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 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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/roms/Makefile b/roms/Makefile
index 78d5dd18c30..0bcfa665ccf 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
+EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom
 
 default:
 	@echo "nothing is build by default"
@@ -120,8 +120,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 $(EFIROM) if the variable is overridden
+ifeq "$(origin EFIROM)" "file"
 $(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] roms: Allow the EFIROM variable to be overridden

Posted by Olaf Hering 6 years, 7 months ago

Am Fri,  5 Apr 2019 13:55:29 +0200
schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:

> +EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom

This name is too generic and will conflict with ipxe.git if any of "bios seavgabios pxerom" is used for 'make -C roms'.

Olaf

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

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

On 4/5/19 2:09 PM, Olaf Hering wrote:
> Am Fri,  5 Apr 2019 13:55:29 +0200
> schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
> 
>> +EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom
> 
> This name is too generic and will conflict with ipxe.git if any of "bios seavgabios pxerom" is used for 'make -C roms'.

This is similar to commit c9d18c1c150c84e where you said "it used to
work", what is the difference?

IPXE override the EFIROM variable, so there is no change there.

How can I trigger a SUSE package build with this patch?

Thanks,

Phil.

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

Posted by Olaf Hering 6 years, 7 months ago

Am Fri, 5 Apr 2019 14:59:16 +0200
schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:

> On 4/5/19 2:09 PM, Olaf Hering wrote:
> > Am Fri,  5 Apr 2019 13:55:29 +0200
> > schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
> >   
> >> +EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom  
> > 
> > This name is too generic and will conflict with ipxe.git if any of "bios seavgabios pxerom" is used for 'make -C roms'.  
> 
> This is similar to commit c9d18c1c150c84e where you said "it used to
> work", what is the difference?

Before there was no need to pass EFIROM= to make, so ipxe.git was happy.
Now it is required, and ipxe.git gets an unexpected value.

> IPXE override the EFIROM variable, so there is no change there.

For me ipxe tries to rm $(type -P EfiRom). Let me double check.

> How can I trigger a SUSE package build with this patch?

Not so easy without a VM.

Olaf

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

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

On 4/5/19 3:04 PM, Olaf Hering wrote:
> Am Fri, 5 Apr 2019 14:59:16 +0200
> schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
>> On 4/5/19 2:09 PM, Olaf Hering wrote:
>>> Am Fri,  5 Apr 2019 13:55:29 +0200
>>> schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
>>>   
>>>> +EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom  
>>>
>>> This name is too generic and will conflict with ipxe.git if any of "bios seavgabios pxerom" is used for 'make -C roms'.  
>>
>> This is similar to commit c9d18c1c150c84e where you said "it used to
>> work", what is the difference?
> 
> Before there was no need to pass EFIROM= to make, so ipxe.git was happy.
> Now it is required, and ipxe.git gets an unexpected value.
> 
>> IPXE override the EFIROM variable, so there is no change there.
> 
> For me ipxe tries to rm $(type -P EfiRom). Let me double check.

Eh you are right...

$ make -C roms EFIROM=/bin/true clean
[...]
make -C ipxe/src veryclean
make[1]: Entering directory '/home/phil/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 /bin/true ./util/efifatbin
./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
rm: cannot remove '/bin/true': Permission denied
make[1]: *** [Makefile.housekeeping:1564: clean] Error 1

This seems to be a pre-existent IPXE bug:

$ git checkout c9d18c1c150c84e7a976df989ad04ddf01083f46
$ make -C roms EFIROM=/bin/true clean
[...]
rm: cannot remove '/bin/true': Permission denied
make[1]: *** [Makefile.housekeeping:1564: clean] Error 1

>> How can I trigger a SUSE package build with this patch?
> 
> Not so easy without a VM.

Oh, unfortunate :(

> 
> Olaf
>

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

Posted by Michael S. Tsirkin 6 years, 7 months ago

On Fri, Apr 05, 2019 at 03:14:30PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/5/19 3:04 PM, Olaf Hering wrote:
> > Am Fri, 5 Apr 2019 14:59:16 +0200
> > schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
> >> On 4/5/19 2:09 PM, Olaf Hering wrote:
> >>> Am Fri,  5 Apr 2019 13:55:29 +0200
> >>> schrieb Philippe Mathieu-Daudé <philmd@redhat.com>:
> >>>   
> >>>> +EFIROM ?= edk2/BaseTools/Source/C/bin/EfiRom  
> >>>
> >>> This name is too generic and will conflict with ipxe.git if any of "bios seavgabios pxerom" is used for 'make -C roms'.  
> >>
> >> This is similar to commit c9d18c1c150c84e where you said "it used to
> >> work", what is the difference?
> > 
> > Before there was no need to pass EFIROM= to make, so ipxe.git was happy.
> > Now it is required, and ipxe.git gets an unexpected value.
> > 
> >> IPXE override the EFIROM variable, so there is no change there.
> > 
> > For me ipxe tries to rm $(type -P EfiRom). Let me double check.
> 
> Eh you are right...
> 
> $ make -C roms EFIROM=/bin/true clean
> [...]
> make -C ipxe/src veryclean
> make[1]: Entering directory '/home/phil/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 /bin/true ./util/efifatbin
> ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
> rm: cannot remove '/bin/true': Permission denied
> make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
> 
> This seems to be a pre-existent IPXE bug:
> 
> $ git checkout c9d18c1c150c84e7a976df989ad04ddf01083f46
> $ make -C roms EFIROM=/bin/true clean
> [...]
> rm: cannot remove '/bin/true': Permission denied
> make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
> 
> >> How can I trigger a SUSE package build with this patch?
> > 
> > Not so easy without a VM.
> 
> Oh, unfortunate :(
> 
> > 
> > Olaf
> > 
> 

Let's keep it simple: set a config variable, then
check it here.

-- 
MST

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

Posted by Olaf Hering 6 years, 7 months ago

Am Fri, 5 Apr 2019 19:17:35 -0400
schrieb "Michael S. Tsirkin" <mst@redhat.com>:

> Let's keep it simple: set a config variable, then check it here.

Better revert f590a812c21074e82228de3e1dfb57b75fc02b62 and 23858f4092fc9ebf9e7a5e5110e44abef6fc6643 for the time being. It looks like a no-op right now. How did people get away without the system-wide EfiRom until now? It is certainly only a very small inconvenience to build a private EfiRom for those who can not possibly receive it from the vendor.

Also roms/ seems to be unable to receive configure options.

Olaf

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

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

On 4/8/19 8:21 AM, Olaf Hering wrote:
> Am Fri, 5 Apr 2019 19:17:35 -0400
> schrieb "Michael S. Tsirkin" <mst@redhat.com>:
> 
>> Let's keep it simple: set a config variable, then check it here.
> 
> Better revert f590a812c21074e82228de3e1dfb57b75fc02b62 and 23858f4092fc9ebf9e7a5e5110e44abef6fc6643 for the time being. It looks like a no-op right now. How did people get away without the system-wide EfiRom until now? It is certainly only a very small inconvenience to build a private EfiRom for those who can not possibly receive it from the vendor.

But f590a812c210 and 23858f4092fc are used for ACPI UEFI testing, what
about the v2 which looks "simple" to me:
https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01033.html

> Also roms/ seems to be unable to receive configure options.
> 
> Olaf
>