* [Xen-devel] [PATCH v6 01/10] make passthrough/pci.c:deassign_device() static
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-23 9:51 ` Roger Pau Monné
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag Paul Durrant
` (8 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v6:
- Re-base due to re-positioning in series
v5:
- minor style fixes
- use %pd, rather than d%d
---
xen/drivers/passthrough/pci.c | 99 ++++++++++++++++++-----------------
xen/include/xen/iommu.h | 1 -
2 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 7c196ba58b..89536cc067 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -889,6 +889,56 @@ static int pci_clean_dpci_irqs(struct domain *d)
return 0;
}
+/* caller should hold the pcidevs_lock */
+static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
+ uint8_t devfn)
+{
+ const struct domain_iommu *hd = dom_iommu(d);
+ struct pci_dev *pdev;
+ int ret = 0;
+
+ if ( !iommu_enabled || !hd->platform_ops )
+ 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 "%pd: deassign %04x:%02x:%02x.%u failed (%d)\n",
+ d, 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,
+ "%pd: deassign device (%04x:%02x:%02x.%u) failed\n",
+ d, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ return ret;
+ }
+
+ pdev->fault.count = 0;
+
+ if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
+ iommu_teardown(d);
+
+ return ret;
+}
+
int pci_release_devices(struct domain *d)
{
struct pci_dev *pdev;
@@ -1476,55 +1526,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 ( !iommu_enabled || !hd->platform_ops )
- 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;
-
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
-
- 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 48f87480a7..314f28f323 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] 35+ messages in thread* [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 01/10] make passthrough/pci.c:deassign_device() static Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-23 10:05 ` Roger Pau Monné
2019-08-23 12:23 ` Andrew Cooper
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 03/10] x86/domain: remove the 'oos_off' flag Paul Durrant
` (7 subsequent siblings)
9 siblings, 2 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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, Roger Pau Monné
The hap_enabled() macro can determine whether the feature is available
using the domain 'options'; there is no need for a separate flag.
NOTE: Furthermore, by extending sanitiziing of the domain 'options', the
macro can be transformed into an inline function and re-located to
xen/sched.h. This also makes hap_enabled() common, thus allowing
removal of an ugly ifdef CONFIG_X86 from the common iommu code.
Signed-off-by: Paul Durrant <paul.durrant@citrix.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: 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: George Dunlap <george.dunlap@eu.citrix.com>
v4:
- Add missing perentheses and move hap_enabled()
- Fix the shim build
v3:
- Re-worked as suggested by Jan
- Not adding Roger's R-b as the patch has changed substantially
v2:
- Defer changes to shadow_domain_init() to patch #4
---
xen/arch/x86/domain.c | 13 +++++++------
xen/arch/x86/mm/paging.c | 4 ++--
xen/common/domain.c | 7 +++++++
xen/drivers/passthrough/iommu.c | 2 --
xen/include/asm-x86/hvm/domain.h | 7 -------
xen/include/asm-x86/paging.h | 2 +-
xen/include/xen/sched.h | 6 ++++++
7 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9a6eb89ddc..bc0db03387 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -460,6 +460,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
+ {
+ dprintk(XENLOG_INFO, "HAP enabled but not supported\n");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -564,12 +570,7 @@ int arch_domain_create(struct domain *d,
HYPERVISOR_COMPAT_VIRT_START(d) =
is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
- /* Need to determine if HAP is enabled before initialising paging */
- if ( is_hvm_domain(d) )
- d->arch.hvm.hap_enabled =
- hvm_hap_supported() && (config->flags & XEN_DOMCTL_CDF_hap);
-
- if ( (rc = paging_domain_init(d, config->flags)) != 0 )
+ if ( (rc = paging_domain_init(d)) != 0 )
goto fail;
paging_initialised = true;
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 011089368a..097a27f608 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -632,7 +632,7 @@ void paging_log_dirty_init(struct domain *d, const struct log_dirty_ops *ops)
/* CODE FOR PAGING SUPPORT */
/************************************************/
/* Domain paging struct initialization. */
-int paging_domain_init(struct domain *d, unsigned int domcr_flags)
+int paging_domain_init(struct domain *d)
{
int rc;
@@ -653,7 +653,7 @@ int paging_domain_init(struct domain *d, unsigned int domcr_flags)
if ( hap_enabled(d) )
hap_domain_init(d);
else
- rc = shadow_domain_init(d, domcr_flags);
+ rc = shadow_domain_init(d, d->options);
return rc;
}
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 744b572195..6109623730 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -313,6 +313,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
+ (config->flags & XEN_DOMCTL_CDF_hap) )
+ {
+ dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
+ return -EINVAL;
+ }
+
return arch_sanitise_domain_config(config);
}
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index f8c3bf53bd..37eb0f7d01 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -573,9 +573,7 @@ int iommu_do_domctl(
void iommu_share_p2m_table(struct domain* d)
{
-#ifdef CONFIG_X86
ASSERT(hap_enabled(d));
-#endif
/*
* iommu_use_hap_pt(d) cannot be used here because during domain
* construction need_iommu(d) will always return false here.
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 6c7c4f5aa6..bcc5621797 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -156,7 +156,6 @@ struct hvm_domain {
struct viridian_domain *viridian;
- bool_t hap_enabled;
bool_t mem_sharing_enabled;
bool_t qemu_mapcache_invalidate;
bool_t is_s3_suspended;
@@ -195,12 +194,6 @@ struct hvm_domain {
};
};
-#ifdef CONFIG_HVM
-#define hap_enabled(d) (is_hvm_domain(d) && (d)->arch.hvm.hap_enabled)
-#else
-#define hap_enabled(d) ({(void)(d); false;})
-#endif
-
#endif /* __ASM_X86_HVM_DOMAIN_H__ */
/*
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index cf57ca708d..ab7887f23c 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -207,7 +207,7 @@ void paging_vcpu_init(struct vcpu *v);
/* Set up the paging-assistance-specific parts of a domain struct at
* start of day. Called for every domain from arch_domain_create() */
-int paging_domain_init(struct domain *d, unsigned int domcr_flags);
+int paging_domain_init(struct domain *d);
/* Handler for paging-control ops: operations from user-space to enable
* and disable ephemeral shadow modes (test mode and log-dirty mode) and
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2e6e0d3488..07a64947ed 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -954,6 +954,12 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
return is_hvm_domain(v->domain);
}
+static inline bool hap_enabled(const struct domain *d)
+{
+ return IS_ENABLED(CONFIG_HVM) && /* necessary for pv shim build */
+ evaluate_nospec(d->options & XEN_DOMCTL_CDF_hap);
+}
+
static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
{
return (is_hardware_domain(v->domain) &&
--
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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag Paul Durrant
@ 2019-08-23 10:05 ` Roger Pau Monné
2019-08-23 12:23 ` Andrew Cooper
1 sibling, 0 replies; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 10:05 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, xen-devel
On Fri, Aug 16, 2019 at 06:19:53PM +0100, Paul Durrant wrote:
> The hap_enabled() macro can determine whether the feature is available
> using the domain 'options'; there is no need for a separate flag.
>
> NOTE: Furthermore, by extending sanitiziing of the domain 'options', the
> macro can be transformed into an inline function and re-located to
> xen/sched.h. This also makes hap_enabled() common, thus allowing
> removal of an ugly ifdef CONFIG_X86 from the common iommu code.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag Paul Durrant
2019-08-23 10:05 ` Roger Pau Monné
@ 2019-08-23 12:23 ` Andrew Cooper
2019-08-23 12:25 ` Andrew Cooper
1 sibling, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2019-08-23 12:23 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Tim Deegan, Ian Jackson, Julien Grall,
Jan Beulich, Roger Pau Monné
On 16/08/2019 18:19, Paul Durrant wrote:
> The hap_enabled() macro can determine whether the feature is available
> using the domain 'options'; there is no need for a separate flag.
>
> NOTE: Furthermore, by extending sanitiziing of the domain 'options', the
s/ii/i/
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 9a6eb89ddc..bc0db03387 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -460,6 +460,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
> + {
> + dprintk(XENLOG_INFO, "HAP enabled but not supported\n");
s/enabled/requested/
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 744b572195..6109623730 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -313,6 +313,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
> + (config->flags & XEN_DOMCTL_CDF_hap) )
> + {
> + dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
Again, I think 'requested' would be better here.
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2e6e0d3488..07a64947ed 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -954,6 +954,12 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
> return is_hvm_domain(v->domain);
> }
>
> +static inline bool hap_enabled(const struct domain *d)
> +{
> + return IS_ENABLED(CONFIG_HVM) && /* necessary for pv shim build */
> + evaluate_nospec(d->options & XEN_DOMCTL_CDF_hap);
I'm not sure how helpful this comment is. What should be here however
is a note saying that this logic depends on domain_create() rejecting
!HVM and HAP.
All can be adjusted on commit if there are no other concerns.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
2019-08-23 12:23 ` Andrew Cooper
@ 2019-08-23 12:25 ` Andrew Cooper
2019-08-27 8:19 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2019-08-23 12:25 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Tim Deegan, Ian Jackson, Julien Grall,
Jan Beulich, Roger Pau Monné
On 23/08/2019 13:23, Andrew Cooper wrote:
> On 16/08/2019 18:19, Paul Durrant wrote:
>> The hap_enabled() macro can determine whether the feature is available
>> using the domain 'options'; there is no need for a separate flag.
>>
>> NOTE: Furthermore, by extending sanitiziing of the domain 'options', the
> s/ii/i/
>
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 9a6eb89ddc..bc0db03387 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -460,6 +460,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> return -EINVAL;
>> }
>>
>> + if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
>> + {
>> + dprintk(XENLOG_INFO, "HAP enabled but not supported\n");
> s/enabled/requested/
>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 744b572195..6109623730 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -313,6 +313,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>> return -EINVAL;
>> }
>>
>> + if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
>> + (config->flags & XEN_DOMCTL_CDF_hap) )
>> + {
>> + dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
> Again, I think 'requested' would be better here.
>
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 2e6e0d3488..07a64947ed 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -954,6 +954,12 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
>> return is_hvm_domain(v->domain);
>> }
>>
>> +static inline bool hap_enabled(const struct domain *d)
>> +{
>> + return IS_ENABLED(CONFIG_HVM) && /* necessary for pv shim build */
>> + evaluate_nospec(d->options & XEN_DOMCTL_CDF_hap);
> I'm not sure how helpful this comment is. What should be here however
> is a note saying that this logic depends on domain_create() rejecting
> !HVM and HAP.
>
> All can be adjusted on commit if there are no other concerns.
One other thing. Why is this eval_nospec()?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
2019-08-23 12:25 ` Andrew Cooper
@ 2019-08-27 8:19 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-27 8:19 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Roger Pau Monne
> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Sent: 23 August 2019 13:26
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Tim (Xen.org) <tim@xen.org>; Ian
> Jackson <Ian.Jackson@citrix.com>; Julien Grall <julien.grall@arm.com>; Jan Beulich
> <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag
>
> On 23/08/2019 13:23, Andrew Cooper wrote:
> > On 16/08/2019 18:19, Paul Durrant wrote:
> >> The hap_enabled() macro can determine whether the feature is available
> >> using the domain 'options'; there is no need for a separate flag.
> >>
> >> NOTE: Furthermore, by extending sanitiziing of the domain 'options', the
> > s/ii/i/
Oh yes.
> >
> >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> >> index 9a6eb89ddc..bc0db03387 100644
> >> --- a/xen/arch/x86/domain.c
> >> +++ b/xen/arch/x86/domain.c
> >> @@ -460,6 +460,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> >> return -EINVAL;
> >> }
> >>
> >> + if ( (config->flags & XEN_DOMCTL_CDF_hap) && !hvm_hap_supported() )
> >> + {
> >> + dprintk(XENLOG_INFO, "HAP enabled but not supported\n");
> > s/enabled/requested/
> >
I'm not fussed... I just went with the incumbent flag name.
> >> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >> index 744b572195..6109623730 100644
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -313,6 +313,13 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> >> return -EINVAL;
> >> }
> >>
> >> + if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
> >> + (config->flags & XEN_DOMCTL_CDF_hap) )
> >> + {
> >> + dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
> > Again, I think 'requested' would be better here.
> >
> >> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> >> index 2e6e0d3488..07a64947ed 100644
> >> --- a/xen/include/xen/sched.h
> >> +++ b/xen/include/xen/sched.h
> >> @@ -954,6 +954,12 @@ static inline bool is_hvm_vcpu(const struct vcpu *v)
> >> return is_hvm_domain(v->domain);
> >> }
> >>
> >> +static inline bool hap_enabled(const struct domain *d)
> >> +{
> >> + return IS_ENABLED(CONFIG_HVM) && /* necessary for pv shim build */
> >> + evaluate_nospec(d->options & XEN_DOMCTL_CDF_hap);
> > I'm not sure how helpful this comment is. What should be here however
> > is a note saying that this logic depends on domain_create() rejecting
> > !HVM and HAP.
> >
> > All can be adjusted on commit if there are no other concerns.
>
Ok.
> One other thing. Why is this eval_nospec()?
>
General paranoia about what might happen in speculation if the inline evaluates false and we wander into e.g. shadow code.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 03/10] x86/domain: remove the 'oos_off' flag
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 01/10] make passthrough/pci.c:deassign_device() static Paul Durrant
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 02/10] x86/hvm/domain: remove the 'hap_enabled' flag Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 04/10] domain: remove the 'is_xenstore' flag Paul Durrant
` (6 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, George Dunlap, Andrew Cooper, Tim Deegan, Paul Durrant,
Jan Beulich, Roger Pau Monné
The flag is not needed since the domain 'options' can now be tested
directly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
v3:
- Force 'oos_off' to be set for PV guests (to avoid call to
is_hvm_domain() except in ASSERT)
- Dropped Tim's A-b because of the change
v2:
- Move some of the hunks from patch #3
- Also update the definition of shadow_domain_init() in none.c
---
xen/arch/x86/mm/paging.c | 2 +-
xen/arch/x86/mm/shadow/common.c | 7 ++++---
xen/arch/x86/mm/shadow/none.c | 2 +-
xen/common/domain.c | 16 ++++++++++++----
xen/include/asm-x86/domain.h | 1 -
xen/include/asm-x86/shadow.h | 2 +-
6 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 097a27f608..69aa228e46 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -653,7 +653,7 @@ int paging_domain_init(struct domain *d)
if ( hap_enabled(d) )
hap_domain_init(d);
else
- rc = shadow_domain_init(d, d->options);
+ rc = shadow_domain_init(d);
return rc;
}
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index fa18de0bb6..1187176993 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -46,7 +46,7 @@ static void sh_clean_dirty_bitmap(struct domain *);
/* Set up the shadow-specific parts of a domain struct at start of day.
* Called for every domain from arch_domain_create() */
-int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
+int shadow_domain_init(struct domain *d)
{
static const struct log_dirty_ops sh_ops = {
.enable = sh_enable_log_dirty,
@@ -62,7 +62,6 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
d->arch.paging.shadow.oos_active = 0;
- d->arch.paging.shadow.oos_off = domcr_flags & XEN_DOMCTL_CDF_oos_off;
#endif
d->arch.paging.shadow.pagetable_dying_op = 0;
@@ -2523,11 +2522,13 @@ static void sh_update_paging_modes(struct vcpu *v)
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
/* We need to check that all the vcpus have paging enabled to
* unsync PTs. */
- if ( is_hvm_domain(d) && !d->arch.paging.shadow.oos_off )
+ if ( !(d->options & XEN_DOMCTL_CDF_oos_off) )
{
int pe = 1;
struct vcpu *vptr;
+ ASSERT(is_hvm_domain(d));
+
for_each_vcpu(d, vptr)
{
if ( !hvm_paging_enabled(vptr) )
diff --git a/xen/arch/x86/mm/shadow/none.c b/xen/arch/x86/mm/shadow/none.c
index a70888bd98..2fddf4274c 100644
--- a/xen/arch/x86/mm/shadow/none.c
+++ b/xen/arch/x86/mm/shadow/none.c
@@ -18,7 +18,7 @@ static void _clean_dirty_bitmap(struct domain *d)
ASSERT(is_pv_domain(d));
}
-int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
+int shadow_domain_init(struct domain *d)
{
static const struct log_dirty_ops sh_none_ops = {
.enable = _enable_log_dirty,
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6109623730..95321482ef 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -313,11 +313,19 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
- if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) &&
- (config->flags & XEN_DOMCTL_CDF_hap) )
+ if ( !(config->flags & XEN_DOMCTL_CDF_hvm_guest) )
{
- dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
- return -EINVAL;
+ if ( config->flags & XEN_DOMCTL_CDF_hap )
+ {
+ dprintk(XENLOG_INFO, "HAP enabled for non-HVM guest\n");
+ return -EINVAL;
+ }
+
+ /*
+ * It is only meaningful for XEN_DOMCTL_CDF_oos_off to be clear
+ * for HVM guests.
+ */
+ config->flags |= XEN_DOMCTL_CDF_oos_off;
}
return arch_sanitise_domain_config(config);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 933b85901f..5f9899469c 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -115,7 +115,6 @@ struct shadow_domain {
/* OOS */
bool_t oos_active;
- bool_t oos_off;
/* Has this domain ever used HVMOP_pagetable_dying? */
bool_t pagetable_dying_op;
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index f29f0f652b..8ebb89c027 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -49,7 +49,7 @@
/* Set up the shadow-specific parts of a domain struct at start of day.
* Called from paging_domain_init(). */
-int shadow_domain_init(struct domain *d, unsigned int domcr_flags);
+int shadow_domain_init(struct domain *d);
/* Setup the shadow-specific parts of a vcpu struct. It is called by
* paging_vcpu_init() in paging.c */
--
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] 35+ messages in thread* [Xen-devel] [PATCH v6 04/10] domain: remove the 'is_xenstore' flag
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (2 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 03/10] x86/domain: remove the 'oos_off' flag Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-19 20:44 ` Daniel De Graaf
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 05/10] x86/domain: remove the 's3_integrity' flag Paul Durrant
` (5 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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, Daniel De Graaf,
Roger Pau Monné
This patch introduces a convenience macro, is_xenstore_domain(), which
tests the domain 'options' directly and then uses that in place of
the 'is_xenstore' flag.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com>
Acked-by: George Dunlap <George.Dunlap@eu.citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@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: Wei Liu <wl@xen.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
v2:
- Set 'disable_migrate' to true rather 1
---
xen/common/domain.c | 9 +++------
xen/common/domctl.c | 2 +-
xen/include/xen/sched.h | 7 +++++--
xen/include/xsm/dummy.h | 2 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 95321482ef..76e6976617 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -363,7 +363,7 @@ struct domain *domain_create(domid_t domid,
if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
panic("The value of hardware_dom must be a valid domain ID\n");
- d->disable_migrate = 1;
+ d->disable_migrate = true;
old_hwdom = hardware_domain;
hardware_domain = d;
}
@@ -442,11 +442,8 @@ struct domain *domain_create(domid_t domid,
watchdog_domain_init(d);
init_status |= INIT_watchdog;
- if ( d->options & XEN_DOMCTL_CDF_xs_domain )
- {
- d->is_xenstore = 1;
- d->disable_migrate = 1;
- }
+ if ( is_xenstore_domain(d) )
+ d->disable_migrate = true;
d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex);
d->irq_caps = rangeset_new(d, "Interrupts", 0);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b48e408583..6e6e9b9866 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -186,7 +186,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
(d->is_shut_down ? XEN_DOMINF_shutdown : 0) |
(d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) |
(d->debugger_attached ? XEN_DOMINF_debugged : 0) |
- (d->is_xenstore ? XEN_DOMINF_xs_domain : 0) |
+ (is_xenstore_domain(d) ? XEN_DOMINF_xs_domain : 0) |
(is_hvm_domain(d) ? XEN_DOMINF_hvm_guest : 0) |
d->shutdown_code << XEN_DOMINF_shutdownshift;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 07a64947ed..df331a3bb0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -375,8 +375,6 @@ struct domain
bool is_privileged;
/* Can this guest access the Xen console? */
bool is_console;
- /* Is this a xenstore domain (not dom0)? */
- bool is_xenstore;
/* Non-migratable and non-restoreable? */
bool disable_migrate;
/* Is this guest being debugged by dom0? */
@@ -979,6 +977,11 @@ static inline bool is_vcpu_online(const struct vcpu *v)
return !test_bit(_VPF_down, &v->pause_flags);
}
+static inline bool is_xenstore_domain(const struct domain *d)
+{
+ return d->options & XEN_DOMCTL_CDF_xs_domain;
+}
+
extern bool sched_smt_power_savings;
extern enum cpufreq_controller {
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index ef52bb1764..b8e185e6fa 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -79,7 +79,7 @@ static always_inline int xsm_default_action(
{
return 0;
case XSM_XS_PRIV:
- if ( src->is_xenstore )
+ if ( is_xenstore_domain(src) )
return 0;
}
/* fall through */
--
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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 04/10] domain: remove the 'is_xenstore' flag
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 04/10] domain: remove the 'is_xenstore' flag Paul Durrant
@ 2019-08-19 20:44 ` Daniel De Graaf
0 siblings, 0 replies; 35+ messages in thread
From: Daniel De Graaf @ 2019-08-19 20:44 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Daniel De Graaf, Roger Pau Monné
On 8/16/19 1:19 PM, Paul Durrant wrote:
> This patch introduces a convenience macro, is_xenstore_domain(), which
> tests the domain 'options' directly and then uses that in place of
> the 'is_xenstore' flag.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com>
> Acked-by: George Dunlap <George.Dunlap@eu.citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 05/10] x86/domain: remove the 's3_integrity' flag
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (3 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 04/10] domain: remove the 'is_xenstore' flag Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag Paul Durrant
` (4 subsequent siblings)
9 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné
The flag is not needed since the domain 'options' can now be tested
directly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
v4:
- s/TBOOT/CONFIG_TBOOT/g
v3:
- Also sanitise the flag against CONFIG_TBOOT being set
---
xen/arch/x86/domain.c | 9 +++++++--
xen/arch/x86/setup.c | 2 +-
xen/arch/x86/tboot.c | 2 +-
xen/include/asm-x86/domain.h | 2 --
4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bc0db03387..f144d8fe9a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -466,6 +466,13 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ if ( (config->flags & XEN_DOMCTL_CDF_s3_integrity) &&
+ !IS_ENABLED(CONFIG_TBOOT) )
+ {
+ dprintk(XENLOG_INFO, "S3 integrity check not valid without CONFIG_TBOOT\n");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -544,8 +551,6 @@ int arch_domain_create(struct domain *d,
d->domain_id);
}
- d->arch.s3_integrity = config->flags & XEN_DOMCTL_CDF_s3_integrity;
-
emflags = config->arch.emulation_flags;
if ( is_hardware_domain(d) && is_pv_domain(d) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 87fc7c90da..d0b35b0ce2 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -696,7 +696,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
.stop_bits = 1
};
struct xen_domctl_createdomain dom0_cfg = {
- .flags = XEN_DOMCTL_CDF_s3_integrity,
+ .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
.max_grant_frames = opt_max_grant_frames,
.max_maptrack_frames = opt_max_maptrack_frames,
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index f3fdee4d39..3db8a8a8d8 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -212,7 +212,7 @@ static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
vmac_set_key((uint8_t *)key, &ctx);
for_each_domain( d )
{
- if ( !d->arch.s3_integrity )
+ if ( !(d->options & XEN_DOMCTL_CDF_s3_integrity) )
continue;
printk("MACing Domain %u\n", d->domain_id);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 5f9899469c..5c038a1065 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -295,8 +295,6 @@ struct arch_domain
uint32_t pci_cf8;
uint8_t cmos_idx;
- bool_t s3_integrity;
-
union {
struct pv_domain pv;
struct hvm_domain hvm;
--
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] 35+ messages in thread* [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (4 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 05/10] x86/domain: remove the 's3_integrity' flag Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-23 10:32 ` Roger Pau Monné
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
` (3 subsequent siblings)
9 siblings, 1 reply; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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
(i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject
the flag if !iommu_enabled.
A new helper function, is_iommu_enabled(), is added to test the flag and
iommu_domain_init() will return immediately if !is_iommu_enabled(). This is
slightly different to the previous behaviour based on !iommu_enabled where
the call to arch_iommu_domain_init() was made regardless, however it appears
that this call was only necessary to initialize the dt_devices list for ARM
such that iommu_release_dt_devices() can be called unconditionally by
domain_relinquish_resources(). Adding a simple check of is_iommu_enabled()
into iommu_release_dt_devices() keeps this unconditional call working.
No functional change should be observed with this patch applied.
Subsequent patches will allow the toolstack to control whether use of the
IOMMU is enabled for a domain.
NOTE: The introduction of the is_iommu_enabled() helper function might
seem excessive but its use is expected to increase with subsequent
patches. Also, having iommu_domain_init() bail before calling
arch_iommu_domain_init() is not strictly necessary, but I think the
consequent addition of the call to is_iommu_enabled() in
iommu_release_dt_devices() makes the code clearer.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
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>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v6:
- Remove the toolstack parts as there's no nice method of testing whether
the IOMMU is enabled in an architecture-neutral way
v5:
- Move is_iommu_enabled() check into iommu_domain_init()
- Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled
- Use evaluate_nospec() in defintion of is_iommu_enabled()
---
xen/arch/arm/setup.c | 3 +++
xen/arch/x86/setup.c | 3 +++
xen/common/domain.c | 9 ++++++++-
xen/common/domctl.c | 8 ++++++++
xen/drivers/passthrough/device_tree.c | 3 +++
xen/drivers/passthrough/iommu.c | 6 +++---
xen/include/public/domctl.h | 4 ++++
xen/include/xen/sched.h | 5 +++++
8 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2c5d1372c0..20021ee0ca 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -914,6 +914,9 @@ void __init start_xen(unsigned long boot_phys_offset,
dom0_cfg.arch.tee_type = tee_get_type();
dom0_cfg.max_vcpus = dom0_max_vcpus();
+ if ( iommu_enabled )
+ dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+
dom0 = domain_create(0, &dom0_cfg, true);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
panic("Error creating domain 0\n");
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d0b35b0ce2..fa226a2bab 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1733,6 +1733,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
}
dom0_cfg.max_vcpus = dom0_max_vcpus();
+ if ( iommu_enabled )
+ dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+
/* Create initial domain 0. */
dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 76e6976617..e832a5c4aa 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
XEN_DOMCTL_CDF_hap |
XEN_DOMCTL_CDF_s3_integrity |
XEN_DOMCTL_CDF_oos_off |
- XEN_DOMCTL_CDF_xs_domain) )
+ XEN_DOMCTL_CDF_xs_domain |
+ XEN_DOMCTL_CDF_iommu) )
{
dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
return -EINVAL;
@@ -328,6 +329,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
config->flags |= XEN_DOMCTL_CDF_oos_off;
}
+ if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled )
+ {
+ dprintk(XENLOG_INFO, "IOMMU is not enabled\n");
+ return -EINVAL;
+ }
+
return arch_sanitise_domain_config(config);
}
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 6e6e9b9866..fddf20f1b5 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -515,6 +515,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
rover = dom;
}
+ /*
+ * For now, make sure the createdomain IOMMU flag is set if the
+ * IOMMU is enabled. When the flag comes under toolstack control
+ * this can go away.
+ */
+ if ( iommu_enabled )
+ op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu;
+
d = domain_create(dom, &op->u.createdomain, false);
if ( IS_ERR(d) )
{
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index b6eaae7283..d32b172664 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -119,6 +119,9 @@ int iommu_release_dt_devices(struct domain *d)
struct dt_device_node *dev, *_dev;
int rc;
+ if ( !is_iommu_enabled(d) )
+ return 0;
+
list_for_each_entry_safe(dev, _dev, &hd->dt_devices, domain_list)
{
rc = iommu_deassign_dt_device(d, dev);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 37eb0f7d01..e61d3d1368 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -151,13 +151,13 @@ int iommu_domain_init(struct domain *d)
struct domain_iommu *hd = dom_iommu(d);
int ret = 0;
+ if ( !is_iommu_enabled(d) )
+ return 0;
+
ret = arch_iommu_domain_init(d);
if ( ret )
return ret;
- if ( !iommu_enabled )
- return 0;
-
hd->platform_ops = iommu_get_ops();
return hd->platform_ops->init(d);
}
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 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 df331a3bb0..258924242f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -982,6 +982,11 @@ static inline bool is_xenstore_domain(const struct domain *d)
return d->options & XEN_DOMCTL_CDF_xs_domain;
}
+static inline bool is_iommu_enabled(const struct domain *d)
+{
+ return evaluate_nospec(d->options & XEN_DOMCTL_CDF_iommu);
+}
+
extern bool sched_smt_power_savings;
extern enum cpufreq_controller {
--
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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag Paul Durrant
@ 2019-08-23 10:32 ` Roger Pau Monné
2019-08-29 9:00 ` Paul Durrant
0 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 10:32 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Anthony PERARD, xen-devel,
Volodymyr Babchuk
On Fri, Aug 16, 2019 at 06:19:57PM +0100, Paul Durrant wrote:
> This patch introduces a common domain creation flag to determine whether
> the domain is permitted to make use of the IOMMU. Currently the flag is
> always set (for both dom0 and domU) if the IOMMU is globally enabled
> (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject
> the flag if !iommu_enabled.
>
> A new helper function, is_iommu_enabled(), is added to test the flag and
> iommu_domain_init() will return immediately if !is_iommu_enabled(). This is
> slightly different to the previous behaviour based on !iommu_enabled where
> the call to arch_iommu_domain_init() was made regardless, however it appears
> that this call was only necessary to initialize the dt_devices list for ARM
> such that iommu_release_dt_devices() can be called unconditionally by
> domain_relinquish_resources(). Adding a simple check of is_iommu_enabled()
> into iommu_release_dt_devices() keeps this unconditional call working.
>
> No functional change should be observed with this patch applied.
>
> Subsequent patches will allow the toolstack to control whether use of the
> IOMMU is enabled for a domain.
>
> NOTE: The introduction of the is_iommu_enabled() helper function might
> seem excessive but its use is expected to increase with subsequent
> patches. Also, having iommu_domain_init() bail before calling
> arch_iommu_domain_init() is not strictly necessary, but I think the
> consequent addition of the call to is_iommu_enabled() in
> iommu_release_dt_devices() makes the code clearer.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
I have one ARM-related question and one 'nice to have', but the code
LGTM:
Reviewed-by: Roger Pau Monné <roger.pau@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>
>
> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
>
> v6:
> - Remove the toolstack parts as there's no nice method of testing whether
> the IOMMU is enabled in an architecture-neutral way
>
> v5:
> - Move is_iommu_enabled() check into iommu_domain_init()
> - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled
> - Use evaluate_nospec() in defintion of is_iommu_enabled()
> ---
> xen/arch/arm/setup.c | 3 +++
> xen/arch/x86/setup.c | 3 +++
> xen/common/domain.c | 9 ++++++++-
> xen/common/domctl.c | 8 ++++++++
> xen/drivers/passthrough/device_tree.c | 3 +++
> xen/drivers/passthrough/iommu.c | 6 +++---
> xen/include/public/domctl.h | 4 ++++
> xen/include/xen/sched.h | 5 +++++
> 8 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2c5d1372c0..20021ee0ca 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -914,6 +914,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> dom0_cfg.arch.tee_type = tee_get_type();
> dom0_cfg.max_vcpus = dom0_max_vcpus();
>
> + if ( iommu_enabled )
> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
> dom0 = domain_create(0, &dom0_cfg, true);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> panic("Error creating domain 0\n");
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d0b35b0ce2..fa226a2bab 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1733,6 +1733,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> }
> dom0_cfg.max_vcpus = dom0_max_vcpus();
>
> + if ( iommu_enabled )
> + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> +
> /* Create initial domain 0. */
> dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 76e6976617..e832a5c4aa 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> XEN_DOMCTL_CDF_hap |
> XEN_DOMCTL_CDF_s3_integrity |
> XEN_DOMCTL_CDF_oos_off |
> - XEN_DOMCTL_CDF_xs_domain) )
> + XEN_DOMCTL_CDF_xs_domain |
> + XEN_DOMCTL_CDF_iommu) )
> {
> dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
> return -EINVAL;
> @@ -328,6 +329,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> config->flags |= XEN_DOMCTL_CDF_oos_off;
> }
>
> + if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled )
> + {
> + dprintk(XENLOG_INFO, "IOMMU is not enabled\n");
> + return -EINVAL;
> + }
> +
> return arch_sanitise_domain_config(config);
> }
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 6e6e9b9866..fddf20f1b5 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -515,6 +515,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> rover = dom;
> }
>
> + /*
> + * For now, make sure the createdomain IOMMU flag is set if the
> + * IOMMU is enabled. When the flag comes under toolstack control
> + * this can go away.
> + */
> + if ( iommu_enabled )
> + op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu;
Can you add some kind of safety check here to make sure this bodge is
removed when the toolstack takes control of the flag, ie:
BUG_ON(op->u.createdomain.flags & XEN_DOMCTL_CDF_iommu);
Or maybe an ASSERT_UNREACHABLE() followed by returning EINVAL?
> +
> d = domain_create(dom, &op->u.createdomain, false);
> if ( IS_ERR(d) )
> {
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index b6eaae7283..d32b172664 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -119,6 +119,9 @@ int iommu_release_dt_devices(struct domain *d)
> struct dt_device_node *dev, *_dev;
> int rc;
>
> + if ( !is_iommu_enabled(d) )
> + return 0;
How could you get here? If the domain doesn't have an iommu how did it
got the devices assigned in the first place?
The hardware domain on ARM would be an exception here, since it uses
an identity second stage translation, but I don't think
iommu_release_dt_devices is ever used against the hardware domain.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
2019-08-23 10:32 ` Roger Pau Monné
@ 2019-08-29 9:00 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 9:00 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Anthony Perard, xen-devel, Volodymyr Babchuk
> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 23 August 2019 11:33
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>;
> Anthony Perard <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall <julien.grall@arm.com>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag
>
> On Fri, Aug 16, 2019 at 06:19:57PM +0100, Paul Durrant wrote:
> > This patch introduces a common domain creation flag to determine whether
> > the domain is permitted to make use of the IOMMU. Currently the flag is
> > always set (for both dom0 and domU) if the IOMMU is globally enabled
> > (i.e. iommu_enabled == 1). sanitise_domain_config() is modified to reject
> > the flag if !iommu_enabled.
> >
> > A new helper function, is_iommu_enabled(), is added to test the flag and
> > iommu_domain_init() will return immediately if !is_iommu_enabled(). This is
> > slightly different to the previous behaviour based on !iommu_enabled where
> > the call to arch_iommu_domain_init() was made regardless, however it appears
> > that this call was only necessary to initialize the dt_devices list for ARM
> > such that iommu_release_dt_devices() can be called unconditionally by
> > domain_relinquish_resources(). Adding a simple check of is_iommu_enabled()
> > into iommu_release_dt_devices() keeps this unconditional call working.
> >
> > No functional change should be observed with this patch applied.
> >
> > Subsequent patches will allow the toolstack to control whether use of the
> > IOMMU is enabled for a domain.
> >
> > NOTE: The introduction of the is_iommu_enabled() helper function might
> > seem excessive but its use is expected to increase with subsequent
> > patches. Also, having iommu_domain_init() bail before calling
> > arch_iommu_domain_init() is not strictly necessary, but I think the
> > consequent addition of the call to is_iommu_enabled() in
> > iommu_release_dt_devices() makes the code clearer.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> I have one ARM-related question and one 'nice to have', but the code
> LGTM:
>
> Reviewed-by: Roger Pau Monné <roger.pau@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>
> >
> > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> >
> > v6:
> > - Remove the toolstack parts as there's no nice method of testing whether
> > the IOMMU is enabled in an architecture-neutral way
> >
> > v5:
> > - Move is_iommu_enabled() check into iommu_domain_init()
> > - Reject XEN_DOMCTL_CDF_iommu in sanitise_domain_config() if !iommu_enabled
> > - Use evaluate_nospec() in defintion of is_iommu_enabled()
> > ---
> > xen/arch/arm/setup.c | 3 +++
> > xen/arch/x86/setup.c | 3 +++
> > xen/common/domain.c | 9 ++++++++-
> > xen/common/domctl.c | 8 ++++++++
> > xen/drivers/passthrough/device_tree.c | 3 +++
> > xen/drivers/passthrough/iommu.c | 6 +++---
> > xen/include/public/domctl.h | 4 ++++
> > xen/include/xen/sched.h | 5 +++++
> > 8 files changed, 37 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 2c5d1372c0..20021ee0ca 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -914,6 +914,9 @@ void __init start_xen(unsigned long boot_phys_offset,
> > dom0_cfg.arch.tee_type = tee_get_type();
> > dom0_cfg.max_vcpus = dom0_max_vcpus();
> >
> > + if ( iommu_enabled )
> > + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > +
> > dom0 = domain_create(0, &dom0_cfg, true);
> > if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> > panic("Error creating domain 0\n");
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index d0b35b0ce2..fa226a2bab 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1733,6 +1733,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > }
> > dom0_cfg.max_vcpus = dom0_max_vcpus();
> >
> > + if ( iommu_enabled )
> > + dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > +
> > /* Create initial domain 0. */
> > dom0 = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
> > if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 76e6976617..e832a5c4aa 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> > XEN_DOMCTL_CDF_hap |
> > XEN_DOMCTL_CDF_s3_integrity |
> > XEN_DOMCTL_CDF_oos_off |
> > - XEN_DOMCTL_CDF_xs_domain) )
> > + XEN_DOMCTL_CDF_xs_domain |
> > + XEN_DOMCTL_CDF_iommu) )
> > {
> > dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
> > return -EINVAL;
> > @@ -328,6 +329,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> > config->flags |= XEN_DOMCTL_CDF_oos_off;
> > }
> >
> > + if ( (config->flags & XEN_DOMCTL_CDF_iommu) && !iommu_enabled )
> > + {
> > + dprintk(XENLOG_INFO, "IOMMU is not enabled\n");
> > + return -EINVAL;
> > + }
> > +
> > return arch_sanitise_domain_config(config);
> > }
> >
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index 6e6e9b9866..fddf20f1b5 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -515,6 +515,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > rover = dom;
> > }
> >
> > + /*
> > + * For now, make sure the createdomain IOMMU flag is set if the
> > + * IOMMU is enabled. When the flag comes under toolstack control
> > + * this can go away.
> > + */
> > + if ( iommu_enabled )
> > + op->u.createdomain.flags |= XEN_DOMCTL_CDF_iommu;
>
> Can you add some kind of safety check here to make sure this bodge is
> removed when the toolstack takes control of the flag, ie:
>
> BUG_ON(op->u.createdomain.flags & XEN_DOMCTL_CDF_iommu);
>
> Or maybe an ASSERT_UNREACHABLE() followed by returning EINVAL?
Ok, the former is a bit severe so I think I'll go for the latter.
>
> > +
> > d = domain_create(dom, &op->u.createdomain, false);
> > if ( IS_ERR(d) )
> > {
> > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> > index b6eaae7283..d32b172664 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -119,6 +119,9 @@ int iommu_release_dt_devices(struct domain *d)
> > struct dt_device_node *dev, *_dev;
> > int rc;
> >
> > + if ( !is_iommu_enabled(d) )
> > + return 0;
>
> How could you get here? If the domain doesn't have an iommu how did it
> got the devices assigned in the first place?
>
As I say in the commit comment, iommu_release_dt_device() is called unconditionally from domain_relinquish_resources().
> The hardware domain on ARM would be an exception here, since it uses
> an identity second stage translation, but I don't think
> iommu_release_dt_devices is ever used against the hardware domain.
>
No, it is only called from domain_relinquish_resources() so should not apply to the h/w domain.
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (5 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 06/10] domain: introduce XEN_DOMCTL_CDF_iommu flag Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-19 20:55 ` Daniel De Graaf
` (3 more replies)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables Paul Durrant
` (2 subsequent siblings)
9 siblings, 4 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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, Daniel De Graaf, 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. Also, whilst the style of the 'if' in
flask_iommu_resource_use_perm() is fixed, I have not translated any
instances of u32 into uint32_t to keep consistency. IMO such a
translation would be better done globally for the source module in
a separate patch.
The change in the initialization of the 'hd' variable in iommu_map()
and iommu_unmap() is done to keep the PV shim build happy. Without
this change it will fail to compile with errors of the form:
iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
const struct domain_iommu *hd = dom_iommu(d);
^~
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>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v5:
- Fix logic in ARM p2m_init()
- Make iommu_do_domctl() return -EOPNOTSUPP rather than -ENOSYS if the
IOMMU is not enabled
- Fix test in pci_enable_acs()
- Fix test in flask_iommu_resource_use_perm()
---
xen/arch/arm/p2m.c | 2 +-
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 | 39 ++++++++++++-----------
xen/drivers/passthrough/pci.c | 16 +++++-----
xen/drivers/passthrough/vtd/iommu.c | 2 +-
xen/drivers/passthrough/vtd/x86/hvm.c | 2 +-
xen/drivers/passthrough/x86/iommu.c | 2 +-
xen/xsm/flask/hooks.c | 18 +++++------
17 files changed, 59 insertions(+), 58 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e28ea1c85a..7f1442932a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1531,7 +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 &&
+ p2m->clean_pte = is_iommu_enabled(d) &&
!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 16f14abe8f..ed27e8def7 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..93d031cc6c 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(d) && 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 7f2dd662af..1f2bcfbe15 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -821,7 +821,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 e61d3d1368..27c75e0786 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);
@@ -296,16 +296,18 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned int page_order, unsigned int flags,
unsigned int *flush_flags)
{
- const struct domain_iommu *hd = dom_iommu(d);
+ const struct domain_iommu *hd;
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)));
ASSERT(IS_ALIGNED(mfn_x(mfn), (1ul << page_order)));
+ hd = dom_iommu(d);
+
for ( i = 0; i < (1ul << page_order); i++ )
{
rc = iommu_call(hd->platform_ops, map_page, d, dfn_add(dfn, i),
@@ -356,15 +358,17 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
unsigned int *flush_flags)
{
- const struct domain_iommu *hd = dom_iommu(d);
+ const struct domain_iommu *hd;
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)));
+ hd = dom_iommu(d);
+
for ( i = 0; i < (1ul << page_order); i++ )
{
int err = iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
@@ -413,7 +417,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 +446,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 +474,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,8 +560,8 @@ int iommu_do_domctl(
{
int ret = -ENODEV;
- if ( !iommu_enabled )
- return -ENOSYS;
+ if ( !is_iommu_enabled(d) )
+ return -EOPNOTSUPP;
#ifdef CONFIG_HAS_PCI
ret = iommu_do_pci_domctl(domctl, d, u_domctl);
@@ -576,9 +580,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 +612,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 89536cc067..dee47fe08b 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -595,7 +595,7 @@ static void pci_enable_acs(struct pci_dev *pdev)
u16 cap, ctrl, seg = pdev->seg;
u8 bus = pdev->bus;
- if ( !iommu_enabled )
+ if ( !is_iommu_enabled(pdev->domain) )
return;
pos = pci_find_ext_capability(seg, bus, pdev->devfn, PCI_EXT_CAP_ID_ACS);
@@ -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) )
@@ -897,7 +897,7 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
struct pci_dev *pdev;
int ret = 0;
- if ( !iommu_enabled || !hd->platform_ops )
+ if ( !is_iommu_enabled(d) )
return -EINVAL;
ASSERT(pcidevs_locked());
@@ -1383,7 +1383,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));
@@ -1412,7 +1412,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;
@@ -1428,7 +1428,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; )
@@ -1471,7 +1471,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
@@ -1537,7 +1537,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");
}
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 791c1f66af..5a0f2e723e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -886,7 +886,7 @@ static int flask_map_domain_msi (struct domain *d, int irq, const void *data,
#endif
}
-static u32 flask_iommu_resource_use_perm(void)
+static u32 flask_iommu_resource_use_perm(struct domain *d)
{
/* Obtain the permission level required for allowing a domain
* to use an assigned device.
@@ -899,7 +899,7 @@ static u32 flask_iommu_resource_use_perm(void)
*/
u32 perm = RESOURCE__USE_NOIOMMU;
- if (iommu_enabled)
+ if ( is_iommu_enabled(d) )
perm = ( iommu_intremap ? RESOURCE__USE_IOMMU :
RESOURCE__USE_IOMMU_NOINTREMAP );
return perm;
@@ -910,7 +910,7 @@ static int flask_map_domain_irq (struct domain *d, int irq, const void *data)
u32 sid, dsid;
int rc = -EPERM;
struct avc_audit_data ad;
- u32 dperm = flask_iommu_resource_use_perm();
+ u32 dperm = flask_iommu_resource_use_perm(d);
if ( irq >= nr_static_irqs && data ) {
rc = flask_map_domain_msi(d, irq, data, &sid, &ad);
@@ -976,7 +976,7 @@ static int flask_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *b
int rc = -EPERM;
int irq;
struct avc_audit_data ad;
- u32 dperm = flask_iommu_resource_use_perm();
+ u32 dperm = flask_iommu_resource_use_perm(d);
rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
if ( rc )
@@ -1049,7 +1049,7 @@ static int flask_iomem_permission(struct domain *d, uint64_t start, uint64_t end
data.ssid = domain_sid(current->domain);
data.dsid = domain_sid(d);
- data.use_perm = flask_iommu_resource_use_perm();
+ data.use_perm = flask_iommu_resource_use_perm(d);
return security_iterate_iomem_sids(start, end, _iomem_has_perm, &data);
}
@@ -1074,7 +1074,7 @@ static int flask_pci_config_permission(struct domain *d, uint32_t machine_bdf, u
if ( access && (end >= 0x10 && start < 0x28) )
perm = RESOURCE__SETUP;
else
- perm = flask_iommu_resource_use_perm();
+ perm = flask_iommu_resource_use_perm(d);
AVC_AUDIT_DATA_INIT(&ad, DEV);
ad.device = (unsigned long) machine_bdf;
@@ -1299,7 +1299,7 @@ static int flask_assign_device(struct domain *d, uint32_t machine_bdf)
u32 dsid, rsid;
int rc = -EPERM;
struct avc_audit_data ad;
- u32 dperm = flask_iommu_resource_use_perm();
+ u32 dperm = flask_iommu_resource_use_perm(d);
if ( !d )
return flask_test_assign_device(machine_bdf);
@@ -1358,7 +1358,7 @@ static int flask_assign_dtdevice(struct domain *d, const char *dtpath)
u32 dsid, rsid;
int rc = -EPERM;
struct avc_audit_data ad;
- u32 dperm = flask_iommu_resource_use_perm();
+ u32 dperm = flask_iommu_resource_use_perm(d);
if ( !d )
return flask_test_assign_dtdevice(dtpath);
@@ -1543,7 +1543,7 @@ static int flask_ioport_permission(struct domain *d, uint32_t start, uint32_t en
data.ssid = domain_sid(current->domain);
data.dsid = domain_sid(d);
- data.use_perm = flask_iommu_resource_use_perm();
+ data.use_perm = flask_iommu_resource_use_perm(d);
return security_iterate_ioport_sids(start, end, _ioport_has_perm, &data);
}
--
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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
@ 2019-08-19 20:55 ` Daniel De Graaf
2019-08-23 3:04 ` Tian, Kevin
` (2 subsequent siblings)
3 siblings, 0 replies; 35+ messages in thread
From: Daniel De Graaf @ 2019-08-19 20:55 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu,
George Dunlap, Andrew Cooper, Brian Woods, Julien Grall,
Jan Beulich, Daniel De Graaf, Volodymyr Babchuk,
Suravee Suthikulpanit, Roger Pau Monné
On 8/16/19 1:19 PM, Paul Durrant wrote:
> ...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. Also, whilst the style of the 'if' in
> flask_iommu_resource_use_perm() is fixed, I have not translated any
> instances of u32 into uint32_t to keep consistency. IMO such a
> translation would be better done globally for the source module in
> a separate patch.
> The change in the initialization of the 'hd' variable in iommu_map()
> and iommu_unmap() is done to keep the PV shim build happy. Without
> this change it will fail to compile with errors of the form:
>
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> const struct domain_iommu *hd = dom_iommu(d);
> ^~
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
2019-08-19 20:55 ` Daniel De Graaf
@ 2019-08-23 3:04 ` Tian, Kevin
2019-08-23 10:55 ` Roger Pau Monné
2019-08-29 13:29 ` Jan Beulich
3 siblings, 0 replies; 35+ messages in thread
From: Tian, Kevin @ 2019-08-23 3:04 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Stefano Stabellini, Nakajima, Jun, Wei Liu, George Dunlap,
Andrew Cooper, Brian Woods, Julien Grall, Jan Beulich,
Daniel De Graaf, Volodymyr Babchuk, Suravee Suthikulpanit,
Roger Pau Monné
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: Saturday, August 17, 2019 1:20 AM
>
> ...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. Also, whilst the style of the 'if' in
> flask_iommu_resource_use_perm() is fixed, I have not translated any
> instances of u32 into uint32_t to keep consistency. IMO such a
> translation would be better done globally for the source module in
> a separate patch.
> The change in the initialization of the 'hd' variable in iommu_map()
> and iommu_unmap() is done to keep the PV shim build happy. Without
> this change it will fail to compile with errors of the form:
>
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> const struct domain_iommu *hd = dom_iommu(d);
> ^~
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
2019-08-19 20:55 ` Daniel De Graaf
2019-08-23 3:04 ` Tian, Kevin
@ 2019-08-23 10:55 ` Roger Pau Monné
2019-08-29 9:17 ` Paul Durrant
2019-08-29 13:29 ` Jan Beulich
3 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 10:55 UTC (permalink / raw)
To: Paul Durrant
Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu,
George Dunlap, Andrew Cooper, Brian Woods, Julien Grall,
Jan Beulich, xen-devel, Daniel De Graaf, Volodymyr Babchuk,
Suravee Suthikulpanit
On Fri, Aug 16, 2019 at 06:19:58PM +0100, Paul Durrant wrote:
> ...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. Also, whilst the style of the 'if' in
> flask_iommu_resource_use_perm() is fixed, I have not translated any
> instances of u32 into uint32_t to keep consistency. IMO such a
> translation would be better done globally for the source module in
> a separate patch.
I've usually changed those instances as the line gets modified anyway,
but I'm not going to ask everyone to do this if they don't feel like
it.
The problem with doing wholesale changes of u32 -> uint32_t is that
you introduce a lot of noise when trying to use git blame afterwards
for example. Doing it when touching the line for legitimate changes
avoids the noise.
> The change in the initialization of the 'hd' variable in iommu_map()
> and iommu_unmap() is done to keep the PV shim build happy. Without
> this change it will fail to compile with errors of the form:
>
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> const struct domain_iommu *hd = dom_iommu(d);
> ^~
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
I haven't looked however if there are further cases where
iommu_enabled should be changed into is_iommu_enabled.
> @@ -556,8 +560,8 @@ int iommu_do_domctl(
> {
> int ret = -ENODEV;
>
> - if ( !iommu_enabled )
> - return -ENOSYS;
> + if ( !is_iommu_enabled(d) )
> + return -EOPNOTSUPP;
I would use ENOENT here, but I don't have a strong opinion. The
hypercall is supported, it's just that there's no iommu for this
domain.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-23 10:55 ` Roger Pau Monné
@ 2019-08-29 9:17 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 9:17 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Kevin Tian, Stefano Stabellini, Jun Nakajima, Wei Liu,
Andrew Cooper, Brian Woods, George Dunlap, Julien Grall,
Jan Beulich, xen-devel, Daniel De Graaf, Volodymyr Babchuk,
Suravee Suthikulpanit
> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 23 August 2019 11:56
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien.grall@arm.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; George Dunlap <George.Dunlap@citrix.com>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>; Daniel De
> Graaf <dgdegra@tycho.nsa.gov>
> Subject: Re: [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
>
> On Fri, Aug 16, 2019 at 06:19:58PM +0100, Paul Durrant wrote:
> > ...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. Also, whilst the style of the 'if' in
> > flask_iommu_resource_use_perm() is fixed, I have not translated any
> > instances of u32 into uint32_t to keep consistency. IMO such a
> > translation would be better done globally for the source module in
> > a separate patch.
>
> I've usually changed those instances as the line gets modified anyway,
> but I'm not going to ask everyone to do this if they don't feel like
> it.
>
> The problem with doing wholesale changes of u32 -> uint32_t is that
> you introduce a lot of noise when trying to use git blame afterwards
> for example. Doing it when touching the line for legitimate changes
> avoids the noise.
>
> > The change in the initialization of the 'hd' variable in iommu_map()
> > and iommu_unmap() is done to keep the PV shim build happy. Without
> > this change it will fail to compile with errors of the form:
> >
> > iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> > const struct domain_iommu *hd = dom_iommu(d);
> > ^~
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
Thanks.
> I haven't looked however if there are further cases where
> iommu_enabled should be changed into is_iommu_enabled.
>
Jan had clearly checked on a previous review iteration because he spotted a couple of places I had missed. I'm reasonably confident I've found them all now.
Paul
> > @@ -556,8 +560,8 @@ int iommu_do_domctl(
> > {
> > int ret = -ENODEV;
> >
> > - if ( !iommu_enabled )
> > - return -ENOSYS;
> > + if ( !is_iommu_enabled(d) )
> > + return -EOPNOTSUPP;
>
> I would use ENOENT here, but I don't have a strong opinion. The
> hypercall is supported, it's just that there's no iommu for this
> domain.
>
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
` (2 preceding siblings ...)
2019-08-23 10:55 ` Roger Pau Monné
@ 2019-08-29 13:29 ` Jan Beulich
3 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-08-29 13:29 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, DanielDe Graaf, Brian Woods,
Roger Pau Monné
On 16.08.2019 19:19, Paul Durrant wrote:
> ...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. Also, whilst the style of the 'if' in
> flask_iommu_resource_use_perm() is fixed, I have not translated any
> instances of u32 into uint32_t to keep consistency. IMO such a
> translation would be better done globally for the source module in
> a separate patch.
> The change in the initialization of the 'hd' variable in iommu_map()
> and iommu_unmap() is done to keep the PV shim build happy. Without
> this change it will fail to compile with errors of the form:
>
> iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> const struct domain_iommu *hd = dom_iommu(d);
> ^~
Why would that be? Afaict there's no short-circuiting of
is_iommu_enabled() (albeit there could be, as we don't mean the shim
to have/use an IOMMU). Oh - I guess I see it: Instead of this change
what I think we want is for x86's iommu_call() to evaluate its 1st
argument. Otherwise we're liable to run into the same issue elsewhere
again.
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -886,7 +886,7 @@ static int flask_map_domain_msi (struct domain *d, int irq, const void *data,
> #endif
> }
>
> -static u32 flask_iommu_resource_use_perm(void)
> +static u32 flask_iommu_resource_use_perm(struct domain *d)
const?
With these adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (6 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate Paul Durrant
@ 2019-08-16 17:19 ` Paul Durrant
2019-08-16 17:24 ` Razvan Cojocaru
` (2 more replies)
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
9 siblings, 3 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:19 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 removed.
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>
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: Petre Pircalabu <ppircalabu@bitdefender.com>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v5:
- Minor style fixes
---
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 7f1442932a..692565757e 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..5ad15eafe0 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 27c75e0786..dc7b75fab6 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) )
@@ -578,11 +504,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);
}
@@ -629,8 +552,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 dee47fe08b..28b77b3e30 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -933,9 +933,6 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
pdev->fault.count = 0;
- if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
- iommu_teardown(d);
-
return ret;
}
@@ -1484,13 +1481,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 )
{
@@ -1519,8 +1509,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;
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..1577e83d2b 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 314f28f323..4b6871936c 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -73,15 +73,9 @@ void iommu_domain_destroy(struct domain *d);
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.
@@ -248,13 +242,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;
@@ -269,9 +256,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 258924242f..b376768424 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -965,10 +965,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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-08-16 17:24 ` Razvan Cojocaru
2019-08-23 11:34 ` Roger Pau Monné
2019-08-29 13:39 ` Jan Beulich
2 siblings, 0 replies; 35+ messages in thread
From: Razvan Cojocaru @ 2019-08-16 17:24 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Jan Beulich,
Alexandru Isaila, Volodymyr Babchuk, Roger Pau Monné
On 8/16/19 8:19 PM, 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 removed.
>
> 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.
This has slipped under my radar, sorry.
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-08-16 17:24 ` Razvan Cojocaru
@ 2019-08-23 11:34 ` Roger Pau Monné
2019-08-29 9:23 ` Paul Durrant
2019-08-29 13:39 ` Jan Beulich
2 siblings, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 11:34 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre 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,
Alexandru Isaila, xen-devel, Volodymyr Babchuk
On Fri, Aug 16, 2019 at 06:19:59PM +0100, 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 removed.
>
> 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>
> 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: Petre Pircalabu <ppircalabu@bitdefender.com>
>
> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
>
> v5:
> - Minor style fixes
> ---
> 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 7f1442932a..692565757e 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));
I'm not sure this is correct, at the point where dom0_compute_nr_pages
gets called the iommu has not been initialized yet (the call to
iommu_hwdom_init is done afterwards), so the iommu status field which
is used by iommu_use_hap_pt is not yet initialized.
> 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..5ad15eafe0 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 27c75e0786..dc7b75fab6 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();
> - }
Don't you need to add the domain pages to the page-tables?
iommu_hwdom_init is called after the memory for the domain has been
added. Maybe this is fine because the iommu would be enabled earlier
and thus pages added to the domain would already be added to the iommu
page-tables if required?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-23 11:34 ` Roger Pau Monné
@ 2019-08-29 9:23 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 9:23 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Petre Pircalabu, Stefano Stabellini, Razvan Cojocaru, Wei Liu,
Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, Jan Beulich,
Ian Jackson, Alexandru Isaila, xen-devel, Volodymyr Babchuk
> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 23 August 2019 12:34
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Alexandru Isaila <aisaila@bitdefender.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>;
> Tamas K Lengyel <tamas@tklengyel.com>; Razvan Cojocaru <rcojocaru@bitdefender.com>; Petre Pircalabu
> <ppircalabu@bitdefender.com>
> Subject: Re: [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
>
> On Fri, Aug 16, 2019 at 06:19:59PM +0100, 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 removed.
> >
> > 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>
> > 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: Petre Pircalabu <ppircalabu@bitdefender.com>
> >
> > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> >
> > v5:
> > - Minor style fixes
> > ---
> > 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 7f1442932a..692565757e 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));
>
> I'm not sure this is correct, at the point where dom0_compute_nr_pages
> gets called the iommu has not been initialized yet (the call to
> iommu_hwdom_init is done afterwards), so the iommu status field which
> is used by iommu_use_hap_pt is not yet initialized.
Note that this patch removes the iommu status field.
>
> > 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..5ad15eafe0 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 27c75e0786..dc7b75fab6 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();
> > - }
>
> Don't you need to add the domain pages to the page-tables?
>
Not any more.
> iommu_hwdom_init is called after the memory for the domain has been
> added. Maybe this is fine because the iommu would be enabled earlier
> and thus pages added to the domain would already be added to the iommu
> page-tables if required?
Yes, that's right. Because we don't have the late init any more, the pages will be added to the IOMMU mappings when they get added to the domain.
Paul
>
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables Paul Durrant
2019-08-16 17:24 ` Razvan Cojocaru
2019-08-23 11:34 ` Roger Pau Monné
@ 2019-08-29 13:39 ` Jan Beulich
2019-08-29 13:44 ` Paul Durrant
2 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-08-29 13:39 UTC (permalink / raw)
To: Paul Durrant
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
KonradRzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Tamas K Lengyel, Alexandru Isaila,
xen-devel, Volodymyr Babchuk, Roger Pau Monné
On 16.08.2019 19:19, Paul Durrant wrote:
> --- 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)
This really should have const, but I realize ...
> +{
> + if ( iommu_hwdom_none || !paging_mode_translate(d) )
> + return;
> +
> + arch_iommu_check_autotranslated_hwdom(d);
... this one wants non-const (for - afaict - no reason).
> @@ -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().
> + */
Doesn't this comment belong to ...
> + 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);
... this if()?
> @@ -629,8 +552,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;
Didn't you agree to retain the hwdom part of the condition here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
2019-08-29 13:39 ` Jan Beulich
@ 2019-08-29 13:44 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 13:44 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: Petre Pircalabu, Stefano Stabellini, Wei Liu, Razvan Cojocaru,
KonradRzeszutek Wilk, Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Tamas K Lengyel, Ian Jackson,
Alexandru Isaila, xen-devel, Volodymyr Babchuk, Roger Pau Monne
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 29 August 2019 14:39
> 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>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; KonradRzeszutek Wilk <konrad.wilk@oracle.com>; Tamas K Lengyel
> <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables
>
> On 16.08.2019 19:19, Paul Durrant wrote:
> > --- 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)
>
> This really should have const, but I realize ...
>
> > +{
> > + if ( iommu_hwdom_none || !paging_mode_translate(d) )
> > + return;
> > +
> > + arch_iommu_check_autotranslated_hwdom(d);
>
> ... this one wants non-const (for - afaict - no reason).
>
> > @@ -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().
> > + */
>
> Doesn't this comment belong to ...
>
> > + 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);
>
> ... this if()?
Yes, it's probably adrift from where it should be now.
>
> > @@ -629,8 +552,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;
>
> Didn't you agree to retain the hwdom part of the condition here?
>
Indeed I have as of today, but this was posted 2 weeks ago :-)
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (7 preceding siblings ...)
2019-08-16 17:19 ` [Xen-devel] [PATCH v6 08/10] remove late (on-demand) construction of IOMMU page tables Paul Durrant
@ 2019-08-16 17:20 ` Paul Durrant
2019-08-23 11:39 ` Roger Pau Monné
2019-08-29 13:50 ` Jan Beulich
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
9 siblings, 2 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:20 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>
Previously part of https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
---
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 | 11 +++++++++++
xen/include/xen/sched.h | 6 ------
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index dc7b75fab6..b24be5ffa6 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 1577e83d2b..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..5e7ca98170 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -268,6 +268,17 @@ 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 */
+#ifdef CONFIG_HAS_PASSTHROUGH
+#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
+#else
+#define need_iommu_pt_sync(d) false
+#endif
+
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 b376768424..7e4c24f8d0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -964,12 +964,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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
@ 2019-08-23 11:39 ` Roger Pau Monné
2019-08-29 13:50 ` Jan Beulich
1 sibling, 0 replies; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 11:39 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, xen-devel, Volodymyr Babchuk
On Fri, Aug 16, 2019 at 06:20:00PM +0100, 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.
>
> 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>
>
> Previously part of https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> ---
> 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 | 11 +++++++++++
> xen/include/xen/sched.h | 6 ------
> 5 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index dc7b75fab6..b24be5ffa6 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
I would prefer if you would print a warning message in iommu_setup if
iommu_hap_pt_share is set to false on ARM, and set it to true.
The rest LGTM.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
2019-08-23 11:39 ` Roger Pau Monné
@ 2019-08-29 13:50 ` Jan Beulich
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2019-08-29 13:50 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 16.08.2019 19:20, Paul Durrant wrote:
> --- 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;
I think you/we should go further here: Arm should #define this to
true, and here we should have "#ifndef iommu_hap_pt_share".
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -268,6 +268,17 @@ 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 */
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> +#else
> +#define need_iommu_pt_sync(d) false
I think you'd better evaluate d here; one (somewhat in risk of
opposition) variant would be
#define need_iommu_pt_sync(d) (!(d))
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-16 17:19 [Xen-devel] [PATCH v6 00/10] use stashed domain create flags Paul Durrant
` (8 preceding siblings ...)
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 09/10] iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync() macros Paul Durrant
@ 2019-08-16 17:20 ` Paul Durrant
2019-08-23 14:16 ` Roger Pau Monné
2019-08-29 14:07 ` Jan Beulich
9 siblings, 2 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-16 17:20 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.
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>
Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
v6:
- Remove the libxl_physinfo() call since it's usefulness is limited to x86
v5:
- Expand xen_domctl_createdomain flags field and hence bump interface
version
- Fix spelling mistakes in context line
---
docs/man/xl.cfg.5.pod.in | 52 +++++++++++++++++++++++++++++++++
tools/libxl/libxl.h | 5 ++++
tools/libxl/libxl_create.c | 9 ++++++
tools/libxl/libxl_types.idl | 7 +++++
tools/xl/xl_parse.c | 38 ++++++++++++++++++++++++
xen/arch/arm/domain.c | 10 ++++++-
xen/arch/x86/domain.c | 2 +-
xen/common/domain.c | 7 +++++
xen/drivers/passthrough/iommu.c | 13 ++++++++-
xen/include/public/domctl.h | 6 +++-
xen/include/xen/iommu.h | 19 ++++++++----
11 files changed, 158 insertions(+), 10 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..fd35685e9e 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 (e.g. AMD), 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 03ce166f4f..f288e13dc1 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -564,6 +564,15 @@ 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));
+
+ if (info->passthrough != LIBXL_PASSTHROUGH_DISABLED)
+ 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 941bbff4fe..b12de6ff3d 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 )
{
@@ -674,7 +682,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 ( (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 f144d8fe9a..4f7dad49c5 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 ( (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 e832a5c4aa..142b08174b 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 b24be5ffa6..a526aa6c09 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..922ed50a11 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
uint32_t flags;
+#define _XEN_DOMCTL_IOMMU_no_sharept 0
+#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
+ uint32_t iommu_opts;
+
/*
* Various domain limits, which impact the quantity of resources (global
* mapping space, xenheap, etc) a guest may consume.
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 5e7ca98170..01025e372b 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);
@@ -257,9 +257,17 @@ struct domain_iommu {
DECLARE_BITMAP(features, IOMMU_FEAT_count);
/*
- * 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).
+ * 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 require mappings to be synchronized, to maintain
+ * 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 */
#ifdef CONFIG_HAS_PASSTHROUGH
--
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] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
@ 2019-08-23 14:16 ` Roger Pau Monné
2019-08-29 15:25 ` Paul Durrant
2019-08-29 14:07 ` Jan Beulich
1 sibling, 1 reply; 35+ messages in thread
From: Roger Pau Monné @ 2019-08-23 14:16 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Anthony PERARD, xen-devel,
Volodymyr Babchuk
On Fri, Aug 16, 2019 at 06:20:01PM +0100, Paul Durrant wrote:
> ...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.
>
> 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>
>
> Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
>
> v6:
> - Remove the libxl_physinfo() call since it's usefulness is limited to x86
>
> v5:
> - Expand xen_domctl_createdomain flags field and hence bump interface
> version
> - Fix spelling mistakes in context line
> ---
> docs/man/xl.cfg.5.pod.in | 52 +++++++++++++++++++++++++++++++++
> tools/libxl/libxl.h | 5 ++++
> tools/libxl/libxl_create.c | 9 ++++++
> tools/libxl/libxl_types.idl | 7 +++++
> tools/xl/xl_parse.c | 38 ++++++++++++++++++++++++
> xen/arch/arm/domain.c | 10 ++++++-
> xen/arch/x86/domain.c | 2 +-
> xen/common/domain.c | 7 +++++
> xen/drivers/passthrough/iommu.c | 13 ++++++++-
> xen/include/public/domctl.h | 6 +++-
> xen/include/xen/iommu.h | 19 ++++++++----
> 11 files changed, 158 insertions(+), 10 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index c99d40307e..fd35685e9e 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>
I would maybe name this exclusive_pt, but historically it's been named
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 (e.g. AMD), 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 03ce166f4f..f288e13dc1 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -564,6 +564,15 @@ 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));
> +
> + if (info->passthrough != LIBXL_PASSTHROUGH_DISABLED)
> + 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);
> + }
Don't you need a break here?
> + 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);
> + }
And here likely (or a /* fallthrough */ comment)
> + 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 941bbff4fe..b12de6ff3d 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 )
> {
> @@ -674,7 +682,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 ( (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 f144d8fe9a..4f7dad49c5 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 ( (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 e832a5c4aa..142b08174b 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 b24be5ffa6..a526aa6c09 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..922ed50a11 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
> #include "hvm/save.h"
> #include "memory.h"
>
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
>
> /*
> * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
>
> uint32_t flags;
>
> +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> + uint32_t iommu_opts;
> +
> /*
> * Various domain limits, which impact the quantity of resources (global
> * mapping space, xenheap, etc) a guest may consume.
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 5e7ca98170..01025e372b 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);
>
> @@ -257,9 +257,17 @@ struct domain_iommu {
> DECLARE_BITMAP(features, IOMMU_FEAT_count);
>
> /*
> - * 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).
> + * 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 require mappings to be synchronized, to maintain
> + * 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;
I'm lost here, doesn't hap_pt_share = !need_sync?
ie: sync is required because the page-tables are not shared?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-23 14:16 ` Roger Pau Monné
@ 2019-08-29 15:25 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 15:25 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Tim (Xen.org),
George Dunlap, Julien Grall, Jan Beulich, Ian Jackson,
Anthony Perard, xen-devel, Volodymyr Babchuk
> -----Original Message-----
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: 23 August 2019 15:17
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu <wl@xen.org>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Julien Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>;
> Anthony Perard <anthony.perard@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
>
> On Fri, Aug 16, 2019 at 06:20:01PM +0100, Paul Durrant wrote:
> > ...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.
> >
> > 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>
> >
> > Previously part of series https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02267.html
> >
> > v6:
> > - Remove the libxl_physinfo() call since it's usefulness is limited to x86
> >
> > v5:
> > - Expand xen_domctl_createdomain flags field and hence bump interface
> > version
> > - Fix spelling mistakes in context line
> > ---
> > docs/man/xl.cfg.5.pod.in | 52 +++++++++++++++++++++++++++++++++
> > tools/libxl/libxl.h | 5 ++++
> > tools/libxl/libxl_create.c | 9 ++++++
> > tools/libxl/libxl_types.idl | 7 +++++
> > tools/xl/xl_parse.c | 38 ++++++++++++++++++++++++
> > xen/arch/arm/domain.c | 10 ++++++-
> > xen/arch/x86/domain.c | 2 +-
> > xen/common/domain.c | 7 +++++
> > xen/drivers/passthrough/iommu.c | 13 ++++++++-
> > xen/include/public/domctl.h | 6 +++-
> > xen/include/xen/iommu.h | 19 ++++++++----
> > 11 files changed, 158 insertions(+), 10 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index c99d40307e..fd35685e9e 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>
>
> I would maybe name this exclusive_pt, but historically it's been named
> sync_pt.
>
Yes, I prefer to stick with the incumbent name.
[snip]
> > 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);
> > + }
>
> Don't you need a break here?
>
> > + 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);
> > + }
>
> And here likely (or a /* fallthrough */ comment)
>
Nope. Missing breaks in both cases. Good spot.
> > + 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 941bbff4fe..b12de6ff3d 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 )
> > {
> > @@ -674,7 +682,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 ( (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 f144d8fe9a..4f7dad49c5 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 ( (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 e832a5c4aa..142b08174b 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 b24be5ffa6..a526aa6c09 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..922ed50a11 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -38,7 +38,7 @@
> > #include "hvm/save.h"
> > #include "memory.h"
> >
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
> >
> > /*
> > * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> > @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
> >
> > uint32_t flags;
> >
> > +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> > +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> > + uint32_t iommu_opts;
> > +
> > /*
> > * Various domain limits, which impact the quantity of resources (global
> > * mapping space, xenheap, etc) a guest may consume.
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 5e7ca98170..01025e372b 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);
> >
> > @@ -257,9 +257,17 @@ struct domain_iommu {
> > DECLARE_BITMAP(features, IOMMU_FEAT_count);
> >
> > /*
> > - * 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).
> > + * 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 require mappings to be synchronized, to maintain
> > + * 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;
>
> I'm lost here, doesn't hap_pt_share = !need_sync?
>
> ie: sync is required because the page-tables are not shared?
>
I thought the comment was clear, but maybe not... Consider the 'inclusive' PV h/w domain mappings. No sharing (because it's a PV domain) and no sync, because all RAM is mapped anyway. Can I phrase it better?
Paul
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-16 17:20 ` [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg Paul Durrant
2019-08-23 14:16 ` Roger Pau Monné
@ 2019-08-29 14:07 ` Jan Beulich
2019-08-29 15:27 ` Paul Durrant
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2019-08-29 14:07 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 16.08.2019 19:20, Paul Durrant wrote:
> ...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.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a question/suggestion and a nit:
> --- 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;
> + }
Seeing this I wonder whether XEN_DOMCTL_CDF_iommu wouldn't better be
bit 0 of iommu_opts.
> @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
>
> uint32_t flags;
>
> +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
Please can you add the missing blanks around << ?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
2019-08-29 14:07 ` Jan Beulich
@ 2019-08-29 15:27 ` Paul Durrant
0 siblings, 0 replies; 35+ messages in thread
From: Paul Durrant @ 2019-08-29 15:27 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: 29 August 2019 15:07
> 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 v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
>
> On 16.08.2019 19:20, Paul Durrant wrote:
> > ...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.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> with a question/suggestion and a nit:
>
> > --- 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;
> > + }
>
> Seeing this I wonder whether XEN_DOMCTL_CDF_iommu wouldn't better be
> bit 0 of iommu_opts.
>
I debated this with myself and I prefer to stick with separate flag and options.
> > @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
> >
> > uint32_t flags;
> >
> > +#define _XEN_DOMCTL_IOMMU_no_sharept 0
> > +#define XEN_DOMCTL_IOMMU_no_sharept (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
>
> Please can you add the missing blanks around << ?
>
Sure.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread