[PATCH for-next 0/5] Load ipxe from a standalone file

[PATCH for-next 0/5] Load ipxe from a standalone file

[Wei Liu]

Seeing yet another bug report regarding ipxe today, I would rather have this
done than having to continuously cherry-pick ipxe commits.

I have written these patches from scratch to my liking.

Only very light testing is done. I will do a bit more testing when I have time.

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Anoob Soman <anoob.soman@citrix.com>
Cc: Doug Goldstein <cardoe@cardoe.com>

Wei Liu (5):
  Tools.mk.in: drop unused variables
  ipxe: produce a single binary from its build
  libxc: allow HVM guest to have modules
  tools: load IPXE from standalone file
  tools: provide --with-system-ipxe

 config/Tools.mk.in                   |  3 +-
 tools/config.h.in                    |  3 ++
 tools/configure                      | 53 ++++++++++++++++++++++++++++++++++++
 tools/configure.ac                   | 18 ++++++++++++
 tools/firmware/Makefile              |  6 ++++
 tools/firmware/etherboot/Makefile    |  6 +++-
 tools/firmware/hvmloader/Makefile    |  9 +-----
 tools/firmware/hvmloader/hvmloader.c |  8 +++++-
 tools/firmware/hvmloader/rombios.c   | 23 +++++++++++-----
 tools/libxc/xc_dom_x86.c             | 32 ++++++++++++----------
 tools/libxl/libxl_dom.c              | 10 +++++++
 tools/libxl/libxl_internal.h         |  1 +
 tools/libxl/libxl_paths.c            |  9 ++++++
 13 files changed, 148 insertions(+), 33 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH for-next 1/5] Tools.mk.in: drop unused variables

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 config/Tools.mk.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 2d6c440324..4cc9f29090 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -20,8 +20,6 @@ BCC                 := @BCC@
 IASL                := @IASL@
 AWK                 := @AWK@
 FETCHER             := @FETCHER@
-SEABIOS_PATH        := @seabios_path@
-OVMF_PATH           := @ovmf_path@
 
 # Extra folder for libs/includes
 PREPEND_INCLUDES    := @PREPEND_INCLUDES@
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH for-next 2/5] ipxe: produce a single binary from its build

[Wei Liu]

And switch hvmloader/Makefile to use that binary. This will help later
when we change hvmloader to pick a user provided binary.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/firmware/etherboot/Makefile | 6 +++++-
 tools/firmware/hvmloader/Makefile | 8 ++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/firmware/etherboot/Makefile b/tools/firmware/etherboot/Makefile
index e33458d2fe..37896ed4fc 100644
--- a/tools/firmware/etherboot/Makefile
+++ b/tools/firmware/etherboot/Makefile
@@ -18,11 +18,15 @@ D=ipxe
 T=ipxe.tar.gz
 
 ROMS = $(addprefix $D/src/bin/, $(addsuffix .rom, $(ETHERBOOT_NICS)))
+ROM = $D/src/bin/ipxe.bin
 
 .NOTPARALLEL:
 
 .PHONY: all
-all: $(ROMS)
+all: $(ROM)
+
+$(ROM): $(ROMS)
+	cat $^ > $@
 
 %.rom: $D/src/arch/i386/Makefile
 	$(MAKE) -C $D/src bin/$(*F).rom
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index a5b4c32c1a..16255ebddd 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -51,7 +51,7 @@ CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
 else
 CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.bin
 endif
-ETHERBOOT_ROMS := $(addprefix ../etherboot/ipxe/src/bin/, $(addsuffix .rom, $(ETHERBOOT_NICS)))
+ETHERBOOT_ROM := ../etherboot/ipxe/src/bin/ipxe.bin
 endif
 
 ROMS := 
@@ -60,7 +60,7 @@ ifeq ($(CONFIG_ROMBIOS),y)
 OBJS += optionroms.o 32bitbios_support.o rombios.o
 CFLAGS += -DENABLE_ROMBIOS
 ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
-ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROMS)
+ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROM)
 endif
 
 .PHONY: all
@@ -105,9 +105,9 @@ ifneq ($(CIRRUSVGA_ROM),)
 	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
-ifneq ($(ETHERBOOT_ROMS),)
+ifneq ($(ETHERBOOT_ROM),)
 	echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-	sh ../../misc/mkhex etherboot $(ETHERBOOT_ROMS) >> $@.new
+	sh ../../misc/mkhex etherboot $(ETHERBOOT_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 2/5] ipxe: produce a single binary from its build

[Jan Beulich]

>>> On 15.05.18 at 20:22, <wei.liu2@citrix.com> wrote:
> --- a/tools/firmware/etherboot/Makefile
> +++ b/tools/firmware/etherboot/Makefile
> @@ -18,11 +18,15 @@ D=ipxe
>  T=ipxe.tar.gz
>  
>  ROMS = $(addprefix $D/src/bin/, $(addsuffix .rom, $(ETHERBOOT_NICS)))
> +ROM = $D/src/bin/ipxe.bin
>  
>  .NOTPARALLEL:
>  
>  .PHONY: all
> -all: $(ROMS)
> +all: $(ROM)
> +
> +$(ROM): $(ROMS)
> +	cat $^ > $@

Simple commands like this always make me worry what happens if someone
interrupts the build at the "right" point in time: Does the target file get
removed in that case, so that a subsequent re-invocation of make will
correctly rebuild it? If not, you may need to go through an intermediate file,
which you then rename to $@.

hvmloader part
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 2/5] ipxe: produce a single binary from its build

[Wei Liu]

On Fri, May 18, 2018 at 09:38:01AM -0600, Jan Beulich wrote:
> >>> On 15.05.18 at 20:22, <wei.liu2@citrix.com> wrote:
> > --- a/tools/firmware/etherboot/Makefile
> > +++ b/tools/firmware/etherboot/Makefile
> > @@ -18,11 +18,15 @@ D=ipxe
> >  T=ipxe.tar.gz
> >  
> >  ROMS = $(addprefix $D/src/bin/, $(addsuffix .rom, $(ETHERBOOT_NICS)))
> > +ROM = $D/src/bin/ipxe.bin
> >  
> >  .NOTPARALLEL:
> >  
> >  .PHONY: all
> > -all: $(ROMS)
> > +all: $(ROM)
> > +
> > +$(ROM): $(ROMS)
> > +	cat $^ > $@
> 
> Simple commands like this always make me worry what happens if someone
> interrupts the build at the "right" point in time: Does the target file get
> removed in that case, so that a subsequent re-invocation of make will
> correctly rebuild it? If not, you may need to go through an intermediate file,
> which you then rename to $@.

Good point. I will use an intermediary file in my next version.

> 
> hvmloader part
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 

Thanks.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH for-next 3/5] libxc: allow HVM guest to have modules

[Wei Liu]

Lift the loading code out of PVH specific branch. Take the chance to
make the debug message more useful.

IPXE will be loaded as a module of Rombios.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxc/xc_dom_x86.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e33a28847d..ed4973a997 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1698,20 +1698,6 @@ static int bootlate_hvm(struct xc_dom_image *dom)
                                 ((uintptr_t)cmdline - (uintptr_t)start_info);
         }
 
-        for ( i = 0; i < dom->num_modules; i++ )
-        {
-            struct xc_hvm_firmware_module mod;
-
-            DOMPRINTF("Adding module %u", i);
-            mod.guest_addr_out =
-                dom->modules[i].seg.vstart - dom->parms.virt_base;
-            mod.length =
-                dom->modules[i].seg.vend - dom->modules[i].seg.vstart;
-
-            add_module_to_list(dom, &mod, dom->modules[i].cmdline,
-                               modlist, start_info);
-        }
-
         /* ACPI module 0 is the RSDP */
         start_info->rsdp_paddr = dom->acpi_modules[0].guest_addr_out ? : 0;
     }
@@ -1721,6 +1707,24 @@ static int bootlate_hvm(struct xc_dom_image *dom)
                            modlist, start_info);
     }
 
+    for ( i = 0; i < dom->num_modules; i++ )
+    {
+        struct xc_hvm_firmware_module mod;
+        uint64_t base = dom->parms.virt_base != UNSET_ADDR ?
+            dom->parms.virt_base : 0;
+
+        mod.guest_addr_out =
+            dom->modules[i].seg.vstart - base;
+        mod.length =
+            dom->modules[i].seg.vend - dom->modules[i].seg.vstart;
+
+        DOMPRINTF("Adding module %u guest_addr %"PRIx64" len %u",
+                  i, mod.guest_addr_out, mod.length);
+
+        add_module_to_list(dom, &mod, dom->modules[i].cmdline,
+                           modlist, start_info);
+    }
+
     if ( start_info->nr_modules )
     {
         start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH for-next 4/5] tools: load IPXE from standalone file

[Wei Liu]

Do not embed IPXE into Rombios anymore. Instead, it is loaded by the
toolstack from a file as a separate module.

Ability to let user specify an IPXE blob will come later.

No user visible change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/firmware/Makefile              |  6 ++++++
 tools/firmware/hvmloader/Makefile    |  9 +--------
 tools/firmware/hvmloader/hvmloader.c |  8 +++++++-
 tools/firmware/hvmloader/rombios.c   | 23 ++++++++++++++++-------
 tools/libxl/libxl_dom.c              | 10 ++++++++++
 tools/libxl/libxl_internal.h         |  1 +
 tools/libxl/libxl_paths.c            |  5 +++++
 7 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 5a7cf7766d..0bef579637 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -55,6 +55,9 @@ endif
 ifeq ($(CONFIG_OVMF),y)
 	$(INSTALL_DATA) ovmf-dir/ovmf.bin $(INST_DIR)/ovmf.bin
 endif
+ifeq ($(CONFIG_IPXE),y)
+	$(INSTALL_DATA) etherboot/ipxe/src/bin/ipxe.bin $(INST_DIR)/ipxe.bin
+endif
 ifeq ($(CONFIG_PV_SHIM),y)
 	$(INSTALL_DATA) xen-dir/xen-shim $(INST_DIR)/xen-shim
 	$(INSTALL_DATA) xen-dir/xen-shim-syms $(DEBG_DIR)/xen-shim-syms
@@ -69,6 +72,9 @@ endif
 ifeq ($(CONFIG_OVMF),y)
 	rm -f $(INST_DIR)/ovmf.bin
 endif
+ifeq ($(CONFIG_IPXE),y)
+	rm -r $(INST_DIR)/ipxe.bin
+endif
 ifeq ($(CONFIG_PV_SHIM),y)
 	rm -f $(INST_DIR)/xen-shim
 	rm -f $(DEBG_DIR)/xen-shim-syms
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 16255ebddd..496ac72b77 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -51,7 +51,6 @@ CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
 else
 CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.bin
 endif
-ETHERBOOT_ROM := ../etherboot/ipxe/src/bin/ipxe.bin
 endif
 
 ROMS := 
@@ -60,7 +59,7 @@ ifeq ($(CONFIG_ROMBIOS),y)
 OBJS += optionroms.o 32bitbios_support.o rombios.o
 CFLAGS += -DENABLE_ROMBIOS
 ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
-ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROM)
+ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM)
 endif
 
 .PHONY: all
@@ -105,12 +104,6 @@ ifneq ($(CIRRUSVGA_ROM),)
 	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
-ifneq ($(ETHERBOOT_ROM),)
-	echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-	sh ../../misc/mkhex etherboot $(ETHERBOOT_ROM) >> $@.new
-	echo "#endif" >> $@.new
-endif
-
 	mv $@.new $@
 
 .PHONY: clean
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index f603f68ded..f546cfb3ab 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -368,7 +368,13 @@ int main(void)
 #ifdef ENABLE_ROMBIOS
     else if ( bios == &rombios_config )
     {
-        bios->bios_load(bios, NULL, 0);
+        const struct hvm_modlist_entry *ipxe;
+        uint32_t paddr = 0;
+
+        ipxe = get_module_entry(hvm_start_info, "ipxe");
+        if ( ipxe )
+            paddr = ipxe->paddr;
+        bios->bios_load(bios, (void*)paddr, 0 /* unused */);
     }
 #endif
     else
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index c736fd9dea..8c44839ce3 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -63,6 +63,8 @@ static void rombios_setup_bios_info(void)
     memset(info, 0, sizeof(*info));
 }
 
+static void *ipxe_module_addr;
+
 static void rombios_load_roms(void)
 {
     int option_rom_sz = 0, vgabios_sz = 0, etherboot_sz = 0;
@@ -95,13 +97,17 @@ static void rombios_load_roms(void)
     etherboot_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
     if ( etherboot_phys_addr < OPTIONROM_PHYSICAL_ADDRESS )
         etherboot_phys_addr = OPTIONROM_PHYSICAL_ADDRESS;
-    etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
-                                      etherboot_phys_addr,
-                                      etherboot);
 
-    option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
-    option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
-                                         option_rom_phys_addr);
+    if ( ipxe_module_addr )
+    {
+        etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
+                                          etherboot_phys_addr,
+                                          ipxe_module_addr);
+
+        option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
+        option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
+                                             option_rom_phys_addr);
+    }
 
     printf("Option ROMs:\n");
     if ( vgabios_sz )
@@ -119,7 +125,7 @@ static void rombios_load_roms(void)
 }
 
 static void rombios_load(const struct bios_config *config,
-                         void *unused_addr, uint32_t unused_size)
+                         void *ipxe_addr, uint32_t unused_size)
 {
     uint32_t bioshigh;
     struct rombios_info *info;
@@ -133,6 +139,9 @@ static void rombios_load(const struct bios_config *config,
 
     info = (struct rombios_info *)BIOS_INFO_PHYSICAL_ADDRESS;
     info->bios32_entry = bioshigh;
+
+    /* Stash ipxe address */
+    ipxe_module_addr = ipxe_addr;
 }
 
 /*
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f0fd5fd3a3..1b9e45c492 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1128,6 +1128,16 @@ static int libxl__domain_firmware(libxl__gc *gc,
         if (rc) goto out;
     }
 
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
+        info->u.hvm.bios == LIBXL_BIOS_TYPE_ROMBIOS &&
+        libxl__ipxe_path())
+        rc = xc_dom_module_file(dom, libxl__ipxe_path(), "ipxe");
+
+    if (rc) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
         info->u.hvm.smbios_firmware) {
         data = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c582894589..518b75592c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2362,6 +2362,7 @@ _hidden const char *libxl__lock_dir_path(void);
 _hidden const char *libxl__run_dir_path(void);
 _hidden const char *libxl__seabios_path(void);
 _hidden const char *libxl__ovmf_path(void);
+_hidden const char *libxl__ipxe_path(void);
 
 /*----- subprocess execution with timeout -----*/
 
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index 0643c1b3a4..8498f82781 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -53,6 +53,11 @@ const char *libxl__ovmf_path(void)
 #endif
 }
 
+const char *libxl__ipxe_path(void)
+{
+    return XENFIRMWAREDIR "/ipxe.bin";
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 4/5] tools: load IPXE from standalone file

[Jan Beulich]

