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

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

[Anthony PERARD]

Hi all,

Many changes in V3:
  no more cmdline, but use each modules' cmdline to provide a name for it.
  in libxc:
    - use xc_dom_alloc_segment() to load modules. see patch #1.
    - avoid duplication of code between PVHv2 and hvmloader for the
      initialisation of hvm_start_info.
  in xl/libxl:
    - only one new option, bios_firmware. acpi_firmware is reused, see
      patch #6.
  in hvmloader:
    - handle rombios as separate case.
  more detail in each patches.

I've 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.

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 and store a name for each module into the module cmdline.
- hvmloader read the addresses from the hvm_start_info, find out which
  module to load and copy the blob to the right place.

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

Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

Regards,

Anthony PERARD (16):
  libxc: Rework extra module initialisation
  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 hvm_start_info pointer
  hvmloader: Locate the BIOS blob
  hvmloader: Check modules whereabouts
  hvmloader: Load SeaBIOS from hvm_start_info modules
  hvmloader: Load OVMF from modules
  hvmloader: Specific bios_load function required
  hvmloader: Load ACPI tables from hvm_start_info module
  hvmloader: Compile out the qemu-xen ACPI tables
  hvmloader: Always build-in SeaBIOS and OVMF loader
  hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...

 .gitignore                                |   1 +
 tools/configure.ac                        |  12 ++-
 tools/firmware/Makefile                   |  15 +++-
 tools/firmware/hvmloader/Makefile         |  39 +--------
 tools/firmware/hvmloader/acpi/Makefile    |   9 ++-
 tools/firmware/hvmloader/config.h         |   4 +-
 tools/firmware/hvmloader/hvm_start_info.h |  32 ++++++++
 tools/firmware/hvmloader/hvmloader.c      |  85 ++++++++++++++++---
 tools/firmware/hvmloader/ovmf.c           |  39 ++++-----
 tools/firmware/hvmloader/rombios.c        |   6 +-
 tools/firmware/hvmloader/seabios.c        |  33 ++++----
 tools/firmware/hvmloader/util.h           |   2 +
 tools/libxc/include/xc_dom.h              |   4 +
 tools/libxc/xc_dom_hvmloader.c            | 128 ++++++++---------------------
 tools/libxc/xc_dom_x86.c                  | 130 ++++++++++++++++++++----------
 tools/libxl/libxl.h                       |   8 ++
 tools/libxl/libxl_dom.c                   |  83 +++++++++++++++++++
 tools/libxl/libxl_internal.h              |   2 +
 tools/libxl/libxl_paths.c                 |  10 +++
 tools/libxl/libxl_types.idl               |   1 +
 tools/libxl/xl_cmdimpl.c                  |  11 ++-
 21 files changed, 415 insertions(+), 239 deletions(-)
 create mode 100644 tools/firmware/hvmloader/hvm_start_info.h

-- 
Anthony PERARD


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

[PATCH v3 01/16] libxc: Rework extra module initialisation

[Anthony PERARD]

This patch use xc_dom_alloc_segment() to allocate the memory space for the
ACPI modules and the SMBIOS modules. This is to replace the arbitrary
placement of 1MB after the hvmloader image.

In later patches, while trying to load a firmware such as OVMF, the later
could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
process, clear the memory, before loading the modules.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
New patch in V3.
---
 tools/libxc/xc_dom_hvmloader.c | 124 +++++++++++------------------------------
 1 file changed, 31 insertions(+), 93 deletions(-)

diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 330d5e8..54e096c 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -129,98 +129,45 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
     return rc;
 }
 
-static int modules_init(struct xc_dom_image *dom,
-                        uint64_t vend, struct elf_binary *elf,
-                        uint64_t *mstart_out, uint64_t *mend_out)
+static int module_init_one(struct xc_dom_image *dom,
+                           struct xc_hvm_firmware_module *module,
+                           char *name)
 {
-#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;
+    struct xc_dom_seg seg;
+    void *dest;
 
-    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
-        return 0;
-
-    /* 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);
-    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);
-    *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;
-    if ( dom->smbios_module.length != 0 )
-        dom->smbios_module.guest_addr_out = *mstart_out + offset1;
+    if ( module->length )
+    {
+        if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
+            goto err;
+        dest = xc_dom_seg_to_ptr(dom, &seg);
+        if ( dest == NULL )
+        {
+            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
+                      __FUNCTION__);
+            goto err;
+        }
+        memcpy(dest, module->data, module->length);
+        module->guest_addr_out = seg.vstart;
+    }
 
     return 0;
+err:
+    return -1;
 }
 
-static int loadmodules(struct xc_dom_image *dom,
-                       uint64_t mstart, uint64_t mend,
-                       uint32_t domid)
+static int modules_init(struct xc_dom_image *dom)
 {
-    privcmd_mmap_entry_t *entries = NULL;
-    unsigned long pfn_start;
-    unsigned long pfn_end;
-    size_t pages;
-    uint32_t i;
-    uint8_t *dest;
-    int rc = -1;
-    xc_interface *xch = dom->xch;
-
-    if ( mstart == 0 || mend == 0 )
-        return 0;
-
-    pfn_start = (unsigned long)(mstart >> PAGE_SHIFT);
-    pfn_end = (unsigned long)((mend + PAGE_SIZE - 1) >> PAGE_SHIFT);
-    pages = pfn_end - pfn_start;
-
-    /* Map address space for module list. */
-    entries = calloc(pages, sizeof(privcmd_mmap_entry_t));
-    if ( entries == NULL )
-        goto error_out;
-
-    for ( i = 0; i < pages; i++ )
-        entries[i].mfn = (mstart >> PAGE_SHIFT) + i;
-
-    dest = xc_map_foreign_ranges(
-        xch, domid, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT,
-        entries, pages);
-    if ( dest == NULL )
-        goto error_out;
-
-    /* Zero the range so padding is clear between modules */
-    memset(dest, 0, pages << PAGE_SHIFT);
-
-    /* Load modules into range */
-    if ( dom->acpi_module.length != 0 )
-    {
-        memcpy(dest,
-               dom->acpi_module.data,
-               dom->acpi_module.length);
-    }
-    if ( dom->smbios_module.length != 0 )
-    {
-        memcpy(dest + (dom->smbios_module.guest_addr_out - mstart),
-               dom->smbios_module.data,
-               dom->smbios_module.length);
-    }
-
-    munmap(dest, pages << PAGE_SHIFT);
-    rc = 0;
+    int rc;
 
- error_out:
-    free(entries);
+    rc = module_init_one(dom, &dom->acpi_module, "acpi module");
+    if ( rc ) goto err;
+    rc = module_init_one(dom, &dom->smbios_module, "smbios module");
+    if ( rc ) goto err;
 
-    return rc;
+    return 0;
+err:
+    return -1;
 }
 
 static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
@@ -229,7 +176,6 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
     privcmd_mmap_entry_t *entries = NULL;
     size_t pages = (elf->pend - elf->pstart + PAGE_SIZE - 1) >> PAGE_SHIFT;
     elf_errorstatus rc;
-    uint64_t m_start = 0, m_end = 0;
     int i;
 
     /* Map address space for initial elf image. */
@@ -262,15 +208,7 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
 
     munmap(elf->dest_base, elf->dest_size);
 
