Re: [Xen-devel] [PATCH V2 8/25] tools/libxl: Add a user configurable parameter to control vIOMMU attributes

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:09PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
[...]
> -=back 
> +=back
> +
> +=item B<viommu="VIOMMU_STRING">
> +
> +Specifies the vIOMMU 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'.
> +
> +=item B<x2apic=BOOLEAN>
> +
> +Specifies whether the vIOMMU should support x2apic mode and default 'true'.
> +Only valid for "intel_vtd".

Why not expose base address and length as well?

> +
> +=back
>  
>  =head3 Guest Virtual Time Controls
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 8a9849c..7abd70c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -450,6 +450,21 @@ 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", [
> +    ("u", KeyedUnion(None, libxl_viommu_type, "type",
> +           [("intel_vtd", Struct(None, [
> +                 ("x2apic",     libxl_defbool)]))
> +           ])),
> +    ("intremap",        libxl_defbool),
> +    ("cap",             uint64),
> +    ("base_addr",       uint64),
> +    ("len",             uint64),
> +    ])
> +
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("avail_vcpus",     libxl_bitmap),
> @@ -506,6 +521,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      # 65000 which is reserved by the toolstack.
>      ("device_tree",      string),
>      ("acpi",             libxl_defbool),
> +    ("viommu",           libxl_viommu_info),

An array please, we shouldn't limit the number of 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 5c2bf17..11c4eb2 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -17,6 +17,7 @@
>  #include <limits.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <xen/domctl.h>

Why is this needed?

>  #include <xen/hvm/e820.h>
>  #include <xen/hvm/params.h>
>  
> @@ -30,6 +31,9 @@
>  
>  extern void set_default_nic_values(libxl_device_nic *nic);
>  
> +#define VIOMMU_VTD_BASE_ADDR        0xfed90000ULL
> +#define VIOMMU_VTD_REGISTER_LEN     0x1000ULL
> +
>  #define ARRAY_EXTEND_INIT__CORE(array,count,initfn,more)                \
>      ({                                                                  \
>          typeof((count)) array_extend_old_count = (count);               \
> @@ -804,6 +808,61 @@ 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;
> +    }
> +
> +    ptr = strtok_r(NULL, ",", &saveptr);
> +    if (MATCH_OPTION("intremap", ptr, oparg)) {
> +        libxl_defbool_set(&viommu->intremap, !!strtoul(oparg, NULL, 0));
> +    }
> +
> +    if (viommu->type == LIBXL_VIOMMU_TYPE_INTEL_VTD) {
> +        for (ptr = strtok_r(NULL, ",", &saveptr); ptr;
> +             ptr = strtok_r(NULL, ",", &saveptr)) {
> +            if (MATCH_OPTION("x2apic", ptr, oparg)) {
> +                libxl_defbool_set(&viommu->u.intel_vtd.x2apic,
> +                                  !!strtoul(oparg, NULL, 0));
> +            } else {
> +                fprintf(stderr, "Unknown string `%s' in viommu spec\n", ptr);
> +                return 1;
> +            }
> +        }
> +
> +        if (libxl_defbool_is_default(viommu->intremap))
> +            libxl_defbool_set(&viommu->intremap, true);
> +
> +        if (!libxl_defbool_val(viommu->intremap)) {
> +            fprintf(stderr, "Cannot create one virtual VTD without 
> intremap\n");
> +            return 1;
> +        }
> +
> +        /* Set default values to unexposed fields */
> +        viommu->base_addr = VIOMMU_VTD_BASE_ADDR;
> +        viommu->len = VIOMMU_VTD_REGISTER_LEN;
> +

You're doing this is xl. This is not right. The default value should be
set from within libxl.

You should have a libxl_XXX_setdefault function for this type.

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