* [PATCH v3 01/11] x86/iommu: re-arrange arch_iommu to separate common fields...
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 15:53 ` Jan Beulich
2020-08-03 12:29 ` [PATCH v3 02/11] x86/iommu: add common page-table allocator Paul Durrant
` (9 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Wei Liu, Andrew Cooper, Paul Durrant,
Lukasz Hawrylko, Jan Beulich, Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
... from those specific to VT-d or AMD IOMMU, and put the latter in a union.
There is no functional change in this patch, although the initialization of
the 'mapped_rmrrs' list occurs slightly later in iommu_domain_init() since
it is now done (correctly) in VT-d specific code rather than in general x86
code.
NOTE: I have not combined the AMD IOMMU 'root_table' and VT-d 'pgd_maddr'
fields even though they perform essentially the same function. The
concept of 'root table' in the VT-d code is different from that in the
AMD code so attempting to use a common name will probably only serve
to confuse the reader.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Lukasz Hawrylko <lukasz.hawrylko@linux.intel.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: Kevin Tian <kevin.tian@intel.com>
v2:
- s/amd_iommu/amd
- Definitions still left inline as re-arrangement into implementation
headers is non-trivial
- Also s/u64/uint64_t and s/int/unsigned int
---
xen/arch/x86/tboot.c | 4 +-
xen/drivers/passthrough/amd/iommu_guest.c | 8 ++--
xen/drivers/passthrough/amd/iommu_map.c | 14 +++---
xen/drivers/passthrough/amd/pci_amd_iommu.c | 35 +++++++-------
xen/drivers/passthrough/vtd/iommu.c | 53 +++++++++++----------
xen/drivers/passthrough/x86/iommu.c | 1 -
xen/include/asm-x86/iommu.h | 27 +++++++----
7 files changed, 78 insertions(+), 64 deletions(-)
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index 320e06f129..e66b0940c4 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -230,8 +230,8 @@ static void tboot_gen_domain_integrity(const uint8_t key[TB_KEY_SIZE],
{
const struct domain_iommu *dio = dom_iommu(d);
- update_iommu_mac(&ctx, dio->arch.pgd_maddr,
- agaw_to_level(dio->arch.agaw));
+ update_iommu_mac(&ctx, dio->arch.vtd.pgd_maddr,
+ agaw_to_level(dio->arch.vtd.agaw));
}
}
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 014a72a54b..30b7353cd6 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -50,12 +50,12 @@ static uint16_t guest_bdf(struct domain *d, uint16_t machine_bdf)
static inline struct guest_iommu *domain_iommu(struct domain *d)
{
- return dom_iommu(d)->arch.g_iommu;
+ return dom_iommu(d)->arch.amd.g_iommu;
}
static inline struct guest_iommu *vcpu_iommu(struct vcpu *v)
{
- return dom_iommu(v->domain)->arch.g_iommu;
+ return dom_iommu(v->domain)->arch.amd.g_iommu;
}
static void guest_iommu_enable(struct guest_iommu *iommu)
@@ -823,7 +823,7 @@ int guest_iommu_init(struct domain* d)
guest_iommu_reg_init(iommu);
iommu->mmio_base = ~0ULL;
iommu->domain = d;
- hd->arch.g_iommu = iommu;
+ hd->arch.amd.g_iommu = iommu;
tasklet_init(&iommu->cmd_buffer_tasklet, guest_iommu_process_command, d);
@@ -845,5 +845,5 @@ void guest_iommu_destroy(struct domain *d)
tasklet_kill(&iommu->cmd_buffer_tasklet);
xfree(iommu);
- dom_iommu(d)->arch.g_iommu = NULL;
+ dom_iommu(d)->arch.amd.g_iommu = NULL;
}
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 93e96cd69c..47b4472e8a 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -180,8 +180,8 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
struct page_info *table;
const struct domain_iommu *hd = dom_iommu(d);
- table = hd->arch.root_table;
- level = hd->arch.paging_mode;
+ table = hd->arch.amd.root_table;
+ level = hd->arch.amd.paging_mode;
BUG_ON( table == NULL || level < 1 || level > 6 );
@@ -325,7 +325,7 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
spin_lock(&hd->arch.mapping_lock);
- if ( !hd->arch.root_table )
+ if ( !hd->arch.amd.root_table )
{
spin_unlock(&hd->arch.mapping_lock);
return 0;
@@ -450,7 +450,7 @@ int __init amd_iommu_quarantine_init(struct domain *d)
unsigned int level = amd_iommu_get_paging_mode(end_gfn);
struct amd_iommu_pte *table;
- if ( hd->arch.root_table )
+ if ( hd->arch.amd.root_table )
{
ASSERT_UNREACHABLE();
return 0;
@@ -458,11 +458,11 @@ int __init amd_iommu_quarantine_init(struct domain *d)
spin_lock(&hd->arch.mapping_lock);
- hd->arch.root_table = alloc_amd_iommu_pgtable();
- if ( !hd->arch.root_table )
+ hd->arch.amd.root_table = alloc_amd_iommu_pgtable();
+ if ( !hd->arch.amd.root_table )
goto out;
- table = __map_domain_page(hd->arch.root_table);
+ table = __map_domain_page(hd->arch.amd.root_table);
while ( level )
{
struct page_info *pg;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 5f5f4a2eac..c27bfbd48e 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -91,7 +91,8 @@ static void amd_iommu_setup_domain_device(
u8 bus = pdev->bus;
const struct domain_iommu *hd = dom_iommu(domain);
- BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
+ BUG_ON( !hd->arch.amd.root_table ||
+ !hd->arch.amd.paging_mode ||
!iommu->dev_table.buffer );
if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
@@ -110,8 +111,8 @@ static void amd_iommu_setup_domain_device(
/* bind DTE to domain page-tables */
amd_iommu_set_root_page_table(
- dte, page_to_maddr(hd->arch.root_table), domain->domain_id,
- hd->arch.paging_mode, valid);
+ dte, page_to_maddr(hd->arch.amd.root_table),
+ domain->domain_id, hd->arch.amd.paging_mode, valid);
/* Undo what amd_iommu_disable_domain_device() may have done. */
ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
@@ -131,8 +132,8 @@ static void amd_iommu_setup_domain_device(
"root table = %#"PRIx64", "
"domain = %d, paging mode = %d\n",
req_id, pdev->type,
- page_to_maddr(hd->arch.root_table),
- domain->domain_id, hd->arch.paging_mode);
+ page_to_maddr(hd->arch.amd.root_table),
+ domain->domain_id, hd->arch.amd.paging_mode);
}
spin_unlock_irqrestore(&iommu->lock, flags);
@@ -206,10 +207,10 @@ static int iov_enable_xt(void)
int amd_iommu_alloc_root(struct domain_iommu *hd)
{
- if ( unlikely(!hd->arch.root_table) )
+ if ( unlikely(!hd->arch.amd.root_table) )
{
- hd->arch.root_table = alloc_amd_iommu_pgtable();
- if ( !hd->arch.root_table )
+ hd->arch.amd.root_table = alloc_amd_iommu_pgtable();
+ if ( !hd->arch.amd.root_table )
return -ENOMEM;
}
@@ -239,7 +240,7 @@ static int amd_iommu_domain_init(struct domain *d)
* physical address space we give it, but this isn't known yet so use 4
* unilaterally.
*/
- hd->arch.paging_mode = amd_iommu_get_paging_mode(
+ hd->arch.amd.paging_mode = amd_iommu_get_paging_mode(
is_hvm_domain(d)
? 1ul << (DEFAULT_DOMAIN_ADDRESS_WIDTH - PAGE_SHIFT)
: get_upper_mfn_bound() + 1);
@@ -305,7 +306,7 @@ static void amd_iommu_disable_domain_device(const struct domain *domain,
AMD_IOMMU_DEBUG("Disable: device id = %#x, "
"domain = %d, paging mode = %d\n",
req_id, domain->domain_id,
- dom_iommu(domain)->arch.paging_mode);
+ dom_iommu(domain)->arch.amd.paging_mode);
}
spin_unlock_irqrestore(&iommu->lock, flags);
@@ -420,10 +421,11 @@ static void deallocate_iommu_page_tables(struct domain *d)
struct domain_iommu *hd = dom_iommu(d);
spin_lock(&hd->arch.mapping_lock);
- if ( hd->arch.root_table )
+ if ( hd->arch.amd.root_table )
{
- deallocate_next_page_table(hd->arch.root_table, hd->arch.paging_mode);
- hd->arch.root_table = NULL;
+ deallocate_next_page_table(hd->arch.amd.root_table,
+ hd->arch.amd.paging_mode);
+ hd->arch.amd.root_table = NULL;
}
spin_unlock(&hd->arch.mapping_lock);
}
@@ -598,11 +600,12 @@ static void amd_dump_p2m_table(struct domain *d)
{
const struct domain_iommu *hd = dom_iommu(d);
- if ( !hd->arch.root_table )
+ if ( !hd->arch.amd.root_table )
return;
- printk("p2m table has %d levels\n", hd->arch.paging_mode);
- amd_dump_p2m_table_level(hd->arch.root_table, hd->arch.paging_mode, 0, 0);
+ printk("p2m table has %d levels\n", hd->arch.amd.paging_mode);
+ amd_dump_p2m_table_level(hd->arch.amd.root_table,
+ hd->arch.amd.paging_mode, 0, 0);
}
static const struct iommu_ops __initconstrel _iommu_ops = {
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index deaeab095d..94e0455a4d 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -257,20 +257,20 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus)
static u64 addr_to_dma_page_maddr(struct domain *domain, u64 addr, int alloc)
{
struct domain_iommu *hd = dom_iommu(domain);
- int addr_width = agaw_to_width(hd->arch.agaw);
+ int addr_width = agaw_to_width(hd->arch.vtd.agaw);
struct dma_pte *parent, *pte = NULL;
- int level = agaw_to_level(hd->arch.agaw);
+ int level = agaw_to_level(hd->arch.vtd.agaw);
int offset;
u64 pte_maddr = 0;
addr &= (((u64)1) << addr_width) - 1;
ASSERT(spin_is_locked(&hd->arch.mapping_lock));
- if ( !hd->arch.pgd_maddr &&
+ if ( !hd->arch.vtd.pgd_maddr &&
(!alloc ||
- ((hd->arch.pgd_maddr = alloc_pgtable_maddr(1, hd->node)) == 0)) )
+ ((hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node)) == 0)) )
goto out;
- parent = (struct dma_pte *)map_vtd_domain_page(hd->arch.pgd_maddr);
+ parent = (struct dma_pte *)map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
while ( level > 1 )
{
offset = address_level_offset(addr, level);
@@ -593,7 +593,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d, dfn_t dfn,
{
iommu = drhd->iommu;
- if ( !test_bit(iommu->index, &hd->arch.iommu_bitmap) )
+ if ( !test_bit(iommu->index, &hd->arch.vtd.iommu_bitmap) )
continue;
flush_dev_iotlb = !!find_ats_dev_drhd(iommu);
@@ -1278,7 +1278,10 @@ void __init iommu_free(struct acpi_drhd_unit *drhd)
static int intel_iommu_domain_init(struct domain *d)
{
- dom_iommu(d)->arch.agaw = width_to_agaw(DEFAULT_DOMAIN_ADDRESS_WIDTH);
+ struct domain_iommu *hd = dom_iommu(d);
+
+ hd->arch.vtd.agaw = width_to_agaw(DEFAULT_DOMAIN_ADDRESS_WIDTH);
+ INIT_LIST_HEAD(&hd->arch.vtd.mapped_rmrrs);
return 0;
}
@@ -1375,10 +1378,10 @@ int domain_context_mapping_one(
spin_lock(&hd->arch.mapping_lock);
/* Ensure we have pagetables allocated down to leaf PTE. */
- if ( hd->arch.pgd_maddr == 0 )
+ if ( hd->arch.vtd.pgd_maddr == 0 )
{
addr_to_dma_page_maddr(domain, 0, 1);
- if ( hd->arch.pgd_maddr == 0 )
+ if ( hd->arch.vtd.pgd_maddr == 0 )
{
nomem:
spin_unlock(&hd->arch.mapping_lock);
@@ -1389,7 +1392,7 @@ int domain_context_mapping_one(
}
/* Skip top levels of page tables for 2- and 3-level DRHDs. */
- pgd_maddr = hd->arch.pgd_maddr;
+ pgd_maddr = hd->arch.vtd.pgd_maddr;
for ( agaw = level_to_agaw(4);
agaw != level_to_agaw(iommu->nr_pt_levels);
agaw-- )
@@ -1443,7 +1446,7 @@ int domain_context_mapping_one(
if ( rc > 0 )
rc = 0;
- set_bit(iommu->index, &hd->arch.iommu_bitmap);
+ set_bit(iommu->index, &hd->arch.vtd.iommu_bitmap);
unmap_vtd_domain_page(context_entries);
@@ -1714,7 +1717,7 @@ static int domain_context_unmap(struct domain *domain, u8 devfn,
{
int iommu_domid;
- clear_bit(iommu->index, &dom_iommu(domain)->arch.iommu_bitmap);
+ clear_bit(iommu->index, &dom_iommu(domain)->arch.vtd.iommu_bitmap);
iommu_domid = domain_iommu_domid(domain, iommu);
if ( iommu_domid == -1 )
@@ -1739,7 +1742,7 @@ static void iommu_domain_teardown(struct domain *d)
if ( list_empty(&acpi_drhd_units) )
return;
- list_for_each_entry_safe ( mrmrr, tmp, &hd->arch.mapped_rmrrs, list )
+ list_for_each_entry_safe ( mrmrr, tmp, &hd->arch.vtd.mapped_rmrrs, list )
{
list_del(&mrmrr->list);
xfree(mrmrr);
@@ -1751,8 +1754,9 @@ static void iommu_domain_teardown(struct domain *d)
return;
spin_lock(&hd->arch.mapping_lock);
- iommu_free_pagetable(hd->arch.pgd_maddr, agaw_to_level(hd->arch.agaw));
- hd->arch.pgd_maddr = 0;
+ 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);
}
@@ -1892,7 +1896,7 @@ 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.pgd_maddr =
+ dom_iommu(d)->arch.vtd.pgd_maddr =
pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));
}
@@ -1912,7 +1916,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
* No need to acquire hd->arch.mapping_lock: Both insertion and removal
* get done while holding pcidevs_lock.
*/
- list_for_each_entry( mrmrr, &hd->arch.mapped_rmrrs, list )
+ list_for_each_entry( mrmrr, &hd->arch.vtd.mapped_rmrrs, list )
{
if ( mrmrr->base == rmrr->base_address &&
mrmrr->end == rmrr->end_address )
@@ -1959,7 +1963,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
mrmrr->base = rmrr->base_address;
mrmrr->end = rmrr->end_address;
mrmrr->count = 1;
- list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
+ list_add_tail(&mrmrr->list, &hd->arch.vtd.mapped_rmrrs);
return 0;
}
@@ -2657,8 +2661,9 @@ static void vtd_dump_p2m_table(struct domain *d)
return;
hd = dom_iommu(d);
- printk("p2m table has %d levels\n", agaw_to_level(hd->arch.agaw));
- vtd_dump_p2m_table_level(hd->arch.pgd_maddr, agaw_to_level(hd->arch.agaw), 0, 0);
+ 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);
}
static int __init intel_iommu_quarantine_init(struct domain *d)
@@ -2669,7 +2674,7 @@ static int __init intel_iommu_quarantine_init(struct domain *d)
unsigned int level = agaw_to_level(agaw);
int rc;
- if ( hd->arch.pgd_maddr )
+ if ( hd->arch.vtd.pgd_maddr )
{
ASSERT_UNREACHABLE();
return 0;
@@ -2677,11 +2682,11 @@ static int __init intel_iommu_quarantine_init(struct domain *d)
spin_lock(&hd->arch.mapping_lock);
- hd->arch.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
- if ( !hd->arch.pgd_maddr )
+ hd->arch.vtd.pgd_maddr = alloc_pgtable_maddr(1, hd->node);
+ if ( !hd->arch.vtd.pgd_maddr )
goto out;
- parent = map_vtd_domain_page(hd->arch.pgd_maddr);
+ parent = map_vtd_domain_page(hd->arch.vtd.pgd_maddr);
while ( level )
{
uint64_t maddr;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 3d7670e8c6..a12109a1de 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -139,7 +139,6 @@ int arch_iommu_domain_init(struct domain *d)
struct domain_iommu *hd = dom_iommu(d);
spin_lock_init(&hd->arch.mapping_lock);
- INIT_LIST_HEAD(&hd->arch.mapped_rmrrs);
return 0;
}
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 6c9d5e5632..8ce97c981f 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -45,16 +45,23 @@ typedef uint64_t daddr_t;
struct arch_iommu
{
- u64 pgd_maddr; /* io page directory machine address */
- spinlock_t mapping_lock; /* io page table lock */
- int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
- u64 iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
- struct list_head mapped_rmrrs;
-
- /* amd iommu support */
- int paging_mode;
- struct page_info *root_table;
- struct guest_iommu *g_iommu;
+ spinlock_t mapping_lock; /* io page table lock */
+
+ union {
+ /* Intel VT-d */
+ struct {
+ uint64_t pgd_maddr; /* io page directory machine address */
+ unsigned int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
+ uint64_t iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
+ struct list_head mapped_rmrrs;
+ } vtd;
+ /* AMD IOMMU */
+ struct {
+ unsigned int paging_mode;
+ struct page_info *root_table;
+ struct guest_iommu *g_iommu;
+ } amd;
+ };
};
extern struct iommu_ops iommu_ops;
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 01/11] x86/iommu: re-arrange arch_iommu to separate common fields...
2020-08-03 12:29 ` [PATCH v3 01/11] x86/iommu: re-arrange arch_iommu to separate common fields Paul Durrant
@ 2020-08-03 15:53 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-08-03 15:53 UTC (permalink / raw)
To: Paul Durrant
Cc: Kevin Tian, Wei Liu, Andrew Cooper, Paul Durrant,
Lukasz Hawrylko, xen-devel, Roger Pau Monné
On 03.08.2020 14:29, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... from those specific to VT-d or AMD IOMMU, and put the latter in a union.
>
> There is no functional change in this patch, although the initialization of
> the 'mapped_rmrrs' list occurs slightly later in iommu_domain_init() since
> it is now done (correctly) in VT-d specific code rather than in general x86
> code.
>
> NOTE: I have not combined the AMD IOMMU 'root_table' and VT-d 'pgd_maddr'
> fields even though they perform essentially the same function. The
> concept of 'root table' in the VT-d code is different from that in the
> AMD code so attempting to use a common name will probably only serve
> to confuse the reader.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
with a remark (can be taken care of while committing if no other
need for a v4 arises):
> @@ -598,11 +600,12 @@ static void amd_dump_p2m_table(struct domain *d)
> {
> const struct domain_iommu *hd = dom_iommu(d);
>
> - if ( !hd->arch.root_table )
> + if ( !hd->arch.amd.root_table )
> return;
>
> - printk("p2m table has %d levels\n", hd->arch.paging_mode);
> - amd_dump_p2m_table_level(hd->arch.root_table, hd->arch.paging_mode, 0, 0);
> + printk("p2m table has %d levels\n", hd->arch.amd.paging_mode);
> + amd_dump_p2m_table_level(hd->arch.amd.root_table,
> + hd->arch.amd.paging_mode, 0, 0);
At least where you touch the format string anyway, converting %d
to %u where you converted fron plain int to unsigned int would be
nice.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 02/11] x86/iommu: add common page-table allocator
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
2020-08-03 12:29 ` [PATCH v3 01/11] x86/iommu: re-arrange arch_iommu to separate common fields Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 15:58 ` Jan Beulich
2020-08-03 12:29 ` [PATCH v3 03/11] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
` (8 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
Instead of having separate page table allocation functions in VT-d and AMD
IOMMU code, we could use a common allocation function in the general x86 code.
This patch adds a new allocation function, iommu_alloc_pgtable(), for this
purpose. The function adds the page table pages to a list. The pages in this
list are then freed by iommu_free_pgtables(), which is called by
domain_relinquish_resources() after PCI devices have been de-assigned.
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>
v2:
- This is split out from a larger patch of the same name in v1
---
xen/arch/x86/domain.c | 9 +++++-
xen/drivers/passthrough/x86/iommu.c | 50 +++++++++++++++++++++++++++++
xen/include/asm-x86/iommu.h | 7 ++++
3 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f8084dc9e3..d1ecc7b83b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2153,7 +2153,8 @@ int domain_relinquish_resources(struct domain *d)
d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
enum {
- PROG_paging = 1,
+ PROG_iommu_pagetables = 1,
+ PROG_paging,
PROG_vcpu_pagetables,
PROG_shared,
PROG_xen,
@@ -2168,6 +2169,12 @@ int domain_relinquish_resources(struct domain *d)
if ( ret )
return ret;
+ PROGRESS(iommu_pagetables):
+
+ ret = iommu_free_pgtables(d);
+ if ( ret )
+ return ret;
+
PROGRESS(paging):
/* Tear down paging-assistance stuff. */
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index a12109a1de..c0d4865dd7 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -140,6 +140,9 @@ int arch_iommu_domain_init(struct domain *d)
spin_lock_init(&hd->arch.mapping_lock);
+ INIT_PAGE_LIST_HEAD(&hd->arch.pgtables.list);
+ spin_lock_init(&hd->arch.pgtables.lock);
+
return 0;
}
@@ -257,6 +260,53 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
return;
}
+int iommu_free_pgtables(struct domain *d)
+{
+ struct domain_iommu *hd = dom_iommu(d);
+ struct page_info *pg;
+
+ while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
+ {
+ free_domheap_page(pg);
+
+ if ( general_preempt_check() )
+ return -ERESTART;
+ }
+
+ return 0;
+}
+
+struct page_info *iommu_alloc_pgtable(struct domain *d)
+{
+ struct domain_iommu *hd = dom_iommu(d);
+ unsigned int memflags = 0;
+ struct page_info *pg;
+ void *p;
+
+#ifdef CONFIG_NUMA
+ if (hd->node != NUMA_NO_NODE)
+ memflags = MEMF_node(hd->node);
+#endif
+
+ pg = alloc_domheap_page(NULL, memflags);
+ if ( !pg )
+ return NULL;
+
+ p = __map_domain_page(pg);
+ clear_page(p);
+
+ if ( hd->platform_ops->sync_cache )
+ iommu_vcall(hd->platform_ops, sync_cache, p, PAGE_SIZE);
+
+ unmap_domain_page(p);
+
+ spin_lock(&hd->arch.pgtables.lock);
+ page_list_add(pg, &hd->arch.pgtables.list);
+ spin_unlock(&hd->arch.pgtables.lock);
+
+ return pg;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 8ce97c981f..31f6d4a8d8 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -46,6 +46,10 @@ typedef uint64_t daddr_t;
struct arch_iommu
{
spinlock_t mapping_lock; /* io page table lock */
+ struct {
+ struct page_list_head list;
+ spinlock_t lock;
+ } pgtables;
union {
/* Intel VT-d */
@@ -131,6 +135,9 @@ int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
iommu_vcall(ops, sync_cache, addr, size); \
})
+int __must_check iommu_free_pgtables(struct domain *d);
+struct page_info * __must_check iommu_alloc_pgtable(struct domain *d);
+
#endif /* !__ARCH_X86_IOMMU_H__ */
/*
* Local variables:
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 02/11] x86/iommu: add common page-table allocator
2020-08-03 12:29 ` [PATCH v3 02/11] x86/iommu: add common page-table allocator Paul Durrant
@ 2020-08-03 15:58 ` Jan Beulich
2020-08-03 16:17 ` Durrant, Paul
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-08-03 15:58 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Roger Pau Monné, Wei Liu, Andrew Cooper
On 03.08.2020 14:29, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> Instead of having separate page table allocation functions in VT-d and AMD
> IOMMU code, we could use a common allocation function in the general x86 code.
>
> This patch adds a new allocation function, iommu_alloc_pgtable(), for this
> purpose. The function adds the page table pages to a list. The pages in this
> list are then freed by iommu_free_pgtables(), which is called by
> domain_relinquish_resources() after PCI devices have been de-assigned.
>
> 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>
>
> v2:
> - This is split out from a larger patch of the same name in v1
> ---
> xen/arch/x86/domain.c | 9 +++++-
> xen/drivers/passthrough/x86/iommu.c | 50 +++++++++++++++++++++++++++++
> xen/include/asm-x86/iommu.h | 7 ++++
> 3 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f8084dc9e3..d1ecc7b83b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2153,7 +2153,8 @@ int domain_relinquish_resources(struct domain *d)
> d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
>
> enum {
> - PROG_paging = 1,
> + PROG_iommu_pagetables = 1,
> + PROG_paging,
> PROG_vcpu_pagetables,
> PROG_shared,
> PROG_xen,
Is there a particular reason to make this new item the very first
one?
> @@ -257,6 +260,53 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> return;
> }
>
> +int iommu_free_pgtables(struct domain *d)
> +{
> + struct domain_iommu *hd = dom_iommu(d);
> + struct page_info *pg;
> +
> + while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
> + {
> + free_domheap_page(pg);
> +
> + if ( general_preempt_check() )
> + return -ERESTART;
Perhaps better only check once every so many pages?
> +struct page_info *iommu_alloc_pgtable(struct domain *d)
> +{
> + struct domain_iommu *hd = dom_iommu(d);
> + unsigned int memflags = 0;
> + struct page_info *pg;
> + void *p;
> +
> +#ifdef CONFIG_NUMA
> + if (hd->node != NUMA_NO_NODE)
Missing blanks inside parentheses.
> @@ -131,6 +135,9 @@ int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
> iommu_vcall(ops, sync_cache, addr, size); \
> })
>
> +int __must_check iommu_free_pgtables(struct domain *d);
> +struct page_info * __must_check iommu_alloc_pgtable(struct domain *d);
Commonly we put a blank on the left side of *, but none to its right.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH v3 02/11] x86/iommu: add common page-table allocator
2020-08-03 15:58 ` Jan Beulich
@ 2020-08-03 16:17 ` Durrant, Paul
0 siblings, 0 replies; 18+ messages in thread
From: Durrant, Paul @ 2020-08-03 16:17 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 03 August 2020 16:59
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: RE: [EXTERNAL] [PATCH v3 02/11] x86/iommu: add common page-table allocator
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 03.08.2020 14:29, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > Instead of having separate page table allocation functions in VT-d and AMD
> > IOMMU code, we could use a common allocation function in the general x86 code.
> >
> > This patch adds a new allocation function, iommu_alloc_pgtable(), for this
> > purpose. The function adds the page table pages to a list. The pages in this
> > list are then freed by iommu_free_pgtables(), which is called by
> > domain_relinquish_resources() after PCI devices have been de-assigned.
> >
> > 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>
> >
> > v2:
> > - This is split out from a larger patch of the same name in v1
> > ---
> > xen/arch/x86/domain.c | 9 +++++-
> > xen/drivers/passthrough/x86/iommu.c | 50 +++++++++++++++++++++++++++++
> > xen/include/asm-x86/iommu.h | 7 ++++
> > 3 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index f8084dc9e3..d1ecc7b83b 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -2153,7 +2153,8 @@ int domain_relinquish_resources(struct domain *d)
> > d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
> >
> > enum {
> > - PROG_paging = 1,
> > + PROG_iommu_pagetables = 1,
> > + PROG_paging,
> > PROG_vcpu_pagetables,
> > PROG_shared,
> > PROG_xen,
>
> Is there a particular reason to make this new item the very first
> one?
It seems like the logical place as it comes straight after device de-assignment.
>
> > @@ -257,6 +260,53 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> > return;
> > }
> >
> > +int iommu_free_pgtables(struct domain *d)
> > +{
> > + struct domain_iommu *hd = dom_iommu(d);
> > + struct page_info *pg;
> > +
> > + while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) )
> > + {
> > + free_domheap_page(pg);
> > +
> > + if ( general_preempt_check() )
> > + return -ERESTART;
>
> Perhaps better only check once every so many pages?
>
Yes, that would be reasonable.
> > +struct page_info *iommu_alloc_pgtable(struct domain *d)
> > +{
> > + struct domain_iommu *hd = dom_iommu(d);
> > + unsigned int memflags = 0;
> > + struct page_info *pg;
> > + void *p;
> > +
> > +#ifdef CONFIG_NUMA
> > + if (hd->node != NUMA_NO_NODE)
>
> Missing blanks inside parentheses.
>
Oh yes... bad conversion from ternary statement in previous version.
> > @@ -131,6 +135,9 @@ int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
> > iommu_vcall(ops, sync_cache, addr, size); \
> > })
> >
> > +int __must_check iommu_free_pgtables(struct domain *d);
> > +struct page_info * __must_check iommu_alloc_pgtable(struct domain *d);
>
> Commonly we put a blank on the left side of *, but none to its right.
>
Kind of felt wrong not to separate it from '__must_check'.
Paul
> Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 03/11] x86/iommu: convert VT-d code to use new page table allocator
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
2020-08-03 12:29 ` [PATCH v3 01/11] x86/iommu: re-arrange arch_iommu to separate common fields Paul Durrant
2020-08-03 12:29 ` [PATCH v3 02/11] x86/iommu: add common page-table allocator Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 16:03 ` Jan Beulich
2020-08-03 12:29 ` [PATCH v3 04/11] x86/iommu: convert AMD IOMMU " Paul Durrant
` (7 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Kevin Tian, Jan Beulich
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>
---
Cc: 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] 18+ messages in thread* Re: [PATCH v3 03/11] x86/iommu: convert VT-d code to use new page table allocator
2020-08-03 12:29 ` [PATCH v3 03/11] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
@ 2020-08-03 16:03 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-08-03 16:03 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant, Kevin Tian
On 03.08.2020 14:29, Paul Durrant wrote:
> 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'.
I think I disagree here (this is __init code and hence there's no
risk of races), but anyway (as it doesn't hurt) ...
> 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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 04/11] x86/iommu: convert AMD IOMMU code to use new page table allocator
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (2 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 03/11] x86/iommu: convert VT-d code to use new page table allocator Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 16:05 ` Jan Beulich
2020-08-03 12:29 ` [PATCH v3 05/11] iommu: remove unused iommu_ops method and tasklet Paul Durrant
` (6 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
This patch converts the AMD IOMMU code to use the new 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.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v2:
- New in v2 (split from "add common page-table allocator")
---
xen/drivers/passthrough/amd/iommu.h | 18 +----
xen/drivers/passthrough/amd/iommu_map.c | 10 +--
xen/drivers/passthrough/amd/pci_amd_iommu.c | 75 +++------------------
3 files changed, 16 insertions(+), 87 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 3489c2a015..e2d174f3b4 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -226,7 +226,7 @@ int __must_check amd_iommu_map_page(struct domain *d, dfn_t dfn,
unsigned int *flush_flags);
int __must_check amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
unsigned int *flush_flags);
-int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
+int __must_check amd_iommu_alloc_root(struct domain *d);
int amd_iommu_reserve_domain_unity_map(struct domain *domain,
paddr_t phys_addr, unsigned long size,
int iw, int ir);
@@ -356,22 +356,6 @@ static inline int amd_iommu_get_paging_mode(unsigned long max_frames)
return level;
}
-static inline struct page_info *alloc_amd_iommu_pgtable(void)
-{
- struct page_info *pg = alloc_domheap_page(NULL, 0);
-
- if ( pg )
- clear_domain_page(page_to_mfn(pg));
-
- return pg;
-}
-
-static inline void free_amd_iommu_pgtable(struct page_info *pg)
-{
- if ( pg )
- free_domheap_page(pg);
-}
-
static inline void *__alloc_amd_iommu_tables(unsigned int order)
{
return alloc_xenheap_pages(order, 0);
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 47b4472e8a..54b991294a 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -217,7 +217,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
mfn = next_table_mfn;
/* allocate lower level page table */
- table = alloc_amd_iommu_pgtable();
+ table = iommu_alloc_pgtable(d);
if ( table == NULL )
{
AMD_IOMMU_DEBUG("Cannot allocate I/O page table\n");
@@ -248,7 +248,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
if ( next_table_mfn == 0 )
{
- table = alloc_amd_iommu_pgtable();
+ table = iommu_alloc_pgtable(d);
if ( table == NULL )
{
AMD_IOMMU_DEBUG("Cannot allocate I/O page table\n");
@@ -286,7 +286,7 @@ int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
spin_lock(&hd->arch.mapping_lock);
- rc = amd_iommu_alloc_root(hd);
+ rc = amd_iommu_alloc_root(d);
if ( rc )
{
spin_unlock(&hd->arch.mapping_lock);
@@ -458,7 +458,7 @@ int __init amd_iommu_quarantine_init(struct domain *d)
spin_lock(&hd->arch.mapping_lock);
- hd->arch.amd.root_table = alloc_amd_iommu_pgtable();
+ hd->arch.amd.root_table = iommu_alloc_pgtable(d);
if ( !hd->arch.amd.root_table )
goto out;
@@ -473,7 +473,7 @@ int __init amd_iommu_quarantine_init(struct domain *d)
* page table pages, and the resulting allocations are always
* zeroed.
*/
- pg = alloc_amd_iommu_pgtable();
+ pg = iommu_alloc_pgtable(d);
if ( !pg )
break;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index c27bfbd48e..d79668f948 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -205,11 +205,13 @@ static int iov_enable_xt(void)
return 0;
}
-int amd_iommu_alloc_root(struct domain_iommu *hd)
+int amd_iommu_alloc_root(struct domain *d)
{
+ struct domain_iommu *hd = dom_iommu(d);
+
if ( unlikely(!hd->arch.amd.root_table) )
{
- hd->arch.amd.root_table = alloc_amd_iommu_pgtable();
+ hd->arch.amd.root_table = iommu_alloc_pgtable(d);
if ( !hd->arch.amd.root_table )
return -ENOMEM;
}
@@ -217,12 +219,13 @@ int amd_iommu_alloc_root(struct domain_iommu *hd)
return 0;
}
-static int __must_check allocate_domain_resources(struct domain_iommu *hd)
+static int __must_check allocate_domain_resources(struct domain *d)
{
+ struct domain_iommu *hd = dom_iommu(d);
int rc;
spin_lock(&hd->arch.mapping_lock);
- rc = amd_iommu_alloc_root(hd);
+ rc = amd_iommu_alloc_root(d);
spin_unlock(&hd->arch.mapping_lock);
return rc;
@@ -254,7 +257,7 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
{
const struct amd_iommu *iommu;
- if ( allocate_domain_resources(dom_iommu(d)) )
+ if ( allocate_domain_resources(d) )
BUG();
for_each_amd_iommu ( iommu )
@@ -323,7 +326,6 @@ static int reassign_device(struct domain *source, struct domain *target,
{
struct amd_iommu *iommu;
int bdf, rc;
- struct domain_iommu *t = dom_iommu(target);
bdf = PCI_BDF2(pdev->bus, pdev->devfn);
iommu = find_iommu_for_device(pdev->seg, bdf);
@@ -344,7 +346,7 @@ static int reassign_device(struct domain *source, struct domain *target,
pdev->domain = target;
}
- rc = allocate_domain_resources(t);
+ rc = allocate_domain_resources(target);
if ( rc )
return rc;
@@ -376,65 +378,9 @@ static int amd_iommu_assign_device(struct domain *d, u8 devfn,
return reassign_device(pdev->domain, d, devfn, pdev);
}
-static void deallocate_next_page_table(struct page_info *pg, int level)
-{
- 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 deallocate_page_table(struct page_info *pg)
-{
- struct amd_iommu_pte *table_vaddr;
- unsigned int index, level = PFN_ORDER(pg);
-
- PFN_ORDER(pg) = 0;
-
- if ( level <= 1 )
- {
- free_amd_iommu_pgtable(pg);
- return;
- }
-
- table_vaddr = __map_domain_page(pg);
-
- for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
- {
- struct amd_iommu_pte *pde = &table_vaddr[index];
-
- if ( pde->mfn && pde->next_level && pde->pr )
- {
- /* We do not support skip levels yet */
- ASSERT(pde->next_level == level - 1);
- deallocate_next_page_table(mfn_to_page(_mfn(pde->mfn)),
- pde->next_level);
- }
- }
-
- unmap_domain_page(table_vaddr);
- free_amd_iommu_pgtable(pg);
-}
-
-static void deallocate_iommu_page_tables(struct domain *d)
-{
- struct domain_iommu *hd = dom_iommu(d);
-
- spin_lock(&hd->arch.mapping_lock);
- if ( hd->arch.amd.root_table )
- {
- deallocate_next_page_table(hd->arch.amd.root_table,
- hd->arch.amd.paging_mode);
- hd->arch.amd.root_table = NULL;
- }
- spin_unlock(&hd->arch.mapping_lock);
-}
-
-
static void amd_iommu_domain_destroy(struct domain *d)
{
- deallocate_iommu_page_tables(d);
- amd_iommu_flush_all_pages(d);
+ dom_iommu(d)->arch.amd.root_table = NULL;
}
static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev)
@@ -620,7 +566,6 @@ static const struct iommu_ops __initconstrel _iommu_ops = {
.unmap_page = amd_iommu_unmap_page,
.iotlb_flush = amd_iommu_flush_iotlb_pages,
.iotlb_flush_all = amd_iommu_flush_iotlb_all,
- .free_page_table = deallocate_page_table,
.reassign_device = reassign_device,
.get_device_group_id = amd_iommu_group_id,
.enable_x2apic = iov_enable_xt,
--
2.20.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 04/11] x86/iommu: convert AMD IOMMU code to use new page table allocator
2020-08-03 12:29 ` [PATCH v3 04/11] x86/iommu: convert AMD IOMMU " Paul Durrant
@ 2020-08-03 16:05 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-08-03 16:05 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant, Andrew Cooper
On 03.08.2020 14:29, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> This patch converts the AMD IOMMU code to use the new 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.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 05/11] iommu: remove unused iommu_ops method and tasklet
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (3 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 04/11] x86/iommu: convert AMD IOMMU " Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 16:06 ` Jan Beulich
2020-08-03 12:29 ` [PATCH v3 06/11] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
` (5 subsequent siblings)
10 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
The VT-d and AMD IOMMU 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>
---
Cc: 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] 18+ messages in thread* [PATCH v3 06/11] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (4 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 05/11] iommu: remove unused iommu_ops method and tasklet Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 12:29 ` [PATCH v3 07/11] iommu: make map, unmap and flush all take both an order and a count Paul Durrant
` (4 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 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>
v2:
- New in v2
---
xen/drivers/passthrough/iommu.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 660dc5deb2..e2c0193a09 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -274,6 +274,10 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
break;
}
+ /* Something went wrong so flush everything and clear flush flags */
+ if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
+ flush_flags = 0;
+
return rc;
}
@@ -283,14 +287,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 +328,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
}
}
+ /* Something went wrong so flush everything and clear flush flags */
+ if ( unlikely(rc) && iommu_iotlb_flush_all(d, *flush_flags) )
+ flush_flags = 0;
+
return rc;
}
@@ -338,14 +340,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] 18+ messages in thread* [PATCH v3 07/11] iommu: make map, unmap and flush all take both an order and a count
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (5 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 06/11] iommu: flush I/O TLB if iommu_map() or iommu_unmap() fail Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 12:29 ` [PATCH v3 08/11] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
` (3 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima,
Wei Liu, Andrew Cooper, Paul Durrant, Ian Jackson, George Dunlap,
Jan Beulich, Volodymyr Babchuk, Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
At the moment iommu_map() and iommu_unmap() take a page order but not a
count, whereas iommu_iotlb_flush() takes a count but not a page order.
This patch simply makes them consistent with each other.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.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: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
v2:
- New in v2
---
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/mm/p2m-ept.c | 2 +-
xen/common/memory.c | 4 +--
xen/drivers/passthrough/amd/iommu.h | 2 +-
xen/drivers/passthrough/amd/iommu_map.c | 4 +--
xen/drivers/passthrough/arm/ipmmu-vmsa.c | 2 +-
xen/drivers/passthrough/arm/smmu.c | 2 +-
xen/drivers/passthrough/iommu.c | 31 ++++++++++++------------
xen/drivers/passthrough/vtd/iommu.c | 4 +--
xen/drivers/passthrough/x86/iommu.c | 2 +-
xen/include/xen/iommu.h | 9 ++++---
11 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ce59f2b503..71f4a78425 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1061,7 +1061,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
flush_flags |= IOMMU_FLUSHF_added;
rc = iommu_iotlb_flush(p2m->domain, _dfn(gfn_x(sgfn)),
- 1UL << page_order, flush_flags);
+ page_order, 1, flush_flags);
}
else
rc = 0;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b8154a7ecc..b2ac912cde 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -843,7 +843,7 @@ 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), (1u << order), 1,
(iommu_flags ? IOMMU_FLUSHF_added : 0) |
(vtd_pte_present ? IOMMU_FLUSHF_modified
: 0));
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 714077c1e5..8de334ff10 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -851,12 +851,12 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
this_cpu(iommu_dont_flush_iotlb) = 0;
- ret = iommu_iotlb_flush(d, _dfn(xatp->idx - done), done,
+ ret = iommu_iotlb_flush(d, _dfn(xatp->idx - done), 0, done,
IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
if ( unlikely(ret) && rc >= 0 )
rc = ret;
- ret = iommu_iotlb_flush(d, _dfn(xatp->gpfn - done), done,
+ ret = iommu_iotlb_flush(d, _dfn(xatp->gpfn - done), 0, done,
IOMMU_FLUSHF_added | IOMMU_FLUSHF_modified);
if ( unlikely(ret) && rc >= 0 )
rc = ret;
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..0cb948d114 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -351,7 +351,7 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
return 0;
}
-static unsigned long flush_count(unsigned long dfn, unsigned int page_count,
+static unsigned long flush_count(unsigned long dfn, unsigned long page_count,
unsigned int order)
{
unsigned long start = dfn >> order;
@@ -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/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
index b2a65dfaaf..346165c3fa 100644
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -945,7 +945,7 @@ static int __must_check ipmmu_iotlb_flush_all(struct domain *d)
}
static int __must_check ipmmu_iotlb_flush(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags)
{
ASSERT(flush_flags);
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 94662a8501..06f9bda47d 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2534,7 +2534,7 @@ static int __must_check arm_smmu_iotlb_flush_all(struct domain *d)
}
static int __must_check arm_smmu_iotlb_flush(struct domain *d, dfn_t dfn,
- unsigned int page_count,
+ unsigned long page_count,
unsigned int flush_flags)
{
ASSERT(flush_flags);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index e2c0193a09..568a4a5661 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -235,8 +235,8 @@ 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 int *flush_flags)
+ unsigned int page_order, unsigned int page_count,
+ unsigned int flags, unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
unsigned long i;
@@ -248,7 +248,7 @@ int iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
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 < ((unsigned long)page_count << page_order); i++ )
{
rc = iommu_call(hd->platform_ops, map_page, d, dfn_add(dfn, i),
mfn_add(mfn, i), flags, flush_flags);
@@ -285,16 +285,16 @@ int iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned int page_order, 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_order, 1, 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, (1u << page_order), 1, flush_flags);
return rc;
}
int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
- unsigned int *flush_flags)
+ unsigned int page_count, unsigned int *flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
unsigned long i;
@@ -305,7 +305,7 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
ASSERT(IS_ALIGNED(dfn_x(dfn), (1ul << page_order)));
- for ( i = 0; i < (1ul << page_order); i++ )
+ for ( i = 0; i < ((unsigned long)page_count << page_order); i++ )
{
int err = iommu_call(hd->platform_ops, unmap_page, d, dfn_add(dfn, i),
flush_flags);
@@ -338,10 +338,10 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
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);
+ int rc = iommu_unmap(d, dfn, page_order, 1, &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, (1u << page_order), 1, flush_flags);
return rc;
}
@@ -357,8 +357,8 @@ 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,
- unsigned int flush_flags)
+int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_order,
+ unsigned int page_count, unsigned int flush_flags)
{
const struct domain_iommu *hd = dom_iommu(d);
int rc;
@@ -370,14 +370,15 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_count,
if ( dfn_eq(dfn, INVALID_DFN) )
return -EINVAL;
- rc = iommu_call(hd->platform_ops, iotlb_flush, d, dfn, page_count,
- flush_flags);
+ rc = iommu_call(hd->platform_ops, iotlb_flush, d, dfn,
+ (unsigned long)page_count << page_order, flush_flags);
if ( unlikely(rc) )
{
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->domain_id, rc, dfn_x(dfn), page_count, flush_flags);
+ "d%d: IOMMU IOTLB flush failed: %d, dfn %"PRI_dfn", page order %u, page count %u flags %x\n",
+ d->domain_id, rc, dfn_x(dfn), page_order, page_count,
+ flush_flags);
if ( !is_hardware_domain(d) )
domain_crash(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 c0d4865dd7..5d1a7cb296 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), PAGE_ORDER_4K, 1,
IOMMUF_readable | IOMMUF_writable, &flush_flags);
if ( rc )
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 1831dc66b0..d9c2e764aa 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -146,10 +146,10 @@ 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 int *flush_flags);
+ unsigned int page_order, unsigned int 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 int page_order, unsigned int page_count,
unsigned int *flush_flags);
int __must_check iommu_legacy_map(struct domain *d, dfn_t dfn, mfn_t mfn,
@@ -162,6 +162,7 @@ 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_order,
unsigned int page_count,
unsigned int flush_flags);
int __must_check iommu_iotlb_flush_all(struct domain *d,
@@ -281,7 +282,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] 18+ messages in thread* [PATCH v3 08/11] remove remaining uses of iommu_legacy_map/unmap
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (6 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 07/11] iommu: make map, unmap and flush all take both an order and a count Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 12:29 ` [PATCH v3 09/11] common/grant_table: batch flush I/O TLB Paul Durrant
` (2 subsequent siblings)
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Julien Grall, Jun Nakajima,
Wei Liu, Andrew Cooper, Paul Durrant, Ian Jackson, George Dunlap,
Jan Beulich, Roger Pau Monné
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(). Also, it 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 fails if the number of pages passed to
a single call overflows an unsigned int. I don't believe this will
ever happen in practice.
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>
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 | 21 +++++++++++++++-----
xen/arch/x86/mm/p2m-ept.c | 20 +++++++++++--------
xen/arch/x86/mm/p2m-pt.c | 15 +++++++++++----
xen/arch/x86/mm/p2m.c | 26 ++++++++++++++++++-------
xen/arch/x86/x86_64/mm.c | 27 +++++++++++++-------------
xen/common/grant_table.c | 34 ++++++++++++++++++++++++---------
xen/common/memory.c | 5 +++--
xen/drivers/passthrough/iommu.c | 25 +-----------------------
xen/include/xen/iommu.h | 21 +++++---------------
9 files changed, 106 insertions(+), 88 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 638f6bf580..062af1f684 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2446,10 +2446,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), PAGE_ORDER_4K, 1, &flush_flags);
if ( !rc )
- rc = rc2;
+ rc = err;
+
+ err = iommu_iotlb_flush(d, _dfn(mfn), PAGE_ORDER_4K, 1, flush_flags);
+ if ( !rc )
+ rc = err;
}
if ( likely(!is_special_page(page)) )
@@ -2971,12 +2977,17 @@ 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)), PAGE_ORDER_4K);
+ rc = iommu_unmap(d, dfn, PAGE_ORDER_4K, 1, &flush_flags);
else
- rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn, PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable);
+ rc = iommu_map(d, dfn, mfn, PAGE_ORDER_4K, 1,
+ IOMMUF_readable | IOMMUF_writable, &flush_flags);
+
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, dfn, PAGE_ORDER_4K, 1, flush_flags);
if ( unlikely(rc) )
{
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b2ac912cde..e38b0bf95c 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -842,15 +842,19 @@ 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), (1u << order), 1,
- (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, order, iommu_flags) :
- iommu_legacy_unmap(d, _dfn(gfn), order);
+ iommu_map(d, _dfn(gfn), mfn, order, 1, iommu_flags, &flush_flags) :
+ iommu_unmap(d, _dfn(gfn), order, 1, &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), order, 1, 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 badb26bc34..3c0901b56c 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -678,10 +678,17 @@ 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_pte_flags)
- : iommu_legacy_unmap(d, _dfn(gfn), page_order);
+ {
+ unsigned int flush_flags = 0;
+
+ rc = iommu_pte_flags ?
+ iommu_map(d, _dfn(gfn), mfn, page_order, 1, iommu_pte_flags,
+ &flush_flags) :
+ iommu_unmap(d, _dfn(gfn), page_order, 1, &flush_flags);
+
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, _dfn(gfn), page_order, 1, 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 db7bde0230..9f8b9bc5fd 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1350,10 +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), PAGE_ORDER_4K,
- IOMMUF_readable | IOMMUF_writable);
+ unsigned int flush_flags = 0;
+
+ ret = iommu_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K, 1,
+ IOMMUF_readable | IOMMUF_writable, &flush_flags);
+ if ( !ret )
+ ret = iommu_iotlb_flush(d, _dfn(gfn_l), PAGE_ORDER_4K, 1,
+ flush_flags);
+
+ return ret;
}
gfn_lock(p2m, gfn, 0);
@@ -1441,9 +1446,16 @@ 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), PAGE_ORDER_4K);
+ unsigned int flush_flags = 0;
+ int err;
+
+ ret = iommu_unmap(d, _dfn(gfn_l), PAGE_ORDER_4K, 1, &flush_flags);
+
+ err = iommu_iotlb_flush(d, _dfn(gfn_l), PAGE_ORDER_4K, 1, flush_flags);
+ if ( !ret )
+ ret = err;
+
+ 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 102079a801..02684bcf9d 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1413,21 +1413,22 @@ 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),
- 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),
- PAGE_ORDER_4K) )
- continue;
+ unsigned int flush_flags = 0;
+ unsigned int n = epfn - spfn;
+ int rc;
+ ret = -EOVERFLOW;
+ if ( spfn + n != epfn )
+ goto destroy_m2p;
+
+ rc = iommu_map(hardware_domain, _dfn(i), _mfn(i),
+ PAGE_ORDER_4K, n, IOMMUF_readable | IOMMUF_writable,
+ &flush_flags);
+ if ( !rc )
+ rc = iommu_iotlb_flush(hardware_domain, _dfn(i), PAGE_ORDER_4K, n,
+ flush_flags);
+ if ( rc )
goto destroy_m2p;
- }
}
/* We can't revert any more */
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 9f0cae52c0..d6526bca12 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1225,11 +1225,23 @@ map_grant_ref(
kind = IOMMUF_readable;
else
kind = 0;
- if ( kind && iommu_legacy_map(ld, _dfn(mfn_x(mfn)), mfn, 0, 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, 0, 1, kind, &flush_flags);
+ if ( !err )
+ err = iommu_iotlb_flush(ld, dfn, 0, 1, flush_flags);
+ if ( err )
+ rc = GNTST_general_error;
+
+ if ( rc != GNTST_okay )
+ {
+ double_gt_unlock(lgt, rgt);
+ goto undo_out;
+ }
}
}
@@ -1473,21 +1485,25 @@ 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)), 0);
+ err = iommu_unmap(ld, dfn, 0, 1, &flush_flags);
else if ( !(kind & MAPKIND_WRITE) )
- err = iommu_legacy_map(ld, _dfn(mfn_x(op->mfn)), op->mfn, 0,
- IOMMUF_readable);
-
- double_gt_unlock(lgt, rgt);
+ err = iommu_map(ld, dfn, op->mfn, 0, 1, IOMMUF_readable,
+ &flush_flags);
+ if ( !err )
+ err = iommu_iotlb_flush(ld, dfn, 0, 1, flush_flags);
if ( err )
rc = GNTST_general_error;
+
+ double_gt_unlock(lgt, rgt);
}
/* If just unmapped a writable mapping, mark as dirtied */
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 8de334ff10..2891bef57b 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,6 +844,8 @@ 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;
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 568a4a5661..ab44c332bb 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -281,18 +281,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 int page_order, unsigned int flags)
-{
- unsigned int flush_flags = 0;
- int rc = iommu_map(d, dfn, mfn, page_order, 1, flags, &flush_flags);
-
- if ( !this_cpu(iommu_dont_flush_iotlb) && !rc )
- rc = iommu_iotlb_flush(d, dfn, (1u << page_order), 1, flush_flags);
-
- return rc;
-}
-
int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
unsigned int page_count, unsigned int *flush_flags)
{
@@ -335,17 +323,6 @@ int iommu_unmap(struct domain *d, dfn_t dfn, unsigned int page_order,
return rc;
}
-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, 1, &flush_flags);
-
- if ( !this_cpu(iommu_dont_flush_iotlb) && ! rc )
- rc = iommu_iotlb_flush(d, dfn, (1u << page_order), 1, flush_flags);
-
- return rc;
-}
-
int iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t *mfn,
unsigned int *flags)
{
@@ -364,7 +341,7 @@ int iommu_iotlb_flush(struct domain *d, dfn_t dfn, unsigned int page_order,
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 d9c2e764aa..b7e5d3da09 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 int page_order, unsigned int 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 int flags);
-int __must_check iommu_legacy_unmap(struct domain *d, dfn_t dfn,
- unsigned int page_order);
-
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_order,
unsigned int page_count,
@@ -370,15 +362,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 unecessary 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] 18+ messages in thread* [PATCH v3 09/11] common/grant_table: batch flush I/O TLB
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (7 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 08/11] remove remaining uses of iommu_legacy_map/unmap Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 12:29 ` [PATCH v3 10/11] iommu: remove the share_p2m operation Paul Durrant
2020-08-03 12:29 ` [PATCH v3 11/11] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Paul Durrant, Ian Jackson, George Dunlap, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
This patch avoids calling iommu_iotlb_flush() for each individual GNTTABOP and
insteads calls iommu_iotlb_flush_all() at the end of the hypercall. This
should mean batched map/unmap operations perform better but may be slightly
detrimental to singleton performance.
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>
v3:
- New in v3
---
xen/common/grant_table.c | 132 ++++++++++++++++++++++++---------------
1 file changed, 80 insertions(+), 52 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d6526bca12..f382e0be52 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -979,7 +979,7 @@ static unsigned int mapkind(
static void
map_grant_ref(
- struct gnttab_map_grant_ref *op)
+ struct gnttab_map_grant_ref *op, unsigned int *flush_flags)
{
struct domain *ld, *rd, *owner = NULL;
struct grant_table *lgt, *rgt;
@@ -1228,17 +1228,12 @@ 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, 0, 1, kind, &flush_flags);
- if ( !err )
- err = iommu_iotlb_flush(ld, dfn, 0, 1, flush_flags);
+ err = iommu_map(ld, dfn, mfn, 0, 1, kind, flush_flags);
if ( err )
- rc = GNTST_general_error;
-
- if ( rc != GNTST_okay )
{
+ rc = GNTST_general_error;
double_gt_unlock(lgt, rgt);
goto undo_out;
}
@@ -1322,6 +1317,8 @@ gnttab_map_grant_ref(
{
int i;
struct gnttab_map_grant_ref op;
+ unsigned int flush_flags = 0;
+ int err, rc = 0;
for ( i = 0; i < count; i++ )
{
@@ -1329,20 +1326,30 @@ gnttab_map_grant_ref(
return i;
if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) )
- return -EFAULT;
+ {
+ rc = -EFAULT;
+ break;
+ }
- map_grant_ref(&op);
+ map_grant_ref(&op, &flush_flags);
if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) )
- return -EFAULT;
+ {
+ rc = -EFAULT;
+ break;
+ }
}
- return 0;
+ err = iommu_iotlb_flush_all(current->domain, flush_flags);
+ if ( !rc )
+ rc = err;
+
+ return rc;
}
static void
unmap_common(
- struct gnttab_unmap_common *op)
+ struct gnttab_unmap_common *op, unsigned int *flush_flags)
{
domid_t dom;
struct domain *ld, *rd;
@@ -1486,20 +1493,16 @@ 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, 0, 1, &flush_flags);
+ err = iommu_unmap(ld, dfn, 0, 1, flush_flags);
else if ( !(kind & MAPKIND_WRITE) )
err = iommu_map(ld, dfn, op->mfn, 0, 1, IOMMUF_readable,
- &flush_flags);
-
- if ( !err )
- err = iommu_iotlb_flush(ld, dfn, 0, 1, flush_flags);
+ flush_flags);
if ( err )
rc = GNTST_general_error;
@@ -1600,8 +1603,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,
+ unsigned int *flush_flags)
{
common->host_addr = op->host_addr;
common->dev_bus_addr = op->dev_bus_addr;
@@ -1613,7 +1616,7 @@ unmap_grant_ref(
common->rd = NULL;
common->mfn = INVALID_MFN;
- unmap_common(common);
+ unmap_common(common, flush_flags);
op->status = common->status;
}
@@ -1622,31 +1625,50 @@ 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 domain *currd = current->domain;
struct gnttab_unmap_grant_ref op;
struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ int rc = 0;
while ( count != 0 )
{
+ unsigned int i, c, partial_done = 0, done = 0;
+ unsigned int flush_flags = 0;
+ int err;
+
c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
- partial_done = 0;
for ( i = 0; i < c; i++ )
{
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], &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);
+ 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;
@@ -1654,20 +1676,14 @@ 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_common *common,
+ unsigned int *flush_flags)
{
common->host_addr = op->host_addr;
common->new_addr = op->new_addr;
@@ -1679,7 +1695,7 @@ unmap_and_replace(
common->rd = NULL;
common->mfn = INVALID_MFN;
- unmap_common(common);
+ unmap_common(common, flush_flags);
op->status = common->status;
}
@@ -1687,31 +1703,50 @@ 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 domain *currd = current->domain;
struct gnttab_unmap_and_replace op;
struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE];
+ int rc = 0;
while ( count != 0 )
{
+ unsigned int i, c, partial_done = 0, done = 0;
+ unsigned int flush_flags = 0;
+ int err;
+
c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE);
- partial_done = 0;
for ( i = 0; i < c; i++ )
{
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], &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);
+ 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;
@@ -1719,14 +1754,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] 18+ messages in thread* [PATCH v3 10/11] iommu: remove the share_p2m operation
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (8 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 09/11] common/grant_table: batch flush I/O TLB Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
2020-08-03 12:29 ` [PATCH v3 11/11] iommu: stop calling IOMMU page tables 'p2m tables' Paul Durrant
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Wei Liu, Andrew Cooper, Paul Durrant, George Dunlap,
Jan Beulich, Roger Pau Monné
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.
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>
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/iommu.c | 90 ++++++++++++++++-------------
xen/include/xen/iommu.h | 3 -
4 files changed, 50 insertions(+), 54 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9f8b9bc5fd..3bd8d83d23 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 ab44c332bb..7464f10d1c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -498,14 +498,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/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 68cf0e535a..a532d9e88c 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, struct vtd_iommu *iommu)
+{
+ 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(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 )
+ 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);
+ 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/include/xen/iommu.h b/xen/include/xen/iommu.h
index b7e5d3da09..1f25d2082f 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -271,7 +271,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,
@@ -348,8 +347,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] 18+ messages in thread* [PATCH v3 11/11] iommu: stop calling IOMMU page tables 'p2m tables'
2020-08-03 12:29 [PATCH v3 00/11] IOMMU cleanup Paul Durrant
` (9 preceding siblings ...)
2020-08-03 12:29 ` [PATCH v3 10/11] iommu: remove the share_p2m operation Paul Durrant
@ 2020-08-03 12:29 ` Paul Durrant
10 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2020-08-03 12:29 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Paul Durrant, Kevin Tian, Jan Beulich, Paul Durrant
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.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Kevin Tian <kevin.tian@intel.com>
v2:
- Moved all output into implementation specific code
---
xen/drivers/passthrough/amd/pci_amd_iommu.c | 16 ++++++-------
xen/drivers/passthrough/iommu.c | 21 ++++-------------
xen/drivers/passthrough/vtd/iommu.c | 26 +++++++++++----------
xen/include/xen/iommu.h | 2 +-
4 files changed, 28 insertions(+), 37 deletions(-)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index d79668f948..b3e95cf18e 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;
@@ -529,7 +529,7 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
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 %d 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 table has %d levels\n", 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 7464f10d1c..0f468379e1 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);
}
@@ -533,16 +533,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();
@@ -553,14 +549,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 a532d9e88c..f8da4fe0e7 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;
@@ -2612,8 +2612,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 +2624,19 @@ 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("VT-D sharing EPT table\n");
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("VT-D table has %d levels\n", 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 +2736,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 1f25d2082f..23e884f54b 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -277,7 +277,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] 18+ messages in thread