>>> On 15.05.18 at 20:22, <wei.liu2@citrix.com> wrote:
> @@ -95,13 +97,17 @@ static void rombios_load_roms(void)
>      etherboot_phys_addr = VGABIOS_PHYSICAL_ADDRESS + vgabios_sz;
>      if ( etherboot_phys_addr < OPTIONROM_PHYSICAL_ADDRESS )
>          etherboot_phys_addr = OPTIONROM_PHYSICAL_ADDRESS;
> -    etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
> -                                      etherboot_phys_addr,
> -                                      etherboot);
>  
> -    option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
> -    option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
> -                                         option_rom_phys_addr);
> +    if ( ipxe_module_addr )
> +    {
> +        etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
> +                                          etherboot_phys_addr,
> +                                          ipxe_module_addr);
> +
> +        option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
> +        option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
> +                                             option_rom_phys_addr);
> +    }

I guess you want to alter existing logic as little as possible, hence you
don't want to leverage the fact that you (could) get the size passed ...

> @@ -133,6 +139,9 @@ static void rombios_load(const struct bios_config *config,
>  
>      info = (struct rombios_info *)BIOS_INFO_PHYSICAL_ADDRESS;
>      info->bios32_entry = bioshigh;
> +
> +    /* Stash ipxe address */
> +    ipxe_module_addr = ipxe_addr;
>  }

... here (but you ignore it). If the size was used, scan_option_rom() could be
made not even look at non-option-ROMs.

But on the basis that you simply retain existing functionality
Acked-by: Jan Beulich <jbeulich@suse.com>
for the hvmloader parts.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 4/5] tools: load IPXE from standalone file

[Anthony PERARD]

On Tue, May 15, 2018 at 07:22:42PM +0100, Wei Liu wrote:
> diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> index f603f68ded..f546cfb3ab 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -368,7 +368,13 @@ int main(void)
>  #ifdef ENABLE_ROMBIOS
>      else if ( bios == &rombios_config )
>      {
> -        bios->bios_load(bios, NULL, 0);
> +        const struct hvm_modlist_entry *ipxe;
> +        uint32_t paddr = 0;
> +
> +        ipxe = get_module_entry(hvm_start_info, "ipxe");
> +        if ( ipxe )
> +            paddr = ipxe->paddr;
> +        bios->bios_load(bios, (void*)paddr, 0 /* unused */);

I don't think it's a good idea to change the meaning of the arguments of
bios_load(). For every other use of bios_load, the second argument is
used at the address of the firmware to load. Here "ipxe" is an extra
blob to load.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 4/5] tools: load IPXE from standalone file

[Wei Liu]

On Mon, Jun 04, 2018 at 02:47:57PM +0100, Anthony PERARD wrote:
> On Tue, May 15, 2018 at 07:22:42PM +0100, Wei Liu wrote:
> > diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> > index f603f68ded..f546cfb3ab 100644
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -368,7 +368,13 @@ int main(void)
> >  #ifdef ENABLE_ROMBIOS
> >      else if ( bios == &rombios_config )
> >      {
> > -        bios->bios_load(bios, NULL, 0);
> > +        const struct hvm_modlist_entry *ipxe;
> > +        uint32_t paddr = 0;
> > +
> > +        ipxe = get_module_entry(hvm_start_info, "ipxe");
> > +        if ( ipxe )
> > +            paddr = ipxe->paddr;
> > +        bios->bios_load(bios, (void*)paddr, 0 /* unused */);
> 
> I don't think it's a good idea to change the meaning of the arguments of
> bios_load(). For every other use of bios_load, the second argument is
> used at the address of the firmware to load. Here "ipxe" is an extra
> blob to load.

We either add more arguments to bios_load or repurpose the existing
one(s). I'm not too fussed with either option really.

Jan and Andrew, your opinion?

Wei.

> 
> -- 
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 4/5] tools: load IPXE from standalone file

[Jan Beulich]

