Re: [PATCH v2 1/3] vhost-user: do not send RESET_OWNER on device reset

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, 4 Oct 2023 at 06:44, Hanna Czenczek <hreitz@redhat.com> wrote:
>
> On 04.10.23 03:45, Stefan Hajnoczi wrote:
> > The VHOST_USER_RESET_OWNER message is deprecated in the spec:
> >
> >     This is no longer used. Used to be sent to request disabling all
> >     rings, but some back-ends interpreted it to also discard connection
> >     state (this interpretation would lead to bugs).  It is recommended
> >     that back-ends either ignore this message, or use it to disable all
> >     rings.
>
> According to the spec, it is then indeed better to not call it in
> vhost_user_reset_device, because it seems like it would be interpreted
> as something completely different.
>
> However, between the three back-end implementations of vhost-user I know
> of (libvhost-user, DPDK, the vhost crates; four if you count RSD), none
> implement RESET_DEVICE.  libvhost-user and DPDK do implement
> RESET_OWNER, though, and they both do it by resetting the device, not by
> disabling any vring.  The vhost crate also implements RESET_OWNER, but
> it doesn’t do anything but forward it as such to the actual device
> implementation (virtiofsd doesn’t implement this function, so ignores
> it).  It does document that it would disable all vrings, but does so in
> the past and has marked it deprecated (ever since the method was
> introduced in the fourth commit to the repository, making it extremely
> unlikely that anyone would implement it).

Hi Hanna,
vhost-user-backend still seems to reset all vhost-user protocol state,
making RESET_OWNER unusable:
https://github.com/rust-vmm/vhost/blob/main/crates/vhost-user-backend/src/handler.rs#L230

Have I missed something?

Stefan

>
> So I would like to know why the spec says that it would disable all
> vrings, when none of the implementations (qemu, libvhost-user, DPDK)
> agree on that.  Let me look it up:
>
> Before commit c61f09ed855, it did say “stopping” instead of
> “disabling”.  The commit doesn’t explain why it changed this. Commit
> a586e65bbd0 (just a week prior) deprecated the command, changing it from
> “connection is about to be closed, [front-end] will no longer own this
> connection” to “deprecated, used to be sent to request stopping all
> vrings”.  To me, the front-end closing the connection sounds like a good
> point to reset, which would indeed stop all vrings, but not just that.
> Notably, qemu agrees, because RESET_OWNER is used only in the
> vhost_user_reset_device() function. a586e65bbd0^ removed that function’s
> use, though, specifically because it would cause a reset, when the
> intention was just to stop.
>
> So it sounds to me like “used to be sent to request stopping all vrings”
> is rather what vhost_net wanted, but specifically not what the message
> did, which was anything between nothing and a reset, I presume (because
> it never specified what the back-end was supposed to do, though
> apparently libvhost-user and DPDK both took it to mean reset).  Why it
> was then changed to “disabling”, I absolutely cannot say.
>
> Now, the code change here is indeed effectively a no-op, as you deduce
> below, but in the context of the whole series the situation is a bit
> different: As far as I understand, the point is to have guest-initiated
> resets be forwarded to back-ends.  But by removing the RESET_OWNER
> fallback, no back-end will actually do a reset still.
>
> I understand that as per the specification, using RESET_OWNER for
> resetting is wrong.  But all implementations that implemented it before
> it was deprecated do interpret it as a reset, so I don’t think using it
> as a fallback is actually wrong.
>
> Hanna
>
> > The only caller of vhost_user_reset_device() is vhost_user_scsi_reset().
> > It checks that F_RESET_DEVICE was negotiated before calling it:
> >
> >    static void vhost_user_scsi_reset(VirtIODevice *vdev)
> >    {
> >        VHostSCSICommon *vsc = VHOST_SCSI_COMMON(vdev);
> >        struct vhost_dev *dev = &vsc->dev;
> >
> >        /*
> >         * Historically, reset was not implemented so only reset devices
> >         * that are expecting it.
> >         */
> >        if (!virtio_has_feature(dev->protocol_features,
> >                                VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
> >            return;
> >        }
> >
> >        if (dev->vhost_ops->vhost_reset_device) {
> >            dev->vhost_ops->vhost_reset_device(dev);
> >        }
> >    }
> >
> > Therefore VHOST_USER_RESET_OWNER is actually never sent by
> > vhost_user_reset_device(). Remove the dead code. This effectively moves
> > the vhost-user protocol specific code from vhost-user-scsi.c into
> > vhost-user.c where it belongs.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   hw/scsi/vhost-user-scsi.c |  9 ---------
> >   hw/virtio/vhost-user.c    | 13 +++++++++----
> >   2 files changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> > index ee99b19e7a..8582b2e8ab 100644
> > --- a/hw/scsi/vhost-user-scsi.c
> > +++ b/hw/scsi/vhost-user-scsi.c
> > @@ -71,15 +71,6 @@ static void vhost_user_scsi_reset(VirtIODevice *vdev)
> >       VHostSCSICommon *vsc = VHOST_SCSI_COMMON(vdev);
> >       struct vhost_dev *dev = &vsc->dev;
> >
> > -    /*
> > -     * Historically, reset was not implemented so only reset devices
> > -     * that are expecting it.
> > -     */
> > -    if (!virtio_has_feature(dev->protocol_features,
> > -                            VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
> > -        return;
> > -    }
> > -
> >       if (dev->vhost_ops->vhost_reset_device) {
> >           dev->vhost_ops->vhost_reset_device(dev);
> >       }
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 8dcf049d42..7bed9ad7d5 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1492,12 +1492,17 @@ static int vhost_user_reset_device(struct vhost_dev *dev)
> >   {
> >       VhostUserMsg msg = {
> >           .hdr.flags = VHOST_USER_VERSION,
> > +        .hdr.request = VHOST_USER_RESET_DEVICE,
> >       };
> >
> > -    msg.hdr.request = virtio_has_feature(dev->protocol_features,
> > -                                         VHOST_USER_PROTOCOL_F_RESET_DEVICE)
> > -        ? VHOST_USER_RESET_DEVICE
> > -        : VHOST_USER_RESET_OWNER;
> > +    /*
> > +     * Historically, reset was not implemented so only reset devices
> > +     * that are expecting it.
> > +     */
> > +    if (!virtio_has_feature(dev->protocol_features,
> > +                            VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
> > +        return -ENOSYS;
> > +    }
> >
> >       return vhost_user_write(dev, &msg, NULL, 0);
> >   }
>
>