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

[Ian Campbell <ian.campbell@citrix.com>]

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

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

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

Ian.

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