Re: [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
> -Original Message- > From: Roger Pau Monne > Sent: 23 August 2019 11:33 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org; Ian Jackson ; Wei > Liu ; > Anthony Perard ; Andrew Cooper > ; George Dunlap > ; Jan Beulich ; Julien Grall > ; > Konrad Rzeszutek Wilk ; Stefano Stabellini > ; Tim > (Xen.org) ; Volodymyr Babchuk > Subject: Re: [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag > > On Fri, Aug 16, 2019 at 06:19:57PM +0100, 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. > > > > 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 > > I have one ARM-related question and one 'nice to have', but the code > LGTM: > > Reviewed-by: Roger Pau Monné > > > --- > > Cc: Ian Jackson > > Cc: Wei Liu > > Cc: Anthony PERARD > > Cc: Andrew Cooper > > Cc: George Dunlap > > Cc: Jan Beulich > > Cc: Julien Grall > > Cc: Konrad Rzeszutek Wilk > > Cc: Stefano Stabellini > > Cc: Tim Deegan > > Cc: Volodymyr Babchuk > > Cc: "Roger Pau Monné" > > > > Previously part of series > > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > > > 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() > > --- > > xen/arch/arm/setup.c | 3 +++ > > xen/arch/x86/setup.c | 3 +++ > > xen/common/domain.c | 9 - > > xen/common/domctl.c | 8 > > 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 + > > 8 files changed, 37 insertions(+), 4 deletions(-) > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > index 2c5d1372c0..20021ee0ca 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, _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(), _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 76e6976617..e832a5c4aa 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 | > >
Re: [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
On Fri, Aug 16, 2019 at 06:19:57PM +0100, 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. > > 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 I have one ARM-related question and one 'nice to have', but the code LGTM: Reviewed-by: Roger Pau Monné > --- > Cc: Ian Jackson > Cc: Wei Liu > Cc: Anthony PERARD > Cc: Andrew Cooper > Cc: George Dunlap > Cc: Jan Beulich > Cc: Julien Grall > Cc: Konrad Rzeszutek Wilk > Cc: Stefano Stabellini > Cc: Tim Deegan > Cc: Volodymyr Babchuk > Cc: "Roger Pau Monné" > > Previously part of series > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html > > 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() > --- > xen/arch/arm/setup.c | 3 +++ > xen/arch/x86/setup.c | 3 +++ > xen/common/domain.c | 9 - > xen/common/domctl.c | 8 > 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 + > 8 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 2c5d1372c0..20021ee0ca 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, _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(), _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 76e6976617..e832a5c4aa 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; > @@ -328,6 +329,12 @@ static int sanitise_domain_config(struct > xen_domctl_createdomain *config) > config->flags |= XEN_DOMCTL_CDF_oos_off; > } > > +if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled ) > +{ >