[PATCH 0/7] vDPA shadow virtqueue iova tree fixes.

[PATCH 0/7] vDPA shadow virtqueue iova tree fixes.

[Eugenio Pérez]

Collection of iova tree fixes detected preparing live migration with real
devices and multiqueue.

These cannot be triggered in simple setups (vdpa_sim_net, no display, no
device reset with different features) but it's possible to trigger them with
real devices or if the kernel fails some step like memory mapping / unmapping.

First two patches are already in the list at [1]. Last one is not a fix by
itself but a straightforward merge of the same code.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-08/msg00773.html

Eugenio Pérez (7):
  vdpa: Skip the maps not in the iova tree
  vdpa: do not save failed dma maps in SVQ iova tree
  util: make a copy of iova_tree_remove_parameter
  vdpa: Remove SVQ vring from iova_tree at shutdown
  vdpa: Make SVQ vring unmapping return void
  vhost: Always store new kick fd on vhost_svq_set_svq_kick_fd
  vdpa: Use ring hwaddr at vhost_vdpa_svq_unmap_ring

 hw/virtio/vhost-shadow-virtqueue.c |  4 +-
 hw/virtio/vhost-vdpa.c             | 70 +++++++++++++++---------------
 util/iova-tree.c                   |  4 +-
 3 files changed, 41 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH 1/7] vdpa: Skip the maps not in the iova tree

[Eugenio Pérez]

Next patch will skip the registering of dma maps that the vdpa device
rejects in the iova tree. We need to consider that here or we cause a
SIGSEGV accessing result.

Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3ff9ce3501..983d3697b0 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -289,6 +289,10 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
         };
 
         result = vhost_iova_tree_find_iova(v->iova_tree, &mem_region);
+        if (!result) {
+            /* The memory listener map wasn't mapped */
+            return;
+        }
         iova = result->iova;
         vhost_iova_tree_remove(v->iova_tree, result);
     }
-- 
2.31.1

[PATCH 2/7] vdpa: do not save failed dma maps in SVQ iova tree

[Eugenio Pérez]

If a map fails for whatever reason, it must not be saved in the tree.
Otherwise, qemu will try to unmap it in cleanup, leaving to more errors.

Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 983d3697b0..7e28d2f674 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -176,6 +176,7 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener)
 static void vhost_vdpa_listener_region_add(MemoryListener *listener,
                                            MemoryRegionSection *section)
 {
+    DMAMap mem_region = {};
     struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
     hwaddr iova;
     Int128 llend, llsize;
@@ -212,13 +213,13 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
 
     llsize = int128_sub(llend, int128_make64(iova));
     if (v->shadow_vqs_enabled) {
-        DMAMap mem_region = {
-            .translated_addr = (hwaddr)(uintptr_t)vaddr,
-            .size = int128_get64(llsize) - 1,
-            .perm = IOMMU_ACCESS_FLAG(true, section->readonly),
-        };
+        int r;
 
-        int r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region);
+        mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr,
+        mem_region.size = int128_get64(llsize) - 1,
+        mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly),
+
+        r = vhost_iova_tree_map_alloc(v->iova_tree, &mem_region);
         if (unlikely(r != IOVA_OK)) {
             error_report("Can't allocate a mapping (%d)", r);
             goto fail;
@@ -232,11 +233,16 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
                              vaddr, section->readonly);
     if (ret) {
         error_report("vhost vdpa map fail!");
-        goto fail;
+        goto fail_map;
     }
 
     return;
 
+fail_map:
+    if (v->shadow_vqs_enabled) {
+        vhost_iova_tree_remove(v->iova_tree, &mem_region);
+    }
+
 fail:
     /*
      * On the initfn path, store the first error in the container so we
-- 
2.31.1

[PATCH 3/7] util: make a copy of iova_tree_remove_parameter

[Eugenio Pérez]

It's convenient to call iova_tree_remove from a map returned from
iova_tree_find or iova_tree_find_iova. With the current code this is not
possible, since we will free it, and then we will try to search for it
again.

Fix it making a copy of the argument. Not applying a fixes tag, since
there is no use like that at the moment.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 util/iova-tree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/iova-tree.c b/util/iova-tree.c
index fee530a579..713e3edd7b 100644
--- a/util/iova-tree.c
+++ b/util/iova-tree.c
@@ -166,9 +166,11 @@ void iova_tree_foreach(IOVATree *tree, iova_tree_iterator iterator)
 
 void iova_tree_remove(IOVATree *tree, const DMAMap *map)
 {
+    /* Just in case caller is calling iova_tree_remove from a result of find */
+    const DMAMap needle = *map;
     const DMAMap *overlap;
 
-    while ((overlap = iova_tree_find(tree, map))) {
+    while ((overlap = iova_tree_find(tree, &needle))) {
         g_tree_remove(tree->tree, overlap);
     }
 }
-- 
2.31.1

[PATCH 4/7] vdpa: Remove SVQ vring from iova_tree at shutdown

[Eugenio Pérez]

Although the device will be reset before usage, the right thing to do is
to clean it.

Reported-by: Lei Yang <leiyang@redhat.com>
Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7e28d2f674..943799c17c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -898,7 +898,12 @@ static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
 
     size = ROUND_UP(result->size, qemu_real_host_page_size());
     r = vhost_vdpa_dma_unmap(v, result->iova, size);
-    return r == 0;
+    if (unlikely(r < 0)) {
+        return false;
+    }
+
+    vhost_iova_tree_remove(v->iova_tree, result);
+    return 0;
 }
 
 static bool vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
-- 
2.31.1

[PATCH 5/7] vdpa: Make SVQ vring unmapping return void

[Eugenio Pérez]

Nothing actually reads the return value, but an error in cleaning some
entries could cause device stop to abort, making a restart impossible.
Better ignore explicitely the return value.

Reported-by: Lei Yang <leiyang@redhat.com>
Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 943799c17c..07d00f5284 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -884,7 +884,7 @@ static int vhost_vdpa_svq_set_fds(struct vhost_dev *dev,
 /**
  * Unmap a SVQ area in the device
  */
-static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
+static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
                                       const DMAMap *needle)
 {
     const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, needle);
@@ -893,37 +893,32 @@ static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
 
     if (unlikely(!result)) {
         error_report("Unable to find SVQ address to unmap");
-        return false;
+        return;
     }
 
     size = ROUND_UP(result->size, qemu_real_host_page_size());
     r = vhost_vdpa_dma_unmap(v, result->iova, size);
     if (unlikely(r < 0)) {
-        return false;
+        return;
     }
 
     vhost_iova_tree_remove(v->iova_tree, result);
-    return 0;
 }
 