-    rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, &m_start,
-                      &m_end);
-    if ( rc != 0 )
-    {
-        DOMPRINTF("%s: insufficient space to load modules.", __func__);
-        goto error;
-    }
-
-    rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
+    rc = modules_init(dom);
     if ( rc != 0 )
     {
         DOMPRINTF("%s: unable to load modules.", __func__);
-- 
Anthony PERARD


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

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

[Anthony PERARD]

This adds two new firmware module, bios_module and full_acpi_module. They
are loaded in the guest memory and final location is provided to hvmloader
via the hvm_start_info struct.

This patch create the hvm_start_info struct for HVM guest that have a
device model, so this is now common code with HVM guest without device
model.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V3:
- rename acpi_table_module to full_acpi_module.
- factorise module loading, using new function to load existing optinal
  module, this should not change anything
- should now use the same code to loads modules as for HVMlite VMs.
  this avoid duplication of code.
- no more generic cmdline with a list of modules, each module have its name
  in the module specific cmdline.
- scope change for common code between hvmlite and hvmloader
---
 tools/libxc/include/xc_dom.h   |   4 ++
 tools/libxc/xc_dom_hvmloader.c |   4 ++
 tools/libxc/xc_dom_x86.c       | 130 ++++++++++++++++++++++++++++-------------
 3 files changed, 96 insertions(+), 42 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6ebe946..7dc3fe7 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -209,6 +209,10 @@ struct xc_dom_image {
     /* If unset disables the setup of the IOREQ pages. */
     bool device_model;
 
+    /* BIOS and ACPI tables passed to HVMLOADER */
+    struct xc_hvm_firmware_module bios_module;
+    struct xc_hvm_firmware_module full_acpi_module;
+
     /* Extra ACPI tables passed to HVMLOADER */
     struct xc_hvm_firmware_module acpi_module;
 
diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 54e096c..2c965c9 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -160,6 +160,10 @@ static int modules_init(struct xc_dom_image *dom)
 {
     int rc;
 
+    rc = module_init_one(dom, &dom->bios_module, "bios module");
+    if ( rc ) goto err;
+    rc = module_init_one(dom, &dom->full_acpi_module, "acpi table module");
+    if ( rc ) goto err;
     rc = module_init_one(dom, &dom->acpi_module, "acpi module");
     if ( rc ) goto err;
     rc = module_init_one(dom, &dom->smbios_module, "smbios module");
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e13a4aa..2b4946c 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -69,6 +69,8 @@
 #define round_up(addr, mask)     ((addr) | (mask))
 #define round_pg_up(addr)  (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1))
 
+#define HVMLOADER_MODULE_MAX_COUNT 2
+
 struct xc_dom_params {
     unsigned levels;
     xen_vaddr_t vaddr_mask;
@@ -590,6 +592,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
     xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
     xc_interface *xch = dom->xch;
+    size_t start_info_size = sizeof(struct hvm_start_info);
 
     /* Allocate and clear special pages. */
     for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
@@ -624,8 +627,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 
     if ( !dom->device_model )
     {
-        size_t start_info_size = sizeof(struct hvm_start_info);
-
         if ( dom->cmdline )
         {
             dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
@@ -635,17 +636,25 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         /* Limited to one module. */
         if ( dom->ramdisk_blob )
             start_info_size += sizeof(struct hvm_modlist_entry);
-
-        rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
-                                  "HVMlite start info", 0, start_info_size);
-        if ( rc != 0 )
-        {
-            DOMPRINTF("Unable to reserve memory for the start info");
-            goto out;
-        }
     }
     else
     {
+        start_info_size +=
+            sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
+        /* Add extra space to write modules name */
+        start_info_size += 10 * HVMLOADER_MODULE_MAX_COUNT;
+    }
+
+    rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
+                              "HVMlite start info", 0, start_info_size);
+    if ( rc != 0 )
+    {
+        DOMPRINTF("Unable to reserve memory for the start info");
+        goto out;
+    }
+
+    if ( dom->device_model )
+    {
         /*
          * Allocate and clear additional ioreq server pages. The default
          * server will use the IOREQ and BUFIOREQ special pages above.
@@ -1699,39 +1708,66 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
     return 0;
 }
 
+static void add_module_to_list(struct xc_dom_image *dom,
+                               struct xc_hvm_firmware_module *module,
+                               const char *name,
+                               struct hvm_modlist_entry *modlist,
+                               struct hvm_start_info *start_info)
+{
+    uint32_t index = start_info->nr_modules;
+    if ( module->length == 0 )
+        return;
+
+    assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT);
+    assert(strnlen(name, 10) < 10);
+
+    modlist[index].paddr = module->guest_addr_out;
+    modlist[index].size = module->length;
+    strncpy((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT) + 10 * index,
+            name, 10);
+    modlist[index].cmdline_paddr =
+        (dom->start_info_seg.pfn << PAGE_SHIFT) +
+        ((uintptr_t)modlist - (uintptr_t)start_info) +
+        sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT +
+        10 * index;
+
+    start_info->nr_modules++;
+}
+
 static int bootlate_hvm(struct xc_dom_image *dom)
 {
     uint32_t domid = dom->guest_domid;
     xc_interface *xch = dom->xch;
+    struct hvm_start_info *start_info;
+    size_t start_info_size;
+    void *start_page;
+    struct hvm_modlist_entry *modlist;
 
-    if ( !dom->device_model )
-    {
-        struct hvm_start_info *start_info;
-        size_t start_info_size;
-        void *start_page;
-
-        start_info_size = sizeof(*start_info) + dom->cmdline_size;
-        if ( dom->ramdisk_blob )
-            start_info_size += sizeof(struct hvm_modlist_entry);
+    start_info_size = sizeof(*start_info) + dom->cmdline_size;
+    if ( dom->ramdisk_blob )
+        start_info_size += sizeof(struct hvm_modlist_entry);
 
-        if ( start_info_size >
-             dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
-        {
-            DOMPRINTF("Trying to map beyond start_info_seg");
-            return -1;
-        }
+    if ( start_info_size >
+         dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
+    {
+        DOMPRINTF("Trying to map beyond start_info_seg");
+        return -1;
+    }
 
-        start_page = xc_map_foreign_range(xch, domid, start_info_size,
-                                          PROT_READ | PROT_WRITE,
-                                          dom->start_info_seg.pfn);
-        if ( start_page == NULL )
-        {
-            DOMPRINTF("Unable to map HVM start info page");
-            return -1;
-        }
+    start_page = xc_map_foreign_range(xch, domid, start_info_size,
+                                      PROT_READ | PROT_WRITE,
+                                      dom->start_info_seg.pfn);
+    if ( start_page == NULL )
+    {
+        DOMPRINTF("Unable to map HVM start info page");
+        return -1;
+    }
 
-        start_info = start_page;
+    start_info = start_page;
+    modlist = start_page + sizeof(*start_info) + dom->cmdline_size;
 
+    if ( !dom->device_model )
+    {
         if ( dom->cmdline )
         {
             char *cmdline = start_page + sizeof(*start_info);
@@ -1743,22 +1779,32 @@ static int bootlate_hvm(struct xc_dom_image *dom)
 
         if ( dom->ramdisk_blob )
         {
-            struct hvm_modlist_entry *modlist =
-                start_page + sizeof(*start_info) + dom->cmdline_size;
 
             modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
             modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
-            start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
-                                ((uintptr_t)modlist - (uintptr_t)start_info);
             start_info->nr_modules = 1;
         }
-
-        start_info->magic = XEN_HVM_START_MAGIC_VALUE;
-
-        munmap(start_page, start_info_size);
     }
     else
     {
+        add_module_to_list(dom, &dom->bios_module, "bios",
+                           modlist, start_info);
+        add_module_to_list(dom, &dom->full_acpi_module, "acpi",
+                           modlist, start_info);
+    }
+
+    if ( start_info->nr_modules )
+    {
+        start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
+                            ((uintptr_t)modlist - (uintptr_t)start_info);
+    }
+
+    start_info->magic = XEN_HVM_START_MAGIC_VALUE;
+
+    munmap(start_page, start_info_size);
+
+    if ( dom->device_model )
+    {
         void *hvm_info_page;
 
         if ( (hvm_info_page = xc_map_foreign_range(
-- 
Anthony PERARD


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

[PATCH v3 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


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

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

[Anthony PERARD]

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

This introduce a rules for dsdt_anycpu_qemu_xen.aml, and change the rule
for dsdt_anycpu_qemu_xen.c to temporarly generate both .c and .aml. The
generation of dsdt_anycpu_qemu_xen.c will go away in a future patch.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V3:
- do not check if ROMs file exist before installing, they should exist
- change rules for dsdt_anycpu_qemu_xen.c in oder to generate both .c and
  .aml files without changing temporarly the other dsdt_*.c rules.
---
 .gitignore                             |  1 +
 tools/firmware/Makefile                | 15 +++++++++++++++
 tools/firmware/hvmloader/acpi/Makefile | 11 +++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 91f690c..abd0dce 100644
--- a/.gitignore
+++ b/.gitignore
@@ -127,6 +127,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..aa1ab54 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),)
+	$(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
+endif
+endif
+ifeq ($(CONFIG_OVMF),y)
+ifeq ($(OVMF_PATH),)
+	$(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
+endif
+endif
+	$(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..a3041d3 100644
--- a/tools/firmware/hvmloader/acpi/Makefile
+++ b/tools/firmware/hvmloader/acpi/Makefile
@@ -23,7 +23,7 @@ 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 $<
@@ -42,12 +42,19 @@ dsdt_%cpu.asl: dsdt.asl mk_dsdt
 	awk 'NR > 1 {print s} {s=$$0}' $< > $@
 	./mk_dsdt --debug=$(debug) --maxcpu $*  >> $@
 
-$(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
+$(filter dsdt_%cpu.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
 
+dsdt_anycpu_qemu_xen.c: dsdt_anycpu_qemu_xen.aml
+dsdt_anycpu_qemu_xen.aml: %.aml: iasl %.asl
+	iasl -vs -p $* -tc $*.asl
+	sed -e 's/AmlCode/$*/g' $*.hex >$*.c
+	echo "int $*_len=sizeof($*);" >>$*.c
+	rm -f $*.hex
+
 iasl:
 	@echo
 	@echo "ACPI ASL compiler (iasl) is needed"
-- 
Anthony PERARD


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

[PATCH v3 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_firmware in the domain_build_info struct by other
libxl user.

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

---
Change in V3:
- move seabios_path and ovmf_path to libxl_path.c (with renaming)
- fix some coding style
- warn for empty file
- remove rombios stuff (will still be built-in hvmloader)
- rename field bios_filename in domain_build_info to bios_firmware to
  follow naming of acpi and smbios.
- log an error after libxl_read_file_contents() only when it return ENOENT
- return an error on empty file.
- added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
---
 tools/libxl/libxl.h          |  8 +++++++
 tools/libxl/libxl_dom.c      | 57 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  2 ++
 tools/libxl/libxl_paths.c    | 10 ++++++++
 tools/libxl/libxl_types.idl  |  1 +
 tools/libxl/xl_cmdimpl.c     | 11 ++++++---
 6 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index fa87f53..d223c35 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -876,6 +876,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
  */
 #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1
 
+/*
+ * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
+ *
+ * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
+ * to provide a different bios blob (like SeaBIOS or OVMF).
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2269998..50abfbc 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -863,6 +863,38 @@ err:
     return ret;
 }
 
+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) {
+        /*
+         * Print a message only on ENOENT, other error are logged by the
+         * function libxl_read_file_contents().
+         */
+        if (e == ENOENT)
+            LOGEV(ERROR, e, "failed to read %s file", what);
+        return ERROR_FAIL;
+    }
+    libxl__ptr_add(gc, data);
+    if (datalen) {
+        /* Only accept non-empty files */
+        m->data = data;
+        m->length = datalen;
+    } else {
+        LOG(ERROR, "file %s for %s is empty", filename, what);
+        return ERROR_FAIL;
+    }
+    return 0;
+}
+
 static int libxl__domain_firmware(libxl__gc *gc,
                                   libxl_domain_build_info *info,
                                   struct xc_dom_image *dom)
@@ -872,6 +904,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
     int e, rc;
     int datalen = 0;
     void *data;
+    const char *bios_filename = NULL;
 
     if (info->u.hvm.firmware)
         firmware = info->u.hvm.firmware;
@@ -915,6 +948,30 @@ static int libxl__domain_firmware(libxl__gc *gc,
         goto out;
     }
 
+    if (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        if (info->u.hvm.bios_firmware) {
+            bios_filename = info->u.hvm.bios_firmware;
+        } else {
+            switch (info->u.hvm.bios) {
+            case LIBXL_BIOS_TYPE_SEABIOS:
+                bios_filename = libxl__seabios_path();
+                break;
+            case LIBXL_BIOS_TYPE_OVMF:
+                bios_filename = libxl__ovmf_path();
+                break;
+            case LIBXL_BIOS_TYPE_ROMBIOS:
+            default:
+                abort();
+            }
+        }
+    }
+
+    if (bios_filename) {
+        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_internal.h b/tools/libxl/libxl_internal.h
index 650a958..0dbff27 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2265,6 +2265,8 @@ _hidden const char *libxl__xen_config_dir_path(void);
 _hidden const char *libxl__xen_script_dir_path(void);
 _hidden const char *libxl__lock_dir_path(void);
 _hidden const char *libxl__run_dir_path(void);
+_hidden const char *libxl__seabios_path(void);
+_hidden const char *libxl__ovmf_path(void);
 
 /*----- subprocess execution with timeout -----*/
 
diff --git a/tools/libxl/libxl_paths.c b/tools/libxl/libxl_paths.c
index 9b7b0d5..6972b90 100644
--- a/tools/libxl/libxl_paths.c
+++ b/tools/libxl/libxl_paths.c
@@ -35,6 +35,16 @@ const char *libxl__run_dir_path(void)
     return XEN_RUN_DIR;
 }
 
+const char *libxl__seabios_path(void)
+{
+    return SEABIOS_PATH;
+}
+
+const char *libxl__ovmf_path(void)
+{
+    return OVMF_PATH;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9ad7eba..95bacd2 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -487,6 +487,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("timer_mode",       libxl_timer_mode),
                                        ("nested_hvm",       libxl_defbool),
                                        ("altp2m",           libxl_defbool),
+                                       ("bios_firmware",    string),
                                        ("smbios_firmware",  string),
                                        ("acpi_firmware",    string),
                                        ("hdtype",           libxl_hdtype),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f40af51..201cff6 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_firmware, 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_firmware)
+            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


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

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

[Anthony PERARD]

A user can provide a different ACPI tables than the default one by using
the existing "acpi_firmware" xl's config option or the field
u.hvm.acpi_firmware.

libxl will check if the provided table is a DSDT or not.

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

---
Change in V3:
- use existing acpi_firmware option to provide an override for the acpi
  tables. Will check if it's a DSDT or an extra tables.
---
 tools/libxl/libxl_dom.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 50abfbc..87853fd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -905,6 +905,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
     int datalen = 0;
     void *data;
     const char *bios_filename = NULL;
+    const char *full_acpi_tables_filename = NULL;
 
     if (info->u.hvm.firmware)
         firmware = info->u.hvm.firmware;
@@ -964,6 +965,10 @@ static int libxl__domain_firmware(libxl__gc *gc,
                 abort();
             }
         }
+
+        full_acpi_tables_filename =
+            libxl__abs_path(gc, "dsdt_anycpu_qemu_xen.aml",
+                            libxl__xenfirmwaredir_path());
     }
 
     if (bios_filename) {
@@ -1008,6 +1013,27 @@ static int libxl__domain_firmware(libxl__gc *gc,
         }
     }
 
+    /*
+     * Check if the user supplied ACPI tables are the full tables, or if
+     * there are only extra tables. The full tables start with the DSDT
+     * table, and the signature is in the first four bytes.
+     */
+    if (!dom->acpi_module.length
+          || strncmp("DSDT", (char*)dom->acpi_module.data, 4)) {
+        if (full_acpi_tables_filename) {
+            rc = libxl__load_hvm_firmware_module(gc, full_acpi_tables_filename,
+                                                 "ACPI tables",
+                                                 &dom->full_acpi_module);
+            if (rc) goto out;
+        }
+    } else {
+        /* Use user supplied DSDT tables. */
+        dom->full_acpi_module.data = dom->acpi_module.data;
+        dom->full_acpi_module.length = dom->acpi_module.length;
+        dom->acpi_module.length = 0;
+        dom->acpi_module.data = NULL;
+    }
+
     return 0;
 out:
     assert(rc != 0);
-- 
Anthony PERARD


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

[PATCH v3 07/16] hvmloader: Grab the hvm_start_info pointer

[Anthony PERARD]

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

---
Change in V3:
- remove cmdline parser
- load hvm_start_info pointer earlier, before calling main().
- Add struct hvm_start_info definition to hvmloader.
---
 tools/firmware/hvmloader/hvm_start_info.h | 32 +++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/hvmloader.c      |  6 ++++++
 2 files changed, 38 insertions(+)
 create mode 100644 tools/firmware/hvmloader/hvm_start_info.h

diff --git a/tools/firmware/hvmloader/hvm_start_info.h b/tools/firmware/hvmloader/hvm_start_info.h
new file mode 100644
index 0000000..b95d60d
--- /dev/null
+++ b/tools/firmware/hvmloader/hvm_start_info.h
@@ -0,0 +1,32 @@
+#ifndef __HVM_START_INFO_H__
+#define __HVM_START_INFO_H__
+
+/* C representation of the x86/HVM start info layout.
+ *
+ * The canonical definition of this layout resides in public/xen.h, this
+ * is just a way to represent the layout described there using C types.
+ *
+ * NB: the packed attribute is not really needed, but it helps us enforce
+ * the fact this this is just a representation, and it might indeed
+ * be required in the future if there are alignment changes.
+ */
+struct hvm_start_info {
+    uint32_t magic;             /* Contains the magic value 0x336ec578       */
+                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
+    uint32_t version;           /* Version of this structure.                */
+    uint32_t flags;             /* SIF_xxx flags.                            */
+    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
+    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
+    uint32_t modlist_paddr;     /* Physical address of an array of           */
+                                /* hvm_modlist_entry.                        */
+    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
+                                /* structure.                                */
+} __attribute__((packed));
+
+struct hvm_modlist_entry {
+    uint64_t paddr;             /* Physical address of the module.           */
+    uint64_t size;              /* Size of the module in bytes.              */
+    uint64_t cmdline_paddr;     /* Physical address of the command line.     */
+    uint64_t reserved;
+} __attribute__((packed));
+#endif /* __HVM_START_INFO_H__ */
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 716d03c..20ec8dc 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -28,6 +28,9 @@
 #include "vnuma.h"
 #include <xen/version.h>
 #include <xen/hvm/params.h>
+#include "hvm_start_info.h"
+
+const struct hvm_start_info *hvm_start_info;
 
 asm (
     "    .text                       \n"
@@ -46,6 +49,8 @@ asm (
     "    ljmp $"STR(SEL_CODE32)",$1f \n"
     "1:  movl $stack_top,%esp        \n"
     "    movl %esp,%ebp              \n"
+    /* store HVM start info ptr */
+    "    mov  %ebx, hvm_start_info   \n"
     "    call main                   \n"
     /* Relocate real-mode trampoline to 0x0. */
     "    mov  $trampoline_start,%esi \n"
@@ -258,6 +263,7 @@ int main(void)
     memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
 
     printf("HVM Loader\n");
+    BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE);
 
     init_hypercalls();
 
-- 
Anthony PERARD


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

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

[Anthony PERARD]

The BIOS can be found an entry called "bios" of the modlist of the
hvm_start_info struct.

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>

---
Changes in V3:
- fix some codying style
- use module.cmdline to look for a module name instead of the main cmdline
  from hvm_start_info.
---
 tools/firmware/hvmloader/config.h    |  2 +-
 tools/firmware/hvmloader/hvmloader.c | 34 ++++++++++++++++++++++++++++++++--
 tools/firmware/hvmloader/ovmf.c      |  3 ++-
 tools/firmware/hvmloader/rombios.c   |  3 ++-
 tools/firmware/hvmloader/util.h      |  2 ++
 5 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index b838cf9..4c6d8ad 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 20ec8dc..d319de0 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -254,10 +254,32 @@ static void acpi_enable_sci(void)
     BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
 }
 
+const struct hvm_modlist_entry *get_module_entry(
+    const struct hvm_start_info *info,
+    const char *name)
+{
+    const struct hvm_modlist_entry *modlist =
+        (struct hvm_modlist_entry *)info->modlist_paddr;
+    int i;
+
+    for ( i = 0; i < info->nr_modules; i++ )
+    {
+        uint32_t module_name = modlist[i].cmdline_paddr;
+        if ( !strcmp(name, (char*)module_name) )
+        {
+            BUG_ON(modlist[i].paddr > UINT_MAX);
+            return &modlist[i];
+        }
+    }
+
+    return NULL;
+}
+
 int main(void)
 {
     const struct bios_config *bios;
     int acpi_enabled;
+    const struct hvm_modlist_entry *bios_module;
 
     /* Initialise hypercall stubs with RET, rendering them no-ops. */
     memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
@@ -293,8 +315,16 @@ int main(void)
     }
 
     printf("Loading %s ...\n", bios->name);
-    if ( bios->bios_load )
-        bios->bios_load(bios);
+    bios_module = get_module_entry(hvm_start_info, "bios");
+    if ( bios_module && bios->bios_load )
+    {
+        uint32_t paddr = bios_module->paddr;
+        bios->bios_load(bios, (void*)paddr, bios_module->size);
+    }
+    else if ( bios->bios_load )
+    {
+        bios->bios_load(bios, 0, 0);
+    }
     else
     {
         BUG_ON(bios->bios_address + bios->image_size >
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index db9fa7a..858a2d4 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;
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index 1f15b94..2ded844 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -121,7 +121,8 @@ static void rombios_load_roms(void)
                option_rom_phys_addr + option_rom_sz - 1);
 }
 
-static void rombios_load(const struct bios_config *config)
+static void rombios_load(const struct bios_config *config,
+                         void *unused_addr, uint32_t unused_size)
 {
     uint32_t bioshigh;
     struct rombios_info *info;
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 132d915..d45c107 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -25,6 +25,8 @@
 #undef NULL
 #define NULL ((void*)0)
 
+#define UINT_MAX (~0U)
+
 void __assert_failed(char *assertion, char *file, int line)
     __attribute__((noreturn));
 #define ASSERT(p) \
-- 
Anthony PERARD


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

[PATCH v3 09/16] hvmloader: Check modules whereabouts

[Anthony PERARD]

As perform_tests() is going to clear memory past 4MB, we check that the
memory can be use.

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

diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index d319de0..a40503d 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -303,6 +303,15 @@ int main(void)
 
     smp_initialise();
 
+    /* Check that tests does not use memory where modules are stored */
+    BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 );
+    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
+    {
+        const struct hvm_modlist_entry *modlist =
+            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
+        if ( modlist[i].size )
+            BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 );
+    }
     perform_tests();
 
     if ( bios->bios_info_setup )