>>> On 25.06.18 at 10:16, <wei.liu2@citrix.com> wrote:
> On Mon, Jun 04, 2018 at 02:47:57PM +0100, Anthony PERARD wrote:
>> On Tue, May 15, 2018 at 07:22:42PM +0100, Wei Liu wrote:
>> > diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
>> > index f603f68ded..f546cfb3ab 100644
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -368,7 +368,13 @@ int main(void)
>> >  #ifdef ENABLE_ROMBIOS
>> >      else if ( bios == &rombios_config )
>> >      {
>> > -        bios->bios_load(bios, NULL, 0);
>> > +        const struct hvm_modlist_entry *ipxe;
>> > +        uint32_t paddr = 0;
>> > +
>> > +        ipxe = get_module_entry(hvm_start_info, "ipxe");
>> > +        if ( ipxe )
>> > +            paddr = ipxe->paddr;
>> > +        bios->bios_load(bios, (void*)paddr, 0 /* unused */);
>> 
>> I don't think it's a good idea to change the meaning of the arguments of
>> bios_load(). For every other use of bios_load, the second argument is
>> used at the address of the firmware to load. Here "ipxe" is an extra
>> blob to load.
> 
> We either add more arguments to bios_load or repurpose the existing
> one(s). I'm not too fussed with either option really.
> 
> Jan and Andrew, your opinion?

Hmm, it did look fine to me, but if Anthony has strong reservations,
maybe adding a new parameter might be better indeed?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH for-next 5/5] tools: provide --with-system-ipxe

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 config/Tools.mk.in        |  1 +
 tools/config.h.in         |  3 +++
 tools/configure           | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 tools/configure.ac        | 18 ++++++++++++++++
 tools/libxl/libxl_paths.c |  6 +++++-
 5 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 4cc9f29090..0964f6f9e9 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -50,6 +50,7 @@ FLASK_POLICY        := @xsmpolicy@
 CONFIG_OVMF         := @ovmf@
 CONFIG_ROMBIOS      := @rombios@
 CONFIG_SEABIOS      := @seabios@
+CONFIG_IPXE         := @ipxe@
 CONFIG_QEMU_TRAD    := @qemu_traditional@
 CONFIG_QEMU_XEN     := @qemu_xen@
 CONFIG_BLKTAP2      := @blktap2@
diff --git a/tools/config.h.in b/tools/config.h.in
index c66a78c9b3..5987f087b8 100644
--- a/tools/config.h.in
+++ b/tools/config.h.in
@@ -96,6 +96,9 @@
 /* libutil header file name */
 #undef INCLUDE_LIBUTIL_H
 
+/* IPXE path */
+#undef IPXE_PATH
+
 /* OVMF path */
 #undef OVMF_PATH
 
diff --git a/tools/configure b/tools/configure
index f282e9f5b3..16960a425b 100755
--- a/tools/configure
+++ b/tools/configure
@@ -703,6 +703,7 @@ AS86
 qemu_traditional
 blktap2
 LINUX_BACKEND_MODULES
+ipxe
 seabios
 ovmf
 xsmpolicy
@@ -805,6 +806,7 @@ enable_ocamltools
 enable_xsmpolicy
 enable_ovmf
 enable_seabios
+enable_ipxe
 with_linux_backend_modules
 enable_blktap2
 enable_qemu_traditional
@@ -812,6 +814,7 @@ enable_rombios
 with_system_qemu
 with_system_seabios
 with_system_ovmf
+with_system_ipxe
 with_extra_qemuu_configure_args
 with_xenstored
 enable_systemd
@@ -1488,6 +1491,7 @@ Optional Features:
   --disable-xsmpolicy     Disable XSM policy compilation (default is ENABLED)
   --enable-ovmf           Enable OVMF (default is DISABLED)
   --disable-seabios       Disable SeaBIOS (default is ENABLED)
