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

[Jason Wang]

On Sat, Apr 9, 2022 at 1:17 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 4/8/2022 1:38 AM, Michael Qiu wrote:
> >
> >
> > 在 2022/4/7 15:35, Jason Wang 写道:
> >>
> >> 在 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:
> >>>
> >>> https://urldefense.com/v3/__http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg00949.html__;!!ACWV5N9M2RV99hQ!bgCEUDnSCLVO8LxXlwcdiHrtwqH5ip_sVcKscrtJve5eSzJfNN9JZuf-8HQIQ1Re$
> >>>
> >>> 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)
> >>
> >>
> >
> > So, you prefer to directly remove RESET_OWNER support now?
> >
> >>>
> >>>> 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().
> >>
> >
> >
> > Yes, but in the next patch I reuse it to reset backend device in
> > vhost_net.
> I recall vhost-user has a different way to reset the net backend,

Yes, it has VHOST_USER_RESET_DEVICE.

Thanks

> so
> probably we can leave out implementing the .vhost_reset_device() op for
> vhost-user as Jason suggested. In that case vhost-user-scsi will call
> into vhost_user_reset_device() directly without using the
> .vhost_reset_device() op.
>
> -Siwei
>
> >
> >
> >> 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;
> >>>>
> >>>>
> >>>
> >>
> >>
> >>
> >
>