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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:09PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> 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.
> 
> 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/libxl/libxl_types.idl | 16 +++++++++++
>  tools/xl/xl_parse.c         | 66 
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 79cb2ea..f259e22 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1545,7 +1545,39 @@ Do not provide a VM generation ID.
>  See also "Virtual Machine Generation ID" by Microsoft:
>  L<http://www.microsoft.com/en-us/download/details.aspx?id=30707>
>  
> -=back 
> +=back

No spurious changes. Leave the extra " " as is.

> +
> +=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:

Should be an array of VIOMMU_STRINGs instead.

> +=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.
                                          ^ an
> +
> +=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".
> +
> +=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),
>      ("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>
>  #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

I don't think those defines should be here at all.

>  #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;
> +        }

Why is that an option anyway if it's not possible to create an IOMMU
without intremap anyway?

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

This should be set in libxl__domain_build_info_setdefault IMHO.

Roger.

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