| Message ID | 1648811173-15293-2-git-send-email-qiudayu@archeros.com |
|---|---|
| State | New |
| Headers | show |
| Series | Refactor vhost device reset | expand |
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;
On 2022/4/2 8:44, Si-Wei Liu wrote: > > > 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. > You are right, I will drop these two lines and remove VHOST_USER_PROTOCOL_F_RESET_DEVICE check. >> } >> } >> 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. > I think it probely worth to check, because for vhost_net_stop() it does not check this flag, otherwise we should check if the backend is vhost user with this flag enabled. > 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; > >
在 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. 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. 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;
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; > >
在 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; >> >> >
在 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.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) > > 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. > 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; >>> >>> >> > > >
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, 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; >>>> >>>> >>> >> >> >> >
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; > >>>> > >>>> > >>> > >> > >> > >> > > >
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); } } 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; + } 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;