| Message ID | 1502310866-10450-9-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 04:34:09PM -0400, Lan Tianyu wrote: > From: Chao Gao <chao.gao@intel.com> [...] > -=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.
On Wed, Aug 09, 2017 at 04:34:09PM -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. > > Signed-off-by: Chao Gao <chao.gao@intel.com> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > 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.
On 2017年08月22日 21:19, Wei Liu wrote: >> +=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? "base address" and "length" of vIOMMU register region is low level vIOMMU configuration. I am afraid users is vary hard to determine which region is suitable for vIOMMU and doesn't conflict with other device model. > >> + >> > +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? > >> > + 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. OK. will update.
On Wed, Aug 23, 2017 at 10:46:13AM +0800, Lan Tianyu wrote: > On 2017年08月22日 21:19, Wei Liu wrote: > >> +=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? > > "base address" and "length" of vIOMMU register region is low level > vIOMMU configuration. I am afraid users is vary hard to determine which > region is suitable for vIOMMU and doesn't conflict with other device model. That's fair. Assuming those two values are hardware specific (as I read in another sub-thread) I'm fine with not exposing them (should they be needed at all).
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 + +=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". + +=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 + #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; + + /* Set desired capbilities */ + viommu->cap = VIOMMU_CAP_IRQ_REMAPPING; + } + return 0; +} + void parse_config_data(const char *config_source, const char *config_data, int config_len, @@ -1037,6 +1096,13 @@ 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_string(config, "viommu", &buf, 0)) { + if (parse_viommu_config(&b_info->viommu, buf)) { + fprintf(stderr, "ERROR: invalid viommu setting\n"); + exit (1); + } + } + switch(b_info->type) { case LIBXL_DOMAIN_TYPE_HVM: kernel_basename = libxl_basename(b_info->kernel);