-static bool vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
+static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
                                        const VhostShadowVirtqueue *svq)
 {
     DMAMap needle = {};
     struct vhost_vdpa *v = dev->opaque;
     struct vhost_vring_addr svq_addr;
-    bool ok;
 
     vhost_svq_get_vring_addr(svq, &svq_addr);
 
     needle.translated_addr = svq_addr.desc_user_addr;
-    ok = vhost_vdpa_svq_unmap_ring(v, &needle);
-    if (unlikely(!ok)) {
-        return false;
-    }
+    vhost_vdpa_svq_unmap_ring(v, &needle);
 
     needle.translated_addr = svq_addr.used_user_addr;
-    return vhost_vdpa_svq_unmap_ring(v, &needle);
+    vhost_vdpa_svq_unmap_ring(v, &needle);
 }
 
 /**
@@ -1094,26 +1089,22 @@ err:
     return false;
 }
 
-static bool vhost_vdpa_svqs_stop(struct vhost_dev *dev)
+static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
 
     if (!v->shadow_vqs) {
-        return true;
+        return;
     }
 
     for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
         VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
-        bool ok = vhost_vdpa_svq_unmap_rings(dev, svq);
-        if (unlikely(!ok)) {
-            return false;
-        }
+        vhost_vdpa_svq_unmap_rings(dev, svq);
     }
 
     if (v->migration_blocker) {
         migrate_del_blocker(v->migration_blocker);
     }
-    return true;
 }
 
 static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
@@ -1130,10 +1121,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
         }
         vhost_vdpa_set_vring_ready(dev);
     } else {
-        ok = vhost_vdpa_svqs_stop(dev);
-        if (unlikely(!ok)) {
-            return -1;
-        }
+        vhost_vdpa_svqs_stop(dev);
         vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
     }
 
-- 
2.31.1

[PATCH 6/7] vhost: Always store new kick fd on vhost_svq_set_svq_kick_fd

[Eugenio Pérez]

We can unbind twice a file descriptor if we call twice
vhost_svq_set_svq_kick_fd because of this. Since it comes from vhost and
not from SVQ, that file descriptor could be a different thing that
guest's vhost notifier.

Likewise, it can happens the same if a guest start and stop the device
multiple times.

Reported-by: Lei Yang <leiyang@redhat.com>
Fixes: dff4426fa6 ("vhost: Add Shadow VirtQueue kick forwarding capabilities")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index e4956728dd..82a784d250 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -602,13 +602,13 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
         event_notifier_set_handler(svq_kick, NULL);
     }
 
+    event_notifier_init_fd(svq_kick, svq_kick_fd);
     /*
      * event_notifier_set_handler already checks for guest's notifications if
      * they arrive at the new file descriptor in the switch, so there is no
      * need to explicitly check for them.
      */
     if (poll_start) {
-        event_notifier_init_fd(svq_kick, svq_kick_fd);
         event_notifier_set(svq_kick);
         event_notifier_set_handler(svq_kick, vhost_handle_guest_kick_notifier);
     }
@@ -655,7 +655,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
  */
 void vhost_svq_stop(VhostShadowVirtqueue *svq)
 {
-    event_notifier_set_handler(&svq->svq_kick, NULL);
+    vhost_svq_set_svq_kick_fd(svq, VHOST_FILE_UNBIND);
     g_autofree VirtQueueElement *next_avail_elem = NULL;
 
     if (!svq->vq) {
-- 
2.31.1

[PATCH 7/7] vdpa: Use ring hwaddr at vhost_vdpa_svq_unmap_ring

[Eugenio Pérez]

Reduce code duplication.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 07d00f5284..45d6e86b45 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -884,10 +884,12 @@ static int vhost_vdpa_svq_set_fds(struct vhost_dev *dev,
 /**
  * Unmap a SVQ area in the device
  */
-static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
-                                      const DMAMap *needle)
+static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v, hwaddr addr)
 {
-    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, needle);
+    const DMAMap needle = {
+        .translated_addr = addr,
+    };
+    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, &needle);
     hwaddr size;
     int r;
 
@@ -908,17 +910,14 @@ static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
 static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
                                        const VhostShadowVirtqueue *svq)
 {
-    DMAMap needle = {};
     struct vhost_vdpa *v = dev->opaque;
     struct vhost_vring_addr svq_addr;
 
     vhost_svq_get_vring_addr(svq, &svq_addr);
 
-    needle.translated_addr = svq_addr.desc_user_addr;
-    vhost_vdpa_svq_unmap_ring(v, &needle);
+    vhost_vdpa_svq_unmap_ring(v, svq_addr.desc_user_addr);
 
-    needle.translated_addr = svq_addr.used_user_addr;
-    vhost_vdpa_svq_unmap_ring(v, &needle);
+    vhost_vdpa_svq_unmap_ring(v, svq_addr.used_user_addr);
 }
 
 /**
@@ -996,7 +995,7 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev *dev,
     ok = vhost_vdpa_svq_map_ring(v, &device_region, errp);
     if (unlikely(!ok)) {
         error_prepend(errp, "Cannot create vq device region: ");
-        vhost_vdpa_svq_unmap_ring(v, &driver_region);
+        vhost_vdpa_svq_unmap_ring(v, driver_region.translated_addr);
     }
     addr->used_user_addr = device_region.iova;
 
-- 
2.31.1

Re: [PATCH 3/7] util: make a copy of iova_tree_remove_parameter

[Jason Wang]

在 2022/8/20 00:53, Eugenio Pérez 写道:
> It's convenient to call iova_tree_remove from a map returned from
> iova_tree_find or iova_tree_find_iova.


The looks like a hint of the defect of current API.


>   With the current code this is not
> possible, since we will free it, and then we will try to search for it
> again.
>
> Fix it making a copy of the argument. Not applying a fixes tag, since
> there is no use like that at the moment.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   util/iova-tree.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/util/iova-tree.c b/util/iova-tree.c
> index fee530a579..713e3edd7b 100644
> --- a/util/iova-tree.c
> +++ b/util/iova-tree.c
> @@ -166,9 +166,11 @@ void iova_tree_foreach(IOVATree *tree, iova_tree_iterator iterator)
>   
>   void iova_tree_remove(IOVATree *tree, const DMAMap *map)
>   {
> +    /* Just in case caller is calling iova_tree_remove from a result of find */
> +    const DMAMap needle = *map;


