Re: [PATCH 6/6] vhost-user: Have reset_status fall back to reset

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Jul 19, 2023 at 04:27:58PM +0200, Hanna Czenczek wrote:
> On 19.07.23 16:11, Hanna Czenczek wrote:
> > On 18.07.23 17:10, Stefan Hajnoczi wrote:
> > > On Tue, Jul 11, 2023 at 05:52:28PM +0200, Hanna Czenczek wrote:
> > > > The only user of vhost_user_reset_status() is vhost_dev_stop(), which
> > > > only uses it as a fall-back to stop the back-end if it does not support
> > > > SUSPEND.  However, vhost-user's implementation is a no-op unless the
> > > > back-end supports SET_STATUS.
> > > > 
> > > > vhost-vdpa's implementation instead just calls
> > > > vhost_vdpa_reset_device(), implying that it's OK to fully reset the
> > > > device if SET_STATUS is not supported.
> > > > 
> > > > To be fair, vhost_vdpa_reset_device() does nothing but to set
> > > > the status
> > > > to zero.  However, that may well be because vhost-vdpa has no method
> > > > besides this to reset a device.  In contrast, vhost-user has
> > > > RESET_DEVICE and a RESET_OWNER, which can be used instead.
> > > > 
> > > > While it is not entirely clear from documentation or git logs, from
> > > > discussions and the order of vhost-user protocol features, it
> > > > appears to
> > > > me as if RESET_OWNER originally had no real meaning for vhost-user, and
> > > > was thus used to signal a device reset to the back-end.  Then,
> > > > RESET_DEVICE was introduced, to have a well-defined dedicated reset
> > > > command.  Finally, vhost-user received full STATUS support, including
> > > > SET_STATUS, so setting the device status to 0 is now the preferred way
> > > > of resetting a device.  Still, RESET_DEVICE and RESET_OWNER should
> > > > remain valid as fall-backs.
> > > > 
> > > > Therefore, have vhost_user_reset_status() fall back to
> > > > vhost_user_reset_device() if the back-end has no STATUS support.
> > > > 
> > > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> > > > ---
> > > >   hw/virtio/vhost-user.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index 4507de5a92..53a881ec2a 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -2833,6 +2833,8 @@ static void vhost_user_reset_status(struct
> > > > vhost_dev *dev)
> > > >       if (virtio_has_feature(dev->protocol_features,
> > > >                              VHOST_USER_PROTOCOL_F_STATUS)) {
> > > >           vhost_user_set_status(dev, 0);
> > > > +    } else {
> > > > +        vhost_user_reset_device(dev);
> > > >       }
> > > >   }
> > > Did you check whether DPDK treats setting the status to 0 as equivalent
> > > to RESET_DEVICE?
> > 
> > If it doesn’t, what’s even the point of using reset_status?
> 
> Sorry, I’m being unclear, and I think this may be important because it ties
> into the question from patch 1, what qemu is even trying to do by running
> SET_STATUS(0) vhost_dev_stop(), so here’s what gave me the impression that
> SET_STATUS(0) and RESET_DEVICE should be equivalent:
> 
> vhost-vdpa.c runs SET_STATUS(0) in a function called
> vhost_vdpa_reset_device().  This is one thing that gave me the impression
> that this is about an actual full reset.
> 
> Another is the whole discussion that we’ve had.  vhost_dev_stop() does not
> call a `vhost_reset_device()` function, it calls `vhost_reset_status()`. 
> Still, we were always talking about resetting the device.

There is some hacky stuff with struct vhost_dev's vq_index_end and
multi-queue devices. I think it's because multi-queue vhost-net device
consist of many vhost_devs and NetClientStates, so certain vhost
operations are skipped unless this is the "first" or "last" vhost_dev
from a large aggregate vhost-net device. That might be responsible for
part of the weirdness.

> 
> It doesn’t make sense to me that vDPA would provide no function to fully
> reset a device, while vhost-user does.  Being able to reset a device sounds
> vital to me.  This also gave me the impression that SET_STATUS(0) on vDPA at
> least is functionally equivalent to a full device reset.
> 
> 
> Maybe SET_STATUS(0) does mean a full device reset on vDPA, but not on
> vhost-user.  That would be a real shame, so I assumed this would not be the
> case; that SET_STATUS(0) does the same thing on both protocols.

Yes, exactly. It has the real VIRTIO spec meaning in vDPA. In vhost-user
it's currently only used by DPDK as a hint for when device
initialization is complete:
https://github.com/DPDK/dpdk/commit/41d201804c4c44738168e2d247d3b1780845faa1

> The virtio specification says “Writing 0 into this field resets the device.”
> about the device_status field.
> 
> This also makes sense, because the device_status field is basically used to
> tell the device that a driver has taken control.  If reset, this indicates
> the driver has given up control, and to me this is a point where a device
> should fully reset itself.
> 
> So all in all, I can’t see the rationale why any implementation that
> supports SET_STATUS would decide to treat SET_STATUS(0) not as equivalent or
> a superset of RESET_DEVICE.  I may be wrong, and this might explain a whole
> deal about what kind of background operations we hope to stop with
> SET_STATUS(0).

I would like vhost-user devices to implement SET_STATUS according to the
VIRTIO specification in the future and they can do that. But I think
front-ends should continue sending RESET_DEVICE in order to support old
devices.

Stefan

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