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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]