Then let's simply make iova_tree_remove() accept const DMAMap instead of 
the pointer to it.


>       const DMAMap *overlap;
>   
> -    while ((overlap = iova_tree_find(tree, map))) {
> +    while ((overlap = iova_tree_find(tree, &needle))) {
>           g_tree_remove(tree->tree, overlap);
>       }


So we had following in iova_tree_insert():

     /* We don't allow to insert range that overlaps with existings */
     if (iova_tree_find(tree, map)) {
         return IOVA_ERR_OVERLAP;
     }

I wonder how overlap can happen?

Thanks



>   }

Re: [PATCH 4/7] vdpa: Remove SVQ vring from iova_tree at shutdown

[Jason Wang]

在 2022/8/20 00:53, Eugenio Pérez 写道:
> Although the device will be reset before usage, the right thing to do is
> to clean it.
>
> Reported-by: Lei Yang <leiyang@redhat.com>
> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 7e28d2f674..943799c17c 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -898,7 +898,12 @@ static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
>   
>       size = ROUND_UP(result->size, qemu_real_host_page_size());
>       r = vhost_vdpa_dma_unmap(v, result->iova, size);
> -    return r == 0;
> +    if (unlikely(r < 0)) {
> +        return false;


vhost-vdpa_svq_map_ring() will call error_report() here, should we do 
the same?

     if (unlikely(r != 0)) {
         error_setg_errno(errp, -r, "Cannot map region to device");

(Btw the error is not very informative, we should dump the map it self 
at least?)

Thanks


> +    }
> +
> +    vhost_iova_tree_remove(v->iova_tree, result);
> +    return 0;
>   }
>   
>   static bool vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,

Re: [PATCH 5/7] vdpa: Make SVQ vring unmapping return void

[Jason Wang]

