Re: [PATCH V3 6/29] tools/libxl: Add a user configurable parameter to control vIOMMU attributes

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Sep 21, 2017 at 11:01:47PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@intel.com>
> 
> A field, viommu_info, is added to struct libxl_domain_build_info. Several
> attributes can be specified by guest config file for virtual IOMMU. These
> attributes are used for DMAR construction and vIOMMU creation.

IMHO this should come much later in the series, ideally you would
introduce the xl/libxl code in the last patches, together with the
xl.cfg man page change.

> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> ---
> v3:
>  - allow an array of viommu other than only one viommu to present to guest.
>  During domain building, an error would be raised for
>  multiple viommus case since we haven't implemented this yet.
>  - provide a libxl__viommu_set_default() for viommu
> 
> ---
>  docs/man/xl.cfg.pod.5.in    | 27 +++++++++++++++++++++++
>  tools/libxl/libxl_create.c  | 52 +++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl | 12 +++++++++++
>  tools/xl/xl_parse.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 142 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 79cb2ea..9cd7dd7 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1547,6 +1547,33 @@ L<http://www.microsoft.com/en-us/download/details.aspx?id=30707>
>  
>  =back 
>  
> +=item B<viommu=[ "VIOMMU_STRING", "VIOMMU_STRING", ...]>
> +
> +Specifies the vIOMMUs which are to be provided to the guest.
> +
> +B<VIOMMU_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where:
> +
> +=over 4
> +
> +=item B<KEY=VALUE>
> +
> +Possible B<KEY>s are:
> +
> +=over 4
> +
> +=item B<type="STRING">
> +
> +Currently there is only one valid type:
> +
> +(x86 only) "intel_vtd" means providing a emulated Intel VT-d to the guest.
> +
> +=item B<intremap=BOOLEAN>
> +
> +Specifies whether the vIOMMU should support interrupt remapping
> +and default 'true'.
> +
> +=back
> +
>  =head3 Guest Virtual Time Controls
>  
>  =over 4
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 9123585..decd7a8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -27,6 +27,8 @@
>  
>  #include <xen-xsm/flask/flask.h>
>  
> +#define VIOMMU_VTD_BASE_ADDR        0xfed90000ULL

This should be in libxl_arch.h see LAPIC_BASE_ADDRESS.

