[RFC PATCH v2 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.

[RFC PATCH v2 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.

[Anthony PERARD]

Hi all,

I've start to look at loading the BIOS and the ACPI tables via the
toolstack instead of having them embedded in the hvmloader binary. After
this patch series, hvmloader compilation would be indenpendant from SeaBIOS
and OVMF compilation.

Compare to the V1, this is now done via the struct hvm_start_info from
Roger's patch series "Introduce HVM without dm and new boot ABI".

Here is a general view of the few step to load the BIOS:
- libxl load the BIOS blob into it's memory and add it to struct
  xc_hvm_build_args.bios_module
- libxc load the blob into the guest memory and fill the struct
  hvm_start_info, with a cmdline which would contain the order in which the
  modules (bios, acpi_table) are loaded into the modlist.
- hvmloader read the addresses from the hvm_start_info, find out which
  module are which by reading the cmdline and copy the blob to the right
  place.

Right now, this patch series would only work for SeaBIOS and OVMF. I have
not look into what to do about qemu-xen-traditionnal and rombios.

A git tree can be found here:
git://xenbits.xen.org/people/aperard/xen-unstable.git
tag: hvmloader-with-separated-bios-v2

Regards,

Anthony PERARD (16):
  hvmloader: Fix scratch_alloc to avoid overlaps
  libxc: Load BIOS and ACPI table into guest memory
  configure: #define SEABIOS_PATH and OVMF_PATH
  firmware/makefile: install BIOS and ACPI blob ...
  libxl: Load guest BIOS from file
  libxl: Load guest ACPI table from file
  hvmloader: Grab the hvmlite info page and parse the cmdline
  hvmloader: Locate the BIOS blob
  hvmloader: Load SeaBIOS from hvm_start_info modules ...
  hvmloader: Load OVMF from modules
  hvmloader: No BIOS ROM image allowed to be compiled in
  hvmloader: Load ACPI tables from hvm_start_info module
  hvmloader/makefile: Compile out SeaBIOS and OVMF ROM blob
  hvmloader: Always build-in SeaBIOS and OVMF loader
  hvmloader: Compile out the qemu-xen ACPI tables
  hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...

 .gitignore                             |   1 +
 tools/configure.ac                     |  12 +++-
 tools/firmware/Makefile                |  15 ++--
 tools/firmware/hvmloader/Makefile      |  32 ---------
 tools/firmware/hvmloader/acpi/Makefile |   8 ++-
 tools/firmware/hvmloader/config.h      |  11 +--
 tools/firmware/hvmloader/hvmloader.c   | 127 ++++++++++++++++++++++++++++-----
 tools/firmware/hvmloader/ovmf.c        |  34 ++++-----
 tools/firmware/hvmloader/seabios.c     |  33 ++++-----
 tools/firmware/hvmloader/util.c        |   2 +-
 tools/libxc/include/xc_dom.h           |   4 ++
 tools/libxc/xc_dom_hvmloader.c         |  44 +++++++++---
 tools/libxc/xc_dom_x86.c               |  90 +++++++++++++++++++++++
 tools/libxl/libxl_dom.c                |  83 +++++++++++++++++++++
 tools/libxl/libxl_types.idl            |   2 +
 tools/libxl/xl_cmdimpl.c               |  14 +++-
 16 files changed, 394 insertions(+), 118 deletions(-)

-- 
Anthony PERARD

[RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Anthony PERARD]

scratch_alloc() set scratch_start to the last byte of the current
allocation.  The value of scratch_start is then reused as is (if it is
already aligned) in the next allocation.  This result in a potential reuse
of the last byte of the previous allocation.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index d779fd7..42e8af4 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
         align = 16;
 
     s = (scratch_start + align - 1) & ~(align - 1);
-    e = s + size - 1;
+    e = s + size;
 
     BUG_ON(e < s);
 
-- 
Anthony PERARD

[RFC PATCH v2 02/16] libxc: Load BIOS and ACPI table into guest memory

[Anthony PERARD]

... and prepare a cmdline for hvmloader with the order of the modules.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xc_dom.h   |  4 ++
 tools/libxc/xc_dom_hvmloader.c | 44 +++++++++++++++++----
 tools/libxc/xc_dom_x86.c       | 90 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 4939f76..c7003a4 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -203,6 +203,10 @@ struct xc_dom_image {
 
     /* Extra SMBIOS structures passed to HVMLOADER */
     struct xc_hvm_firmware_module smbios_module;
+
+    /* BIOS as module */
+    struct xc_hvm_firmware_module bios_module;
+    struct xc_hvm_firmware_module acpi_table_module;
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 79a3b99..3987ed8 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -129,6 +129,18 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
     return rc;
 }
 
+static uint64_t module_init(struct xc_hvm_firmware_module *module,
+                            uint64_t mstart)
+{
+#define MODULE_ALIGN 1UL << 7
+#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
+    if ( module->length != 0 ) {
+        module->guest_addr_out = mstart;
+        return MKALIGN(module->length, MODULE_ALIGN);
+    } else
+        return 0;
+}
+
 static int modules_init(struct xc_dom_image *dom,
                         uint64_t vend, struct elf_binary *elf,
                         uint64_t *mstart_out, uint64_t *mend_out)
@@ -136,33 +148,47 @@ static int modules_init(struct xc_dom_image *dom,
 #define MODULE_ALIGN 1UL << 7
 #define MB_ALIGN     1UL << 20
 #define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
-    uint64_t total_len = 0, offset1 = 0;
+    uint64_t total_len = 0, offset1 = 0, offset0;
+    uint64_t mstart;
 
-    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
-        return 0;
+    /* Want to place the modules 1Mb+change behind the loader image. */
+    mstart = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
 
     /* Find the total length for the firmware modules with a reasonable large
      * alignment size to align each the modules.
      */
-    total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
+    total_len += module_init(&dom->bios_module, mstart + total_len);
+    total_len += module_init(&dom->acpi_table_module, mstart + total_len);
+    offset0 = total_len;
+    total_len += MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
     offset1 = total_len;
     total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
 
-    /* Want to place the modules 1Mb+change behind the loader image. */
-    *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
+    if ( total_len == 0 )
+        return 0;
+
+    *mstart_out = mstart;
     *mend_out = *mstart_out + total_len;
 
     if ( *mend_out > vend )
         return -1;
 
     if ( dom->acpi_module.length != 0 )
-        dom->acpi_module.guest_addr_out = *mstart_out;
+        dom->acpi_module.guest_addr_out = *mstart_out + offset0;
     if ( dom->smbios_module.length != 0 )
         dom->smbios_module.guest_addr_out = *mstart_out + offset1;
 
     return 0;
 }
 
+static void loadmodule(struct xc_hvm_firmware_module *module,
+                       uint8_t *dest, uint64_t mstart)
+{
+    if ( module->length != 0 )
+        memcpy(dest + (module->guest_addr_out - mstart),
+               module->data, module->length);
+}
+
 static int loadmodules(struct xc_dom_image *dom,
                        uint64_t mstart, uint64_t mend,
                        uint32_t domid)
@@ -201,9 +227,11 @@ static int loadmodules(struct xc_dom_image *dom,
     memset(dest, 0, pages << PAGE_SHIFT);
 
     /* Load modules into range */
+    loadmodule(&dom->bios_module, dest, mstart);
+    loadmodule(&dom->acpi_table_module, dest, mstart);
     if ( dom->acpi_module.length != 0 )
     {
-        memcpy(dest,
+        memcpy(dest + (dom->acpi_module.guest_addr_out - mstart),
                dom->acpi_module.data,
                dom->acpi_module.length);
     }
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index c3bb7a3..2444cc2 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -511,6 +511,27 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom)
     hvm_info->checksum = -sum;
 }
 
+static void add_module_to_list(struct xc_hvm_firmware_module *module,
+                               const char *name, char *cmdline, size_t n,
+                               struct hvm_modlist_entry *modlist,
+                               size_t modlist_size, int *module_nr)
+{
+    if ( module->length == 0 )
+        return;
+
+    /* assert(*module_nr < modlist_size); */
+
+    if ( *module_nr == 0 )
+        strcat(cmdline, "modules=");
+    else
+        strcat(cmdline, ",");
+    strcat(cmdline, name);
+
+    modlist[*module_nr].paddr = module->guest_addr_out;
+    modlist[*module_nr].size = module->length;
+    (*module_nr)++;
+}
+
 static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 {
     unsigned long i;
@@ -627,6 +648,75 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     }
     else
     {
+        struct xc_dom_seg seg;
+        struct hvm_start_info *start_info;
+        char *cmdline;
+        struct hvm_modlist_entry *modlist;
+        void *start_page;
+        size_t cmdline_size;
+        size_t start_info_size = sizeof(*start_info);
+
+        char cmdline_new[MAX_GUEST_CMDLINE];
+        int module_nr = 0;
+
+        struct hvm_modlist_entry modlist_building[4];
+
+        cmdline_new[0] = '\0';
+        add_module_to_list(&dom->bios_module, "bios",
+                           cmdline_new, MAX_GUEST_CMDLINE,
+                           modlist_building, ARRAY_SIZE(modlist_building),
+                           &module_nr);
+        add_module_to_list(&dom->acpi_table_module, "acpi_table",
+                           cmdline_new, MAX_GUEST_CMDLINE,
+                           modlist_building, ARRAY_SIZE(modlist_building),
+                           &module_nr);
+
+        start_info_size += sizeof(*modlist) * module_nr;
+        cmdline_size = ROUNDUP(strlen(cmdline_new) + 1, 8);
+        start_info_size += cmdline_size;
+
+        rc = xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
+                                  start_info_size);
+        if ( rc != 0 )
+        {
+            DOMPRINTF("Unable to reserve memory for the start info");
+            goto out;
+        }
+
+        start_page = xc_map_foreign_range(xch, domid, start_info_size,
+                                          PROT_READ | PROT_WRITE,
+                                          seg.pfn);
+        if ( start_page == NULL )
+        {
+            DOMPRINTF("Unable to map HVM start info page");
+            goto error_out;
+        }
+
+        start_info = start_page;
+        cmdline = start_page + sizeof(*start_info);
+        modlist = start_page + sizeof(*start_info) + cmdline_size;
+
+        if ( cmdline_size )
+        {
+            strncpy(cmdline, cmdline_new, MAX_GUEST_CMDLINE);
+            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
+            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
+                                ((xen_pfn_t)cmdline - (xen_pfn_t)start_info);
+        }
+
+        start_info->nr_modules = module_nr;
+        if ( start_info->nr_modules != 0 ) {
+            memcpy(modlist, modlist_building, sizeof(*modlist) * module_nr);
+            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
+                                ((xen_pfn_t)modlist - (xen_pfn_t)start_info);
+        }
+
+        start_info->magic = HVM_START_MAGIC_VALUE;
+
+        munmap(start_page, start_info_size);
+
+        dom->start_info_pfn = seg.pfn;
+
         /*
          * Allocate and clear additional ioreq server pages. The default
          * server will use the IOREQ and BUFIOREQ special pages above.
-- 
Anthony PERARD

[RFC PATCH v2 03/16] configure: #define SEABIOS_PATH and OVMF_PATH

[Anthony PERARD]

Those paths are to be used by libxl, in order to load the firmware in
memory. If a system path is not define, then this default to the Xen
firmware directory.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
Please, run ./autogen.sh on this patch.
---
 tools/configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/configure.ac b/tools/configure.ac
index 6c70040..6929006 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -212,6 +212,9 @@ AC_ARG_WITH([system-seabios],
     esac
 ],[])
 AC_SUBST(seabios_path)
+AC_DEFINE_UNQUOTED([SEABIOS_PATH],
+                   ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
+                   [SeaBIOS path])
 
 AC_ARG_WITH([system-ovmf],
     AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
@@ -223,6 +226,9 @@ AC_ARG_WITH([system-ovmf],
     esac
 ],[])
 AC_SUBST(ovmf_path)
+AC_DEFINE_UNQUOTED([OVMF_PATH],
+                   ["${ovmf_path:-$XENFIRMWAREDIR/ovmf.bin}"],
+                   [OVMF path])
 
 AC_ARG_WITH([extra-qemuu-configure-args],
     AS_HELP_STRING([--with-extra-qemuu-configure-args@<:@="--ARG1 ..."@:>@],
-- 
Anthony PERARD

[RFC PATCH v2 04/16] firmware/makefile: install BIOS and ACPI blob ...

[Anthony PERARD]

... into the firmware directory, along with hvmloader.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 .gitignore                             |  1 +
 tools/firmware/Makefile                | 15 +++++++++++++++
 tools/firmware/hvmloader/acpi/Makefile |  2 +-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index 9ead7c4..7c7bb56 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@ tools/firmware/hvmloader/acpi/mk_dsdt
 tools/firmware/hvmloader/acpi/dsdt*.c
 tools/firmware/hvmloader/acpi/dsdt_*.asl
 tools/firmware/hvmloader/acpi/ssdt_*.h
+tools/firmware/hvmloader/acpi/dsdt_anycpu_qemu_xen.aml
 tools/firmware/hvmloader/hvmloader
 tools/firmware/hvmloader/roms.h
 tools/firmware/hvmloader/roms.inc
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 6cc86ce..9c63991 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -19,6 +19,10 @@ SUBDIRS-y += hvmloader
 
 LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
 
+SEABIOS_ROM := seabios-dir/out/bios.bin
+OVMF_ROM := ovmf-dir/ovmf.bin
+ACPI_TABLE_QEMU_PC_I440FX = hvmloader/acpi/dsdt_anycpu_qemu_xen.aml
+
 ovmf-dir:
 	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
 	cp ovmf-makefile ovmf-dir/Makefile;
@@ -45,6 +49,17 @@ endif
 install: all
 	[ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
 	[ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
+ifeq ($(CONFIG_SEABIOS),y)
+ifeq ($(SEABIOS_PATH),)
+	[ ! -e $(SEABIOS_ROM) ] || $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
+endif
+endif
+ifeq ($(CONFIG_OVMF),y)
+ifeq ($(OVMF_PATH),)
+	[ ! -e $(OVMF_ROM) ] || $(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
+endif
+endif
+	[ ! -e $(ACPI_TABLE_QEMU_PC_I440FX) ] || $(INSTALL_DATA) $(ACPI_TABLE_QEMU_PC_I440FX) $(INST_DIR)
 
 .PHONY: clean
 clean: subdirs-clean
diff --git a/tools/firmware/hvmloader/acpi/Makefile b/tools/firmware/hvmloader/acpi/Makefile
index d3e882a..3d8dd21 100644
--- a/tools/firmware/hvmloader/acpi/Makefile
+++ b/tools/firmware/hvmloader/acpi/Makefile
@@ -46,7 +46,7 @@ $(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
 	iasl -vs -p $* -tc $*.asl
 	sed -e 's/AmlCode/$*/g' $*.hex >$@
 	echo "int $*_len=sizeof($*);" >>$@
-	rm -f $*.aml $*.hex
+	rm -f $*.hex
 
 iasl:
 	@echo
-- 
Anthony PERARD

[RFC PATCH v2 05/16] libxl: Load guest BIOS from file

[Anthony PERARD]

The path to the BIOS blob can be override by the xl's bios_override option,
or provided by u.hvm.bios_filename in the domain_build_info struct by other
libxl user.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dom.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 11 +++++---
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index b40d744..27a0021 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -858,6 +858,42 @@ err:
     return ret;
 }
 
+static const char *seabios_path(libxl__gc *gc)
+{
+    return SEABIOS_PATH;
+}
+
+static const char *ovmf_path(libxl__gc *gc)
+{
+    return OVMF_PATH;
+}
+
+static int libxl__load_hvm_firmware_module(libxl__gc *gc,
+                                           const char *filename,
+                                           const char *what,
+                                           struct xc_hvm_firmware_module *m)
+{
+    int datalen = 0;
+    void *data = NULL;
+    int e;
+
+    LOG(DEBUG, "Loading %s: %s", what, filename);
+    e = libxl_read_file_contents(CTX, filename, &data, &datalen);
+    if (e) {
+        LOGEV(ERROR, e, "failed to read %s file %s",
+              what,
+              filename);
+        return ERROR_FAIL;
+    }
+    libxl__ptr_add(gc, data);
+    if (datalen) {
+        /* Only accept non-empty files */
+        m->data = data;
+        m->length = (uint32_t)datalen;
+    }
+    return 0;
+}
+
 static int libxl__domain_firmware(libxl__gc *gc,
                                   libxl_domain_build_info *info,
                                   struct xc_dom_image *dom)
@@ -910,6 +946,36 @@ static int libxl__domain_firmware(libxl__gc *gc,
         goto out;
     }
 
+    if (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
+        const char *bios_filename;
+        // Look for BIOS and load it
+        if (info->u.hvm.bios_filename) {
+            bios_filename = info->u.hvm.bios_filename;
+        } else {
+            switch (info->u.hvm.bios)
+            {
+            case LIBXL_BIOS_TYPE_ROMBIOS:
+                bios_filename = libxl__abs_path(gc, "rombios.bin",
+                                                libxl__xenfirmwaredir_path());
+                break;
+            case LIBXL_BIOS_TYPE_SEABIOS:
+                bios_filename = seabios_path(gc);
+                break;
+            case LIBXL_BIOS_TYPE_OVMF:
+                bios_filename = ovmf_path(gc);
+                break;
+            default:
+                LOG(ERROR, "invalid bios version %d", info->u.hvm.bios);
+                rc = ERROR_FAIL;
+                goto out;
+            }
+        }
+
+        rc = libxl__load_hvm_firmware_module(gc, bios_filename, "BIOS",
+                                             &dom->bios_module);
+        if (rc) goto out;
+    }
+
     if (info->u.hvm.smbios_firmware) {
         data = NULL;
         e = libxl_read_file_contents(ctx, info->u.hvm.smbios_firmware,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 082fed8..a3fbcab 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -468,6 +468,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
+                                       ("bios_filename",    string),
                                        ("pae",              libxl_defbool),
                                        ("apic",             libxl_defbool),
                                        ("acpi",             libxl_defbool),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 840d73f..27d7c25 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1500,12 +1500,17 @@ static void parse_config_data(const char *config_source,
 
         xlu_cfg_replace_string (config, "firmware_override",
                                 &b_info->u.hvm.firmware, 0);
-        if (!xlu_cfg_get_string(config, "bios", &buf, 0) &&
-            libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
+        xlu_cfg_replace_string (config, "bios_override",
+                                &b_info->u.hvm.bios_filename, 0);
+        if (!xlu_cfg_get_string(config, "bios", &buf, 0)) {
+            if (libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
                 fprintf(stderr, "ERROR: invalid value \"%s\" for \"bios\"\n",
                     buf);
                 exit (1);
-        }
+            }
+        } else if (b_info->u.hvm.bios_filename)
+            fprintf(stderr, "WARNING: "
+                    "bios_override given without specific bios name\n");
 
         xlu_cfg_get_defbool(config, "pae", &b_info->u.hvm.pae, 0);
         xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
-- 
Anthony PERARD

[RFC PATCH v2 06/16] libxl: Load guest ACPI table from file

[Anthony PERARD]

The path to the ACPI tables blob can be override by xl's option
acpi_table_override or by acpi_tables_filename in the domain_build_info
struct for libxl user.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dom.c     | 17 +++++++++++++++++
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 27a0021..b340fa4 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -948,6 +948,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
 
     if (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
         const char *bios_filename;
+        const char *acpi_tables_filename;
         // Look for BIOS and load it
         if (info->u.hvm.bios_filename) {
             bios_filename = info->u.hvm.bios_filename;
@@ -970,10 +971,26 @@ static int libxl__domain_firmware(libxl__gc *gc,
                 goto out;
             }
         }
+        if (info->u.hvm.acpi_tables_filename) {
+            acpi_tables_filename = info->u.hvm.acpi_tables_filename;
+        } else {
+            // this would depend on which machine we emulate
+            // this is going to be eathier qemu-trad or qemu-xen
+            // (later, there will be qemu-xen-q35)
+            acpi_tables_filename = libxl__abs_path(gc,
+                                                   "dsdt_anycpu_qemu_xen.aml",
+                                                   libxl__xenfirmwaredir_path());
+        }
 
         rc = libxl__load_hvm_firmware_module(gc, bios_filename, "BIOS",
                                              &dom->bios_module);
         if (rc) goto out;
+
+        rc = libxl__load_hvm_firmware_module(gc,
+                                             acpi_tables_filename,
+                                             "ACPI tables",
+                                             &dom->acpi_table_module);
+        if (rc) goto out;
     }
 
     if (info->u.hvm.smbios_firmware) {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a3fbcab..d7eaa8d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -469,6 +469,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
                                        ("bios_filename",    string),
+                                       ("acpi_tables_filename", string),
                                        ("pae",              libxl_defbool),
                                        ("apic",             libxl_defbool),
                                        ("acpi",             libxl_defbool),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 27d7c25..1fb1300 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1512,6 +1512,9 @@ static void parse_config_data(const char *config_source,
             fprintf(stderr, "WARNING: "
                     "bios_override given without specific bios name\n");
 
+        xlu_cfg_replace_string(config, "acpi_table_override",
+                               &b_info->u.hvm.acpi_tables_filename, 0);
+
         xlu_cfg_get_defbool(config, "pae", &b_info->u.hvm.pae, 0);
         xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
         xlu_cfg_get_defbool(config, "acpi", &b_info->u.hvm.acpi, 0);
-- 
Anthony PERARD

[RFC PATCH v2 07/16] hvmloader: Grab the hvmlite info page and parse the cmdline

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/hvmloader.c | 69 +++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 716d03c..9df12ac 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -62,7 +62,7 @@ asm (
     "    mov  %ax,%ss                \n"
     /* Initialise all 32-bit GPRs to zero. */
     "    xor  %eax,%eax              \n"
-    "    xor  %ebx,%ebx              \n"
+    /* Keep ebx, for HVMLite start info */
     "    xor  %ecx,%ecx              \n"
     "    xor  %edx,%edx              \n"
     "    xor  %esp,%esp              \n"
@@ -249,15 +249,82 @@ static void acpi_enable_sci(void)
     BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
 }
 
+static const char *module_list_order = NULL;
+void cmdline_parser(const char *the_cmdline)
+{
+    char cmdline[MAX_GUEST_CMDLINE];
+    char *p, *q;
+    char *optval;
+
+    strncpy(cmdline, the_cmdline, sizeof (cmdline));
+    cmdline[MAX_GUEST_CMDLINE-1] = '\0';
+
+    for ( p = cmdline; p < (cmdline + MAX_GUEST_CMDLINE) && *p; p++ )
+    {
+        if ( *p == ' ' )
+            continue;
+
+        /* search for the end of the parameter name */
+        for ( q = p; *q && *q != '=' && *q != ' '; q++ )
+            ;
+
+        /* search for the end of the optional paremeter value */
+        if ( *q == '=' )
+        {
+            optval = q+1;
+            if (*optval == '\0' || *optval == ' ') {
+                optval = NULL;
+            }
+        } else
+            optval = NULL;
+        *q = '\0';
+
+        if ( optval )
+        {
+            for ( q = optval; *q && *q != ' '; q++ )
+                ;
+            *q = '\0';
+        }
+
+        /* compare known parameters */
+        if ( !strcmp(p, "modules") )
+        {
+            printf("  cmdline: found '%s', with val '%s'\n", p, optval);
+            if ( optval )
+            {
+                unsigned size = strlen(optval) + 1;
+                char *tmp = scratch_alloc(size, 0);
+                strncpy(tmp, optval, size);
+                module_list_order = tmp;
+            }
+        } else {
+            printf("  Unknown cmdline option '%s'", p);
+            if ( optval )
+                printf(" with val '%s'\n", optval);
+            else
+                printf("\n");
+        }
+
+        p = q;
+    }
+}
+
 int main(void)
 {
     const struct bios_config *bios;
     int acpi_enabled;
+    const struct hvm_start_info *hvmlite_start_info;
+
+    /* Load hvmlite start info pointer from ebx. */
+    asm volatile ( "mov %%ebx,%0" : "=r" (hvmlite_start_info) );
 
     /* Initialise hypercall stubs with RET, rendering them no-ops. */
     memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
 
     printf("HVM Loader\n");
+    BUG_ON(hvmlite_start_info->magic != HVM_START_MAGIC_VALUE);
+    printf("cmdline: %s\n", (char*)hvmlite_start_info->cmdline_paddr);
+    cmdline_parser((char*)hvmlite_start_info->cmdline_paddr);
 
     init_hypercalls();
 
-- 
Anthony PERARD

[RFC PATCH v2 08/16] hvmloader: Locate the BIOS blob

[Anthony PERARD]

The BIOS can be found in one of the entry of the modlist of the
hvm_start_info struct. The order of the modules are define by the command
line option "modules".

The found BIOS blob is not loaded by this patch, but only passed as
argument to bios_load() function. It is going to be used by the next few
patches.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/config.h    |  2 +-
 tools/firmware/hvmloader/hvmloader.c | 34 +++++++++++++++++++++++++++++++++-
 tools/firmware/hvmloader/ovmf.c      |  3 ++-
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index b838cf9..c4539cc 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -22,7 +22,7 @@ struct bios_config {
     /* ROMS */
     void (*load_roms)(void);
 
-    void (*bios_load)(const struct bios_config *config);
+    void (*bios_load)(const struct bios_config *config, void* addr, uint32_t size);
 
     void (*bios_info_setup)(void);
     void (*bios_info_finish)(void);
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 9df12ac..02d7f96 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -309,11 +309,39 @@ void cmdline_parser(const char *the_cmdline)
     }
 }
 
+const struct hvm_modlist_entry *get_module_entry(
+    const struct hvm_start_info *info,
+    const char *name)
+{
+    const char *p = module_list_order;
+    const char *q;
+    unsigned name_length = strlen(name);
+    unsigned current_module_nr = 0;
+    const struct hvm_modlist_entry *modlist =
+        (struct hvm_modlist_entry *)info->modlist_paddr;
+
+    if ( !module_list_order )
+        return NULL;
+
+    for ( ; *p && current_module_nr < info->nr_modules; p++ )
+    {
+        for ( q = p; *q && *q != ','; q++ )
+            ;
+        if ( !strncmp(name, p, max_t(unsigned, name_length, q - p)) )
+            return &modlist[current_module_nr];
+        p = q;
+        current_module_nr++;
+    }
+    printf("module '%s' not found\n", name);
+    return NULL;
+}
+
 int main(void)
 {
     const struct bios_config *bios;
     int acpi_enabled;
     const struct hvm_start_info *hvmlite_start_info;
+    const struct hvm_modlist_entry *bios_module;
 
     /* Load hvmlite start info pointer from ebx. */
     asm volatile ( "mov %%ebx,%0" : "=r" (hvmlite_start_info) );
@@ -353,9 +381,13 @@ int main(void)
         bios->create_smbios_tables();
     }
 
+    // XXX check that the values exist and are correct
+    bios_module = get_module_entry(hvmlite_start_info, "bios");
+    BUG_ON(!bios_module);
+
     printf("Loading %s ...\n", bios->name);
     if ( bios->bios_load )
-        bios->bios_load(bios);
+        bios->bios_load(bios, (void*)(bios_module->paddr), bios_module->size);
     else
     {
         BUG_ON(bios->bios_address + bios->image_size >
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index bb3da93..2be9752 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -93,7 +93,8 @@ static void ovmf_finish_bios_info(void)
     info->checksum = -checksum;
 }
 
-static void ovmf_load(const struct bios_config *config)
+static void ovmf_load(const struct bios_config *config,
+                      void *bios_addr, uint32_t bios_length)
 {
     xen_pfn_t mfn;
     uint64_t addr = OVMF_BEGIN;
-- 
Anthony PERARD

[RFC PATCH v2 09/16] hvmloader: Load SeaBIOS from hvm_start_info modules ...

[Anthony PERARD]

... and do not include the SeaBIOS ROM into hvmloader anymore.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/seabios.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index c6b3d9f..766bd6b 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -27,9 +27,6 @@
 #include "smbios_types.h"
 #include "acpi/acpi2_0.h"
 
-#define ROM_INCLUDE_SEABIOS
-#include "roms.inc"
-
 extern unsigned char dsdt_anycpu_qemu_xen[];
 extern int dsdt_anycpu_qemu_xen_len;
 
@@ -121,6 +118,7 @@ static void seabios_create_pir_tables(void)
     add_table(create_pir_tables());
 }
 
+unsigned int seabios_bios_address = 0;
 static void seabios_setup_e820(void)
 {
     struct seabios_info *info = (void *)BIOS_INFO_PHYSICAL_ADDRESS;
@@ -128,21 +126,27 @@ static void seabios_setup_e820(void)
     info->e820 = (uint32_t)e820;
 
     /* SeaBIOS reserves memory in e820 as necessary so no low reservation. */
-    info->e820_nr = build_e820_table(e820, 0, 0x100000-sizeof(seabios));
+    BUG_ON(seabios_bios_address == 0);
+    info->e820_nr = build_e820_table(e820, 0, seabios_bios_address);
     dump_e820_table(e820, info->e820_nr);
 }
 
-struct bios_config seabios_config = {
-    .name = "SeaBIOS",
+static void seabios_load(const struct bios_config *bios,
+                         void *bios_addr, uint32_t bios_length)
+{
+    unsigned int bios_dest = 0x100000 - bios_length;
+    seabios_bios_address = 0x100000 - bios_length;
 
-    .image = seabios,
-    .image_size = sizeof(seabios),
+    BUG_ON(bios_dest + bios_length > HVMLOADER_PHYSICAL_ADDRESS);
+    memcpy((void *)bios_dest, bios_addr, bios_length);
+}
 
-    .bios_address = 0x100000 - sizeof(seabios),
+struct bios_config seabios_config = {
+    .name = "SeaBIOS",
 
     .load_roms = NULL,
 
-    .bios_load = NULL,
+    .bios_load = seabios_load,
 
     .bios_info_setup = seabios_setup_bios_info,
     .bios_info_finish = seabios_finish_bios_info,
-- 
Anthony PERARD

[RFC PATCH v2 10/16] hvmloader: Load OVMF from modules

[Anthony PERARD]

... and do not include the OVMF ROM into hvmloader anymore.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 2be9752..3c0ec91 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -34,17 +34,9 @@
 #include <xen/hvm/ioreq.h>
 #include <xen/memory.h>
 
-#define ROM_INCLUDE_OVMF
-#include "roms.inc"
-
-#define OVMF_SIZE               (sizeof(ovmf))
-#define OVMF_MAXOFFSET          0x000FFFFFULL
-#define OVMF_BEGIN              (0x100000000ULL - ((OVMF_SIZE + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
-#define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
 #define LOWCHUNK_BEGIN          0x000F0000
 #define LOWCHUNK_SIZE           0x00010000
 #define LOWCHUNK_MAXOFFSET      0x0000FFFF
-#define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
 #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
 
 extern unsigned char dsdt_anycpu[];
@@ -96,12 +88,16 @@ static void ovmf_finish_bios_info(void)
 static void ovmf_load(const struct bios_config *config,
                       void *bios_addr, uint32_t bios_length)
 {
+#define OVMF_MAXOFFSET  0x000FFFFFULL
+#define OVMF_BEGIN      (0x100000000ULL - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
+#define OVMF_END        (OVMF_BEGIN + bios_length)
     xen_pfn_t mfn;
     uint64_t addr = OVMF_BEGIN;
+    unsigned int dest = OVMF_BEGIN;
 
     /* Copy low-reset vector portion. */
-    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
-           + OVMF_SIZE
+    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) bios_addr
+           + bios_length
            - LOWCHUNK_SIZE,
            LOWCHUNK_SIZE);
 
@@ -114,7 +110,7 @@ static void ovmf_load(const struct bios_config *config,
     }
 
     /* Copy FD. */
-    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
+    memcpy((void *) dest, bios_addr, bios_length);
 }
 
 static void ovmf_acpi_build_tables(void)
@@ -151,10 +147,6 @@ static void ovmf_setup_e820(void)
 struct bios_config ovmf_config =  {
     .name = "OVMF",
 
-    .image = ovmf,
-    .image_size = sizeof(ovmf),
-
-    .bios_address = OVMF_BEGIN,
     .bios_load = ovmf_load,
 
     .load_roms = 0,
-- 
Anthony PERARD

[RFC PATCH v2 11/16] hvmloader: No BIOS ROM image allowed to be compiled in

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/config.h    |  7 -------
 tools/firmware/hvmloader/hvmloader.c | 16 ++++------------
 2 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index c4539cc..0ddd897 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -12,13 +12,6 @@ extern unsigned long igd_opregion_pgbase;
 struct bios_config {
     const char *name;
 
-    /* BIOS ROM image bits */
-    void *image;
-    unsigned int image_size;
-
-    /* Physical address to load at */
-    unsigned int bios_address;
-
     /* ROMS */
     void (*load_roms)(void);
 
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 02d7f96..b131b1d 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -386,15 +386,7 @@ int main(void)
     BUG_ON(!bios_module);
 
     printf("Loading %s ...\n", bios->name);
-    if ( bios->bios_load )
-        bios->bios_load(bios, (void*)(bios_module->paddr), bios_module->size);
-    else
-    {
-        BUG_ON(bios->bios_address + bios->image_size >
-               HVMLOADER_PHYSICAL_ADDRESS);
-        memcpy((void *)bios->bios_address, bios->image,
-               bios->image_size);
-    }
+    bios->bios_load(bios, (void*)(bios_module->paddr), bios_module->size);
 
     if ( (hvm_info->nr_vcpus > 1) || hvm_info->apic_mode )
     {
@@ -432,9 +424,9 @@ int main(void)
     if ( SCRATCH_PHYSICAL_ADDRESS != scratch_start )
         printf(" %05x-%05lx: Scratch space\n",
                SCRATCH_PHYSICAL_ADDRESS, scratch_start);
-    printf(" %05x-%05x: Main BIOS\n",
-           bios->bios_address,
-           bios->bios_address + bios->image_size - 1);
+    /* printf(" %05x-%05x: Main BIOS\n", */
+           /* bios->bios_address, */
+           /* bios->bios_address + bios->image_size - 1); */
 
     if ( bios->e820_setup )
         bios->e820_setup();
-- 
Anthony PERARD

[RFC PATCH v2 12/16] hvmloader: Load ACPI tables from hvm_start_info module

[Anthony PERARD]

... and use it with both SeaBIOS and OVMF.

This may change the behavior of guest using OVMF as the ACPI tables
currently loaded are the one for qemu-xen-traditionnal. After this change,
the ACPI tables will the one intended for the device model used.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/config.h    | 2 +-
 tools/firmware/hvmloader/hvmloader.c | 6 +++++-
 tools/firmware/hvmloader/ovmf.c      | 9 +++------
 tools/firmware/hvmloader/seabios.c   | 9 +++------
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 0ddd897..1df5fd9 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -22,7 +22,7 @@ struct bios_config {
 
     void (*e820_setup)(void);
 
-    void (*acpi_build_tables)(void);
+    void (*acpi_build_tables)(void* addr, uint32_t size);
     void (*create_mp_tables)(void);
     void (*create_smbios_tables)(void);
     void (*create_pir_tables)(void);
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index b131b1d..9e5b9d4 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -407,8 +407,12 @@ int main(void)
 
         if ( bios->acpi_build_tables )
         {
+            const struct hvm_modlist_entry *acpi_module;
+            acpi_module = get_module_entry(hvmlite_start_info, "acpi_tables");
+            BUG_ON(!acpi_module);
             printf("Loading ACPI ...\n");
-            bios->acpi_build_tables();
+            bios->acpi_build_tables((void*)acpi_module->paddr,
+                                    acpi_module->size);
         }
 
         acpi_enable_sci();
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 3c0ec91..1a9d457 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -39,9 +39,6 @@
 #define LOWCHUNK_MAXOFFSET      0x0000FFFF
 #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
 
-extern unsigned char dsdt_anycpu[];
-extern int dsdt_anycpu_len;
-
 #define OVMF_INFO_MAX_TABLES 4
 struct ovmf_info {
     char signature[14]; /* XenHVMOVMF\0\0\0\0 */
@@ -113,11 +110,11 @@ static void ovmf_load(const struct bios_config *config,
     memcpy((void *) dest, bios_addr, bios_length);
 }
 
-static void ovmf_acpi_build_tables(void)
+static void ovmf_acpi_build_tables(void *addr, uint32_t length)
 {
     struct acpi_config config = {
-        .dsdt_anycpu = dsdt_anycpu,
-        .dsdt_anycpu_len = dsdt_anycpu_len,
+        .dsdt_anycpu = addr,
+        .dsdt_anycpu_len = length,
         .dsdt_15cpu = NULL, 
         .dsdt_15cpu_len = 0
     };
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index 766bd6b..4cd035e 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -27,9 +27,6 @@
 #include "smbios_types.h"
 #include "acpi/acpi2_0.h"
 
-extern unsigned char dsdt_anycpu_qemu_xen[];
-extern int dsdt_anycpu_qemu_xen_len;
-
 struct seabios_info {
     char signature[14]; /* XenHVMSeaBIOS\0 */
     uint8_t length;     /* Length of this struct */
@@ -87,12 +84,12 @@ static void add_table(uint32_t t)
     info->tables_nr++;
 }
 
-static void seabios_acpi_build_tables(void)
+static void seabios_acpi_build_tables(void *addr, uint32_t length)
 {
     uint32_t rsdp = (uint32_t)scratch_alloc(sizeof(struct acpi_20_rsdp), 0);
     struct acpi_config config = {
-        .dsdt_anycpu = dsdt_anycpu_qemu_xen,
-        .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
+        .dsdt_anycpu = addr,
+        .dsdt_anycpu_len = length,
         .dsdt_15cpu = NULL,
         .dsdt_15cpu_len = 0,
     };
-- 
Anthony PERARD

[RFC PATCH v2 13/16] hvmloader/makefile: Compile out SeaBIOS and OVMF ROM blob

[Anthony PERARD]

Those are not used anymore by hvmloader.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/Makefile | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 0560a7b..da6491b 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -43,9 +43,7 @@ endif
 
 CIRRUSVGA_DEBUG ?= n
 
-OVMF_DIR := ../ovmf-dir
 ROMBIOS_DIR := ../rombios
-SEABIOS_DIR := ../seabios-dir
 
 ifeq ($(CONFIG_ROMBIOS),y)
 STDVGA_ROM    := ../vgabios/VGABIOS-lgpl-latest.bin
@@ -62,12 +60,6 @@ ROMS :=
 ifeq ($(CONFIG_OVMF),y)
 OBJS += ovmf.o
 CFLAGS += -DENABLE_OVMF
-ifeq ($(OVMF_PATH),)
-	OVMF_ROM := $(OVMF_DIR)/ovmf.bin
-else
-	OVMF_ROM := $(OVMF_PATH)
-endif
-ROMS += $(OVMF_ROM)
 endif
 
 ifeq ($(CONFIG_ROMBIOS),y)
@@ -80,12 +72,6 @@ endif
 ifeq ($(CONFIG_SEABIOS),y)
 OBJS += seabios.o
 CFLAGS += -DENABLE_SEABIOS
-ifeq ($(SEABIOS_PATH),)
-	SEABIOS_ROM := $(SEABIOS_DIR)/out/bios.bin
-else
-	SEABIOS_ROM := $(SEABIOS_PATH)
-endif
-ROMS += $(SEABIOS_ROM)
 endif
 
 .PHONY: all
@@ -109,18 +95,6 @@ ifneq ($(ROMBIOS_ROM),)
 	echo "#endif" >> $@.new
 endif
 
-ifneq ($(SEABIOS_ROM),)
-	echo "#ifdef ROM_INCLUDE_SEABIOS" >> $@.new
-	sh ./mkhex seabios $(SEABIOS_ROM) >> $@.new
-	echo "#endif" >> $@.new
-endif
-
-ifneq ($(OVMF_ROM),)
-	echo "#ifdef ROM_INCLUDE_OVMF" >> $@.new
-	sh ./mkhex ovmf $(OVMF_ROM) >> $@.new
-	echo "#endif" >> $@.new	
-endif 
-
 ifneq ($(STDVGA_ROM),)
 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
 	sh ./mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
-- 
Anthony PERARD

[RFC PATCH v2 14/16] hvmloader: Always build-in SeaBIOS and OVMF loader

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/Makefile    | 6 ------
 tools/firmware/hvmloader/hvmloader.c | 4 ----
 2 files changed, 10 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index da6491b..9921781 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -57,10 +57,7 @@ endif
 
 ROMS := 
 
-ifeq ($(CONFIG_OVMF),y)
 OBJS += ovmf.o
-CFLAGS += -DENABLE_OVMF
-endif
 
 ifeq ($(CONFIG_ROMBIOS),y)
 OBJS += optionroms.o 32bitbios_support.o rombios.o
@@ -69,10 +66,7 @@ ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
 ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROMS)
 endif
 
-ifeq ($(CONFIG_SEABIOS),y)
 OBJS += seabios.o
-CFLAGS += -DENABLE_SEABIOS
-endif
 
 .PHONY: all
 all: subdirs-all
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 9e5b9d4..4a1872e 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -205,12 +205,8 @@ struct bios_info {
 #ifdef ENABLE_ROMBIOS
     { "rombios", &rombios_config, },
 #endif
-#ifdef ENABLE_SEABIOS
     { "seabios", &seabios_config, },
-#endif
-#ifdef ENABLE_OVMF
     { "ovmf", &ovmf_config, },
-#endif
     { NULL, NULL }
 };
 
-- 
Anthony PERARD

[RFC PATCH v2 15/16] hvmloader: Compile out the qemu-xen ACPI tables

[Anthony PERARD]

It should now be loaded by libxl.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/firmware/hvmloader/acpi/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/acpi/Makefile b/tools/firmware/hvmloader/acpi/Makefile
index 3d8dd21..e444c8c 100644
--- a/tools/firmware/hvmloader/acpi/Makefile
+++ b/tools/firmware/hvmloader/acpi/Makefile
@@ -17,13 +17,13 @@
 XEN_ROOT = $(CURDIR)/../../../..
 include $(XEN_ROOT)/tools/firmware/Rules.mk
 
-C_SRC = build.c dsdt_anycpu.c dsdt_15cpu.c static_tables.c dsdt_anycpu_qemu_xen.c
+C_SRC = build.c dsdt_anycpu.c dsdt_15cpu.c static_tables.c
 OBJS  = $(patsubst %.c,%.o,$(C_SRC))
 
 CFLAGS += $(CFLAGS_xeninclude)
 
 vpath iasl $(PATH)
-all: acpi.a
+all: acpi.a dsdt_anycpu_qemu_xen.aml
 
 ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h: %.h: %.asl iasl
 	iasl -vs -p $* -tc $<
@@ -47,6 +47,8 @@ $(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
 	sed -e 's/AmlCode/$*/g' $*.hex >$@
 	echo "int $*_len=sizeof($*);" >>$@
 	rm -f $*.hex
+dsdt_anycpu_qemu_xen.aml: %.aml: iasl %.asl
+	iasl -vs -p $* $*.asl
 
 iasl:
 	@echo
-- 
Anthony PERARD

[RFC PATCH v2 16/16] hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...

[Anthony PERARD]

... to compile SeaBIOS and OVMF. Only depends on CONFIG_*.

If --with-system-* configure option is used, then set *_CONFIG=n to not
compile SEABIOS and OVMF.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
Please, run ./autogen.sh on this patch.
---
 tools/configure.ac      | 6 ++++--
 tools/firmware/Makefile | 8 --------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/configure.ac b/tools/configure.ac
index 6929006..0cff3fc 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -206,12 +206,13 @@ AC_ARG_WITH([system-seabios],
     AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@],
        [Use system supplied seabios PATH instead of building and installing
         our own version]),[
+    # Disable compilation of SeaBIOS.
+    seabios=n
     case $withval in
         no) seabios_path= ;;
         *)  seabios_path=$withval ;;
     esac
 ],[])
-AC_SUBST(seabios_path)
 AC_DEFINE_UNQUOTED([SEABIOS_PATH],
                    ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
                    [SeaBIOS path])
@@ -220,12 +221,13 @@ AC_ARG_WITH([system-ovmf],
     AS_HELP_STRING([--with-system-ovmf@<:@=PATH@:>@],
        [Use system supplied OVMF PATH instead of building and installing
         our own version]),[
+    # Disable compilation of OVMF.
+    ovmf=n
     case $withval in
         no) ovmf_path= ;;
         *)  ovmf_path=$withval ;;
     esac
 ],[])
-AC_SUBST(ovmf_path)
 AC_DEFINE_UNQUOTED([OVMF_PATH],
                    ["${ovmf_path:-$XENFIRMWAREDIR/ovmf.bin}"],
                    [OVMF path])
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 9c63991..d9e525e 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,12 +6,8 @@ TARGET      := hvmloader/hvmloader
 INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
 
 SUBDIRS-y :=
-ifeq ($(OVMF_PATH),)
 SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
-endif
-ifeq ($(SEABIOS_PATH),)
 SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
-endif
 SUBDIRS-$(CONFIG_ROMBIOS) += rombios
 SUBDIRS-$(CONFIG_ROMBIOS) += vgabios
 SUBDIRS-$(CONFIG_ROMBIOS) += etherboot
@@ -50,15 +46,11 @@ install: all
 	[ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
 	[ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
 ifeq ($(CONFIG_SEABIOS),y)
-ifeq ($(SEABIOS_PATH),)
 	[ ! -e $(SEABIOS_ROM) ] || $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
 endif
-endif
 ifeq ($(CONFIG_OVMF),y)
-ifeq ($(OVMF_PATH),)
 	[ ! -e $(OVMF_ROM) ] || $(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
 endif
-endif
 	[ ! -e $(ACPI_TABLE_QEMU_PC_I440FX) ] || $(INSTALL_DATA) $(ACPI_TABLE_QEMU_PC_I440FX) $(INST_DIR)
 
 .PHONY: clean
-- 
Anthony PERARD

Re: [RFC PATCH v2 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> Hi all,
> 
> I've start to look at loading the BIOS and the ACPI tables via the
> toolstack instead of having them embedded in the hvmloader binary. After
> this patch series, hvmloader compilation would be indenpendant from
> SeaBIOS
> and OVMF compilation.
> 
> Compare to the V1, this is now done via the struct hvm_start_info from
> Roger's patch series "Introduce HVM without dm and new boot ABI".

Just to be clear, does this therefore requires the rest of (or some more
of) Roger's series to be applied or just the initial dozen patches which
are already in?

> Here is a general view of the few step to load the BIOS:
> - libxl load the BIOS blob into it's memory and add it to struct
>   xc_hvm_build_args.bios_module
> - libxc load the blob into the guest memory and fill the struct
>   hvm_start_info, with a cmdline which would contain the order in which the
>   modules (bios, acpi_table) are loaded into the modlist.
> - hvmloader read the addresses from the hvm_start_info, find out which
>   module are which by reading the cmdline and copy the blob to the right
>   place.

Hrm, it's a shame that the mod list doesn't contain this information,
laundering it via a string cmdline seems a bit sub optimal. I haven't
looked yet but my intuition is that this will involve hvmloader doing some
string parsing, which I'm not keen on.

Is the modlist a stable API (yet?) or can we extend it if we want?

Failing that perhaps hvmloader and libx? could collude such that the first
module is always some data structure (a private interface between hvmloader
and the tools) which describes the contents of the remaining modules?

> Right now, this patch series would only work for SeaBIOS and OVMF. I have
> not look into what to do about qemu-xen-traditionnal and rombios.

FWIW I think it would be fine to leave those legacy components using the
existing mechanisms. No one in their right mind is going to want to use a
system supplied version of rambios... Or if they do then I think it is up
to them to patch it. (Unless actually doing this makes your life easier
somehow WRT your actual goal)

> 
> A git tree can be found here:
> git://xenbits.xen.org/people/aperard/xen-unstable.git
> tag: hvmloader-with-separated-bios-v2
> 
> Regards,
> 
> Anthony PERARD (16):
>   hvmloader: Fix scratch_alloc to avoid overlaps
>   libxc: Load BIOS and ACPI table into guest memory
>   configure: #define SEABIOS_PATH and OVMF_PATH
>   firmware/makefile: install BIOS and ACPI blob ...
>   libxl: Load guest BIOS from file
>   libxl: Load guest ACPI table from file
>   hvmloader: Grab the hvmlite info page and parse the cmdline
>   hvmloader: Locate the BIOS blob
>   hvmloader: Load SeaBIOS from hvm_start_info modules ...
>   hvmloader: Load OVMF from modules
>   hvmloader: No BIOS ROM image allowed to be compiled in
>   hvmloader: Load ACPI tables from hvm_start_info module
>   hvmloader/makefile: Compile out SeaBIOS and OVMF ROM blob
>   hvmloader: Always build-in SeaBIOS and OVMF loader
>   hvmloader: Compile out the qemu-xen ACPI tables
>   hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...
> 
>  .gitignore                             |   1 +
>  tools/configure.ac                     |  12 +++-
>  tools/firmware/Makefile                |  15 ++--
>  tools/firmware/hvmloader/Makefile      |  32 ---------
>  tools/firmware/hvmloader/acpi/Makefile |   8 ++-
>  tools/firmware/hvmloader/config.h      |  11 +--
>  tools/firmware/hvmloader/hvmloader.c   | 127
> ++++++++++++++++++++++++++++-----
>  tools/firmware/hvmloader/ovmf.c        |  34 ++++-----
>  tools/firmware/hvmloader/seabios.c     |  33 ++++-----
>  tools/firmware/hvmloader/util.c        |   2 +-
>  tools/libxc/include/xc_dom.h           |   4 ++
>  tools/libxc/xc_dom_hvmloader.c         |  44 +++++++++---
>  tools/libxc/xc_dom_x86.c               |  90 +++++++++++++++++++++++
>  tools/libxl/libxl_dom.c                |  83 +++++++++++++++++++++
>  tools/libxl/libxl_types.idl            |   2 +
>  tools/libxl/xl_cmdimpl.c               |  14 +++-
>  16 files changed, 394 insertions(+), 118 deletions(-)
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:

Please remember to CC the relevant maintainers.

> scratch_alloc() set scratch_start to the last byte of the current
> allocation.  The value of scratch_start is then reused as is (if it is
> already aligned) in the next allocation.  This result in a potential reuse
> of the last byte of the previous allocation.

This will, in one obscure case, now result in the BUG_ON triggering when it
wouldn't before. Namely in the case where e was previously MAX_INT and is
now 0.

I'm not sure we care, since the end result previously would be that the
next scratch allocation came from address 0, which seems unlikely to end
well. So the only case we might actually care would be if the allocation
triggering the BUG_ON was the last one.

I think we probably don't care about this obscure corner case, especially
given that the allocations are starting from 0x00010000 and using enough
scratch to wrap seems like an indication the world has gone wrong anyway.

Maybe you want to discuss some of this in an updated commit message.

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(NB: hvmloader is formally maintained by the X86 ARCHITECTURE maintainers,
not the tools ones)

> ---
>  tools/firmware/hvmloader/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/util.c
> b/tools/firmware/hvmloader/util.c
> index d779fd7..42e8af4 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
>          align = 16;
>  
>      s = (scratch_start + align - 1) & ~(align - 1);
> -    e = s + size - 1;
> +    e = s + size;
>  
>      BUG_ON(e < s);
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 02/16] libxc: Load BIOS and ACPI table into guest memory

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:

At first it seems like not loading the ACPI table already was an unnoticed
functional regression from the switch to HVM using the regular PV domain
builder. But then looking at that code I see there is an acpi_module field
which Roger added to the same xc_dom_image struct and appears to have added
code to support.

So I guess I am confused about how what you are adding here differs from
that, which at the least requires discussion in the commit message, but it
seems like either one or the other is misleadingly named now or they should
somehow be combined.

Ian.

> ... and prepare a cmdline for hvmloader with the order of the modules.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxc/include/xc_dom.h   |  4 ++
>  tools/libxc/xc_dom_hvmloader.c | 44 +++++++++++++++++----
>  tools/libxc/xc_dom_x86.c       | 90
> ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 4939f76..c7003a4 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -203,6 +203,10 @@ struct xc_dom_image {
>  
>      /* Extra SMBIOS structures passed to HVMLOADER */
>      struct xc_hvm_firmware_module smbios_module;
> +
> +    /* BIOS as module */
> +    struct xc_hvm_firmware_module bios_module;
> +    struct xc_hvm_firmware_module acpi_table_module;
>  };
>  
>  /* --- pluggable kernel loader ------------------------------------- */
> diff --git a/tools/libxc/xc_dom_hvmloader.c
> b/tools/libxc/xc_dom_hvmloader.c
> index 79a3b99..3987ed8 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -129,6 +129,18 @@ static elf_errorstatus
> xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
>      return rc;
>  }
>  
> +static uint64_t module_init(struct xc_hvm_firmware_module *module,
> +                            uint64_t mstart)
> +{
> +#define MODULE_ALIGN 1UL << 7
> +#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> +    if ( module->length != 0 ) {
> +        module->guest_addr_out = mstart;
> +        return MKALIGN(module->length, MODULE_ALIGN);
> +    } else
> +        return 0;
> +}
> +
>  static int modules_init(struct xc_dom_image *dom,
>                          uint64_t vend, struct elf_binary *elf,
>                          uint64_t *mstart_out, uint64_t *mend_out)
> @@ -136,33 +148,47 @@ static int modules_init(struct xc_dom_image *dom,
>  #define MODULE_ALIGN 1UL << 7
>  #define MB_ALIGN     1UL << 20
>  #define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> -    uint64_t total_len = 0, offset1 = 0;
> +    uint64_t total_len = 0, offset1 = 0, offset0;
> +    uint64_t mstart;
>  
> -    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0
> )
> -        return 0;
> +    /* Want to place the modules 1Mb+change behind the loader image. */
> +    mstart = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
>  
>      /* Find the total length for the firmware modules with a reasonable
> large
>       * alignment size to align each the modules.
>       */
> -    total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> +    total_len += module_init(&dom->bios_module, mstart + total_len);
> +    total_len += module_init(&dom->acpi_table_module, mstart +
> total_len);
> +    offset0 = total_len;
> +    total_len += MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
>      offset1 = total_len;
>      total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
>  
> -    /* Want to place the modules 1Mb+change behind the loader image. */
> -    *mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> +    if ( total_len == 0 )
> +        return 0;
> +
> +    *mstart_out = mstart;
>      *mend_out = *mstart_out + total_len;
>  
>      if ( *mend_out > vend )
>          return -1;
>  
>      if ( dom->acpi_module.length != 0 )
> -        dom->acpi_module.guest_addr_out = *mstart_out;
> +        dom->acpi_module.guest_addr_out = *mstart_out + offset0;
>      if ( dom->smbios_module.length != 0 )
>          dom->smbios_module.guest_addr_out = *mstart_out + offset1;
>  
>      return 0;
>  }
>  
> +static void loadmodule(struct xc_hvm_firmware_module *module,
> +                       uint8_t *dest, uint64_t mstart)
> +{
> +    if ( module->length != 0 )
> +        memcpy(dest + (module->guest_addr_out - mstart),
> +               module->data, module->length);
> +}
> +
>  static int loadmodules(struct xc_dom_image *dom,
>                         uint64_t mstart, uint64_t mend,
>                         uint32_t domid)
> @@ -201,9 +227,11 @@ static int loadmodules(struct xc_dom_image *dom,
>      memset(dest, 0, pages << PAGE_SHIFT);
>  
>      /* Load modules into range */
> +    loadmodule(&dom->bios_module, dest, mstart);
> +    loadmodule(&dom->acpi_table_module, dest, mstart);
>      if ( dom->acpi_module.length != 0 )
>      {
> -        memcpy(dest,
> +        memcpy(dest + (dom->acpi_module.guest_addr_out - mstart),
>                 dom->acpi_module.data,
>                 dom->acpi_module.length);
>      }
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index c3bb7a3..2444cc2 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -511,6 +511,27 @@ static void build_hvm_info(void *hvm_info_page,
> struct xc_dom_image *dom)
>      hvm_info->checksum = -sum;
>  }
>  
> +static void add_module_to_list(struct xc_hvm_firmware_module *module,
> +                               const char *name, char *cmdline, size_t
> n,
> +                               struct hvm_modlist_entry *modlist,
> +                               size_t modlist_size, int *module_nr)
> +{
> +    if ( module->length == 0 )
> +        return;
> +
> +    /* assert(*module_nr < modlist_size); */
> +
> +    if ( *module_nr == 0 )
> +        strcat(cmdline, "modules=");
> +    else
> +        strcat(cmdline, ",");
> +    strcat(cmdline, name);
> +
> +    modlist[*module_nr].paddr = module->guest_addr_out;
> +    modlist[*module_nr].size = module->length;
> +    (*module_nr)++;
> +}
> +
>  static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>  {
>      unsigned long i;
> @@ -627,6 +648,75 @@ static int alloc_magic_pages_hvm(struct xc_dom_image
> *dom)
>      }
>      else
>      {
> +        struct xc_dom_seg seg;
> +        struct hvm_start_info *start_info;
> +        char *cmdline;
> +        struct hvm_modlist_entry *modlist;
> +        void *start_page;
> +        size_t cmdline_size;
> +        size_t start_info_size = sizeof(*start_info);
> +
> +        char cmdline_new[MAX_GUEST_CMDLINE];
> +        int module_nr = 0;
> +
> +        struct hvm_modlist_entry modlist_building[4];
> +
> +        cmdline_new[0] = '\0';
> +        add_module_to_list(&dom->bios_module, "bios",
> +                           cmdline_new, MAX_GUEST_CMDLINE,
> +                           modlist_building,
> ARRAY_SIZE(modlist_building),
> +                           &module_nr);
> +        add_module_to_list(&dom->acpi_table_module, "acpi_table",
> +                           cmdline_new, MAX_GUEST_CMDLINE,
> +                           modlist_building,
> ARRAY_SIZE(modlist_building),
> +                           &module_nr);
> +
> +        start_info_size += sizeof(*modlist) * module_nr;
> +        cmdline_size = ROUNDUP(strlen(cmdline_new) + 1, 8);
> +        start_info_size += cmdline_size;
> +
> +        rc = xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
> +                                  start_info_size);
> +        if ( rc != 0 )
> +        {
> +            DOMPRINTF("Unable to reserve memory for the start info");
> +            goto out;
> +        }
> +
> +        start_page = xc_map_foreign_range(xch, domid, start_info_size,
> +                                          PROT_READ | PROT_WRITE,
> +                                          seg.pfn);
> +        if ( start_page == NULL )
> +        {
> +            DOMPRINTF("Unable to map HVM start info page");
> +            goto error_out;
> +        }
> +
> +        start_info = start_page;
> +        cmdline = start_page + sizeof(*start_info);
> +        modlist = start_page + sizeof(*start_info) + cmdline_size;
> +
> +        if ( cmdline_size )
> +        {
> +            strncpy(cmdline, cmdline_new, MAX_GUEST_CMDLINE);
> +            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
> +            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
> +                                ((xen_pfn_t)cmdline -
> (xen_pfn_t)start_info);
> +        }
> +
> +        start_info->nr_modules = module_nr;
> +        if ( start_info->nr_modules != 0 ) {
> +            memcpy(modlist, modlist_building, sizeof(*modlist) *
> module_nr);
> +            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
> +                                ((xen_pfn_t)modlist -
> (xen_pfn_t)start_info);
> +        }
> +
> +        start_info->magic = HVM_START_MAGIC_VALUE;
> +
> +        munmap(start_page, start_info_size);
> +
> +        dom->start_info_pfn = seg.pfn;
> +
>          /*
>           * Allocate and clear additional ioreq server pages. The default
>           * server will use the IOREQ and BUFIOREQ special pages above.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.

[Anthony PERARD]

On Tue, Nov 03, 2015 at 05:30:32PM +0000, Ian Campbell wrote:
> On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> > Hi all,
> > 
> > I've start to look at loading the BIOS and the ACPI tables via the
> > toolstack instead of having them embedded in the hvmloader binary. After
> > this patch series, hvmloader compilation would be indenpendant from
> > SeaBIOS
> > and OVMF compilation.
> > 
> > Compare to the V1, this is now done via the struct hvm_start_info from
> > Roger's patch series "Introduce HVM without dm and new boot ABI".
> 
> Just to be clear, does this therefore requires the rest of (or some more
> of) Roger's series to be applied or just the initial dozen patches which
> are already in?

The struct in introduced in this patch:
<1443800943-17668-30-git-send-email-roger.pau@citrix.com>
[PATCH v7 29/32] libxc/xen: introduce a start info structure for HVMlite guests

And is not yet applied, so yes, it does require the rest of the patch
series, I have not look at which patches in particular.

> > Here is a general view of the few step to load the BIOS:
> > - libxl load the BIOS blob into it's memory and add it to struct
> >   xc_hvm_build_args.bios_module
> > - libxc load the blob into the guest memory and fill the struct
> >   hvm_start_info, with a cmdline which would contain the order in which the
> >   modules (bios, acpi_table) are loaded into the modlist.
> > - hvmloader read the addresses from the hvm_start_info, find out which
> >   module are which by reading the cmdline and copy the blob to the right
> >   place.
> 
> Hrm, it's a shame that the mod list doesn't contain this information,
> laundering it via a string cmdline seems a bit sub optimal. I haven't
> looked yet but my intuition is that this will involve hvmloader doing some
> string parsing, which I'm not keen on.
> 
> Is the modlist a stable API (yet?) or can we extend it if we want?

I'm not sure what could be added to it. An extra string that describe the
module maybe? Or ...

> Failing that perhaps hvmloader and libx? could collude such that the first
> module is always some data structure (a private interface between hvmloader
> and the tools) which describes the contents of the remaining modules?

... or we could have the modules been allocated in the same order, on a
specific position. So BIOS always first, ACPI table always second ..., and
if one is missing or not needed, replace it by a module of size 0.

> > Right now, this patch series would only work for SeaBIOS and OVMF. I have
> > not look into what to do about qemu-xen-traditionnal and rombios.
> 
> FWIW I think it would be fine to leave those legacy components using the
> existing mechanisms. No one in their right mind is going to want to use a
> system supplied version of rambios... Or if they do then I think it is up
> to them to patch it. (Unless actually doing this makes your life easier
> somehow WRT your actual goal)

I'll think about it.

-- 
Anthony PERARD

Re: [RFC PATCH v2 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.

[Ian Campbell]

On Tue, 2015-11-03 at 17:50 +0000, Anthony PERARD wrote:
> On Tue, Nov 03, 2015 at 05:30:32PM +0000, Ian Campbell wrote:
> > On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> > > Hi all,
> > > 
> > > I've start to look at loading the BIOS and the ACPI tables via the
> > > toolstack instead of having them embedded in the hvmloader binary.
> > > After
> > > this patch series, hvmloader compilation would be indenpendant from
> > > SeaBIOS
> > > and OVMF compilation.
> > > 
> > > Compare to the V1, this is now done via the struct hvm_start_info
> > > from
> > > Roger's patch series "Introduce HVM without dm and new boot ABI".
> > 
> > Just to be clear, does this therefore requires the rest of (or some
> > more
> > of) Roger's series to be applied or just the initial dozen patches
> > which
> > are already in?
> 
> The struct in introduced in this patch:
> <1443800943-17668-30-git-send-email-roger.pau@citrix.com>
> [PATCH v7 29/32] libxc/xen: introduce a start info structure for HVMlite
> guests
> 
> And is not yet applied, so yes, it does require the rest of the patch
> series, I have not look at which patches in particular.

OK, I'll keep it in mind and not try to apply this one if it ends up ready
first.

> > > Here is a general view of the few step to load the BIOS:
> > > - libxl load the BIOS blob into it's memory and add it to struct
> > >   xc_hvm_build_args.bios_module
> > > - libxc load the blob into the guest memory and fill the struct
> > >   hvm_start_info, with a cmdline which would contain the order in
> > > which the
> > >   modules (bios, acpi_table) are loaded into the modlist.
> > > - hvmloader read the addresses from the hvm_start_info, find out
> > > which
> > >   module are which by reading the cmdline and copy the blob to the
> > > right
> > >   place.
> > 
> > Hrm, it's a shame that the mod list doesn't contain this information,
> > laundering it via a string cmdline seems a bit sub optimal. I haven't
> > looked yet but my intuition is that this will involve hvmloader doing
> > some
> > string parsing, which I'm not keen on.
> > 
> > Is the modlist a stable API (yet?) or can we extend it if we want?
> 
> I'm not sure what could be added to it. An extra string that describe the
> module maybe? Or ...

I should have CCd Roger, now done.

I think this new interface (is going to) form part of the PVH boot ABI,
which is currently not formally stabilised but it means any semantics we
choose to give it need to be considered in that light and not just in the
"internal between tools and hvmloader" one.

I'm not sure what this means for adding a type to it, but on first glance
that seems like an "internal between tools and hvmloader" thing not a
public ABI thing.

> > Failing that perhaps hvmloader and libx? could collude such that the
> > first
> > module is always some data structure (a private interface between
> > hvmloader
> > and the tools) which describes the contents of the remaining modules?
> 
> ... or we could have the modules been allocated in the same order, on a
> specific position. So BIOS always first, ACPI table always second ..., and
> if one is missing or not needed, replace it by a module of size 0.

Since this would be an "internal interface" I think we could do, although
it might be more convenient for developers trying to change this in the
future to include a little more flexibility? (e.g. co-bisection of tools
and hvmloader over a change to the list ordering)

In the end if this is an "internal interface" I suppose it doesn't matter
too much what colour we paint it to start with.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 03/16] configure: #define SEABIOS_PATH and OVMF_PATH

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> Those paths are to be used by libxl, in order to load the firmware in
> memory. If a system path is not define, then this default to the Xen
> firmware directory.

AIUI these are both existing variables which are right now exposed only to
the Makefiles and not to the C code, and this adds the latter.

That only matters because I then don't have to think about the semantics of
a "new" option, what the matching command line option behaviour is, docs
etc.

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Please, run ./autogen.sh on this patch.
> ---
>  tools/configure.ac | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 6c70040..6929006 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -212,6 +212,9 @@ AC_ARG_WITH([system-seabios],
>      esac
>  ],[])
>  AC_SUBST(seabios_path)
> +AC_DEFINE_UNQUOTED([SEABIOS_PATH],
> +                   ["${seabios_path:-$XENFIRMWAREDIR/seabios.bin}"],
> +                   [SeaBIOS path])

I thought ${foo:-bar} might not be POSIXly-correct, but it turns out it is
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html ,
so good!


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 04/16] firmware/makefile: install BIOS and ACPI blob ...

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> ... into the firmware directory, along with hvmloader.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  .gitignore                             |  1 +
>  tools/firmware/Makefile                | 15 +++++++++++++++
>  tools/firmware/hvmloader/acpi/Makefile |  2 +-
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 9ead7c4..7c7bb56 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -117,6 +117,7 @@ tools/firmware/hvmloader/acpi/mk_dsdt
>  tools/firmware/hvmloader/acpi/dsdt*.c
>  tools/firmware/hvmloader/acpi/dsdt_*.asl
>  tools/firmware/hvmloader/acpi/ssdt_*.h
> +tools/firmware/hvmloader/acpi/dsdt_anycpu_qemu_xen.aml
>  tools/firmware/hvmloader/hvmloader
>  tools/firmware/hvmloader/roms.h
>  tools/firmware/hvmloader/roms.inc
> diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
> index 6cc86ce..9c63991 100644
> --- a/tools/firmware/Makefile
> +++ b/tools/firmware/Makefile
> @@ -19,6 +19,10 @@ SUBDIRS-y += hvmloader
>  
>  LD32BIT-$(CONFIG_FreeBSD) := LD32BIT_FLAG=-melf_i386_fbsd
>  
> +SEABIOS_ROM := seabios-dir/out/bios.bin
> +OVMF_ROM := ovmf-dir/ovmf.bin
> +ACPI_TABLE_QEMU_PC_I440FX = hvmloader/acpi/dsdt_anycpu_qemu_xen.aml
> +
>  ovmf-dir:
>  	GIT=$(GIT) $(XEN_ROOT)/scripts/git-checkout.sh
> $(OVMF_UPSTREAM_URL) $(OVMF_UPSTREAM_REVISION) ovmf-dir
>  	cp ovmf-makefile ovmf-dir/Makefile;
> @@ -45,6 +49,17 @@ endif
>  install: all
>  	[ -d $(INST_DIR) ] || $(INSTALL_DIR) $(INST_DIR)
>  	[ ! -e $(TARGET) ] || $(INSTALL_DATA) $(TARGET) $(INST_DIR)
> +ifeq ($(CONFIG_SEABIOS),y)
> +ifeq ($(SEABIOS_PATH),)
> +	[ ! -e $(SEABIOS_ROM) ] || $(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin

If $(SEABIOS_ROM) doesn't exist surely that indicates something has gone
wrong prior to this?

The -e $(TARGET) you are (presumably) copying is similarly strange,
although maybe that is to do with it not being inside an ifeq like the
CONFIG_SEABIOS which protects your new one.

IOW I think you can drop the -e check from both, the ifeq is clearer and
less prone to carrying on in the face of other errors AFAICT.

> +endif
> +endif
> +ifeq ($(CONFIG_OVMF),y)
> +ifeq ($(OVMF_PATH),)
> +	[ ! -e $(OVMF_ROM) ] || $(INSTALL_DATA) $(OVMF_ROM)
> $(INST_DIR)/ovmf.bin
> +endif
> +endif
> +> 	> [ ! -e $(ACPI_TABLE_QEMU_PC_I440FX) ] || $(INSTALL_DATA) $(ACPI_TABLE_QEMU_PC_I440FX) $(INST_DIR)
>  
>  .PHONY: clean
>  clean: subdirs-clean
> diff --git a/tools/firmware/hvmloader/acpi/Makefile
> b/tools/firmware/hvmloader/acpi/Makefile
> index d3e882a..3d8dd21 100644
> --- a/tools/firmware/hvmloader/acpi/Makefile
> +++ b/tools/firmware/hvmloader/acpi/Makefile
> @@ -46,7 +46,7 @@ $(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
>  	iasl -vs -p $* -tc $*.asl
>  	sed -e 's/AmlCode/$*/g' $*.hex >$@
>  	echo "int $*_len=sizeof($*);" >>$@
> -	rm -f $*.aml $*.hex
> +	rm -f $*.hex

Is $*.aml an incidental output of iasl? The iasl manpage is not terribly
informative on the issue, but it says that -tc will create C output in a
.hex fix. I suppose the .aml is implied.
        ./mk_dsdt --debug=$(debug) --maxcpu $*  >> $@

I think (but please check, there may be some weird subtlety with this sort
of rule) that it would be correct to change the rule here from:
    $(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
into:
    $(filter dsdt_%.c,$(C_SRC)): %.c %.aml: iasl %.asl

(my actual preference would be to invoke iasl twice, once for asl->aml and
then again for aml->hex, but that doesn't seem possible).

Oh, maybe the need for .hex file will go away later in this series and so
you can adjust this to only talk about the .aml? In which case after
mentioning that in the commit message you could ignore all the above and
fix up the rules properly at that point

Do you not need to also add this newly left lying around file to
.gitignore.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 07/16] hvmloader: Grab the hvmlite info page and parse the cmdline

[Andrew Cooper]

On 26/10/15 16:03, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/hvmloader.c | 69 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)

As part of using the DMLite infrastructure, hvmloader should gain
appropriate elfnotes.  This will avoid the need to duplicate the dmlite
codepath in libxc just for hvmloader.

>
> diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> index 716d03c..9df12ac 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -62,7 +62,7 @@ asm (
>      "    mov  %ax,%ss                \n"

mov %ebx, start_info

and have

static const struct hvm_start_info *hvmlite_start_info;

somewhere other than the main() stack.  However, I would avoid naming it
"hvmlite".

>      /* Initialise all 32-bit GPRs to zero. */
>      "    xor  %eax,%eax              \n"
> -    "    xor  %ebx,%ebx              \n"
> +    /* Keep ebx, for HVMLite start info */
>      "    xor  %ecx,%ecx              \n"
>      "    xor  %edx,%edx              \n"
>      "    xor  %esp,%esp              \n"
> @@ -249,15 +249,82 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +static const char *module_list_order = NULL;
> +void cmdline_parser(const char *the_cmdline)
> +{
> +    char cmdline[MAX_GUEST_CMDLINE];
> +    char *p, *q;
> +    char *optval;
> +
> +    strncpy(cmdline, the_cmdline, sizeof (cmdline));
> +    cmdline[MAX_GUEST_CMDLINE-1] = '\0';

Why do you need to copy the command line?  It is already in writable
hvmloader memory.

You also need to check for NULL, being the indication that no command
line has been provided.

~Andrew

> +
> +    for ( p = cmdline; p < (cmdline + MAX_GUEST_CMDLINE) && *p; p++ )
> +    {
> +        if ( *p == ' ' )
> +            continue;
> +
> +        /* search for the end of the parameter name */
> +        for ( q = p; *q && *q != '=' && *q != ' '; q++ )
> +            ;
> +
> +        /* search for the end of the optional paremeter value */
> +        if ( *q == '=' )
> +        {
> +            optval = q+1;
> +            if (*optval == '\0' || *optval == ' ') {
> +                optval = NULL;
> +            }
> +        } else
> +            optval = NULL;
> +        *q = '\0';
> +
> +        if ( optval )
> +        {
> +            for ( q = optval; *q && *q != ' '; q++ )
> +                ;
> +            *q = '\0';
> +        }
> +
> +        /* compare known parameters */
> +        if ( !strcmp(p, "modules") )
> +        {
> +            printf("  cmdline: found '%s', with val '%s'\n", p, optval);
> +            if ( optval )
> +            {
> +                unsigned size = strlen(optval) + 1;
> +                char *tmp = scratch_alloc(size, 0);
> +                strncpy(tmp, optval, size);
> +                module_list_order = tmp;
> +            }
> +        } else {
> +            printf("  Unknown cmdline option '%s'", p);
> +            if ( optval )
> +                printf(" with val '%s'\n", optval);
> +            else
> +                printf("\n");
> +        }
> +
> +        p = q;
> +    }
> +}
> +
>  int main(void)
>  {
>      const struct bios_config *bios;
>      int acpi_enabled;
> +    const struct hvm_start_info *hvmlite_start_info;
> +
> +    /* Load hvmlite start info pointer from ebx. */
> +    asm volatile ( "mov %%ebx,%0" : "=r" (hvmlite_start_info) );
>  
>      /* Initialise hypercall stubs with RET, rendering them no-ops. */
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
>  
>      printf("HVM Loader\n");
> +    BUG_ON(hvmlite_start_info->magic != HVM_START_MAGIC_VALUE);
> +    printf("cmdline: %s\n", (char*)hvmlite_start_info->cmdline_paddr);
> +    cmdline_parser((char*)hvmlite_start_info->cmdline_paddr);
>  
>      init_hypercalls();
>  

Re: [RFC PATCH v2 05/16] libxl: Load guest BIOS from file

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> The path to the BIOS blob can be override by the xl's bios_override
> option,
> or provided by u.hvm.bios_filename in the domain_build_info struct by
> other
> libxl user.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_dom.c     | 66
> +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    | 11 +++++---
>  3 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index b40d744..27a0021 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -858,6 +858,42 @@ err:
>      return ret;
>  }
>  
> +static const char *seabios_path(libxl__gc *gc)
> +{
> +    return SEABIOS_PATH;
> +}
> +
> +static const char *ovmf_path(libxl__gc *gc)
> +{
> +    return OVMF_PATH;
> +}

Can you put these in libxl_paths.c (for consistency) please.

> +
> +static int libxl__load_hvm_firmware_module(libxl__gc *gc,
> +                                           const char *filename,
> +                                           const char *what,
> +                                           struct xc_hvm_firmware_module
> *m)
> +{
> +    int datalen = 0;
> +    void *data = NULL;
> +    int e;
> +
> +    LOG(DEBUG, "Loading %s: %s", what, filename);
> +    e = libxl_read_file_contents(CTX, filename, &data, &datalen);
> +    if (e) {
> +        LOGEV(ERROR, e, "failed to read %s file %s",

"e" here should be an errno val, not a libxl ERROR_*. So you need to print
manually via the format string using either LOG or LOGE (if
libxl_read_file_contents also sets errno, probably not).


> +              what,
> +              filename);
> +        return ERROR_FAIL;
> +    }
> +    libxl__ptr_add(gc, data);
> +    if (datalen) {
> +        /* Only accept non-empty files */

Error or logging otherwise? Silently ignoring seems likely to surprise
someone eventually.

> +        m->data = data;
> +        m->length = (uint32_t)datalen;


I hope that cast isn't really necessary. 

> +    }
> +    return 0;
> +}
> +
>  static int libxl__domain_firmware(libxl__gc *gc,
>                                    libxl_domain_build_info *info,
>                                    struct xc_dom_image *dom)
> @@ -910,6 +946,36 @@ static int libxl__domain_firmware(libxl__gc *gc,
>          goto out;
>      }
>  
> +    if (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
> +        const char *bios_filename;
> +        // Look for BIOS and load it

Proper comment style please.

> +        if (info->u.hvm.bios_filename) {
> +            bios_filename = info->u.hvm.bios_filename;
> +        } else {
> +            switch (info->u.hvm.bios)
> +            {
> +            case LIBXL_BIOS_TYPE_ROMBIOS:
> +                bios_filename = libxl__abs_path(gc, "rombios.bin",
> +                                                libxl__xenfirmwaredir_path());

The cover letter implied that rombios was still built in, this suggests
not?

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 082fed8..a3fbcab 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -468,6 +468,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",         string),
>                                         ("bios",             libxl_bios_type),
> +                                       ("bios_filename",    string),

Such new additions require a #define LIBXL_HAVE_* in libxl.h so
applications know they can use it.



> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 840d73f..27d7c25 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1500,12 +1500,17 @@ static void parse_config_data(const char *config_source,
>  
>          xlu_cfg_replace_string (config, "firmware_override",
>                                  &b_info->u.hvm.firmware, 0);
> -        if (!xlu_cfg_get_string(config, "bios", &buf, 0) &&
> -            libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {
> +        xlu_cfg_replace_string (config, "bios_override",
> +                                &b_info->u.hvm.bios_filename, 0);
> +        if (!xlu_cfg_get_string(config, "bios", &buf, 0)) {
> +            if (libxl_bios_type_from_string(buf, &b_info->u.hvm.bios)) {

Please update the xl.cfg man page to reflect this.

>                  fprintf(stderr, "ERROR: invalid value \"%s\" for \"bios\"\n",
>                      buf);
>                  exit (1);
> -        }
> +            }
> +        } else if (b_info->u.hvm.bios_filename)
> +            fprintf(stderr, "WARNING: "
> +                    "bios_override given without specific bios name\n");

What I'm about to say is probably not a good idea, but:

Given that we might keep rombios in-hvmloader (hence bios=rombios
bios_override!=NULL could be considered an error) can we get away with
saying that any use of bios_filename implies some sort of common non-
specific BIOS version?

Eyeballing the diff between tools/firmware/hvmloader/{seabios,ovmf}.c I
think the answer is already no and in any case doing this may tie our hands
in the future with some fancy new thing.

Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 06/16] libxl: Load guest ACPI table from file

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> The path to the ACPI tables blob can be override by xl's option

"overridden"

> acpi_table_override or by acpi_tables_filename in the domain_build_info
> struct for libxl user.


This needs the same libxl.h #define and xl.cfg update I mentioned before.

The code, docs and commit message all need further consideration of the
interactions with the existing acpi_firmware option which exists in libxl
and is exposed in xl. It allows you to specify some extra tables which are
merged (by hvmloader) into the base ones.

The naming is a bit unfortunate but we are now stuck with those semantics
for the existing option I think. If we can distinguish partial from full
tables in the tools reusing the name and doing so might be the best
approach.

If we can't tell the difference then the new option needs some suitable
name such that it is clear it is the full or base table or something.

Ian.

Re: [RFC PATCH v2 07/16] hvmloader: Grab the hvmlite info page and parse the cmdline

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/hvmloader.c | 69
> +++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/hvmloader.c
> b/tools/firmware/hvmloader/hvmloader.c
> index 716d03c..9df12ac 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -62,7 +62,7 @@ asm (
>      "    mov  %ax,%ss                \n"
>      /* Initialise all 32-bit GPRs to zero. */
>      "    xor  %eax,%eax              \n"
> -    "    xor  %ebx,%ebx              \n"
> +    /* Keep ebx, for HVMLite start info */

Isn't this now an ABI between the tools and HVM loader, which is embedded
in the more formal PVH/HVMLite startup protocol? In which case hopefully
there is somewhere it can be documented. Is a HVMlite/PVH guest allowed to
assume anything about %ebx, those sorts of questions need to be considered.

>      "    xor  %ecx,%ecx              \n"
>      "    xor  %edx,%edx              \n"
>      "    xor  %esp,%esp              \n"
> @@ -249,15 +249,82 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +static const char *module_list_order = NULL;
> +void cmdline_parser(const char *the_cmdline)

I'll leave this until we've decided if this approach is the one we want.

>  int main(void)
>  {
>      const struct bios_config *bios;
>      int acpi_enabled;
> +    const struct hvm_start_info *hvmlite_start_info;
> +
> +    /* Load hvmlite start info pointer from ebx. */
> +    asm volatile ( "mov %%ebx,%0" : "=r" (hvmlite_start_info) );

The stub which calls main should perhaps arrange for this to be in a
register which ends up being a parameter to this struct. I think otherwise
nothing ensures that the function prolog hasn't clobbered %ebx already.

>  
>      /* Initialise hypercall stubs with RET, rendering them no-ops. */
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */,
> PAGE_SIZE);
>  
>      printf("HVM Loader\n");
> +    BUG_ON(hvmlite_start_info->magic != HVM_START_MAGIC_VALUE);
> +    printf("cmdline: %s\n", (char*)hvmlite_start_info->cmdline_paddr);
> +    cmdline_parser((char*)hvmlite_start_info->cmdline_paddr);
>  
>      init_hypercalls();
>  
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 08/16] hvmloader: Locate the BIOS blob

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> The BIOS can be found in one of the entry of the modlist of the
> hvm_start_info struct. The order of the modules are define by the command
> line option "modules".
> 
> The found BIOS blob is not loaded by this patch, but only passed as
> argument to bios_load() function. It is going to be used by the next few
> patches.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/config.h    |  2 +-
>  tools/firmware/hvmloader/hvmloader.c | 34 +++++++++++++++++++++++++++++++++-
>  tools/firmware/hvmloader/ovmf.c      |  3 ++-

But not seabios.c or rombios.c? Doesn't this break rombios since it has
bios_load hook? (Seabios doesn't it seems).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 09/16] hvmloader: Load SeaBIOS from hvm_start_info modules ...

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> ... and do not include the SeaBIOS ROM into hvmloader anymore.

... but don't yet stop including it in rom.inc (I suppose that comes later)

Can we add a new hook to chose an address for the bios which the common
code which handles the no-bios_load-hook case could use and which could be
propagated to setup_e280 instead of using a global?

seabios_config isn't a const, so you could even just set .bios_address
dynamically?

> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/seabios.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/seabios.c
> b/tools/firmware/hvmloader/seabios.c
> index c6b3d9f..766bd6b 100644
> --- a/tools/firmware/hvmloader/seabios.c
> +++ b/tools/firmware/hvmloader/seabios.c
> @@ -27,9 +27,6 @@
>  #include "smbios_types.h"
>  #include "acpi/acpi2_0.h"
>  
> -#define ROM_INCLUDE_SEABIOS
> -#include "roms.inc"
> -
>  extern unsigned char dsdt_anycpu_qemu_xen[];
>  extern int dsdt_anycpu_qemu_xen_len;
>  
> @@ -121,6 +118,7 @@ static void seabios_create_pir_tables(void)
>      add_table(create_pir_tables());
>  }
>  
> +unsigned int seabios_bios_address = 0;

If it really has to remain should at least be static.

>  static void seabios_setup_e820(void)
>  {
>      struct seabios_info *info = (void *)BIOS_INFO_PHYSICAL_ADDRESS;
> @@ -128,21 +126,27 @@ static void seabios_setup_e820(void)
>      info->e820 = (uint32_t)e820;
>  
>      /* SeaBIOS reserves memory in e820 as necessary so no low
> reservation. */
> -    info->e820_nr = build_e820_table(e820, 0, 0x100000-sizeof(seabios));
> +    BUG_ON(seabios_bios_address == 0);
> +    info->e820_nr = build_e820_table(e820, 0, seabios_bios_address);
>      dump_e820_table(e820, info->e820_nr);
>  }
>  
> -struct bios_config seabios_config = {
> -    .name = "SeaBIOS",
> +static void seabios_load(const struct bios_config *bios,
> +                         void *bios_addr, uint32_t bios_length)
> +{
> +    unsigned int bios_dest = 0x100000 - bios_length;
> +    seabios_bios_address = 0x100000 - bios_length;
>  
> -    .image = seabios,
> -    .image_size = sizeof(seabios),
> +    BUG_ON(bios_dest + bios_length > HVMLOADER_PHYSICAL_ADDRESS);
> +    memcpy((void *)bios_dest, bios_addr, bios_length);
> +}
>  
> -    .bios_address = 0x100000 - sizeof(seabios),
> +struct bios_config seabios_config = {
> +    .name = "SeaBIOS",
>  
>      .load_roms = NULL,
>  
> -    .bios_load = NULL,
> +    .bios_load = seabios_load,
>  
>      .bios_info_setup = seabios_setup_bios_info,
>      .bios_info_finish = seabios_finish_bios_info,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 10/16] hvmloader: Load OVMF from modules

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> ... and do not include the OVMF ROM into hvmloader anymore.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/ovmf.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/ovmf.c
> b/tools/firmware/hvmloader/ovmf.c
> index 2be9752..3c0ec91 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -34,17 +34,9 @@
>  #include <xen/hvm/ioreq.h>
>  #include <xen/memory.h>
>  
> -#define ROM_INCLUDE_OVMF
> -#include "roms.inc"
> -
> -#define OVMF_SIZE               (sizeof(ovmf))
> -#define OVMF_MAXOFFSET          0x000FFFFFULL
> -#define OVMF_BEGIN              (0x100000000ULL - ((OVMF_SIZE +
> OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
> -#define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
>  #define LOWCHUNK_BEGIN          0x000F0000
>  #define LOWCHUNK_SIZE           0x00010000
>  #define LOWCHUNK_MAXOFFSET      0x0000FFFF
> -#define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
>  #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
>  
>  extern unsigned char dsdt_anycpu[];
> @@ -96,12 +88,16 @@ static void ovmf_finish_bios_info(void)
>  static void ovmf_load(const struct bios_config *config,
>                        void *bios_addr, uint32_t bios_length)
>  {
> +#define OVMF_MAXOFFSET  0x000FFFFFULL
> +#define OVMF_BEGIN      (0x100000000ULL - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
> +#define OVMF_END        (OVMF_BEGIN + bios_length)

Would be far better converted to proper (possibly const) local variables
IMHO. But if you don't want to do that (and can give a good reason not to)
then you should at least #undef them when the things they refer to go out
of scope.

>      xen_pfn_t mfn;
>      uint64_t addr = OVMF_BEGIN;
> +    unsigned int dest = OVMF_BEGIN;

Two things both assigned OVMF_BEGIN, but with very differently sized types.
One of them is suspicious (IMHO the new one)
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 11/16] hvmloader: No BIOS ROM image allowed to be compiled in

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:

This might invalidate some of my earlier comments about reusing bios_addr,
in which case that other commit ought to make reference to the eventual
goal.

But...
> @@ -432,9 +424,9 @@ int main(void)
>      if ( SCRATCH_PHYSICAL_ADDRESS != scratch_start )
>          printf(" %05x-%05lx: Scratch space\n",
>                 SCRATCH_PHYSICAL_ADDRESS, scratch_start);
> -    printf(" %05x-%05x: Main BIOS\n",
> -           bios->bios_address,
> -           bios->bios_address + bios->image_size - 1);
> +    /* printf(" %05x-%05x: Main BIOS\n", */
> +           /* bios->bios_address, */
> +           /* bios->bios_address + bios->image_size - 1); */

A way should be found to keep this useful debug output, which may involve
having the per-bios code propagate what they have done, or logging it
themselves etc.

Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC PATCH v2 12/16] hvmloader: Load ACPI tables from hvm_start_info module

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> ... and use it with both SeaBIOS and OVMF.
> 
> This may change the behavior of guest using OVMF as the ACPI tables
> currently loaded are the one for qemu-xen-traditionnal.

"traditional".

I'm a bit surprised using the trad ones works, given OVMF is always
associated with qemu-xen.

I'm inclined to suggest that this bugfix should be split out and committed
sooner rather than later, independently of this series. Apart from cleanly
separating out a potentially interesting change it also gives us a longer
runway for testing it doesn't knacker anything.

Re: [RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Jan Beulich]

>>> On 26.10.15 at 17:03, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
>          align = 16;
>  
>      s = (scratch_start + align - 1) & ~(align - 1);
> -    e = s + size - 1;
> +    e = s + size;

This further increases the delta to the actually quite similar
mem_alloc(). I'd prefer the two to remain in sync as much as
possible, and hence either the other one to also be adjusted
or this one to be fixed the other way around (dropping the
first "- 1" in the assignment to s, but requiring changes
elsewhere too).

Jan

Re: [RFC PATCH v2 06/16] libxl: Load guest ACPI table from file

[Anthony PERARD]

On Wed, Nov 04, 2015 at 10:57:35AM +0000, Ian Campbell wrote:
> On Mon, 2015-10-26 at 16:03 +0000, Anthony PERARD wrote:
> > The path to the ACPI tables blob can be override by xl's option
> 
> "overridden"
> 
> > acpi_table_override or by acpi_tables_filename in the domain_build_info
> > struct for libxl user.
> 
> 
> This needs the same libxl.h #define and xl.cfg update I mentioned before.
> 
> The code, docs and commit message all need further consideration of the
> interactions with the existing acpi_firmware option which exists in libxl
> and is exposed in xl. It allows you to specify some extra tables which are
> merged (by hvmloader) into the base ones.
> 
> The naming is a bit unfortunate but we are now stuck with those semantics
> for the existing option I think. If we can distinguish partial from full
> tables in the tools reusing the name and doing so might be the best
> approach.
> 
> If we can't tell the difference then the new option needs some suitable
> name such that it is clear it is the full or base table or something.

We can probably tell the difference between both full and extra tables. I
think the full one should be a DSDT table, and the extra tables that can be
supplied by acpi_firmware should not be a DSDT.

So, if the AML supplied via acpi_firmware match 'DSDT' for it's signature
(the first four bytes), then it's a replacement for the full acpi tables.
Otherwise, it's probably extra tables, so we would give to hvmloader both
the default full acpi tables as well as the extra one from the user.

Would that be OK as an extention of the usage of acpi_firmware (in both
libxl and xl)? If user wants to supply both the DSDT and extra tables, it
would concatenate the extra tables to the DSDT, the DSDT tables should be
first.

-- 
Anthony PERARD