+  --disable-ipxe          Disable IPXE (default is ENABLED)
   --enable-blktap2        Enable blktap2, (DEFAULT is off)
   --enable-qemu-traditional
                           Enable qemu traditional device model, (DEFAULT is on
@@ -1527,6 +1531,9 @@ Optional Packages:
   --with-system-ovmf[=PATH]
                           Use system supplied OVMF PATH instead of building
                           and installing our own version
+  --with-system-ipxe[=PATH]
+                          Use system supplied IPXE PATH instead of building
+                          and installing our own version
   --with-extra-qemuu-configure-args[="--ARG1 ..."]
                           List of additional configure options for upstream
                           qemu
@@ -4184,6 +4191,29 @@ seabios=$ax_cv_seabios
 
 
 
+# Check whether --enable-ipxe was given.
+if test "${enable_ipxe+set}" = set; then :
+  enableval=$enable_ipxe;
+fi
+
+
+if test "x$enable_ipxe" = "xno"; then :
+
+    ax_cv_ipxe="n"
+
+elif test "x$enable_ipxe" = "xyes"; then :
+
+    ax_cv_ipxe="y"
+
+elif test -z $ax_cv_ipxe; then :
+
+    ax_cv_ipxe="y"
+
+fi
+ipxe=$ax_cv_ipxe
+
+
+
 
 # Check whether --with-linux-backend-modules was given.
 if test "${with_linux_backend_modules+set}" = set; then :
@@ -4573,6 +4603,29 @@ _ACEOF
 fi
 
 
+# Check whether --with-system-ipxe was given.
+if test "${with_system_ipxe+set}" = set; then :
+  withval=$with_system_ipxe;
+    # Disable compilation of IPXE.
+    ipxe=n
+    case $withval in
+        no) ipxe_path= ;;
+        *)  ipxe_path=$withval ;;
+    esac
+
+fi
+
+if test "x$ipxe" = "xy" -o -n "$ipxe_path" ; then :
+
+
+cat >>confdefs.h <<_ACEOF
+#define IPXE_PATH "${ipxe_path:-$XENFIRMWAREDIR/ipxe.bin}"
+_ACEOF
+
+
+fi
+
+
 # Check whether --with-extra-qemuu-configure-args was given.
 if test "${with_extra_qemuu_configure_args+set}" = set; then :
   withval=$with_extra_qemuu_configure_args;
diff --git a/tools/configure.ac b/tools/configure.ac
index 0826af8cbc..8e4b173d6f 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -84,6 +84,7 @@ AX_ARG_DEFAULT_ENABLE([ocamltools], [Disable Ocaml tools])
 AX_ARG_DEFAULT_ENABLE([xsmpolicy], [Disable XSM policy compilation])
 AX_ARG_DEFAULT_DISABLE([ovmf], [Enable OVMF])
 AX_ARG_DEFAULT_ENABLE([seabios], [Disable SeaBIOS])
