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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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?

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

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