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

From: Michael Qiu <qiudayu@archeros.com>
To: Jason Wang <jasowang@redhat.com>, mst@redhat.com, si-wei.liu@oracle.com
Cc: eperezma@redhat.com, lingshan.zhu@intel.com,
	qemu-devel@nongnu.org, lulu@redhat.com
Subject: Re: [PATCH 1/3] vhost: Refactor vhost_reset_device() in VhostOps
Date: Sat, 2 Apr 2022 13:14:46 +0800	[thread overview]
Message-ID: <f4ac0e4e-38ae-ecd6-6b0d-ab6abb535e54@archeros.com> (raw)
In-Reply-To: <c8e8459d-32b5-55ff-44f4-1f03edb8a597@redhat.com>

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.

> 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, 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;
> 
> 