1. Patchew
  2. QEMU
  3. View series

[PATCH] vhost-user: send SET_STATUS 0 after GET_VRING_BASE

Stefan Hajnoczi posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230420130706.541219-1-stefanha@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
hw/virtio/vhost-user.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[PATCH] vhost-user: send SET_STATUS 0 after GET_VRING_BASE
Posted by Stefan Hajnoczi 1 year ago 
Setting the VIRTIO Device Status Field to 0 resets the device. The
device's state is lost, including the vring configuration.

vhost-user.c currently sends SET_STATUS 0 before GET_VRING_BASE. This
risks confusion about the lifetime of the vhost-user state (e.g. vring
last_avail_idx) across VIRTIO device reset.

Eugenio Pérez <eperezma@redhat.com> adjusted the order for vhost-vdpa.c
in commit c3716f260bff ("vdpa: move vhost reset after get vring base")
and in that commit description suggested doing the same for vhost-user
in the future.

Go ahead and adjust vhost-user.c now. I ran various online code searches
to identify vhost-user backends implementing SET_STATUS. It seems only
DPDK implements SET_STATUS and Yajun Wu <yajunw@nvidia.com> has
confirmed that it is safe to make this change.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Cindy Lu <lulu@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/vhost-user.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e5285df4ba..2d40b1b3e7 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2677,10 +2677,20 @@ static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
                                           VIRTIO_CONFIG_S_DRIVER |
                                           VIRTIO_CONFIG_S_DRIVER_OK);
     } else {
-        return vhost_user_set_status(dev, 0);
+        return 0;
     }
 }
 
+static void vhost_user_reset_status(struct vhost_dev *dev)
+{
+    /* Set device status only for last queue pair */
+    if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
+        return;
+    }
+
+    vhost_user_set_status(dev, 0);
+}
+
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
         .vhost_backend_init = vhost_user_backend_init,
@@ -2716,4 +2726,5 @@ const VhostOps user_ops = {
         .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
         .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
         .vhost_dev_start = vhost_user_dev_start,
+        .vhost_reset_status = vhost_user_reset_status,
 };
-- 
2.39.2
Re: [PATCH] vhost-user: send SET_STATUS 0 after GET_VRING_BASE
Posted by Michael S. Tsirkin 1 year ago 
On Thu, Apr 20, 2023 at 09:07:06AM -0400, Stefan Hajnoczi wrote:
> Setting the VIRTIO Device Status Field to 0 resets the device. The
> device's state is lost, including the vring configuration.
> 
> vhost-user.c currently sends SET_STATUS 0 before GET_VRING_BASE. This
> risks confusion about the lifetime of the vhost-user state (e.g. vring
> last_avail_idx) across VIRTIO device reset.
> 
> Eugenio Pérez <eperezma@redhat.com> adjusted the order for vhost-vdpa.c
> in commit c3716f260bff ("vdpa: move vhost reset after get vring base")
> and in that commit description suggested doing the same for vhost-user
> in the future.
> 
> Go ahead and adjust vhost-user.c now. I ran various online code searches
> to identify vhost-user backends implementing SET_STATUS. It seems only
> DPDK implements SET_STATUS and Yajun Wu <yajunw@nvidia.com> has
> confirmed that it is safe to make this change.