在 2022/8/20 00:53, Eugenio Pérez 写道:
> Nothing actually reads the return value, but an error in cleaning some
> entries could cause device stop to abort, making a restart impossible.
> Better ignore explicitely the return value.
>
> Reported-by: Lei Yang <leiyang@redhat.com>
> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/virtio/vhost-vdpa.c | 32 ++++++++++----------------------
>   1 file changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 943799c17c..07d00f5284 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -884,7 +884,7 @@ static int vhost_vdpa_svq_set_fds(struct vhost_dev *dev,
>   /**
>    * Unmap a SVQ area in the device
>    */
> -static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
> +static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
>                                         const DMAMap *needle)
>   {
>       const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, needle);
> @@ -893,37 +893,32 @@ static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
>   
>       if (unlikely(!result)) {
>           error_report("Unable to find SVQ address to unmap");
> -        return false;
> +        return;
>       }
>   
>       size = ROUND_UP(result->size, qemu_real_host_page_size());
>       r = vhost_vdpa_dma_unmap(v, result->iova, size);
>       if (unlikely(r < 0)) {
> -        return false;
> +        return;
>       }
>   
>       vhost_iova_tree_remove(v->iova_tree, result);
> -    return 0;
>   }
>   
> -static bool vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
> +static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
>                                          const VhostShadowVirtqueue *svq)
>   {
>       DMAMap needle = {};
>       struct vhost_vdpa *v = dev->opaque;
>       struct vhost_vring_addr svq_addr;
> -    bool ok;
>   
>       vhost_svq_get_vring_addr(svq, &svq_addr);
>   
>       needle.translated_addr = svq_addr.desc_user_addr;
> -    ok = vhost_vdpa_svq_unmap_ring(v, &needle);
> -    if (unlikely(!ok)) {
> -        return false;
> -    }
> +    vhost_vdpa_svq_unmap_ring(v, &needle);
>   
>       needle.translated_addr = svq_addr.used_user_addr;
> -    return vhost_vdpa_svq_unmap_ring(v, &needle);
> +    vhost_vdpa_svq_unmap_ring(v, &needle);
>   }
>   
>   /**
> @@ -1094,26 +1089,22 @@ err:
>       return false;
>   }
>   
> -static bool vhost_vdpa_svqs_stop(struct vhost_dev *dev)
> +static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
>   {
>       struct vhost_vdpa *v = dev->opaque;
>   
>       if (!v->shadow_vqs) {
> -        return true;
> +        return;
>       }
>   
>       for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
>           VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> -        bool ok = vhost_vdpa_svq_unmap_rings(dev, svq);
> -        if (unlikely(!ok)) {
> -            return false;
> -        }
> +        vhost_vdpa_svq_unmap_rings(dev, svq);
>       }
>   
>       if (v->migration_blocker) {
>           migrate_del_blocker(v->migration_blocker);
>       }
> -    return true;
>   }
>   
>   static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> @@ -1130,10 +1121,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>           }
>           vhost_vdpa_set_vring_ready(dev);
>       } else {
> -        ok = vhost_vdpa_svqs_stop(dev);
> -        if (unlikely(!ok)) {
> -            return -1;
> -        }
> +        vhost_vdpa_svqs_stop(dev);
>           vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>       }
>   

Re: [PATCH 6/7] vhost: Always store new kick fd on vhost_svq_set_svq_kick_fd

[Jason Wang]

在 2022/8/20 00:53, Eugenio Pérez 写道:
> We can unbind twice a file descriptor if we call twice
> vhost_svq_set_svq_kick_fd because of this. Since it comes from vhost and
> not from SVQ, that file descriptor could be a different thing that
> guest's vhost notifier.
>
> Likewise, it can happens the same if a guest start and stop the device
> multiple times.
>
> Reported-by: Lei Yang <leiyang@redhat.com>
> Fixes: dff4426fa6 ("vhost: Add Shadow VirtQueue kick forwarding capabilities")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---


Acked-by: Jason Wang <jasowang@redhat.com>


>   hw/virtio/vhost-shadow-virtqueue.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index e4956728dd..82a784d250 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -602,13 +602,13 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
>           event_notifier_set_handler(svq_kick, NULL);
>       }
>   
> +    event_notifier_init_fd(svq_kick, svq_kick_fd);
>       /*
>        * event_notifier_set_handler already checks for guest's notifications if
>        * they arrive at the new file descriptor in the switch, so there is no
>        * need to explicitly check for them.
>        */
>       if (poll_start) {
> -        event_notifier_init_fd(svq_kick, svq_kick_fd);
>           event_notifier_set(svq_kick);
>           event_notifier_set_handler(svq_kick, vhost_handle_guest_kick_notifier);
>       }
> @@ -655,7 +655,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>    */
>   void vhost_svq_stop(VhostShadowVirtqueue *svq)
>   {
> -    event_notifier_set_handler(&svq->svq_kick, NULL);
> +    vhost_svq_set_svq_kick_fd(svq, VHOST_FILE_UNBIND);
>       g_autofree VirtQueueElement *next_avail_elem = NULL;
>   
>       if (!svq->vq) {

Re: [PATCH 7/7] vdpa: Use ring hwaddr at vhost_vdpa_svq_unmap_ring

[Jason Wang]

在 2022/8/20 00:53, Eugenio Pérez 写道:
> Reduce code duplication.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>

(In the future, we need to look for other cases where a function may use 
only a partial of DMAMap.)

Thanks


> ---
>   hw/virtio/vhost-vdpa.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 07d00f5284..45d6e86b45 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -884,10 +884,12 @@ static int vhost_vdpa_svq_set_fds(struct vhost_dev *dev,
>   /**
>    * Unmap a SVQ area in the device
>    */
> -static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
> -                                      const DMAMap *needle)
> +static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v, hwaddr addr)
>   {
> -    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, needle);
> +    const DMAMap needle = {
> +        .translated_addr = addr,
> +    };
> +    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, &needle);
>       hwaddr size;
>       int r;
>   
> @@ -908,17 +910,14 @@ static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
>   static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
>                                          const VhostShadowVirtqueue *svq)
>   {
> -    DMAMap needle = {};
>       struct vhost_vdpa *v = dev->opaque;
>       struct vhost_vring_addr svq_addr;
>   
>       vhost_svq_get_vring_addr(svq, &svq_addr);
>   
> -    needle.translated_addr = svq_addr.desc_user_addr;
> -    vhost_vdpa_svq_unmap_ring(v, &needle);
> +    vhost_vdpa_svq_unmap_ring(v, svq_addr.desc_user_addr);
>   
> -    needle.translated_addr = svq_addr.used_user_addr;
> -    vhost_vdpa_svq_unmap_ring(v, &needle);
> +    vhost_vdpa_svq_unmap_ring(v, svq_addr.used_user_addr);
>   }
>   
>   /**
> @@ -996,7 +995,7 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev *dev,
>       ok = vhost_vdpa_svq_map_ring(v, &device_region, errp);
>       if (unlikely(!ok)) {
>           error_prepend(errp, "Cannot create vq device region: ");
> -        vhost_vdpa_svq_unmap_ring(v, &driver_region);
> +        vhost_vdpa_svq_unmap_ring(v, driver_region.translated_addr);
>       }
>       addr->used_user_addr = device_region.iova;
>   

Re: [PATCH 3/7] util: make a copy of iova_tree_remove_parameter

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 8:18 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 00:53, Eugenio Pérez 写道:
> > It's convenient to call iova_tree_remove from a map returned from
> > iova_tree_find or iova_tree_find_iova.
>
>
> The looks like a hint of the defect of current API.
>
>
> >   With the current code this is not
> > possible, since we will free it, and then we will try to search for it
> > again.
> >
> > Fix it making a copy of the argument. Not applying a fixes tag, since
> > there is no use like that at the moment.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   util/iova-tree.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/iova-tree.c b/util/iova-tree.c
> > index fee530a579..713e3edd7b 100644
> > --- a/util/iova-tree.c
> > +++ b/util/iova-tree.c
> > @@ -166,9 +166,11 @@ void iova_tree_foreach(IOVATree *tree, iova_tree_iterator iterator)
> >
> >   void iova_tree_remove(IOVATree *tree, const DMAMap *map)
> >   {
> > +    /* Just in case caller is calling iova_tree_remove from a result of find */
> > +    const DMAMap needle = *map;
>
>
> Then let's simply make iova_tree_remove() accept const DMAMap instead of
> the pointer to it.
>

