Make iPXE a standalone ROM

Make iPXE a standalone ROM

[Anoob Soman]

Make the iPXE ROM be built as a standalone ROM, rather than being embedded
into hvmloader and pass the iPXE ROM to hvmloader via module, in the same way
as OVMF/SeaBIOS are currently passed

Introduce a ./configure --with-system-ipxe=$path option

This allows us to disentangle iPXE from hvmloader, and allows us to
edit/modify/upgrade iPXE independently of Xen.

Anoob Soman (5):
  tools/firmware: Build ipxe as a standalone ROM
  tools/firmware: #define IPXE_PATH
  libxc: Allow loading of firmware modules for HVM guest
  libxl: Load iPXE ROM from a file
  hvmloader: Use iPXE ROM loaded from a standalone file

 config/Tools.mk.in                   |  2 ++
 tools/configure.ac                   | 18 ++++++++++++++++++
 tools/firmware/Makefile              |  5 ++++-
 tools/firmware/hvmloader/Makefile    | 18 +++++++++++-------
 tools/firmware/hvmloader/config.h    |  5 +++++
 tools/firmware/hvmloader/hvmloader.c |  9 ++++++++-
 tools/firmware/hvmloader/rombios.c   | 13 +++++++++----
 tools/libxc/xc_dom_x86.c             | 13 +++++++++++++
 tools/libxl/libxl_dom.c              | 12 ++++++++++++
 tools/libxl/libxl_internal.h         |  1 +
 tools/libxl/libxl_paths.c            |  9 +++++++++
 11 files changed, 92 insertions(+), 13 deletions(-)

-- 
1.8.3.1


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

[PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Anoob Soman]

This patches doesn't get rid of etherboot[] from roms.inc. Instead,
makes a standalone iPXE rom, which will later be used by hvmloader (when
all the plubming to use standalone iPXE rom are in place)

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 tools/firmware/Makefile           | 3 +++
 tools/firmware/hvmloader/Makefile | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 5a7cf77..20cab38 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -59,6 +59,9 @@ 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
 endif
+ifeq ($(CONFIG_ROMBIOS),y)
+	$(INSTALL_DATA) etherboot/ipxe/src/bin/ipxe.bin $(INST_DIR)/ipxe.bin
+endif
 
 .PHONY: uninstall
 uninstall:
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index a5b4c32..087b41d 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -52,6 +52,7 @@ 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 := 
@@ -71,7 +72,7 @@ all: acpi subdirs-all
 acpi:
 	$(MAKE) -C $(ACPI_PATH)  ACPI_BUILD_DIR=$(CURDIR) DSDT_FILES="$(DSDT_FILES)"
 
-rombios.o: roms.inc
+rombios.o: $(ETHERBOOT_ROM) roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
 
 ACPI_PATH = ../../libacpi
@@ -113,6 +114,11 @@ endif
 
 	mv $@.new $@
 
+ifneq ($(ETHERBOOT_ROMS),)
+$(ETHERBOOT_ROM): $(ETHERBOOT_ROMS)
+	cat $^ > $@
+endif
+
 .PHONY: clean
 clean: subdirs-clean
 	rm -f roms.inc roms.inc.new acpi.h
-- 
1.8.3.1


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

[PATCH 2/5] tools/firmware: #define IPXE_PATH

[Anoob Soman]

--with-system-ipxe allows the user to specify ipxe rom. If this option
is given, use system supplied ipxe instead of building and installing
our own version

Plumbing for using iPXE roms, specified with --with-system-ipxe, doesn't
exist and will be added in future commits.

Re-run of autoconf is needed.

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 config/Tools.mk.in                |  2 ++
 tools/configure.ac                | 18 ++++++++++++++++++
 tools/firmware/Makefile           |  4 ++--
 tools/firmware/hvmloader/Makefile |  3 +++
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 0f79f4e..dc6eae8 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -24,6 +24,7 @@ AWK                 := @AWK@
 FETCHER             := @FETCHER@
 SEABIOS_PATH        := @seabios_path@
 OVMF_PATH           := @ovmf_path@
