|
| From: | Michael Qiu |
| Subject: | Re: [PATCH 1/3] vhost: Refactor vhost_reset_device() in VhostOps |
| Date: | Fri, 8 Apr 2022 16:38:26 +0800 |
| User-agent: | Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 |
在 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: http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg00949.htmlSeems 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.
Thanksand vhost-user-scsi also use device reset. Thanks, MichaelThanks.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.hindex 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;
| [Prev in Thread] | Current Thread | [Next in Thread] |