[Xen-devel] [PATCH v4 01/15] re-work commit 3e06b989 "IOMMU: make page table population preemptible"...

[Paul Durrant <paul.durrant@xxxxxxxxxx>]

...to simplify the implementation and turn need_iommu back into a boolean.

As noted in [1] the tri-state nature of need_iommu after commit 3e06b989 is
confusing, as is the implementation of pre-emption using relmem_list.

This patch instead uses a simple count of pages already populated stored in
the x86 variant of struct arch_iommu and skips over that number of pages
if arch_iommu_populate_page_table() is re-invoked after pre-emption.

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-07/msg01870.html

Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
---
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

v4:
 - New in v4.
---
 xen/arch/x86/domain.c                 |  6 ---
 xen/arch/x86/mm/p2m-pod.c             |  3 +-
 xen/drivers/passthrough/device_tree.c | 19 ++++-----
 xen/drivers/passthrough/iommu.c       |  8 ++--
 xen/drivers/passthrough/x86/iommu.c   | 79 ++++++++++++++---------------------
 xen/include/asm-x86/iommu.h           |  1 +
 xen/include/xen/sched.h               |  4 +-
 7 files changed, 48 insertions(+), 72 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b5bb0f3b22..e68591d791 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1983,12 +1983,6 @@ int domain_relinquish_resources(struct domain *d)
         }
 
         d->arch.relmem = RELMEM_xen;
-
-        spin_lock(&d->page_alloc_lock);
-        page_list_splice(&d->arch.relmem_list, &d->page_list);
-        INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
-        spin_unlock(&d->page_alloc_lock);
-
         /* Fallthrough. Relinquish every page of memory. */
     case RELMEM_xen:
         ret = relinquish_memory(d, &d->xenpage_list, ~0UL);
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 631e9aec33..80f5ab1ea4 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -462,8 +462,7 @@ p2m_pod_offline_or_broken_hit(struct page_info *p)
 
 pod_hit:
     lock_page_alloc(p2m);
-    /* Insertion must be at list head (see iommu_populate_page_table()). */
-    page_list_add(p, &d->arch.relmem_list);
+    page_list_add_tail(p, &d->arch.relmem_list);
     unlock_page_alloc(p2m);
     pod_unlock(p2m);
     return 1;
diff --git a/xen/drivers/passthrough/device_tree.c 
b/xen/drivers/passthrough/device_tree.c
index 421f003438..671c8db1d0 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -40,17 +40,14 @@ int iommu_assign_dt_device(struct domain *d, struct 
dt_device_node *dev)
     if ( !list_empty(&dev->domain_list) )
         goto fail;
 
-    if ( need_iommu(d) <= 0 )
-    {
-        /*
-         * The hwdom is forced to use IOMMU for protecting assigned
-         * device. Therefore the IOMMU data is already set up.
-         */
-        ASSERT(!is_hardware_domain(d));
-        rc = iommu_construct(d);
-        if ( rc )
-            goto fail;
-    }
+    /*
+     * The hwdom is forced to use IOMMU for protecting assigned
+     * device. Therefore the IOMMU data is already set up.
+     */
+    ASSERT(!is_hardware_domain(d) || need_iommu(d));
+    rc = iommu_construct(d);
+    if ( rc )
+        goto fail;
 
     /* The flag field doesn't matter to DT device. */
     rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev), 0);
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 70d218f910..49974e5634 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -214,14 +214,14 @@ void iommu_teardown(struct domain *d)
 {
     const struct domain_iommu *hd = dom_iommu(d);
 
-    d->need_iommu = 0;
+    d->need_iommu = false;
     hd->platform_ops->teardown(d);
     tasklet_schedule(&iommu_pt_cleanup_tasklet);
 }
 
 int iommu_construct(struct domain *d)
 {
-    if ( need_iommu(d) > 0 )
+    if ( need_iommu(d) )
         return 0;
 
     if ( !iommu_use_hap_pt(d) )
@@ -233,7 +233,7 @@ int iommu_construct(struct domain *d)
             return rc;
     }
 