+IPXE_PATH           := @ipxe_path@
 
 # Extra folder for libs/includes
 PREPEND_INCLUDES    := @PREPEND_INCLUDES@
@@ -59,6 +60,7 @@ CONFIG_QEMU_XEN     := @qemu_xen@
 CONFIG_BLKTAP2      := @blktap2@
 CONFIG_QEMUU_EXTRA_ARGS:= @EXTRA_QEMUU_CONFIGURE_ARGS@
 CONFIG_LIBNL        := @libnl@
+CONFIG_IPXE         := @ipxe@
 
 CONFIG_SYSTEMD      := @systemd@
 SYSTEMD_CFLAGS      := @SYSTEMD_CFLAGS@
diff --git a/tools/configure.ac b/tools/configure.ac
index 06eb16d..022a2f9 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/firmware/Makefile b/tools/firmware/Makefile
index 20cab38..ca333a9 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -15,7 +15,7 @@ SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
 SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
 SUBDIRS-$(CONFIG_ROMBIOS) += rombios
 SUBDIRS-$(CONFIG_ROMBIOS) += vgabios
-SUBDIRS-$(CONFIG_ROMBIOS) += etherboot
+SUBDIRS-$(CONFIG_IPXE)    += etherboot
 SUBDIRS-$(CONFIG_PV_SHIM) += xen-dir
 SUBDIRS-y += hvmloader
 
@@ -59,7 +59,7 @@ 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
 endif
-ifeq ($(CONFIG_ROMBIOS),y)
+ifeq ($(CONFIG_IPXE),y)
 	$(INSTALL_DATA) etherboot/ipxe/src/bin/ipxe.bin $(INST_DIR)/ipxe.bin
 endif
 
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 087b41d..fa1bf30 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -51,6 +51,9 @@ CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
 else
 CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.bin
 endif
+endif
+
+ifeq ($(CONFIG_IPXE),y)
 ETHERBOOT_ROMS := $(addprefix ../etherboot/ipxe/src/bin/, $(addsuffix .rom, $(ETHERBOOT_NICS)))
 ETHERBOOT_ROM  := ../etherboot/ipxe/src/bin/ipxe.bin
 endif
-- 
1.8.3.1


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

[PATCH 3/5] libxc: Allow loading of firmware modules for HVM guest

[Anoob Soman]

This allows to load iPXE rom as a firmware module, instead of requiring
it to be embedded into hvmloader.

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0b65dab..be06d43 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1723,6 +1723,19 @@ static int bootlate_hvm(struct xc_dom_image *dom)
     {
         add_module_to_list(dom, &dom->system_firmware_module, "firmware",
                            modlist, 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;
+            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);
+        }
     }
 
     if ( start_info->nr_modules )
-- 
1.8.3.1


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

[PATCH 4/5] libxl: Load iPXE ROM from a file

[Anoob Soman]

Load iPXE ROM from a file pointed to by IPXE_PATH. If --with-system-ipxe
is not specified default Xen firmware directory is picked up as
IPXE_PATH

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 tools/libxl/libxl_dom.c      | 12 ++++++++++++
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_paths.c    |  9 +++++++++
 3 files changed, 22 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2e29b52..104d6a0 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1003,6 +1003,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
     int datalen = 0;
     void *data;
     const char *bios_filename = NULL;
+    const char *ipxe_filename = NULL;
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (info->u.hvm.firmware) {
@@ -1094,6 +1095,17 @@ static int libxl__domain_firmware(libxl__gc *gc,
         assert(info->type == LIBXL_DOMAIN_TYPE_HVM);
         rc = xc_dom_kernel_file(dom, libxl__abs_path(gc, firmware,
                                                  libxl__xenfirmwaredir_path()));
+        if (rc) {
+            LOGE(ERROR, "xc_dom_kernel_file failed");
+            goto out;
+        }
+        if ((ipxe_filename = libxl__ipxe_path())) {
+            rc = xc_dom_module_file(dom, ipxe_filename, "ipxe");
+            if (rc) {
+                LOGE(ERROR, "xc_dom_ipxe_module_file failed");
+                goto out;
+            }
+       }
     }
 
     if (rc != 0) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 506687f..f649696 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2359,6 +2359,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 0643c1b..3f6a336 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -53,6 +53,15 @@ const char *libxl__ovmf_path(void)
 #endif
 }
 
+const char *libxl__ipxe_path(void)
+{
+#ifdef IPXE_PATH
+    return IPXE_PATH;
+#else
+    return NULL;
+#endif
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.8.3.1


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

[PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Anoob Soman]

splatering of mkhex-ed etherboot inside hvmloader/rombios is removed,
instead hvmloader/rombios now relies on iPXE ROM to be added,loaded as
a module.

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 tools/firmware/hvmloader/Makefile    |  7 +------
 tools/firmware/hvmloader/config.h    |  5 +++++
 tools/firmware/hvmloader/hvmloader.c |  9 ++++++++-
 tools/firmware/hvmloader/rombios.c   | 13 +++++++++----
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index fa1bf30..f347cb4 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -64,7 +64,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)
 endif
 
 .PHONY: all
@@ -109,11 +109,6 @@ ifneq ($(CIRRUSVGA_ROM),)
 	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
-ifneq ($(ETHERBOOT_ROMS),)
-	echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-	sh ../../misc/mkhex etherboot $(ETHERBOOT_ROMS) >> $@.new
-	echo "#endif" >> $@.new
-endif
 
 	mv $@.new $@
 
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 6e00413..8f5d1040 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -33,6 +33,11 @@ struct bios_config {
     void (*create_mp_tables)(void);
     void (*create_smbios_tables)(void);
     void (*create_pir_tables)(void);
+
+    /* Physical address of iPXE ROM, loaded by domain builder
+     * when using ROMBIOS
+     */
+    unsigned int *ipxe_rom_addresss;
 };
 
 extern struct bios_config rombios_config;
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index f603f68..731cd81 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -323,6 +323,8 @@ int main(void)
     const struct bios_config *bios;
     int acpi_enabled;
     const struct hvm_modlist_entry *bios_module;
+    const struct hvm_modlist_entry *ipxe_module = NULL;
+    unsigned int ipxe_rom_addresss = 0;
 
     /* Initialise hypercall stubs with RET, rendering them no-ops. */
     memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
