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

[Wei Liu]

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 
> ---
> 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_MODULES4
> +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, >system_firmware_module,
>   "System Firmware module");
>  if ( rc ) goto err;
> -rc = module_init_one(dom, >acpi_module, "ACPI module");
> +/* Only one module can be added */
> +rc = module_init_one(dom, >acpi_modules[0], "ACPI module");
>  if ( rc ) goto err;
>  rc = module_init_one(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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

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 
---
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_MODULES4
+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, >system_firmware_module,
  "System Firmware module");
 if ( rc ) goto err;
-rc = module_init_one(dom, >acpi_module, "ACPI module");
+/* Only one module can be added */
+rc = module_init_one(dom, >acpi_modules[0], "ACPI module");
 if ( rc ) goto err;
 rc = module_init_one(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)
 {
 char *path = NULL;
 int ret = 0;
@@ -839,18 +840,20 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
 goto err;
 }
 
-if (dom->acpi_module.guest_addr_out) {
+/* Only one module can be passed. PVHv2 guests do not support this. */
+if (dom->acpi_modules[0].guest_addr_out && 
+info->device_model_version !=LIBXL_DEVICE_MODEL_VERSION_NONE) {
 path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_ADDRESS, domid);
 
 ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%"PRIx64,
-   dom->acpi_module.guest_addr_out);
+   dom->acpi_modules[0].guest_addr_out);
 if (ret)
 goto err;
 
 path = GCSPRINTF("/local/domain/%d/"HVM_XS_ACPI_PT_LENGTH, domid);
 
 ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%x",
-   dom->acpi_module.length);
+   dom->acpi_modules[0].length);
 if (ret)
 goto err;
 }
@@ -994,6 +997,13 @@ static int libxl__domain_firmware(libxl__gc *gc,
 }
 
 if (info->u.hvm.acpi_firmware) {
+
+if (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE) {
+LOGE(ERROR, "PVH guests do not allow loading ACPI modules");
+rc = ERROR_FAIL;
+goto out;
+}
+
 data = NULL;
 e = libxl_read_file_contents(ctx, info->u.hvm.acpi_firmware,
  , );
@@ -1005,9 +1015,9 @@ static int libxl__domain_firmware(libxl__gc *gc,
 }
 libxl__ptr_add(gc, data);
 if (datalen) {
-/* Only accept non-empty files */
-dom->acpi_module.data = data;
-dom->acpi_module.length = (uint32_t)datalen;
+/* Only accept a non-empty file */
+dom->acpi_modules[0].data = data;
+dom->acpi_modules[0].length = (uint32_t)datalen;
 }
 }
 
@@ -1143,7 +1153,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
 goto out;
 }
 
-rc = hvm_build_set_xs_values(gc, domid, dom);
+rc = hvm_build_set_xs_values(gc, domid, dom, info);
 if (rc != 0) {
 LOG(ERROR, "hvm build set