-- 
Anthony PERARD


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

[PATCH v3 10/16] hvmloader: Load SeaBIOS from hvm_start_info modules

[Anthony PERARD]

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

This also fix the dependency on roms.inc, hvmloader.o does not include it.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V3:
- change makefile to not include seabios roms into roms.inc.
- update the struct bios_config with the location of the bios blob.
---
 tools/firmware/hvmloader/Makefile  | 15 +--------------
 tools/firmware/hvmloader/seabios.c | 24 ++++++++++++++----------
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index f2f4791..ed0bfad 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -45,7 +45,6 @@ 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
@@ -80,19 +79,13 @@ 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
 all: subdirs-all
 	$(MAKE) hvmloader
 
-ovmf.o rombios.o seabios.o hvmloader.o: roms.inc
+ovmf.o rombios.o: roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
 
 hvmloader: $(OBJS) acpi/acpi.a
@@ -109,12 +102,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
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index c6b3d9f..5d896e3 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;
 
@@ -127,22 +124,29 @@ static void seabios_setup_e820(void)
     struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
     info->e820 = (uint32_t)e820;
 
+    BUG_ON(seabios_config.bios_address == 0);
     /* SeaBIOS reserves memory in e820 as necessary so no low reservation. */
-    info->e820_nr = build_e820_table(e820, 0, 0x100000-sizeof(seabios));
+    info->e820_nr = build_e820_table(e820, 0, seabios_config.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;
 
-    .image = seabios,
-    .image_size = sizeof(seabios),
+    BUG_ON(bios_dest + bios_length > HVMLOADER_PHYSICAL_ADDRESS);
+    memcpy((void *)bios_dest, bios_addr, bios_length);
+    seabios_config.bios_address = bios_dest;
+    seabios_config.image_size = 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


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

[PATCH v3 11/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>
---
Change in V3:
- change makefile to not include ovmf rom into roms.inc.
- update the struct bios_config with bios destination
---
 tools/firmware/hvmloader/Makefile | 15 +--------------
 tools/firmware/hvmloader/ovmf.c   | 27 ++++++++++-----------------
 2 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index ed0bfad..dee1c6b 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -43,7 +43,6 @@ endif
 
 CIRRUSVGA_DEBUG ?= n
 
-OVMF_DIR := ../ovmf-dir
 ROMBIOS_DIR := ../rombios
 
 ifeq ($(CONFIG_ROMBIOS),y)
@@ -61,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)
@@ -85,7 +78,7 @@ endif
 all: subdirs-all
 	$(MAKE) hvmloader
 
-ovmf.o rombios.o: roms.inc
+rombios.o: roms.inc
 smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
 
 hvmloader: $(OBJS) acpi/acpi.a
@@ -102,12 +95,6 @@ ifneq ($(ROMBIOS_ROM),)
 	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
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 858a2d4..89a005e 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -34,17 +34,10 @@
 #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_qemu_xen[];
@@ -97,16 +90,20 @@ 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;
+    uint64_t addr = 0x100000000ULL
+        - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
+    uint64_t ovmf_end = addr + bios_length;
+
+    ovmf_config.bios_address = addr;
+    ovmf_config.image_size = bios_length;
 
     /* Copy low-reset vector portion. */
-    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
-           + OVMF_SIZE
-           - LOWCHUNK_SIZE,
+    memcpy((void *) LOWCHUNK_BEGIN,
+           (uint8_t *) bios_addr + bios_length - LOWCHUNK_SIZE,
            LOWCHUNK_SIZE);
 
     /* Ensure we have backing page prior to moving FD. */
-    while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
+    while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
     {
         mfn = (uint32_t) (addr >> PAGE_SHIFT);
         addr += PAGE_SIZE;
@@ -114,7 +111,7 @@ static void ovmf_load(const struct bios_config *config,
     }
 
     /* Copy FD. */
-    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
+    memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
 }
 
 static void ovmf_acpi_build_tables(void)
@@ -151,10 +148,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


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

[PATCH v3 12/16] hvmloader: Specific bios_load function required

[Anthony PERARD]

All BIOS but ROMBIOS needs to be loaded via modules.

ROMBIOS is handled as a special case.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Change in V3:
- reprint Main BIOS in bios map with now available information from bios
  modules.
- handle rombios, and keep its built-in ROMs.
---
 tools/firmware/hvmloader/hvmloader.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index a40503d..cc52302 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -325,21 +325,25 @@ int main(void)
 
     printf("Loading %s ...\n", bios->name);
     bios_module = get_module_entry(hvm_start_info, "bios");
-    if ( bios_module && bios->bios_load )
+    if ( bios_module )
     {
         uint32_t paddr = bios_module->paddr;
         bios->bios_load(bios, (void*)paddr, bios_module->size);
     }
-    else if ( bios->bios_load )
+#ifdef ENABLE_ROMBIOS
+    else if ( bios == &rombios_config )
     {
         bios->bios_load(bios, 0, 0);
     }
+#endif
     else
     {
-        BUG_ON(bios->bios_address + bios->image_size >
-               HVMLOADER_PHYSICAL_ADDRESS);
-        memcpy((void *)bios->bios_address, bios->image,
-               bios->image_size);
+        /*
+         * If there is no BIOS module supplied and if there is no embeded BIOS
+         * image, then we failed. Only rombios might have an embedded bios blob.
+         */
+        printf("no BIOS ROM image found\n");
+        BUG();
     }
 
     if ( (hvm_info->nr_vcpus > 1) || hvm_info->apic_mode )
-- 
Anthony PERARD


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

[PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module

[Anthony PERARD]

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

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

---
Changes in V3:
- handle rombios
---
 tools/firmware/hvmloader/config.h    |  2 +-
 tools/firmware/hvmloader/hvmloader.c | 20 +++++++++++++++++++-
 tools/firmware/hvmloader/ovmf.c      |  9 +++------
 tools/firmware/hvmloader/rombios.c   |  3 ++-
 tools/firmware/hvmloader/seabios.c   |  9 +++------
 5 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 4c6d8ad..2c39637 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -29,7 +29,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 cc52302..e7b0139 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -365,8 +365,26 @@ int main(void)
 
         if ( bios->acpi_build_tables )
         {
+            const struct hvm_modlist_entry *acpi_module;
+            acpi_module = get_module_entry(hvm_start_info, "acpi");
             printf("Loading ACPI ...\n");
-            bios->acpi_build_tables();
+            if ( acpi_module )
+            {
+                uint32_t paddr = acpi_module->paddr;
+                bios->acpi_build_tables((void*)paddr,
+                                        acpi_module->size);
+            }
+#ifdef ENABLE_ROMBIOS
+            else if ( bios == &rombios_config )
+            {
+                bios->acpi_build_tables(0, 0);
+            }
+#endif
+            else
+            {
+                printf("no ACPI DSDT image found\n");
+                BUG();
+            }
         }
 
         acpi_enable_sci();
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 89a005e..c1612ea 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -40,9 +40,6 @@
 #define LOWCHUNK_MAXOFFSET      0x0000FFFF
 #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
 
-extern unsigned char dsdt_anycpu_qemu_xen[];
-extern int dsdt_anycpu_qemu_xen_len;
-
 #define OVMF_INFO_MAX_TABLES 4
 struct ovmf_info {
     char signature[14]; /* XenHVMOVMF\0\0\0\0 */
@@ -114,11 +111,11 @@ static void ovmf_load(const struct bios_config *config,
     memcpy((void *) ovmf_config.bios_address, 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_qemu_xen,
-        .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
+        .dsdt_anycpu = addr,
+        .dsdt_anycpu_len = length,
         .dsdt_15cpu = NULL, 
         .dsdt_15cpu_len = 0
     };
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index 2ded844..732a7ff 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -172,7 +172,8 @@ static void reset_bios_checksum(void)
     *((uint8_t *)(ROMBIOS_BEGIN + ROMBIOS_MAXOFFSET)) = -checksum;
 }
 
-static void rombios_acpi_build_tables(void)
+static void rombios_acpi_build_tables(void *unused_addr,
+                                      uint32_t unused_size)
 {
     struct acpi_config config = {
         .dsdt_anycpu = dsdt_anycpu,
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index 5d896e3..b55a7c9 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


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

[PATCH v3 14/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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/firmware/hvmloader/acpi/Makefile b/tools/firmware/hvmloader/acpi/Makefile
index a3041d3..3252ddf 100644
--- a/tools/firmware/hvmloader/acpi/Makefile
+++ b/tools/firmware/hvmloader/acpi/Makefile
@@ -17,7 +17,7 @@
 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)
@@ -48,12 +48,8 @@ $(filter dsdt_%cpu.c,$(C_SRC)): %.c: iasl %.asl
 	echo "int $*_len=sizeof($*);" >>$@
 	rm -f $*.aml $*.hex
 
-dsdt_anycpu_qemu_xen.c: dsdt_anycpu_qemu_xen.aml
 dsdt_anycpu_qemu_xen.aml: %.aml: iasl %.asl
-	iasl -vs -p $* -tc $*.asl
-	sed -e 's/AmlCode/$*/g' $*.hex >$*.c
-	echo "int $*_len=sizeof($*);" >>$*.c
-	rm -f $*.hex
+	iasl -vs -p $* $*.asl
 
 iasl:
 	@echo
-- 
Anthony PERARD


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

[PATCH v3 15/16] hvmloader: Always build-in SeaBIOS and OVMF loader

[Anthony PERARD]

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

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index dee1c6b..9f7357f 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -37,6 +37,7 @@ OBJS  = hvmloader.o mp_tables.o util.o smbios.o
 OBJS += smp.o cacheattr.o xenbus.o vnuma.o
 OBJS += e820.o pci.o pir.o ctype.o
 OBJS += hvm_param.o
+OBJS += ovmf.o seabios.o
 ifeq ($(debug),y)
 OBJS += tests.o
 endif
@@ -57,11 +58,6 @@ 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
 CFLAGS += -DENABLE_ROMBIOS
@@ -69,11 +65,6 @@ 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
 	$(MAKE) hvmloader
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index e7b0139..bd774d5 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -210,12 +210,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


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

[PATCH v3 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 aa1ab54..d7b508e 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),)
 	$(INSTALL_DATA) $(SEABIOS_ROM) $(INST_DIR)/seabios.bin
 endif
-endif
 ifeq ($(CONFIG_OVMF),y)
-ifeq ($(OVMF_PATH),)
 	$(INSTALL_DATA) $(OVMF_ROM) $(INST_DIR)/ovmf.bin
 endif
-endif
 	$(INSTALL_DATA) $(ACPI_TABLE_QEMU_PC_I440FX) $(INST_DIR)
 
 .PHONY: clean
-- 
Anthony PERARD


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

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

[Boris Ostrovsky]

On 02/25/2016 09:55 AM, Anthony PERARD wrote:
> Hi all,
>
> Many changes in V3:
>    no more cmdline, but use each modules' cmdline to provide a name for it.
>    in libxc:
>      - use xc_dom_alloc_segment() to load modules. see patch #1.
>      - avoid duplication of code between PVHv2 and hvmloader for the
>        initialisation of hvm_start_info.
>    in xl/libxl:
>      - only one new option, bios_firmware. acpi_firmware is reused, see
>        patch #6.
>    in hvmloader:
>      - handle rombios as separate case.
>    more detail in each patches.
>
> I've look at loading the BIOS and the ACPI tables via the toolstack instead
> of having them embedded in the hvmloader binary.

This is what I've been trying to do for HVMlite/PVHv2 guests. Roger and 
I have a PoC toolstack code that loads limited ACPI data into the guest 
but I wonder whether these patches already do this.

I haven't gone over the series in detail yet. Does this allow toolstack 
to load RSDP and then R/XSDT, FADT etc. directly to guest memory or are 
they passed to hvmloader to do so via hvm_start_info? (I think it's the 
latter based on how I read patch 13 but I am not sure).


-boris


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

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

[Anthony PERARD]

On Thu, Feb 25, 2016 at 11:16:18AM -0500, Boris Ostrovsky wrote:
> On 02/25/2016 09:55 AM, Anthony PERARD wrote:
> >Hi all,
> >
> >Many changes in V3:
> >   no more cmdline, but use each modules' cmdline to provide a name for it.
> >   in libxc:
> >     - use xc_dom_alloc_segment() to load modules. see patch #1.
> >     - avoid duplication of code between PVHv2 and hvmloader for the
> >       initialisation of hvm_start_info.
> >   in xl/libxl:
> >     - only one new option, bios_firmware. acpi_firmware is reused, see
> >       patch #6.
> >   in hvmloader:
> >     - handle rombios as separate case.
> >   more detail in each patches.
> >
> >I've look at loading the BIOS and the ACPI tables via the toolstack instead
> >of having them embedded in the hvmloader binary.
> 
> This is what I've been trying to do for HVMlite/PVHv2 guests. Roger and I
> have a PoC toolstack code that loads limited ACPI data into the guest but I
> wonder whether these patches already do this.
> 
> I haven't gone over the series in detail yet. Does this allow toolstack to
> load RSDP and then R/XSDT, FADT etc. directly to guest memory or are they
> passed to hvmloader to do so via hvm_start_info? (I think it's the latter
> based on how I read patch 13 but I am not sure).

Those patches only allow to load the DSDT table from a file, rather than
having it embedded in the hvmloader binary.

hvmloader take care of building the RSDT XSDT FADT etc.

And yes, the DSDT table is passed to hvmloader via the hvm_start_info.

-- 
Anthony PERARD

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

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

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> @@ -42,12 +42,19 @@ dsdt_%cpu.asl: dsdt.asl mk_dsdt
>  	awk 'NR > 1 {print s} {s=$$0}' $< > $@
>  	./mk_dsdt --debug=$(debug) --maxcpu $*  >> $@
>  
> -$(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
> +$(filter dsdt_%cpu.c,$(C_SRC)): %.c: iasl %.asl

Would there be anything wrong with leaving this alone and ...

>  	iasl -vs -p $* -tc $*.asl
>  	sed -e 's/AmlCode/$*/g' $*.hex >$@
>  	echo "int $*_len=sizeof($*);" >>$@
>  	rm -f $*.aml $*.hex

... simply dropping the deletion of $*.aml here, plus ...

> +dsdt_anycpu_qemu_xen.c: dsdt_anycpu_qemu_xen.aml

... an equivalent of this added?

> +dsdt_anycpu_qemu_xen.aml: %.aml: iasl %.asl
> +	iasl -vs -p $* -tc $*.asl

Otherwise, if this is the rule to remain long term, then please make
use of $< here, which aiui would require flipping around the two
dependencies.

Whichever route you chose, with a respective adjustment
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v3 07/16] hvmloader: Grab the hvm_start_info pointer

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> --- /dev/null
> +++ b/tools/firmware/hvmloader/hvm_start_info.h
> @@ -0,0 +1,32 @@
> +#ifndef __HVM_START_INFO_H__
> +#define __HVM_START_INFO_H__
> +
> +/* C representation of the x86/HVM start info layout.
> + *
> + * The canonical definition of this layout resides in public/xen.h, this
> + * is just a way to represent the layout described there using C types.
> + *
> + * NB: the packed attribute is not really needed, but it helps us enforce
> + * the fact this this is just a representation, and it might indeed
> + * be required in the future if there are alignment changes.
> + */
> +struct hvm_start_info {
> +    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> +                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> +    uint32_t version;           /* Version of this structure.                */
> +    uint32_t flags;             /* SIF_xxx flags.                            */
> +    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> +    uint32_t modlist_paddr;     /* Physical address of an array of           */
> +                                /* hvm_modlist_entry.                        */
> +    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
> +                                /* structure.                                */
> +} __attribute__((packed));
> +
> +struct hvm_modlist_entry {
> +    uint64_t paddr;             /* Physical address of the module.           */
> +    uint64_t size;              /* Size of the module in bytes.              */
> +    uint64_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint64_t reserved;
> +} __attribute__((packed));
> +#endif /* __HVM_START_INFO_H__ */

With there already being such a C representation in libxc, can't we
make sure we have only one instance that will need adjustment upon
future enhancements? (Perhaps it would still have been a good idea
to expose such a C representation in public/xen.h.)

> @@ -258,6 +263,7 @@ int main(void)
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
>  
>      printf("HVM Loader\n");
> +    BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE);

Wait - public/xen.h clearly says this is being handed for PVH guests
only. How can you even de-reference the pointer unconditionally?

Jan


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

Re: [PATCH v3 07/16] hvmloader: Grab the hvm_start_info pointer

[Jan Beulich]

>>> On 29.02.16 at 17:37, <JBeulich@suse.com> wrote:
>>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
>> @@ -258,6 +263,7 @@ int main(void)
>>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
>>  
>>      printf("HVM Loader\n");
>> +    BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE);
> 
> Wait - public/xen.h clearly says this is being handed for PVH guests
> only. How can you even de-reference the pointer unconditionally?

Ah, I see this is a result of patch 2, which so far I didn't look at
in any detail, the title of which doesn't suggest such a behavioral
change, and which also doesn't update the comment in
public/xen.h.

Jan


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

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

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -254,10 +254,32 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +const struct hvm_modlist_entry *get_module_entry(
> +    const struct hvm_start_info *info,
> +    const char *name)
> +{
> +    const struct hvm_modlist_entry *modlist =
> +        (struct hvm_modlist_entry *)info->modlist_paddr;
> +    int i;

unsigned int

> +    for ( i = 0; i < info->nr_modules; i++ )
> +    {
> +        uint32_t module_name = modlist[i].cmdline_paddr;

The right side is uint64_t, so you may be truncating here.

Also blank line between declaration(s) and statement(s) please.

> @@ -293,8 +315,16 @@ int main(void)
>      }
>  
>      printf("Loading %s ...\n", bios->name);
> -    if ( bios->bios_load )
> -        bios->bios_load(bios);
> +    bios_module = get_module_entry(hvm_start_info, "bios");
> +    if ( bios_module && bios->bios_load )
> +    {
> +        uint32_t paddr = bios_module->paddr;
> +        bios->bios_load(bios, (void*)paddr, bios_module->size);

bios_module->size is a 64-bit quantity too, so would also need
checking.

> +    }
> +    else if ( bios->bios_load )
> +    {
> +        bios->bios_load(bios, 0, 0);
> +    }
>      else
>      {
>          BUG_ON(bios->bios_address + bios->image_size >

Is all of the remaining code (from the first "else if" above) intended
to go away eventually?

Jan


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

Re: [PATCH v3 09/16] hvmloader: Check modules whereabouts

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -303,6 +303,15 @@ int main(void)
>  
>      smp_initialise();
>  
> +    /* Check that tests does not use memory where modules are stored */
> +    BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 );
> +    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
> +    {
> +        const struct hvm_modlist_entry *modlist =
> +            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
> +        if ( modlist[i].size )
> +            BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 );
> +    }

First of all both checks should use > instead of >=. And then
expecting all modules to live below 4Mb doesn't seem very
reasonable.

Jan


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

Re: [PATCH v3 10/16] hvmloader: Load SeaBIOS from hvm_start_info modules

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> @@ -127,22 +124,29 @@ static void seabios_setup_e820(void)
>      struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
>      info->e820 = (uint32_t)e820;
>  
> +    BUG_ON(seabios_config.bios_address == 0);

I think this is too lax: Surely this shouldn't live below 0xC0000
nor above 0x100000?

Jan


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

Re: [PATCH v3 01/16] libxc: Rework extra module initialisation

[Wei Liu]

On Thu, Feb 25, 2016 at 02:55:59PM +0000, Anthony PERARD wrote:
> This patch use xc_dom_alloc_segment() to allocate the memory space for the
> ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> placement of 1MB after the hvmloader image.
> 
> In later patches, while trying to load a firmware such as OVMF, the later
> could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
> hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
> process, clear the memory, before loading the modules.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

> ---
> New patch in V3.
> ---
>  tools/libxc/xc_dom_hvmloader.c | 124 +++++++++++------------------------------
>  1 file changed, 31 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index 330d5e8..54e096c 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -129,98 +129,45 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
>      return rc;
>  }
>  
> -static int modules_init(struct xc_dom_image *dom,
> -                        uint64_t vend, struct elf_binary *elf,
> -                        uint64_t *mstart_out, uint64_t *mend_out)
> +static int module_init_one(struct xc_dom_image *dom,
> +                           struct xc_hvm_firmware_module *module,
> +                           char *name)
>  {
> -#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;
> +    struct xc_dom_seg seg;
> +    void *dest;
>  
> -    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> -        return 0;
> -
> -    /* 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);
> -    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);
> -    *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;
> -    if ( dom->smbios_module.length != 0 )
> -        dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> +    if ( module->length )
> +    {
> +        if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> +            goto err;
> +        dest = xc_dom_seg_to_ptr(dom, &seg);
> +        if ( dest == NULL )
> +        {
> +            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> +                      __FUNCTION__);
> +            goto err;
> +        }
> +        memcpy(dest, module->data, module->length);
> +        module->guest_addr_out = seg.vstart;

One thing you might want to take care is that the guest_addr_out is
actually within a reasonable range that hvmloader can access?


Wei.

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

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

[Wei Liu]

On Thu, Feb 25, 2016 at 02:56:00PM +0000, Anthony PERARD wrote:
> This adds two new firmware module, bios_module and full_acpi_module. They
> are loaded in the guest memory and final location is provided to hvmloader
> via the hvm_start_info struct.
> 
> This patch create the hvm_start_info struct for HVM guest that have a
> device model, so this is now common code with HVM guest without device
> model.
> 

I think you also need to update the relevant headers to say the
structures is now used by both PVHv2 and HVM.

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Change in V3:
> - rename acpi_table_module to full_acpi_module.
> - factorise module loading, using new function to load existing optinal
>   module, this should not change anything
> - should now use the same code to loads modules as for HVMlite VMs.
>   this avoid duplication of code.
> - no more generic cmdline with a list of modules, each module have its name
>   in the module specific cmdline.
> - scope change for common code between hvmlite and hvmloader
> ---
>  tools/libxc/include/xc_dom.h   |   4 ++
>  tools/libxc/xc_dom_hvmloader.c |   4 ++
>  tools/libxc/xc_dom_x86.c       | 130 ++++++++++++++++++++++++++++-------------
>  3 files changed, 96 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 6ebe946..7dc3fe7 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -209,6 +209,10 @@ struct xc_dom_image {
>      /* If unset disables the setup of the IOREQ pages. */
>      bool device_model;
>  
> +    /* BIOS and ACPI tables passed to HVMLOADER */
> +    struct xc_hvm_firmware_module bios_module;
> +    struct xc_hvm_firmware_module full_acpi_module;
> +
>      /* Extra ACPI tables passed to HVMLOADER */
>      struct xc_hvm_firmware_module acpi_module;
>  

What is the difference between "ACPI tables" and "Extra ACPI tables"?

> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index 54e096c..2c965c9 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -160,6 +160,10 @@ static int modules_init(struct xc_dom_image *dom)
>  {
>      int rc;
[...]
>      }
>      else
>      {
> +        start_info_size +=
> +            sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> +        /* Add extra space to write modules name */
> +        start_info_size += 10 * HVMLOADER_MODULE_MAX_COUNT;

Why magic number 10? I guess this is the length limit of the name? Can
you have a #define somewhere?

Wei.

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

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

[Wei Liu]

On Thu, Feb 25, 2016 at 02:56:01PM +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.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 

There is already --with-system-seabios and --with-system-ovmf that you
can use.

Wei.

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

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

[Wei Liu]

On Thu, Feb 25, 2016 at 02:56:03PM +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_firmware in the domain_build_info struct by other
> libxl user.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Change in V3:
> - move seabios_path and ovmf_path to libxl_path.c (with renaming)
> - fix some coding style
> - warn for empty file
> - remove rombios stuff (will still be built-in hvmloader)
> - rename field bios_filename in domain_build_info to bios_firmware to
>   follow naming of acpi and smbios.
> - log an error after libxl_read_file_contents() only when it return ENOENT
> - return an error on empty file.
> - added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> ---
>  tools/libxl/libxl.h          |  8 +++++++
>  tools/libxl/libxl_dom.c      | 57 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |  2 ++
>  tools/libxl/libxl_paths.c    | 10 ++++++++
>  tools/libxl/libxl_types.idl  |  1 +
>  tools/libxl/xl_cmdimpl.c     | 11 ++++++---

You also need to patch manpage for this new option.

How does this new option interacts with bios= option?

>  6 files changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index fa87f53..d223c35 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -876,6 +876,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
>   */
>  #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1
>  
> +/*
> + * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> + *
> + * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
> + * to provide a different bios blob (like SeaBIOS or OVMF).
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> +
>  typedef char **libxl_string_list;
>  void libxl_string_list_dispose(libxl_string_list *sl);
>  int libxl_string_list_length(const libxl_string_list *sl);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2269998..50abfbc 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -863,6 +863,38 @@ err:
>      return ret;
>  }
>  
> +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) {
> +        /*
> +         * Print a message only on ENOENT, other error are logged by the
> +         * function libxl_read_file_contents().
> +         */
> +        if (e == ENOENT)
> +            LOGEV(ERROR, e, "failed to read %s file", what);
> +        return ERROR_FAIL;
> +    }
> +    libxl__ptr_add(gc, data);
> +    if (datalen) {
> +        /* Only accept non-empty files */
> +        m->data = data;
> +        m->length = datalen;
> +    } else {
> +        LOG(ERROR, "file %s for %s is empty", filename, what);
> +        return ERROR_FAIL;

ERROR_INVAL is more appropriate.


Wei.

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

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

[Wei Liu]

On Thu, Feb 25, 2016 at 02:56:04PM +0000, Anthony PERARD wrote:
> A user can provide a different ACPI tables than the default one by using
> the existing "acpi_firmware" xl's config option or the field
> u.hvm.acpi_firmware.
> 
> libxl will check if the provided table is a DSDT or not.
> 

According to xl.cfg manpage, acpi_firmware= cann't be used to override
DSDT, so you seem to be changing the semantics of existing option.

Wei.

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

Re: [PATCH v3 11/16] hvmloader: Load OVMF from modules

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -34,17 +34,10 @@
>  #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_qemu_xen[];
> @@ -97,16 +90,20 @@ 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;
> +    uint64_t addr = 0x100000000ULL
> +        - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
> +    uint64_t ovmf_end = addr + bios_length;
> +
> +    ovmf_config.bios_address = addr;
> +    ovmf_config.image_size = bios_length;
>  
>      /* Copy low-reset vector portion. */
> -    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
> -           + OVMF_SIZE
> -           - LOWCHUNK_SIZE,
> +    memcpy((void *) LOWCHUNK_BEGIN,
> +           (uint8_t *) bios_addr + bios_length - LOWCHUNK_SIZE,
>             LOWCHUNK_SIZE);
>  
>      /* Ensure we have backing page prior to moving FD. */
> -    while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
> +    while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
>      {
>          mfn = (uint32_t) (addr >> PAGE_SHIFT);
>          addr += PAGE_SIZE;
> @@ -114,7 +111,7 @@ static void ovmf_load(const struct bios_config *config,
>      }
>  
>      /* Copy FD. */
> -    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> +    memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
>  }

Is this safe, considering that source and destination may now
overlap? Thinking about it, the same consideration applies to
BIOS placement below 1Mb too. Roger - should we perhaps
amend the respective hvm_start_info comment to exclude
anything getting placed below 1Mb?

Jan


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

Re: [PATCH v3 12/16] hvmloader: Specific bios_load function required

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> All BIOS but ROMBIOS needs to be loaded via modules.
> 
> ROMBIOS is handled as a special case.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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


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

Re: [PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -365,8 +365,26 @@ int main(void)
>  
>          if ( bios->acpi_build_tables )
>          {
> +            const struct hvm_modlist_entry *acpi_module;
> +            acpi_module = get_module_entry(hvm_start_info, "acpi");
>              printf("Loading ACPI ...\n");
> -            bios->acpi_build_tables();
> +            if ( acpi_module )
> +            {
> +                uint32_t paddr = acpi_module->paddr;
> +                bios->acpi_build_tables((void*)paddr,
> +                                        acpi_module->size);
> +            }

Hmm, so far it was the build process which ensured the right ACPI
tables would be used with the corresponding BIOS. The disconnect
that gets introduced here worries me a little, since things having
got out of sync may be rather hard to diagnose (as they may
surface only much later).

> +#ifdef ENABLE_ROMBIOS
> +            else if ( bios == &rombios_config )
> +            {
> +                bios->acpi_build_tables(0, 0);
> +            }
> +#endif
> +            else
> +            {
> +                printf("no ACPI DSDT image found\n");
> +                BUG();
> +            }

Is this really fatal? It's not like "no ACPI" == "end of the world",
is it?

Jan


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

Re: [PATCH v3 14/16] hvmloader: Compile out the qemu-xen ACPI tables

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> It should now be loaded by libxl.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Pending concerns to previous patches, this one on its own clearly
looks fine, i.e.
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v3 15/16] hvmloader: Always build-in SeaBIOS and OVMF loader

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Same as previous patch:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v3 16/16] hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...

[Jan Beulich]

>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> ... 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>

The subject prefix is a little misleading, as this doesn't really touch
hvmloader in any way.

Jan


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

Re: [PATCH v3 16/16] hvmloader: do not depend on SEABIOS_PATH or OVMF_PATH ...

[Wei Liu]

On Thu, Feb 25, 2016 at 02:56:14PM +0000, Anthony PERARD wrote:
> ... 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>
> 

Just notice this patch belongs to toolstack.

But I guess it should be modified or changed given my comment on the
--with-system-* in previous patch.

Wei.

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

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

[Anthony PERARD]

On Mon, Feb 29, 2016 at 09:31:11AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > @@ -42,12 +42,19 @@ dsdt_%cpu.asl: dsdt.asl mk_dsdt
> >  	awk 'NR > 1 {print s} {s=$$0}' $< > $@
> >  	./mk_dsdt --debug=$(debug) --maxcpu $*  >> $@
> >  
> > -$(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
> > +$(filter dsdt_%cpu.c,$(C_SRC)): %.c: iasl %.asl
> 
> Would there be anything wrong with leaving this alone and ...
> 
> >  	iasl -vs -p $* -tc $*.asl
> >  	sed -e 's/AmlCode/$*/g' $*.hex >$@
> >  	echo "int $*_len=sizeof($*);" >>$@
> >  	rm -f $*.aml $*.hex
> 
> ... simply dropping the deletion of $*.aml here, plus ...
> 
> > +dsdt_anycpu_qemu_xen.c: dsdt_anycpu_qemu_xen.aml
> 
> ... an equivalent of this added?
> 
> > +dsdt_anycpu_qemu_xen.aml: %.aml: iasl %.asl
> > +	iasl -vs -p $* -tc $*.asl
> 
> Otherwise, if this is the rule to remain long term, then please make
> use of $< here, which aiui would require flipping around the two
> dependencies.

Yes, this rules is here to stay. The dsdt_anycpu_qemu_xen.c won't be needed
anymore after the patch series. The rule change in patch #14. I will use
$<.

> Whichever route you chose, with a respective adjustment
> Acked-by: Jan Beulich <jbeulich@suse.com>

-- 
Anthony PERARD

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

Re: [PATCH v3 09/16] hvmloader: Check modules whereabouts

[Anthony PERARD]

On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -303,6 +303,15 @@ int main(void)
> >  
> >      smp_initialise();
> >  
> > +    /* Check that tests does not use memory where modules are stored */
> > +    BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 );
> > +    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
> > +    {
> > +        const struct hvm_modlist_entry *modlist =
> > +            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
> > +        if ( modlist[i].size )
> > +            BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 );
> > +    }
> 
> First of all both checks should use > instead of >=. And then
> expecting all modules to live below 4Mb doesn't seem very
> reasonable.

Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure
what to do about this, should I ignore perform_tests() if it going to
clear the memory used by the modules?

I should probably check that the modules are outside the memory region that
is going to be used by perform_tests(), instead of just checking that they
are bellow 4MB.

-- 
Anthony PERARD

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

Re: [PATCH v3 10/16] hvmloader: Load SeaBIOS from hvm_start_info modules

[Anthony PERARD]

On Mon, Feb 29, 2016 at 10:02:10AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > @@ -127,22 +124,29 @@ static void seabios_setup_e820(void)
> >      struct e820entry *e820 = scratch_alloc(sizeof(struct e820entry)*16, 0);
> >      info->e820 = (uint32_t)e820;
> >  
> > +    BUG_ON(seabios_config.bios_address == 0);
> 
> I think this is too lax: Surely this shouldn't live below 0xC0000
> nor above 0x100000?

I was just trying to check that seabios_load() was called before this one.
But I guest it does not hurt to expand the assertion.

-- 
Anthony PERARD

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

Re: [PATCH v3 09/16] hvmloader: Check modules whereabouts

[Jan Beulich]

>>> On 03.03.16 at 17:00, <anthony.perard@citrix.com> wrote:
> On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote:
>> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -303,6 +303,15 @@ int main(void)
>> >  
>> >      smp_initialise();
>> >  
>> > +    /* Check that tests does not use memory where modules are stored */
>> > +    BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 
> << 20 );
>> > +    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
>> > +    {
>> > +        const struct hvm_modlist_entry *modlist =
>> > +            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
>> > +        if ( modlist[i].size )
>> > +            BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 );
>> > +    }
>> 
>> First of all both checks should use > instead of >=. And then
>> expecting all modules to live below 4Mb doesn't seem very
>> reasonable.
> 
> Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure
> what to do about this, should I ignore perform_tests() if it going to
> clear the memory used by the modules?

Temporarily this might be an option, but ultimately the tests
shouldn't assume a fixed memory location to be available.

> I should probably check that the modules are outside the memory region that
> is going to be used by perform_tests(), instead of just checking that they
> are bellow 4MB.

Yes.

Jan


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

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

[Anthony PERARD]

On Mon, Feb 29, 2016 at 09:56:48AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -254,10 +254,32 @@ static void acpi_enable_sci(void)
> >      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> >  }
> >  
> > +const struct hvm_modlist_entry *get_module_entry(
> > +    const struct hvm_start_info *info,
> > +    const char *name)
> > +{
> > +    const struct hvm_modlist_entry *modlist =
> > +        (struct hvm_modlist_entry *)info->modlist_paddr;
> > +    int i;
> 
> unsigned int

ok

> > +    for ( i = 0; i < info->nr_modules; i++ )
> > +    {
> > +        uint32_t module_name = modlist[i].cmdline_paddr;
> 
> The right side is uint64_t, so you may be truncating here.

Yes, I need to check that.

> Also blank line between declaration(s) and statement(s) please.

ok.

> > @@ -293,8 +315,16 @@ int main(void)
> >      }
> >  
> >      printf("Loading %s ...\n", bios->name);
> > -    if ( bios->bios_load )
> > -        bios->bios_load(bios);
> > +    bios_module = get_module_entry(hvm_start_info, "bios");
> > +    if ( bios_module && bios->bios_load )
> > +    {
> > +        uint32_t paddr = bios_module->paddr;
> > +        bios->bios_load(bios, (void*)paddr, bios_module->size);
> 
> bios_module->size is a 64-bit quantity too, so would also need
> checking.

Yes.

> > +    }
> > +    else if ( bios->bios_load )
> > +    {
> > +        bios->bios_load(bios, 0, 0);
> > +    }
> >      else
> >      {
> >          BUG_ON(bios->bios_address + bios->image_size >
> 
> Is all of the remaining code (from the first "else if" above) intended
> to go away eventually?

The "else if" would be kept for the benefit of rombios (if it's compiled
in), and the last "else" will be turned into a BUG.

-- 
Anthony PERARD

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

Re: [PATCH v3 01/16] libxc: Rework extra module initialisation

[Anthony PERARD]

On Tue, Mar 01, 2016 at 11:51:26AM +0000, Wei Liu wrote:
> On Thu, Feb 25, 2016 at 02:55:59PM +0000, Anthony PERARD wrote:
> > This patch use xc_dom_alloc_segment() to allocate the memory space for the
> > ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> > placement of 1MB after the hvmloader image.
> > 
> > In later patches, while trying to load a firmware such as OVMF, the later
> > could easily be loaded past the address 4MB (OVMF is a 2MB binary), but
> > hvmloader use a range of memory from 4MB to 8MB to perform tests and in the
> > process, clear the memory, before loading the modules.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> > ---
> > New patch in V3.
> > ---
> >  tools/libxc/xc_dom_hvmloader.c | 124 +++++++++++------------------------------
> >  1 file changed, 31 insertions(+), 93 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > index 330d5e8..54e096c 100644
> > --- a/tools/libxc/xc_dom_hvmloader.c
> > +++ b/tools/libxc/xc_dom_hvmloader.c
> > @@ -129,98 +129,45 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> >      return rc;
> >  }
> >  
> > -static int modules_init(struct xc_dom_image *dom,
> > -                        uint64_t vend, struct elf_binary *elf,
> > -                        uint64_t *mstart_out, uint64_t *mend_out)
> > +static int module_init_one(struct xc_dom_image *dom,
> > +                           struct xc_hvm_firmware_module *module,
> > +                           char *name)
> >  {
> > -#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;
> > +    struct xc_dom_seg seg;
> > +    void *dest;
> >  
> > -    if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> > -        return 0;
> > -
> > -    /* 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);
> > -    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);
> > -    *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;
> > -    if ( dom->smbios_module.length != 0 )
> > -        dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> > +    if ( module->length )
> > +    {
> > +        if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> > +            goto err;
> > +        dest = xc_dom_seg_to_ptr(dom, &seg);
> > +        if ( dest == NULL )
> > +        {
> > +            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> > +                      __FUNCTION__);
> > +            goto err;
> > +        }
> > +        memcpy(dest, module->data, module->length);
> > +        module->guest_addr_out = seg.vstart;
> 
> One thing you might want to take care is that the guest_addr_out is
> actually within a reasonable range that hvmloader can access?

I guest that would be a good idee. So maybe they should be bellow 3GB.

-- 
Anthony PERARD

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

Re: [PATCH v3 09/16] hvmloader: Check modules whereabouts

[Andrew Cooper]

On 03/03/16 16:00, Anthony PERARD wrote:
> On Mon, Feb 29, 2016 at 09:58:29AM -0700, Jan Beulich wrote:
>>>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
>>> --- a/tools/firmware/hvmloader/hvmloader.c
>>> +++ b/tools/firmware/hvmloader/hvmloader.c
>>> @@ -303,6 +303,15 @@ int main(void)
>>>  
>>>      smp_initialise();
>>>  
>>> +    /* Check that tests does not use memory where modules are stored */
>>> +    BUG_ON( ((uint32_t)hvm_start_info + sizeof(struct hvm_start_info)) >= 4 << 20 );
>>> +    for ( unsigned i = 0; i < hvm_start_info->nr_modules; i++ )
>>> +    {
>>> +        const struct hvm_modlist_entry *modlist =
>>> +            (struct hvm_modlist_entry *)hvm_start_info->modlist_paddr;
>>> +        if ( modlist[i].size )
>>> +            BUG_ON( modlist[i].paddr + modlist[i].size >= 4ul << 20 );
>>> +    }
>> First of all both checks should use > instead of >=. And then
>> expecting all modules to live below 4Mb doesn't seem very
>> reasonable.
> Yes, and this can be an issue with OVMF, it's a 2MB binary. I'm not sure
> what to do about this, should I ignore perform_tests() if it going to
> clear the memory used by the modules?
>
> I should probably check that the modules are outside the memory region that
> is going to be used by perform_tests(), instead of just checking that they
> are bellow 4MB.

Now that I have published my Xen Test Framework, these hvmloader tests
should be moved across and removed entirely from hvmloader.  This was
one of the original intentions.

~Andrew

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

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

[Anthony PERARD]

On Tue, Mar 01, 2016 at 11:51:30AM +0000, Wei Liu wrote:
> On Thu, Feb 25, 2016 at 02:56:00PM +0000, Anthony PERARD wrote:
> > This adds two new firmware module, bios_module and full_acpi_module. They
> > are loaded in the guest memory and final location is provided to hvmloader
> > via the hvm_start_info struct.
> > 
> > This patch create the hvm_start_info struct for HVM guest that have a
> > device model, so this is now common code with HVM guest without device
> > model.
> > 
> 
> I think you also need to update the relevant headers to say the
> structures is now used by both PVHv2 and HVM.

Yes, I can do that.

> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > Change in V3:
> > - rename acpi_table_module to full_acpi_module.
> > - factorise module loading, using new function to load existing optinal
> >   module, this should not change anything
> > - should now use the same code to loads modules as for HVMlite VMs.
> >   this avoid duplication of code.
> > - no more generic cmdline with a list of modules, each module have its name
> >   in the module specific cmdline.
> > - scope change for common code between hvmlite and hvmloader
> > ---
> >  tools/libxc/include/xc_dom.h   |   4 ++
> >  tools/libxc/xc_dom_hvmloader.c |   4 ++
> >  tools/libxc/xc_dom_x86.c       | 130 ++++++++++++++++++++++++++++-------------
> >  3 files changed, 96 insertions(+), 42 deletions(-)
> > 
> > diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> > index 6ebe946..7dc3fe7 100644
> > --- a/tools/libxc/include/xc_dom.h
> > +++ b/tools/libxc/include/xc_dom.h
> > @@ -209,6 +209,10 @@ struct xc_dom_image {
> >      /* If unset disables the setup of the IOREQ pages. */
> >      bool device_model;
> >  
> > +    /* BIOS and ACPI tables passed to HVMLOADER */
> > +    struct xc_hvm_firmware_module bios_module;
> > +    struct xc_hvm_firmware_module full_acpi_module;
> > +
> >      /* Extra ACPI tables passed to HVMLOADER */
> >      struct xc_hvm_firmware_module acpi_module;
> >  
> 
> What is the difference between "ACPI tables" and "Extra ACPI tables"?

The first one is a DSDT table that we generate and compile into AML that is
currently embedded into hvmloader.

The extra one are tables that a user can provide via the "acpi_firmware"
option from xl.

> > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > index 54e096c..2c965c9 100644
> > --- a/tools/libxc/xc_dom_hvmloader.c
> > +++ b/tools/libxc/xc_dom_hvmloader.c
> > @@ -160,6 +160,10 @@ static int modules_init(struct xc_dom_image *dom)
> >  {
> >      int rc;
> [...]
> >      }
> >      else
> >      {
> > +        start_info_size +=
> > +            sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> > +        /* Add extra space to write modules name */
> > +        start_info_size += 10 * HVMLOADER_MODULE_MAX_COUNT;
> 
> Why magic number 10? I guess this is the length limit of the name? Can
> you have a #define somewhere?

I wanted to reserve some space to write the names of the modules without
knowing the actual size. I can add a define for it.

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Tue, Mar 01, 2016 at 11:51:36AM +0000, Wei Liu wrote:
> On Thu, Feb 25, 2016 at 02:56:01PM +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.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> 
> There is already --with-system-seabios and --with-system-ovmf that you
> can use.

The path generated by --with-system-seabios is only for the benefit of the
Makefiles. With this patch, I'm exporting the value to the .c files. And in
the case where --with-system-* is not used, it provide a default path to
where we are going to install the firmware we compiled.

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Tue, Mar 01, 2016 at 11:51:43AM +0000, Wei Liu wrote:
> On Thu, Feb 25, 2016 at 02:56:04PM +0000, Anthony PERARD wrote:
> > A user can provide a different ACPI tables than the default one by using
> > the existing "acpi_firmware" xl's config option or the field
> > u.hvm.acpi_firmware.
> > 
> > libxl will check if the provided table is a DSDT or not.
> > 
> 
> According to xl.cfg manpage, acpi_firmware= cann't be used to override
> DSDT, so you seem to be changing the semantics of existing option.

Yes, that was an idea from Ian Campbell <1446634655.6461.48.camel@citrix.com>
I should at least change the manual.

Would it be OK to reuse this options? Or should I add a new option, maybe
acpi_dsdt_override, or some other name?

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Tue, Mar 01, 2016 at 11:51:40AM +0000, Wei Liu wrote:
> On Thu, Feb 25, 2016 at 02:56:03PM +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_firmware in the domain_build_info struct by other
> > libxl user.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > ---
> > Change in V3:
> > - move seabios_path and ovmf_path to libxl_path.c (with renaming)
> > - fix some coding style
> > - warn for empty file
> > - remove rombios stuff (will still be built-in hvmloader)
> > - rename field bios_filename in domain_build_info to bios_firmware to
> >   follow naming of acpi and smbios.
> > - log an error after libxl_read_file_contents() only when it return ENOENT
> > - return an error on empty file.
> > - added #define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> > ---
> >  tools/libxl/libxl.h          |  8 +++++++
> >  tools/libxl/libxl_dom.c      | 57 ++++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxl_internal.h |  2 ++
> >  tools/libxl/libxl_paths.c    | 10 ++++++++
> >  tools/libxl/libxl_types.idl  |  1 +
> >  tools/libxl/xl_cmdimpl.c     | 11 ++++++---
> 
> You also need to patch manpage for this new option.

Yes, I'll do that.

> How does this new option interacts with bios= option?

That would be the same interaction that there is between
device_model_version and device_model_override.

If someone provide bios_override without bios, the guest could fail to
boot.

> >  6 files changed, 86 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index fa87f53..d223c35 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -876,6 +876,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
> >   */
> >  #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1
> >  
> > +/*
> > + * LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> > + *
> > + * libxl_domain_build_info has u.hvm.bios_firmware field which can be use
> > + * to provide a different bios blob (like SeaBIOS or OVMF).
> > + */
> > +#define LIBXL_HAVE_BUILDINFO_HVM_BIOS_FIRMWARE
> > +
> >  typedef char **libxl_string_list;
> >  void libxl_string_list_dispose(libxl_string_list *sl);
> >  int libxl_string_list_length(const libxl_string_list *sl);
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 2269998..50abfbc 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -863,6 +863,38 @@ err:
> >      return ret;
> >  }
> >  
> > +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) {
> > +        /*
> > +         * Print a message only on ENOENT, other error are logged by the
> > +         * function libxl_read_file_contents().
> > +         */
> > +        if (e == ENOENT)
> > +            LOGEV(ERROR, e, "failed to read %s file", what);
> > +        return ERROR_FAIL;
> > +    }
> > +    libxl__ptr_add(gc, data);
> > +    if (datalen) {
> > +        /* Only accept non-empty files */
> > +        m->data = data;
> > +        m->length = datalen;
> > +    } else {
> > +        LOG(ERROR, "file %s for %s is empty", filename, what);
> > +        return ERROR_FAIL;
> 
> ERROR_INVAL is more appropriate.

OK.

-- 
Anthony PERARD

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

Re: [PATCH v3 11/16] hvmloader: Load OVMF from modules

[Anthony PERARD]

On Tue, Mar 01, 2016 at 09:03:42AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/ovmf.c
> > +++ b/tools/firmware/hvmloader/ovmf.c
> > @@ -34,17 +34,10 @@
> >  #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_qemu_xen[];
> > @@ -97,16 +90,20 @@ 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;
> > +    uint64_t addr = 0x100000000ULL
> > +        - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
> > +    uint64_t ovmf_end = addr + bios_length;
> > +
> > +    ovmf_config.bios_address = addr;
> > +    ovmf_config.image_size = bios_length;
> >  
> >      /* Copy low-reset vector portion. */
> > -    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
> > -           + OVMF_SIZE
> > -           - LOWCHUNK_SIZE,
> > +    memcpy((void *) LOWCHUNK_BEGIN,
> > +           (uint8_t *) bios_addr + bios_length - LOWCHUNK_SIZE,
> >             LOWCHUNK_SIZE);
> >  
> >      /* Ensure we have backing page prior to moving FD. */
> > -    while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
> > +    while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
> >      {
> >          mfn = (uint32_t) (addr >> PAGE_SHIFT);
> >          addr += PAGE_SIZE;
> > @@ -114,7 +111,7 @@ static void ovmf_load(const struct bios_config *config,
> >      }
> >  
> >      /* Copy FD. */
> > -    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> > +    memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
> >  }
> 
> Is this safe, considering that source and destination may now
> overlap? Thinking about it, the same consideration applies to
> BIOS placement below 1Mb too.

It's probably not safe, it just happen to work right now. I guest I can add
some checking, or leave it up to libxc to manage the memory and write this
blob just after hvmloader, like it's done right now.

I think I'll add some bug_on to catch unexpected changes.

-- 
Anthony PERARD

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

Re: [PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module

[Anthony PERARD]

On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote:
> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -365,8 +365,26 @@ int main(void)
> >  
> >          if ( bios->acpi_build_tables )
> >          {
> > +            const struct hvm_modlist_entry *acpi_module;
> > +            acpi_module = get_module_entry(hvm_start_info, "acpi");
> >              printf("Loading ACPI ...\n");
> > -            bios->acpi_build_tables();
> > +            if ( acpi_module )
> > +            {
> > +                uint32_t paddr = acpi_module->paddr;
> > +                bios->acpi_build_tables((void*)paddr,
> > +                                        acpi_module->size);
> > +            }
> 
> Hmm, so far it was the build process which ensured the right ACPI
> tables would be used with the corresponding BIOS. The disconnect
> that gets introduced here worries me a little, since things having
> got out of sync may be rather hard to diagnose (as they may
> surface only much later).

So, my ultimate goal with this series was to be able to create a guest with
QEMU's Q35 machine, which would need a different ACPI tables.

Also, I would say that the ACPI tables are already disconnected from the
thing they describe, the device model QEMU. I don't think there is much
information about the BIOS backed into the DSDT table.

> > +#ifdef ENABLE_ROMBIOS
> > +            else if ( bios == &rombios_config )
> > +            {
> > +                bios->acpi_build_tables(0, 0);
> > +            }
> > +#endif
> > +            else
> > +            {
> > +                printf("no ACPI DSDT image found\n");
> > +                BUG();
> > +            }
> 
> Is this really fatal? It's not like "no ACPI" == "end of the world",
> is it?

I guest it's not fatal, I'll give it a try and just print a warning.

-- 
Anthony PERARD

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

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

[Anthony PERARD]

In this series, there are plenty of places where one blob loaded by libxl
to be consume by hvmloader is called acpi_module or acpi_table... where in
fact it is only the DSDT table. I think I'm going to do some renaming to
include "dsdt" into those names.

-- 
Anthony PERARD

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

Re: [PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module

[Jan Beulich]

>>> On 03.03.16 at 18:59, <anthony.perard@citrix.com> wrote:
> On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote:
>> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -365,8 +365,26 @@ int main(void)
>> >  
>> >          if ( bios->acpi_build_tables )
>> >          {
>> > +            const struct hvm_modlist_entry *acpi_module;
>> > +            acpi_module = get_module_entry(hvm_start_info, "acpi");
>> >              printf("Loading ACPI ...\n");
>> > -            bios->acpi_build_tables();
>> > +            if ( acpi_module )
>> > +            {
>> > +                uint32_t paddr = acpi_module->paddr;
>> > +                bios->acpi_build_tables((void*)paddr,
>> > +                                        acpi_module->size);
>> > +            }
>> 
>> Hmm, so far it was the build process which ensured the right ACPI
>> tables would be used with the corresponding BIOS. The disconnect
>> that gets introduced here worries me a little, since things having
>> got out of sync may be rather hard to diagnose (as they may
>> surface only much later).
> 
> So, my ultimate goal with this series was to be able to create a guest with
> QEMU's Q35 machine, which would need a different ACPI tables.
> 
> Also, I would say that the ACPI tables are already disconnected from the
> thing they describe, the device model QEMU. I don't think there is much
> information about the BIOS backed into the DSDT table.

But then why would the Q35 model need a different one? Or the
other way around, why wouldn't that other one be usable with
with the current machine type (or more generally, why couldn't
we have one that fits all machine types we mean to support)?

Jan


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

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

[Andrew Cooper]

On 03/03/16 18:03, Anthony PERARD wrote:
> In this series, there are plenty of places where one blob loaded by libxl
> to be consume by hvmloader is called acpi_module or acpi_table... where in
> fact it is only the DSDT table. I think I'm going to do some renaming to
> include "dsdt" into those names.

The DSDT cannot possibly come from anywhere other than hvmloader (as a
logic extension of Xen).  It is very hardware specific, including bits
of ACPI emulated by Xen itself.

There are plenty of improvements which can be made over the current
status quo by splitting out the optional parts into extra SSDTs, but
having the DSDT itself come from another source will cause all kinds of
problems for the domain.

~Andrew

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

Re: [PATCH v3 11/16] hvmloader: Load OVMF from modules

[Wei Liu]

On Thu, Mar 03, 2016 at 05:39:50PM +0000, Anthony PERARD wrote:
> On Tue, Mar 01, 2016 at 09:03:42AM -0700, Jan Beulich wrote:
> > >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > > --- a/tools/firmware/hvmloader/ovmf.c
> > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > @@ -34,17 +34,10 @@
> > >  #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_qemu_xen[];
> > > @@ -97,16 +90,20 @@ 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;
> > > +    uint64_t addr = 0x100000000ULL
> > > +        - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
> > > +    uint64_t ovmf_end = addr + bios_length;
> > > +
> > > +    ovmf_config.bios_address = addr;
> > > +    ovmf_config.image_size = bios_length;
> > >  
> > >      /* Copy low-reset vector portion. */
> > > -    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
> > > -           + OVMF_SIZE
> > > -           - LOWCHUNK_SIZE,
> > > +    memcpy((void *) LOWCHUNK_BEGIN,
> > > +           (uint8_t *) bios_addr + bios_length - LOWCHUNK_SIZE,
> > >             LOWCHUNK_SIZE);
> > >  
> > >      /* Ensure we have backing page prior to moving FD. */
> > > -    while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
> > > +    while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
> > >      {
> > >          mfn = (uint32_t) (addr >> PAGE_SHIFT);
> > >          addr += PAGE_SIZE;
> > > @@ -114,7 +111,7 @@ static void ovmf_load(const struct bios_config *config,
> > >      }
> > >  
> > >      /* Copy FD. */
> > > -    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> > > +    memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
> > >  }
> > 
> > Is this safe, considering that source and destination may now
> > overlap? Thinking about it, the same consideration applies to
> > BIOS placement below 1Mb too.
> 
> It's probably not safe, it just happen to work right now. I guest I can add
> some checking, or leave it up to libxc to manage the memory and write this
> blob just after hvmloader, like it's done right now.
> 
> I think I'll add some bug_on to catch unexpected changes.
> 

If the only concern is overlapping region (not overwriting something
that needs to be used later), using memmove should be good enough?

Wei.

> -- 
> Anthony PERARD

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

Re: [PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module

[Anthony PERARD]

On Fri, Mar 04, 2016 at 01:39:38AM -0700, Jan Beulich wrote:
> >>> On 03.03.16 at 18:59, <anthony.perard@citrix.com> wrote:
> > On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote:
> >> >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> >> > --- a/tools/firmware/hvmloader/hvmloader.c
> >> > +++ b/tools/firmware/hvmloader/hvmloader.c
> >> > @@ -365,8 +365,26 @@ int main(void)
> >> >  
> >> >          if ( bios->acpi_build_tables )
> >> >          {
> >> > +            const struct hvm_modlist_entry *acpi_module;
> >> > +            acpi_module = get_module_entry(hvm_start_info, "acpi");
> >> >              printf("Loading ACPI ...\n");
> >> > -            bios->acpi_build_tables();
> >> > +            if ( acpi_module )
> >> > +            {
> >> > +                uint32_t paddr = acpi_module->paddr;
> >> > +                bios->acpi_build_tables((void*)paddr,
> >> > +                                        acpi_module->size);
> >> > +            }
> >> 
> >> Hmm, so far it was the build process which ensured the right ACPI
> >> tables would be used with the corresponding BIOS. The disconnect
> >> that gets introduced here worries me a little, since things having
> >> got out of sync may be rather hard to diagnose (as they may
> >> surface only much later).
> > 
> > So, my ultimate goal with this series was to be able to create a guest with
> > QEMU's Q35 machine, which would need a different ACPI tables.
> > 
> > Also, I would say that the ACPI tables are already disconnected from the
> > thing they describe, the device model QEMU. I don't think there is much
> > information about the BIOS backed into the DSDT table.
> 
> But then why would the Q35 model need a different one? Or the
> other way around, why wouldn't that other one be usable with
> with the current machine type (or more generally, why couldn't
> we have one that fits all machine types we mean to support)?

I have not though about using the same one for both. I could try to change
the tables we have to make it work with both. But that for another time.

Thanks,

-- 
Anthony PERARD

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

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

[Anthony PERARD]

On Fri, Mar 04, 2016 at 10:57:59AM +0000, Andrew Cooper wrote:
> On 03/03/16 18:03, Anthony PERARD wrote:
> > In this series, there are plenty of places where one blob loaded by libxl
> > to be consume by hvmloader is called acpi_module or acpi_table... where in
> > fact it is only the DSDT table. I think I'm going to do some renaming to
> > include "dsdt" into those names.
> 
> The DSDT cannot possibly come from anywhere other than hvmloader (as a
> logic extension of Xen).  It is very hardware specific, including bits
> of ACPI emulated by Xen itself.
> 
> There are plenty of improvements which can be made over the current
> status quo by splitting out the optional parts into extra SSDTs, but
> having the DSDT itself come from another source will cause all kinds of
> problems for the domain.

Ok.

I think I leave the acpi tables alone in the next revision of the series
and only load the BIOS.

-- 
Anthony PERARD

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

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

[Wei Liu]

On Thu, Mar 03, 2016 at 05:03:00PM +0000, Anthony PERARD wrote:
> On Tue, Mar 01, 2016 at 11:51:36AM +0000, Wei Liu wrote:
> > On Thu, Feb 25, 2016 at 02:56:01PM +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.
> > > 
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > 
> > 
> > There is already --with-system-seabios and --with-system-ovmf that you
> > can use.
> 
> The path generated by --with-system-seabios is only for the benefit of the
> Makefiles. With this patch, I'm exporting the value to the .c files. And in
> the case where --with-system-* is not used, it provide a default path to
> where we are going to install the firmware we compiled.
> 

I see. SEABIOS_PATH and OVMF_PATH are already defined in Tools.mk.in.
This approach is fine then.

Wei.

> -- 
> Anthony PERARD

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

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

[Wei Liu]

On Thu, Mar 03, 2016 at 05:12:07PM +0000, Anthony PERARD wrote:
> On Tue, Mar 01, 2016 at 11:51:43AM +0000, Wei Liu wrote:
> > On Thu, Feb 25, 2016 at 02:56:04PM +0000, Anthony PERARD wrote:
> > > A user can provide a different ACPI tables than the default one by using
> > > the existing "acpi_firmware" xl's config option or the field
> > > u.hvm.acpi_firmware.
> > > 
> > > libxl will check if the provided table is a DSDT or not.
> > > 
> > 
> > According to xl.cfg manpage, acpi_firmware= cann't be used to override
> > DSDT, so you seem to be changing the semantics of existing option.
> 
> Yes, that was an idea from Ian Campbell <1446634655.6461.48.camel@citrix.com>
> I should at least change the manual.
> 
> Would it be OK to reuse this options? Or should I add a new option, maybe
> acpi_dsdt_override, or some other name?
> 

If repurposing the old option won't break existing guest then that's
fine, otherwise a new option is required.

Wei.

> -- 
> Anthony PERARD

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