diff mbox

[3/7] ipxe: add local patches

Message ID 1428675432-31433-4-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann April 10, 2015, 2:17 p.m. UTC 
There are two ipxe patches needed to make efi pxe boots work.
They didn't made it upstream yet, and I don't want to wait any
longer with updating qemu.  So add them here, with some logic
to apply them before building ipxe.

/me still hopes I can revert that patch some day.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 roms/Makefile                                      |  12 +-
 ...rove-compliance-with-the-EFI_SIMPLE_NETWO.patch | 160 +++++++++++++++++++++
 ...0002-efi-make-load-file-protocol-optional.patch | 102 +++++++++++++
 3 files changed, 271 insertions(+), 3 deletions(-)
 create mode 100644 roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch
 create mode 100644 roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch

Comments

Andreas Färber April 11, 2015, 3:10 a.m. UTC | #1 
Am 10.04.2015 um 16:17 schrieb Gerd Hoffmann:
> There are two ipxe patches needed to make efi pxe boots work.
> They didn't made it upstream yet, and I don't want to wait any
> longer with updating qemu.  So add them here, with some logic
> to apply them before building ipxe.
> 
> /me still hopes I can revert that patch some day.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  roms/Makefile                                      |  12 +-
>  ...rove-compliance-with-the-EFI_SIMPLE_NETWO.patch | 160 +++++++++++++++++++++
>  ...0002-efi-make-load-file-protocol-optional.patch | 102 +++++++++++++
>  3 files changed, 271 insertions(+), 3 deletions(-)
>  create mode 100644 roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch
>  create mode 100644 roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch
> 
> diff --git a/roms/Makefile b/roms/Makefile
> index 461cb49..ab4532c 100644
> --- a/roms/Makefile
> +++ b/roms/Makefile
> @@ -115,12 +115,12 @@ efi-rom-%: build-pxe-roms build-efi-roms
>  		-ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \
>  		-o ../pc-bios/efi-$*.rom
>  
> -build-pxe-roms: ipxe/src/config/local/general.h
> +build-pxe-roms: ipxe/qemu-patches ipxe/src/config/local/general.h
>  	$(MAKE) -C ipxe/src GITVERSION="" \
>  		CROSS_COMPILE=$(x86_64_cross_prefix) \
>  		$(patsubst %,bin/%.rom,$(pxerom_targets))
>  
> -build-efi-roms: build-pxe-roms ipxe/src/config/local/general.h
> +build-efi-roms: ipxe/qemu-patches build-pxe-roms ipxe/src/config/local/general.h
>  	$(MAKE) -C ipxe/src GITVERSION="" \
>  		CROSS_COMPILE=$(x86_64_cross_prefix) \
>  		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
> @@ -129,6 +129,12 @@ build-efi-roms: build-pxe-roms ipxe/src/config/local/general.h
>  ipxe/src/config/local/%: config.ipxe.%
>  	cp $< $@
>  
> +ipxe/qemu-patches:

Looks like this is only ever removed by clean? Should depend on the
patch files, in case they change with an update.

But why are you adding patch files in the first place? Can't we just
push the commits to a branch on git.qemu-project.org and update the
submodule config accordingly?

Regards,
Andreas