Fixes tag?

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cindy Lu <lulu@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/vhost-user.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e5285df4ba..2d40b1b3e7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2677,10 +2677,20 @@ static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>                                            VIRTIO_CONFIG_S_DRIVER |
>                                            VIRTIO_CONFIG_S_DRIVER_OK);
>      } else {
> -        return vhost_user_set_status(dev, 0);
> +        return 0;
>      }
>  }
>  
> +static void vhost_user_reset_status(struct vhost_dev *dev)
> +{
> +    /* Set device status only for last queue pair */
> +    if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> +        return;
> +    }
> +
> +    vhost_user_set_status(dev, 0);
> +}
> +
>  const VhostOps user_ops = {
>          .backend_type = VHOST_BACKEND_TYPE_USER,
>          .vhost_backend_init = vhost_user_backend_init,
> @@ -2716,4 +2726,5 @@ const VhostOps user_ops = {
>          .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
>          .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
>          .vhost_dev_start = vhost_user_dev_start,
> +        .vhost_reset_status = vhost_user_reset_status,
>  };
> -- 
> 2.39.2
Re: [PATCH] vhost-user: send SET_STATUS 0 after GET_VRING_BASE
Posted by Yajun Wu 1 year ago 
On 4/20/2023 9:07 PM, Stefan Hajnoczi wrote:
>
> Setting the VIRTIO Device Status Field to 0 resets the device. The
> device's state is lost, including the vring configuration.
>
> vhost-user.c currently sends SET_STATUS 0 before GET_VRING_BASE. This
> risks confusion about the lifetime of the vhost-user state (e.g. vring
> last_avail_idx) across VIRTIO device reset.
>
> Eugenio Pérez <eperezma@redhat.com> adjusted the order for vhost-vdpa.c
> in commit c3716f260bff ("vdpa: move vhost reset after get vring base")
> and in that commit description suggested doing the same for vhost-user
> in the future.
>
> Go ahead and adjust vhost-user.c now. I ran various online code searches
> to identify vhost-user backends implementing SET_STATUS. It seems only
> DPDK implements SET_STATUS and Yajun Wu <yajunw@nvidia.com> has
> confirmed that it is safe to make this change.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cindy Lu <lulu@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/virtio/vhost-user.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index e5285df4ba..2d40b1b3e7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2677,10 +2677,20 @@ static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>                                             VIRTIO_CONFIG_S_DRIVER |
>                                             VIRTIO_CONFIG_S_DRIVER_OK);
>       } else {
> -        return vhost_user_set_status(dev, 0);
> +        return 0;
>       }
>   }
>
> +static void vhost_user_reset_status(struct vhost_dev *dev)
> +{
> +    /* Set device status only for last queue pair */
> +    if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> +        return;
> +    }
> +
> +    vhost_user_set_status(dev, 0);
> +}
> +
>   const VhostOps user_ops = {
>           .backend_type = VHOST_BACKEND_TYPE_USER,
>           .vhost_backend_init = vhost_user_backend_init,
> @@ -2716,4 +2726,5 @@ const VhostOps user_ops = {
>           .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
>           .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
>           .vhost_dev_start = vhost_user_dev_start,
> +        .vhost_reset_status = vhost_user_reset_status,
>   };
> --
> 2.39.2
>
Thank you for this fix.

Can you add protocol feature bit check, just like we do in 
vhost_user_dev_start?

     if (!virtio_has_feature(dev->protocol_features,
                             VHOST_USER_PROTOCOL_F_STATUS)) {
         return 0;
     }
Re: [PATCH] vhost-user: send SET_STATUS 0 after GET_VRING_BASE
Posted by Stefan Hajnoczi 12 months ago 
On Fri, Apr 21, 2023 at 01:30:48PM +0800, Yajun Wu wrote:
> 
> On 4/20/2023 9:07 PM, Stefan Hajnoczi wrote:
> > 
> > Setting the VIRTIO Device Status Field to 0 resets the device. The
> > device's state is lost, including the vring configuration.
> > 
> > vhost-user.c currently sends SET_STATUS 0 before GET_VRING_BASE. This
> > risks confusion about the lifetime of the vhost-user state (e.g. vring
> > last_avail_idx) across VIRTIO device reset.
> > 
> > Eugenio Pérez <eperezma@redhat.com> adjusted the order for vhost-vdpa.c
> > in commit c3716f260bff ("vdpa: move vhost reset after get vring base")
> > and in that commit description suggested doing the same for vhost-user
> > in the future.
> > 
> > Go ahead and adjust vhost-user.c now. I ran various online code searches
> > to identify vhost-user backends implementing SET_STATUS. It seems only
> > DPDK implements SET_STATUS and Yajun Wu <yajunw@nvidia.com> has
> > confirmed that it is safe to make this change.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Cindy Lu <lulu@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   hw/virtio/vhost-user.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index e5285df4ba..2d40b1b3e7 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -2677,10 +2677,20 @@ static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> >                                             VIRTIO_CONFIG_S_DRIVER |
> >                                             VIRTIO_CONFIG_S_DRIVER_OK);
> >       } else {
> > -        return vhost_user_set_status(dev, 0);
> > +        return 0;
> >       }
> >   }
> > 
> > +static void vhost_user_reset_status(struct vhost_dev *dev)
> > +{
> > +    /* Set device status only for last queue pair */
> > +    if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> > +        return;
> > +    }
> > +
> > +    vhost_user_set_status(dev, 0);
> > +}
> > +
> >   const VhostOps user_ops = {
> >           .backend_type = VHOST_BACKEND_TYPE_USER,
> >           .vhost_backend_init = vhost_user_backend_init,
> > @@ -2716,4 +2726,5 @@ const VhostOps user_ops = {
> >           .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
> >           .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
> >           .vhost_dev_start = vhost_user_dev_start,
> > +        .vhost_reset_status = vhost_user_reset_status,
> >   };
> > --
> > 2.39.2
> > 
> Thank you for this fix.
> 
> Can you add protocol feature bit check, just like we do in
> vhost_user_dev_start?
> 
>     if (!virtio_has_feature(dev->protocol_features,
>                             VHOST_USER_PROTOCOL_F_STATUS)) {
>         return 0;
>     }

Sure, will fix in v2.

Stefan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.