Do you mean to accept DMAMap by value, isn't it? (no need for const it
then, isn't it?).

>
> >       const DMAMap *overlap;
> >
> > -    while ((overlap = iova_tree_find(tree, map))) {
> > +    while ((overlap = iova_tree_find(tree, &needle))) {
> >           g_tree_remove(tree->tree, overlap);
> >       }
>
>
> So we had following in iova_tree_insert():
>
>      /* We don't allow to insert range that overlaps with existings */
>      if (iova_tree_find(tree, map)) {
>          return IOVA_ERR_OVERLAP;
>      }
>
> I wonder how overlap can happen?
>

I don't get this part. The problem is that iova_tree_find returns a
pointer to an internal structure, there is no need to insert multiple
times overlapping maps.

Thanks!

Re: [PATCH 4/7] vdpa: Remove SVQ vring from iova_tree at shutdown

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 8:25 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 00:53, Eugenio Pérez 写道:
> > Although the device will be reset before usage, the right thing to do is
> > to clean it.
> >
> > Reported-by: Lei Yang <leiyang@redhat.com>
> > Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 7e28d2f674..943799c17c 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -898,7 +898,12 @@ static bool vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
> >
> >       size = ROUND_UP(result->size, qemu_real_host_page_size());
> >       r = vhost_vdpa_dma_unmap(v, result->iova, size);
> > -    return r == 0;
> > +    if (unlikely(r < 0)) {
> > +        return false;
>
>
> vhost-vdpa_svq_map_ring() will call error_report() here, should we do
> the same?
>

We can use it unconditionally on error paths if we don't report
errors. I can try to check if we use that way at this moment.

>      if (unlikely(r != 0)) {
>          error_setg_errno(errp, -r, "Cannot map region to device");
>
> (Btw the error is not very informative, we should dump the map it self
> at least?)
>
> Thanks
>
>
> > +    }
> > +
> > +    vhost_iova_tree_remove(v->iova_tree, result);
> > +    return 0;
> >   }
> >
> >   static bool vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
>

Re: [PATCH 7/7] vdpa: Use ring hwaddr at vhost_vdpa_svq_unmap_ring

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 8:40 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 00:53, Eugenio Pérez 写道:
> > Reduce code duplication.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> (In the future, we need to look for other cases where a function may use
> only a partial of DMAMap.)
>

Do you have a use case in mind? The IOVA tree also trusts the memory
listener will unmap and map chunks symmetrically.

Thanks!

> Thanks
>
>
> > ---
> >   hw/virtio/vhost-vdpa.c | 17 ++++++++---------
> >   1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 07d00f5284..45d6e86b45 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -884,10 +884,12 @@ static int vhost_vdpa_svq_set_fds(struct vhost_dev *dev,
> >   /**
> >    * Unmap a SVQ area in the device
> >    */
> > -static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
> > -                                      const DMAMap *needle)
> > +static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v, hwaddr addr)
> >   {
> > -    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, needle);
> > +    const DMAMap needle = {
> > +        .translated_addr = addr,
> > +    };
> > +    const DMAMap *result = vhost_iova_tree_find_iova(v->iova_tree, &needle);
> >       hwaddr size;
> >       int r;
> >
> > @@ -908,17 +910,14 @@ static void vhost_vdpa_svq_unmap_ring(struct vhost_vdpa *v,
> >   static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev,
> >                                          const VhostShadowVirtqueue *svq)
> >   {
> > -    DMAMap needle = {};
> >       struct vhost_vdpa *v = dev->opaque;
> >       struct vhost_vring_addr svq_addr;
> >
> >       vhost_svq_get_vring_addr(svq, &svq_addr);
> >
> > -    needle.translated_addr = svq_addr.desc_user_addr;
> > -    vhost_vdpa_svq_unmap_ring(v, &needle);
> > +    vhost_vdpa_svq_unmap_ring(v, svq_addr.desc_user_addr);
> >
> > -    needle.translated_addr = svq_addr.used_user_addr;
> > -    vhost_vdpa_svq_unmap_ring(v, &needle);
> > +    vhost_vdpa_svq_unmap_ring(v, svq_addr.used_user_addr);
> >   }
> >
> >   /**
> > @@ -996,7 +995,7 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev *dev,
> >       ok = vhost_vdpa_svq_map_ring(v, &device_region, errp);
> >       if (unlikely(!ok)) {
> >           error_prepend(errp, "Cannot create vq device region: ");
> > -        vhost_vdpa_svq_unmap_ring(v, &driver_region);
> > +        vhost_vdpa_svq_unmap_ring(v, driver_region.translated_addr);
> >       }
> >       addr->used_user_addr = device_region.iova;
> >
>