+AX_ARG_DEFAULT_ENABLE([ipxe], [Disable IPXE])
 
 AC_ARG_WITH([linux-backend-modules],
     AS_HELP_STRING([--with-linux-backend-modules="mod1 mod2"],
@@ -241,6 +242,23 @@ AS_IF([test "x$ovmf" = "xy" -o -n "$ovmf_path" ], [
                        [OVMF path])
 ])
 
+AC_ARG_WITH([system-ipxe],
+    AS_HELP_STRING([--with-system-ipxe@<:@=PATH@:>@],
+       [Use system supplied IPXE PATH instead of building and installing
+        our own version]),[
+    # Disable compilation of IPXE.
+    ipxe=n
+    case $withval in
+        no) ipxe_path= ;;
+        *)  ipxe_path=$withval ;;
+    esac
+],[])
+AS_IF([test "x$ipxe" = "xy" -o -n "$ipxe_path" ], [
+    AC_DEFINE_UNQUOTED([IPXE_PATH],
+                       ["${ipxe_path:-$XENFIRMWAREDIR/ipxe.bin}"],
+                       [IPXE path])
+])
+
 AC_ARG_WITH([extra-qemuu-configure-args],
     AS_HELP_STRING([--with-extra-qemuu-configure-args@<:@="--ARG1 ..."@:>@],
        [List of additional configure options for upstream qemu]),[
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index 8498f82781..3f6a33628e 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -55,7 +55,11 @@ const char *libxl__ovmf_path(void)
 
 const char *libxl__ipxe_path(void)
 {
-    return XENFIRMWAREDIR "/ipxe.bin";
+#ifdef IPXE_PATH
+    return IPXE_PATH;
+#else
+    return NULL;
+#endif
 }
 
 /*
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 5/5] tools: provide --with-system-ipxe

[Anthony PERARD]

On Tue, May 15, 2018 at 07:22:43PM +0100, Wei Liu wrote:
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 0826af8cbc..8e4b173d6f 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -241,6 +242,23 @@ AS_IF([test "x$ovmf" = "xy" -o -n "$ovmf_path" ], [
>                         [OVMF path])
>  ])
>  
> +AC_ARG_WITH([system-ipxe],
> +    AS_HELP_STRING([--with-system-ipxe@<:@=PATH@:>@],
> +       [Use system supplied IPXE PATH instead of building and installing
> +        our own version]),[
> +    # Disable compilation of IPXE.
> +    ipxe=n
> +    case $withval in
> +        no) ipxe_path= ;;
> +        *)  ipxe_path=$withval ;;
> +    esac
> +],[])
> +AS_IF([test "x$ipxe" = "xy" -o -n "$ipxe_path" ], [
> +    AC_DEFINE_UNQUOTED([IPXE_PATH],
> +                       ["${ipxe_path:-$XENFIRMWAREDIR/ipxe.bin}"],
> +                       [IPXE path])
> +])
> +

I think there are issues with the way we handle --with-system-X. If a
user run `configure --with-system-ipxe`, without a = or a PATH, then in
the hunk abrove, $withval will be "yes" and this will result with
IPXE_PATH=yes

I don't know if you want to look at that, since you're introducing a new
one.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-next 5/5] tools: provide --with-system-ipxe

[Wei Liu]

On Mon, Jun 04, 2018 at 02:59:21PM +0100, Anthony PERARD wrote:
> On Tue, May 15, 2018 at 07:22:43PM +0100, Wei Liu wrote:
> > diff --git a/tools/configure.ac b/tools/configure.ac
> > index 0826af8cbc..8e4b173d6f 100644
> > --- a/tools/configure.ac
> > +++ b/tools/configure.ac
> > @@ -241,6 +242,23 @@ AS_IF([test "x$ovmf" = "xy" -o -n "$ovmf_path" ], [
> >                         [OVMF path])
> >  ])
> >  
> > +AC_ARG_WITH([system-ipxe],
> > +    AS_HELP_STRING([--with-system-ipxe@<:@=PATH@:>@],
> > +       [Use system supplied IPXE PATH instead of building and installing
> > +        our own version]),[
> > +    # Disable compilation of IPXE.
> > +    ipxe=n
> > +    case $withval in
> > +        no) ipxe_path= ;;
> > +        *)  ipxe_path=$withval ;;
> > +    esac
> > +],[])
> > +AS_IF([test "x$ipxe" = "xy" -o -n "$ipxe_path" ], [
> > +    AC_DEFINE_UNQUOTED([IPXE_PATH],
> > +                       ["${ipxe_path:-$XENFIRMWAREDIR/ipxe.bin}"],
> > +                       [IPXE path])
> > +])
> > +
> 
> I think there are issues with the way we handle --with-system-X. If a
> user run `configure --with-system-ipxe`, without a = or a PATH, then in
> the hunk abrove, $withval will be "yes" and this will result with
> IPXE_PATH=yes
> 
> I don't know if you want to look at that, since you're introducing a new
> one.
> 

We can add a branch to all of them to print out an error when
$withval is yes.

Wei.

> -- 
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel