[Qemu-devel] [RFC 0/2] memory/vfio: notify region_del() when unregister listeners

[Qemu-devel] [RFC 0/2] memory/vfio: notify region_del() when unregister listeners

[Peter Xu]

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

[Qemu-devel] [RFC 1/2] memory: do explicit cleanup when remove listeners

[Peter Xu]

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.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1531393
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/memory.c b/memory.c
index 4b41fb837b..7d0064bd52 100644
--- a/memory.c
+++ b/memory.c
@@ -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

Re: [Qemu-devel] [RFC 1/2] memory: do explicit cleanup when remove listeners

[Paolo Bonzini]

On 19/01/2018 09:42, Peter Xu wrote:
> +    FOR_EACH_FLAT_RANGE(fr, view) {

listener->begin is missing before the loop.

Paolo

> +        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);

[Qemu-devel] [RFC 2/2] vfio: listener unregister before unset container

[Peter Xu]

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)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/vfio/common.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b77be3a8b3..76cf28d462 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -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

Re: [Qemu-devel] [RFC 0/2] memory/vfio: notify region_del() when unregister listeners

[Paolo Bonzini]

On 19/01/2018 09:42, Peter Xu wrote:
> 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.

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

Re: [Qemu-devel] [RFC 0/2] memory/vfio: notify region_del() when unregister listeners

[Peter Xu]

On Fri, Jan 19, 2018 at 12:41:01PM +0100, Paolo Bonzini wrote:
> On 19/01/2018 09:42, Peter Xu wrote:
> > 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.
> 
> 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.

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.

> 
> - 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

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

> 
> - virtio seems okay
> 
> Thanks,
> 
> Paolo

-- 
Peter Xu