| Message ID | 20190902145014.36442-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add per-domain IOMMU control | expand |
> On 2 Sep 2019, at 15:50, Paul Durrant <paul.durrant@citrix.com> wrote: > > tools/ocaml/libs/xc/xenctrl.ml | 8 +++++++- > tools/ocaml/libs/xc/xenctrl.mli | 8 +++++++- Acked-by: Christian Lindig <christian.lindig@citrix.com>
Hi Paul, Apologies for the late answer. A couple of comments below. On 9/2/19 3:50 PM, Paul Durrant wrote: > 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 > (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject > the flag if !iommu_enabled. This is not entirely correct for Arm, the flag will not be set for domU created by Xen directly (i.e dom0less). Device passthrough is not yet supported for dom0less so this is not much a concern. However, there are a series to add support for platform device passthrough (see [1]). So there are two possibilities: 1) If your series goes first, then we should at least document it in the commit message that IOMMU will be disabled for domain (other than dom0) created by Xen. 2) If your series goes second, then you (or Stefano) would have to ensure this does not break [1]. I haven't yet had the chance to review the latest version of Stefano and this patch looks in good shape. So 1) seems more suitable. Stefano would have to ensure the flags is set when doing device assignment. > > A new helper function, is_iommu_enabled(), is added to test the flag and > iommu_domain_init() will return immediately if !is_iommu_enabled(). This is > slightly different to the previous behaviour based on !iommu_enabled where > the call to arch_iommu_domain_init() was made regardless, however it appears > that this call was only necessary to initialize the dt_devices list for ARM > such that iommu_release_dt_devices() can be called unconditionally by > domain_relinquish_resources(). Adding a simple check of is_iommu_enabled() > 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. Also, having iommu_domain_init() bail before calling > arch_iommu_domain_init() is not strictly necessary, but I think the > consequent addition of the call to is_iommu_enabled() in > iommu_release_dt_devices() makes the code clearer. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com> > Acked-by: Jan Beulich <jbeulich@suse.com> > --- > Cc: Christian Lindig <christian.lindig@citrix.com> > Cc: David Scott <dave@recoil.org> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.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> > > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > v7: > - Add a check to verify that the toolstack has not set XEN_DOMCTL_CDF_iommu > - Add missing ocaml binding changes > > v6: > - Remove the toolstack parts as there's no nice method of testing whether > the IOMMU is enabled in an architecture-neutral way > > v5: > - Move is_iommu_enabled() check into iommu_domain_init() > - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled > - Use evaluate_nospec() in defintion of is_iommu_enabled() > --- > tools/ocaml/libs/xc/xenctrl.ml | 8 +++++++- > tools/ocaml/libs/xc/xenctrl.mli | 8 +++++++- > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 3 +++ > xen/common/domain.c | 9 ++++++++- > xen/common/domctl.c | 13 +++++++++++++ > 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, 56 insertions(+), 6 deletions(-) > > diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml > index 35958b94d5..bdf3f2e395 100644 > --- a/tools/ocaml/libs/xc/xenctrl.ml > +++ b/tools/ocaml/libs/xc/xenctrl.ml > @@ -56,7 +56,13 @@ type arch_domainconfig = > | ARM of xen_arm_arch_domainconfig > | X86 of xen_x86_arch_domainconfig > > -type domain_create_flag = CDF_HVM | CDF_HAP > +type domain_create_flag = > + | CDF_HVM > + | CDF_HAP > + | CDF_S3_INTEGRITY > + | CDF_OOS_OFF > + | CDF_XS_DOMAIN > + | CDF_IOMMU This patch is only adding the last flag, but you are adding more here. I understand that you are just re-syncing the type with Xen. IHMO, this should belong in a separate patch as we may want to backport it. If backporting is not necessary, then the change should at least be mentioned in the commit message as this seems a bit off-topic. Cheers, [1] https://lists.xen.org/archives/html/xen-devel/2019-08/msg01974.html
Hi, On 9/2/19 3:50 PM, Paul Durrant wrote: > diff --git a/xen/common/domain.c b/xen/common/domain.c > index e9d2c613e0..7dfb257c50 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; > @@ -320,6 +321,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) > return -EINVAL; > } > > + if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled ) > + { > + dprintk(XENLOG_INFO, "IOMMU is not enabled\n"); > + return -EINVAL; > + } > + Looking at this patch again, the implementation of arch_sanitise_domain_config() for Arm will only accepts config->flags to be equal to CDF_hvm_guest | CDF_hap. So after this patch, it will not be possible to create any domain when CDF_iommu is set. Cheers,
> -----Original Message----- > From: Julien Grall <julien.grall@arm.com> > Sent: 05 September 2019 21:06 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Christian Lindig > <christian.lindig@citrix.com>; David Scott <dave@recoil.org>; Ian Jackson <Ian.Jackson@citrix.com>; > Wei Liu <wl@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v8 2/6] domain: introduce XEN_DOMCTL_CDF_iommu flag > > Hi, > > On 9/2/19 3:50 PM, Paul Durrant wrote: > > diff --git a/xen/common/domain.c b/xen/common/domain.c > > index e9d2c613e0..7dfb257c50 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; > > @@ -320,6 +321,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) > > return -EINVAL; > > } > > > > + if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled ) > > + { > > + dprintk(XENLOG_INFO, "IOMMU is not enabled\n"); > > + return -EINVAL; > > + } > > + > > Looking at this patch again, the implementation of > arch_sanitise_domain_config() for Arm will only accepts config->flags to > be equal to CDF_hvm_guest | CDF_hap. > > So after this patch, it will not be possible to create any domain when > CDF_iommu is set. You're right, I'm not sure how I missed that. I think I had changed it in development then managed to lose the hunk. Clearly ARM needs to accept the flag too. Paul > > Cheers, > > -- > Julien Grall
> -----Original Message----- [snip] > > diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml > > index 35958b94d5..bdf3f2e395 100644 > > --- a/tools/ocaml/libs/xc/xenctrl.ml > > +++ b/tools/ocaml/libs/xc/xenctrl.ml > > @@ -56,7 +56,13 @@ type arch_domainconfig = > > | ARM of xen_arm_arch_domainconfig > > | X86 of xen_x86_arch_domainconfig > > > > -type domain_create_flag = CDF_HVM | CDF_HAP > > +type domain_create_flag = > > + | CDF_HVM > > + | CDF_HAP > > + | CDF_S3_INTEGRITY > > + | CDF_OOS_OFF > > + | CDF_XS_DOMAIN > > + | CDF_IOMMU > > This patch is only adding the last flag, but you are adding more here. I > understand that you are just re-syncing the type with Xen. IHMO, this > should belong in a separate patch as we may want to backport it. > > If backporting is not necessary, then the change should at least be > mentioned in the commit message as this seems a bit off-topic. > The apparently extraneous flag additions are because of the way that the code at http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/ocaml/libs/xc/xenctrl_stubs.c;hb=HEAD#l149 works. TL;DR the index of the item in the list needs to match the bit position in the flags field. Since you queried it I guess it'd better have a comment in the code. Paul > Cheers, > > [1] https://lists.xen.org/archives/html/xen-devel/2019-08/msg01974.html > > -- > Julien Grall
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index 35958b94d5..bdf3f2e395 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -56,7 +56,13 @@ type arch_domainconfig = | ARM of xen_arm_arch_domainconfig | X86 of xen_x86_arch_domainconfig -type domain_create_flag = CDF_HVM | CDF_HAP +type domain_create_flag = + | CDF_HVM + | CDF_HAP + | CDF_S3_INTEGRITY + | CDF_OOS_OFF + | CDF_XS_DOMAIN + | CDF_IOMMU type domctl_create_config = { diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index 6c4268d453..fc40885671 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -49,7 +49,13 @@ type arch_domainconfig = | ARM of xen_arm_arch_domainconfig | X86 of xen_x86_arch_domainconfig -type domain_create_flag = CDF_HVM | CDF_HAP +type domain_create_flag = + | CDF_HVM + | CDF_HAP + | CDF_S3_INTEGRITY + | CDF_OOS_OFF + | CDF_XS_DOMAIN + | CDF_IOMMU type domctl_create_config = { ssidref: int32; diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index fa6c110b11..dc2640f8b9 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -961,6 +961,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/setup.c b/xen/arch/x86/setup.c index d0b35b0ce2..fa226a2bab 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1733,6 +1733,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 e9d2c613e0..7dfb257c50 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; @@ -320,6 +321,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } + if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled ) + { + dprintk(XENLOG_INFO, "IOMMU is not enabled\n"); + return -EINVAL; + } + return arch_sanitise_domain_config(config); } diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 6e6e9b9866..5dcfe3c8f6 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -515,6 +515,19 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) rover = dom; } + /* + * For now, make sure the createdomain IOMMU flag is set if the + * IOMMU is enabled. When the flag comes under toolstack control + * this can go away. + */ + if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_iommu ) + { + ASSERT_UNREACHABLE(); + return -EINVAL; + } + if ( iommu_enabled ) + op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu; + d = domain_create(dom, &op->u.createdomain, false); if ( IS_ERR(d) ) { 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..e61d3d1368 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 ( !is_iommu_enabled(d) ) + return 0; + 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 72d5133cba..5f55a2f6e1 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 d2bbe03bd9..3c0db42b82 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -983,6 +983,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 evaluate_nospec(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 (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject the flag if !iommu_enabled. A new helper function, is_iommu_enabled(), is added to test the flag and iommu_domain_init() will return immediately if !is_iommu_enabled(). This is slightly different to the previous behaviour based on !iommu_enabled where the call to arch_iommu_domain_init() was made regardless, however it appears that this call was only necessary to initialize the dt_devices list for ARM such that iommu_release_dt_devices() can be called unconditionally by domain_relinquish_resources(). Adding a simple check of is_iommu_enabled() 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. Also, having iommu_domain_init() bail before calling arch_iommu_domain_init() is not strictly necessary, but I think the consequent addition of the call to is_iommu_enabled() in iommu_release_dt_devices() makes the code clearer. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> --- Cc: Christian Lindig <christian.lindig@citrix.com> Cc: David Scott <dave@recoil.org> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.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> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html v7: - Add a check to verify that the toolstack has not set XEN_DOMCTL_CDF_iommu - Add missing ocaml binding changes v6: - Remove the toolstack parts as there's no nice method of testing whether the IOMMU is enabled in an architecture-neutral way v5: - Move is_iommu_enabled() check into iommu_domain_init() - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled - Use evaluate_nospec() in defintion of is_iommu_enabled() --- tools/ocaml/libs/xc/xenctrl.ml | 8 +++++++- tools/ocaml/libs/xc/xenctrl.mli | 8 +++++++- xen/arch/arm/setup.c | 3 +++ xen/arch/x86/setup.c | 3 +++ xen/common/domain.c | 9 ++++++++- xen/common/domctl.c | 13 +++++++++++++ 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, 56 insertions(+), 6 deletions(-)