On Wed, 4 Oct 2023 at 06:44, Hanna Czenczek <hre...@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 <stefa...@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); > > } > >
