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

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

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