* [PATCH v5 1/8] x86/iommu: convert VT-d code to use new page table allocator
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-07 7:40 ` [PATCH v5 2/8] iommu: remove unused iommu_ops method and tasklet Paul Durrant
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich, Kevin Tian
From: Paul Durrant <pdurrant@amazon.com>
This patch converts the VT-d code to use the new IOMMU page table allocator
function. This allows all the free-ing code to be removed (since it is now
handled by the general x86 code) which reduces TLB and cache thrashing as well
as shortening the code.
The scope of the mapping_lock in intel_iommu_quarantine_init() has also been
increased slightly; it should have always covered accesses to
'arch.vtd.pgd_maddr'.
NOTE: The common IOMMU needs a slight modification to avoid scheduling the
cleanup tasklet if the free_page_table() method is not present (since
the tasklet will unconditionally call it).
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Kevin Tian <kevin.tian@intel.com>
v2:
- New in v2 (split from "add common page-table allocator")
---
xen/drivers/passthrough/iommu.c | 6 +-
xen/drivers/passthrough/vtd/iommu.c | 101 ++++++++++------------------
2 files changed, 39 insertions(+), 68 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 1d644844ab..2b1db8022c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -225,8 +225,10 @@ static void iommu_teardown(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- hd->platform_ops->teardown(d);
- tasklet_schedule(&iommu_pt_cleanup_tasklet);
+ iommu_vcall(hd->platform_ops, teardown, d);
+
+ if ( hd->platform_ops->free_page_table )
+ tasklet_schedule(&iommu_pt_cleanup_tasklet);
}
void iommu_domain_destroy(struct domain *d)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 94e0455a4d..607e8b5e65 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -265,10 +265,15 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
addr &= (((u64)1) << addr_width) - 1;
ASSERT(spin_is_locked(&hd->arch.mapping_lock));
- if ( !hd->arch.vtd.pgd_maddr &&
- (!alloc ||
- ((hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node)) == 0)) )
- goto out;
+ if ( !hd->arch.vtd.pgd_maddr )
+ {
+ struct page_info *pg;
+
+ if ( !alloc || !(pg = iommu_alloc_pgtable(domain)) )
+ goto out;
+
+ hd->arch.vtd.pgd_maddr = page_to_maddr(pg);
+ }
parent = (struct dma_pte *)map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
while ( level > 1 )
@@ -279,13 +284,16 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
pte_maddr = dma_pte_addr(*pte);
if ( !pte_maddr )
{
+ struct page_info *pg;
+
if ( !alloc )
break;
- pte_maddr = alloc_pgtable_maddr(1, hd->node);
- if ( !pte_maddr )
+ pg = iommu_alloc_pgtable(domain);
+ if ( !pg )
break;
+ pte_maddr = page_to_maddr(pg);
dma_set_pte_addr(*pte, pte_maddr);
/*
@@ -675,45 +683,6 @@ static void dma_pte_clear_one(struct domain *domain, uint64_t addr,
unmap_vtd_domain_page(page);
}
-static void iommu_free_pagetable(u64 pt_maddr, int level)
-{
- struct page_info *pg = maddr_to_page(pt_maddr);
-
- if ( pt_maddr == 0 )
- return;
-
- PFN_ORDER(pg) = level;
- spin_lock(&iommu_pt_cleanup_lock);
- page_list_add_tail(pg, &iommu_pt_cleanup_list);
- spin_unlock(&iommu_pt_cleanup_lock);
-}
-
-static void iommu_free_page_table(struct page_info *pg)
-{
- unsigned int i, next_level = PFN_ORDER(pg) - 1;
- u64 pt_maddr = page_to_maddr(pg);
- struct dma_pte *pt_vaddr, *pte;
-
- PFN_ORDER(pg) = 0;
- pt_vaddr = (struct dma_pte *)map_vtd_domain_page(pt_maddr);
-
- for ( i = 0; i < PTE_NUM; i++ )
- {
- pte = &pt_vaddr[i];
- if ( !dma_pte_present(*pte) )
- continue;
-
- if ( next_level >= 1 )
- iommu_free_pagetable(dma_pte_addr(*pte), next_level);
-
- dma_clear_pte(*pte);
- iommu_sync_cache(pte, sizeof(struct dma_pte));
- }
-
- unmap_vtd_domain_page(pt_vaddr);
- free_pgtable_maddr(pt_maddr);
-}
-
static int iommu_set_root_entry(struct vtd_iommu *iommu)
{
u32 sts;
@@ -1748,16 +1717,7 @@ static void iommu_domain_teardown(struct domain *d)
xfree(mrmrr);
}
- ASSERT(is_iommu_enabled(d));
-
- if ( iommu_use_hap_pt(d) )
- return;
-
- spin_lock(&hd->arch.mapping_lock);
- iommu_free_pagetable(hd->arch.vtd.pgd_maddr,
- agaw_to_level(hd->arch.vtd.agaw));
hd->arch.vtd.pgd_maddr = 0;
- spin_unlock(&hd->arch.mapping_lock);
}
static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn,
@@ -2669,23 +2629,28 @@ static void vtd_dump_p2m_table(struct domain *d)
static int __init intel_iommu_quarantine_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
+ struct page_info *pg;
struct dma_pte *parent;
unsigned int agaw = width_to_agaw(DEFAULT_DOMAIN_ADDRESS_WIDTH);
unsigned int level = agaw_to_level(agaw);
- int rc;
+ int rc = 0;
+
+ spin_lock(&hd->arch.mapping_lock);
if ( hd->arch.vtd.pgd_maddr )
{
ASSERT_UNREACHABLE();
- return 0;
+ goto out;
}
- spin_lock(&hd->arch.mapping_lock);
+ pg = iommu_alloc_pgtable(d);
- hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
- if ( !hd->arch.vtd.pgd_maddr )
+ rc = -ENOMEM;
+ if ( !pg )
goto out;
+ hd->arch.vtd.pgd_maddr = page_to_maddr(pg);
+
parent = map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
while ( level )
{
@@ -2697,10 +2662,12 @@ static int __init intel_iommu_quarantine_init(struct domain *d)
* page table pages, and the resulting allocations are always
* zeroed.
*/
- maddr = alloc_pgtable_maddr(1, hd->node);
- if ( !maddr )
- break;
+ pg = iommu_alloc_pgtable(d);
+
+ if ( !pg )
+ goto out;
+ maddr = page_to_maddr(pg);
for ( offset = 0; offset < PTE_NUM; offset++ )
{
struct dma_pte *pte = &parent[offset];
@@ -2716,13 +2683,16 @@ static int __init intel_iommu_quarantine_init(struct domain *d)
}
unmap_vtd_domain_page(parent);
+ rc = 0;
+
out:
spin_unlock(&hd->arch.mapping_lock);
- rc = iommu_flush_iotlb_all(d);
+ if ( !rc )
+ rc = iommu_flush_iotlb_all(d);
- /* Pages leaked in failure case */
- return level ? -ENOMEM : rc;
+ /* Pages may be leaked in failure case */
+ return rc;
}
static struct iommu_ops __initdata vtd_ops = {
@@ -2737,7 +2707,6 @@ static struct iommu_ops __initdata vtd_ops = {
.map_page = intel_iommu_map_page,
.unmap_page = intel_iommu_unmap_page,
.lookup_page = intel_iommu_lookup_page,
- .free_page_table = iommu_free_page_table,
.reassign_device = reassign_device_ownership,
.get_device_group_id = intel_iommu_group_id,
.enable_x2apic = intel_iommu_enable_eim,
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 2/8] iommu: remove unused iommu_ops method and tasklet
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
2020-09-07 7:40 ` [PATCH v5 1/8] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-07 7:40 ` [PATCH v5 3/8] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
The VT-d and AMD IOMMU implementations both use the general x86 IOMMU page
table allocator and ARM always shares page tables with CPU. Hence there is no
need to retain the free_page_table() method or the tasklet which invokes it.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v2:
- New in v2 (split from "add common page-table allocator")
---
xen/drivers/passthrough/iommu.c | 25 -------------------------
xen/include/xen/iommu.h | 2 --
2 files changed, 27 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 2b1db8022c..660dc5deb2 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -49,10 +49,6 @@ bool_t __read_mostly amd_iommu_perdev_intremap = 1;
DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
-DEFINE_SPINLOCK(iommu_pt_cleanup_lock);
-PAGE_LIST_HEAD(iommu_pt_cleanup_list);
-static struct tasklet iommu_pt_cleanup_tasklet;
-
static int __init parse_iommu_param(const char *s)
{
const char *ss;
@@ -226,9 +222,6 @@ static void iommu_teardown(struct domain *d)
struct domain_iommu *hd = dom_iommu(d);
iommu_vcall(hd->platform_ops, teardown, d);
-
- if ( hd->platform_ops->free_page_table )
- tasklet_schedule(&iommu_pt_cleanup_tasklet);
}
void iommu_domain_destroy(struct domain *d)
@@ -368,23 +361,6 @@ int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
return iommu_call(hd->platform_ops, lookup_page, d, dfn, mfn, flags);
}
-static void iommu_free_pagetables(void *unused)
-{
- do {
- struct page_info *pg;
-
- spin_lock(&iommu_pt_cleanup_lock);
- pg = page_list_remove_head(&iommu_pt_cleanup_list);
- spin_unlock(&iommu_pt_cleanup_lock);
- if ( !pg )
- return;
- iommu_vcall(iommu_get_ops(), free_page_table, pg);
- } while ( !softirq_pending(smp_processor_id()) );
-
- tasklet_schedule_on_cpu(&iommu_pt_cleanup_tasklet,
- cpumask_cycle(smp_processor_id(), &cpu_online_map));
-}
-
int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_count,
unsigned int flush_flags)
{
@@ -508,7 +484,6 @@ int __init iommu_setup(void)
#ifndef iommu_intremap
printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis");
#endif
- tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, NULL);
}
return rc;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3272874958..1831dc66b0 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -263,8 +263,6 @@ struct iommu_ops {
int __must_check (*lookup_page)(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags);
- void (*free_page_table)(struct page_info *);
-
#ifdef CONFIG_X86
int (*enable_x2apic)(void);
void (*disable_x2apic)(void);
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 3/8] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
2020-09-07 7:40 ` [PATCH v5 1/8] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
2020-09-07 7:40 ` [PATCH v5 2/8] iommu: remove unused iommu_ops method and tasklet Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-09 13:16 ` Jan Beulich
2020-09-07 7:40 ` [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush Paul Durrant
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
This patch adds a full I/O TLB flush to the error paths of iommu_map() and
iommu_unmap().
Without this change callers need constructs such as:
rc = iommu_map/unmap(...)
err = iommu_flush(...)
if ( !rc )
rc = err;
With this change, it can be simplified to:
rc = iommu_map/unmap(...)
if ( !rc )
rc = iommu_flush(...)
because, if the map or unmap fails the flush will be unnecessary. This saves
a stack variable and generally makes the call sites tidier.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
v5:
- Avoid unnecessary flushing if 'page_order' is 0
- Add missing indirection on 'flush_flags'
v2:
- New in v2
---
xen/drivers/passthrough/iommu.c | 34 +++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 660dc5deb2..f27facd52d 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -274,6 +274,13 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
break;
}
+ /*
+ * Something went wrong so, if we were dealing with more than a single
+ * page, flush everything and clear flush flags.
+ */
+ if ( page_order && unlikely(rc) && !iommu_iotlb_flush_all(d, *flush_flags) )
+ *flush_flags = 0;
+
return rc;
}
@@ -283,14 +290,8 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned int flush_flags = 0;
int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags);
- if ( !this_cpu(iommu_dont_flush_iotlb) )
- {
- int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
- flush_flags);
-
- if ( !rc )
- rc = err;
- }
+ if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
+ rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
return rc;
}
@@ -330,6 +331,13 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
}
}
+ /*
+ * Something went wrong so, if we were dealing with more than a single
+ * page, flush everything and clear flush flags.
+ */
+ if ( page_order && unlikely(rc) && !iommu_iotlb_flush_all(d, *flush_flags) )
+ *flush_flags = 0;
+
return rc;
}
@@ -338,14 +346,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_order)
unsigned int flush_flags = 0;
int rc = iommu_unmap(d, dfn, page_order, &flush_flags);
- if ( !this_cpu(iommu_dont_flush_iotlb) )
- {
- int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
- flush_flags);
-
- if ( !rc )
- rc = err;
- }
+ if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
+ rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
return rc;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 3/8] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail
2020-09-07 7:40 ` [PATCH v5 3/8] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
@ 2020-09-09 13:16 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-09-09 13:16 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant
On 07.09.2020 09:40, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> This patch adds a full I/O TLB flush to the error paths of iommu_map() and
> iommu_unmap().
>
> Without this change callers need constructs such as:
>
> rc = iommu_map/unmap(...)
> err = iommu_flush(...)
> if ( !rc )
> rc = err;
>
> With this change, it can be simplified to:
>
> rc = iommu_map/unmap(...)
> if ( !rc )
> rc = iommu_flush(...)
>
> because, if the map or unmap fails the flush will be unnecessary. This saves
> a stack variable and generally makes the call sites tidier.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one cosmetic issue taken care of (perhaps while committing):
> @@ -338,14 +346,8 @@ int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_order)
> unsigned int flush_flags = 0;
> int rc = iommu_unmap(d, dfn, page_order, &flush_flags);
>
> - if ( !this_cpu(iommu_dont_flush_iotlb) )
> - {
> - int err = iommu_iotlb_flush(d, dfn, (1u << page_order),
> - flush_flags);
> -
> - if ( !rc )
> - rc = err;
> - }
> + if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
There's a stray blank after the latter ! here.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
` (2 preceding siblings ...)
2020-09-07 7:40 ` [PATCH v5 3/8] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-10 12:47 ` Jan Beulich
2020-09-07 7:40 ` [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Andrew Cooper, Wei Liu,
Roger Pau Monné, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Jun Nakajima, Kevin Tian
From: Paul Durrant <pdurrant@amazon.com>
At the moment iommu_map() and iommu_unmap() take a page order rather than a
count, whereas iommu_iotlb_flush() takes a page count rather than an order.
This patch makes them consistent with each other, opting for a page count since
CPU page orders are not necessarily the same as those of an IOMMU.
NOTE: The 'page_count' parameter is also made an unsigned long in all the
aforementioned functions.
Signed-off-by: Paul Durrant <pdurrant@amazon.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@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
v5:
- Re-worked to just use a page count, rather than both a count and an order
v2:
- New in v2
---
xen/arch/x86/mm.c | 6 +++--
xen/arch/x86/mm/p2m-ept.c | 6 ++---
xen/arch/x86/mm/p2m-pt.c | 4 +--
xen/arch/x86/mm/p2m.c | 5 ++--
xen/arch/x86/x86_64/mm.c | 4 +--
xen/common/grant_table.c | 6 ++---
xen/drivers/passthrough/amd/iommu.h | 2 +-
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/iommu.c | 35 +++++++++++--------------
xen/drivers/passthrough/vtd/iommu.c | 4 +--
xen/drivers/passthrough/x86/iommu.c | 2 +-
xen/include/xen/iommu.h | 12 ++++-----
12 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 35ec0e11f6..e133ccf0d8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2966,9 +2966,11 @@ static int _get_page_type(struct page_info *page, unsigned long type,
mfn_t mfn = page_to_mfn(page);
if ( (x & PGT_type_mask) == PGT_writable_page )
- rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
+ rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
+ 1ul << PAGE_ORDER_4K);
else
- rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
+ rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
+ 1ul << PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable);
if ( unlikely(rc) )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b8154a7ecc..12cf38f6eb 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -843,14 +843,14 @@ out:
need_modify_vtd_table )
{
if ( iommu_use_hap_pt(d) )
- rc = iommu_iotlb_flush(d, _dfn(gfn), (1u << order),
+ rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order,
(iommu_flags ? IOMMU_FLUSHF_added : 0) |
(vtd_pte_present ? IOMMU_FLUSHF_modified
: 0));
else if ( need_iommu_pt_sync(d) )
rc = iommu_flags ?
- iommu_legacy_map(d, _dfn(gfn), mfn, order, iommu_flags) :
- iommu_legacy_unmap(d, _dfn(gfn), order);
+ iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << order, iommu_flags) :
+ iommu_legacy_unmap(d, _dfn(gfn), 1ul << order);
}
unmap_domain_page(table);
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index badb26bc34..3af51be78e 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -679,9 +679,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
if ( need_iommu_pt_sync(p2m->domain) &&
(iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
rc = iommu_pte_flags
- ? iommu_legacy_map(d, _dfn(gfn), mfn, page_order,
+ ? iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << page_order,
iommu_pte_flags)
- : iommu_legacy_unmap(d, _dfn(gfn), page_order);
+ : iommu_legacy_unmap(d, _dfn(gfn), 1ul << page_order);
/*
* Free old intermediate tables if necessary. This has to be the
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index db7bde0230..928344be30 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1352,7 +1352,8 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
{
if ( !is_iommu_enabled(d) )
return 0;
- return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
+ return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l),
+ 1ul << PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable);
}
@@ -1443,7 +1444,7 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
{
if ( !is_iommu_enabled(d) )
return 0;
- return iommu_legacy_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K);
+ return iommu_legacy_unmap(d, _dfn(gfn_l), 1ul << PAGE_ORDER_4K);
}
gfn_lock(p2m, gfn, 0);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 28bc98f8f2..6f2c43181a 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1480,7 +1480,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
{
for ( i = spfn; i < epfn; i++ )
if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i),
- PAGE_ORDER_4K,
+ 1ul << PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable) )
break;
if ( i != epfn )
@@ -1488,7 +1488,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
while (i-- > old_max)
/* If statement to satisfy __must_check. */
if ( iommu_legacy_unmap(hardware_domain, _dfn(i),
- PAGE_ORDER_4K) )
+ 1ul << PAGE_ORDER_4K) )
continue;
goto destroy_m2p;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 9f0cae52c0..c5a1ef5fc3 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1225,7 +1225,7 @@ map_grant_ref(
kind = IOMMUF_readable;
else
kind = 0;
- if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind) )
+ if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1ul, kind) )
{
double_gt_unlock(lgt, rgt);
rc = GNTST_general_error;
@@ -1479,9 +1479,9 @@ unmap_common(
kind = mapkind(lgt, rd, op->mfn);
if ( !kind )
- err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 0);
+ err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 1ul);
else if ( !(kind & MAPKIND_WRITE) )
- err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 0,
+ err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 1ul,
IOMMUF_readable);
double_gt_unlock(lgt, rgt);
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index e2d174f3b4..f1f0415469 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -231,7 +231,7 @@ int amd_iommu_reserve_domain_unity_map(struct domain *domain,
paddr_t phys_addr, unsigned long size,
int iw, int ir);
int __must_check amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags);
int __must_check amd_iommu_flush_iotlb_all(struct domain *d);
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 54b991294a..ee6eb49bef 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -362,7 +362,7 @@ static unsigned long flush_count(unsigned long dfn, unsigned int page_count,
}
int amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags)
{
unsigned long dfn_l = dfn_x(dfn);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index f27facd52d..e664f4ac79 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -235,7 +235,7 @@ void iommu_domain_destroy(struct domain *d)
}
int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags,
+ unsigned long page_count, unsigned int flags,
unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
@@ -245,10 +245,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
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)));
-
- for ( i = 0; i < (1ul << page_order); i++ )
+ for ( i = 0; i < page_count; i++ )
{
rc = iommu_call(hd->platform_ops, map_page, d, dfn_add(dfn, i),
mfn_add(mfn, i), flags, flush_flags);
@@ -278,25 +275,26 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
* Something went wrong so, if we were dealing with more than a single
* page, flush everything and clear flush flags.
*/
- if ( page_order && unlikely(rc) && !iommu_iotlb_flush_all(d, *flush_flags) )
+ if ( page_count > 1 && unlikely(rc) &&
+ !iommu_iotlb_flush_all(d, *flush_flags) )
*flush_flags = 0;
return rc;
}
int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags)
+ unsigned long page_count, unsigned int flags)
{
unsigned int flush_flags = 0;
- int rc = iommu_map(d, dfn, mfn, page_order, flags, &flush_flags);
+ int rc = iommu_map(d, dfn, mfn, page_count, flags, &flush_flags);
if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
- rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
+ rc = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
return rc;
}
-int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
+int iommu_unmap(struct domain *d, dfn_t dfn, unsigned long page_count,
unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
@@ -306,9 +304,7 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
if ( !is_iommu_enabled(d) )
return 0;
- ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
-
- for ( i = 0; i < (1ul << page_order); i++ )
+ for ( i = 0; i < page_count; i++ )
{
int err = iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
flush_flags);
@@ -335,19 +331,20 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
* Something went wrong so, if we were dealing with more than a single
* page, flush everything and clear flush flags.
*/
- if ( page_order && unlikely(rc) && !iommu_iotlb_flush_all(d, *flush_flags) )
+ if ( page_count > 1 && unlikely(rc) &&
+ !iommu_iotlb_flush_all(d, *flush_flags) )
*flush_flags = 0;
return rc;
}
-int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned int page_order)
+int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned long page_count)
{
unsigned int flush_flags = 0;
- int rc = iommu_unmap(d, dfn, page_order, &flush_flags);
+ int rc = iommu_unmap(d, dfn, page_count, &flush_flags);
if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
- rc = iommu_iotlb_flush(d, dfn, (1u << page_order), flush_flags);
+ rc = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
return rc;
}
@@ -363,7 +360,7 @@ int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
return iommu_call(hd->platform_ops, lookup_page, d, dfn, mfn, flags);
}
-int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_count,
+int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned long page_count,
unsigned int flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
@@ -382,7 +379,7 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_count,
{
if ( !d->is_shutting_down && printk_ratelimit() )
printk(XENLOG_ERR
- "d%d: IOMMU IOTLB flush failed: %d, dfn %"PRI_dfn", page count %u flags %x\n",
+ "d%d: IOMMU IOTLB flush failed: %d, dfn %"PRI_dfn", page count %lu flags %x\n",
d->domain_id, rc, dfn_x(dfn), page_count, flush_flags);
if ( !is_hardware_domain(d) )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 607e8b5e65..68cf0e535a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -584,7 +584,7 @@ static int __must_check iommu_flush_all(void)
static int __must_check iommu_flush_iotlb(struct domain *d, dfn_t dfn,
bool_t dma_old_pte_present,
- unsigned int page_count)
+ unsigned long page_count)
{
struct domain_iommu *hd = dom_iommu(d);
struct acpi_drhd_unit *drhd;
@@ -632,7 +632,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d, dfn_t dfn,
static int __must_check iommu_flush_iotlb_pages(struct domain *d,
dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags)
{
ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index aea07e47c4..f17b1820f4 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -244,7 +244,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
else if ( paging_mode_translate(d) )
rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
else
- rc = iommu_map(d, _dfn(pfn), _mfn(pfn), PAGE_ORDER_4K,
+ rc = iommu_map(d, _dfn(pfn), _mfn(pfn), 1ul << PAGE_ORDER_4K,
IOMMUF_readable | IOMMUF_writable, &flush_flags);
if ( rc )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 1831dc66b0..13f68dc93d 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -146,23 +146,23 @@ enum
#define IOMMU_FLUSHF_modified (1u << _IOMMU_FLUSHF_modified)
int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order, unsigned int flags,
+ unsigned long page_count, unsigned int flags,
unsigned int *flush_flags);
int __must_check iommu_unmap(struct domain *d, dfn_t dfn,
- unsigned int page_order,
+ unsigned long page_count,
unsigned int *flush_flags);
int __must_check iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned int page_order,
+ unsigned long page_count,
unsigned int flags);
int __must_check iommu_legacy_unmap(struct domain *d, dfn_t dfn,
- unsigned int page_order);
+ unsigned long page_count);
int __must_check iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags);
int __must_check iommu_iotlb_flush(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags);
int __must_check iommu_iotlb_flush_all(struct domain *d,
unsigned int flush_flags);
@@ -281,7 +281,7 @@ struct iommu_ops {
void (*share_p2m)(struct domain *d);
void (*crash_shutdown)(void);
int __must_check (*iotlb_flush)(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags);
int __must_check (*iotlb_flush_all)(struct domain *d);
int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush
2020-09-07 7:40 ` [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush Paul Durrant
@ 2020-09-10 12:47 ` Jan Beulich
2020-09-10 13:51 ` Paul Durrant
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 12:47 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, Wei Liu,
Roger Pau Monné, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Jun Nakajima, Kevin Tian
On 07.09.2020 09:40, Paul Durrant wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2966,9 +2966,11 @@ static int _get_page_type(struct page_info *page, unsigned long type,
> mfn_t mfn = page_to_mfn(page);
>
> if ( (x & PGT_type_mask) == PGT_writable_page )
> - rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
> + rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
> + 1ul << PAGE_ORDER_4K);
> else
> - rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
> + rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
> + 1ul << PAGE_ORDER_4K,
> IOMMUF_readable | IOMMUF_writable);
>
> if ( unlikely(rc) )
A few hundred lines up from here there is
int rc2 = iommu_legacy_unmap(d, _dfn(mfn), PAGE_ORDER_4K);
in cleanup_page_mappings().
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1225,7 +1225,7 @@ map_grant_ref(
> kind = IOMMUF_readable;
> else
> kind = 0;
> - if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind) )
> + if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1ul, kind) )
> {
> double_gt_unlock(lgt, rgt);
> rc = GNTST_general_error;
> @@ -1479,9 +1479,9 @@ unmap_common(
>
> kind = mapkind(lgt, rd, op->mfn);
> if ( !kind )
> - err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 0);
> + err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 1ul);
> else if ( !(kind & MAPKIND_WRITE) )
> - err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 0,
> + err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 1ul,
For all three of these, I guess either 1ul << PAGE_ORDER_4K or simply 1?
(Given that the code didn't use PAGE_ORDER_4K so far, I'd slightly
prefer the latter. I'd be fine making the change while committing, but
it looks like v6 is going to be needed anyway.)
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -362,7 +362,7 @@ static unsigned long flush_count(unsigned long dfn, unsigned int page_count,
> }
>
> int amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
> - unsigned int page_count,
> + unsigned long page_count,
This ought to be accompanied by a similar change to its flush_count()
helper.
> @@ -632,7 +632,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d, dfn_t dfn,
>
> static int __must_check iommu_flush_iotlb_pages(struct domain *d,
> dfn_t dfn,
> - unsigned int page_count,
> + unsigned long page_count,
> unsigned int flush_flags)
> {
> ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
This similarly ought to be accompanied by a change to its
iommu_flush_iotlb() helper.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush
2020-09-10 12:47 ` Jan Beulich
@ 2020-09-10 13:51 ` Paul Durrant
0 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-10 13:51 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'Wei Liu', 'Roger Pau Monné',
'George Dunlap', 'Ian Jackson',
'Julien Grall', 'Stefano Stabellini',
'Jun Nakajima', 'Kevin Tian'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 10 September 2020 13:48
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall
> <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush
>
> On 07.09.2020 09:40, Paul Durrant wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2966,9 +2966,11 @@ static int _get_page_type(struct page_info *page, unsigned long type,
> > mfn_t mfn = page_to_mfn(page);
> >
> > if ( (x & PGT_type_mask) == PGT_writable_page )
> > - rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)), PAGE_ORDER_4K);
> > + rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
> > + 1ul << PAGE_ORDER_4K);
> > else
> > - rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
> > + rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
> > + 1ul << PAGE_ORDER_4K,
> > IOMMUF_readable | IOMMUF_writable);
> >
> > if ( unlikely(rc) )
>
> A few hundred lines up from here there is
>
> int rc2 = iommu_legacy_unmap(d, _dfn(mfn), PAGE_ORDER_4K);
>
> in cleanup_page_mappings().
Oh yes. Fixed.
>
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1225,7 +1225,7 @@ map_grant_ref(
> > kind = IOMMUF_readable;
> > else
> > kind = 0;
> > - if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, kind) )
> > + if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1ul, kind) )
> > {
> > double_gt_unlock(lgt, rgt);
> > rc = GNTST_general_error;
> > @@ -1479,9 +1479,9 @@ unmap_common(
> >
> > kind = mapkind(lgt, rd, op->mfn);
> > if ( !kind )
> > - err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 0);
> > + err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 1ul);
> > else if ( !(kind & MAPKIND_WRITE) )
> > - err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 0,
> > + err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 1ul,
>
> For all three of these, I guess either 1ul << PAGE_ORDER_4K or simply 1?
> (Given that the code didn't use PAGE_ORDER_4K so far, I'd slightly
> prefer the latter. I'd be fine making the change while committing, but
> it looks like v6 is going to be needed anyway.)
Ok. Changed.
>
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -362,7 +362,7 @@ static unsigned long flush_count(unsigned long dfn, unsigned int page_count,
> > }
> >
> > int amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
> > - unsigned int page_count,
> > + unsigned long page_count,
>
> This ought to be accompanied by a similar change to its flush_count()
> helper.
>
> > @@ -632,7 +632,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d, dfn_t dfn,
>
> > static int __must_check iommu_flush_iotlb_pages(struct domain *d,
> > dfn_t dfn,
> > - unsigned int page_count,
> > + unsigned long page_count,
> > unsigned int flush_flags)
> > {
> > ASSERT(page_count && !dfn_eq(dfn, INVALID_DFN));
>
> This similarly ought to be accompanied by a change to its
> iommu_flush_iotlb() helper.
>
It is... in the previous hunk.
Paul
> Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
` (3 preceding siblings ...)
2020-09-07 7:40 ` [PATCH v5 4/8] iommu: make map and unmap take a page count, similar to flush Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-10 13:18 ` Jan Beulich
2020-09-07 7:40 ` [PATCH v5 6/8] common/grant_table: batch flush I/O TLB Paul Durrant
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Andrew Cooper, Wei Liu,
Roger Pau Monné, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Jun Nakajima, Kevin Tian
From: Paul Durrant <pdurrant@amazon.com>
The 'legacy' functions do implicit flushing so amend the callers to do the
appropriate flushing.
Unfortunately, because of the structure of the P2M code, we cannot remove
the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it
facilitates. It is now checked directly iommu_iotlb_flush(). This is safe
because callers of iommu_iotlb_flush() on another CPU will not be affected,
and hence a flush will be done. Also, 'iommu_dont_flush_iotlb' is now declared
as bool (rather than bool_t) and setting/clearing it are no longer pointlessly
gated on is_iommu_enabled() returning true. (Arguably it is also pointless to
gate the call to iommu_iotlb_flush() on that condition - since it is a no-op
in that case - but the if clause allows the scope of a stack variable to be
restricted).
NOTE: The code in memory_add() now sets 'ret' if iommu_map() or
iommu_iotlb_flush() fails. This seems to be have been missed before,
meaning the error path could actually return 0.
Signed-off-by: Paul Durrant <pdurrant@amazon.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@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
v5:
- Re-base
- Removed failure case on overflow of unsigned int as it is no longer
necessary
v3:
- Same as v2; elected to implement batch flushing in the grant table code as
a subsequent patch
v2:
- Shorten the diff (mainly because of a prior patch introducing automatic
flush-on-fail into iommu_map() and iommu_unmap())
---
xen/arch/x86/mm.c | 24 +++++++++++++++++-------
xen/arch/x86/mm/p2m-ept.c | 21 +++++++++++++--------
xen/arch/x86/mm/p2m-pt.c | 16 ++++++++++++----
xen/arch/x86/mm/p2m.c | 25 +++++++++++++++++--------
xen/arch/x86/x86_64/mm.c | 20 +++++++-------------
xen/common/grant_table.c | 29 ++++++++++++++++++++++-------
xen/common/memory.c | 7 +++----
xen/drivers/passthrough/iommu.c | 25 +------------------------
xen/include/xen/iommu.h | 21 +++++----------------
9 files changed, 97 insertions(+), 91 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e133ccf0d8..f9a4ccf8d5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2439,10 +2439,16 @@ static int cleanup_page_mappings(struct page_info *page)
if ( d && unlikely(need_iommu_pt_sync(d)) && is_pv_domain(d) )
{
- int rc2 = iommu_legacy_unmap(d, _dfn(mfn), PAGE_ORDER_4K);
+ unsigned int flush_flags = 0;
+ int err;
+
+ err = iommu_unmap(d, _dfn(mfn), 1ul << PAGE_ORDER_4K, &flush_flags);
+ if ( !err )
+ err = iommu_iotlb_flush(d, _dfn(mfn), 1ul << PAGE_ORDER_4K,
+ flush_flags);
if ( !rc )
- rc = rc2;
+ rc = err;
}
if ( likely(!is_special_page(page)) )
@@ -2964,14 +2970,18 @@ static int _get_page_type(struct page_info *page, unsigned long type,
if ( d && unlikely(need_iommu_pt_sync(d)) && is_pv_domain(d) )
{
mfn_t mfn = page_to_mfn(page);
+ dfn_t dfn = _dfn(mfn_x(mfn));
+ unsigned int flush_flags = 0;
if ( (x & PGT_type_mask) == PGT_writable_page )
- rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
- 1ul << PAGE_ORDER_4K);
+ rc = iommu_unmap(d, dfn, 1ul << PAGE_ORDER_4K, &flush_flags);
else
- rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
- 1ul << PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable);
+ rc = iommu_map(d, dfn, mfn, 1ul << PAGE_ORDER_4K,
+ IOMMUF_readable | IOMMUF_writable, &flush_flags);
+
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, dfn, 1ul << PAGE_ORDER_4K,
+ flush_flags);
if ( unlikely(rc) )
{
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 12cf38f6eb..3821063487 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -842,15 +842,20 @@ out:
if ( rc == 0 && p2m_is_hostp2m(p2m) &&
need_modify_vtd_table )
{
- if ( iommu_use_hap_pt(d) )
- rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order,
- (iommu_flags ? IOMMU_FLUSHF_added : 0) |
- (vtd_pte_present ? IOMMU_FLUSHF_modified
- : 0));
- else if ( need_iommu_pt_sync(d) )
+ unsigned int flush_flags = 0;
+
+ if ( need_iommu_pt_sync(d) )
rc = iommu_flags ?
- iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << order, iommu_flags) :
- iommu_legacy_unmap(d, _dfn(gfn), 1ul << order);
+ iommu_map(d, _dfn(gfn), mfn, 1ul << order, iommu_flags,
+ &flush_flags) :
+ iommu_unmap(d, _dfn(gfn), 1ul << order, &flush_flags);
+ else if ( iommu_use_hap_pt(d) )
+ flush_flags =
+ (iommu_flags ? IOMMU_FLUSHF_added : 0) |
+ (vtd_pte_present ? IOMMU_FLUSHF_modified : 0);
+
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << order, flush_flags);
}
unmap_domain_page(table);
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 3af51be78e..942d73d845 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -678,10 +678,18 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
if ( need_iommu_pt_sync(p2m->domain) &&
(iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
- rc = iommu_pte_flags
- ? iommu_legacy_map(d, _dfn(gfn), mfn, 1ul << page_order,
- iommu_pte_flags)
- : iommu_legacy_unmap(d, _dfn(gfn), 1ul << page_order);
+ {
+ unsigned int flush_flags = 0;
+
+ rc = iommu_pte_flags ?
+ iommu_map(d, _dfn(gfn), mfn, 1ul << page_order, iommu_pte_flags,
+ &flush_flags) :
+ iommu_unmap(d, _dfn(gfn), 1ul << page_order, &flush_flags);
+
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, _dfn(gfn), 1ul << page_order,
+ flush_flags);
+ }
/*
* Free old intermediate tables if necessary. This has to be the
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 928344be30..01ff92862d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1350,11 +1350,15 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn_l,
if ( !paging_mode_translate(p2m->domain) )
{
- if ( !is_iommu_enabled(d) )
- return 0;
- return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l),
- 1ul << PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable);
+ unsigned int flush_flags = 0;
+
+ ret = iommu_map(d, _dfn(gfn_l), _mfn(gfn_l), 1ul << PAGE_ORDER_4K,
+ IOMMUF_readable | IOMMUF_writable, &flush_flags);
+ if ( !ret )
+ ret = iommu_iotlb_flush(d, _dfn(gfn_l), 1ul << PAGE_ORDER_4K,
+ flush_flags);
+
+ return ret;
}
gfn_lock(p2m, gfn, 0);
@@ -1442,9 +1446,14 @@ int clear_identity_p2m_entry(struct domain *d, unsigned long gfn_l)
if ( !paging_mode_translate(d) )
{
- if ( !is_iommu_enabled(d) )
- return 0;
- return iommu_legacy_unmap(d, _dfn(gfn_l), 1ul << PAGE_ORDER_4K);
+ unsigned int flush_flags = 0;
+
+ ret = iommu_unmap(d, _dfn(gfn_l), 1ul << PAGE_ORDER_4K, &flush_flags);
+ if ( !ret )
+ ret = iommu_iotlb_flush(d, _dfn(gfn_l), 1ul << PAGE_ORDER_4K,
+ flush_flags);
+
+ return ret;
}
gfn_lock(p2m, gfn, 0);
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 6f2c43181a..13b2f973fa 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1478,21 +1478,15 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
!iommu_use_hap_pt(hardware_domain) &&
!need_iommu_pt_sync(hardware_domain) )
{
- for ( i = spfn; i < epfn; i++ )
- if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i),
- 1ul << PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable) )
- break;
- if ( i != epfn )
- {
- while (i-- > old_max)
- /* If statement to satisfy __must_check. */
- if ( iommu_legacy_unmap(hardware_domain, _dfn(i),
- 1ul << PAGE_ORDER_4K) )
- continue;
+ unsigned int flush_flags = 0;
+ unsigned long n = epfn - spfn;
+ ret = iommu_map(hardware_domain, _dfn(i), _mfn(i), n,
+ IOMMUF_readable | IOMMUF_writable, &flush_flags);
+ if ( !ret )
+ ret = iommu_iotlb_flush(hardware_domain, _dfn(i), n, flush_flags);
+ if ( ret )
goto destroy_m2p;
- }
}
/* We can't revert any more */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c5a1ef5fc3..a844a94a5b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1225,11 +1225,22 @@ map_grant_ref(
kind = IOMMUF_readable;
else
kind = 0;
- if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 1ul, kind) )
+
+ if ( kind )
{
- double_gt_unlock(lgt, rgt);
- rc = GNTST_general_error;
- goto undo_out;
+ dfn_t dfn = _dfn(mfn_x(mfn));
+ unsigned int flush_flags = 0;
+ int err;
+
+ err = iommu_map(ld, dfn, mfn, 1ul, kind, &flush_flags);
+ if ( !err )
+ err = iommu_iotlb_flush(ld, dfn, 1ul, flush_flags);
+ if ( err )
+ {
+ double_gt_unlock(lgt, rgt);
+ rc = GNTST_general_error;
+ goto undo_out;
+ }
}
}
@@ -1473,19 +1484,23 @@ unmap_common(
if ( rc == GNTST_okay && gnttab_need_iommu_mapping(ld) )
{
unsigned int kind;
+ dfn_t dfn = _dfn(mfn_x(op->mfn));
+ unsigned int flush_flags = 0;
int err = 0;
double_gt_lock(lgt, rgt);
kind = mapkind(lgt, rd, op->mfn);
if ( !kind )
- err = iommu_legacy_unmap(ld, _dfn(mfn_x(op->mfn)), 1ul);
+ err = iommu_unmap(ld, dfn, 1ul, &flush_flags);
else if ( !(kind & MAPKIND_WRITE) )
- err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 1ul,
- IOMMUF_readable);
+ err = iommu_map(ld, dfn, op->mfn, 1ul, IOMMUF_readable,
+ &flush_flags);
double_gt_unlock(lgt, rgt);
+ if ( !err )
+ err = iommu_iotlb_flush(ld, dfn, 1ul, flush_flags);
if ( err )
rc = GNTST_general_error;
}
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 714077c1e5..fedbd9019e 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -824,8 +824,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
xatp->gpfn += start;
xatp->size -= start;
- if ( is_iommu_enabled(d) )
- this_cpu(iommu_dont_flush_iotlb) = 1;
+ this_cpu(iommu_dont_flush_iotlb) = true;
while ( xatp->size > done )
{
@@ -845,12 +844,12 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
}
}
+ this_cpu(iommu_dont_flush_iotlb) = false;
+
if ( is_iommu_enabled(d) )
{
int ret;
- this_cpu(iommu_dont_flush_iotlb) = 0;
-
ret = iommu_iotlb_flush(d, _dfn(xatp->idx - done), done,
IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
if ( unlikely(ret) && rc >= 0 )
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e664f4ac79..e0d36e6bef 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -282,18 +282,6 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
return rc;
}
-int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned long page_count, unsigned int flags)
-{
- unsigned int flush_flags = 0;
- int rc = iommu_map(d, dfn, mfn, page_count, flags, &flush_flags);
-
- if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
- rc = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
-
- return rc;
-}
-
int iommu_unmap(struct domain *d, dfn_t dfn, unsigned long page_count,
unsigned int *flush_flags)
{
@@ -338,17 +326,6 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned long page_count,
return rc;
}
-int iommu_legacy_unmap(struct domain *d, dfn_t dfn, unsigned long page_count)
-{
- unsigned int flush_flags = 0;
- int rc = iommu_unmap(d, dfn, page_count, &flush_flags);
-
- if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
- rc = iommu_iotlb_flush(d, dfn, page_count, flush_flags);
-
- return rc;
-}
-
int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags)
{
@@ -367,7 +344,7 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned long page_count,
int rc;
if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush ||
- !page_count || !flush_flags )
+ !page_count || !flush_flags || this_cpu(iommu_dont_flush_iotlb) )
return 0;
if ( dfn_eq(dfn, INVALID_DFN) )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 13f68dc93d..a2eefe8582 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -151,16 +151,8 @@ int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
int __must_check iommu_unmap(struct domain *d, dfn_t dfn,
unsigned long page_count,
unsigned int *flush_flags);
-
-int __must_check iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
- unsigned long page_count,
- unsigned int flags);
-int __must_check iommu_legacy_unmap(struct domain *d, dfn_t dfn,
- unsigned long page_count);
-
int __must_check iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags);
-
int __must_check iommu_iotlb_flush(struct domain *d, dfn_t dfn,
unsigned long page_count,
unsigned int flush_flags);
@@ -369,15 +361,12 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
/*
* The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
- * avoid unecessary iotlb_flush in the low level IOMMU code.
- *
- * iommu_map_page/iommu_unmap_page must flush the iotlb but somethimes
- * this operation can be really expensive. This flag will be set by the
- * caller to notify the low level IOMMU code to avoid the iotlb flushes.
- * iommu_iotlb_flush/iommu_iotlb_flush_all will be explicitly called by
- * the caller.
+ * avoid unnecessary IOMMU flushing while updating the P2M.
+ * Setting the value to true will cause iommu_iotlb_flush() to return without
+ * actually performing a flush. A batch flush must therefore be done by the
+ * calling code after setting the value back to false.
*/
-DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
+DECLARE_PER_CPU(bool, iommu_dont_flush_iotlb);
extern struct spinlock iommu_pt_cleanup_lock;
extern struct page_list_head iommu_pt_cleanup_list;
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap
2020-09-07 7:40 ` [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
@ 2020-09-10 13:18 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 13:18 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, Wei Liu,
Roger Pau Monné, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Jun Nakajima, Kevin Tian
On 07.09.2020 09:40, Paul Durrant wrote:
> NOTE: The code in memory_add() now sets 'ret' if iommu_map() or
> iommu_iotlb_flush() fails. This seems to be have been missed before,
> meaning the error path could actually return 0.
I agree this is the better way, but I'm not sure it really was
unintended: It could well have been considered a "best effort"
approach to get IOMMU mappings in place, back at the time.
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one nit and one further remark:
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1478,21 +1478,15 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
> !iommu_use_hap_pt(hardware_domain) &&
> !need_iommu_pt_sync(hardware_domain) )
> {
> - for ( i = spfn; i < epfn; i++ )
> - if ( iommu_legacy_map(hardware_domain, _dfn(i), _mfn(i),
> - 1ul << PAGE_ORDER_4K,
> - IOMMUF_readable | IOMMUF_writable) )
> - break;
> - if ( i != epfn )
> - {
> - while (i-- > old_max)
> - /* If statement to satisfy __must_check. */
> - if ( iommu_legacy_unmap(hardware_domain, _dfn(i),
> - 1ul << PAGE_ORDER_4K) )
> - continue;
> + unsigned int flush_flags = 0;
> + unsigned long n = epfn - spfn;
>
> + ret = iommu_map(hardware_domain, _dfn(i), _mfn(i), n,
> + IOMMUF_readable | IOMMUF_writable, &flush_flags);
There's one blank too little here.
> @@ -367,7 +344,7 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned long page_count,
> int rc;
>
> if ( !is_iommu_enabled(d) || !hd->platform_ops->iotlb_flush ||
> - !page_count || !flush_flags )
> + !page_count || !flush_flags || this_cpu(iommu_dont_flush_iotlb) )
> return 0;
I'm still somewhat uneasy about this change, because there could in
theory be fallout. I don't think there is in practice, so I'm willing
to let it be this way. Long term we wan want to introduce a flag
into flush_flags to allow overriding this behavior.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
` (4 preceding siblings ...)
2020-09-07 7:40 ` [PATCH v5 5/8] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-10 13:48 ` Jan Beulich
2020-09-07 7:40 ` [PATCH v5 7/8] iommu: remove the share_p2m operation Paul Durrant
2020-09-07 7:40 ` [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
This patch avoids calling iommu_iotlb_flush() for each individual GNTTABOP and
instead calls iommu_iotlb_flush_all() at the end of a batch. This should mean
non-singleton map/unmap operations perform better.
NOTE: A batch is the number of operations done before a pre-emption check and,
in the case of unmap, a TLB flush.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
v5:
- Add batching to gnttab_map_grant_ref() to handle flushing before pre-
emption check
- Maintain per-op flushing in the case of singletons
v3:
- New in v3
---
xen/common/grant_table.c | 199 ++++++++++++++++++++++++++-------------
1 file changed, 134 insertions(+), 65 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a844a94a5b..8c6d1dcea7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -241,7 +241,13 @@ struct gnttab_unmap_common {
grant_ref_t ref;
};
-/* Number of unmap operations that are done between each tlb flush */
+/* Number of map operations that are done between each pre-emption check */
+#define GNTTAB_MAP_BATCH_SIZE 32
+
+/*
+ * Number of unmap operations that are done between each tlb flush and
+ * pre-emption check.
+ */
#define GNTTAB_UNMAP_BATCH_SIZE 32
@@ -979,7 +985,7 @@ static unsigned int mapkind(
static void
map_grant_ref(
- struct gnttab_map_grant_ref *op)
+ struct gnttab_map_grant_ref *op, bool do_flush, unsigned int *flush_flags)
{
struct domain *ld, *rd, *owner = NULL;
struct grant_table *lgt, *rgt;
@@ -1229,12 +1235,11 @@ map_grant_ref(
if ( kind )
{
dfn_t dfn = _dfn(mfn_x(mfn));
- unsigned int flush_flags = 0;
int err;
- err = iommu_map(ld, dfn, mfn, 1ul, kind, &flush_flags);
- if ( !err )
- err = iommu_iotlb_flush(ld, dfn, 1ul, flush_flags);
+ err = iommu_map(ld, dfn, mfn, 1ul, kind, flush_flags);
+ if ( do_flush && !err )
+ err = iommu_iotlb_flush(ld, dfn, 1ul, *flush_flags);
if ( err )
{
double_gt_unlock(lgt, rgt);
@@ -1319,29 +1324,60 @@ static long
gnttab_map_grant_ref(
XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
{
- int i;
- struct gnttab_map_grant_ref op;
+ struct domain *currd = current->domain;
+ unsigned int done = 0;
+ int rc = 0;
- for ( i = 0; i < count; i++ )
+ while ( count )
{
- if ( i && hypercall_preempt_check() )
- return i;
+ unsigned int i, c = min_t(unsigned int, count, GNTTAB_MAP_BATCH_SIZE);
+ unsigned int flush_flags = 0;
- if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
- return -EFAULT;
+ for ( i = 0; i < c; i++ )
+ {
+ struct gnttab_map_grant_ref op;
- map_grant_ref(&op);
+ if ( unlikely(__copy_from_guest(&op, uop, 1)) )
+ {
+ rc = -EFAULT;
+ break;
+ }
- if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
- return -EFAULT;
+ map_grant_ref(&op, c == 1, &flush_flags);
+
+ if ( unlikely(__copy_to_guest(uop, &op, 1)) )
+ {
+ rc = -EFAULT;
+ break;
+ }
+
+ guest_handle_add_offset(uop, 1);
+ }
+
+ if ( c > 1 )
+ {
+ int err = iommu_iotlb_flush_all(currd, flush_flags);
+
+ if ( !rc )
+ rc = err;
+ }
+
+ if ( rc )
+ break;
+
+ count -= c;
+ done += c;
+
+ if ( count && hypercall_preempt_check() )
+ return done;
}
- return 0;
+ return rc;
}
static void
unmap_common(
- struct gnttab_unmap_common *op)
+ struct gnttab_unmap_common *op, bool do_flush, unsigned int *flush_flags)
{
domid_t dom;
struct domain *ld, *rd;
@@ -1485,22 +1521,21 @@ unmap_common(
{
unsigned int kind;
dfn_t dfn = _dfn(mfn_x(op->mfn));
- unsigned int flush_flags = 0;
int err = 0;
double_gt_lock(lgt, rgt);
kind = mapkind(lgt, rd, op->mfn);
if ( !kind )
- err = iommu_unmap(ld, dfn, 1ul, &flush_flags);
+ err = iommu_unmap(ld, dfn, 1ul, flush_flags);
else if ( !(kind & MAPKIND_WRITE) )
err = iommu_map(ld, dfn, op->mfn, 1ul, IOMMUF_readable,
- &flush_flags);
+ flush_flags);
double_gt_unlock(lgt, rgt);
- if ( !err )
- err = iommu_iotlb_flush(ld, dfn, 1ul, flush_flags);
+ if ( do_flush && !err )
+ err = iommu_iotlb_flush(ld, dfn, 1ul, *flush_flags);
if ( err )
rc = GNTST_general_error;
}
@@ -1599,8 +1634,8 @@ unmap_common_complete(struct gnttab_unmap_common *op)
static void
unmap_grant_ref(
- struct gnttab_unmap_grant_ref *op,
- struct gnttab_unmap_common *common)
+ struct gnttab_unmap_grant_ref *op, struct gnttab_unmap_common *common,
+ bool do_flush, unsigned int *flush_flags)
{
common->host_addr = op->host_addr;
common->dev_bus_addr = op->dev_bus_addr;
@@ -1612,7 +1647,7 @@ unmap_grant_ref(
common->rd = NULL;
common->mfn = INVALID_MFN;
- unmap_common(common);
+ unmap_common(common, do_flush, flush_flags);
op->status = common->status;
}
@@ -1621,31 +1656,55 @@ static long
gnttab_unmap_grant_ref(
XEN_GUEST_HANDLE_PARAM(gnttab_unmap_grant_ref_t) uop, unsigned int count)
{
- int i, c, partial_done, done = 0;
- struct gnttab_unmap_grant_ref op;
- struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ struct domain *currd = current->domain;
+ unsigned int done = 0;
+ int rc = 0;
- while ( count != 0 )
+ while ( count )
{
- c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
- partial_done = 0;
+ struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ unsigned int i, c, partial_done = 0;
+ unsigned int flush_flags = 0;
+
+ c = min_t(unsigned int, count, GNTTAB_UNMAP_BATCH_SIZE);
for ( i = 0; i < c; i++ )
{
+ struct gnttab_unmap_grant_ref op;
+
if ( unlikely(__copy_from_guest(&op, uop, 1)) )
- goto fault;
- unmap_grant_ref(&op, &common[i]);
+ {
+ rc = -EFAULT;
+ break;
+ }
+
+ unmap_grant_ref(&op, &common[i], c == 1, &flush_flags);
++partial_done;
+
if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
- goto fault;
+ {
+ rc = -EFAULT;
+ break;
+ }
+
guest_handle_add_offset(uop, 1);
}
- gnttab_flush_tlb(current->domain);
+ gnttab_flush_tlb(currd);
+ if ( c > 1 )
+ {
+ int err = iommu_iotlb_flush_all(currd, flush_flags);
+
+ if ( !rc )
+ rc = err;
+ }
for ( i = 0; i < partial_done; i++ )
unmap_common_complete(&common[i]);
+ if ( rc )
+ break;
+
count -= c;
done += c;
@@ -1653,20 +1712,13 @@ gnttab_unmap_grant_ref(
return done;
}
- return 0;
-
-fault:
- gnttab_flush_tlb(current->domain);
-
- for ( i = 0; i < partial_done; i++ )
- unmap_common_complete(&common[i]);
- return -EFAULT;
+ return rc;
}
static void
unmap_and_replace(
- struct gnttab_unmap_and_replace *op,
- struct gnttab_unmap_common *common)
+ struct gnttab_unmap_and_replace *op, struct gnttab_unmap_common *common,
+ bool do_flush, unsigned int *flush_flags)
{
common->host_addr = op->host_addr;
common->new_addr = op->new_addr;
@@ -1678,7 +1730,7 @@ unmap_and_replace(
common->rd = NULL;
common->mfn = INVALID_MFN;
- unmap_common(common);
+ unmap_common(common, do_flush, flush_flags);
op->status = common->status;
}
@@ -1686,31 +1738,55 @@ static long
gnttab_unmap_and_replace(
XEN_GUEST_HANDLE_PARAM(gnttab_unmap_and_replace_t) uop, unsigned int count)
{
- int i, c, partial_done, done = 0;
- struct gnttab_unmap_and_replace op;
- struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ struct domain *currd = current->domain;
+ unsigned int done = 0;
+ int rc = 0;
- while ( count != 0 )
+ while ( count )
{
- c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
- partial_done = 0;
+ struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ unsigned int i, c, partial_done = 0;
+ unsigned int flush_flags = 0;
+
+ c = min_t(unsigned int, count, GNTTAB_UNMAP_BATCH_SIZE);
for ( i = 0; i < c; i++ )
{
+ struct gnttab_unmap_and_replace op;
+
if ( unlikely(__copy_from_guest(&op, uop, 1)) )
- goto fault;
- unmap_and_replace(&op, &common[i]);
+ {
+ rc = -EFAULT;
+ break;
+ }
+
+ unmap_and_replace(&op, &common[i], c == 1, &flush_flags);
++partial_done;
+
if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
- goto fault;
+ {
+ rc = -EFAULT;
+ break;
+ }
+
guest_handle_add_offset(uop, 1);
}
- gnttab_flush_tlb(current->domain);
+ gnttab_flush_tlb(currd);
+ if ( c > 1 )
+ {
+ int err = iommu_iotlb_flush_all(currd, flush_flags);
+
+ if ( !rc )
+ rc = err;
+ }
for ( i = 0; i < partial_done; i++ )
unmap_common_complete(&common[i]);
+ if ( rc )
+ break;
+
count -= c;
done += c;
@@ -1718,14 +1794,7 @@ gnttab_unmap_and_replace(
return done;
}
- return 0;
-
-fault:
- gnttab_flush_tlb(current->domain);
-
- for ( i = 0; i < partial_done; i++ )
- unmap_common_complete(&common[i]);
- return -EFAULT;
+ return rc;
}
static int
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
2020-09-07 7:40 ` [PATCH v5 6/8] common/grant_table: batch flush I/O TLB Paul Durrant
@ 2020-09-10 13:48 ` Jan Beulich
2020-09-10 14:17 ` Paul Durrant
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 13:48 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
On 07.09.2020 09:40, Paul Durrant wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -241,7 +241,13 @@ struct gnttab_unmap_common {
> grant_ref_t ref;
> };
>
> -/* Number of unmap operations that are done between each tlb flush */
> +/* Number of map operations that are done between each pre-emption check */
> +#define GNTTAB_MAP_BATCH_SIZE 32
> +
> +/*
> + * Number of unmap operations that are done between each tlb flush and
> + * pre-emption check.
> + */
> #define GNTTAB_UNMAP_BATCH_SIZE 32
Interesting - I don't think I've ever seen preemption spelled like
this (with a hyphen). In the interest of grep-ability, would you
mind changing to the more "conventional" spelling? Albeit I now
notice we have two such spellings in the tree already ...
> @@ -979,7 +985,7 @@ static unsigned int mapkind(
>
> static void
> map_grant_ref(
> - struct gnttab_map_grant_ref *op)
> + struct gnttab_map_grant_ref *op, bool do_flush, unsigned int *flush_flags)
Why two parameters? Simply pass NULL for singletons instead? Although,
you'd need another local variable then, which maybe isn't overly much
nicer.
> @@ -1319,29 +1324,60 @@ static long
> gnttab_map_grant_ref(
> XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
> {
> - int i;
> - struct gnttab_map_grant_ref op;
> + struct domain *currd = current->domain;
Is this a worthwhile variable to have in this case? It's used
exactly once, granted in the loop body, but still not the inner
one.
> + unsigned int done = 0;
> + int rc = 0;
>
> - for ( i = 0; i < count; i++ )
> + while ( count )
> {
> - if ( i && hypercall_preempt_check() )
> - return i;
> + unsigned int i, c = min_t(unsigned int, count, GNTTAB_MAP_BATCH_SIZE);
> + unsigned int flush_flags = 0;
>
> - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
> - return -EFAULT;
> + for ( i = 0; i < c; i++ )
> + {
> + struct gnttab_map_grant_ref op;
>
> - map_grant_ref(&op);
> + if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> + {
> + rc = -EFAULT;
> + break;
> + }
>
> - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
> - return -EFAULT;
> + map_grant_ref(&op, c == 1, &flush_flags);
> +
> + if ( unlikely(__copy_to_guest(uop, &op, 1)) )
> + {
> + rc = -EFAULT;
> + break;
> + }
> +
> + guest_handle_add_offset(uop, 1);
> + }
> +
> + if ( c > 1 )
> + {
> + int err = iommu_iotlb_flush_all(currd, flush_flags);
There's still some double flushing involved in the error case here.
Trying to eliminate this (by having map_grant_ref() write zero
into *flush_flags) may not be overly important, but I thought I'd
still mention it.
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
2020-09-10 13:48 ` Jan Beulich
@ 2020-09-10 14:17 ` Paul Durrant
2020-09-10 14:39 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-10 14:17 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'George Dunlap', 'Ian Jackson',
'Julien Grall', 'Stefano Stabellini',
'Wei Liu'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 10 September 2020 14:49
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
>
> On 07.09.2020 09:40, Paul Durrant wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -241,7 +241,13 @@ struct gnttab_unmap_common {
> > grant_ref_t ref;
> > };
> >
> > -/* Number of unmap operations that are done between each tlb flush */
> > +/* Number of map operations that are done between each pre-emption check */
> > +#define GNTTAB_MAP_BATCH_SIZE 32
> > +
> > +/*
> > + * Number of unmap operations that are done between each tlb flush and
> > + * pre-emption check.
> > + */
> > #define GNTTAB_UNMAP_BATCH_SIZE 32
>
> Interesting - I don't think I've ever seen preemption spelled like
> this (with a hyphen). In the interest of grep-ability, would you
> mind changing to the more "conventional" spelling? Albeit I now
> notice we have two such spellings in the tree already ...
>
Sure, I'll change it.
> > @@ -979,7 +985,7 @@ static unsigned int mapkind(
> >
> > static void
> > map_grant_ref(
> > - struct gnttab_map_grant_ref *op)
> > + struct gnttab_map_grant_ref *op, bool do_flush, unsigned int *flush_flags)
>
> Why two parameters? Simply pass NULL for singletons instead? Although,
> you'd need another local variable then, which maybe isn't overly much
> nicer.
>
No, I think the extra arg is clearer.
> > @@ -1319,29 +1324,60 @@ static long
> > gnttab_map_grant_ref(
> > XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
> > {
> > - int i;
> > - struct gnttab_map_grant_ref op;
> > + struct domain *currd = current->domain;
>
> Is this a worthwhile variable to have in this case? It's used
> exactly once, granted in the loop body, but still not the inner
> one.
>
I thought it was nicer for consistency with the unmap function (where curd is used more than once) but I'll drop it.
> > + unsigned int done = 0;
> > + int rc = 0;
> >
> > - for ( i = 0; i < count; i++ )
> > + while ( count )
> > {
> > - if ( i && hypercall_preempt_check() )
> > - return i;
> > + unsigned int i, c = min_t(unsigned int, count, GNTTAB_MAP_BATCH_SIZE);
> > + unsigned int flush_flags = 0;
> >
> > - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
> > - return -EFAULT;
> > + for ( i = 0; i < c; i++ )
> > + {
> > + struct gnttab_map_grant_ref op;
> >
> > - map_grant_ref(&op);
> > + if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> > + {
> > + rc = -EFAULT;
> > + break;
> > + }
> >
> > - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
> > - return -EFAULT;
> > + map_grant_ref(&op, c == 1, &flush_flags);
> > +
> > + if ( unlikely(__copy_to_guest(uop, &op, 1)) )
> > + {
> > + rc = -EFAULT;
> > + break;
> > + }
> > +
> > + guest_handle_add_offset(uop, 1);
> > + }
> > +
> > + if ( c > 1 )
> > + {
> > + int err = iommu_iotlb_flush_all(currd, flush_flags);
>
> There's still some double flushing involved in the error case here.
> Trying to eliminate this (by having map_grant_ref() write zero
> into *flush_flags) may not be overly important, but I thought I'd
> still mention it.
>
This only kicks in if there's more than one operation and is it safe to squash the flush_flags if some ops succeed and others fail? If all ops fail then flush_flags should still be zero because the call to iommu_map() is the last failure point in map_grant_ref() anyway.
Paul
> Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
2020-09-10 14:17 ` Paul Durrant
@ 2020-09-10 14:39 ` Jan Beulich
2020-09-10 14:41 ` Paul Durrant
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 14:39 UTC (permalink / raw)
To: paul
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'George Dunlap', 'Ian Jackson',
'Julien Grall', 'Stefano Stabellini',
'Wei Liu'
On 10.09.2020 16:17, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 10 September 2020 14:49
>>
>> On 07.09.2020 09:40, Paul Durrant wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -241,7 +241,13 @@ struct gnttab_unmap_common {
>>> grant_ref_t ref;
>>> };
>>>
>>> -/* Number of unmap operations that are done between each tlb flush */
>>> +/* Number of map operations that are done between each pre-emption check */
>>> +#define GNTTAB_MAP_BATCH_SIZE 32
>>> +
>>> +/*
>>> + * Number of unmap operations that are done between each tlb flush and
>>> + * pre-emption check.
>>> + */
>>> #define GNTTAB_UNMAP_BATCH_SIZE 32
>>
>> Interesting - I don't think I've ever seen preemption spelled like
>> this (with a hyphen). In the interest of grep-ability, would you
>> mind changing to the more "conventional" spelling? Albeit I now
>> notice we have two such spellings in the tree already ...
>>
>
> Sure, I'll change it.
>
>>> @@ -979,7 +985,7 @@ static unsigned int mapkind(
>>>
>>> static void
>>> map_grant_ref(
>>> - struct gnttab_map_grant_ref *op)
>>> + struct gnttab_map_grant_ref *op, bool do_flush, unsigned int *flush_flags)
>>
>> Why two parameters? Simply pass NULL for singletons instead? Although,
>> you'd need another local variable then, which maybe isn't overly much
>> nicer.
>>
>
> No, I think the extra arg is clearer.
>
>>> @@ -1319,29 +1324,60 @@ static long
>>> gnttab_map_grant_ref(
>>> XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
>>> {
>>> - int i;
>>> - struct gnttab_map_grant_ref op;
>>> + struct domain *currd = current->domain;
>>
>> Is this a worthwhile variable to have in this case? It's used
>> exactly once, granted in the loop body, but still not the inner
>> one.
>>
>
> I thought it was nicer for consistency with the unmap function (where curd is used more than once) but I'll drop it.
>
>>> + unsigned int done = 0;
>>> + int rc = 0;
>>>
>>> - for ( i = 0; i < count; i++ )
>>> + while ( count )
>>> {
>>> - if ( i && hypercall_preempt_check() )
>>> - return i;
>>> + unsigned int i, c = min_t(unsigned int, count, GNTTAB_MAP_BATCH_SIZE);
>>> + unsigned int flush_flags = 0;
>>>
>>> - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
>>> - return -EFAULT;
>>> + for ( i = 0; i < c; i++ )
>>> + {
>>> + struct gnttab_map_grant_ref op;
>>>
>>> - map_grant_ref(&op);
>>> + if ( unlikely(__copy_from_guest(&op, uop, 1)) )
>>> + {
>>> + rc = -EFAULT;
>>> + break;
>>> + }
>>>
>>> - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
>>> - return -EFAULT;
>>> + map_grant_ref(&op, c == 1, &flush_flags);
>>> +
>>> + if ( unlikely(__copy_to_guest(uop, &op, 1)) )
>>> + {
>>> + rc = -EFAULT;
>>> + break;
>>> + }
>>> +
>>> + guest_handle_add_offset(uop, 1);
>>> + }
>>> +
>>> + if ( c > 1 )
>>> + {
>>> + int err = iommu_iotlb_flush_all(currd, flush_flags);
>>
>> There's still some double flushing involved in the error case here.
>> Trying to eliminate this (by having map_grant_ref() write zero
>> into *flush_flags) may not be overly important, but I thought I'd
>> still mention it.
>>
>
> This only kicks in if there's more than one operation and is it safe to squash the flush_flags if some ops succeed and others fail? If all ops fail then flush_flags should still be zero because the call to iommu_map() is the last failure point in map_grant_ref() anyway.
Well, yes, not a common thing to run into. With the small changes
further up
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
2020-09-10 14:39 ` Jan Beulich
@ 2020-09-10 14:41 ` Paul Durrant
0 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-10 14:41 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'George Dunlap', 'Ian Jackson',
'Julien Grall', 'Stefano Stabellini',
'Wei Liu'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 10 September 2020 15:39
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; 'Stefano Stabellini'
> <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>
> Subject: Re: [PATCH v5 6/8] common/grant_table: batch flush I/O TLB
>
> On 10.09.2020 16:17, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 10 September 2020 14:49
> >>
> >> On 07.09.2020 09:40, Paul Durrant wrote:
> >>> --- a/xen/common/grant_table.c
> >>> +++ b/xen/common/grant_table.c
> >>> @@ -241,7 +241,13 @@ struct gnttab_unmap_common {
> >>> grant_ref_t ref;
> >>> };
> >>>
> >>> -/* Number of unmap operations that are done between each tlb flush */
> >>> +/* Number of map operations that are done between each pre-emption check */
> >>> +#define GNTTAB_MAP_BATCH_SIZE 32
> >>> +
> >>> +/*
> >>> + * Number of unmap operations that are done between each tlb flush and
> >>> + * pre-emption check.
> >>> + */
> >>> #define GNTTAB_UNMAP_BATCH_SIZE 32
> >>
> >> Interesting - I don't think I've ever seen preemption spelled like
> >> this (with a hyphen). In the interest of grep-ability, would you
> >> mind changing to the more "conventional" spelling? Albeit I now
> >> notice we have two such spellings in the tree already ...
> >>
> >
> > Sure, I'll change it.
> >
> >>> @@ -979,7 +985,7 @@ static unsigned int mapkind(
> >>>
> >>> static void
> >>> map_grant_ref(
> >>> - struct gnttab_map_grant_ref *op)
> >>> + struct gnttab_map_grant_ref *op, bool do_flush, unsigned int *flush_flags)
> >>
> >> Why two parameters? Simply pass NULL for singletons instead? Although,
> >> you'd need another local variable then, which maybe isn't overly much
> >> nicer.
> >>
> >
> > No, I think the extra arg is clearer.
> >
> >>> @@ -1319,29 +1324,60 @@ static long
> >>> gnttab_map_grant_ref(
> >>> XEN_GUEST_HANDLE_PARAM(gnttab_map_grant_ref_t) uop, unsigned int count)
> >>> {
> >>> - int i;
> >>> - struct gnttab_map_grant_ref op;
> >>> + struct domain *currd = current->domain;
> >>
> >> Is this a worthwhile variable to have in this case? It's used
> >> exactly once, granted in the loop body, but still not the inner
> >> one.
> >>
> >
> > I thought it was nicer for consistency with the unmap function (where curd is used more than once)
> but I'll drop it.
> >
> >>> + unsigned int done = 0;
> >>> + int rc = 0;
> >>>
> >>> - for ( i = 0; i < count; i++ )
> >>> + while ( count )
> >>> {
> >>> - if ( i && hypercall_preempt_check() )
> >>> - return i;
> >>> + unsigned int i, c = min_t(unsigned int, count, GNTTAB_MAP_BATCH_SIZE);
> >>> + unsigned int flush_flags = 0;
> >>>
> >>> - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
> >>> - return -EFAULT;
> >>> + for ( i = 0; i < c; i++ )
> >>> + {
> >>> + struct gnttab_map_grant_ref op;
> >>>
> >>> - map_grant_ref(&op);
> >>> + if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> >>> + {
> >>> + rc = -EFAULT;
> >>> + break;
> >>> + }
> >>>
> >>> - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
> >>> - return -EFAULT;
> >>> + map_grant_ref(&op, c == 1, &flush_flags);
> >>> +
> >>> + if ( unlikely(__copy_to_guest(uop, &op, 1)) )
> >>> + {
> >>> + rc = -EFAULT;
> >>> + break;
> >>> + }
> >>> +
> >>> + guest_handle_add_offset(uop, 1);
> >>> + }
> >>> +
> >>> + if ( c > 1 )
> >>> + {
> >>> + int err = iommu_iotlb_flush_all(currd, flush_flags);
> >>
> >> There's still some double flushing involved in the error case here.
> >> Trying to eliminate this (by having map_grant_ref() write zero
> >> into *flush_flags) may not be overly important, but I thought I'd
> >> still mention it.
> >>
> >
> > This only kicks in if there's more than one operation and is it safe to squash the flush_flags if
> some ops succeed and others fail? If all ops fail then flush_flags should still be zero because the
> call to iommu_map() is the last failure point in map_grant_ref() anyway.
>
> Well, yes, not a common thing to run into. With the small changes
> further up
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Thanks. Will send v6 shortly.
Paul
> Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 7/8] iommu: remove the share_p2m operation
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
` (5 preceding siblings ...)
2020-09-07 7:40 ` [PATCH v5 6/8] common/grant_table: batch flush I/O TLB Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-10 14:20 ` Jan Beulich
2020-09-07 7:40 ` [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné, Kevin Tian
From: Paul Durrant <pdurrant@amazon.com>
Sharing of HAP tables is now VT-d specific so the operation is never defined
for AMD IOMMU any more. There's also no need to pro-actively set vtd.pgd_maddr
when using shared EPT as it is straightforward to simply define a helper
function to return the appropriate value in the shared and non-shared cases.
NOTE: This patch also modifies unmap_vtd_domain_page() to take a const
pointer since the only thing it calls, unmap_domain_page(), also takes
a const pointer.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>
v5:
- Pass 'nr_pt_levels' into domain_pgd_maddr() directly
v2:
- Put the PGD level adjust into the helper function too, since it is
irrelevant in the shared EPT case
---
xen/arch/x86/mm/p2m.c | 3 -
xen/drivers/passthrough/iommu.c | 8 ---
xen/drivers/passthrough/vtd/extern.h | 2 +-
xen/drivers/passthrough/vtd/iommu.c | 90 +++++++++++++++------------
xen/drivers/passthrough/vtd/x86/vtd.c | 2 +-
xen/include/xen/iommu.h | 3 -
6 files changed, 52 insertions(+), 56 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 01ff92862d..d382199c88 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -726,9 +726,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
p2m->phys_table = pagetable_from_mfn(top_mfn);
- if ( hap_enabled(d) )
- iommu_share_p2m_table(d);
-
p2m_unlock(p2m);
return 0;
}
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e0d36e6bef..e9b6c9a1ec 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -500,14 +500,6 @@ int iommu_do_domctl(
return ret;
}
-void iommu_share_p2m_table(struct domain* d)
-{
- ASSERT(hap_enabled(d));
-
- if ( iommu_use_hap_pt(d) )
- iommu_get_ops()->share_p2m(d);
-}
-
void iommu_crash_shutdown(void)
{
if ( !iommu_crash_disable )
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index ada3c3098c..9cf5b578c9 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -72,7 +72,7 @@ void flush_all_cache(void);
uint64_t alloc_pgtable_maddr(unsigned long npages, nodeid_t node);
void free_pgtable_maddr(u64 maddr);
void *map_vtd_domain_page(u64 maddr);
-void unmap_vtd_domain_page(void *va);
+void unmap_vtd_domain_page(const void *va);
int domain_context_mapping_one(struct domain *domain, struct vtd_iommu *iommu,
u8 bus, u8 devfn, const struct pci_dev *);
int domain_context_unmap_one(struct domain *domain, struct vtd_iommu *iommu,
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 68cf0e535a..2ced3a5fa5 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -318,6 +318,48 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
return pte_maddr;
}
+static uint64_t domain_pgd_maddr(struct domain *d, unsigned int nr_pt_levels)
+{
+ struct domain_iommu *hd = dom_iommu(d);
+ uint64_t pgd_maddr;
+ unsigned int agaw;
+
+ ASSERT(spin_is_locked(&hd->arch.mapping_lock));
+
+ if ( iommu_use_hap_pt(d) )
+ {
+ mfn_t pgd_mfn =
+ pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
+
+ return pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
+ }
+
+ if ( !hd->arch.vtd.pgd_maddr )
+ {
+ addr_to_dma_page_maddr(d, 0, 1);
+
+ if ( !hd->arch.vtd.pgd_maddr )
+ return 0;
+ }
+
+ pgd_maddr = hd->arch.vtd.pgd_maddr;
+
+ /* Skip top levels of page tables for 2- and 3-level DRHDs. */
+ for ( agaw = level_to_agaw(4);
+ agaw != level_to_agaw(nr_pt_levels);
+ agaw-- )
+ {
+ const struct dma_pte *p = map_vtd_domain_page(pgd_maddr);
+
+ pgd_maddr = dma_pte_addr(*p);
+ unmap_vtd_domain_page(p);
+ if ( !pgd_maddr )
+ return 0;
+ }
+
+ return pgd_maddr;
+}
+
static void iommu_flush_write_buffer(struct vtd_iommu *iommu)
{
u32 val;
@@ -1286,7 +1328,7 @@ int domain_context_mapping_one(
struct context_entry *context, *context_entries;
u64 maddr, pgd_maddr;
u16 seg = iommu->drhd->segment;
- int agaw, rc, ret;
+ int rc, ret;
bool_t flush_dev_iotlb;
ASSERT(pcidevs_locked());
@@ -1340,37 +1382,18 @@ int domain_context_mapping_one(
if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
{
context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
- agaw = level_to_agaw(iommu->nr_pt_levels);
}
else
{
spin_lock(&hd->arch.mapping_lock);
- /* Ensure we have pagetables allocated down to leaf PTE. */
- if ( hd->arch.vtd.pgd_maddr == 0 )
+ pgd_maddr = domain_pgd_maddr(domain, iommu->nr_pt_levels);
+ if ( !pgd_maddr )
{
- addr_to_dma_page_maddr(domain, 0, 1);
- if ( hd->arch.vtd.pgd_maddr == 0 )
- {
- nomem:
- spin_unlock(&hd->arch.mapping_lock);
- spin_unlock(&iommu->lock);
- unmap_vtd_domain_page(context_entries);
- return -ENOMEM;
- }
- }
-
- /* Skip top levels of page tables for 2- and 3-level DRHDs. */
- pgd_maddr = hd->arch.vtd.pgd_maddr;
- for ( agaw = level_to_agaw(4);
- agaw != level_to_agaw(iommu->nr_pt_levels);
- agaw-- )
- {
- struct dma_pte *p = map_vtd_domain_page(pgd_maddr);
- pgd_maddr = dma_pte_addr(*p);
- unmap_vtd_domain_page(p);
- if ( pgd_maddr == 0 )
- goto nomem;
+ spin_unlock(&hd->arch.mapping_lock);
+ spin_unlock(&iommu->lock);
+ unmap_vtd_domain_page(context_entries);
+ return -ENOMEM;
}
context_set_address_root(*context, pgd_maddr);
@@ -1389,7 +1412,7 @@ int domain_context_mapping_one(
return -EFAULT;
}
- context_set_address_width(*context, agaw);
+ context_set_address_width(*context, level_to_agaw(iommu->nr_pt_levels));
context_set_fault_enable(*context);
context_set_present(*context);
iommu_sync_cache(context, sizeof(struct context_entry));
@@ -1848,18 +1871,6 @@ static int __init vtd_ept_page_compatible(struct vtd_iommu *iommu)
(ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap);
}
-/*
- * set VT-d page table directory to EPT table if allowed
- */
-static void iommu_set_pgd(struct domain *d)
-{
- mfn_t pgd_mfn;
-
- pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
- dom_iommu(d)->arch.vtd.pgd_maddr =
- pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
-}
-
static int rmrr_identity_mapping(struct domain *d, bool_t map,
const struct acpi_rmrr_unit *rmrr,
u32 flag)
@@ -2719,7 +2730,6 @@ static struct iommu_ops __initdata vtd_ops = {
.adjust_irq_affinities = adjust_vtd_irq_affinities,
.suspend = vtd_suspend,
.resume = vtd_resume,
- .share_p2m = iommu_set_pgd,
.crash_shutdown = vtd_crash_shutdown,
.iotlb_flush = iommu_flush_iotlb_pages,
.iotlb_flush_all = iommu_flush_iotlb_all,
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c
index bbe358dc36..6681dccd69 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -42,7 +42,7 @@ void *map_vtd_domain_page(u64 maddr)
return map_domain_page(_mfn(paddr_to_pfn(maddr)));
}
-void unmap_vtd_domain_page(void *va)
+void unmap_vtd_domain_page(const void *va)
{
unmap_domain_page(va);
}
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index a2eefe8582..373145266f 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -270,7 +270,6 @@ struct iommu_ops {
int __must_check (*suspend)(void);
void (*resume)(void);
- void (*share_p2m)(struct domain *d);
void (*crash_shutdown)(void);
int __must_check (*iotlb_flush)(struct domain *d, dfn_t dfn,
unsigned long page_count,
@@ -347,8 +346,6 @@ void iommu_resume(void);
void iommu_crash_shutdown(void);
int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
-void iommu_share_p2m_table(struct domain *d);
-
#ifdef CONFIG_HAS_PCI
int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 7/8] iommu: remove the share_p2m operation
2020-09-07 7:40 ` [PATCH v5 7/8] iommu: remove the share_p2m operation Paul Durrant
@ 2020-09-10 14:20 ` Jan Beulich
2020-09-10 14:25 ` Paul Durrant
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 14:20 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné, Kevin Tian
On 07.09.2020 09:40, Paul Durrant wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -318,6 +318,48 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
> return pte_maddr;
> }
>
> +static uint64_t domain_pgd_maddr(struct domain *d, unsigned int nr_pt_levels)
> +{
> + struct domain_iommu *hd = dom_iommu(d);
> + uint64_t pgd_maddr;
> + unsigned int agaw;
> +
> + ASSERT(spin_is_locked(&hd->arch.mapping_lock));
> +
> + if ( iommu_use_hap_pt(d) )
> + {
> + mfn_t pgd_mfn =
> + pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> +
> + return pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
I thought I had already asked about this odd going through a pagetable_t
a 2nd time without a clear need. Why not simply
if ( iommu_use_hap_pt(d) )
{
pagetable_t pgt = p2m_get_pagetable(p2m_get_hostp2m(d));
return pagetable_get_paddr(pgt);
}
?
> + }
> +
> + if ( !hd->arch.vtd.pgd_maddr )
> + {
> + addr_to_dma_page_maddr(d, 0, 1);
Ahead of this, would you mind retaining ...
> @@ -1340,37 +1382,18 @@ int domain_context_mapping_one(
> if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
> {
> context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
> - agaw = level_to_agaw(iommu->nr_pt_levels);
> }
> else
> {
> spin_lock(&hd->arch.mapping_lock);
>
> - /* Ensure we have pagetables allocated down to leaf PTE. */
> - if ( hd->arch.vtd.pgd_maddr == 0 )
> + pgd_maddr = domain_pgd_maddr(domain, iommu->nr_pt_levels);
> + if ( !pgd_maddr )
> {
> - addr_to_dma_page_maddr(domain, 0, 1);
... the comment you remove here?
With these adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v5 7/8] iommu: remove the share_p2m operation
2020-09-10 14:20 ` Jan Beulich
@ 2020-09-10 14:25 ` Paul Durrant
0 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-10 14:25 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'George Dunlap', 'Wei Liu',
'Roger Pau Monné', 'Kevin Tian'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 10 September 2020 15:20
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v5 7/8] iommu: remove the share_p2m operation
>
> On 07.09.2020 09:40, Paul Durrant wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -318,6 +318,48 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
> > return pte_maddr;
> > }
> >
> > +static uint64_t domain_pgd_maddr(struct domain *d, unsigned int nr_pt_levels)
> > +{
> > + struct domain_iommu *hd = dom_iommu(d);
> > + uint64_t pgd_maddr;
> > + unsigned int agaw;
> > +
> > + ASSERT(spin_is_locked(&hd->arch.mapping_lock));
> > +
> > + if ( iommu_use_hap_pt(d) )
> > + {
> > + mfn_t pgd_mfn =
> > + pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> > +
> > + return pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
>
> I thought I had already asked about this odd going through a pagetable_t
> a 2nd time without a clear need. Why not simply
>
> if ( iommu_use_hap_pt(d) )
> {
> pagetable_t pgt = p2m_get_pagetable(p2m_get_hostp2m(d));
>
> return pagetable_get_paddr(pgt);
> }
>
> ?
It's code movement (which IIRC you had acknowledged) but I can change it if you wish.
>
> > + }
> > +
> > + if ( !hd->arch.vtd.pgd_maddr )
> > + {
> > + addr_to_dma_page_maddr(d, 0, 1);
>
> Ahead of this, would you mind retaining ...
>
> > @@ -1340,37 +1382,18 @@ int domain_context_mapping_one(
> > if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
> > {
> > context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
> > - agaw = level_to_agaw(iommu->nr_pt_levels);
> > }
> > else
> > {
> > spin_lock(&hd->arch.mapping_lock);
> >
> > - /* Ensure we have pagetables allocated down to leaf PTE. */
> > - if ( hd->arch.vtd.pgd_maddr == 0 )
> > + pgd_maddr = domain_pgd_maddr(domain, iommu->nr_pt_levels);
> > + if ( !pgd_maddr )
> > {
> > - addr_to_dma_page_maddr(domain, 0, 1);
>
> ... the comment you remove here?
>
Sure.
> With these adjustments
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Thanks.
Paul
> Jan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables'
2020-09-07 7:40 [PATCH v5 0/8] IOMMU cleanup Paul Durrant
` (6 preceding siblings ...)
2020-09-07 7:40 ` [PATCH v5 7/8] iommu: remove the share_p2m operation Paul Durrant
@ 2020-09-07 7:40 ` Paul Durrant
2020-09-10 14:27 ` Jan Beulich
7 siblings, 1 reply; 21+ messages in thread
From: Paul Durrant @ 2020-09-07 7:40 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich, Andrew Cooper, Kevin Tian
From: Paul Durrant <pdurrant@amazon.com>
It's confusing and not consistent with the terminology introduced with 'dfn_t'.
Just call them IOMMU page tables.
Also remove a pointless check of the 'acpi_drhd_units' list in
vtd_dump_page_table_level(). If the list is empty then IOMMU mappings would
not have been enabled for the domain in the first place.
NOTE: All calls to printk() have also been removed from
iommu_dump_page_tables(); the implementation specific code is now
responsible for all output.
The check for the global 'iommu_enabled' has also been replaced by an
ASSERT since iommu_dump_page_tables() is not registered as a key handler
unless IOMMU mappings are enabled.
Error messages are now prefixed with the name of the function.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>
v5:
- Make sure domain id is in the output
- Use VTDPREFIX in output for consistency
v2:
- Moved all output into implementation specific code
---
xen/drivers/passthrough/amd/pci_amd_iommu.c | 22 +++++++--------
xen/drivers/passthrough/iommu.c | 21 ++++-----------
xen/drivers/passthrough/vtd/iommu.c | 30 ++++++++++++---------
xen/include/xen/iommu.h | 2 +-
4 files changed, 34 insertions(+), 41 deletions(-)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 3390c22cf3..80b0f58f88 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -491,8 +491,8 @@ static int amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
#include <asm/io_apic.h>
-static void amd_dump_p2m_table_level(struct page_info* pg, int level,
- paddr_t gpa, int indent)
+static void amd_dump_page_table_level(struct page_info* pg, int level,
+ paddr_t gpa, int indent)
{
paddr_t address;
struct amd_iommu_pte *table_vaddr;
@@ -504,7 +504,7 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
table_vaddr = __map_domain_page(pg);
if ( table_vaddr == NULL )
{
- printk("Failed to map IOMMU domain page %"PRIpaddr"\n",
+ printk("%s: failed to map domain page %"PRIpaddr"\n", __func__,
page_to_maddr(pg));
return;
}
@@ -521,15 +521,15 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
if ( pde->next_level && (pde->next_level != (level - 1)) )
{
- printk("IOMMU p2m table error. next_level = %d, expected %d\n",
- pde->next_level, level - 1);
+ printk("%s: table error. next_level = %d, expected %d\n",
+ __func__, pde->next_level, level - 1);
continue;
}
address = gpa + amd_offset_level_address(index, level);
if ( pde->next_level >= 1 )
- amd_dump_p2m_table_level(
+ amd_dump_page_table_level(
mfn_to_page(_mfn(pde->mfn)), pde->next_level,
address, indent + 1);
else
@@ -542,16 +542,16 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
unmap_domain_page(table_vaddr);
}
-static void amd_dump_p2m_table(struct domain *d)
+static void amd_dump_page_tables(struct domain *d)
{
const struct domain_iommu *hd = dom_iommu(d);
if ( !hd->arch.amd.root_table )
return;
- printk("p2m table has %u levels\n", hd->arch.amd.paging_mode);
- amd_dump_p2m_table_level(hd->arch.amd.root_table,
- hd->arch.amd.paging_mode, 0, 0);
+ printk("AMD IOMMU %pd table has %u levels\n", d, hd->arch.amd.paging_mode);
+ amd_dump_page_table_level(hd->arch.amd.root_table,
+ hd->arch.amd.paging_mode, 0, 0);
}
static const struct iommu_ops __initconstrel _iommu_ops = {
@@ -578,7 +578,7 @@ static const struct iommu_ops __initconstrel _iommu_ops = {
.suspend = amd_iommu_suspend,
.resume = amd_iommu_resume,
.crash_shutdown = amd_iommu_crash_shutdown,
- .dump_p2m_table = amd_dump_p2m_table,
+ .dump_page_tables = amd_dump_page_tables,
};
static const struct iommu_init_ops __initconstrel _iommu_init_ops = {
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e9b6c9a1ec..f5cd04fb3e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -22,7 +22,7 @@
#include <xen/keyhandler.h>
#include <xsm/xsm.h>
-static void iommu_dump_p2m_table(unsigned char key);
+static void iommu_dump_page_tables(unsigned char key);
unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
@@ -212,7 +212,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
if ( !is_iommu_enabled(d) )
return;
- register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
+ register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0);
hd->platform_ops->hwdom_init(d);
}
@@ -535,16 +535,12 @@ bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
}
-static void iommu_dump_p2m_table(unsigned char key)
+static void iommu_dump_page_tables(unsigned char key)
{
struct domain *d;
const struct iommu_ops *ops;
- if ( !iommu_enabled )
- {
- printk("IOMMU not enabled!\n");
- return;
- }
+ ASSERT(iommu_enabled);
ops = iommu_get_ops();
@@ -555,14 +551,7 @@ static void iommu_dump_p2m_table(unsigned char key)
if ( is_hardware_domain(d) || !is_iommu_enabled(d) )
continue;
- if ( iommu_use_hap_pt(d) )
- {
- printk("\ndomain%d IOMMU p2m table shared with MMU: \n", d->domain_id);
- continue;
- }
-
- printk("\ndomain%d IOMMU p2m table: \n", d->domain_id);
- ops->dump_p2m_table(d);
+ ops->dump_page_tables(d);
}
rcu_read_unlock(&domlist_read_lock);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 2ced3a5fa5..d84c000a8e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2582,8 +2582,8 @@ static void vtd_resume(void)
}
}
-static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
- int indent)
+static void vtd_dump_page_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
+ int indent)
{
paddr_t address;
int i;
@@ -2596,7 +2596,8 @@ static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
pt_vaddr = map_vtd_domain_page(pt_maddr);
if ( pt_vaddr == NULL )
{
- printk("Failed to map VT-D domain page %"PRIpaddr"\n", pt_maddr);
+ printk("%s: failed to map domain page %"PRIpaddr"\n", __func__,
+ pt_maddr);
return;
}
@@ -2612,8 +2613,8 @@ static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
address = gpa + offset_level_address(i, level);
if ( next_level >= 1 )
- vtd_dump_p2m_table_level(dma_pte_addr(*pte), next_level,
- address, indent + 1);
+ vtd_dump_page_table_level(dma_pte_addr(*pte), next_level,
+ address, indent + 1);
else
printk("%*sdfn: %08lx mfn: %08lx\n",
indent, "",
@@ -2624,17 +2625,20 @@ static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa,
unmap_vtd_domain_page(pt_vaddr);
}
-static void vtd_dump_p2m_table(struct domain *d)
+static void vtd_dump_page_tables(struct domain *d)
{
- const struct domain_iommu *hd;
+ const struct domain_iommu *hd = dom_iommu(d);
- if ( list_empty(&acpi_drhd_units) )
+ if ( iommu_use_hap_pt(d) )
+ {
+ printk(VTDPREFIX " %pd sharing EPT table\n", d);
return;
+ }
- hd = dom_iommu(d);
- printk("p2m table has %d levels\n", agaw_to_level(hd->arch.vtd.agaw));
- vtd_dump_p2m_table_level(hd->arch.vtd.pgd_maddr,
- agaw_to_level(hd->arch.vtd.agaw), 0, 0);
+ printk(VTDPREFIX" %pd table has %d levels\n", d,
+ agaw_to_level(hd->arch.vtd.agaw));
+ vtd_dump_page_table_level(hd->arch.vtd.pgd_maddr,
+ agaw_to_level(hd->arch.vtd.agaw), 0, 0);
}
static int __init intel_iommu_quarantine_init(struct domain *d)
@@ -2734,7 +2738,7 @@ static struct iommu_ops __initdata vtd_ops = {
.iotlb_flush = iommu_flush_iotlb_pages,
.iotlb_flush_all = iommu_flush_iotlb_all,
.get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
- .dump_p2m_table = vtd_dump_p2m_table,
+ .dump_page_tables = vtd_dump_page_tables,
};
const struct iommu_init_ops __initconstrel intel_iommu_init_ops = {
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 373145266f..7a539522b1 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -276,7 +276,7 @@ struct iommu_ops {
unsigned int flush_flags);
int __must_check (*iotlb_flush_all)(struct domain *d);
int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
- void (*dump_p2m_table)(struct domain *d);
+ void (*dump_page_tables)(struct domain *d);
#ifdef CONFIG_HAS_DEVICE_TREE
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables'
2020-09-07 7:40 ` [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
@ 2020-09-10 14:27 ` Jan Beulich
2020-09-10 14:40 ` Paul Durrant
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-09-10 14:27 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant, Andrew Cooper, Kevin Tian
On 07.09.2020 09:40, Paul Durrant wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -491,8 +491,8 @@ static int amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
>
> #include <asm/io_apic.h>
>
> -static void amd_dump_p2m_table_level(struct page_info* pg, int level,
> - paddr_t gpa, int indent)
> +static void amd_dump_page_table_level(struct page_info* pg, int level,
Could you flip * and space here as you touch the line anyway?
> @@ -504,7 +504,7 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
> table_vaddr = __map_domain_page(pg);
> if ( table_vaddr == NULL )
> {
> - printk("Failed to map IOMMU domain page %"PRIpaddr"\n",
> + printk("%s: failed to map domain page %"PRIpaddr"\n", __func__,
> page_to_maddr(pg));
Why the addition of __func__? Personally I'd rather see most of its
uses go away ... (There are two more further down.) Preferably with
this dropped again
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables'
2020-09-10 14:27 ` Jan Beulich
@ 2020-09-10 14:40 ` Paul Durrant
0 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2020-09-10 14:40 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: xen-devel, 'Paul Durrant', 'Andrew Cooper',
'Kevin Tian'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 10 September 2020 15:27
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v5 8/8] iommu: stop calling IOMMU page tables 'p2m tables'
>
> On 07.09.2020 09:40, Paul Durrant wrote:
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -491,8 +491,8 @@ static int amd_iommu_group_id(u16 seg, u8 bus, u8 devfn)
> >
> > #include <asm/io_apic.h>
> >
> > -static void amd_dump_p2m_table_level(struct page_info* pg, int level,
> > - paddr_t gpa, int indent)
> > +static void amd_dump_page_table_level(struct page_info* pg, int level,
>
> Could you flip * and space here as you touch the line anyway?
>
Sure.
> > @@ -504,7 +504,7 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
> > table_vaddr = __map_domain_page(pg);
> > if ( table_vaddr == NULL )
> > {
> > - printk("Failed to map IOMMU domain page %"PRIpaddr"\n",
> > + printk("%s: failed to map domain page %"PRIpaddr"\n", __func__,
> > page_to_maddr(pg));
>
> Why the addition of __func__? Personally I'd rather see most of its
> uses go away ... (There are two more further down.) Preferably with
> this dropped again
Ok.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Thanks.
Paul
> Jan
^ permalink raw reply [flat|nested] 21+ messages in thread