[PATCH v3] amd-iommu: remove page merging code

* [PATCH v3] amd-iommu: remove page merging code
@ 2018-11-28  9:55 Paul Durrant
  2018-11-28 10:41 ` Andrew Cooper
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Paul Durrant @ 2018-11-28  9:55 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Brian Woods, Suravee Suthikulpanit

The page merging logic makes use of bits 1-8 and bit 63 of a PTE, which
used to be specified as 'ignored'. However, bits 5 and 6 are now specified
as 'accessed' and 'dirty' bits and their use only remains safe as long as
the DTE 'Host Access Dirty' bits remain unused by Xen, or by hardware
before the domain starts running. (XSA-275 disabled the operation of the
code after domain creation completes).

With the page merging logic present in its current form there are no spare
ignored bits in the PTE at all, but PV-IOMMU support will require at least
one spare bit to track which PTEs are added by hypercall.

This patch removes the code, freeing up the remaining PTE ignored bits
for other use, including PV-IOMMU support, as well as significantly
simplifying and shortening the source by ~170 lines. There may be some
marginal performance cost (but none has been observed in manual testing
with a passed-through NVIDIA GPU) since higher order mappings will now be
ruled out until a mapping order parameter is passed to iommu_ops. That will
be dealt with by a subsequent patch though.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>

v3:
 - Further expand commit comment

v2:
 - Remove 'no_merge' boolean
 - Expand commit comment
---
 xen/drivers/passthrough/amd/iommu_map.c | 175 +-------------------------------
 xen/include/asm-x86/iommu.h             |   1 -
 2 files changed, 1 insertion(+), 175 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 0ac3f473b3..04cb7b3182 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -323,134 +323,6 @@ uint64_t amd_iommu_get_address_from_pte(void *pte)
     return ptr;
 }
 
-/* For each pde, We use ignored bits (bit 1 - bit 8 and bit 63)
- * to save pde count, pde count = 511 is a candidate of page coalescing.
- */
-static unsigned int get_pde_count(uint64_t pde)
-{
-    unsigned int count;
-    uint64_t upper_mask = 1ULL << 63 ;
-    uint64_t lower_mask = 0xFF << 1;
-
-    count = ((pde & upper_mask) >> 55) | ((pde & lower_mask) >> 1);
-    return count;
-}
-
-/* Convert pde count into iommu pte ignored bits */
-static void set_pde_count(uint64_t *pde, unsigned int count)
-{
-    uint64_t upper_mask = 1ULL << 8 ;
-    uint64_t lower_mask = 0xFF;
-    uint64_t pte_mask = (~(1ULL << 63)) & (~(0xFF << 1));
-
-    *pde &= pte_mask;
-    *pde |= ((count & upper_mask ) << 55) | ((count & lower_mask ) << 1);
-}
-
-/* Return 1, if pages are suitable for merging at merge_level.
- * otherwise increase pde count if mfn is contigous with mfn - 1
- */
-static bool iommu_update_pde_count(struct domain *d, unsigned long pt_mfn,
-                                   unsigned long dfn, unsigned long mfn,
-                                   unsigned int merge_level)
-{
-    unsigned int pde_count, next_level;
-    unsigned long first_mfn;
-    uint64_t *table, *pde, *ntable;
-    uint64_t ntable_maddr, mask;
-    struct domain_iommu *hd = dom_iommu(d);
-    bool ok = false;
-
-    ASSERT( spin_is_locked(&hd->arch.mapping_lock) && pt_mfn );
-
-    next_level = merge_level - 1;
-
-    /* get pde at merge level */
-    table = map_domain_page(_mfn(pt_mfn));
-    pde = table + pfn_to_pde_idx(dfn, merge_level);
-
-    /* get page table of next level */
-    ntable_maddr = amd_iommu_get_address_from_pte(pde);
-    ntable = map_domain_page(_mfn(paddr_to_pfn(ntable_maddr)));
-
-    /* get the first mfn of next level */
-    first_mfn = amd_iommu_get_address_from_pte(ntable) >> PAGE_SHIFT;
-
-    if ( first_mfn == 0 )
-        goto out;
-
-    mask = (1ULL<< (PTE_PER_TABLE_SHIFT * next_level)) - 1;
-
-    if ( ((first_mfn & mask) == 0) &&
-         (((dfn & mask) | first_mfn) == mfn) )
-    {
-        pde_count = get_pde_count(*pde);
-
-        if ( pde_count == (PTE_PER_TABLE_SIZE - 1) )
-            ok = true;
-        else if ( pde_count < (PTE_PER_TABLE_SIZE - 1))
-        {
-            pde_count++;
-            set_pde_count(pde, pde_count);
-        }
-    }
-
-    else
-        /* non-contiguous mapping */
-        set_pde_count(pde, 0);
-
-out:
-    unmap_domain_page(ntable);
-    unmap_domain_page(table);
-
-    return ok;
-}
-
-static int iommu_merge_pages(struct domain *d, unsigned long pt_mfn,
-                             unsigned long dfn, unsigned int flags,
-                             unsigned int merge_level)
-{
-    uint64_t *table, *pde, *ntable;
-    uint64_t ntable_mfn;
-    unsigned long first_mfn;
-    struct domain_iommu *hd = dom_iommu(d);
-
-    ASSERT( spin_is_locked(&hd->arch.mapping_lock) && pt_mfn );
-
-    table = map_domain_page(_mfn(pt_mfn));
-    pde = table + pfn_to_pde_idx(dfn, merge_level);
-
-    /* get first mfn */
-    ntable_mfn = amd_iommu_get_address_from_pte(pde) >> PAGE_SHIFT;
-
-    if ( ntable_mfn == 0 )
-    {
-        unmap_domain_page(table);
-        return 1;
-    }
-
-    ntable = map_domain_page(_mfn(ntable_mfn));
-    first_mfn = amd_iommu_get_address_from_pte(ntable) >> PAGE_SHIFT;
-
-    if ( first_mfn == 0 )
-    {
-        unmap_domain_page(ntable);
-        unmap_domain_page(table);
-        return 1;
-    }
-
-    /* setup super page mapping, next level = 0 */
-    set_iommu_pde_present((uint32_t *)pde, first_mfn, 0,
-                          !!(flags & IOMMUF_writable),
-                          !!(flags & IOMMUF_readable));
-
-    amd_iommu_flush_all_pages(d);
-
-    unmap_domain_page(ntable);
-    unmap_domain_page(table);
-    return 0;
-}
-
 /* Walk io page tables and build level page tables if necessary
  * {Re, un}mapping super page frames causes re-allocation of io
  * page tables.
@@ -656,7 +528,6 @@ int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
     struct domain_iommu *hd = dom_iommu(d);
     int rc;
     unsigned long pt_mfn[7];
-    unsigned int merge_level;
 
     if ( iommu_use_hap_pt(d) )
         return 0;
@@ -698,55 +569,14 @@ int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
         return -EFAULT;
     }
 
-    /* Install 4k mapping first */
+    /* Install 4k mapping */
     need_flush = set_iommu_pte_present(pt_mfn[1], dfn_x(dfn), mfn_x(mfn), 1,
                                        !!(flags & IOMMUF_writable),
                                        !!(flags & IOMMUF_readable));
 
     if ( need_flush )
-    {
         amd_iommu_flush_pages(d, dfn_x(dfn), 0);
-        /* No further merging, as the logic doesn't cope. */
-        hd->arch.no_merge = true;
-    }
 
-    /*
-     * Suppress merging of non-R/W mappings or after initial table creation,
-     * as the merge logic does not cope with this.
-     */
-    if ( hd->arch.no_merge || flags != (IOMMUF_writable | IOMMUF_readable) )
-        goto out;
-    if ( d->creation_finished )
-    {
-        hd->arch.no_merge = true;
-        goto out;
-    }
-
-    for ( merge_level = 2; merge_level <= hd->arch.paging_mode;
-          merge_level++ )
-    {
-        if ( pt_mfn[merge_level] == 0 )
-            break;
-        if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
-                                     dfn_x(dfn), mfn_x(mfn), merge_level) )
-            break;
-
-        if ( iommu_merge_pages(d, pt_mfn[merge_level], dfn_x(dfn),
-                               flags, merge_level) )
-        {
-            spin_unlock(&hd->arch.mapping_lock);
-            AMD_IOMMU_DEBUG("Merge iommu page failed at level %d, "
-                            "dfn = %"PRI_dfn" mfn = %"PRI_mfn"\n",
-                            merge_level, dfn_x(dfn), mfn_x(mfn));
-            domain_crash(d);
-            return -EFAULT;
-        }
-
-        /* Deallocate lower level page table */
-        free_amd_iommu_pgtable(mfn_to_page(_mfn(pt_mfn[merge_level - 1])));
-    }
-
-out:
     spin_unlock(&hd->arch.mapping_lock);
     return 0;
 }
@@ -798,9 +628,6 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t dfn)
     /* mark PTE as 'page not present' */
     clear_iommu_pte_present(pt_mfn[1], dfn_x(dfn));
 
-    /* No further merging in amd_iommu_map_page(), as the logic doesn't cope. */
-    hd->arch.no_merge = true;
-
     spin_unlock(&hd->arch.mapping_lock);
 
     amd_iommu_flush_pages(d, dfn_x(dfn), 0);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 055466b5bf..8dc392473d 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -52,7 +52,6 @@ struct arch_iommu
 
     /* amd iommu support */
     int paging_mode;
-    bool no_merge;
     struct page_info *root_table;
     struct guest_iommu *g_iommu;
 };
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread