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

[Jason Wang <jasowang@redhat.com>]

在 2022/4/2 下午1:14, Michael Qiu 写道:
>
>
> On 2022/4/2 10:38, Jason Wang wrote:
>>
>> 在 2022/4/1 下午7:06, Michael Qiu 写道:
>>> 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.
>>
>>
>> I see no reason when RESET_OWNER needs to be done for kernel backend.
>>
>
> In kernel vhost, RESET_OWNER  indeed do vhost device level reset: 
> vhost_net_reset_owner()
>
> static long vhost_net_reset_owner(struct vhost_net *n)
> {
> [...]
>         err = vhost_dev_check_owner(&n->dev);
>         if (err)
>                 goto done;
>         umem = vhost_dev_reset_owner_prepare();
>         if (!umem) {
>                 err = -ENOMEM;
>                 goto done;
>         }
>         vhost_net_stop(n, &tx_sock, &rx_sock);
>         vhost_net_flush(n);
>         vhost_dev_stop(&n->dev);
>         vhost_dev_reset_owner(&n->dev, umem);
>         vhost_net_vq_reset(n);
> [...]
>
> }
>
> In the history of QEMU, There is a commit:
> commit d1f8b30ec8dde0318fd1b98d24a64926feae9625
> Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Date:   Wed Sep 23 12:19:57 2015 +0800
>
>     vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE
>
>     Quote from Michael:
>
>         We really should rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE.
>
> but finally, it has been reverted by the author:
> commit 60915dc4691768c4dc62458bb3e16c843fab091d
> Author: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Date:   Wed Nov 11 21:24:37 2015 +0800
>
>     vhost: rename RESET_DEVICE backto RESET_OWNER
>
>     This patch basically reverts commit d1f8b30e.
>
>     It turned out that it breaks stuff, so revert it:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg00949.html
>
> Seems kernel take RESET_OWNER for reset,but QEMU never call to this 
> function to do a reset.


The question is, we manage to survive by not using RESET_OWNER for past 
10 years. Any reason that we want to use that now?

Note that the RESET_OWNER is only useful the process want to drop the 
its mm refcnt from vhost, it doesn't reset the device (e.g it does not 
even call vhost_vq_reset()).

(Especially, it was deprecated in by the vhost-user protocol since its 
semantics is ambiguous)


>
>> And if I understand the code correctly, vhost-user "abuse" 
>> RESET_OWNER for reset. So the current code looks fine?
>>
>>
>>>
>>> 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) &&
>>> + 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);
>>
>>
>> Actually, I fail to understand why we need an indirection via 
>> vhost_ops. It's guaranteed to be vhost_user_ops.
>>
>>
>>>       }
>>>   }
>>> 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,
>>
>>
>> I think we can delete the current vhost_reset_device() since it not 
>> used in any code path.
>>
>
> I planned to use it for vDPA reset, 


For vhost-vDPA it can call vhost_vdpa_reset_device() directly.

As I mentioned before, the only user of vhost_reset_device config ops is 
vhost-user-scsi but it should directly call the vhost_user_reset_device().

Thanks


> and vhost-user-scsi also use device reset.
>
> Thanks,
> Michael
>
>> Thanks
>>
>>
>>>           .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;
>>> +    }
>>>       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;
>>
>>
>