| Message ID | 1506049330-11196-7-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Thu, Oct 19, 2017 at 10:49:22AM +0100, Roger Pau Monné wrote: >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. It can be put to the end of this series. But I prefer to introduce the vIOMMU from up to down (means the use interface goes first and then how to implement a vIOMMU step by step) for it may be easier to understand. > >> 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. Agree. > >> + >> 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). yes. > >> + 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? sure. Will update. > >> + 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. It is fine to me. Will remove all checks against viommu numbers in toolstack. Thanks chao
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 + 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"); + 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; + } + } + + 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; + } + 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)); + } 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; + while ((buf = xlu_cfg_get_listitem (iommus, b_info->num_viommus)) + != NULL) { + libxl_viommu_info *viommu; + + viommu = ARRAY_EXTEND_INIT_NODEVID(b_info->viommu, + b_info->num_viommus, + libxl_viommu_info_init); + + if (parse_viommu_config(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);