[Qemu-devel] [PATCH v2] vfio: avoid adding same iommu mr for notify

[Peter Xu]

When one IOMMU memory region is splitted into multiple memory sections,
vfio will register multiple same notifiers to a vIOMMU for the same
region. That's not sensible. What we need is to register one IOMMU
notifier for each IOMMU region, not per section.

Solution is simple - we traverse the container->giommu_list, and skip
the registration if memory region is already registered. Instead, we use
a refcount to note down how many sections are referencing the
VFIOGuestIOMMU.

To make vfio's region_add() short, vfio_listener_region_add_iommu() is
introduced.

Signed-off-by: Peter Xu <address@hidden>
---
v2:
- use refcount to make sure the VFIOGuestIOMMU won't be freed until the
  last referencing memory region section is deleted [David]
  (David suggested another way to implement - to store section range
   info in IOMMUNotifier. Will post another patch for it, and we can
   choose either one we want)

Signed-off-by: Peter Xu <address@hidden>
---
 hw/vfio/common.c              | 71 ++++++++++++++++++++++++++++---------------
 include/hw/vfio/vfio-common.h |  1 +
 2 files changed, 47 insertions(+), 25 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 801578b..e75e25c 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -360,6 +360,41 @@ out:
     rcu_read_unlock();
 }
 
+static void vfio_listener_region_add_iommu(VFIOContainer *container,
+                                           MemoryRegionSection *section,
+                                           hwaddr iova,
+                                           hwaddr end)
+{
+    VFIOGuestIOMMU *giommu;
+
+    QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
+        if (giommu->iommu == section->mr) {
+            atomic_inc(&giommu->refcount);
+            return;
+        }
+    }
+
+    trace_vfio_listener_region_add_iommu(iova, end);
+
+    /*
+     * FIXME: For VFIO iommu types which have KVM acceleration to
+     * avoid bouncing all map/unmaps through qemu this way, this
+     * would be the right place to wire that up (tell the KVM
+     * device emulation the VFIO iommu handles to use).
+     */
+    giommu = g_malloc0(sizeof(*giommu));
+    giommu->iommu = section->mr;
+    giommu->iommu_offset = section->offset_within_address_space -
+        section->offset_within_region;
+    giommu->container = container;
+    giommu->n.notify = vfio_iommu_map_notify;
+    giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
+    atomic_set(&giommu->refcount, 1);
+    QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
+    memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
+    memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
+}
+
 static void vfio_listener_region_add(MemoryListener *listener,
                                      MemoryRegionSection *section)
 {
@@ -439,27 +474,7 @@ static void vfio_listener_region_add(MemoryListener 
*listener,
     memory_region_ref(section->mr);
 
     if (memory_region_is_iommu(section->mr)) {
-        VFIOGuestIOMMU *giommu;
-
-        trace_vfio_listener_region_add_iommu(iova, end);
-        /*
-         * FIXME: For VFIO iommu types which have KVM acceleration to
-         * avoid bouncing all map/unmaps through qemu this way, this
-         * would be the right place to wire that up (tell the KVM
-         * device emulation the VFIO iommu handles to use).
-         */
-        giommu = g_malloc0(sizeof(*giommu));
-        giommu->iommu = section->mr;
-        giommu->iommu_offset = section->offset_within_address_space -
-                               section->offset_within_region;
-        giommu->container = container;
-        giommu->n.notify = vfio_iommu_map_notify;
-        giommu->n.notifier_flags = IOMMU_NOTIFIER_ALL;
-        QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
-
-        memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
-        memory_region_iommu_replay(giommu->iommu, &giommu->n, false);
-
+        vfio_listener_region_add_iommu(container, section, iova, end);
         return;
     }
 
@@ -526,10 +541,16 @@ static void vfio_listener_region_del(MemoryListener 
*listener,
 
         QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
             if (giommu->iommu == section->mr) {
-                memory_region_unregister_iommu_notifier(giommu->iommu,
-                                                        &giommu->n);
-                QLIST_REMOVE(giommu, giommu_next);
-                g_free(giommu);
+                /*
+                 * Only release the object when the last referencing
+                 * memory region section is deleted
+                 */
+                if (atomic_fetch_dec(&giommu->refcount) == 1) {
+                    memory_region_unregister_iommu_notifier(giommu->iommu,
+                                                            &giommu->n);
+                    QLIST_REMOVE(giommu, giommu_next);
+                    g_free(giommu);
+                }
                 break;
             }
         }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index c582de1..8817f8a 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -97,6 +97,7 @@ typedef struct VFIOGuestIOMMU {
     MemoryRegion *iommu;
     hwaddr iommu_offset;
     IOMMUNotifier n;
+    unsigned int refcount;
     QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
 } VFIOGuestIOMMU;
 
-- 
2.7.4