memory/vfio: notify region_del() when unregister listeners

I encountered an event loss problem during unplugging vfio devices:

https://bugzilla.redhat.com/show_bug.cgi?id=1531393

I thought it should be a simple VT-d issue but I was wrong. The whole debugging leads me to these patches.

Basically I think what we missed is that when unregistering memory listeners, we don't really call region_del() at all. Instead we just remove ourselves from the listener list. IMHO that's not enough. A clean unregister should undo all possible changes that have done during region_add(). That's patch 1.

Patch 2 fixes a vfio issue when patch 1 is applied.

I'm marking this change as RFC since it touches the core of memory somehow, on which I am not 100% sure about. E.g., I haven't tested all the listener users, so I'm not sure whether it may broke any use case.

But what I'm sure is that it passes the docker tests on compiling/qtests, and it fixes the event loss that reported.

Let's see whether I can get some feedback first. Please review.

Thanks.

Peter Xu (2):
memory: do explicit cleanup when remove listeners
vfio: listener unregister before unset container

 hw/vfio/common.c | 16 ++++++++++++----
 memory.c         | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

2.14.3

When unregister memory listeners, we should call, e.g., region_del() (and possibly other undo operations) on every existing memory region sections there, otherwise we may leak resources that are held during the region_add(). This patch undo the stuff for the listeners, which emulates the case when the address space is set from current to an empty state.

I found this problem when debugging a refcount leak issue that leads to a device unplug event lost (please see the "Bug:" line below). In that case, the leakage of resource is the PCI BAR memory region refcount. And since memory regions are not keeping their own refcount but onto their owners, so the vfio-pci device's (who is the owner of the PCI BAR memory regions) refcount is leaked, and event missing.

---
 memory.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index 4b41fb837b..7d0064bd52 100644
@@ -2609,6 +2609,29 @@ static void listener_add_address_space(MemoryListener *listener,
     flatview_unref(view);
 }
 
+static void listener_del_address_space(MemoryListener *listener,
+                                       AddressSpace *as)
+{
+    FlatView *view;
+    FlatRange *fr;
+
+    view = address_space_get_flatview(as);
+    FOR_EACH_FLAT_RANGE(fr, view) {
+        MemoryRegionSection section = section_from_flat_range(fr, view);
+
+        if (fr->dirty_log_mask && listener->log_stop) {
+            listener->log_stop(listener, &section, fr->dirty_log_mask, 0);
+        }
+        if (listener->region_del) {
+            listener->region_del(listener, &section);
+        }
+    }
+    if (listener->commit) {
+        listener->commit(listener);
+    }
+    flatview_unref(view);
+}
+
 void memory_listener_register(MemoryListener *listener, AddressSpace *as)
 {
     MemoryListener *other = NULL;

@@ -2649,6 +2672,7 @@ void memory_listener_unregister(MemoryListener *listener)
         return;
     }
 
+    listener_del_address_space(listener, listener->address_space);
     QTAILQ_REMOVE(&memory_listeners, listener, link);
     QTAILQ_REMOVE(&listener->address_space->listeners, listener, link_as);
     listener->address_space = NULL;

2.14.3

listener->begin is missing before the loop.

Paolo

After previous patch, listener unregister will need the container to be alive. Let's move this unregister phase to be before unset container, since that operation will free the backend container in kernel, then we'll get these after previous patch:

qemu-system-x86_64: VFIO_UNMAP_DMA: -22
qemu-system-x86_64: vfio_dma_unmap(0x559bf53a4590, 0x0, 0xa0000) = -22 (Invalid argument)

---
 hw/vfio/common.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

index b77be3a8b3..76cf28d462 100644
@@ -1161,19 +1161,27 @@ static void vfio_disconnect_container(VFIOGroup *group)
 {
     VFIOContainer *container = group->container;
 
+    QLIST_REMOVE(group, container_next);
+    group->container = NULL;
+
+    /*
+     * Explicitly release the listener first before unset container,
+     * since unset may destroy the backend container if it's the last
+     * group.
+     */
+    if (QLIST_EMPTY(&container->group_list)) {
+        vfio_listener_release(container);
+    }
+
     if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
         error_report("vfio: error disconnecting group %d from container",
                      group->groupid);
     }
 
-    QLIST_REMOVE(group, container_next);
-    group->container = NULL;
-
     if (QLIST_EMPTY(&container->group_list)) {
         VFIOAddressSpace *space = container->space;
         VFIOGuestIOMMU *giommu, *tmp;
 
-        vfio_listener_release(container);
         QLIST_REMOVE(container, next);
 
         QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {

2.14.3

It makes sense, though of course patch 1 must come second for bisectability. Of the other listeners, most do not implement region_del, but commit must be audited as well. What matters is whether the listener is unregistered, and only few are:

- kvm_arm_machine_init_done must unregister the listener after the QSLIST_FOREACH_SAFE loop.

- Xen seems okay

- vhost needs to remove the memory_region_unref loop after unregistering the listener - and this must be done at the same time as patch 1, not before

- virtio seems okay

Thanks,

Paolo

I'll add another patch for this. I noticed that we haven't really do the list cleanup on kvm_devices_head. Say, the items are still on the list even after they are freed in kvm_arm_machine_init_done():

    QSLIST_FOREACH_SAFE(kd, &kvm_devices_head, entries, tkd) {
        if (kd->kda.addr != -1) {
            kvm_arm_set_device_addr(kd);
        }
        memory_region_unref(kd->mr);
        g_free(kd);
    }

I think it's pretty safe as long as we don't access kvm_devices_head any more (that's what we did), but would it be better to clean the list head too? (CC PeterM too)

Anyway, I'll ignore this for my series and only do what's needed for the memory listener fix.

Noted. I'll also fix the begin() missing problem you pointed out in the other comment. Thanks!

Peter Xu