Re: [PATCH 1/3] vhost: Refactor vhost_reset_device() in VhostOps

[Si-Wei Liu]

On 4/1/2022 4:06 AM, Michael Qiu wrote:
> Currently in vhost framwork, vhost_reset_device() is misnamed.
> Actually, it should be vhost_reset_owner().
>
> In vhost user, it make compatible with reset device ops, but
> vhost kernel does not compatible with it, for vhost vdpa, it
> only implement reset device action.
>
> So we need seperate the function into vhost_reset_owner() and
> vhost_reset_device(). So that different backend could use the
> correct function.
>
> Signde-off-by: Michael Qiu <qiudayu@archeros.com>
> ---
>   hw/scsi/vhost-user-scsi.c         |  6 +++++-
>   hw/virtio/vhost-backend.c         |  4 ++--
>   hw/virtio/vhost-user.c            | 22 ++++++++++++++++++----
>   include/hw/virtio/vhost-backend.h |  2 ++
>   4 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 1b2f7ee..f179626 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -80,8 +80,12 @@ static void vhost_user_scsi_reset(VirtIODevice *vdev)
>           return;
>       }
>   
> -    if (dev->vhost_ops->vhost_reset_device) {
> +    if (virtio_has_feature(dev->protocol_features,
> +                           VHOST_USER_PROTOCOL_F_RESET_DEVICE) &&
This line change is not needed. VHOST_USER_PROTOCOL_F_RESET_DEVICE is 
guaranteed to be available if getting here.
> +                           dev->vhost_ops->vhost_reset_device) {
>           dev->vhost_ops->vhost_reset_device(dev);
> +    } else if (dev->vhost_ops->vhost_reset_owner) {
> +        dev->vhost_ops->vhost_reset_owner(dev);
Nope, drop these two lines. The caller of vhost_user_scsi_reset() 
doesn't expect vhost_reset_owner to be called in case vhost_reset_device 
is not implemented.

>       }
>   }
>   
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index e409a86..abbaa8b 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -191,7 +191,7 @@ static int vhost_kernel_set_owner(struct vhost_dev *dev)
>       return vhost_kernel_call(dev, VHOST_SET_OWNER, NULL);
>   }
>   
> -static int vhost_kernel_reset_device(struct vhost_dev *dev)
> +static int vhost_kernel_reset_owner(struct vhost_dev *dev)
>   {
>       return vhost_kernel_call(dev, VHOST_RESET_OWNER, NULL);
>   }
> @@ -317,7 +317,7 @@ const VhostOps kernel_ops = {
>           .vhost_get_features = vhost_kernel_get_features,
>           .vhost_set_backend_cap = vhost_kernel_set_backend_cap,
>           .vhost_set_owner = vhost_kernel_set_owner,
> -        .vhost_reset_device = vhost_kernel_reset_device,
> +        .vhost_reset_owner = vhost_kernel_reset_owner,
>           .vhost_get_vq_index = vhost_kernel_get_vq_index,
>   #ifdef CONFIG_VHOST_VSOCK
>           .vhost_vsock_set_guest_cid = vhost_kernel_vsock_set_guest_cid,
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 6abbc9d..4412008 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1475,16 +1475,29 @@ static int vhost_user_get_max_memslots(struct vhost_dev 
> *dev,
>       return 0;
>   }
>   
> +static int vhost_user_reset_owner(struct vhost_dev *dev)
> +{
> +    VhostUserMsg msg = {
> +        .hdr.request = VHOST_USER_RESET_OWNER,
> +        .hdr.flags = VHOST_USER_VERSION,
> +    };
> +
> +    return vhost_user_write(dev, &msg, NULL, 0);
> +}
> +
>   static int vhost_user_reset_device(struct vhost_dev *dev)
>   {
>       VhostUserMsg msg = {
> +        .hdr.request = VHOST_USER_RESET_DEVICE,
>           .hdr.flags = VHOST_USER_VERSION,
>       };
>   
> -    msg.hdr.request = virtio_has_feature(dev->protocol_features,
> -                                         VHOST_USER_PROTOCOL_F_RESET_DEVICE)
> -        ? VHOST_USER_RESET_DEVICE
> -        : VHOST_USER_RESET_OWNER;
> +    /* Caller must ensure the backend has VHOST_USER_PROTOCOL_F_RESET_DEVICE
> +     * support */
> +    if (!virtio_has_feature(dev->protocol_features,
> +                       VHOST_USER_PROTOCOL_F_RESET_DEVICE)) {
> +        return -EPERM;
> +    }
I think we can safely remove this check, since the caller already 
guarantees VHOST_USER_PROTOCOL_F_RESET_DEVICE is around as what your 
comment mentions.

The previous branch condition is to reuse the vhost_reset_device op for 
two different ends, but there's no actual user for 
VHOST_USER_RESET_OWNER historically.

Thanks,
-Siwei

>   
>       return vhost_user_write(dev, &msg, NULL, 0);
>   }
> @@ -2548,6 +2561,7 @@ const VhostOps user_ops = {
>           .vhost_set_features = vhost_user_set_features,
>           .vhost_get_features = vhost_user_get_features,
>           .vhost_set_owner = vhost_user_set_owner,
> +        .vhost_reset_owner = vhost_user_reset_owner,
>           .vhost_reset_device = vhost_user_reset_device,
>           .vhost_get_vq_index = vhost_user_get_vq_index,
>           .vhost_set_vring_enable = vhost_user_set_vring_enable,
> diff --git a/include/hw/virtio/vhost-backend.h 
> b/include/hw/virtio/vhost-backend.h
> index 81bf310..affeeb0 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -77,6 +77,7 @@ typedef int (*vhost_get_features_op)(struct vhost_dev *dev,
>                                        uint64_t *features);
>   typedef int (*vhost_set_backend_cap_op)(struct vhost_dev *dev);
>   typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
> +typedef int (*vhost_reset_owner_op)(struct vhost_dev *dev);
>   typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
>   typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
>   typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
> @@ -150,6 +151,7 @@ typedef struct VhostOps {
>       vhost_get_features_op vhost_get_features;
>       vhost_set_backend_cap_op vhost_set_backend_cap;
>       vhost_set_owner_op vhost_set_owner;
> +    vhost_reset_owner_op vhost_reset_owner;
>       vhost_reset_device_op vhost_reset_device;
>       vhost_get_vq_index_op vhost_get_vq_index;
>       vhost_set_vring_enable_op vhost_set_vring_enable;