-    d->need_iommu = 1;
+    d->need_iommu = true;
     /*
      * There may be dirty cache lines when a device is assigned
      * and before need_iommu(d) becoming true, this will cause
@@ -493,7 +493,7 @@ static void iommu_dump_p2m_table(unsigned char key)
     ops = iommu_get_ops();
     for_each_domain(d)
     {
-        if ( is_hardware_domain(d) || need_iommu(d) <= 0 )
+        if ( is_hardware_domain(d) || !need_iommu(d) )
             continue;
 
         if ( iommu_use_hap_pt(d) )
diff --git a/xen/drivers/passthrough/x86/iommu.c 
b/xen/drivers/passthrough/x86/iommu.c
index 68182afd91..22feba572b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -41,62 +41,47 @@ int __init iommu_setup_hpet_msi(struct msi_desc *msi)
 
 int arch_iommu_populate_page_table(struct domain *d)
 {
-    const struct domain_iommu *hd = dom_iommu(d);
+    struct domain_iommu *hd = dom_iommu(d);
+    unsigned long skip = hd->arch.populated;
     struct page_info *page;
-    int rc = 0, n = 0;
+    int rc = 0;
 
-    d->need_iommu = -1;
+    if ( unlikely(d->is_dying) )
+        return -ESRCH;
 
     this_cpu(iommu_dont_flush_iotlb) = 1;
     spin_lock(&d->page_alloc_lock);
 
-    if ( unlikely(d->is_dying) )
-        rc = -ESRCH;
-
-    while ( !rc && (page = page_list_remove_head(&d->page_list)) )
+    page_list_for_each ( page, &d->page_list )
     {
-        if ( is_hvm_domain(d) ||
-            (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
-        {
-            unsigned long mfn = mfn_x(page_to_mfn(page));
-            unsigned long gfn = mfn_to_gmfn(d, mfn);
-
-            if ( gfn != gfn_x(INVALID_GFN) )
-            {
-                ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
-                BUG_ON(SHARED_M2P(gfn));
-                rc = hd->platform_ops->map_page(d, gfn, mfn,
-                                                IOMMUF_readable |
-                                                IOMMUF_writable);
-            }
-            if ( rc )
-            {
-                page_list_add(page, &d->page_list);
-                break;
-            }
-        }
-        page_list_add_tail(page, &d->arch.relmem_list);
-        if ( !(++n & 0xff) && !page_list_empty(&d->page_list) &&
-             hypercall_preempt_check() )
-            rc = -ERESTART;
-    }
+        unsigned long mfn;
+        unsigned long gfn;
 
-    if ( !rc )
-    {
-        /*
-         * The expectation here is that generally there are many normal pages
-         * on relmem_list (the ones we put there) and only few being in an
-         * offline/broken state. The latter ones are always at the head of the
-         * list. Hence we first move the whole list, and then move back the
-         * first few entries.
-         */
-        page_list_move(&d->page_list, &d->arch.relmem_list);
-        while ( !page_list_empty(&d->page_list) &&
-                (page = page_list_first(&d->page_list),
-                 (page->count_info & (PGC_state|PGC_broken))) )
+        if ( !is_hvm_domain(d) &&
+            (page->u.inuse.type_info & PGT_type_mask) != PGT_writable_page )
+            continue;
+
+        mfn = mfn_x(page_to_mfn(page));
+        gfn = mfn_to_gmfn(d, mfn);
+
+        if ( gfn == gfn_x(INVALID_GFN) )
+            continue;
+
+        if ( skip && skip-- ) /* make sure this doesn't underflow */
+            continue;
+
+        ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+        BUG_ON(SHARED_M2P(gfn));
+        rc = hd->platform_ops->map_page(d, gfn, mfn,
+                                        IOMMUF_readable |
+                                        IOMMUF_writable);
+        if ( rc )
+            break;
+
+        if ( !(hd->arch.populated++ & 0xff) && hypercall_preempt_check() )
         {
-            page_list_del(page, &d->page_list);
-            page_list_add_tail(page, &d->arch.relmem_list);
+            rc = -ERESTART;
+            break;
         }
     }
 
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 14ad0489a6..0dcead3b6c 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -37,6 +37,7 @@ struct arch_iommu
     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;
+    unsigned long populated;
 
     /* amd iommu support */
     int paging_mode;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 851f11ecf7..5b9820e4e1 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -372,8 +372,8 @@ struct domain
 #ifdef CONFIG_HAS_PASSTHROUGH
     struct domain_iommu iommu;
 
-    /* Does this guest need iommu mappings (-1 meaning "being set up")? */
-    s8               need_iommu;
+    /* Does this guest need iommu mappings? */
+    bool             need_iommu;
 #endif
     /* is node-affinity automatically computed? */
     bool             auto_node_affinity;
-- 
2.11.0


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