> +
>  int libxl__domain_create_info_setdefault(libxl__gc *gc,
>                                           libxl_domain_create_info *c_info)
>  {
> @@ -59,6 +61,47 @@ void libxl__rdm_setdefault(libxl__gc *gc, libxl_domain_build_info *b_info)
>                              LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
>  }
>  
> +static int libxl__viommu_set_default(libxl__gc *gc,
> +                                     libxl_domain_build_info *b_info)
> +{
> +    int i;
> +
> +    if (!b_info->num_viommus)
> +        return 0;
> +
> +    for (i = 0; i < b_info->num_viommus; i++) {
> +        libxl_viommu_info *viommu = &b_info->viommu[i];
> +
> +        if (libxl_defbool_is_default(viommu->intremap))
> +            libxl_defbool_set(&viommu->intremap, true);
> +
> +        if (!libxl_defbool_val(viommu->intremap)) {
> +            LOGE(ERROR, "Cannot create one virtual VTD without intremap");
> +            return ERROR_INVAL;
> +        }
> +
> +        if (viommu->type == LIBXL_VIOMMU_TYPE_INTEL_VTD) {
> +            /*
> +             * If there are multiple vIOMMUs, we need arrange all vIOMMUs to
> +             * avoid overlap. Put a check here in case we get here for multiple
> +             * vIOMMUs case.
> +             */
> +            if (b_info->num_viommus > 1) {
> +                LOGE(ERROR, "Multiple vIOMMUs support is under implementation");

s/LOGE/LOG/ LOGE should only be used when errno is set (which is not
the case here).

> +                return ERROR_INVAL;
> +            }
> +
> +            /* Set default values to unexposed fields */
> +            viommu->base_addr = VIOMMU_VTD_BASE_ADDR;
> +
> +            /* Set desired capbilities */
> +            viommu->cap = VIOMMU_CAP_IRQ_REMAPPING;

I'm not sure whether this code should be in libxl_x86.c, but
libxl__domain_build_info_setdefault is already quite messed up, so I
guess it's fine.

> +        }

Shouldn't this be:

switch(viommu->type) {
case LIBXL_VIOMMU_TYPE_INTEL_VTD:
    ...
    break;

default:
    return ERROR_INVAL;
}

So that you catch type being set to an invalid vIOMMU type?

> +    }
> +
> +    return 0;
> +}
> +
>  int libxl__domain_build_info_setdefault(libxl__gc *gc,
>                                          libxl_domain_build_info *b_info)
>  {
> @@ -214,6 +257,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>  
>      libxl__arch_domain_build_info_acpi_setdefault(b_info);
>  
> +    if (libxl__viommu_set_default(gc, b_info))
> +        return ERROR_FAIL;
> +
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>          if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -890,6 +936,12 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +    if (d_config->b_info.num_viommus > 1) {
> +        ret = ERROR_INVAL;
> +        LOGD(ERROR, domid, "Cannot support multiple vIOMMUs");
> +        goto error_out;
> +    }

Er, you already have this check in libxl__viommu_set_default, and in
any case I would just rely on the hypervisor failing to create more
than one vIOMMU per domain, rather than adding the same check here.

> +
>      ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
>      if (ret) {
>          LOGD(ERROR, domid, "Unable to set domain create info defaults");
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 173d70a..286c960 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -450,6 +450,17 @@ libxl_altp2m_mode = Enumeration("altp2m_mode", [
>      (3, "limited"),
>      ], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
>  
> +libxl_viommu_type = Enumeration("viommu_type", [
> +    (1, "intel_vtd"),
> +    ])
> +
> +libxl_viommu_info = Struct("viommu_info", [
> +    ("type",            libxl_viommu_type),
> +    ("intremap",        libxl_defbool),
> +    ("cap",             uint64),
> +    ("base_addr",       uint64),
> +    ])
> +
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("avail_vcpus",     libxl_bitmap),
> @@ -506,6 +517,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      # 65000 which is reserved by the toolstack.
>      ("device_tree",      string),
>      ("acpi",             libxl_defbool),
> +    ("viommu",           Array(libxl_viommu_info, "num_viommus")),
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",         string),
>                                         ("bios",             libxl_bios_type),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 02ddd2e..34f8128 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -804,6 +804,38 @@ int parse_usbdev_config(libxl_device_usbdev *usbdev, char *token)
>      return 0;
>  }
>  
> +/* Parses viommu data and adds info into viommu
> + * Returns 1 if the input doesn't form a valid viommu
> + * or parsed values are not correct. Successful parse returns 0 */
> +static int parse_viommu_config(libxl_viommu_info *viommu, const char *info)
> +{
> +    char *ptr, *oparg, *saveptr = NULL, *buf = xstrdup(info);
> +
> +    ptr = strtok_r(buf, ",", &saveptr);
> +    if (MATCH_OPTION("type", ptr, oparg)) {
> +        if (!strcmp(oparg, "intel_vtd")) {
> +            viommu->type = LIBXL_VIOMMU_TYPE_INTEL_VTD;
> +        } else {
> +            fprintf(stderr, "Invalid viommu type: %s\n", oparg);
> +            return 1;
> +        }
> +    } else {
> +        fprintf(stderr, "viommu type should be set first: %s\n", oparg);
> +        return 1;
> +    }
> +
> +    for (ptr = strtok_r(NULL, ",", &saveptr); ptr;
> +         ptr = strtok_r(NULL, ",", &saveptr)) {
> +        if (MATCH_OPTION("intremap", ptr, oparg)) {
> +            libxl_defbool_set(&viommu->intremap, !!strtoul(oparg, NULL, 0));

No need for the !!.

> +        } else {
> +            fprintf(stderr, "Unknown string `%s' in viommu spec\n", ptr);
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  void parse_config_data(const char *config_source,
>                         const char *config_data,
>                         int config_len,
> @@ -813,7 +845,7 @@ void parse_config_data(const char *config_source,
>      long l, vcpus = 0;
>      XLU_Config *config;
>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
> -                   *usbctrls, *usbdevs, *p9devs;
> +                   *usbctrls, *usbdevs, *p9devs, *iommus;
>      XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
>                     *mca_caps;
>      int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
> @@ -1037,6 +1069,24 @@ void parse_config_data(const char *config_source,
>      xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
>      xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0);
>  
> +    if (!xlu_cfg_get_list (config, "viommu", &iommus, 0, 0)) {
> +        b_info->num_viommus = 0;
> +        b_info->viommu = NULL;

This should not be needed, num_viommus and viommu should already be
zeroed by the initialize functions.

Thanks, Roger.

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