memory/vfio: notify region_del() when unregister listeners

PATCH Applied Peter Xu 4 participants 9 messages Jan 22, 2018 – Jan 30, 2018

Share & tools

Peter Xu

author

2018-01-22 06:03 UTC #0 lore raw

v2 cover memory/vfio: notify region_del() when unregister listeners

v2
- add begin() hooks [Paolo]
- move vfio patch to front [Paolo]
- one more patch for arm devlistener unregister [Paolo]
- one more patch for vhost traces
- removing RFC tag

This series fixes bug reported here:

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

The first patch only adds traces for vhost, with which I tested on vhost to make sure that my last patch works as expected with vhost. Please pick it if anyone wants, or ignore it if no one likes it.

The 2nd patch is the arm fix for preparation of the last patch.

The 3rd patch is the vfio fix for preparation of the last patch.

The 4th patch is the core fix of the problem.

Please review, thanks.

Thanks.

Peter Xu (4):
vhost: add traces for memory listeners
arm: postpone device listener unregister
vfio: listener unregister before unset container
memory: do explicit cleanup when remove listeners

 hw/vfio/common.c       | 16 ++++++++++++----
 hw/virtio/trace-events |  6 ++++++
 hw/virtio/vhost.c      | 11 +++++++----
 memory.c               | 27 +++++++++++++++++++++++++++
 target/arm/kvm.c       |  2 +-
 5 files changed, 53 insertions(+), 9 deletions(-)

Signature · 1 line

2.14.3

Peter Xu

author

2018-01-22 06:03 UTC #1 lore raw

PATCH v2 1/4 vhost: add traces for memory listeners

Trace these operations on two memory listeners. It helps to verify the new memory listener fix, and good to keep them there.

Signed-off-by Peter Xu <[email protected]>

---
 hw/virtio/trace-events | 6 ++++++
 hw/virtio/vhost.c      | 7 +++++++
 2 files changed, 13 insertions(+)

hw/virtio/trace-events +6 -0

index 775461ae98..2b8f81eb25 100644
@@ -25,3 +25,9 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
 virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
 virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
 virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: 0x%"PRIx64" num_pages: %d"
+
+# hw/virtio/vhost.c
+vhost_region_add(void *p, const char *mr) "dev %p mr %s"
+vhost_region_del(void *p, const char *mr) "dev %p mr %s"
+vhost_iommu_region_add(void *p, const char *mr) "dev %p mr %s"
+vhost_iommu_region_del(void *p, const char *mr) "dev %p mr %s"

hw/virtio/vhost.c +7 -0

index e4290ce93d..19a7a3cb37 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -687,6 +688,7 @@ static void vhost_region_add(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_region_add(dev, section->mr->name ?: NULL);
     ++dev->n_mem_sections;
     dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
                                 dev->n_mem_sections);