@@ -368,7 +370,12 @@ int main(void)
 #ifdef ENABLE_ROMBIOS
     else if ( bios == &rombios_config )
     {
-        bios->bios_load(bios, NULL, 0);
+        ipxe_module = get_module_entry(hvm_start_info, "ipxe");
+
+        if ( ipxe_module )
+            ipxe_rom_addresss = ipxe_module->paddr;
+
+        bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
     }
 #endif
     else
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index c736fd9..c0e04cb 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -95,9 +95,12 @@ 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);
+
+    if ( rombios_config.ipxe_rom_addresss )
+        etherboot_sz = scan_etherboot_nic(OPTIONROM_PHYSICAL_END,
+                                          etherboot_phys_addr,
+                                          rombios_config.ipxe_rom_addresss);
+
 
     option_rom_phys_addr = etherboot_phys_addr + etherboot_sz;
     option_rom_sz = pci_load_option_roms(OPTIONROM_PHYSICAL_END,
@@ -119,7 +122,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_rom_addr, uint32_t unused_size)
 {
     uint32_t bioshigh;
     struct rombios_info *info;
@@ -133,6 +136,8 @@ static void rombios_load(const struct bios_config *config,
 
     info = (struct rombios_info *)BIOS_INFO_PHYSICAL_ADDRESS;
     info->bios32_entry = bioshigh;
+
+    rombios_config.ipxe_rom_addresss = ipxe_rom_addr;
 }
 
 /*
-- 
1.8.3.1


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

[PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Jan Beulich]

>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
> @@ -71,7 +72,7 @@ all: acpi subdirs-all
>  acpi:
>  	$(MAKE) -C $(ACPI_PATH)  ACPI_BUILD_DIR=$(CURDIR) DSDT_FILES="$(DSDT_FILES)"
>  
> -rombios.o: roms.inc
> +rombios.o: $(ETHERBOOT_ROM) roms.inc

Please don't introduce dead dependencies: If a need for this arises
in a later patch, add the dependency there.

> @@ -113,6 +114,11 @@ endif
>  
>  	mv $@.new $@
>  
> +ifneq ($(ETHERBOOT_ROMS),)
> +$(ETHERBOOT_ROM): $(ETHERBOOT_ROMS)
> +	cat $^ > $@
> +endif

I don't understand this: How can the bare concatenation of multiple
binary blobs produce anything usable? How will the consumer know
where the boundaries are, and which one is which?

Jan


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

Re: [PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Andrew Cooper]

On 16/03/18 11:18, Jan Beulich wrote:
>
>> @@ -113,6 +114,11 @@ endif
>>  
>>  	mv $@.new $@
>>  
>> +ifneq ($(ETHERBOOT_ROMS),)
>> +$(ETHERBOOT_ROM): $(ETHERBOOT_ROMS)
>> +	cat $^ > $@
>> +endif
> I don't understand this: How can the bare concatenation of multiple
> binary blobs produce anything usable? How will the consumer know
> where the boundaries are, and which one is which?

These details are all covered in the option ROM/PnP metadata.  See
scan_etherboot_nic()/scan_option_rom() in hvmloader.

~Andrew

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

Re: [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Jan Beulich]

>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -33,6 +33,11 @@ struct bios_config {
>      void (*create_mp_tables)(void);
>      void (*create_smbios_tables)(void);
>      void (*create_pir_tables)(void);
> +
> +    /* Physical address of iPXE ROM, loaded by domain builder
> +     * when using ROMBIOS
> +     */
> +    unsigned int *ipxe_rom_addresss;

Comment style. And can the pointer be to const?

> @@ -368,7 +370,12 @@ int main(void)
>  #ifdef ENABLE_ROMBIOS
>      else if ( bios == &rombios_config )
>      {
> -        bios->bios_load(bios, NULL, 0);
> +        ipxe_module = get_module_entry(hvm_start_info, "ipxe");
> +
> +        if ( ipxe_module )
> +            ipxe_rom_addresss = ipxe_module->paddr;
> +
> +        bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>      }
>  #endif

Considering the #ifdef around here - is this whole series an
enhancement for qemu-trad only? I don't think we mean to make
any such enhancements anymore.

Jan


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

Re: [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Andrew Cooper]

On 16/03/18 11:26, Jan Beulich wrote:
>>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
>> --- a/tools/firmware/hvmloader/config.h
>> +++ b/tools/firmware/hvmloader/config.h
>> @@ -33,6 +33,11 @@ struct bios_config {
>>      void (*create_mp_tables)(void);
>>      void (*create_smbios_tables)(void);
>>      void (*create_pir_tables)(void);
>> +
>> +    /* Physical address of iPXE ROM, loaded by domain builder
>> +     * when using ROMBIOS
>> +     */
>> +    unsigned int *ipxe_rom_addresss;
> Comment style. And can the pointer be to const?
>
>> @@ -368,7 +370,12 @@ int main(void)
>>  #ifdef ENABLE_ROMBIOS
>>      else if ( bios == &rombios_config )
>>      {
>> -        bios->bios_load(bios, NULL, 0);
>> +        ipxe_module = get_module_entry(hvm_start_info, "ipxe");
>> +
>> +        if ( ipxe_module )
>> +            ipxe_rom_addresss = ipxe_module->paddr;
>> +
>> +        bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>>      }
>>  #endif
> Considering the #ifdef around here - is this whole series an
> enhancement for qemu-trad only? I don't think we mean to make
> any such enhancements anymore.

It is for several important reasons, including allowing distros to
remove their qemu-trad packages completely.


The main purpose is to have iPXE as a ROM not embedded in hvmloader, and
finishes some incomplete earlier work by Anthony to split the SeaBIOS
and OVMF roms out of HVMLoader (and then later some work which Boris
offered to do, but time hasn't materialised yet).

This means that distros don't need to rebuild and redeploy their Xen
packages every time they want to change their iPXE package and/or don't
need to maintain multiple iPXE packages.

It also removes the final non-stubdom bit of the build which downloads a
random tarball from the web, which is something downstreams have been
crying out for for years.

As for the Qemu side of things, RomBIOS functions perfectly well with
qemu-upstream, and is the only viable transition mechanism available for
downstreams wishing to purge themselves of qemu-trad entirely, while
keeping older VMs working.

~Andrew

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

Re: [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Doug Goldstein]

On 3/16/18 6:44 AM, Andrew Cooper wrote:
> On 16/03/18 11:26, Jan Beulich wrote:
>>>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
>>> --- a/tools/firmware/hvmloader/config.h
>>> +++ b/tools/firmware/hvmloader/config.h
>>> @@ -33,6 +33,11 @@ struct bios_config {
>>>      void (*create_mp_tables)(void);
>>>      void (*create_smbios_tables)(void);
>>>      void (*create_pir_tables)(void);
>>> +
>>> +    /* Physical address of iPXE ROM, loaded by domain builder
>>> +     * when using ROMBIOS
>>> +     */
>>> +    unsigned int *ipxe_rom_addresss;
>> Comment style. And can the pointer be to const?
>>
>>> @@ -368,7 +370,12 @@ int main(void)
>>>  #ifdef ENABLE_ROMBIOS
>>>      else if ( bios == &rombios_config )
>>>      {
>>> -        bios->bios_load(bios, NULL, 0);
>>> +        ipxe_module = get_module_entry(hvm_start_info, "ipxe");
>>> +
>>> +        if ( ipxe_module )
>>> +            ipxe_rom_addresss = ipxe_module->paddr;
>>> +
>>> +        bios->bios_load(bios, (void *)ipxe_rom_addresss, 0);
>>>      }
>>>  #endif
>> Considering the #ifdef around here - is this whole series an
>> enhancement for qemu-trad only? I don't think we mean to make
>> any such enhancements anymore.
> 
> It is for several important reasons, including allowing distros to
> remove their qemu-trad packages completely.
> 
> 
> The main purpose is to have iPXE as a ROM not embedded in hvmloader, and
> finishes some incomplete earlier work by Anthony to split the SeaBIOS
> and OVMF roms out of HVMLoader (and then later some work which Boris
> offered to do, but time hasn't materialised yet).
> 
> This means that distros don't need to rebuild and redeploy their Xen
> packages every time they want to change their iPXE package and/or don't
> need to maintain multiple iPXE packages.>
> It also removes the final non-stubdom bit of the build which downloads a
> random tarball from the web, which is something downstreams have been
> crying out for for years.

I was originally asking if this was the case on the cover letter. I'll
lend a +1 to the overall series here. As someone that packages Xen in
two source based distros whose package managers run the build in a
separate phase from the fetch stage this is a welcome change. I've been
hacking around this and am glad to drop that.

-- 
Doug Goldstein

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

Re: [PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Doug Goldstein]

On 3/16/18 6:18 AM, Jan Beulich wrote:
>>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
>> @@ -71,7 +72,7 @@ all: acpi subdirs-all
>>  acpi:
>>  	$(MAKE) -C $(ACPI_PATH)  ACPI_BUILD_DIR=$(CURDIR) DSDT_FILES="$(DSDT_FILES)"
>>  
>> -rombios.o: roms.inc
>> +rombios.o: $(ETHERBOOT_ROM) roms.inc
> 
> Please don't introduce dead dependencies: If a need for this arises
> in a later patch, add the dependency there.

Well this is what's creating the ipxe.bin that's being installed in the
section you snipped out.

But I kind of agree. I feels like patch 1 & 2 belong together maybe?

-- 
Doug Goldstein

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

Re: [PATCH 3/5] libxc: Allow loading of firmware modules for HVM guest

[Doug Goldstein]

On 3/15/18 12:31 PM, Anoob Soman wrote:
> This allows to load iPXE rom as a firmware module, instead of requiring
> it to be embedded into hvmloader.
> 
> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 0b65dab..be06d43 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -1723,6 +1723,19 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>      {
>          add_module_to_list(dom, &dom->system_firmware_module, "firmware",
>                             modlist, start_info);
> +        for ( i = 0; i < dom->num_modules; i++ )
> +        {
> +            struct xc_hvm_firmware_module mod;
> +
> +            DOMPRINTF("Adding module %u", i);

nothing more helpful for debugging than the for loop's number available?

-- 
Doug Goldstein

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

Re: [PATCH 4/5] libxl: Load iPXE ROM from a file

[Doug Goldstein]

On 3/15/18 12:31 PM, Anoob Soman wrote:
> Load iPXE ROM from a file pointed to by IPXE_PATH. If --with-system-ipxe
> is not specified default Xen firmware directory is picked up as
> IPXE_PATH
> 
> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein

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

Re: [PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Jan Beulich]

>>> On 18.03.18 at 02:30, <cardoe@cardoe.com> wrote:
> On 3/16/18 6:18 AM, Jan Beulich wrote:
>>>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
>>> @@ -71,7 +72,7 @@ all: acpi subdirs-all
>>>  acpi:
>>>  	$(MAKE) -C $(ACPI_PATH)  ACPI_BUILD_DIR=$(CURDIR) DSDT_FILES="$(DSDT_FILES)"
>>>  
>>> -rombios.o: roms.inc
>>> +rombios.o: $(ETHERBOOT_ROM) roms.inc
>> 
>> Please don't introduce dead dependencies: If a need for this arises
>> in a later patch, add the dependency there.
> 
> Well this is what's creating the ipxe.bin that's being installed in the
> section you snipped out.

I don't understand: The question isn't what is being generated, but
whether rombios.o really depends on that binary blob, and nothing
in the patch here suggests it does. If the goal is simply to have a
dependency triggering the creation of the blob, then this should be
done via e.g. TARGET, and the whole logic should rather sit in
firmware/Makefile (matching the installation of it done there).

Jan


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

Re: [PATCH 1/5] tools/firmware: Build ipxe as a standalone ROM

[Doug Goldstein]

On 3/19/18 2:48 AM, Jan Beulich wrote:
>>>> On 18.03.18 at 02:30, <cardoe@cardoe.com> wrote:
>> On 3/16/18 6:18 AM, Jan Beulich wrote:
>>>>>> On 15.03.18 at 18:31, <anoob.soman@citrix.com> wrote:
>>>> @@ -71,7 +72,7 @@ all: acpi subdirs-all
>>>>  acpi:
>>>>  	$(MAKE) -C $(ACPI_PATH)  ACPI_BUILD_DIR=$(CURDIR) DSDT_FILES="$(DSDT_FILES)"
>>>>  
>>>> -rombios.o: roms.inc
>>>> +rombios.o: $(ETHERBOOT_ROM) roms.inc
>>>
>>> Please don't introduce dead dependencies: If a need for this arises
>>> in a later patch, add the dependency there.
>>
>> Well this is what's creating the ipxe.bin that's being installed in the
>> section you snipped out.
> 
> I don't understand: The question isn't what is being generated, but
> whether rombios.o really depends on that binary blob, and nothing
> in the patch here suggests it does. If the goal is simply to have a
> dependency triggering the creation of the blob, then this should be
> done via e.g. TARGET, and the whole logic should rather sit in
> firmware/Makefile (matching the installation of it done there).
> 
> Jan
> 

Oh, 100% agreement there.

-- 
Doug Goldstein

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

Re: [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Anoob Soman]

On 16/03/18 11:26, Jan Beulich wrote:
>>
>> +    /* Physical address of iPXE ROM, loaded by domain builder
>> +     * when using ROMBIOS
>> +     */
>> +    unsigned int *ipxe_rom_addresss;
> Comment style. And can the pointer be to const?

I will fixup the comment style and but making ipxe_rom_address a pointer 
to const will require codes changes in hvmloader/rombios.c, 
hvmloader/optionroms.c to make all function that use ipxe_rom_address 
const as well. Are you fine with changing these files/functions.

-Anoob

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

Re: [PATCH 3/5] libxc: Allow loading of firmware modules for HVM guest

[Anoob Soman]

On 18/03/18 01:32, Doug Goldstein wrote:
> On 3/15/18 12:31 PM, Anoob Soman wrote:
>> This allows to load iPXE rom as a firmware module, instead of requiring
>> it to be embedded into hvmloader.
>>
>> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
>> ---
>>   tools/libxc/xc_dom_x86.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 0b65dab..be06d43 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -1723,6 +1723,19 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>>       {
>>           add_module_to_list(dom, &dom->system_firmware_module, "firmware",
>>                              modlist, start_info);
>> +        for ( i = 0; i < dom->num_modules; i++ )
>> +        {
>> +            struct xc_hvm_firmware_module mod;
>> +
>> +            DOMPRINTF("Adding module %u", i);
> nothing more helpful for debugging than the for loop's number available?
>

I will add guest_addr_out, cmdline and length to DOMPRINTF.



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

Re: [PATCH 5/5] hvmloader: Use iPXE ROM loaded from a standalone file

[Jan Beulich]

>>> On 19.03.18 at 15:24, <anoob.soman@citrix.com> wrote:
> On 16/03/18 11:26, Jan Beulich wrote:
>>>
>>> +    /* Physical address of iPXE ROM, loaded by domain builder
>>> +     * when using ROMBIOS
>>> +     */
>>> +    unsigned int *ipxe_rom_addresss;
>> Comment style. And can the pointer be to const?
> 
> I will fixup the comment style and but making ipxe_rom_address a pointer 
> to const will require codes changes in hvmloader/rombios.c, 
> hvmloader/optionroms.c to make all function that use ipxe_rom_address 
> const as well. Are you fine with changing these files/functions.

In a separate patch, yes. If you don't want to create a separate
patch, I'm also fine with your explanation of why it can't be const
for now.

Jan


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

Re: [PATCH 3/5] libxc: Allow loading of firmware modules for HVM guest

[Wei Liu]

On Thu, Mar 15, 2018 at 05:31:51PM +0000, Anoob Soman wrote:
> This allows to load iPXE rom as a firmware module, instead of requiring
> it to be embedded into hvmloader.
> 
> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 0b65dab..be06d43 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -1723,6 +1723,19 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>      {
>          add_module_to_list(dom, &dom->system_firmware_module, "firmware",
>                             modlist, 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;

Shouldn't this be 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);
> +        }

Now that both paths of the if ... else ... structure contain the same
code it should be lifted outside of the loop.

Wei.

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

Re: [PATCH 2/5] tools/firmware: #define IPXE_PATH

[Wei Liu]

On Thu, Mar 15, 2018 at 05:31:50PM +0000, Anoob Soman wrote:
> --with-system-ipxe allows the user to specify ipxe rom. If this option
> is given, use system supplied ipxe instead of building and installing
> our own version
> 
> Plumbing for using iPXE roms, specified with --with-system-ipxe, doesn't
> exist and will be added in future commits.
> 
> Re-run of autoconf is needed.
> 
> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>

This looks fine. But it should probably be squashed to one of the other
patches.

Wei.

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

Re: [PATCH 4/5] libxl: Load iPXE ROM from a file

[Wei Liu]

On Thu, Mar 15, 2018 at 05:31:52PM +0000, Anoob Soman wrote:
> Load iPXE ROM from a file pointed to by IPXE_PATH. If --with-system-ipxe
> is not specified default Xen firmware directory is picked up as
> IPXE_PATH
> 
> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
> ---
>  tools/libxl/libxl_dom.c      | 12 ++++++++++++
>  tools/libxl/libxl_internal.h |  1 +
>  tools/libxl/libxl_paths.c    |  9 +++++++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2e29b52..104d6a0 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1003,6 +1003,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
>      int datalen = 0;
>      void *data;
>      const char *bios_filename = NULL;
> +    const char *ipxe_filename = NULL;
>  
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          if (info->u.hvm.firmware) {
> @@ -1094,6 +1095,17 @@ static int libxl__domain_firmware(libxl__gc *gc,
>          assert(info->type == LIBXL_DOMAIN_TYPE_HVM);
>          rc = xc_dom_kernel_file(dom, libxl__abs_path(gc, firmware,
>                                                   libxl__xenfirmwaredir_path()));
> +        if (rc) {
> +            LOGE(ERROR, "xc_dom_kernel_file failed");
> +            goto out;
> +        }
> +        if ((ipxe_filename = libxl__ipxe_path())) {
> +            rc = xc_dom_module_file(dom, ipxe_filename, "ipxe");
> +            if (rc) {
> +                LOGE(ERROR, "xc_dom_ipxe_module_file failed");
> +                goto out;

This is the wrong place. Being an HVM guest doesn't mean ipxe should be
loaded. You probably need to look a few lines down and add code in
appropriate places like when ROMBIOS is picked.

Wei.

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

Re: [PATCH 3/5] libxc: Allow loading of firmware modules for HVM guest

[Anoob Soman]

On 21/03/18 15:17, Wei Liu wrote:
> On Thu, Mar 15, 2018 at 05:31:51PM +0000, Anoob Soman wrote:
>>                              modlist, 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;
> Shouldn't this be dom->modules[i].seg.vstart - dom->parms.virt_base?

It didn't work when guest_addr_out = seg.vstart - virt_base. I will try 
to figure out why.

>
>> +            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);
>> +        }
> Now that both paths of the if ... else ... structure contain the same
> code it should be lifted outside of the loop.

Sure, I can do that.

-Anoob

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

Re: [PATCH 4/5] libxl: Load iPXE ROM from a file

[Anoob Soman]

On 21/03/18 15:25, Wei Liu wrote:
>
>> +            LOGE(ERROR, "xc_dom_kernel_file failed");
>> +            goto out;
>> +        }
>> +        if ((ipxe_filename = libxl__ipxe_path())) {
>> +            rc = xc_dom_module_file(dom, ipxe_filename, "ipxe");
>> +            if (rc) {
>> +                LOGE(ERROR, "xc_dom_ipxe_module_file failed");
>> +                goto out;
> This is the wrong place. Being an HVM guest doesn't mean ipxe should be
> loaded. You probably need to look a few lines down and add code in
> appropriate places like when ROMBIOS is picked.

Sure, I will fix it up.

>
> Wei.



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

Re: [PATCH 2/5] tools/firmware: #define IPXE_PATH

[Anoob Soman]

On 21/03/18 15:18, Wei Liu wrote:
> On Thu, Mar 15, 2018 at 05:31:50PM +0000, Anoob Soman wrote:
>> --with-system-ipxe allows the user to specify ipxe rom. If this option
>> is given, use system supplied ipxe instead of building and installing
>> our own version
>>
>> Plumbing for using iPXE roms, specified with --with-system-ipxe, doesn't
>> exist and will be added in future commits.
>>
>> Re-run of autoconf is needed.
>>
>> Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
> This looks fine. But it should probably be squashed to one of the other

Sure, let me see what I can do.

> patches.
>
> Wei.



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