| Message ID | 20190730134419.2739-2-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | per-domain IOMMU control | expand |
On 30.07.2019 15:44, Paul Durrant wrote: > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -673,8 +673,7 @@ int arch_domain_create(struct domain *d, > > ASSERT(config != NULL); > > - /* p2m_init relies on some value initialized by the IOMMU subsystem */ > - if ( (rc = iommu_domain_init(d)) != 0 ) > + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) > goto fail; Instead of this and ... > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -604,7 +604,7 @@ int arch_domain_create(struct domain *d, > if ( (rc = init_domain_irq_mapping(d)) != 0 ) > goto fail; > > - if ( (rc = iommu_domain_init(d)) != 0 ) > + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) > goto fail; ... this (and any further copies in future ports), wouldn't it be better to centrally do this in iommu_domain_init() itself? > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) > XEN_DOMCTL_CDF_hap | > XEN_DOMCTL_CDF_s3_integrity | > XEN_DOMCTL_CDF_oos_off | > - XEN_DOMCTL_CDF_xs_domain) ) > + XEN_DOMCTL_CDF_xs_domain | > + XEN_DOMCTL_CDF_iommu) ) > { > dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags); > return -EINVAL; Also refuse XEN_DOMCTL_CDF_iommu when !iommu_enabled? > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -981,6 +981,11 @@ static inline bool is_xenstore_domain(const struct domain *d) > return d->options & XEN_DOMCTL_CDF_xs_domain; > } > > +static inline bool is_iommu_enabled(const struct domain *d) > +{ > + return d->options & XEN_DOMCTL_CDF_iommu; > +} Perhaps wrap in evaluate_nospec()? Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 07 August 2019 10:22 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Roger Pau Monne > <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; > Wei Liu <wl@xen.org> > Subject: Re: [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu > > On 30.07.2019 15:44, Paul Durrant wrote: > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -673,8 +673,7 @@ int arch_domain_create(struct domain *d, > > > > ASSERT(config != NULL); > > > > - /* p2m_init relies on some value initialized by the IOMMU subsystem */ > > - if ( (rc = iommu_domain_init(d)) != 0 ) > > + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) > > goto fail; > > Instead of this and ... > > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -604,7 +604,7 @@ int arch_domain_create(struct domain *d, > > if ( (rc = init_domain_irq_mapping(d)) != 0 ) > > goto fail; > > > > - if ( (rc = iommu_domain_init(d)) != 0 ) > > + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) > > goto fail; > > ... this (and any further copies in future ports), wouldn't it > be better to centrally do this in iommu_domain_init() itself? Ok... it kind of seemed more logical to avoid the call if the flag is not present... but there's no real consistency on this kind of thing in the Xen codebase so I'll change it to shorten the patch. > > > --- a/xen/common/domain.c > > +++ b/xen/common/domain.c > > @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) > > XEN_DOMCTL_CDF_hap | > > XEN_DOMCTL_CDF_s3_integrity | > > XEN_DOMCTL_CDF_oos_off | > > - XEN_DOMCTL_CDF_xs_domain) ) > > + XEN_DOMCTL_CDF_xs_domain | > > + XEN_DOMCTL_CDF_iommu) ) > > { > > dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags); > > return -EINVAL; > > Also refuse XEN_DOMCTL_CDF_iommu when !iommu_enabled? Yes, that would be reasonable. > > > --- a/xen/include/xen/sched.h > > +++ b/xen/include/xen/sched.h > > @@ -981,6 +981,11 @@ static inline bool is_xenstore_domain(const struct domain *d) > > return d->options & XEN_DOMCTL_CDF_xs_domain; > > } > > > > +static inline bool is_iommu_enabled(const struct domain *d) > > +{ > > + return d->options & XEN_DOMCTL_CDF_iommu; > > +} > > Perhaps wrap in evaluate_nospec()? > Given the codepaths that it will eventually get, yes it probably should have the guard. Paul > Jan
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 03ce166f4f..feb9f1ce0c 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -555,6 +555,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, .max_grant_frames = b_info->max_grant_frames, .max_maptrack_frames = b_info->max_maptrack_frames, }; + libxl_physinfo physinfo; if (info->type != LIBXL_DOMAIN_TYPE_PV) { create.flags |= XEN_DOMCTL_CDF_hvm_guest; @@ -564,6 +565,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off; } + rc = libxl_get_physinfo(ctx, &physinfo); + if (rc < 0) + goto out; + + if ( physinfo.cap_hvm_directio ) + create.flags |= XEN_DOMCTL_CDF_iommu; + /* Ultimately, handle is an array of 16 uint8_t, same as uuid */ libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid); diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 941bbff4fe..e06bd27dad 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -673,8 +673,7 @@ int arch_domain_create(struct domain *d, ASSERT(config != NULL); - /* p2m_init relies on some value initialized by the IOMMU subsystem */ - if ( (rc = iommu_domain_init(d)) != 0 ) + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) goto fail; if ( (rc = p2m_init(d)) != 0 ) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 215746a5c3..fca1e62901 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -914,6 +914,9 @@ void __init start_xen(unsigned long boot_phys_offset, dom0_cfg.arch.tee_type = tee_get_type(); dom0_cfg.max_vcpus = dom0_max_vcpus(); + if ( iommu_enabled ) + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; + dom0 = domain_create(0, &dom0_cfg, true); if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) panic("Error creating domain 0\n"); diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index fbc70b9f94..42778099da 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -604,7 +604,7 @@ int arch_domain_create(struct domain *d, if ( (rc = init_domain_irq_mapping(d)) != 0 ) goto fail; - if ( (rc = iommu_domain_init(d)) != 0 ) + if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 ) goto fail; psr_domain_init(d); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 277170f386..e048f70eef 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1726,6 +1726,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) } dom0_cfg.max_vcpus = dom0_max_vcpus(); + if ( iommu_enabled ) + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; + /* Create initial domain 0. */ dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim); if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) diff --git a/xen/common/domain.c b/xen/common/domain.c index 9b70626753..0df4b47352 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) XEN_DOMCTL_CDF_hap | XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off | - XEN_DOMCTL_CDF_xs_domain) ) + XEN_DOMCTL_CDF_xs_domain | + XEN_DOMCTL_CDF_iommu) ) { dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags); return -EINVAL; diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index b6eaae7283..d32b172664 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -119,6 +119,9 @@ int iommu_release_dt_devices(struct domain *d) struct dt_device_node *dev, *_dev; int rc; + if ( !is_iommu_enabled(d) ) + return 0; + list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list) { rc = iommu_deassign_dt_device(d, dev); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 37eb0f7d01..0a00279067 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -151,13 +151,13 @@ int iommu_domain_init(struct domain *d) struct domain_iommu *hd = dom_iommu(d); int ret = 0; + if ( !iommu_enabled ) + return -EOPNOTSUPP; + ret = arch_iommu_domain_init(d); if ( ret ) return ret; - if ( !iommu_enabled ) - return 0; - hd->platform_ops = iommu_get_ops(); return hd->platform_ops->init(d); } diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 19486d5e32..3f82c78870 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -64,6 +64,10 @@ struct xen_domctl_createdomain { /* Is this a xenstore domain? */ #define _XEN_DOMCTL_CDF_xs_domain 4 #define XEN_DOMCTL_CDF_xs_domain (1U<<_XEN_DOMCTL_CDF_xs_domain) + /* Should this domain be permitted to use the IOMMU? */ +#define _XEN_DOMCTL_CDF_iommu 5 +#define XEN_DOMCTL_CDF_iommu (1U<<_XEN_DOMCTL_CDF_iommu) + uint32_t flags; /* diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index a62161cc54..bad9734626 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -981,6 +981,11 @@ static inline bool is_xenstore_domain(const struct domain *d) return d->options & XEN_DOMCTL_CDF_xs_domain; } +static inline bool is_iommu_enabled(const struct domain *d) +{ + return d->options & XEN_DOMCTL_CDF_iommu; +} + extern bool sched_smt_power_savings; extern enum cpufreq_controller {
This patch introduces a common domain creation flag to determine whether the domain is permitted to make use of the IOMMU. Currently the flag is always set (for both dom0 and domU) if the IOMMU is globally enabled. This patch gates calls to iommu_domain_init() on the new flag. The function was previously called unconditionally, but was largely a no-op if the global iommu_enabled flag was not set. The only thing that was done even if iommu_enabled was not set was the call to arch_iommu_domain_init(), but it appears that this was only necessary to initialize the dt_devices list for ARM such that iommu_release_dt_devices() can be called unconditionally by domain_relinquish_resourcs(). Adding a simple check of is_iommu_emabled() into iommu_release_dt_devices() keeps this unconditional call working. No functional change should be observed with this patch applied. Subsequent patches will allow the toolstack to control whether use of the IOMMU is enabled for a domain. NOTE: The introduction of the is_iommu_enabled() helper function might seem excessive but its use is expected to increase with subsequent patches. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> Cc: "Roger Pau Monné" <roger.pau@citrix.com> --- tools/libxl/libxl_create.c | 8 ++++++++ xen/arch/arm/domain.c | 3 +-- xen/arch/arm/setup.c | 3 +++ xen/arch/x86/domain.c | 2 +- xen/arch/x86/setup.c | 3 +++ xen/common/domain.c | 3 ++- xen/drivers/passthrough/device_tree.c | 3 +++ xen/drivers/passthrough/iommu.c | 6 +++--- xen/include/public/domctl.h | 4 ++++ xen/include/xen/sched.h | 5 +++++ 10 files changed, 33 insertions(+), 7 deletions(-)