@@ -706,6 +708,7 @@ static void vhost_region_del(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_region_del(dev, section->mr->name ?: NULL);
     vhost_set_memory(listener, section, false);
     memory_region_unref(section->mr);
     for (i = 0; i < dev->n_mem_sections; ++i) {

@@ -743,6 +746,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_iommu_region_add(dev, section->mr->name ?: NULL);
+
     iommu = g_malloc0(sizeof(*iommu));
     end = int128_add(int128_make64(section->offset_within_region),
                      section->size);

@@ -771,6 +776,8 @@ static void vhost_iommu_region_del(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_iommu_region_del(dev, section->mr->name ?: NULL);
+
     QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
         if (iommu->mr == section->mr &&
             iommu->n.start == section->offset_within_region) {

Signature · 1 line

2.14.3

Michael S. Tsirkin

Acked-by on v2 1/4

2018-01-26 17:20 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v2 1/4] vhost: add traces for memory listeners

Quoted 4 lines · Peter Xu

Trace these operations on two memory listeners. It helps to verify the new memory listener fix, and good to keep them there.
Signed-off-by Peter Xu <[email protected]>

I'm not sure what this have to do with vfio, but anyway:

Acked-by Michael S. Tsirkin <[email protected]>

Quoted 67 lines

---
 hw/virtio/trace-events | 6 ++++++
 hw/virtio/vhost.c      | 7 +++++++
 2 files changed, 13 insertions(+)

hw/virtio/trace-events +6 -0

index 775461ae98..2b8f81eb25 100644
@@ -25,3 +25,9 @@ virtio_balloon_handle_output(const char *name, uint64_t gpa) "section name: %s g
 virtio_balloon_get_config(uint32_t num_pages, uint32_t actual) "num_pages: %d actual: %d"
 virtio_balloon_set_config(uint32_t actual, uint32_t oldactual) "actual: %d oldactual: %d"
 virtio_balloon_to_target(uint64_t target, uint32_t num_pages) "balloon target: 0x%"PRIx64" num_pages: %d"
+
+# hw/virtio/vhost.c
+vhost_region_add(void *p, const char *mr) "dev %p mr %s"
+vhost_region_del(void *p, const char *mr) "dev %p mr %s"
+vhost_iommu_region_add(void *p, const char *mr) "dev %p mr %s"
+vhost_iommu_region_del(void *p, const char *mr) "dev %p mr %s"

hw/virtio/vhost.c +7 -0

index e4290ce93d..19a7a3cb37 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -687,6 +688,7 @@ static void vhost_region_add(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_region_add(dev, section->mr->name ?: NULL);
     ++dev->n_mem_sections;
     dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
                                 dev->n_mem_sections);

@@ -706,6 +708,7 @@ static void vhost_region_del(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_region_del(dev, section->mr->name ?: NULL);
     vhost_set_memory(listener, section, false);
     memory_region_unref(section->mr);
     for (i = 0; i < dev->n_mem_sections; ++i) {

@@ -743,6 +746,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_iommu_region_add(dev, section->mr->name ?: NULL);
+
     iommu = g_malloc0(sizeof(*iommu));
     end = int128_add(int128_make64(section->offset_within_region),
                      section->size);

@@ -771,6 +776,8 @@ static void vhost_iommu_region_del(MemoryListener *listener,
         return;
     }
 
+    trace_vhost_iommu_region_del(dev, section->mr->name ?: NULL);
+
     QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
         if (iommu->mr == section->mr &&
             iommu->n.start == section->offset_within_region) {

Signature · 1 line

2.14.3

Peter Xu

author

2018-01-22 06:03 UTC #3 lore raw

PATCH v2 2/4 arm: postpone device listener unregister

In reply to Peter Xu

It's a preparation for follow-up patch to call region_del() in memory_listener_unregister(), otherwise all device addr attached with kvm_devices_head will be reset before calling kvm_arm_set_device_addr.

Signed-off-by Peter Xu <[email protected]>

---
 target/arm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/kvm.c +1 -1

index 211a7bf7be..1219d0062b 100644
@@ -266,7 +266,6 @@ static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
 {
     KVMDevice *kd, *tkd;
 
-    memory_listener_unregister(&devlistener);
     QSLIST_FOREACH_SAFE(kd, &kvm_devices_head, entries, tkd) {
         if (kd->kda.addr != -1) {
             kvm_arm_set_device_addr(kd);

@@ -274,6 +273,7 @@ static void kvm_arm_machine_init_done(Notifier *notifier, void *data)
         memory_region_unref(kd->mr);
         g_free(kd);
     }
+    memory_listener_unregister(&devlistener);
 }
 
 static Notifier notify = {

Signature · 1 line

2.14.3

Peter Xu

author

2018-01-22 06:03 UTC #4 lore raw

PATCH v2 3/4 vfio: listener unregister before unset container

In reply to Peter Xu

After next 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, otherwise we'll get these after next 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 <[email protected]>

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

hw/vfio/common.c +12 -4

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

Signature · 1 line

2.14.3

Alex Williamson

Acked-by on v2 3/4

2018-01-26 16:46 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v2 3/4] vfio: listener unregister before unset container

Quoted 10 lines · On Mon, 22 Jan 2018 14:02:43 +0800 Peter Xu <[email protected]> wrote:

After next 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, otherwise we'll get these after next 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 <[email protected]>
---

Acked-by Alex Williamson <[email protected]>

Not sure which branch this will go through, let me know if mine. Thanks,

Alex

Quoted 39 lines

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

hw/vfio/common.c +12 -4

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

Peter Xu

author

2018-01-22 06:03 UTC #6 lore raw

PATCH v2 4/4 memory: do explicit cleanup when remove listeners

In reply to 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.

We had encountered similar issues before and fixed in other way (ee4c112846, "vhost: Release memory references on cleanup"). This patch can be seen as a more high-level fix of similar problems that are caused by the resource leaks from memory listeners. So now we can remove the explicit unref of memory regions since that'll be done altogether during unregistering of listeners now.

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

Signed-off-by Peter Xu <[email protected]>

---
 hw/virtio/vhost.c |  4 ----
 memory.c          | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

hw/virtio/vhost.c +0 -4

index 19a7a3cb37..54480ffd85 100644
@@ -1368,10 +1368,6 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     if (hdev->mem) {
         /* those are only safe after successful init */
         memory_listener_unregister(&hdev->memory_listener);
-        for (i = 0; i < hdev->n_mem_sections; ++i) {
-            MemoryRegionSection *section = &hdev->mem_sections[i];
-            memory_region_unref(section->mr);
-        }
         QLIST_REMOVE(hdev, entry);
     }
     if (hdev->migration_blocker) {

memory.c +27 -0

index 4b41fb837b..9ad98b701b 100644
@@ -2609,6 +2609,32 @@ 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;
+
+    if (listener->begin) {
+        listener->begin(listener);
+    }
+    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 +2675,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;

Signature · 1 line

2.14.3

Paolo Bonzini

Reviewed-by on the v2 series

2018-01-26 09:17 UTC #7 lore raw

In reply to Peter Xu

Quoted 31 lines · Peter Xu

v2
- add begin() hooks [Paolo]
- move vfio patch to front [Paolo]
- one more patch for arm devlistener unregister [Paolo]
- one more patch for vhost traces
- removing RFC tag
This series fixes bug reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1531393
The first patch only adds traces for vhost, with which I tested on vhost to make sure that my last patch works as expected with vhost. Please pick it if anyone wants, or ignore it if no one likes it.
The 2nd patch is the arm fix for preparation of the last patch.
The 3rd patch is the vfio fix for preparation of the last patch.
The 4th patch is the core fix of the problem.
Please review, thanks.
Thanks.
Peter Xu (4):
vhost: add traces for memory listeners
arm: postpone device listener unregister
vfio: listener unregister before unset container
memory: do explicit cleanup when remove listeners

Quoted 7 lines

 hw/vfio/common.c       | 16 ++++++++++++----
 hw/virtio/trace-events |  6 ++++++
 hw/virtio/vhost.c      | 11 +++++++----
 memory.c               | 27 +++++++++++++++++++++++++++
 target/arm/kvm.c       |  2 +-
 5 files changed, 53 insertions(+), 9 deletions(-)

Reviewed-by Paolo Bonzini <[email protected]>

Alex, Peter, can you ack or apply patches 2/3?

Thanks,

Paolo

Paolo Bonzini

Applied

2018-01-30 19:14 UTC #8 lore raw

In reply to Peter Xu

Quoted 31 lines · Peter Xu

v2
- add begin() hooks [Paolo]
- move vfio patch to front [Paolo]
- one more patch for arm devlistener unregister [Paolo]
- one more patch for vhost traces
- removing RFC tag
This series fixes bug reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1531393
The first patch only adds traces for vhost, with which I tested on vhost to make sure that my last patch works as expected with vhost. Please pick it if anyone wants, or ignore it if no one likes it.
The 2nd patch is the arm fix for preparation of the last patch.
The 3rd patch is the vfio fix for preparation of the last patch.
The 4th patch is the core fix of the problem.
Please review, thanks.
Thanks.
Peter Xu (4):
vhost: add traces for memory listeners
arm: postpone device listener unregister
vfio: listener unregister before unset container
memory: do explicit cleanup when remove listeners

Quoted 7 lines

 hw/vfio/common.c       | 16 ++++++++++++----
 hw/virtio/trace-events |  6 ++++++
 hw/virtio/vhost.c      | 11 +++++++----
 memory.c               | 27 +++++++++++++++++++++++++++
 target/arm/kvm.c       |  2 +-
 5 files changed, 53 insertions(+), 9 deletions(-)

Queued, thanks.

Paolo