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

[Hanna Czenczek <hreitz@redhat.com>]

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).

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);
>   }