Re: [Xen-devel] [PATCH v2 18/23] libxc/libxl: Allow multiple ACPI modules

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 04, 2016 at 05:06:46PM -0400, Boris Ostrovsky wrote:
> Provide ability to load multiple ACPI modules. Thie feature is needed
> by PVHv2 guests and will be used in subsequent patches.
> 
> We assume that PVHv2 guests do not load their ACPI modules specified
> in the configuration file. We can extend support for that in the future
> if desired.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
> v2:
> * New patch. PVH uses 3 modules:
>     - rsdp (8 bytes that store RSDT address)
>     - acpi_info (1 page, Xen-specific)
>     - ACPI tables (multiple pages)
>   As an alternative, we could add rsdp_val and rsdp_ptr and
>   keep acpi_module as a scalar (acpi_info and ACPI tables are
>   usually laid out contiguously). This patch provides a more
>   general solution (IMO).
> 
>  tools/libxc/include/xc_dom.h   |  5 +++--
>  tools/libxc/xc_dom_hvmloader.c |  3 ++-
>  tools/libxl/libxl_dom.c        | 26 ++++++++++++++++++--------
>  3 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index de7dca9..608cbc2 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -212,8 +212,9 @@ struct xc_dom_image {
>      /* BIOS/Firmware passed to HVMLOADER */
>      struct xc_hvm_firmware_module system_firmware_module;
>  
> -    /* Extra ACPI tables passed to HVMLOADER */
> -    struct xc_hvm_firmware_module acpi_module;
> +    /* Extra ACPI tables */
> +#define MAX_ACPI_MODULES        4
> +    struct xc_hvm_firmware_module acpi_modules[MAX_ACPI_MODULES];
>  
>      /* Extra SMBIOS structures passed to HVMLOADER */
>      struct xc_hvm_firmware_module smbios_module;
> diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> index 6eb8516..59f94e5 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -172,7 +172,8 @@ static int modules_init(struct xc_dom_image *dom)
>      rc = module_init_one(dom, &dom->system_firmware_module,
>                           "System Firmware module");
>      if ( rc ) goto err;
> -    rc = module_init_one(dom, &dom->acpi_module, "ACPI module");
> +    /* Only one module can be added */
> +    rc = module_init_one(dom, &dom->acpi_modules[0], "ACPI module");
>      if ( rc ) goto err;
>      rc = module_init_one(dom, &dom->smbios_module, "SMBIOS module");
>      if ( rc ) goto err;
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 2a1793d..22d6868 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -818,7 +818,8 @@ static int hvm_build_set_params(xc_interface *handle, 
> uint32_t domid,
>  
>  static int hvm_build_set_xs_values(libxl__gc *gc,
>                                     uint32_t domid,
> -                                   struct xc_dom_image *dom)
> +                                   struct xc_dom_image *dom,
> +                                   libxl_domain_build_info *info)

My slight preference is that if only device_model_version is needed you
pass that directly to this function. But I don't feel too strong about
this.  If you're going to pass info please constify it.

Code-wise, looks good.

Wei.

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