* [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-07 9:21 ` Jan Beulich
2019-07-30 13:44 ` [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate Paul Durrant
` (4 subsequent siblings)
5 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Jan Beulich, Anthony PERARD,
Volodymyr Babchuk, Roger Pau Monné
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(-)
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 {
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu
2019-07-30 13:44 ` [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu Paul Durrant
@ 2019-08-07 9:21 ` Jan Beulich
2019-08-12 12:22 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-07 9:21 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, TimDeegan,
Julien Grall, Anthony PERARD, xen-devel, Volodymyr Babchuk,
Roger Pau Monné
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu
2019-08-07 9:21 ` Jan Beulich
@ 2019-08-12 12:22 ` Paul Durrant
0 siblings, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-12 12:22 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Ian Jackson, Anthony Perard,
xen-devel, Volodymyr Babchuk, Roger Pau Monne
> -----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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
2019-07-30 13:44 ` [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-07 9:55 ` Jan Beulich
2019-07-30 13:44 ` [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables Paul Durrant
` (3 subsequent siblings)
5 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu,
George Dunlap, Andrew Cooper, Brian Woods, Julien Grall,
Paul Durrant, Jan Beulich, Volodymyr Babchuk,
Suravee Suthikulpanit, Roger Pau Monné
...rather than testing the global iommu_enabled flag and ops pointer.
Now that there is a per-domain flag indicating whether the domain is
permitted to use the IOMMU (which determines whether the ops pointer will
be set), many tests of the global iommu_enabled flag and ops pointer can
be translated into tests of the per-domain flag. Some of the other tests of
purely the global iommu_enabled flag can also be translated into tests of
the per-domain flag.
NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
disappeared some time ago.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
---
xen/arch/arm/p2m.c | 3 +--
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/domctl.c | 4 ++--
xen/arch/x86/hvm/hvm.c | 6 ++---
xen/arch/x86/hvm/vioapic.c | 2 +-
xen/arch/x86/hvm/vmx/vmcs.c | 2 +-
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 4 ++--
xen/drivers/passthrough/amd/iommu_guest.c | 2 +-
xen/drivers/passthrough/device_tree.c | 4 ++--
xen/drivers/passthrough/io.c | 8 +++----
xen/drivers/passthrough/iommu.c | 29 ++++++++++-------------
xen/drivers/passthrough/pci.c | 14 +++++------
xen/drivers/passthrough/vtd/iommu.c | 2 +-
xen/drivers/passthrough/vtd/x86/hvm.c | 2 +-
xen/drivers/passthrough/x86/iommu.c | 2 +-
16 files changed, 42 insertions(+), 46 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e28ea1c85a..c5cea25caa 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
* shared with the CPU, Xen has to make sure that the PT changes have
* reached the memory
*/
- p2m->clean_pte = iommu_enabled &&
- !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
+ p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
rc = p2m_alloc_table(d);
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c69570920c..d381784edd 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -356,7 +356,7 @@ unsigned long __init dom0_compute_nr_pages(
avail -= d->max_vcpus - 1;
/* Reserve memory for iommu_dom0_init() (rough estimate). */
- if ( iommu_enabled )
+ if ( is_iommu_enabled(d) )
{
unsigned int s;
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 2d45e5b8a8..be4b206068 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -715,7 +715,7 @@ long arch_do_domctl(
break;
ret = -ESRCH;
- if ( iommu_enabled )
+ if ( is_iommu_enabled(d) )
{
pcidevs_lock();
ret = pt_irq_create_bind(d, bind);
@@ -744,7 +744,7 @@ long arch_do_domctl(
if ( ret )
break;
- if ( iommu_enabled )
+ if ( is_iommu_enabled(d) )
{
pcidevs_lock();
ret = pt_irq_destroy_bind(d, bind);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 029eea3b85..172c860acc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -465,7 +465,7 @@ void hvm_migrate_timers(struct vcpu *v)
void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
{
- ASSERT(iommu_enabled &&
+ ASSERT(is_iommu_enabled(v->domain) &&
(is_hardware_domain(v->domain) || hvm_domain_irq(v->domain)->dpci));
if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
@@ -496,7 +496,7 @@ void hvm_migrate_pirqs(struct vcpu *v)
{
struct domain *d = v->domain;
- if ( !iommu_enabled || !hvm_domain_irq(d)->dpci )
+ if ( !is_iommu_enabled(d) || !hvm_domain_irq(d)->dpci )
return;
spin_lock(&d->event_lock);
@@ -2264,7 +2264,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
}
if ( ((value ^ old_value) & X86_CR0_CD) &&
- iommu_enabled && hvm_funcs.handle_cd &&
+ is_iommu_enabled(d) && hvm_funcs.handle_cd &&
(!rangeset_is_empty(d->iomem_caps) ||
!rangeset_is_empty(d->arch.ioport_caps) ||
has_arch_pdevs(d)) )
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 9c25f72b4d..9aeef32a14 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -536,7 +536,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
ent->fields.remote_irr = 0;
- if ( iommu_enabled )
+ if ( is_iommu_enabled(d) )
{
spin_unlock(&d->arch.hvm.irq_lock);
hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 45d18493df..9850da460a 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1087,7 +1087,7 @@ static int construct_vmcs(struct vcpu *v)
vmx_clear_msr_intercept(v, MSR_IA32_SYSENTER_CS, VMX_MSR_RW);
vmx_clear_msr_intercept(v, MSR_IA32_SYSENTER_ESP, VMX_MSR_RW);
vmx_clear_msr_intercept(v, MSR_IA32_SYSENTER_EIP, VMX_MSR_RW);
- if ( paging_mode_hap(d) && (!iommu_enabled || iommu_snoop) )
+ if ( paging_mode_hap(d) && (!is_iommu_enabled(d) || iommu_snoop) )
vmx_clear_msr_intercept(v, MSR_IA32_CR_PAT, VMX_MSR_RW);
if ( (vmexit_ctl & VM_EXIT_CLEAR_BNDCFGS) &&
(vmentry_ctl & VM_ENTRY_LOAD_BNDCFGS) )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0060310d74..3b3d5b6250 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1210,7 +1210,7 @@ static void vmx_handle_cd(struct vcpu *v, unsigned long value)
{
v->arch.hvm.cache_mode = NORMAL_CACHE_MODE;
vmx_set_guest_pat(v, *pat);
- if ( !iommu_enabled || iommu_snoop )
+ if ( !is_iommu_enabled(v->domain) || iommu_snoop )
vmx_clear_msr_intercept(v, MSR_IA32_CR_PAT, VMX_MSR_RW);
hvm_asid_flush_vcpu(v); /* no need to flush cache */
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 6b8468c793..ef1423fe2d 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -260,7 +260,7 @@ static bool_t ept_split_super_page(struct p2m_domain *p2m,
*epte = *ept_entry;
epte->sp = (level > 1);
epte->mfn += i * trunk;
- epte->snp = (iommu_enabled && iommu_snoop);
+ epte->snp = is_iommu_enabled(p2m->domain) && iommu_snoop;
epte->suppress_ve = 1;
ept_p2m_type_to_flags(p2m, epte, epte->sa_p2mt, epte->access);
@@ -766,7 +766,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
new_entry.sp = !!i;
new_entry.sa_p2mt = p2mt;
new_entry.access = p2ma;
- new_entry.snp = (iommu_enabled && iommu_snoop);
+ new_entry.snp = is_iommu_enabled(p2m->domain) && iommu_snoop;
/* the caller should take care of the previous page */
new_entry.mfn = mfn_x(mfn);
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 328e7509d5..5fc3b1a56c 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -857,7 +857,7 @@ int guest_iommu_init(struct domain* d)
struct guest_iommu *iommu;
struct domain_iommu *hd = dom_iommu(d);
- if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled ||
+ if ( !is_hvm_domain(d) || !is_iommu_enabled(d) || !iommuv2_enabled ||
!has_viommu(d) )
return 0;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index d32b172664..12f2c4c3f2 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -29,7 +29,7 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
int rc = -EBUSY;
struct domain_iommu *hd = dom_iommu(d);
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return -EINVAL;
if ( !dt_device_is_protected(dev) )
@@ -71,7 +71,7 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev)
const struct domain_iommu *hd = dom_iommu(d);
int rc;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return -EINVAL;
if ( !dt_device_is_protected(dev) )
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 4290c7c710..b292e79382 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -434,7 +434,7 @@ int pt_irq_create_bind(
if ( vcpu )
pirq_dpci->gmsi.posted = true;
}
- if ( vcpu && iommu_enabled )
+ if ( vcpu && is_iommu_enabled(d) )
hvm_migrate_pirq(pirq_dpci, vcpu);
/* Use interrupt posting if it is supported. */
@@ -817,7 +817,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq)
ASSERT(is_hvm_domain(d));
- if ( !iommu_enabled || (!is_hardware_domain(d) && !dpci) ||
+ if ( !is_iommu_enabled(d) || (!is_hardware_domain(d) && !dpci) ||
!pirq_dpci || !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
return 0;
@@ -869,7 +869,7 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
void hvm_dpci_msi_eoi(struct domain *d, int vector)
{
- if ( !iommu_enabled ||
+ if ( !is_iommu_enabled(d) ||
(!hvm_domain_irq(d)->dpci && !is_hardware_domain(d)) )
return;
@@ -1001,7 +1001,7 @@ void hvm_dpci_eoi(struct domain *d, unsigned int guest_gsi,
const struct hvm_irq_dpci *hvm_irq_dpci;
const struct hvm_girq_dpci_mapping *girq;
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
return;
if ( is_hardware_domain(d) )
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 0a00279067..d0200d82f0 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -179,7 +179,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
check_hwdom_reqs(d);
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
return;
register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
@@ -284,7 +284,7 @@ int iommu_construct(struct domain *d)
void iommu_domain_destroy(struct domain *d)
{
- if ( !iommu_enabled || !dom_iommu(d)->platform_ops )
+ if ( !is_iommu_enabled(d) )
return;
iommu_teardown(d);
@@ -300,7 +300,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned long i;
int rc = 0;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return 0;
ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
@@ -360,7 +360,7 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
unsigned long i;
int rc = 0;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return 0;
ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
@@ -413,7 +413,7 @@ int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
{
const struct domain_iommu *hd = dom_iommu(d);
- if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->lookup_page )
+ if ( !is_iommu_enabled(d) || !hd->platform_ops->lookup_page )
return -EOPNOTSUPP;
return iommu_call(hd->platform_ops, lookup_page, d, dfn, mfn, flags);
@@ -442,8 +442,8 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_count,
const struct domain_iommu *hd = dom_iommu(d);
int rc;
- if ( !iommu_enabled || !hd->platform_ops ||
- !hd->platform_ops->iotlb_flush || !page_count || !flush_flags )
+ if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush ||
+ !page_count || !flush_flags )
return 0;
if ( dfn_eq(dfn, INVALID_DFN) )
@@ -470,8 +470,8 @@ int iommu_iotlb_flush_all(struct domain *d, unsigned int flush_flags)
const struct domain_iommu *hd = dom_iommu(d);
int rc;
- if ( !iommu_enabled || !hd->platform_ops ||
- !hd->platform_ops->iotlb_flush_all || !flush_flags )
+ if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush_all ||
+ !flush_flags )
return 0;
/*
@@ -556,7 +556,7 @@ int iommu_do_domctl(
{
int ret = -ENODEV;
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
return -ENOSYS;
#ifdef CONFIG_HAS_PCI
@@ -576,9 +576,9 @@ void iommu_share_p2m_table(struct domain* d)
ASSERT(hap_enabled(d));
/*
* iommu_use_hap_pt(d) cannot be used here because during domain
- * construction need_iommu(d) will always return false here.
+ * construction has_iommu_pt(d) will always return false here.
*/
- if ( iommu_enabled && iommu_hap_pt_share )
+ if ( is_iommu_enabled(d) && iommu_hap_pt_share )
iommu_get_ops()->share_p2m(d);
}
@@ -608,10 +608,7 @@ int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
{
- if ( !iommu_enabled )
- return 0;
-
- return test_bit(feature, dom_iommu(d)->features);
+ return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
}
static void iommu_dump_p2m_table(unsigned char key)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 7c196ba58b..61b5b330ca 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -864,7 +864,7 @@ static int pci_clean_dpci_irqs(struct domain *d)
{
struct hvm_irq_dpci *hvm_irq_dpci = NULL;
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
return 0;
if ( !is_hvm_domain(d) )
@@ -1333,7 +1333,7 @@ static int iommu_add_device(struct pci_dev *pdev)
ASSERT(pcidevs_locked());
hd = dom_iommu(pdev->domain);
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(pdev->domain) )
return 0;
rc = hd->platform_ops->add_device(pdev->devfn, pci_to_dev(pdev));
@@ -1362,7 +1362,7 @@ static int iommu_enable_device(struct pci_dev *pdev)
ASSERT(pcidevs_locked());
hd = dom_iommu(pdev->domain);
- if ( !iommu_enabled || !hd->platform_ops ||
+ if ( !is_iommu_enabled(pdev->domain) ||
!hd->platform_ops->enable_device )
return 0;
@@ -1378,7 +1378,7 @@ static int iommu_remove_device(struct pci_dev *pdev)
return -EINVAL;
hd = dom_iommu(pdev->domain);
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(pdev->domain) )
return 0;
for ( devfn = pdev->devfn ; pdev->phantom_stride; )
@@ -1421,7 +1421,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
struct pci_dev *pdev;
int rc = 0;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return 0;
/* Prevent device assign if mem paging or mem sharing have been
@@ -1483,7 +1483,7 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
struct pci_dev *pdev = NULL;
int ret = 0;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return -EINVAL;
ASSERT(pcidevs_locked());
@@ -1536,7 +1536,7 @@ static int iommu_get_device_group(
int i = 0;
const struct iommu_ops *ops = hd->platform_ops;
- if ( !iommu_enabled || !ops || !ops->get_device_group_id )
+ if ( !is_iommu_enabled(d) || !ops->get_device_group_id )
return 0;
group_id = ops->get_device_group_id(seg, bus, devfn);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5d72270c5b..01f0bc4689 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1757,7 +1757,7 @@ static void iommu_domain_teardown(struct domain *d)
xfree(mrmrr);
}
- ASSERT(iommu_enabled);
+ ASSERT(is_iommu_enabled(d));
/*
* We can't use iommu_use_hap_pt here because either IOMMU state
diff --git a/xen/drivers/passthrough/vtd/x86/hvm.c b/xen/drivers/passthrough/vtd/x86/hvm.c
index 6675dca027..f77b35815c 100644
--- a/xen/drivers/passthrough/vtd/x86/hvm.c
+++ b/xen/drivers/passthrough/vtd/x86/hvm.c
@@ -51,7 +51,7 @@ void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq)
struct hvm_irq_dpci *dpci = NULL;
ASSERT(isairq < NR_ISAIRQS);
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
return;
spin_lock(&d->event_lock);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index fd05075bb5..9879558c17 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -178,7 +178,7 @@ int arch_iommu_populate_page_table(struct domain *d)
void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
{
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(d) )
panic("Presently, iommu must be enabled for PVH hardware domain\n");
}
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...
2019-07-30 13:44 ` [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate Paul Durrant
@ 2019-08-07 9:55 ` Jan Beulich
2019-08-07 10:22 ` Julien Grall
2019-08-12 14:53 ` Paul Durrant
0 siblings, 2 replies; 38+ messages in thread
From: Jan Beulich @ 2019-08-07 9:55 UTC (permalink / raw)
To: Paul Durrant
Cc: Jun Nakajima, Kevin Tian, Stefano Stabellini, VolodymyrBabchuk,
Wei Liu, George Dunlap, Andrew Cooper, Julien Grall,
Suravee Suthikulpanit, xen-devel, Brian Woods,
Roger Pau Monné
On 30.07.2019 15:44, Paul Durrant wrote:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
> * shared with the CPU, Xen has to make sure that the PT changes have
> * reached the memory
> */
> - p2m->clean_pte = iommu_enabled &&
> - !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> + p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
I can't tell if the original code was meant to be this way, but I'm
afraid your transformation is not correct: The prior construct,
expanding iommu_has_feature(), was
iommu_enabled && !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
which transforms through
iommu_enabled && (!iommu_enabled || !test_bit(feature, dom_iommu(d)->features))
to
(iommu_enabled && !iommu_enabled) || (iommu_enabled && !test_bit(feature, dom_iommu(d)->features))
and hence
iommu_enabled && !test_bit(feature, dom_iommu(d)->features)
whereas the new code simply is
!(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
i.e.
!iommu_enabled || !test_bit(feature, dom_iommu(d)->features)
> @@ -766,7 +766,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
> new_entry.sp = !!i;
> new_entry.sa_p2mt = p2mt;
> new_entry.access = p2ma;
> - new_entry.snp = (iommu_enabled && iommu_snoop);
> + new_entry.snp = is_iommu_enabled(p2m->domain) && iommu_snoop;
Please use d here.
Seeing that this is the last change in x86/mm/, did you overlook
the use in p2m_pt_set_entry()? Or is this meant to go on top of
Alexandru's "x86/mm: Clean IOMMU flags from p2m-pt code" (which
should then be noted in a post-commit-message remark)?
> @@ -556,7 +556,7 @@ int iommu_do_domctl(
> {
> int ret = -ENODEV;
>
> - if ( !iommu_enabled )
> + if ( !is_iommu_enabled(d) )
> return -ENOSYS;
ENOSYS was wrong here from the beginning, but it certainly gets
worse with this no longer being a system wide property. Please
change to EOPNOTSUPP or some other suitable one.
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -864,7 +864,7 @@ static int pci_clean_dpci_irqs(struct domain *d)
Above here there's another use in pci_enable_acs(), which should
imo act on pdev->domain.
There's another use in flask_iommu_resource_use_perm(). All
callers of the function hold a struct domain * in their hands,
which I think they should pass into this function such that the
conditional can be replaced.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...
2019-08-07 9:55 ` Jan Beulich
@ 2019-08-07 10:22 ` Julien Grall
2019-08-12 14:53 ` Paul Durrant
1 sibling, 0 replies; 38+ messages in thread
From: Julien Grall @ 2019-08-07 10:22 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu,
George Dunlap, VolodymyrBabchuk, Suravee Suthikulpanit,
Andrew Cooper, xen-devel, Brian Woods, Roger Pau Monné
Hi Jan,
On 07/08/2019 10:55, Jan Beulich wrote:
> On 30.07.2019 15:44, Paul Durrant wrote:
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
>> * shared with the CPU, Xen has to make sure that the PT changes have
>> * reached the memory
>> */
>> - p2m->clean_pte = iommu_enabled &&
>> - !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>> + p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>
> I can't tell if the original code was meant to be this way, but I'm
> afraid your transformation is not correct: The prior construct,
> expanding iommu_has_feature(), was
The original code is meant to be this way. There are no need to clean the PTE on
update unless you have one IOMMU that is not able to snoop the cache.
iommu_has_feature(...) will return false when the IOMMU is not in use. The
iommu_enabled prevent to clean PTE on those setups.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...
2019-08-07 9:55 ` Jan Beulich
2019-08-07 10:22 ` Julien Grall
@ 2019-08-12 14:53 ` Paul Durrant
1 sibling, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-12 14:53 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Kevin Tian, Stefano Stabellini, VolodymyrBabchuk, Wei Liu,
Jun Nakajima, Andrew Cooper, George Dunlap, Julien Grall,
Suravee Suthikulpanit, xen-devel, Brian Woods, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 10:56
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; VolodymyrBabchuk
> <Volodymyr_Babchuk@epam.com>; George Dunlap <George.Dunlap@citrix.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 2/6] use is_iommu_enabled() where appropriate...
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
> > * shared with the CPU, Xen has to make sure that the PT changes have
> > * reached the memory
> > */
> > - p2m->clean_pte = iommu_enabled &&
> > - !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> > + p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>
> I can't tell if the original code was meant to be this way, but I'm
> afraid your transformation is not correct: The prior construct,
> expanding iommu_has_feature(), was
>
> iommu_enabled && !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
>
> which transforms through
>
> iommu_enabled && (!iommu_enabled || !test_bit(feature, dom_iommu(d)->features))
>
> to
>
> (iommu_enabled && !iommu_enabled) || (iommu_enabled && !test_bit(feature, dom_iommu(d)->features))
>
> and hence
>
> iommu_enabled && !test_bit(feature, dom_iommu(d)->features)
>
> whereas the new code simply is
>
> !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
>
> i.e.
>
> !iommu_enabled || !test_bit(feature, dom_iommu(d)->features)
Yes, somehow I'd read that the iommu_enabled was inverted in the first instance. I'll add a check of is_iommu_enabled() back into p2m_init().
>
> > @@ -766,7 +766,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
> > new_entry.sp = !!i;
> > new_entry.sa_p2mt = p2mt;
> > new_entry.access = p2ma;
> > - new_entry.snp = (iommu_enabled && iommu_snoop);
> > + new_entry.snp = is_iommu_enabled(p2m->domain) && iommu_snoop;
>
> Please use d here.
>
> Seeing that this is the last change in x86/mm/, did you overlook
> the use in p2m_pt_set_entry()? Or is this meant to go on top of
> Alexandru's "x86/mm: Clean IOMMU flags from p2m-pt code" (which
> should then be noted in a post-commit-message remark)?
Yes, it needs to go on top of Alexandru's patch. I said the series is based on that patch in the cover letter but I can state it here as well if it helps.
>
> > @@ -556,7 +556,7 @@ int iommu_do_domctl(
> > {
> > int ret = -ENODEV;
> >
> > - if ( !iommu_enabled )
> > + if ( !is_iommu_enabled(d) )
> > return -ENOSYS;
>
> ENOSYS was wrong here from the beginning, but it certainly gets
> worse with this no longer being a system wide property. Please
> change to EOPNOTSUPP or some other suitable one.
Sure. I'll go with EOPNOTSUPP.
>
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -864,7 +864,7 @@ static int pci_clean_dpci_irqs(struct domain *d)
>
> Above here there's another use in pci_enable_acs(), which should
> imo act on pdev->domain.
Oh yes, I'll fix that.
>
> There's another use in flask_iommu_resource_use_perm(). All
> callers of the function hold a struct domain * in their hands,
> which I think they should pass into this function such that the
> conditional can be replaced.
I wasn't sure about this one. It looks more like the perm passed back is based on the hardware features available but I guess it will DTRT if the IOMMU is not enabled for the domain.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
2019-07-30 13:44 ` [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu Paul Durrant
2019-07-30 13:44 ` [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-01 8:05 ` Alexandru Stefan ISAILA
2019-08-07 10:31 ` Jan Beulich
2019-07-30 13:44 ` [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static Paul Durrant
` (2 subsequent siblings)
5 siblings, 2 replies; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel
Cc: Petre Pircalabu, Stefano Stabellini, Razvan Cojocaru, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Paul Durrant, Tamas K Lengyel,
Jan Beulich, Alexandru Isaila, Volodymyr Babchuk,
Roger Pau Monné
Now that there is a per-domain IOMMU enable flag, which should be enabled if
any device is going to be passed through, stop deferring page table
construction until the assignment is done. Also don't tear down the tables
again when the last device is de-assigned; defer that task until domain
destruction.
This allows the has_iommu_pt() helper and iommu_status enumeration to be
removed. Calls to has_iommu_pt() are simply replaced by calls to
is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also
be replaced by calls to iommu_use_hap_pt().
The arch_iommu_populate_page_table() and iommu_construct() functions become
redundant, as does the 'strict mode' dom0 page_list mapping code in
iommu_hwdom_init(), and iommu_teardown() can be made static is its only
remaining caller, iommu_domain_destroy(), is within the same source
module.
All in all, about 220 lines of code are rmeoved.
NOTE: This patch will cause a small amount of extra resource to be used
to accommodate IOMMU page tables that may never be used, since the
per-domain IOMMU flag enable flag is currently set to the value
of the global iommu_enable flag. A subsequent patch will add an
option to the toolstack to allow it to be turned off if there is
no intention to assign passthrough hardware to the domain.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Alexandru Isaila <aisaila@bitdefender.com>
Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
---
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/dom0_build.c | 2 +-
xen/arch/x86/hvm/mtrr.c | 5 +-
xen/arch/x86/mm/mem_sharing.c | 2 +-
xen/arch/x86/mm/paging.c | 2 +-
xen/arch/x86/x86_64/mm.c | 2 +-
xen/common/memory.c | 4 +-
xen/common/vm_event.c | 2 +-
xen/drivers/passthrough/device_tree.c | 11 ---
xen/drivers/passthrough/iommu.c | 134 ++++++--------------------
xen/drivers/passthrough/pci.c | 12 ---
xen/drivers/passthrough/vtd/iommu.c | 10 +-
xen/drivers/passthrough/x86/iommu.c | 95 ------------------
xen/include/asm-arm/iommu.h | 2 +-
xen/include/asm-x86/iommu.h | 2 +-
xen/include/xen/iommu.h | 16 ---
xen/include/xen/sched.h | 2 -
17 files changed, 42 insertions(+), 263 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c5cea25caa..b091c64e1e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1056,7 +1056,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
!mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
p2m_free_entry(p2m, orig_pte, level);
- if ( has_iommu_pt(p2m->domain) &&
+ if ( is_iommu_enabled(p2m->domain) &&
(lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
{
unsigned int flush_flags = 0;
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index d381784edd..7cfab2dc25 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -365,7 +365,7 @@ unsigned long __init dom0_compute_nr_pages(
}
need_paging = is_hvm_domain(d) &&
- (!iommu_hap_pt_share || !paging_mode_hap(d));
+ (!iommu_use_hap_pt(d) || !paging_mode_hap(d));
for ( ; ; need_paging = false )
{
nr_pages = get_memsize(&dom0_size, avail);
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 7ccd85bcea..829b089e79 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1,
void memory_type_changed(struct domain *d)
{
- if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] )
+ if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) && d->vcpu &&
+ d->vcpu[0] )
{
p2m_memory_type_changed(d);
flush_all(FLUSH_CACHE);
@@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
return MTRR_TYPE_UNCACHABLE;
}
- if ( !has_iommu_pt(d) && !cache_flush_permitted(d) )
+ if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
{
*ipat = 1;
return MTRR_TYPE_WRBACK;
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index a5fe89e339..efb8821768 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1664,7 +1664,7 @@ int mem_sharing_domctl(struct domain *d, struct xen_domctl_mem_sharing_op *mec)
case XEN_DOMCTL_MEM_SHARING_CONTROL:
{
rc = 0;
- if ( unlikely(has_iommu_pt(d) && mec->u.enable) )
+ if ( unlikely(is_iommu_enabled(d) && mec->u.enable) )
rc = -EXDEV;
else
d->arch.hvm.mem_sharing_enabled = mec->u.enable;
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 69aa228e46..d9a52c4db4 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -213,7 +213,7 @@ int paging_log_dirty_enable(struct domain *d, bool_t log_global)
{
int ret;
- if ( has_iommu_pt(d) && log_global )
+ if ( is_iommu_enabled(d) && log_global )
{
/*
* Refuse to turn on global log-dirty mode
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 1919cae18b..827b3f5e27 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1433,7 +1433,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
* shared or being kept in sync then newly added memory needs to be
* mapped here.
*/
- if ( has_iommu_pt(hardware_domain) &&
+ if ( is_iommu_enabled(hardware_domain) &&
!iommu_use_hap_pt(hardware_domain) &&
!need_iommu_pt_sync(hardware_domain) )
{
diff --git a/xen/common/memory.c b/xen/common/memory.c
index d9b35a608c..71445c2f53 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -823,7 +823,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
xatp->gpfn += start;
xatp->size -= start;
- if ( has_iommu_pt(d) )
+ if ( is_iommu_enabled(d) )
this_cpu(iommu_dont_flush_iotlb) = 1;
while ( xatp->size > done )
@@ -844,7 +844,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
}
}
- if ( has_iommu_pt(d) )
+ if ( is_iommu_enabled(d) )
{
int ret;
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 2a1c87e44b..3b18195ebf 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -630,7 +630,7 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec)
/* No paging if iommu is used */
rc = -EMLINK;
- if ( unlikely(has_iommu_pt(d)) )
+ if ( unlikely(is_iommu_enabled(d)) )
break;
rc = -EXDEV;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 12f2c4c3f2..ea9fd54e3b 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -40,17 +40,6 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
if ( !list_empty(&dev->domain_list) )
goto fail;
- /*
- * The hwdom is forced to use IOMMU for protecting assigned
- * device. Therefore the IOMMU data is already set up.
- */
- ASSERT(!is_hardware_domain(d) ||
- hd->status == IOMMU_STATUS_initialized);
-
- rc = iommu_construct(d);
- if ( rc )
- goto fail;
-
/* The flag field doesn't matter to DT device. */
rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), 0);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index d0200d82f0..30976b4406 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -146,6 +146,17 @@ static int __init parse_dom0_iommu_param(const char *s)
}
custom_param("dom0-iommu", parse_dom0_iommu_param);
+static void __hwdom_init check_hwdom_reqs(struct domain *d)
+{
+ if ( iommu_hwdom_none || !paging_mode_translate(d) )
+ return;
+
+ arch_iommu_check_autotranslated_hwdom(d);
+
+ iommu_hwdom_passthrough = false;
+ iommu_hwdom_strict = true;
+}
+
int iommu_domain_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
@@ -159,129 +170,44 @@ int iommu_domain_init(struct domain *d)
return ret;
hd->platform_ops = iommu_get_ops();
- return hd->platform_ops->init(d);
-}
+ ret = hd->platform_ops->init(d);
+ if ( ret )
+ return ret;
-static void __hwdom_init check_hwdom_reqs(struct domain *d)
-{
- if ( iommu_hwdom_none || !paging_mode_translate(d) )
- return;
+ /*
+ * NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
+ * in-sync with their assigned pages because all host RAM will be
+ * mapped during hwdom_init().
+ */
+ if ( is_hardware_domain(d) )
+ check_hwdom_reqs(d); /* may modify iommu_hwdom_strict */
- arch_iommu_check_autotranslated_hwdom(d);
+ if ( !is_hardware_domain(d) || iommu_hwdom_strict )
+ hd->need_sync = !iommu_use_hap_pt(d);
- iommu_hwdom_passthrough = false;
- iommu_hwdom_strict = true;
+ return 0;
}
void __hwdom_init iommu_hwdom_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- check_hwdom_reqs(d);
-
if ( !is_iommu_enabled(d) )
return;
register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
- hd->status = IOMMU_STATUS_initializing;
- /*
- * NB: relaxed hw domains don't need sync because all ram is already
- * mapped in the iommu page tables.
- */
- hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
- if ( need_iommu_pt_sync(d) )
- {
- struct page_info *page;
- unsigned int i = 0, flush_flags = 0;
- int rc = 0;
-
- page_list_for_each ( page, &d->page_list )
- {
- unsigned long mfn = mfn_x(page_to_mfn(page));
- unsigned long dfn = mfn_to_gmfn(d, mfn);
- unsigned int mapping = IOMMUF_readable;
- int ret;
-
- if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
- ((page->u.inuse.type_info & PGT_type_mask)
- == PGT_writable_page) )
- mapping |= IOMMUF_writable;
-
- ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
- &flush_flags);
-
- if ( !rc )
- rc = ret;
-
- if ( !(i++ & 0xfffff) )
- process_pending_softirqs();
- }
-
- /* Use while-break to avoid compiler warning */
- while ( iommu_iotlb_flush_all(d, flush_flags) )
- break;
-
- if ( rc )
- printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
- d->domain_id, rc);
- }
-
hd->platform_ops->hwdom_init(d);
-
- hd->status = IOMMU_STATUS_initialized;
}
-void iommu_teardown(struct domain *d)
+static void iommu_teardown(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- hd->status = IOMMU_STATUS_disabled;
hd->platform_ops->teardown(d);
tasklet_schedule(&iommu_pt_cleanup_tasklet);
}
-int iommu_construct(struct domain *d)
-{
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( hd->status == IOMMU_STATUS_initialized )
- return 0;
-
- hd->status = IOMMU_STATUS_initializing;
-
- if ( !iommu_use_hap_pt(d) )
- {
- int rc;
-
- hd->need_sync = true;
-
- rc = arch_iommu_populate_page_table(d);
- if ( rc )
- {
- if ( rc != -ERESTART )
- {
- hd->need_sync = false;
- hd->status = IOMMU_STATUS_disabled;
- }
-
- return rc;
- }
- }
-
- hd->status = IOMMU_STATUS_initialized;
-
- /*
- * There may be dirty cache lines when a device is assigned
- * and before has_iommu_pt(d) becoming true, this will cause
- * memory_type_changed lose effect if memory type changes.
- * Call memory_type_changed here to amend this.
- */
- memory_type_changed(d);
-
- return 0;
-}
-
void iommu_domain_destroy(struct domain *d)
{
if ( !is_iommu_enabled(d) )
@@ -574,11 +500,8 @@ int iommu_do_domctl(
void iommu_share_p2m_table(struct domain* d)
{
ASSERT(hap_enabled(d));
- /*
- * iommu_use_hap_pt(d) cannot be used here because during domain
- * construction has_iommu_pt(d) will always return false here.
- */
- if ( is_iommu_enabled(d) && iommu_hap_pt_share )
+
+ if ( iommu_use_hap_pt(d) )
iommu_get_ops()->share_p2m(d);
}
@@ -625,8 +548,7 @@ static void iommu_dump_p2m_table(unsigned char key)
ops = iommu_get_ops();
for_each_domain(d)
{
- if ( is_hardware_domain(d) ||
- dom_iommu(d)->status < IOMMU_STATUS_initialized )
+ if ( !is_iommu_enabled(d) )
continue;
if ( iommu_use_hap_pt(d) )
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 61b5b330ca..25ff10f4cb 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1434,13 +1434,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
if ( !pcidevs_trylock() )
return -ERESTART;
- rc = iommu_construct(d);
- if ( rc )
- {
- pcidevs_unlock();
- return rc;
- }
-
pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
if ( !pdev )
{
@@ -1469,8 +1462,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
}
done:
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
pcidevs_unlock();
return rc;
@@ -1519,9 +1510,6 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
pdev->fault.count = 0;
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
-
return ret;
}
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 01f0bc4689..4ac5da197a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1759,15 +1759,7 @@ static void iommu_domain_teardown(struct domain *d)
ASSERT(is_iommu_enabled(d));
- /*
- * We can't use iommu_use_hap_pt here because either IOMMU state
- * is already changed to IOMMU_STATUS_disabled at this point or
- * has always been IOMMU_STATUS_disabled.
- *
- * We also need to test if HAP is enabled because PV guests can
- * enter this path too.
- */
- if ( hap_enabled(d) && iommu_hap_pt_share )
+ if ( iommu_use_hap_pt(d) )
return;
spin_lock(&hd->arch.mapping_lock);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 9879558c17..47a3e55213 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -81,101 +81,6 @@ int __init iommu_setup_hpet_msi(struct msi_desc *msi)
return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : -ENODEV;
}
-int arch_iommu_populate_page_table(struct domain *d)
-{
- struct page_info *page;
- int rc = 0, n = 0;
-
- spin_lock(&d->page_alloc_lock);
-
- if ( unlikely(d->is_dying) )
- rc = -ESRCH;
-
- while ( !rc && (page = page_list_remove_head(&d->page_list)) )
- {
- if ( is_hvm_domain(d) ||
- (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
- {
- unsigned long mfn = mfn_x(page_to_mfn(page));
- unsigned long gfn = mfn_to_gmfn(d, mfn);
- unsigned int flush_flags = 0;
-
- if ( gfn != gfn_x(INVALID_GFN) )
- {
- ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
- BUG_ON(SHARED_M2P(gfn));
- rc = iommu_map(d, _dfn(gfn), _mfn(mfn), PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable,
- &flush_flags);
-
- /*
- * We may be working behind the back of a running guest, which
- * may change the type of a page at any time. We can't prevent
- * this (for instance, by bumping the type count while mapping
- * the page) without causing legitimate guest type-change
- * operations to fail. So after adding the page to the IOMMU,
- * check again to make sure this is still valid. NB that the
- * writable entry in the iommu is harmless until later, when
- * the actual device gets assigned.
- */
- if ( !rc && !is_hvm_domain(d) &&
- ((page->u.inuse.type_info & PGT_type_mask) !=
- PGT_writable_page) )
- {
- rc = iommu_unmap(d, _dfn(gfn), PAGE_ORDER_4K, &flush_flags);
- /* If the type changed yet again, simply force a retry. */
- if ( !rc && ((page->u.inuse.type_info & PGT_type_mask) ==
- PGT_writable_page) )
- rc = -ERESTART;
- }
- }
- if ( rc )
- {
- page_list_add(page, &d->page_list);
- break;
- }
- }
- page_list_add_tail(page, &d->arch.relmem_list);
- if ( !(++n & 0xff) && !page_list_empty(&d->page_list) &&
- hypercall_preempt_check() )
- rc = -ERESTART;
- }
-
- if ( !rc )
- {
- /*
- * The expectation here is that generally there are many normal pages
- * on relmem_list (the ones we put there) and only few being in an
- * offline/broken state. The latter ones are always at the head of the
- * list. Hence we first move the whole list, and then move back the
- * first few entries.
- */
- page_list_move(&d->page_list, &d->arch.relmem_list);
- while ( !page_list_empty(&d->page_list) &&
- (page = page_list_first(&d->page_list),
- (page->count_info & (PGC_state|PGC_broken))) )
- {
- page_list_del(page, &d->page_list);
- page_list_add_tail(page, &d->arch.relmem_list);
- }
- }
-
- spin_unlock(&d->page_alloc_lock);
-
- if ( !rc )
- /*
- * flush_flags are not tracked across hypercall pre-emption so
- * assume a full flush is necessary.
- */
- rc = iommu_iotlb_flush_all(
- d, IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
-
- if ( rc && rc != -ERESTART )
- iommu_teardown(d);
-
- return rc;
-}
-
void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d)
{
if ( !is_iommu_enabled(d) )
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 904c9aec11..e5050636d7 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -21,7 +21,7 @@ struct arch_iommu
};
/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) (has_iommu_pt(d))
+#define iommu_use_hap_pt(d) (is_iommu_enabled(d))
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index facf835ada..6d024d5c0e 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -81,7 +81,7 @@ extern const struct iommu_init_ops *iommu_init_ops;
/* Are we using the domain P2M table as its IOMMU pagetable? */
#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && has_iommu_pt(d) && iommu_hap_pt_share)
+ (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 48f87480a7..5b9611a134 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -74,15 +74,9 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn);
void arch_iommu_domain_destroy(struct domain *d);
int arch_iommu_domain_init(struct domain *d);
-int arch_iommu_populate_page_table(struct domain *d);
void arch_iommu_check_autotranslated_hwdom(struct domain *d);
void arch_iommu_hwdom_init(struct domain *d);
-int iommu_construct(struct domain *d);
-
-/* Function used internally, use iommu_domain_destroy */
-void iommu_teardown(struct domain *d);
-
/*
* The following flags are passed to map operations and passed by lookup
* operations.
@@ -249,13 +243,6 @@ struct iommu_ops {
# define iommu_vcall iommu_call
#endif
-enum iommu_status
-{
- IOMMU_STATUS_disabled,
- IOMMU_STATUS_initializing,
- IOMMU_STATUS_initialized
-};
-
struct domain_iommu {
struct arch_iommu arch;
@@ -270,9 +257,6 @@ struct domain_iommu {
/* Features supported by the IOMMU */
DECLARE_BITMAP(features, IOMMU_FEAT_count);
- /* Status of guest IOMMU mappings */
- enum iommu_status status;
-
/*
* Does the guest reqire mappings to be synchonized, to maintain
* the default dfn == pfn map. (See comment on dfn at the top of
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index bad9734626..538be7120b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -964,10 +964,8 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
}
#ifdef CONFIG_HAS_PASSTHROUGH
-#define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
#else
-#define has_iommu_pt(d) false
#define need_iommu_pt_sync(d) false
#endif
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-07-30 13:44 ` [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-08-01 8:05 ` Alexandru Stefan ISAILA
2019-08-07 10:31 ` Jan Beulich
1 sibling, 0 replies; 38+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-08-01 8:05 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Petre Ovidiu PIRCALABU, Stefano Stabellini, Razvan Cojocaru,
Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall, Tamas K Lengyel,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
On 30.07.2019 16:44, Paul Durrant wrote:
> Now that there is a per-domain IOMMU enable flag, which should be enabled if
> any device is going to be passed through, stop deferring page table
> construction until the assignment is done. Also don't tear down the tables
> again when the last device is de-assigned; defer that task until domain
> destruction.
>
> This allows the has_iommu_pt() helper and iommu_status enumeration to be
> removed. Calls to has_iommu_pt() are simply replaced by calls to
> is_iommu_enabled(). Remaining open-code tests of iommu_hap_pt_share can also
> be replaced by calls to iommu_use_hap_pt().
> The arch_iommu_populate_page_table() and iommu_construct() functions become
> redundant, as does the 'strict mode' dom0 page_list mapping code in
> iommu_hwdom_init(), and iommu_teardown() can be made static is its only
> remaining caller, iommu_domain_destroy(), is within the same source
> module.
>
> All in all, about 220 lines of code are rmeoved.
>
> NOTE: This patch will cause a small amount of extra resource to be used
> to accommodate IOMMU page tables that may never be used, since the
> per-domain IOMMU flag enable flag is currently set to the value
> of the global iommu_enable flag. A subsequent patch will add an
> option to the toolstack to allow it to be turned off if there is
> no intention to assign passthrough hardware to the domain.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
For the vm_event part.
Reviewed-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Alexandru Isaila <aisaila@bitdefender.com>
> Cc: Petre Pircalabu <ppircalabu@bitdefender.com>
> ---
> xen/arch/arm/p2m.c | 2 +-
> xen/arch/x86/dom0_build.c | 2 +-
> xen/arch/x86/hvm/mtrr.c | 5 +-
> xen/arch/x86/mm/mem_sharing.c | 2 +-
> xen/arch/x86/mm/paging.c | 2 +-
> xen/arch/x86/x86_64/mm.c | 2 +-
> xen/common/memory.c | 4 +-
> xen/common/vm_event.c | 2 +-
> xen/drivers/passthrough/device_tree.c | 11 ---
> xen/drivers/passthrough/iommu.c | 134 ++++++--------------------
> xen/drivers/passthrough/pci.c | 12 ---
> xen/drivers/passthrough/vtd/iommu.c | 10 +-
> xen/drivers/passthrough/x86/iommu.c | 95 ------------------
> xen/include/asm-arm/iommu.h | 2 +-
> xen/include/asm-x86/iommu.h | 2 +-
> xen/include/xen/iommu.h | 16 ---
> xen/include/xen/sched.h | 2 -
> 17 files changed, 42 insertions(+), 263 deletions(-)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-07-30 13:44 ` [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-08-01 8:05 ` Alexandru Stefan ISAILA
@ 2019-08-07 10:31 ` Jan Beulich
2019-08-12 15:41 ` Paul Durrant
1 sibling, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-07 10:31 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Alexandru Isaila,
xen-devel, VolodymyrBabchuk, Roger Pau Monné
On 30.07.2019 15:44, Paul Durrant wrote:
> NOTE: This patch will cause a small amount of extra resource to be used
> to accommodate IOMMU page tables that may never be used, since the
> per-domain IOMMU flag enable flag is currently set to the value
> of the global iommu_enable flag. A subsequent patch will add an
> option to the toolstack to allow it to be turned off if there is
> no intention to assign passthrough hardware to the domain.
In particular if the default of this is going to be "true" (I
didn't look at that patch yet, but the wording above makes me
assume so), in auto-ballooning mode without shared page tables
more memory should imo be ballooned out of Dom0 now. It has
always been a bug that IOMMU page tables weren't accounted for,
but it would become even more prominent then.
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1,
>
> void memory_type_changed(struct domain *d)
> {
> - if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] )
> + if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) && d->vcpu &&
> + d->vcpu[0] )
As a really minor comment - I think it wouldn't be bad for both
d->vcpu references to end up on the same line.
> @@ -625,8 +548,7 @@ static void iommu_dump_p2m_table(unsigned char key)
> ops = iommu_get_ops();
> for_each_domain(d)
> {
> - if ( is_hardware_domain(d) ||
> - dom_iommu(d)->status < IOMMU_STATUS_initialized )
> + if ( !is_iommu_enabled(d) )
> continue;
Why do you drop the hwdom check here?
> --- a/xen/include/asm-arm/iommu.h
> +++ b/xen/include/asm-arm/iommu.h
> @@ -21,7 +21,7 @@ struct arch_iommu
> };
>
> /* Always share P2M Table between the CPU and the IOMMU */
> -#define iommu_use_hap_pt(d) (has_iommu_pt(d))
> +#define iommu_use_hap_pt(d) (is_iommu_enabled(d))
I'd suggest dropping the stray outer pair of parentheses at the
same time.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-07 10:31 ` Jan Beulich
@ 2019-08-12 15:41 ` Paul Durrant
2019-08-12 16:26 ` Paul Durrant
2019-08-27 7:45 ` Jan Beulich
0 siblings, 2 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-12 15:41 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, Ian Jackson,
Alexandru Isaila, xen-devel, VolodymyrBabchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 11:32
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Alexandru Isaila
> <aisaila@bitdefender.com>; Petre Pircalabu <ppircalabu@bitdefender.com>; Razvan Cojocaru
> <rcojocaru@bitdefender.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; VolodymyrBabchuk <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>; Tamas K Lengyel
> <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > NOTE: This patch will cause a small amount of extra resource to be used
> > to accommodate IOMMU page tables that may never be used, since the
> > per-domain IOMMU flag enable flag is currently set to the value
> > of the global iommu_enable flag. A subsequent patch will add an
> > option to the toolstack to allow it to be turned off if there is
> > no intention to assign passthrough hardware to the domain.
>
> In particular if the default of this is going to be "true" (I
> didn't look at that patch yet, but the wording above makes me
> assume so), in auto-ballooning mode without shared page tables
> more memory should imo be ballooned out of Dom0 now. It has
> always been a bug that IOMMU page tables weren't accounted for,
> but it would become even more prominent then.
Ultimately, once the whole series is applied, then nothing much should change for those specifying passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a domain that was not originally created with IOMMU pagetables.
With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess I'll take a look at the auto-ballooning code and see what needs to be done.
>
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, hvm_load_mtrr_msr, 1,
> >
> > void memory_type_changed(struct domain *d)
> > {
> > - if ( (has_iommu_pt(d) || cache_flush_permitted(d)) && d->vcpu && d->vcpu[0] )
> > + if ( (is_iommu_enabled(d) || cache_flush_permitted(d)) && d->vcpu &&
> > + d->vcpu[0] )
>
> As a really minor comment - I think it wouldn't be bad for both
> d->vcpu references to end up on the same line.
Ok.
>
> > @@ -625,8 +548,7 @@ static void iommu_dump_p2m_table(unsigned char key)
> > ops = iommu_get_ops();
> > for_each_domain(d)
> > {
> > - if ( is_hardware_domain(d) ||
> > - dom_iommu(d)->status < IOMMU_STATUS_initialized )
> > + if ( !is_iommu_enabled(d) )
> > continue;
>
> Why do you drop the hwdom check here?
Because is_iommu_enabled() for the h/w domain will always be true if iommu_enabled is true, so no need for a special case.
>
> > --- a/xen/include/asm-arm/iommu.h
> > +++ b/xen/include/asm-arm/iommu.h
> > @@ -21,7 +21,7 @@ struct arch_iommu
> > };
> >
> > /* Always share P2M Table between the CPU and the IOMMU */
> > -#define iommu_use_hap_pt(d) (has_iommu_pt(d))
> > +#define iommu_use_hap_pt(d) (is_iommu_enabled(d))
>
> I'd suggest dropping the stray outer pair of parentheses at the
> same time.
Ok, will do.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-12 15:41 ` Paul Durrant
@ 2019-08-12 16:26 ` Paul Durrant
2019-08-14 9:39 ` Paul Durrant
2019-08-27 7:47 ` Jan Beulich
2019-08-27 7:45 ` Jan Beulich
1 sibling, 2 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-12 16:26 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim (Xen.org),
George Dunlap, 'Julien Grall', 'Tamas K Lengyel',
Ian Jackson, 'Alexandru Isaila',
'xen-devel@lists.xenproject.org',
'VolodymyrBabchuk',
Roger Pau Monne
> -----Original Message-----
[snip]
> >
> > On 30.07.2019 15:44, Paul Durrant wrote:
> > > NOTE: This patch will cause a small amount of extra resource to be used
> > > to accommodate IOMMU page tables that may never be used, since the
> > > per-domain IOMMU flag enable flag is currently set to the value
> > > of the global iommu_enable flag. A subsequent patch will add an
> > > option to the toolstack to allow it to be turned off if there is
> > > no intention to assign passthrough hardware to the domain.
> >
> > In particular if the default of this is going to be "true" (I
> > didn't look at that patch yet, but the wording above makes me
> > assume so), in auto-ballooning mode without shared page tables
> > more memory should imo be ballooned out of Dom0 now. It has
> > always been a bug that IOMMU page tables weren't accounted for,
> > but it would become even more prominent then.
>
> Ultimately, once the whole series is applied, then nothing much should change for those specifying
> passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a
> domain that was not originally created with IOMMU pagetables.
> With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess I'll
> take a look at the auto-ballooning code and see what needs to be done.
>
Ok, I've had a look...
I could make a rough calculation in libxl_domain_need_memory() based on the domain's max_memkb and an assumption of a 4 level translation with 512 PTEs per level, or would prefer such guestimation to be overridable using an xl.cfg parameter in a broadly similar way to shadow_memkb?
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-12 16:26 ` Paul Durrant
@ 2019-08-14 9:39 ` Paul Durrant
2019-08-27 7:48 ` Jan Beulich
2019-08-27 7:47 ` Jan Beulich
1 sibling, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 9:39 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim (Xen.org),
George Dunlap, 'Julien Grall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
Ian Jackson, 'VolodymyrBabchuk',
Roger Pau Monne
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul Durrant
> Sent: 12 August 2019 17:26
> To: 'Jan Beulich' <jbeulich@suse.com>
> Cc: 'Petre Pircalabu' <ppircalabu@bitdefender.com>; 'Stefano Stabellini' <sstabellini@kernel.org>;
> 'Wei Liu' <wl@xen.org>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; 'Konrad Rzeszutek Wilk'
> <konrad.wilk@oracle.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> George Dunlap <George.Dunlap@citrix.com>; 'Julien Grall' <julien.grall@arm.com>; 'Tamas K Lengyel'
> <tamas@tklengyel.com>; Ian Jackson <Ian.Jackson@citrix.com>; 'Alexandru Isaila'
> <aisaila@bitdefender.com>; 'xen-devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>;
> 'VolodymyrBabchuk' <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>
> > -----Original Message-----
> [snip]
> > >
> > > On 30.07.2019 15:44, Paul Durrant wrote:
> > > > NOTE: This patch will cause a small amount of extra resource to be used
> > > > to accommodate IOMMU page tables that may never be used, since the
> > > > per-domain IOMMU flag enable flag is currently set to the value
> > > > of the global iommu_enable flag. A subsequent patch will add an
> > > > option to the toolstack to allow it to be turned off if there is
> > > > no intention to assign passthrough hardware to the domain.
> > >
> > > In particular if the default of this is going to be "true" (I
> > > didn't look at that patch yet, but the wording above makes me
> > > assume so), in auto-ballooning mode without shared page tables
> > > more memory should imo be ballooned out of Dom0 now. It has
> > > always been a bug that IOMMU page tables weren't accounted for,
> > > but it would become even more prominent then.
> >
> > Ultimately, once the whole series is applied, then nothing much should change for those specifying
> > passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a
> > domain that was not originally created with IOMMU pagetables.
> > With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess
> I'll
> > take a look at the auto-ballooning code and see what needs to be done.
> >
>
> Ok, I've had a look...
>
> I could make a rough calculation in libxl_domain_need_memory() based on the domain's max_memkb and an
> assumption of a 4 level translation with 512 PTEs per level, or would prefer such guestimation to be
> overridable using an xl.cfg parameter in a broadly similar way to shadow_memkb?
>
I think I'm going to say no to this anyway since, as you say, the overhead as never been accounted for and having to make assumptions about the IOMMU table structure is not very attractive. Given that any issue is going to be transient (i.e. until patch #6 is committed) I don't think fixing auto-ballooning ought to be in scope for this series.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-14 9:39 ` Paul Durrant
@ 2019-08-27 7:48 ` Jan Beulich
2019-08-29 9:33 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-27 7:48 UTC (permalink / raw)
To: Paul Durrant
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim(Xen.org),
George Dunlap, 'JulienGrall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
IanJackson, 'VolodymyrBabchuk',
Roger Pau Monne
On 14.08.2019 11:39, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul Durrant
>> Sent: 12 August 2019 17:26
>> To: 'Jan Beulich' <jbeulich@suse.com>
>> Cc: 'Petre Pircalabu' <ppircalabu@bitdefender.com>; 'Stefano Stabellini' <sstabellini@kernel.org>;
>> 'Wei Liu' <wl@xen.org>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; 'Konrad Rzeszutek Wilk'
>> <konrad.wilk@oracle.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>;
>> George Dunlap <George.Dunlap@citrix.com>; 'Julien Grall' <julien.grall@arm.com>; 'Tamas K Lengyel'
>> <tamas@tklengyel.com>; Ian Jackson <Ian.Jackson@citrix.com>; 'Alexandru Isaila'
>> <aisaila@bitdefender.com>; 'xen-devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>;
>> 'VolodymyrBabchuk' <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>>
>>> -----Original Message-----
>> [snip]
>>>>
>>>> On 30.07.2019 15:44, Paul Durrant wrote:
>>>>> NOTE: This patch will cause a small amount of extra resource to be used
>>>>> to accommodate IOMMU page tables that may never be used, since the
>>>>> per-domain IOMMU flag enable flag is currently set to the value
>>>>> of the global iommu_enable flag. A subsequent patch will add an
>>>>> option to the toolstack to allow it to be turned off if there is
>>>>> no intention to assign passthrough hardware to the domain.
>>>>
>>>> In particular if the default of this is going to be "true" (I
>>>> didn't look at that patch yet, but the wording above makes me
>>>> assume so), in auto-ballooning mode without shared page tables
>>>> more memory should imo be ballooned out of Dom0 now. It has
>>>> always been a bug that IOMMU page tables weren't accounted for,
>>>> but it would become even more prominent then.
>>>
>>> Ultimately, once the whole series is applied, then nothing much should change for those specifying
>>> passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a
>>> domain that was not originally created with IOMMU pagetables.
>>> With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess
>> I'll
>>> take a look at the auto-ballooning code and see what needs to be done.
>>>
>>
>> Ok, I've had a look...
>>
>> I could make a rough calculation in libxl_domain_need_memory() based on the domain's max_memkb and an
>> assumption of a 4 level translation with 512 PTEs per level, or would prefer such guestimation to be
>> overridable using an xl.cfg parameter in a broadly similar way to shadow_memkb?
>>
>
> I think I'm going to say no to this anyway since, as you say, the overhead as never been accounted for and having to make assumptions about the IOMMU table structure is not very attractive. Given that any issue is going to be transient (i.e. until patch #6 is committed) I don't think fixing auto-ballooning ought to be in scope for this series.
I'm afraid I disagree: The series extends a pre-existing problem
affecting some guests to all ones (at least by default).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-27 7:48 ` Jan Beulich
@ 2019-08-29 9:33 ` Paul Durrant
2019-08-29 9:52 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-08-29 9:33 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim (Xen.org),
George Dunlap, 'JulienGrall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
Ian Jackson, 'VolodymyrBabchuk',
Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 27 August 2019 08:49
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'JulienGrall' <julien.grall@arm.com>; 'Alexandru Isaila' <aisaila@bitdefender.com>; 'Petre
> Pircalabu' <ppircalabu@bitdefender.com>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; 'VolodymyrBabchuk'
> <Volodymyr_Babchuk@epam.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'xen-
> devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>; 'Konrad Rzeszutek Wilk'
> <konrad.wilk@oracle.com>; 'Tamas K Lengyel' <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; 'Wei
> Liu' <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>
> On 14.08.2019 11:39, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul Durrant
> >> Sent: 12 August 2019 17:26
> >> To: 'Jan Beulich' <jbeulich@suse.com>
> >> Cc: 'Petre Pircalabu' <ppircalabu@bitdefender.com>; 'Stefano Stabellini' <sstabellini@kernel.org>;
> >> 'Wei Liu' <wl@xen.org>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; 'Konrad Rzeszutek Wilk'
> >> <konrad.wilk@oracle.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>;
> >> George Dunlap <George.Dunlap@citrix.com>; 'Julien Grall' <julien.grall@arm.com>; 'Tamas K Lengyel'
> >> <tamas@tklengyel.com>; Ian Jackson <Ian.Jackson@citrix.com>; 'Alexandru Isaila'
> >> <aisaila@bitdefender.com>; 'xen-devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>;
> >> 'VolodymyrBabchuk' <Volodymyr_Babchuk@epam.com>; Roger Pau Monne <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
> >>
> >>> -----Original Message-----
> >> [snip]
> >>>>
> >>>> On 30.07.2019 15:44, Paul Durrant wrote:
> >>>>> NOTE: This patch will cause a small amount of extra resource to be used
> >>>>> to accommodate IOMMU page tables that may never be used, since the
> >>>>> per-domain IOMMU flag enable flag is currently set to the value
> >>>>> of the global iommu_enable flag. A subsequent patch will add an
> >>>>> option to the toolstack to allow it to be turned off if there is
> >>>>> no intention to assign passthrough hardware to the domain.
> >>>>
> >>>> In particular if the default of this is going to be "true" (I
> >>>> didn't look at that patch yet, but the wording above makes me
> >>>> assume so), in auto-ballooning mode without shared page tables
> >>>> more memory should imo be ballooned out of Dom0 now. It has
> >>>> always been a bug that IOMMU page tables weren't accounted for,
> >>>> but it would become even more prominent then.
> >>>
> >>> Ultimately, once the whole series is applied, then nothing much should change for those specifying
> >>> passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a
> >>> domain that was not originally created with IOMMU pagetables.
> >>> With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess
> >> I'll
> >>> take a look at the auto-ballooning code and see what needs to be done.
> >>>
> >>
> >> Ok, I've had a look...
> >>
> >> I could make a rough calculation in libxl_domain_need_memory() based on the domain's max_memkb and
> an
> >> assumption of a 4 level translation with 512 PTEs per level, or would prefer such guestimation to
> be
> >> overridable using an xl.cfg parameter in a broadly similar way to shadow_memkb?
> >>
> >
> > I think I'm going to say no to this anyway since, as you say, the overhead as never been accounted
> for and having to make assumptions about the IOMMU table structure is not very attractive. Given that
> any issue is going to be transient (i.e. until patch #6 is committed) I don't think fixing auto-
> ballooning ought to be in scope for this series.
>
> I'm afraid I disagree: The series extends a pre-existing problem
> affecting some guests to all ones (at least by default).
TBH I've seen sufficient numbers of domain create failures when using auto-ballooning that I stopped using it some time ago (besides the fact that it's slow). If you're happy with the simplistic double-the-page-table-reservation calculation then I can add that but IMO it would be better to add another patch to just remove auto-ballooning.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-29 9:33 ` Paul Durrant
@ 2019-08-29 9:52 ` Jan Beulich
2019-08-29 10:20 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-29 9:52 UTC (permalink / raw)
To: Paul Durrant
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim(Xen.org),
George Dunlap, 'JulienGrall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
IanJackson, 'VolodymyrBabchuk',
Roger Pau Monne
On 29.08.2019 11:33, Paul Durrant wrote:
> TBH I've seen sufficient numbers of domain create failures when using
> auto-ballooning that I stopped using it some time ago (besides the fact
> that it's slow). If you're happy with the simplistic
> double-the-page-table-reservation calculation then I can add that but
> IMO it would be better to add another patch to just remove auto-ballooning.
I'd not be overly happy, but I could live with this. But this needs
consent by others, not the least the tool stack maintainers.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-29 9:52 ` Jan Beulich
@ 2019-08-29 10:20 ` Paul Durrant
2019-08-29 10:33 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-08-29 10:20 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim (Xen.org),
George Dunlap, 'JulienGrall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
Ian Jackson, 'VolodymyrBabchuk',
Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 29 August 2019 10:52
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'JulienGrall' <julien.grall@arm.com>; 'Alexandru Isaila' <aisaila@bitdefender.com>; 'Petre
> Pircalabu' <ppircalabu@bitdefender.com>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; 'VolodymyrBabchuk'
> <Volodymyr_Babchuk@epam.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'xen-
> devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>; 'Konrad Rzeszutek Wilk'
> <konrad.wilk@oracle.com>; 'Tamas K Lengyel' <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; 'Wei
> Liu' <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>
> On 29.08.2019 11:33, Paul Durrant wrote:
> > TBH I've seen sufficient numbers of domain create failures when using
> > auto-ballooning that I stopped using it some time ago (besides the fact
> > that it's slow). If you're happy with the simplistic
> > double-the-page-table-reservation calculation then I can add that but
> > IMO it would be better to add another patch to just remove auto-ballooning.
>
> I'd not be overly happy, but I could live with this. But this needs
> consent by others, not the least the tool stack maintainers.
>
Ok. I'll deal with that later. Looking again though, I'm not sure what you mean by 'the amount reserved for page tables'? Do you mean 'shadow_memkb' or something else?
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-29 10:20 ` Paul Durrant
@ 2019-08-29 10:33 ` Jan Beulich
0 siblings, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2019-08-29 10:33 UTC (permalink / raw)
To: Paul Durrant
Cc: 'Petre Pircalabu', 'Stefano Stabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim(Xen.org),
George Dunlap, 'JulienGrall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
IanJackson, 'VolodymyrBabchuk',
Roger Pau Monne
On 29.08.2019 12:20, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 29 August 2019 10:52
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: 'JulienGrall' <julien.grall@arm.com>; 'Alexandru Isaila' <aisaila@bitdefender.com>; 'Petre
>> Pircalabu' <ppircalabu@bitdefender.com>; 'Razvan Cojocaru' <rcojocaru@bitdefender.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
>> <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; 'VolodymyrBabchuk'
>> <Volodymyr_Babchuk@epam.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'xen-
>> devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>; 'Konrad Rzeszutek Wilk'
>> <konrad.wilk@oracle.com>; 'Tamas K Lengyel' <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; 'Wei
>> Liu' <wl@xen.org>
>> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>>
>> On 29.08.2019 11:33, Paul Durrant wrote:
>>> TBH I've seen sufficient numbers of domain create failures when using
>>> auto-ballooning that I stopped using it some time ago (besides the fact
>>> that it's slow). If you're happy with the simplistic
>>> double-the-page-table-reservation calculation then I can add that but
>>> IMO it would be better to add another patch to just remove auto-ballooning.
>>
>> I'd not be overly happy, but I could live with this. But this needs
>> consent by others, not the least the tool stack maintainers.
>>
>
> Ok. I'll deal with that later. Looking again though, I'm not sure what
> you mean by 'the amount reserved for page tables'? Do you mean
> 'shadow_memkb' or something else?
This one, I think, yes, or the value it gets defaulted to when not set
in the guest config file.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-12 16:26 ` Paul Durrant
2019-08-14 9:39 ` Paul Durrant
@ 2019-08-27 7:47 ` Jan Beulich
1 sibling, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2019-08-27 7:47 UTC (permalink / raw)
To: Paul Durrant
Cc: 'Petre Pircalabu', 'StefanoStabellini',
'Wei Liu', 'Razvan Cojocaru',
'Konrad Rzeszutek Wilk', Andrew Cooper, Tim(Xen.org),
George Dunlap, 'Julien Grall', 'Tamas K Lengyel',
'xen-devel@lists.xenproject.org',
'Alexandru Isaila',
Ian Jackson, 'VolodymyrBabchuk',
Roger Pau Monne
On 12.08.2019 18:26, Paul Durrant wrote:
>> -----Original Message-----
> [snip]
>>>
>>> On 30.07.2019 15:44, Paul Durrant wrote:
>>>> NOTE: This patch will cause a small amount of extra resource to be used
>>>> to accommodate IOMMU page tables that may never be used, since the
>>>> per-domain IOMMU flag enable flag is currently set to the value
>>>> of the global iommu_enable flag. A subsequent patch will add an
>>>> option to the toolstack to allow it to be turned off if there is
>>>> no intention to assign passthrough hardware to the domain.
>>>
>>> In particular if the default of this is going to be "true" (I
>>> didn't look at that patch yet, but the wording above makes me
>>> assume so), in auto-ballooning mode without shared page tables
>>> more memory should imo be ballooned out of Dom0 now. It has
>>> always been a bug that IOMMU page tables weren't accounted for,
>>> but it would become even more prominent then.
>>
>> Ultimately, once the whole series is applied, then nothing much should change for those specifying
>> passthrough h/w in an xl.cfg. The main difference will be that h/w cannot be passed through to a
>> domain that was not originally created with IOMMU pagetables.
>> With patches applied up to this point then, yes, every domain will get IOMMU page tables. I guess I'll
>> take a look at the auto-ballooning code and see what needs to be done.
>>
>
> Ok, I've had a look...
>
> I could make a rough calculation in libxl_domain_need_memory() based on
> the domain's max_memkb and an assumption of a 4 level translation with
> 512 PTEs per level, or would prefer such guestimation to be overridable
> using an xl.cfg parameter in a broadly similar way to shadow_memkb?
I've always been thinking that for the non-shared case the amount
reserved for page tables could simply be doubled.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-12 15:41 ` Paul Durrant
2019-08-12 16:26 ` Paul Durrant
@ 2019-08-27 7:45 ` Jan Beulich
2019-08-29 9:28 ` Paul Durrant
1 sibling, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-27 7:45 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, JulienGrall, Tamas K Lengyel, xen-devel,
Alexandru Isaila, IanJackson, VolodymyrBabchuk, RogerPau Monne
On 12.08.2019 17:41, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 07 August 2019 11:32
>>
>> On 30.07.2019 15:44, Paul Durrant wrote:
>>> @@ -625,8 +548,7 @@ static void iommu_dump_p2m_table(unsigned char key)
>>> ops = iommu_get_ops();
>>> for_each_domain(d)
>>> {
>>> - if ( is_hardware_domain(d) ||
>>> - dom_iommu(d)->status < IOMMU_STATUS_initialized )
>>> + if ( !is_iommu_enabled(d) )
>>> continue;
>>
>> Why do you drop the hwdom check here?
>
> Because is_iommu_enabled() for the h/w domain will always be true if
> iommu_enabled is true, so no need for a special case.
But the effect of the extra check was to _skip_ Dom0. If you mean to
change this, then you should say so (and why) in the description.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
2019-08-27 7:45 ` Jan Beulich
@ 2019-08-29 9:28 ` Paul Durrant
0 siblings, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-29 9:28 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, JulienGrall, Tamas K Lengyel, xen-devel,
Alexandru Isaila, Ian Jackson, VolodymyrBabchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 27 August 2019 08:46
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: JulienGrall <julien.grall@arm.com>; Alexandru Isaila <aisaila@bitdefender.com>; Petre Pircalabu
> <ppircalabu@bitdefender.com>; Razvan Cojocaru <rcojocaru@bitdefender.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; VolodymyrBabchuk
> <Volodymyr_Babchuk@epam.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tamas K Lengyel
> <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables
>
> On 12.08.2019 17:41, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 07 August 2019 11:32
> >>
> >> On 30.07.2019 15:44, Paul Durrant wrote:
> >>> @@ -625,8 +548,7 @@ static void iommu_dump_p2m_table(unsigned char key)
> >>> ops = iommu_get_ops();
> >>> for_each_domain(d)
> >>> {
> >>> - if ( is_hardware_domain(d) ||
> >>> - dom_iommu(d)->status < IOMMU_STATUS_initialized )
> >>> + if ( !is_iommu_enabled(d) )
> >>> continue;
> >>
> >> Why do you drop the hwdom check here?
> >
> > Because is_iommu_enabled() for the h/w domain will always be true if
> > iommu_enabled is true, so no need for a special case.
>
> But the effect of the extra check was to _skip_ Dom0. If you mean to
> change this, then you should say so (and why) in the description.
>
Ah, yes, it does still need to remain.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
` (2 preceding siblings ...)
2019-07-30 13:44 ` [Xen-devel] [PATCH 3/6] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-06 15:54 ` Jan Beulich
2019-07-30 13:44 ` [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
2019-07-30 13:44 ` [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
5 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich
This function is only ever called from within the same source module and
really has no business being declared xen/iommu.h. This patch relocates
the function ahead of the first called and makes it static.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/passthrough/pci.c | 92 +++++++++++++++++------------------
xen/include/xen/iommu.h | 1 -
2 files changed, 46 insertions(+), 47 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 25ff10f4cb..449a0ee13b 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -889,6 +889,52 @@ static int pci_clean_dpci_irqs(struct domain *d)
return 0;
}
+/* caller should hold the pcidevs_lock */
+static int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
+{
+ const struct domain_iommu *hd = dom_iommu(d);
+ struct pci_dev *pdev = NULL;
+ int ret = 0;
+
+ if ( !is_iommu_enabled(d) )
+ return -EINVAL;
+
+ ASSERT(pcidevs_locked());
+ pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
+ if ( !pdev )
+ return -ENODEV;
+
+ while ( pdev->phantom_stride )
+ {
+ devfn += pdev->phantom_stride;
+ if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
+ break;
+ ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+ pci_to_dev(pdev));
+ if ( !ret )
+ continue;
+
+ printk(XENLOG_G_ERR "d%d: deassign %04x:%02x:%02x.%u failed (%d)\n",
+ d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
+ return ret;
+ }
+
+ devfn = pdev->devfn;
+ ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+ pci_to_dev(pdev));
+ if ( ret )
+ {
+ dprintk(XENLOG_G_ERR,
+ "d%d: deassign device (%04x:%02x:%02x.%u) failed\n",
+ d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ return ret;
+ }
+
+ pdev->fault.count = 0;
+
+ return ret;
+}
+
int pci_release_devices(struct domain *d)
{
struct pci_dev *pdev;
@@ -1467,52 +1513,6 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
return rc;
}
-/* caller should hold the pcidevs_lock */
-int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
-{
- const struct domain_iommu *hd = dom_iommu(d);
- struct pci_dev *pdev = NULL;
- int ret = 0;
-
- if ( !is_iommu_enabled(d) )
- return -EINVAL;
-
- ASSERT(pcidevs_locked());
- pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
- if ( !pdev )
- return -ENODEV;
-
- while ( pdev->phantom_stride )
- {
- devfn += pdev->phantom_stride;
- if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
- break;
- ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
- pci_to_dev(pdev));
- if ( !ret )
- continue;
-
- printk(XENLOG_G_ERR "d%d: deassign %04x:%02x:%02x.%u failed (%d)\n",
- d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
- return ret;
- }
-
- devfn = pdev->devfn;
- ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
- pci_to_dev(pdev));
- if ( ret )
- {
- dprintk(XENLOG_G_ERR,
- "d%d: deassign device (%04x:%02x:%02x.%u) failed\n",
- d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
- return ret;
- }
-
- pdev->fault.count = 0;
-
- return ret;
-}
-
static int iommu_get_device_group(
struct domain *d, u16 seg, u8 bus, u8 devfn,
XEN_GUEST_HANDLE_64(uint32) buf, int max_sdevs)
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5b9611a134..4b6871936c 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -70,7 +70,6 @@ int iommu_hardware_setup(void);
int iommu_domain_init(struct domain *d);
void iommu_hwdom_init(struct domain *d);
void iommu_domain_destroy(struct domain *d);
-int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn);
void arch_iommu_domain_destroy(struct domain *d);
int arch_iommu_domain_init(struct domain *d);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static
2019-07-30 13:44 ` [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static Paul Durrant
@ 2019-08-06 15:54 ` Jan Beulich
2019-08-14 9:42 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-06 15:54 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel
On 30.07.2019 15:44, Paul Durrant wrote:
> This function is only ever called from within the same source module and
> really has no business being declared xen/iommu.h. This patch relocates
> the function ahead of the first called and makes it static.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
But of course it would have been nice if some minimal and obvious
style corrections were done at the same time:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -889,6 +889,52 @@ static int pci_clean_dpci_irqs(struct domain *d)
> return 0;
> }
>
> +/* caller should hold the pcidevs_lock */
> +static int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
uint<N>_t
> +{
> + const struct domain_iommu *hd = dom_iommu(d);
> + struct pci_dev *pdev = NULL;
stray initializer
> + int ret = 0;
> +
> + if ( !is_iommu_enabled(d) )
> + return -EINVAL;
> +
> + ASSERT(pcidevs_locked());
> + pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
> + if ( !pdev )
> + return -ENODEV;
> +
> + while ( pdev->phantom_stride )
> + {
> + devfn += pdev->phantom_stride;
> + if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
> + break;
> + ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
> + pci_to_dev(pdev));
> + if ( !ret )
> + continue;
> +
> + printk(XENLOG_G_ERR "d%d: deassign %04x:%02x:%02x.%u failed (%d)\n",
> + d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
(less "minimal") use %pd (also once more below)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static
2019-08-06 15:54 ` Jan Beulich
@ 2019-08-14 9:42 ` Paul Durrant
0 siblings, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 9:42 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: xen-devel
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 August 2019 16:54
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 4/6] make passthrough/pci.c:deassign_device() static
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > This function is only ever called from within the same source module and
> > really has no business being declared xen/iommu.h. This patch relocates
> > the function ahead of the first called and makes it static.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
Thanks
> But of course it would have been nice if some minimal and obvious
> style corrections were done at the same time:
Ok, I'll fold these into v2.
Paul
>
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -889,6 +889,52 @@ static int pci_clean_dpci_irqs(struct domain *d)
> > return 0;
> > }
> >
> > +/* caller should hold the pcidevs_lock */
> > +static int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
>
> uint<N>_t
>
> > +{
> > + const struct domain_iommu *hd = dom_iommu(d);
> > + struct pci_dev *pdev = NULL;
>
> stray initializer
>
> > + int ret = 0;
> > +
> > + if ( !is_iommu_enabled(d) )
> > + return -EINVAL;
> > +
> > + ASSERT(pcidevs_locked());
> > + pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
> > + if ( !pdev )
> > + return -ENODEV;
> > +
> > + while ( pdev->phantom_stride )
> > + {
> > + devfn += pdev->phantom_stride;
> > + if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
> > + break;
> > + ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
> > + pci_to_dev(pdev));
> > + if ( !ret )
> > + continue;
> > +
> > + printk(XENLOG_G_ERR "d%d: deassign %04x:%02x:%02x.%u failed (%d)\n",
> > + d->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), ret);
>
> (less "minimal") use %pd (also once more below)
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
` (3 preceding siblings ...)
2019-07-30 13:44 ` [Xen-devel] [PATCH 4/6] make passthrough/pci.c:deassign_device() static Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-07 10:41 ` Jan Beulich
2019-07-30 13:44 ` [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
5 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Jan Beulich, Volodymyr Babchuk,
Roger Pau Monné
Thes macros really ought to live in the common xen/iommu.h header rather
then being distributed amongst architecture specific iommu headers and
xen/sched.h. This patch moves them there.
NOTE: Disabling 'sharept' in the command line iommu options should really
be hard error on ARM (as opposed to just being ignored), so avoid
parsing that option if CONFIG_ARM is set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@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: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
xen/drivers/passthrough/iommu.c | 2 ++
xen/include/asm-arm/iommu.h | 3 ---
xen/include/asm-x86/iommu.h | 4 ----
xen/include/xen/iommu.h | 7 +++++++
xen/include/xen/sched.h | 6 ------
5 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 30976b4406..67855eeed5 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -102,8 +102,10 @@ static int __init parse_iommu_param(const char *s)
iommu_hwdom_passthrough = val;
else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
iommu_hwdom_strict = val;
+#ifndef CONFIG_ARM
else if ( (val = parse_boolean("sharept", s, ss)) >= 0 )
iommu_hap_pt_share = val;
+#endif
else
rc = -EINVAL;
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index e5050636d7..77a94b29eb 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -20,9 +20,6 @@ struct arch_iommu
void *priv;
};
-/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) (is_iommu_enabled(d))
-
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 6d024d5c0e..25d2aee9a9 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -79,10 +79,6 @@ struct iommu_init_ops {
extern const struct iommu_init_ops *iommu_init_ops;
-/* Are we using the domain P2M table as its IOMMU pagetable? */
-#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
-
void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
int iommu_setup_hpet_msi(struct msi_desc *);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4b6871936c..45ec6cfe44 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -268,6 +268,13 @@ struct domain_iommu {
#define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
#define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
+/* Are we using the domain P2M table as its IOMMU pagetable? */
+#define iommu_use_hap_pt(d) \
+ (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
+
+/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
+#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
+
int __must_check iommu_suspend(void);
void iommu_resume(void);
void iommu_crash_shutdown(void);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 538be7120b..6568f2b85b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
cpumask_weight(v->cpu_hard_affinity) == 1);
}
-#ifdef CONFIG_HAS_PASSTHROUGH
-#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
-#else
-#define need_iommu_pt_sync(d) false
-#endif
-
static inline bool is_vcpu_online(const struct vcpu *v)
{
return !test_bit(_VPF_down, &v->pause_flags);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-07-30 13:44 ` [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
@ 2019-08-07 10:41 ` Jan Beulich
2019-08-14 10:13 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-07 10:41 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 30.07.2019 15:44, Paul Durrant wrote:
> Thes macros really ought to live in the common xen/iommu.h header rather
> then being distributed amongst architecture specific iommu headers and
> xen/sched.h. This patch moves them there.
>
> NOTE: Disabling 'sharept' in the command line iommu options should really
> be hard error on ARM (as opposed to just being ignored), so avoid
> parsing that option if CONFIG_ARM is set.
Agreed. At that point the latest it would perhaps be good to have
Arm have
#define iommu_hap_pt_share true
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -268,6 +268,13 @@ struct domain_iommu {
> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
>
> +/* Are we using the domain P2M table as its IOMMU pagetable? */
> +#define iommu_use_hap_pt(d) \
> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
Does this build for Arm, seeing that there's no hap_enabled()
definition there? Or have I missed its addition earlier in this
series?
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> cpumask_weight(v->cpu_hard_affinity) == 1);
> }
>
> -#ifdef CONFIG_HAS_PASSTHROUGH
> -#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> -#else
> -#define need_iommu_pt_sync(d) false
> -#endif
The "#else" part of this gets lost - is this intentional, i.e.
are there no references left that could be a problem without
HAS_PASSTHROUGH?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-07 10:41 ` Jan Beulich
@ 2019-08-14 10:13 ` Paul Durrant
2019-08-14 10:20 ` Julien Grall
2019-08-27 7:53 ` Jan Beulich
0 siblings, 2 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 10:13 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Ian Jackson, xen-devel,
Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 11:41
> 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>; 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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > Thes macros really ought to live in the common xen/iommu.h header rather
> > then being distributed amongst architecture specific iommu headers and
> > xen/sched.h. This patch moves them there.
> >
> > NOTE: Disabling 'sharept' in the command line iommu options should really
> > be hard error on ARM (as opposed to just being ignored), so avoid
> > parsing that option if CONFIG_ARM is set.
>
> Agreed. At that point the latest it would perhaps be good to have
> Arm have
> #define iommu_hap_pt_share true
I don't quite follow. iommu_hap_pt_share is a global bool_t defined in passthrough/iommu.c... I'm just preventing an ARM command line from being able to change the value... so in effect it will always be true for ARM.
>
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -268,6 +268,13 @@ struct domain_iommu {
> > #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
> > #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
> >
> > +/* Are we using the domain P2M table as its IOMMU pagetable? */
> > +#define iommu_use_hap_pt(d) \
> > + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
>
> Does this build for Arm, seeing that there's no hap_enabled()
> definition there? Or have I missed its addition earlier in this
> series?
It moved to common code sched.h in an earlier patch.
>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> > cpumask_weight(v->cpu_hard_affinity) == 1);
> > }
> >
> > -#ifdef CONFIG_HAS_PASSTHROUGH
> > -#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> > -#else
> > -#define need_iommu_pt_sync(d) false
> > -#endif
>
> The "#else" part of this gets lost - is this intentional, i.e.
> are there no references left that could be a problem without
> HAS_PASSTHROUGH?
Not that it can be turned off at the moment, but yes there does appear to be a problem with gnttab_need_iommu_mapping() if I force HAS_PASSTHROUGH off... I'll add an equivalent ifdef.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 10:13 ` Paul Durrant
@ 2019-08-14 10:20 ` Julien Grall
2019-08-14 10:27 ` Paul Durrant
2019-08-27 7:53 ` Jan Beulich
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2019-08-14 10:20 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
Hi Paul,
On 14/08/2019 11:13, Paul Durrant wrote:
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -268,6 +268,13 @@ struct domain_iommu {
>>> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
>>> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
>>>
>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
>>> +#define iommu_use_hap_pt(d) \
>>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
>>
>> Does this build for Arm, seeing that there's no hap_enabled()
>> definition there? Or have I missed its addition earlier in this
>> series?
>
> It moved to common code sched.h in an earlier patch.
I went through the series and didn't find where hap_enabled() is defined for Arm
in this series. Do you mind pointing the exact patch?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 10:20 ` Julien Grall
@ 2019-08-14 10:27 ` Paul Durrant
2019-08-14 10:44 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 10:27 UTC (permalink / raw)
To: 'Julien Grall', 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
> -----Original Message-----
> From: Julien Grall <julien.grall@arm.com>
> Sent: 14 August 2019 11:21
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>
> Hi Paul,
>
> On 14/08/2019 11:13, Paul Durrant wrote:
> >>> --- a/xen/include/xen/iommu.h
> >>> +++ b/xen/include/xen/iommu.h
> >>> @@ -268,6 +268,13 @@ struct domain_iommu {
> >>> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
> >>> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
> >>>
> >>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
> >>> +#define iommu_use_hap_pt(d) \
> >>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
> >>
> >> Does this build for Arm, seeing that there's no hap_enabled()
> >> definition there? Or have I missed its addition earlier in this
> >> series?
> >
> > It moved to common code sched.h in an earlier patch.
>
> I went through the series and didn't find where hap_enabled() is defined for Arm
> in this series. Do you mind pointing the exact patch?
>
Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags", which is a pre-requisite for this series (as called out in the cover letter). The change is made in patch #2 of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 10:27 ` Paul Durrant
@ 2019-08-14 10:44 ` Julien Grall
2019-08-14 11:11 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2019-08-14 10:44 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
Hi,
On 14/08/2019 11:27, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien.grall@arm.com>
>> Sent: 14 August 2019 11:21
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>>
>> Hi Paul,
>>
>> On 14/08/2019 11:13, Paul Durrant wrote:
>>>>> --- a/xen/include/xen/iommu.h
>>>>> +++ b/xen/include/xen/iommu.h
>>>>> @@ -268,6 +268,13 @@ struct domain_iommu {
>>>>> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
>>>>> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
>>>>>
>>>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
>>>>> +#define iommu_use_hap_pt(d) \
>>>>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
>>>>
>>>> Does this build for Arm, seeing that there's no hap_enabled()
>>>> definition there? Or have I missed its addition earlier in this
>>>> series?
>>>
>>> It moved to common code sched.h in an earlier patch.
>>
>> I went through the series and didn't find where hap_enabled() is defined for Arm
>> in this series. Do you mind pointing the exact patch?
>>
>
> Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags", which is a pre-requisite for this series (as called out in the cover letter). The change is made in patch #2 of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
Oh. I understand this adds benefits as the implementation is now common. But the
downside is hap_enabled() will now require evaluation on Arm even it is
evaluates to true... This will prevent the compiler to remove any non-HAP code
paths (assuming there are any in the common code).
Furthermore, 2 parts of the iommu_use_hap_pt() condition will always returning
always true. But as they are non-constant, so they will always be evaluated.
It is also probably going to confuse developer as they may think non-HAP is
supported on Arm. You can't find easily that both hap_enabled(...) and
iommu_hap_pt_share will always evaluate to true.
So aside the common implementation, what is the real gain for Arm?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 10:44 ` Julien Grall
@ 2019-08-14 11:11 ` Paul Durrant
2019-08-14 12:28 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 11:11 UTC (permalink / raw)
To: 'Julien Grall', 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
> -----Original Message-----
> From: Julien Grall <julien.grall@arm.com>
> Sent: 14 August 2019 11:45
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>
> Hi,
>
> On 14/08/2019 11:27, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien.grall@arm.com>
> >> Sent: 14 August 2019 11:21
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
> >>
> >> Hi Paul,
> >>
> >> On 14/08/2019 11:13, Paul Durrant wrote:
> >>>>> --- a/xen/include/xen/iommu.h
> >>>>> +++ b/xen/include/xen/iommu.h
> >>>>> @@ -268,6 +268,13 @@ struct domain_iommu {
> >>>>> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
> >>>>> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
> >>>>>
> >>>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
> >>>>> +#define iommu_use_hap_pt(d) \
> >>>>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
> >>>>
> >>>> Does this build for Arm, seeing that there's no hap_enabled()
> >>>> definition there? Or have I missed its addition earlier in this
> >>>> series?
> >>>
> >>> It moved to common code sched.h in an earlier patch.
> >>
> >> I went through the series and didn't find where hap_enabled() is defined for Arm
> >> in this series. Do you mind pointing the exact patch?
> >>
> >
> > Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags", which
> is a pre-requisite for this series (as called out in the cover letter). The change is made in patch #2
> of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
>
> Oh. I understand this adds benefits as the implementation is now common. But the
> downside is hap_enabled() will now require evaluation on Arm even it is
> evaluates to true... This will prevent the compiler to remove any non-HAP code
> paths (assuming there are any in the common code).
There was one in the common iommu code that thus required a #ifdef for ARM.
>
> Furthermore, 2 parts of the iommu_use_hap_pt() condition will always returning
> always true. But as they are non-constant, so they will always be evaluated.
>
> It is also probably going to confuse developer as they may think non-HAP is
> supported on Arm. You can't find easily that both hap_enabled(...) and
> iommu_hap_pt_share will always evaluate to true.
>
> So aside the common implementation, what is the real gain for Arm?
There's no real gain for ARM, the gain is in the reduction in ifdef-ery and thus tidiness of code. I could put back some ifdefs if you'd prefer, or I could just put a comment stating that iommu_use_hap_pt() will always be true for ARM. Which would you prefer?
Paul
>
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 11:11 ` Paul Durrant
@ 2019-08-14 12:28 ` Julien Grall
2019-08-14 12:35 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2019-08-14 12:28 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
Hi,
On 14/08/2019 12:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien.grall@arm.com>
>> Sent: 14 August 2019 11:45
>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>>
>> Hi,
>>
>> On 14/08/2019 11:27, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall <julien.grall@arm.com>
>>>> Sent: 14 August 2019 11:21
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
>>>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@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 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
>>>>
>>>> Hi Paul,
>>>>
>>>> On 14/08/2019 11:13, Paul Durrant wrote:
>>>>>>> --- a/xen/include/xen/iommu.h
>>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>>> @@ -268,6 +268,13 @@ struct domain_iommu {
>>>>>>> #define iommu_set_feature(d, f) set_bit(f, dom_iommu(d)->features)
>>>>>>> #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
>>>>>>>
>>>>>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
>>>>>>> +#define iommu_use_hap_pt(d) \
>>>>>>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
>>>>>>
>>>>>> Does this build for Arm, seeing that there's no hap_enabled()
>>>>>> definition there? Or have I missed its addition earlier in this
>>>>>> series?
>>>>>
>>>>> It moved to common code sched.h in an earlier patch.
>>>>
>>>> I went through the series and didn't find where hap_enabled() is defined for Arm
>>>> in this series. Do you mind pointing the exact patch?
>>>>
>>>
>>> Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags", which
>> is a pre-requisite for this series (as called out in the cover letter). The change is made in patch #2
>> of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
>>
>> Oh. I understand this adds benefits as the implementation is now common. But the
>> downside is hap_enabled() will now require evaluation on Arm even it is
>> evaluates to true... This will prevent the compiler to remove any non-HAP code
>> paths (assuming there are any in the common code).
>
> There was one in the common iommu code that thus required a #ifdef for ARM.
Any CONFIG_{ARM, X86} feels an abuse of common code. So I am always in favor of
dropping them :). My concern is that a few of the helpers will likely return a
single value for any architecture by x86. But that's a different problem...
>
>>
>> Furthermore, 2 parts of the iommu_use_hap_pt() condition will always returning
>> always true. But as they are non-constant, so they will always be evaluated.
>>
>> It is also probably going to confuse developer as they may think non-HAP is
>> supported on Arm. You can't find easily that both hap_enabled(...) and
>> iommu_hap_pt_share will always evaluate to true.
>>
>> So aside the common implementation, what is the real gain for Arm?
>
> There's no real gain for ARM, the gain is in the reduction in ifdef-ery and thus tidiness of code. I could put back some ifdefs if you'd prefer, or I could just put a comment stating that iommu_use_hap_pt() will always be true for ARM. Which would you prefer?
Looking at the patch #6, iommu_use_hap_pt() is reimplemented with:
#define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
You also have a comment mentioning Arm systems in the same patch.
So I would be happy with this patch assuming that patch #6 does not change.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 12:28 ` Julien Grall
@ 2019-08-14 12:35 ` Paul Durrant
0 siblings, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 12:35 UTC (permalink / raw)
To: 'Julien Grall', 'Jan Beulich'
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, xen-devel, Volodymyr Babchuk,
Roger Pau Monne
> -----Original Message-----
[snip]
> >>>>>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
> >>>>>>> +#define iommu_use_hap_pt(d) \
> >>>>>>> + (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
> >>>>>>
> >>>>>> Does this build for Arm, seeing that there's no hap_enabled()
> >>>>>> definition there? Or have I missed its addition earlier in this
> >>>>>> series?
> >>>>>
> >>>>> It moved to common code sched.h in an earlier patch.
> >>>>
> >>>> I went through the series and didn't find where hap_enabled() is defined for Arm
> >>>> in this series. Do you mind pointing the exact patch?
> >>>>
> >>>
> >>> Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags",
> which
> >> is a pre-requisite for this series (as called out in the cover letter). The change is made in patch
> #2
> >> of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
> >>
> >> Oh. I understand this adds benefits as the implementation is now common. But the
> >> downside is hap_enabled() will now require evaluation on Arm even it is
> >> evaluates to true... This will prevent the compiler to remove any non-HAP code
> >> paths (assuming there are any in the common code).
> >
> > There was one in the common iommu code that thus required a #ifdef for ARM.
>
> Any CONFIG_{ARM, X86} feels an abuse of common code. So I am always in favor of
> dropping them :). My concern is that a few of the helpers will likely return a
> single value for any architecture by x86. But that's a different problem...
>
> >
> >>
> >> Furthermore, 2 parts of the iommu_use_hap_pt() condition will always returning
> >> always true. But as they are non-constant, so they will always be evaluated.
> >>
> >> It is also probably going to confuse developer as they may think non-HAP is
> >> supported on Arm. You can't find easily that both hap_enabled(...) and
> >> iommu_hap_pt_share will always evaluate to true.
> >>
> >> So aside the common implementation, what is the real gain for Arm?
> >
> > There's no real gain for ARM, the gain is in the reduction in ifdef-ery and thus tidiness of code. I
> could put back some ifdefs if you'd prefer, or I could just put a comment stating that
> iommu_use_hap_pt() will always be true for ARM. Which would you prefer?
>
> Looking at the patch #6, iommu_use_hap_pt() is reimplemented with:
>
> #define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
>
> You also have a comment mentioning Arm systems in the same patch.
>
> So I would be happy with this patch assuming that patch #6 does not change.
>
Ok, thanks. I'll see about adding a patch for arch specific defs of things like is_hvm_domain() and is_pv_domain(), and hap_enabled().
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
2019-08-14 10:13 ` Paul Durrant
2019-08-14 10:20 ` Julien Grall
@ 2019-08-27 7:53 ` Jan Beulich
1 sibling, 0 replies; 38+ messages in thread
From: Jan Beulich @ 2019-08-27 7:53 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, JulienGrall, xen-devel, IanJackson,
Volodymyr Babchuk, Roger Pau Monne
On 14.08.2019 12:13, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 07 August 2019 11:41
>>
>> On 30.07.2019 15:44, Paul Durrant wrote:
>>> Thes macros really ought to live in the common xen/iommu.h header rather
>>> then being distributed amongst architecture specific iommu headers and
>>> xen/sched.h. This patch moves them there.
>>>
>>> NOTE: Disabling 'sharept' in the command line iommu options should really
>>> be hard error on ARM (as opposed to just being ignored), so avoid
>>> parsing that option if CONFIG_ARM is set.
>>
>> Agreed. At that point the latest it would perhaps be good to have
>> Arm have
>> #define iommu_hap_pt_share true
>
> I don't quite follow. iommu_hap_pt_share is a global bool_t defined in passthrough/iommu.c... I'm just preventing an ARM command line from being able to change the value... so in effect it will always be true for ARM.
The point of my comment was to allow the compiler to eliminate dead
(on Arm) code. Along the lines of Julien's subsequent replies, things
like hap_enabled() and the one here should imo be compile time
constants (rather than runtime ones) for this reason.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg...
2019-07-30 13:44 [Xen-devel] [PATCH 0/6] per-domain IOMMU control Paul Durrant
` (4 preceding siblings ...)
2019-07-30 13:44 ` [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
@ 2019-07-30 13:44 ` Paul Durrant
2019-08-07 12:12 ` Jan Beulich
5 siblings, 1 reply; 38+ messages in thread
From: Paul Durrant @ 2019-07-30 13:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Paul Durrant, Jan Beulich, Anthony PERARD,
Volodymyr Babchuk, Roger Pau Monné
...and hence the ability to disable IOMMU mappings, and control EPT
sharing.
This patch introduces a new 'libxl_passthrough' enumeration into
libxl_domain_create_info. The value will be set by xl either when it parses
a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
hardware specified for the domain.
If the value of the passthrough configuration option is 'disabled' then
the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
flags, thus allowing the toolstack to control whether the domain gets
IOMMU mappings or not (where previously they were globally set).
If the value of the passthrough configuration option is 'sync_pt' then
a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
set in iommu_hap_pt_share, thus allowing the toolstack to control whether
EPT sharing is used for the domain.
NOTE: The call to libxl_get_physinfo() in libxl__domain_make() is left
in place to allow attempts to passthrough hardware on a hypervisor
with disabled IOMMU support to be rejected early.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
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: 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: Anthony PERARD <anthony.perard@citrix.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
docs/man/xl.cfg.5.pod.in | 52 +++++++++++++++++++++++++++++++++
tools/libxl/libxl.h | 5 ++++
tools/libxl/libxl_create.c | 16 +++++++++-
tools/libxl/libxl_types.idl | 7 +++++
tools/xl/xl_parse.c | 38 ++++++++++++++++++++++++
xen/arch/arm/domain.c | 11 ++++++-
xen/arch/x86/domain.c | 3 +-
xen/common/domain.c | 7 +++++
xen/drivers/passthrough/iommu.c | 13 ++++++++-
xen/include/public/domctl.h | 6 +++-
xen/include/xen/iommu.h | 17 +++++++----
11 files changed, 165 insertions(+), 10 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..c669524bec 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -605,6 +605,58 @@ option should only be used with a trusted device tree.
Note that the partial device tree should avoid using the phandle 65000
which is reserved by the toolstack.
+=item B<passthrough="STRING">
+
+Specify whether IOMMU mappings are enabled for the domain and hence whether
+it will be enabled for passthrough hardware. Valid values for this option
+are:
+
+=over 4
+
+=item B<disabled>
+
+IOMMU mappings are disabled for the domain and so hardware may not be
+passed through.
+
+This option is the default if no passthrough hardware is specified
+in the domain's configuration.
+
+=item B<sync_pt>
+
+This option means that IOMMU mappings will be synchronized with the
+domain's P2M table as follows:
+
+For a PV domain, all writable pages assigned to the domain are identity
+mapped by MFN in the IOMMU page table. Thus a device driver running in the
+domain may program passthrough hardware for DMA using MFN values
+(i.e. host/machine frame numbers) looked up in its P2M.
+
+For an HVM domain, all non-foreign RAM pages present in its P2M will be
+mapped by GFN in the IOMMU page table. Thus a device driver running in the
+domain may program passthrough hardware using GFN values (i.e. guest
+physical frame numbers) without any further translation.
+
+This option is the default if the domain is PV and passthrough hardware
+is specified in the configuration.
+
+This option is not available on Arm.
+
+=item B<share_pt>
+
+This option is unavailable for a PV domain. For an HVM domain, this option
+means that the IOMMU will be programmed to directly reference the domain's
+P2M table as its page table. From the point of view of a device driver
+running in the domain this is functionally equivalent to B<sync_pt> but
+places less load on the hypervisor and so should generally be selected in
+preference. However, the availability of this option is hardware specific
+and thus, if it is specified for a domain running on hardware that does
+not allow it, B<sync_pt> will be used instead.
+
+This option is the default if the domain is HVM and passthrough hardware
+is specified in the configuration.
+
+=back
+
=back
=head2 Devices
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9bacfb97f0..5de7c07a41 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -394,6 +394,11 @@
*/
#define LIBXL_HAVE_EXTENDED_VKB 1
+/*
+ * libxl_domain_create_info has libxl_passthrough enumeration.
+ */
+#define LIBXL_HAVE_CREATEINFO_PASSTHROUGH 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index feb9f1ce0c..ad5f36484a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -565,12 +565,26 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
}
+ LOG(DETAIL, "passthrough: %s",
+ libxl_passthrough_to_string(info->passthrough));
+
rc = libxl_get_physinfo(ctx, &physinfo);
if (rc < 0)
goto out;
- if ( physinfo.cap_hvm_directio )
+ if (info->passthrough != LIBXL_PASSTHROUGH_DISABLED)
+ {
+ if (!physinfo.cap_hvm_directio) {
+ LOGED(ERROR, *domid, "passthrough not available");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
create.flags |= XEN_DOMCTL_CDF_iommu;
+ }
+
+ if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
+ create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
/* 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/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..7e37de8646 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -263,6 +263,12 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
(2, "LINUX")
])
+libxl_passthrough = Enumeration("passthrough", [
+ (0, "disabled"),
+ (1, "sync_pt"),
+ (2, "share_pt"),
+ ])
+
#
# Complex libxl types
#
@@ -408,6 +414,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
("pool_name", string),
("run_hotplug_scripts",libxl_defbool),
("driver_domain",libxl_defbool),
+ ("passthrough", libxl_passthrough),
], dir=DIR_IN)
libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2bb..c904604008 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2326,6 +2326,44 @@ skip_vfb:
}
}
+ if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
+ libxl_passthrough o;
+
+ e = libxl_passthrough_from_string(buf, &o);
+ if (e) {
+ fprintf(stderr,
+ "ERROR: unknown passthrough option '%s'\n",
+ buf);
+ exit(-ERROR_FAIL);
+ }
+
+ switch (o) {
+ case LIBXL_PASSTHROUGH_DISABLED:
+ if (d_config->num_pcidevs || d_config->num_dtdevs) {
+ fprintf(stderr,
+ "ERROR: passthrough disabled but devices are specified\n");
+ exit(-ERROR_FAIL);
+ }
+ case LIBXL_PASSTHROUGH_SHARE_PT:
+ if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
+ fprintf(stderr,
+ "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
+ exit(-ERROR_FAIL);
+ }
+ default:
+ break;
+ }
+
+ c_info->passthrough = o;
+ } else if (d_config->num_pcidevs || d_config->num_dtdevs) {
+ /*
+ * Passthrough devices are specified so set an appropriate
+ * default value.
+ */
+ c_info->passthrough = (c_info->type == LIBXL_DOMAIN_TYPE_PV) ?
+ LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+ }
+
if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) {
d_config->num_usbctrls = 0;
d_config->usbctrls = NULL;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e06bd27dad..233dc4d59d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -614,6 +614,14 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ /* Always share P2M Table between the CPU and the IOMMU */
+ if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
+ {
+ dprintk(XENLOG_INFO,
+ "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
+ return -EINVAL;
+ }
+
/* Fill in the native GIC version, passed back to the toolstack. */
if ( config->arch.gic_version == XEN_DOMCTL_CONFIG_GIC_NATIVE )
{
@@ -673,7 +681,8 @@ int arch_domain_create(struct domain *d,
ASSERT(config != NULL);
- if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 )
+ if ( is_iommu_enabled(d) &&
+ (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
if ( (rc = p2m_init(d)) != 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 42778099da..4bbd8de663 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -604,7 +604,8 @@ int arch_domain_create(struct domain *d,
if ( (rc = init_domain_irq_mapping(d)) != 0 )
goto fail;
- if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 )
+ if ( is_iommu_enabled(d) &&
+ (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
goto fail;
psr_domain_init(d);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0df4b47352..b0b9a17a18 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -308,6 +308,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( !(config->flags & XEN_DOMCTL_CDF_iommu) && config->iommu_opts )
+ {
+ dprintk(XENLOG_INFO,
+ "IOMMU options specified but IOMMU not enabled\n");
+ return -EINVAL;
+ }
+
if ( config->max_vcpus < 1 )
{
dprintk(XENLOG_INFO, "No vCPUS\n");
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 67855eeed5..1c556e39ab 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -159,7 +159,7 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d)
iommu_hwdom_strict = true;
}
-int iommu_domain_init(struct domain *d)
+int iommu_domain_init(struct domain *d, unsigned int opts)
{
struct domain_iommu *hd = dom_iommu(d);
int ret = 0;
@@ -176,6 +176,15 @@ int iommu_domain_init(struct domain *d)
if ( ret )
return ret;
+ /*
+ * Use shared page tables for HAP and IOMMU if the global option
+ * is enabled (from which we can infer the h/w is capable) and
+ * the domain options do not disallow it. HAP must, of course, also
+ * be enabled.
+ */
+ hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
+ !(opts & XEN_DOMCTL_IOMMU_no_sharept);
+
/*
* NB: 'relaxed' h/w domains don't need the IOMMU mappings to be kept
* in-sync with their assigned pages because all host RAM will be
@@ -187,6 +196,8 @@ int iommu_domain_init(struct domain *d)
if ( !is_hardware_domain(d) || iommu_hwdom_strict )
hd->need_sync = !iommu_use_hap_pt(d);
+ ASSERT(!(hd->need_sync && hd->hap_pt_share));
+
return 0;
}
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 3f82c78870..ebab653cac 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -68,7 +68,11 @@ struct xen_domctl_createdomain {
#define _XEN_DOMCTL_CDF_iommu 5
#define XEN_DOMCTL_CDF_iommu (1U<<_XEN_DOMCTL_CDF_iommu)
- uint32_t flags;
+ uint16_t flags;
+
+#define _XEN_DOMCTL_IOMMU_no_sharept 0
+#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
+ uint16_t iommu_opts;
/*
* Various domain limits, which impact the quantity of resources (global
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 45ec6cfe44..40e59a4fbf 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -67,7 +67,7 @@ extern unsigned int iommu_dev_iotlb_timeout;
int iommu_setup(void);
int iommu_hardware_setup(void);
-int iommu_domain_init(struct domain *d);
+int iommu_domain_init(struct domain *d, unsigned int opts);
void iommu_hwdom_init(struct domain *d);
void iommu_domain_destroy(struct domain *d);
@@ -256,10 +256,18 @@ struct domain_iommu {
/* Features supported by the IOMMU */
DECLARE_BITMAP(features, IOMMU_FEAT_count);
+ /*
+ * Does the guest share HAP mapping with the IOMMU? This is always
+ * true for ARM systems and may be true for x86 systems where the
+ * the hardware is capable.
+ */
+ bool hap_pt_share;
+
/*
* Does the guest reqire mappings to be synchonized, to maintain
- * the default dfn == pfn map. (See comment on dfn at the top of
- * include/xen/mm.h).
+ * the default dfn == pfn map? (See comment on dfn at the top of
+ * include/xen/mm.h). Note that hap_pt_share == false does not
+ * necessarily imply this is true.
*/
bool need_sync;
};
@@ -269,8 +277,7 @@ struct domain_iommu {
#define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
/* Are we using the domain P2M table as its IOMMU pagetable? */
-#define iommu_use_hap_pt(d) \
- (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
+#define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
/* Does the IOMMU pagetable need to be kept synchronized with the P2M */
#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg...
2019-07-30 13:44 ` [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
@ 2019-08-07 12:12 ` Jan Beulich
2019-08-14 10:40 ` Paul Durrant
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2019-08-07 12:12 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Anthony PERARD, xen-devel, Volodymyr Babchuk,
Roger Pau Monné
On 30.07.2019 15:44, Paul Durrant wrote:
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -605,6 +605,58 @@ option should only be used with a trusted device tree.
> Note that the partial device tree should avoid using the phandle 65000
> which is reserved by the toolstack.
>
> +=item B<passthrough="STRING">
> +
> +Specify whether IOMMU mappings are enabled for the domain and hence whether
> +it will be enabled for passthrough hardware. Valid values for this option
> +are:
> +
> +=over 4
> +
> +=item B<disabled>
> +
> +IOMMU mappings are disabled for the domain and so hardware may not be
> +passed through.
> +
> +This option is the default if no passthrough hardware is specified
> +in the domain's configuration.
> +
> +=item B<sync_pt>
> +
> +This option means that IOMMU mappings will be synchronized with the
> +domain's P2M table as follows:
> +
> +For a PV domain, all writable pages assigned to the domain are identity
> +mapped by MFN in the IOMMU page table. Thus a device driver running in the
> +domain may program passthrough hardware for DMA using MFN values
> +(i.e. host/machine frame numbers) looked up in its P2M.
> +
> +For an HVM domain, all non-foreign RAM pages present in its P2M will be
> +mapped by GFN in the IOMMU page table. Thus a device driver running in the
> +domain may program passthrough hardware using GFN values (i.e. guest
> +physical frame numbers) without any further translation.
> +
> +This option is the default if the domain is PV and passthrough hardware
> +is specified in the configuration.
> +
> +This option is not available on Arm.
> +
> +=item B<share_pt>
> +
> +This option is unavailable for a PV domain. For an HVM domain, this option
> +means that the IOMMU will be programmed to directly reference the domain's
> +P2M table as its page table. From the point of view of a device driver
> +running in the domain this is functionally equivalent to B<sync_pt> but
> +places less load on the hypervisor and so should generally be selected in
> +preference. However, the availability of this option is hardware specific
> +and thus, if it is specified for a domain running on hardware that does
> +not allow it, B<sync_pt> will be used instead.
> +
> +This option is the default if the domain is HVM and passthrough hardware
> +is specified in the configuration.
Perhaps worthwhile to clarify right away that on AMD we'll always
fall back to sync_pt?
Btw, I've no idea what the convention in xl.cfg is, but in the
hypervisor I'd ask to use hyphens instead of underscores in the
option value strings.
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -614,6 +614,14 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + /* Always share P2M Table between the CPU and the IOMMU */
> + if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
> + {
> + dprintk(XENLOG_INFO,
> + "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
> + return -EINVAL;
Seeing our excessive use of EINVAL, I'm generally suggesting to
use almost anything else where EINVAL isn't clearly the right
one to use. EOPNOTSUPP would seem better here, for example, not
the least because I assume it is at least a hypothetically
valid setting (if it was implemented).
> @@ -176,6 +176,15 @@ int iommu_domain_init(struct domain *d)
> if ( ret )
> return ret;
>
> + /*
> + * Use shared page tables for HAP and IOMMU if the global option
> + * is enabled (from which we can infer the h/w is capable) and
> + * the domain options do not disallow it. HAP must, of course, also
> + * be enabled.
> + */
> + hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
> + !(opts & XEN_DOMCTL_IOMMU_no_sharept);
The hap_enabled() again raises the question of whether this
builds for Arm.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -68,7 +68,11 @@ struct xen_domctl_createdomain {
> #define _XEN_DOMCTL_CDF_iommu 5
> #define XEN_DOMCTL_CDF_iommu (1U<<_XEN_DOMCTL_CDF_iommu)
>
> - uint32_t flags;
> + uint16_t flags;
> +
> +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> + uint16_t iommu_opts;
Are we sure of this split into to 16-bit fields? With a
growing number of settings to be established at domain
creation I don't think the 11 remaining bits for the
"general" flags won't last very long. IOW if such a split,
then I think we'd better have two 32-bit fields.
> @@ -256,10 +256,18 @@ struct domain_iommu {
> /* Features supported by the IOMMU */
> DECLARE_BITMAP(features, IOMMU_FEAT_count);
>
> + /*
> + * Does the guest share HAP mapping with the IOMMU? This is always
> + * true for ARM systems and may be true for x86 systems where the
> + * the hardware is capable.
> + */
> + bool hap_pt_share;
> +
> /*
> * Does the guest reqire mappings to be synchonized, to maintain
> - * the default dfn == pfn map. (See comment on dfn at the top of
> - * include/xen/mm.h).
> + * the default dfn == pfn map? (See comment on dfn at the top of
> + * include/xen/mm.h). Note that hap_pt_share == false does not
> + * necessarily imply this is true.
Would you mind also correcting the two spelling mistakes in the
first line of the comment that you don't touch right now?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Xen-devel] [PATCH 6/6] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-07 12:12 ` Jan Beulich
@ 2019-08-14 10:40 ` Paul Durrant
0 siblings, 0 replies; 38+ messages in thread
From: Paul Durrant @ 2019-08-14 10:40 UTC (permalink / raw)
To: 'Jan Beulich', Julien Grall
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Ian Jackson, Anthony Perard, xen-devel,
Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 13:13
> 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 6/6] introduce a 'passthrough' configuration option to xl.cfg...
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -605,6 +605,58 @@ option should only be used with a trusted device tree.
> > Note that the partial device tree should avoid using the phandle 65000
> > which is reserved by the toolstack.
> >
> > +=item B<passthrough="STRING">
> > +
> > +Specify whether IOMMU mappings are enabled for the domain and hence whether
> > +it will be enabled for passthrough hardware. Valid values for this option
> > +are:
> > +
> > +=over 4
> > +
> > +=item B<disabled>
> > +
> > +IOMMU mappings are disabled for the domain and so hardware may not be
> > +passed through.
> > +
> > +This option is the default if no passthrough hardware is specified
> > +in the domain's configuration.
> > +
> > +=item B<sync_pt>
> > +
> > +This option means that IOMMU mappings will be synchronized with the
> > +domain's P2M table as follows:
> > +
> > +For a PV domain, all writable pages assigned to the domain are identity
> > +mapped by MFN in the IOMMU page table. Thus a device driver running in the
> > +domain may program passthrough hardware for DMA using MFN values
> > +(i.e. host/machine frame numbers) looked up in its P2M.
> > +
> > +For an HVM domain, all non-foreign RAM pages present in its P2M will be
> > +mapped by GFN in the IOMMU page table. Thus a device driver running in the
> > +domain may program passthrough hardware using GFN values (i.e. guest
> > +physical frame numbers) without any further translation.
> > +
> > +This option is the default if the domain is PV and passthrough hardware
> > +is specified in the configuration.
> > +
> > +This option is not available on Arm.
> > +
> > +=item B<share_pt>
> > +
> > +This option is unavailable for a PV domain. For an HVM domain, this option
> > +means that the IOMMU will be programmed to directly reference the domain's
> > +P2M table as its page table. From the point of view of a device driver
> > +running in the domain this is functionally equivalent to B<sync_pt> but
> > +places less load on the hypervisor and so should generally be selected in
> > +preference. However, the availability of this option is hardware specific
> > +and thus, if it is specified for a domain running on hardware that does
> > +not allow it, B<sync_pt> will be used instead.
> > +
> > +This option is the default if the domain is HVM and passthrough hardware
> > +is specified in the configuration.
>
> Perhaps worthwhile to clarify right away that on AMD we'll always
> fall back to sync_pt?
Ok.
>
> Btw, I've no idea what the convention in xl.cfg is, but in the
> hypervisor I'd ask to use hyphens instead of underscores in the
> option value strings.
Enums in the idl use underscores and so using them in the xl.cfg parameters means I can take advantage of the autogenerated string-to-enum helper.
>
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -614,6 +614,14 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> > return -EINVAL;
> > }
> >
> > + /* Always share P2M Table between the CPU and the IOMMU */
> > + if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
> > + {
> > + dprintk(XENLOG_INFO,
> > + "Unsupported iommu option: XEN_DOMCTL_IOMMU_no_sharept\n");
> > + return -EINVAL;
>
> Seeing our excessive use of EINVAL, I'm generally suggesting to
> use almost anything else where EINVAL isn't clearly the right
> one to use. EOPNOTSUPP would seem better here, for example, not
> the least because I assume it is at least a hypothetically
> valid setting (if it was implemented).
Julien... Is ARM ever likely to not use shared P2M/IOMMU mappings?
>
> > @@ -176,6 +176,15 @@ int iommu_domain_init(struct domain *d)
> > if ( ret )
> > return ret;
> >
> > + /*
> > + * Use shared page tables for HAP and IOMMU if the global option
> > + * is enabled (from which we can infer the h/w is capable) and
> > + * the domain options do not disallow it. HAP must, of course, also
> > + * be enabled.
> > + */
> > + hd->hap_pt_share = hap_enabled(d) && iommu_hap_pt_share &&
> > + !(opts & XEN_DOMCTL_IOMMU_no_sharept);
>
> The hap_enabled() again raises the question of whether this
> builds for Arm.
It should, as long as the pre-requisite series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02253.html is applied. I'll double-check that though.
>
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -68,7 +68,11 @@ struct xen_domctl_createdomain {
> > #define _XEN_DOMCTL_CDF_iommu 5
> > #define XEN_DOMCTL_CDF_iommu (1U<<_XEN_DOMCTL_CDF_iommu)
> >
> > - uint32_t flags;
> > + uint16_t flags;
> > +
> > +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> > +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> > + uint16_t iommu_opts;
>
> Are we sure of this split into to 16-bit fields? With a
> growing number of settings to be established at domain
> creation I don't think the 11 remaining bits for the
> "general" flags won't last very long. IOW if such a split,
> then I think we'd better have two 32-bit fields.
Ok. With only 6 bits in use for the flags I thought there was enough headroom... I'll grow the structure instead.
>
> > @@ -256,10 +256,18 @@ struct domain_iommu {
> > /* Features supported by the IOMMU */
> > DECLARE_BITMAP(features, IOMMU_FEAT_count);
> >
> > + /*
> > + * Does the guest share HAP mapping with the IOMMU? This is always
> > + * true for ARM systems and may be true for x86 systems where the
> > + * the hardware is capable.
> > + */
> > + bool hap_pt_share;
> > +
> > /*
> > * Does the guest reqire mappings to be synchonized, to maintain
> > - * the default dfn == pfn map. (See comment on dfn at the top of
> > - * include/xen/mm.h).
> > + * the default dfn == pfn map? (See comment on dfn at the top of
> > + * include/xen/mm.h). Note that hap_pt_share == false does not
> > + * necessarily imply this is true.
>
> Would you mind also correcting the two spelling mistakes in the
> first line of the comment that you don't touch right now?
Happy to do that,
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread