[PATCH] vdpa: stop all svq on device deletion

[PATCH] vdpa: stop all svq on device deletion

[Eugenio Pérez]

Not stopping them leave the device in a bad state when virtio-net
fronted device is unplugged with device_del monitor command.

This is not triggable in regular poweroff or qemu forces shutdown
because cleanup is called right after vhost_vdpa_dev_start(false).  But
devices hot unplug does not call vdpa device cleanups.  This lead to all
the vhost_vdpa devices without stop the SVQ but the last.

Fix it and clean the code, making it symmetric with
vhost_vdpa_svqs_start.

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 542e003101..df3a1e92ac 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -689,26 +689,11 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
     return ret;
 }
 
-static void vhost_vdpa_reset_svq(struct vhost_vdpa *v)
-{
-    if (!v->shadow_vqs_enabled) {
-        return;
-    }
-
-    for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
-        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
-        vhost_svq_stop(svq);
-    }
-}
-
 static int vhost_vdpa_reset_device(struct vhost_dev *dev)
 {
-    struct vhost_vdpa *v = dev->opaque;
     int ret;
     uint8_t status = 0;
 
-    vhost_vdpa_reset_svq(v);
-
     ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
     trace_vhost_vdpa_reset_device(dev, status);
     return ret;
@@ -1100,6 +1085,8 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
 
     for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
         VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
+
+        vhost_svq_stop(svq);
         vhost_vdpa_svq_unmap_rings(dev, svq);
 
         event_notifier_cleanup(&svq->hdev_kick);
-- 
2.31.1

Re: [PATCH] vdpa: stop all svq on device deletion

[Laurent Vivier]

On 2/9/23 18:00, Eugenio Pérez wrote:
> Not stopping them leave the device in a bad state when virtio-net
> fronted device is unplugged with device_del monitor command.
> 
> This is not triggable in regular poweroff or qemu forces shutdown
> because cleanup is called right after vhost_vdpa_dev_start(false).  But
> devices hot unplug does not call vdpa device cleanups.  This lead to all
> the vhost_vdpa devices without stop the SVQ but the last.
> 
> Fix it and clean the code, making it symmetric with
> vhost_vdpa_svqs_start.
> 
> Fixes: dff4426fa656 ("vhost: Add Shadow VirtQueue kick forwarding capabilities")
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 542e003101..df3a1e92ac 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -689,26 +689,11 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
>       return ret;
>   }
>   
> -static void vhost_vdpa_reset_svq(struct vhost_vdpa *v)
> -{
> -    if (!v->shadow_vqs_enabled) {
> -        return;
> -    }
> -
> -    for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
> -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> -        vhost_svq_stop(svq);
> -    }
> -}
> -
>   static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>   {
> -    struct vhost_vdpa *v = dev->opaque;
>       int ret;
>       uint8_t status = 0;
>   
> -    vhost_vdpa_reset_svq(v);
> -
>       ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>       trace_vhost_vdpa_reset_device(dev, status);
>       return ret;
> @@ -1100,6 +1085,8 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
>   
>       for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
>           VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> +
> +        vhost_svq_stop(svq);
>           vhost_vdpa_svq_unmap_rings(dev, svq);
>   
>           event_notifier_cleanup(&svq->hdev_kick);

Tested-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH] vdpa: stop all svq on device deletion

[Laurent Vivier]

Hi,

this patch fixes a QEMU crash, could it be merged?

Thanks,
Laurent

On 2/9/23 18:00, Eugenio Pérez wrote:
> Not stopping them leave the device in a bad state when virtio-net
> fronted device is unplugged with device_del monitor command.
> 
> This is not triggable in regular poweroff or qemu forces shutdown
> because cleanup is called right after vhost_vdpa_dev_start(false).  But
> devices hot unplug does not call vdpa device cleanups.  This lead to all
> the vhost_vdpa devices without stop the SVQ but the last.
> 
> Fix it and clean the code, making it symmetric with
> vhost_vdpa_svqs_start.
> 
> Fixes: dff4426fa656 ("vhost: Add Shadow VirtQueue kick forwarding capabilities")
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 542e003101..df3a1e92ac 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -689,26 +689,11 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
>       return ret;
>   }
>   
> -static void vhost_vdpa_reset_svq(struct vhost_vdpa *v)
> -{
> -    if (!v->shadow_vqs_enabled) {
> -        return;
> -    }
> -
> -    for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
> -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> -        vhost_svq_stop(svq);
> -    }
> -}
> -
>   static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>   {
> -    struct vhost_vdpa *v = dev->opaque;
>       int ret;
>       uint8_t status = 0;
>   
> -    vhost_vdpa_reset_svq(v);
> -
>       ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>       trace_vhost_vdpa_reset_device(dev, status);
>       return ret;
> @@ -1100,6 +1085,8 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
>   
>       for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
>           VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> +
> +        vhost_svq_stop(svq);
>           vhost_vdpa_svq_unmap_rings(dev, svq);
>   
>           event_notifier_cleanup(&svq->hdev_kick);

Re: [PATCH] vdpa: stop all svq on device deletion

[Jason Wang]

On Thu, Feb 23, 2023 at 4:51 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> Hi,
>
> this patch fixes a QEMU crash, could it be merged?

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

I think it should go with Michael's tree.

Thanks

>
> Thanks,
> Laurent
>
> On 2/9/23 18:00, Eugenio Pérez wrote:
> > Not stopping them leave the device in a bad state when virtio-net
> > fronted device is unplugged with device_del monitor command.
> >
> > This is not triggable in regular poweroff or qemu forces shutdown
> > because cleanup is called right after vhost_vdpa_dev_start(false).  But
> > devices hot unplug does not call vdpa device cleanups.  This lead to all
> > the vhost_vdpa devices without stop the SVQ but the last.
> >
> > Fix it and clean the code, making it symmetric with
> > vhost_vdpa_svqs_start.
> >
> > Fixes: dff4426fa656 ("vhost: Add Shadow VirtQueue kick forwarding capabilities")
> > Reported-by: Lei Yang <leiyang@redhat.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 17 ++---------------
> >   1 file changed, 2 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 542e003101..df3a1e92ac 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -689,26 +689,11 @@ static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
> >       return ret;
> >   }
> >
> > -static void vhost_vdpa_reset_svq(struct vhost_vdpa *v)
> > -{
> > -    if (!v->shadow_vqs_enabled) {
> > -        return;
> > -    }
> > -
> > -    for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
> > -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> > -        vhost_svq_stop(svq);
> > -    }
> > -}
> > -
> >   static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> >   {
> > -    struct vhost_vdpa *v = dev->opaque;
> >       int ret;
> >       uint8_t status = 0;
> >
> > -    vhost_vdpa_reset_svq(v);
> > -
> >       ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> >       trace_vhost_vdpa_reset_device(dev, status);
> >       return ret;
> > @@ -1100,6 +1085,8 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
> >
> >       for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
> >           VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
> > +
> > +        vhost_svq_stop(svq);
> >           vhost_vdpa_svq_unmap_rings(dev, svq);
> >
> >           event_notifier_cleanup(&svq->hdev_kick);
>