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 <hre...@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? My understanding is that SET_STATUS is mostly ignored by vhost-user back-ends today. Even those that implement it may not treat SET_STATUS 0 as equivalent to RESET_DEVICE. If you decide it's safe to make this change, please also update vhost-user.rst to document that front-ends should use SET_STATUS 0, RESET_DEVICE, and RESET_OWNER in order of preference. Stefan
signature.asc
Description: PGP signature