[Qemu-devel] [PATCH v2 0/4] memory/vfio: notify region_del() when unregister listeners

[Qemu-devel] [PATCH v2 0/4] memory/vfio: notify region_del() when unregister listeners

[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

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

-- 
2.14.3

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

[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 <peterx@redhat.com>
---
 hw/virtio/trace-events | 6 ++++++
 hw/virtio/vhost.c      | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 775461ae98..2b8f81eb25 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -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"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce93d..19a7a3cb37 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/4] arm: postpone device listener unregister

[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 <peterx@redhat.com>
---
 target/arm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 211a7bf7be..1219d0062b 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -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 = {
-- 
2.14.3

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

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

[Qemu-devel] [PATCH v2 4/4] 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.

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 <peterx@redhat.com>
---
 hw/virtio/vhost.c |  4 ----
 memory.c          | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 19a7a3cb37..54480ffd85 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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) {
diff --git a/memory.c b/memory.c
index 4b41fb837b..9ad98b701b 100644
--- a/memory.c
+++ b/memory.c
@@ -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;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 0/4] memory/vfio: notify region_del() when unregister listeners

[Paolo Bonzini]

On 22/01/2018 07:02, Peter Xu wrote:
> 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(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

Thanks,

Paolo

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

[Michael S. Tsirkin]

On Mon, Jan 22, 2018 at 02:02:41PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>

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

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/virtio/trace-events | 6 ++++++
>  hw/virtio/vhost.c      | 7 +++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 775461ae98..2b8f81eb25 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -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"
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce93d..19a7a3cb37 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -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) {
> -- 
> 2.14.3

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

[Alex Williamson]

On Mon, 22 Jan 2018 14:02:43 +0800
Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> ---

Acked-by: Alex Williamson <alex.williamson@redhat.com>

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

Alex

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

Re: [Qemu-devel] [PATCH v2 0/4] memory/vfio: notify region_del() when unregister listeners

[Paolo Bonzini]

On 22/01/2018 01:02, Peter Xu wrote:
> 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(-)
> 

Queued, thanks.

Paolo