> +	for patch in ipxe-patches/*; do \
> +		echo "# applying $$patch"; \
> +		cat $$patch | (cd ipxe; patch -p1); \
> +	done
> +	touch $@
>  
>  slof:
>  	$(MAKE) -C SLOF CROSS=$(powerpc64_cross_prefix) qemu
> @@ -148,6 +154,6 @@ clean:
>  	$(MAKE) -C sgabios clean
>  	rm -f sgabios/.depend
>  	$(MAKE) -C ipxe/src veryclean
> -	(cd ipxe; rm -f src/config/local/*.h)
> +	(cd ipxe; git reset --hard; rm -f qemu-patches src/config/local/*.h)
>  	$(MAKE) -C SLOF clean
>  	rm -rf u-boot/build.e500
[snip]
Gerd Hoffmann April 14, 2015, 7:36 a.m. UTC | #2 
Hi,

> >  
> > +ipxe/qemu-patches:
> 
> Looks like this is only ever removed by clean? Should depend on the
> patch files, in case they change with an update.

Indeed.

> But why are you adding patch files in the first place? Can't we just
> push the commits to a branch on git.qemu-project.org and update the
> submodule config accordingly?

I hope this is just temporary, so I didn't want go through the
administrative to setup a qemu ipxe branch @ git.qemu-project.org.

Just resent the patches to the ipxe list.

Assuming that doesn't work out:  How we are going to maintain ipxe.git @
qemu-project.org?  Should I just send patches to the list?  Can I get
commit access for ipxe.git?  The later would certainly simplify things,
especially when it comes to rebasing the qemu patches to pickup upstream
fixes ...

cheers,
  Gerd
Andreas Färber April 14, 2015, 12:49 p.m. UTC | #3 
Hi Gerd,

+ Stefan, - ipxe-devel

Am 14.04.2015 um 09:36 schrieb Gerd Hoffmann:
>> But why are you adding patch files in the first place? Can't we just
>> push the commits to a branch on git.qemu-project.org and update the
>> submodule config accordingly?
> 
> I hope this is just temporary, so I didn't want go through the
> administrative to setup a qemu ipxe branch @ git.qemu-project.org.
> 
> Just resent the patches to the ipxe list.

Can't judge how quickly things move there, obviously...

> Assuming that doesn't work out:  How we are going to maintain ipxe.git @
> qemu-project.org?  Should I just send patches to the list?  Can I get
> commit access for ipxe.git?  The later would certainly simplify things,
> especially when it comes to rebasing the qemu patches to pickup upstream
> fixes ...

Either push access might be possible, or maybe our mirroring could be
changed/extended to fetch from your git server?

Cheers,
Andreas
diff mbox

Patch

diff --git a/roms/Makefile b/roms/Makefile
index 461cb49..ab4532c 100644
--- a/roms/Makefile
+++ b/roms/Makefile
@@ -115,12 +115,12 @@  efi-rom-%: build-pxe-roms build-efi-roms
 		-ec ipxe/src/bin-x86_64-efi/$(VID)$(DID).efidrv \
 		-o ../pc-bios/efi-$*.rom
 
-build-pxe-roms: ipxe/src/config/local/general.h
+build-pxe-roms: ipxe/qemu-patches ipxe/src/config/local/general.h
 	$(MAKE) -C ipxe/src GITVERSION="" \
 		CROSS_COMPILE=$(x86_64_cross_prefix) \
 		$(patsubst %,bin/%.rom,$(pxerom_targets))
 
-build-efi-roms: build-pxe-roms ipxe/src/config/local/general.h
+build-efi-roms: ipxe/qemu-patches build-pxe-roms ipxe/src/config/local/general.h
 	$(MAKE) -C ipxe/src GITVERSION="" \
 		CROSS_COMPILE=$(x86_64_cross_prefix) \
 		$(patsubst %,bin-i386-efi/%.efidrv,$(pxerom_targets)) \
@@ -129,6 +129,12 @@  build-efi-roms: build-pxe-roms ipxe/src/config/local/general.h
 ipxe/src/config/local/%: config.ipxe.%
 	cp $< $@
 
+ipxe/qemu-patches:
+	for patch in ipxe-patches/*; do \
+		echo "# applying $$patch"; \
+		cat $$patch | (cd ipxe; patch -p1); \
+	done
+	touch $@
 
 slof:
 	$(MAKE) -C SLOF CROSS=$(powerpc64_cross_prefix) qemu
@@ -148,6 +154,6 @@  clean:
 	$(MAKE) -C sgabios clean
 	rm -f sgabios/.depend
 	$(MAKE) -C ipxe/src veryclean
-	(cd ipxe; rm -f src/config/local/*.h)
+	(cd ipxe; git reset --hard; rm -f qemu-patches src/config/local/*.h)
 	$(MAKE) -C SLOF clean
 	rm -rf u-boot/build.e500
diff --git a/roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch b/roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch
new file mode 100644
index 0000000..7f6febf
--- /dev/null
+++ b/roms/ipxe-patches/0001-efi_snp-improve-compliance-with-the-EFI_SIMPLE_NETWO.patch
@@ -0,0 +1,160 @@ 
+From 9e870d92035ec7ca946e702236bfe104e964f8c6 Mon Sep 17 00:00:00 2001
+From: Laszlo Ersek <lersek@redhat.com>
+Date: Thu, 22 Jan 2015 22:05:35 +0100
+Subject: [PATCH 1/2] efi_snp: improve compliance with the
+ EFI_SIMPLE_NETWORK_PROTOCOL spec
+
+The efi_snp interface dates back to 2008, when the GetStatus() interface
+must have been seriously under-specified. The UEFI Specification (2.4)
+specifies EFI_SIMPLE_NETWORK_PROTOCOL in detail however. In short:
+
+- the Transmit() interface is assumed to link (not copy) the SNP client's
+  buffer and return at once (without blocking), taking ownership of the
+  buffer temporarily;
+
+- the GetStatus() interface releases one of the completed (transmitted or
+  internally copied) buffers back to the caller. If there are several
+  completed buffers, it is unspecified which one is returned.
+
+The EFI build of the grub boot loader actually verifies the buffer address
+returned by GetStatus(), therefore in efi_snp we must at least fake the
+queueing of client buffers. This patch doesn't track client buffers
+together with the internally queued io_buffer structures, we consider a
+client buffer recyclable as soon as we make a deep copy of it and queue
+the copy internally.
+
+Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
+---
+ src/include/ipxe/efi/efi_snp.h |  6 +++++
+ src/interface/efi/efi_snp.c    | 54 ++++++++++++++++++++++++------------------
+ 2 files changed, 37 insertions(+), 23 deletions(-)
+
+diff --git a/src/include/ipxe/efi/efi_snp.h b/src/include/ipxe/efi/efi_snp.h
+index a18bced..863a81a 100644
+--- a/src/include/ipxe/efi/efi_snp.h
++++ b/src/include/ipxe/efi/efi_snp.h
+@@ -18,6 +18,8 @@
+ #include <ipxe/efi/Protocol/HiiDatabase.h>
+ #include <ipxe/efi/Protocol/LoadFile.h>
+ 
++#define MAX_RECYCLED_TXBUFS 64
++
+ /** An SNP device */
+ struct efi_snp_device {
+ 	/** List of SNP devices */
+@@ -44,6 +46,10 @@ struct efi_snp_device {
+ 	 * Used in order to generate TX completions.
+ 	 */
+ 	unsigned int tx_count_txbufs;
++	/** Holds the addresses of recycled SNP client buffers; a ring. */
++	void *tx_recycled_txbufs[MAX_RECYCLED_TXBUFS];
++	/** The index of the first buffer to return to the SNP client. */
++	unsigned tx_first_txbuf;
+ 	/** Outstanding RX packet count (via "interrupt status") */
+ 	unsigned int rx_count_interrupts;
+ 	/** Outstanding RX packet count (via WaitForPacket event) */
+diff --git a/src/interface/efi/efi_snp.c b/src/interface/efi/efi_snp.c
+index 67fba34..c21af33 100644
+--- a/src/interface/efi/efi_snp.c
++++ b/src/interface/efi/efi_snp.c
+@@ -68,6 +68,14 @@ static void efi_snp_set_state ( struct efi_snp_device *snpdev ) {
+ 		 */
+ 		mode->State = EfiSimpleNetworkInitialized;
+ 	}
++
++	if (mode->State != EfiSimpleNetworkInitialized) {
++		/* Zero the number of recycled buffers when moving to any other
++		 * state than Initialized. Transmit() and GetStatus() are only
++		 * valid in Initialized.
++		 */
++		snpdev->tx_count_txbufs = 0;
++	}
+ }
+ 
+ /**
+@@ -446,12 +454,12 @@ efi_snp_nvdata ( EFI_SIMPLE_NETWORK_PROTOCOL *snp, BOOLEAN read,
+  *
+  * @v snp		SNP interface
+  * @v interrupts	Interrupt status, or NULL
+- * @v txbufs		Recycled transmit buffer address, or NULL
++ * @v txbuf		Recycled transmit buffer address, or NULL
+  * @ret efirc		EFI status code
+  */
+ static EFI_STATUS EFIAPI
+ efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
+-		     UINT32 *interrupts, VOID **txbufs ) {
++		     UINT32 *interrupts, VOID **txbuf ) {
+ 	struct efi_snp_device *snpdev =
+ 		container_of ( snp, struct efi_snp_device, snp );
+ 
+@@ -485,30 +493,22 @@ efi_snp_get_status ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
+ 		DBGC2 ( snpdev, " INTS:%02x", *interrupts );
+ 	}
+ 
+-	/* TX completions.  It would be possible to design a more
+-	 * idiotic scheme for this, but it would be a challenge.
+-	 * According to the UEFI header file, txbufs will be filled in
+-	 * with a list of "recycled transmit buffers" (i.e. completed
+-	 * TX buffers).  Observant readers may care to note that
+-	 * *txbufs is a void pointer.  Precisely how a list of
+-	 * completed transmit buffers is meant to be represented as an
+-	 * array of voids is left as an exercise for the reader.
+-	 *
+-	 * The only users of this interface (MnpDxe/MnpIo.c and
+-	 * PxeBcDxe/Bc.c within the EFI dev kit) both just poll until
+-	 * seeing a non-NULL result return in txbufs.  This is valid
+-	 * provided that they do not ever attempt to transmit more
+-	 * than one packet concurrently (and that TX never times out).
++	/* In efi_snp_transmit() we enqueue packets by copying them (not by
++	 * linking them), hence we can recycle them immediately to the SNP
++	 * client.
+ 	 */
+-	if ( txbufs ) {
+-		if ( snpdev->tx_count_txbufs &&
+-		     list_empty ( &snpdev->netdev->tx_queue ) ) {
+-			*txbufs = "Which idiot designed this API?";
++	if ( txbuf ) {
++		if ( snpdev->tx_count_txbufs ) {
++			unsigned first;
++
++			first = snpdev->tx_first_txbuf++;
++			snpdev->tx_first_txbuf %= MAX_RECYCLED_TXBUFS;
++			*txbuf = snpdev->tx_recycled_txbufs[first];
+ 			snpdev->tx_count_txbufs--;
+ 		} else {
+-			*txbufs = NULL;
++			*txbuf = NULL;
+ 		}
+-		DBGC2 ( snpdev, " TX:%s", ( *txbufs ? "some" : "none" ) );
++		DBGC2 ( snpdev, " TX:%p", *txbuf );
+ 	}
+ 
+ 	DBGC2 ( snpdev, "\n" );
+@@ -560,6 +560,12 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
+ 	if ( efi_snp_claimed )
+ 		return EFI_NOT_READY;
+ 
++	assert ( snpdev->tx_count_txbufs <= MAX_RECYCLED_TXBUFS );
++	if ( snpdev->tx_count_txbufs == MAX_RECYCLED_TXBUFS ) {
++		/* No room to recycle another buffer. */
++		return EFI_NOT_READY;
++	}
++
+ 	/* Sanity checks */
+ 	if ( ll_header_len ) {
+ 		if ( ll_header_len != ll_protocol->ll_header_len ) {
+@@ -626,7 +632,9 @@ efi_snp_transmit ( EFI_SIMPLE_NETWORK_PROTOCOL *snp,
+ 
+ 	/* Record transmission as outstanding */
+ 	snpdev->tx_count_interrupts++;
+-	snpdev->tx_count_txbufs++;
++	snpdev->tx_recycled_txbufs[(snpdev->tx_first_txbuf +
++				    snpdev->tx_count_txbufs++
++				   ) % MAX_RECYCLED_TXBUFS] = data;
+ 
+ 	return 0;
+ 
+-- 
+1.8.3.1
+
diff --git a/roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch b/roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch
new file mode 100644
index 0000000..f921a3b
--- /dev/null
+++ b/roms/ipxe-patches/0002-efi-make-load-file-protocol-optional.patch
@@ -0,0 +1,102 @@ 
+From 2daea2b8dd2c504a4f76a6b0b67bd3c4a2957fc7 Mon Sep 17 00:00:00 2001
+From: Gerd Hoffmann <kraxel@redhat.com>
+Date: Tue, 10 Feb 2015 14:28:09 +0100
+Subject: [PATCH 2/2] [efi] make load file protocol optional
+
+The load file implementation added by commit
+c7c3d839fc9120aee28de9aabe452dc85ad91502 doesn't support loading
+arbitrary files from the tftp server, so efi applications trying
+to do exactly that fail to boot:
+
+  iPXE 1.0.0+ (17ace) -- Open Source Network Boot Firmware -- http://ipxe.org
+  Features: HTTP DNS TFTP EFI Menu
+
+  net0: 52:54:00:47:d3:07 using virtio-net on PCI00:09.0 (open)
+    [Link:up, TX:0 TXE:0 RX:13 RXE:2]
+    [RXE: 2 x "Operation not supported (http://ipxe.org/3c086083)"]
+  Configuring (net0 52:54:00:47:d3:07)...... ok
+  net0: 192.168.132.93/255.255.255.0 gw 192.168.132.1
+  Next server: 192.168.132.1
+  Filename: shim.efi
+  tftp://192.168.132.1/shim.efi... ok
+  Failed to open grubx64.efi - Not Found
+  Failed to load image grubx64.efi: Not Found
+  Failed to open MokManager.efi - Not Found
+  Failed to load image MokManager.efi: Not Found
+  Could not boot image: Error 0x7f04828e (http://ipxe.org/7f04828e)
+
+  Boot Failed. EFI Network
+
+This is not acceptable for qemu.  efi pxe configurations which work
+just fine with real hardware must work with qemu virtual machines too.
+
+This patch adds a config option for the load file protocol
+implementation, to allow it being disabled, so we can turn it off
+for the pxe roms shipped with qemu.
+
+The default for the new option maintains current behavior, i.e.
+load file is enabled unless you override it in config/local/general.h
+
+Suggested-by: Laszlo Ersek <lersek@redhat.com>
+
+See discussion here:
+  http://lists.ipxe.org/pipermail/ipxe-devel/2015-February/003979.html
+
+Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
+---
+ src/config/general.h        | 6 ++++++
+ src/interface/efi/efi_snp.c | 5 +++++
+ 2 files changed, 11 insertions(+)
+
+diff --git a/src/config/general.h b/src/config/general.h
+index 65c1f85..8c91601 100644
+--- a/src/config/general.h
++++ b/src/config/general.h
+@@ -142,6 +142,12 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
+ #undef	NONPNP_HOOK_INT19	/* Hook INT19 on non-PnP BIOSes */
+ 
+ /*
++ * EFI specific options
++ *
++ */
++#define EFI_PROTO_LOAD_FILE	/* register LOAD_FILE protocol */
++
++/*
+  * Error message tables to include
+  *
+  */
+diff --git a/src/interface/efi/efi_snp.c b/src/interface/efi/efi_snp.c
+index c21af33..85f4fa0 100644
+--- a/src/interface/efi/efi_snp.c
++++ b/src/interface/efi/efi_snp.c
+@@ -34,6 +34,7 @@ FILE_LICENCE ( GPL2_OR_LATER );
+ #include <ipxe/efi/efi_utils.h>
+ #include <ipxe/efi/efi_snp.h>
+ #include <usr/autoboot.h>
++#include <config/general.h>
+ 
+ /** List of SNP devices */
+ static LIST_HEAD ( efi_snp_devices );
+@@ -1033,7 +1034,9 @@ static int efi_snp_probe ( struct net_device *netdev ) {
+ 			&efi_nii_protocol_guid, &snpdev->nii,
+ 			&efi_nii31_protocol_guid, &snpdev->nii,
+ 			&efi_component_name2_protocol_guid, &snpdev->name2,
++#ifdef EFI_PROTO_LOAD_FILE
+ 			&efi_load_file_protocol_guid, &snpdev->load_file,
++#endif
+ 			NULL ) ) != 0 ) {
+ 		rc = -EEFI ( efirc );
+ 		DBGC ( snpdev, "SNPDEV %p could not install protocols: "
+@@ -1082,7 +1085,9 @@ static int efi_snp_probe ( struct net_device *netdev ) {
+ 			&efi_nii_protocol_guid, &snpdev->nii,
+ 			&efi_nii31_protocol_guid, &snpdev->nii,
+ 			&efi_component_name2_protocol_guid, &snpdev->name2,
++#ifdef EFI_PROTO_LOAD_FILE
+ 			&efi_load_file_protocol_guid, &snpdev->load_file,
++#endif
+ 			NULL );
+  err_install_protocol_interface:
+ 	free ( snpdev->path );
+-- 
+1.8.3.1
+