Re: [Xen-devel] [PATCH 9/25] Tools/libacpi: Add a user configurable parameter to control vIOMMU attributes

[Wei Liu <wei.liu2@xxxxxxxxxx>]

Title is wrong. Should be tagged "tools".

On Thu, Jun 29, 2017 at 01:50:41AM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> a field, viommu_info, is added to struct libxl_domain_build_info. Several

Capitalise "a" please.

> attributes can be specified by guest configuration file for the DMAR table
> building and vIOMMU creation.
> 
> In domain creation process, a new logic is added to build ACPI DMAR table in
> tool stack according VM configuration and to pass though it to hvmloader via
> xenstore ACPI PT channel. If there are ACPI tables needed to pass through, we
> joint the tables.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  docs/man/xl.cfg.pod.5.in    | 34 +++++++++++++++++-
>  tools/libacpi/build.c       |  5 +++
>  tools/libacpi/libacpi.h     |  1 +

Can the changes to libacpi be split out to a separate patch?

>  tools/libxl/libxl_dom.c     | 87 
> +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl | 10 ++++++
>  tools/xl/xl_parse.c         | 64 +++++++++++++++++++++++++++++++++
>  6 files changed, 200 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 38084c7..874f3f2 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
[...]
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index 6a4e1cf..0a58d6f 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -109,6 +109,7 @@ struct acpi_config {
>  #define DMAR_X2APIC_OPT_OUT 0x2
>  struct acpi_dmar *construct_dmar(struct acpi_ctxt *ctxt,
>                                   const struct acpi_config *config);
> +uint32_t acpi_get_table_size(struct acpi_header * header);
>  int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config);
>  
>  #endif /* __LIBACPI_H__ */
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5d914a5..f8d61c2 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -19,11 +19,13 @@
>  
>  #include "libxl_internal.h"
>  #include "libxl_arch.h"
> +#include "libacpi/libacpi.h"
>  
>  #include <xc_dom.h>
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/e820.h>
> +#include <xen/viommu.h>
>  
>  #include "_paths.h"
>  
> @@ -925,6 +927,43 @@ out:
>      return rc;
>  }
>  
> +static unsigned long acpi_v2p(struct acpi_ctxt *ctxt, void *v)
> +{
> +    return (unsigned long)v;
> +}
> +
> +static void *acpi_mem_alloc(struct acpi_ctxt *ctxt,
> +                            uint32_t size, uint32_t align)
> +{
> +    return aligned_alloc(align, size);

posix_memalign please.

aligned_alloc is in C11. We support C99.

> +}
> +
> +static void acpi_mem_free(struct acpi_ctxt *ctxt,
> +                          void *v, uint32_t size)
> +{
> +    /* ACPI builder currently doesn't free memory so this is just a stub */
> +}
> +
> +static int libxl__acpi_build_dmar(libxl__gc *gc,
> +                                  struct acpi_config *config,
> +                                  void **data_r, int *datalen_r)

This should probably go to libxl_x86_acpi.

It seems that you should be able to use a lot of code there instead of
writing your own.

In any case, I don't think having two places to construct acpi tables
for guest is a good idea.

> +{
> +    struct acpi_ctxt ctxt;
> +    void *table;
> +
> +    ctxt.mem_ops.alloc = acpi_mem_alloc;
> +    ctxt.mem_ops.free = acpi_mem_free;
> +    ctxt.mem_ops.v2p = acpi_v2p;
> +
> +    table = construct_dmar(&ctxt, config);
> +    if ( !table )
> +        return ERROR_FAIL;
> +
> +    *data_r = table;
> +    *datalen_r = acpi_get_table_size((struct acpi_header *)table);
> +    return 0;
> +}
> +
>  static int libxl__domain_firmware(libxl__gc *gc,
>                                    libxl_domain_build_info *info,
>                                    struct xc_dom_image *dom)
> @@ -1045,6 +1084,54 @@ static int libxl__domain_firmware(libxl__gc *gc,
>          }
>      }
>  
> +    /* build DMAR table according guest configuration and joint it with other
> +     * apci tables specified by acpi_modules */
> +    if ((info->u.hvm.viommu.type == VIOMMU_TYPE_INTEL_VTD) &&
> +        !libxl_defbool_is_default(info->u.hvm.viommu.intremap) &&
> +        info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +        struct acpi_config config;
> +
> +        memset(&config, 0, sizeof(config));
> +        if (libxl_defbool_val(info->u.hvm.viommu.intremap)) {
> +            config.table_flags |= ACPI_HAS_DMAR;
> +            config.dmar_flag = DMAR_INTR_REMAP;
> +            if (!libxl_defbool_is_default(info->u.hvm.viommu.x2apic)
> +                && !libxl_defbool_val(info->u.hvm.viommu.x2apic))
> +                config.dmar_flag |= DMAR_X2APIC_OPT_OUT;
> +
> +            config.viommu_base_addr = info->u.hvm.viommu.base_addr;
> +            data = NULL;
> +            e = libxl__acpi_build_dmar(gc, &config, &data, &datalen);
> +            if (e) {
> +                LOGE(ERROR, "failed to build DMAR table");
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +
> +            libxl__ptr_add(gc, data);
> +            if (datalen) {
> +                if (!dom->acpi_modules[0].data) {
> +                    dom->acpi_modules[0].data = data;
> +                    dom->acpi_modules[0].length = (uint32_t)datalen;
> +                } else {
> +                    /* joint tables */
> +                    void *newdata;
> +                    newdata = malloc(datalen + dom->acpi_modules[0].length);
> +                    if (!newdata) {
> +                        LOGE(ERROR, "failed to joint DMAR table to acpi 
> modules");
> +                        rc = ERROR_FAIL;
> +                        goto out;
> +                    }
> +                    memcpy(newdata, dom->acpi_modules[0].data,
> +                           dom->acpi_modules[0].length);
> +                    memcpy(newdata + dom->acpi_modules[0].length, data, 
> datalen);
> +                    dom->acpi_modules[0].data = newdata;
> +                    dom->acpi_modules[0].length += (uint32_t)datalen;
> +                }
> +            }
> +        }
> +    }
> +

This seems to clash badly with the code in libxl__dom_load_acpi which
also writes to modules[0].

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