hw/net/vhost_net.c | 24 +++++++++++++++++++++--- hw/virtio/vhost-vdpa.c | 15 +++++++++------ hw/virtio/vhost.c | 15 ++++++++++++++- include/hw/virtio/vhost.h | 1 + 4 files changed, 45 insertions(+), 10 deletions(-)
Currently, when VM poweroff, it will trigger vdpa
device(such as mlx bluefield2 VF) reset many times(with 1 datapath
queue pair and one control queue, triggered 3 times), this
leads to below issue:
vhost VQ 2 ring restore failed: -22: Invalid argument (22)
This because in vhost_net_stop(), it will stop all vhost device bind to
this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
, then stop the queue: vhost_virtqueue_stop().
In vhost_dev_stop(), it resets the device, which clear some flags
in low level driver, and in next loop(stop other vhost backends),
qemu try to stop the queue corresponding to the vhost backend,
the driver finds that the VQ is invalied, this is the root cause.
To solve the issue, vdpa should set vring unready, and
remove reset ops in device stop: vhost_dev_start(hdev, false).
and implement a new function vhost_dev_reset, only reset backend
device after all vhost(per-queue) stoped.
Signed-off-by: Michael Qiu<qiudayu@archeros.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
v4 --> v3
Nothing changed, becasue of issue with mimecast,
when the From: tag is different from the sender,
the some mail client will take the patch as an
attachment, RESEND v3 does not work, So resend
the patch as v4
v3 --> v2:
Call vhost_dev_reset() at the end of vhost_net_stop().
Since the vDPA device need re-add the status bit
VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
simply, add them inside vhost_vdpa_reset_device, and
the only way calling vhost_vdpa_reset_device is in
vhost_net_stop(), so it keeps the same behavior as before.
v2 --> v1:
Implement a new function vhost_dev_reset,
reset the backend kernel device at last.
---
hw/net/vhost_net.c | 24 +++++++++++++++++++++---
hw/virtio/vhost-vdpa.c | 15 +++++++++------
hw/virtio/vhost.c | 15 ++++++++++++++-
include/hw/virtio/vhost.h | 1 +
4 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 30379d2..422c9bf 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
int total_notifiers = data_queue_pairs * 2 + cvq;
VirtIONet *n = VIRTIO_NET(dev);
int nvhosts = data_queue_pairs + cvq;
- struct vhost_net *net;
+ struct vhost_net *net = NULL;
int r, e, i, index_end = data_queue_pairs * 2;
NetClientState *peer;
@@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
err_start:
while (--i >= 0) {
peer = qemu_get_peer(ncs , i);
- vhost_net_stop_one(get_vhost_net(peer), dev);
+
+ net = get_vhost_net(peer);
+
+ vhost_net_stop_one(net, dev);
}
+
+ /* We only reset backend vdpa device */
+ if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
+ vhost_dev_reset(&net->dev);
+ }
+
e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
if (e < 0) {
fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
@@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
VirtIONet *n = VIRTIO_NET(dev);
NetClientState *peer;
+ struct vhost_net *net = NULL;
int total_notifiers = data_queue_pairs * 2 + cvq;
int nvhosts = data_queue_pairs + cvq;
int i, r;
@@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
} else {
peer = qemu_get_peer(ncs, n->max_queue_pairs);
}
- vhost_net_stop_one(get_vhost_net(peer), dev);
+
+ net = get_vhost_net(peer);
+
+ vhost_net_stop_one(net, dev);
+ }
+
+ /* We only reset backend vdpa device */
+ if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
+ vhost_dev_reset(&net->dev);
}
r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index c5ed7a3..3ef0199 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
trace_vhost_vdpa_reset_device(dev, status);
+
+ /* Add back this status, so that the device could work next time*/
+ vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
+ VIRTIO_CONFIG_S_DRIVER);
+
return ret;
}
@@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
return idx;
}
-static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
+static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
{
int i;
trace_vhost_vdpa_set_vring_ready(dev);
for (i = 0; i < dev->nvqs; ++i) {
struct vhost_vring_state state = {
.index = dev->vq_index + i,
- .num = 1,
+ .num = ready,
};
vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
}
@@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
if (unlikely(!ok)) {
return -1;
}
- vhost_vdpa_set_vring_ready(dev);
+ vhost_vdpa_set_vring_ready(dev, 1);
} else {
+ vhost_vdpa_set_vring_ready(dev, 0);
ok = vhost_vdpa_svqs_stop(dev);
if (unlikely(!ok)) {
return -1;
@@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
memory_listener_register(&v->listener, &address_space_memory);
return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
} else {
- vhost_vdpa_reset_device(dev);
- vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
- VIRTIO_CONFIG_S_DRIVER);
memory_listener_unregister(&v->listener);
return 0;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b643f42..7e0cdb6 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1820,7 +1820,6 @@ fail_features:
void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
{
int i;
-
/* should only be called after backend is connected */
assert(hdev->vhost_ops);
@@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
return -ENOSYS;
}
+
+int vhost_dev_reset(struct vhost_dev *hdev)
+{
+ int ret = 0;
+
+ /* should only be called after backend is connected */
+ assert(hdev->vhost_ops);
+
+ if (hdev->vhost_ops->vhost_reset_device) {
+ ret = hdev->vhost_ops->vhost_reset_device(hdev);
+ }
+
+ return ret;
+}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 58a73e7..b8b7c20 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
void vhost_dev_cleanup(struct vhost_dev *hdev);
int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_dev_reset(struct vhost_dev *hdev);
int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
--
1.8.3.1Now adays, vhost framework does a misnamed for vhost_reset_device(), actually we need seperate vhost_reset_device() and vhost_reset_owner(), this patchset refactor it, and make different backend call the right function. Base on those work, fix an issue of vdpa device reset for several times. Test with kernel vhost, vhost-vdpa, DPDK vhost-user(vdpa), with shutdown ,reboot, and load/unload virtio_net driver in guest. Michael Qiu (3): vhost: Refactor vhost_reset_device() in VhostOps vhost: add vhost_dev_reset() vdpa: reset the backend device in the end of vhost_net_stop() hw/net/vhost_net.c | 22 +++++++++++++++++++--- hw/scsi/vhost-user-scsi.c | 6 +++++- hw/virtio/vhost-backend.c | 4 ++-- hw/virtio/vhost-user.c | 22 ++++++++++++++++++---- hw/virtio/vhost-vdpa.c | 15 +++++++++------ hw/virtio/vhost.c | 14 ++++++++++++++ include/hw/virtio/vhost-backend.h | 2 ++ include/hw/virtio/vhost.h | 1 + 8 files changed, 70 insertions(+), 16 deletions(-) -- 1.8.3.1
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) &&
+ 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;
--
1.8.3.1
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;
>
>
Not all vhost-user backends support ops->vhost_reset_device(). Instead
of adding backend check and call backend ops directly, it's better to
implement a function in vhost framework, so that it could hide vhost_ops
details.
SIgned-off-by: Michael Qiu <qiudayu@archeros.com>
---
hw/virtio/vhost.c | 14 ++++++++++++++
include/hw/virtio/vhost.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b643f42..26667ae 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1854,3 +1854,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
return -ENOSYS;
}
+
+int vhost_dev_reset(struct vhost_dev *hdev)
+{
+ int ret = 0;
+
+ /* should only be called after backend is connected */
+ assert(hdev->vhost_ops);
+
+ if (hdev->vhost_ops->vhost_reset_device) {
+ ret = hdev->vhost_ops->vhost_reset_device(hdev);
+ }
+
+ return ret;
+}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 58a73e7..b8b7c20 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
void vhost_dev_cleanup(struct vhost_dev *hdev);
int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
+int vhost_dev_reset(struct vhost_dev *hdev);
int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
--
1.8.3.1
On 4/1/2022 4:06 AM, Michael Qiu wrote:
> Not all vhost-user backends support ops->vhost_reset_device(). Instead
> of adding backend check and call backend ops directly, it's better to
> implement a function in vhost framework, so that it could hide vhost_ops
> details.
>
> SIgned-off-by: Michael Qiu <qiudayu@archeros.com>
> ---
> hw/virtio/vhost.c | 14 ++++++++++++++
> include/hw/virtio/vhost.h | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index b643f42..26667ae 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1854,3 +1854,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>
> return -ENOSYS;
> }
> +
> +int vhost_dev_reset(struct vhost_dev *hdev)
Maybe vhost_user_scsi_reset() can call this function instead?
-Siwei
> +{
> + int ret = 0;
> +
> + /* should only be called after backend is connected */
> + assert(hdev->vhost_ops);
> +
> + if (hdev->vhost_ops->vhost_reset_device) {
> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
> + }
> +
> + return ret;
> +}
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 58a73e7..b8b7c20 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> void vhost_dev_cleanup(struct vhost_dev *hdev);
> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_dev_reset(struct vhost_dev *hdev);
> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>
Currently, when VM poweroff, it will trigger vdpa
device(such as mlx bluefield2 VF) reset many times(with 1 datapath
queue pair and one control queue, triggered 3 times), this
leads to below issue:
vhost VQ 2 ring restore failed: -22: Invalid argument (22)
This because in vhost_net_stop(), it will stop all vhost device bind to
this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
, then stop the queue: vhost_virtqueue_stop().
In vhost_dev_stop(), it resets the device, which clear some flags
in low level driver, and in next loop(stop other vhost backends),
qemu try to stop the queue corresponding to the vhost backend,
the driver finds that the VQ is invalied, this is the root cause.
To solve the issue, vdpa should set vring unready, and
remove reset ops in device stop: vhost_dev_start(hdev, false).
and implement a new function vhost_dev_reset, only reset backend
device after all vhost(per-queue) stopped.
Signed-off-by: Michael Qiu<qiudayu@archeros.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
v5 --> v4:
move vhost_dev_reset() call after set_guest_notifiers
remove implementation of vhost_dev_reset()
remove backend check for VHOST_BACKEND_TYPE_VDPA
v4 --> v3:
Nothing changed, becasue of issue with mimecast,
when the From: tag is different from the sender,
the some mail client will take the patch as an
attachment, RESEND v3 does not work, So resend
the patch as v4
v3 --> v2:
Call vhost_dev_reset() at the end of vhost_net_stop().
Since the vDPA device need re-add the status bit
VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
simply, add them inside vhost_vdpa_reset_device, and
the only way calling vhost_vdpa_reset_device is in
vhost_net_stop(), so it keeps the same behavior as before.
v2 --> v1:
Implement a new function vhost_dev_reset,
reset the backend kernel device at last.
---
hw/net/vhost_net.c | 22 +++++++++++++++++++---
hw/virtio/vhost-vdpa.c | 15 +++++++++------
2 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 30379d2..30c76ca 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
int total_notifiers = data_queue_pairs * 2 + cvq;
VirtIONet *n = VIRTIO_NET(dev);
int nvhosts = data_queue_pairs + cvq;
- struct vhost_net *net;
+ struct vhost_net *net = NULL;
int r, e, i, index_end = data_queue_pairs * 2;
NetClientState *peer;
@@ -391,13 +391,21 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
err_start:
while (--i >= 0) {
peer = qemu_get_peer(ncs , i);
- vhost_net_stop_one(get_vhost_net(peer), dev);
+
+ net = get_vhost_net(peer);
+
+ vhost_net_stop_one(net, dev);
}
+
e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
if (e < 0) {
fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
fflush(stderr);
}
+
+ if (net) {
+ vhost_dev_reset(&net->dev);
+ }
err:
return r;
}
@@ -410,6 +418,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
VirtIONet *n = VIRTIO_NET(dev);
NetClientState *peer;
+ struct vhost_net *net = NULL;
int total_notifiers = data_queue_pairs * 2 + cvq;
int nvhosts = data_queue_pairs + cvq;
int i, r;
@@ -420,7 +429,10 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
} else {
peer = qemu_get_peer(ncs, n->max_queue_pairs);
}
- vhost_net_stop_one(get_vhost_net(peer), dev);
+
+ net = get_vhost_net(peer);
+
+ vhost_net_stop_one(net, dev);
}
r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
@@ -429,6 +441,10 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
fflush(stderr);
}
assert(r >= 0);
+
+ if (net) {
+ vhost_dev_reset(&net->dev);
+ }
}
void vhost_net_cleanup(struct vhost_net *net)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index c5ed7a3..3ef0199 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
trace_vhost_vdpa_reset_device(dev, status);
+
+ /* Add back this status, so that the device could work next time*/
+ vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
+ VIRTIO_CONFIG_S_DRIVER);
+
return ret;
}
@@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
return idx;
}
-static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
+static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
{
int i;
trace_vhost_vdpa_set_vring_ready(dev);
for (i = 0; i < dev->nvqs; ++i) {
struct vhost_vring_state state = {
.index = dev->vq_index + i,
- .num = 1,
+ .num = ready,
};
vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
}
@@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
if (unlikely(!ok)) {
return -1;
}
- vhost_vdpa_set_vring_ready(dev);
+ vhost_vdpa_set_vring_ready(dev, 1);
} else {
+ vhost_vdpa_set_vring_ready(dev, 0);
ok = vhost_vdpa_svqs_stop(dev);
if (unlikely(!ok)) {
return -1;
@@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
memory_listener_register(&v->listener, &address_space_memory);
return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
} else {
- vhost_vdpa_reset_device(dev);
- vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
- VIRTIO_CONFIG_S_DRIVER);
memory_listener_unregister(&v->listener);
return 0;
--
1.8.3.1On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
>
> Currently, when VM poweroff, it will trigger vdpa
> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
> queue pair and one control queue, triggered 3 times), this
> leads to below issue:
>
> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>
> This because in vhost_net_stop(), it will stop all vhost device bind to
> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
> , then stop the queue: vhost_virtqueue_stop().
>
> In vhost_dev_stop(), it resets the device, which clear some flags
> in low level driver, and in next loop(stop other vhost backends),
> qemu try to stop the queue corresponding to the vhost backend,
> the driver finds that the VQ is invalied, this is the root cause.
>
> To solve the issue, vdpa should set vring unready, and
> remove reset ops in device stop: vhost_dev_start(hdev, false).
>
> and implement a new function vhost_dev_reset, only reset backend
> device after all vhost(per-queue) stoped.
Typo.
>
> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
Rethink this patch, consider there're devices that don't support
set_vq_ready(). I wonder if we need
1) uAPI to tell the user space whether or not it supports set_vq_ready()
2) userspace will call SET_VRING_ENABLE() when the device supports
otherwise it will use RESET.
And for safety, I suggest tagging this as 7.1.
> ---
> v4 --> v3
> Nothing changed, becasue of issue with mimecast,
> when the From: tag is different from the sender,
> the some mail client will take the patch as an
> attachment, RESEND v3 does not work, So resend
> the patch as v4
>
> v3 --> v2:
> Call vhost_dev_reset() at the end of vhost_net_stop().
>
> Since the vDPA device need re-add the status bit
> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
> simply, add them inside vhost_vdpa_reset_device, and
> the only way calling vhost_vdpa_reset_device is in
> vhost_net_stop(), so it keeps the same behavior as before.
>
> v2 --> v1:
> Implement a new function vhost_dev_reset,
> reset the backend kernel device at last.
> ---
> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
> hw/virtio/vhost-vdpa.c | 15 +++++++++------
> hw/virtio/vhost.c | 15 ++++++++++++++-
> include/hw/virtio/vhost.h | 1 +
> 4 files changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 30379d2..422c9bf 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> int total_notifiers = data_queue_pairs * 2 + cvq;
> VirtIONet *n = VIRTIO_NET(dev);
> int nvhosts = data_queue_pairs + cvq;
> - struct vhost_net *net;
> + struct vhost_net *net = NULL;
> int r, e, i, index_end = data_queue_pairs * 2;
> NetClientState *peer;
>
> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> err_start:
> while (--i >= 0) {
> peer = qemu_get_peer(ncs , i);
> - vhost_net_stop_one(get_vhost_net(peer), dev);
> +
> + net = get_vhost_net(peer);
> +
> + vhost_net_stop_one(net, dev);
> }
> +
> + /* We only reset backend vdpa device */
> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
> + vhost_dev_reset(&net->dev);
> + }
> +
> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> if (e < 0) {
> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> VirtIONet *n = VIRTIO_NET(dev);
> NetClientState *peer;
> + struct vhost_net *net = NULL;
> int total_notifiers = data_queue_pairs * 2 + cvq;
> int nvhosts = data_queue_pairs + cvq;
> int i, r;
> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> } else {
> peer = qemu_get_peer(ncs, n->max_queue_pairs);
> }
> - vhost_net_stop_one(get_vhost_net(peer), dev);
> +
> + net = get_vhost_net(peer);
> +
> + vhost_net_stop_one(net, dev);
> + }
> +
> + /* We only reset backend vdpa device */
> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
> + vhost_dev_reset(&net->dev);
> }
So we've already reset the device in vhost_vdpa_dev_start(), any
reason we need to do it again here?
>
> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index c5ed7a3..3ef0199 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>
> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> trace_vhost_vdpa_reset_device(dev, status);
> +
> + /* Add back this status, so that the device could work next time*/
> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> + VIRTIO_CONFIG_S_DRIVER);
This seems to contradict the semantic of reset.
> +
> return ret;
> }
>
> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> return idx;
> }
>
> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
> {
> int i;
> trace_vhost_vdpa_set_vring_ready(dev);
> for (i = 0; i < dev->nvqs; ++i) {
> struct vhost_vring_state state = {
> .index = dev->vq_index + i,
> - .num = 1,
> + .num = ready,
> };
> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> }
> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> if (unlikely(!ok)) {
> return -1;
> }
> - vhost_vdpa_set_vring_ready(dev);
> + vhost_vdpa_set_vring_ready(dev, 1);
> } else {
> + vhost_vdpa_set_vring_ready(dev, 0);
> ok = vhost_vdpa_svqs_stop(dev);
> if (unlikely(!ok)) {
> return -1;
> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> memory_listener_register(&v->listener, &address_space_memory);
> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> } else {
> - vhost_vdpa_reset_device(dev);
> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> - VIRTIO_CONFIG_S_DRIVER);
> memory_listener_unregister(&v->listener);
>
> return 0;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index b643f42..7e0cdb6 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1820,7 +1820,6 @@ fail_features:
> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> {
> int i;
> -
Unnecessary changes.
> /* should only be called after backend is connected */
> assert(hdev->vhost_ops);
>
> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>
> return -ENOSYS;
> }
> +
> +int vhost_dev_reset(struct vhost_dev *hdev)
> +{
Let's use a separate patch for this.
Thanks
> + int ret = 0;
> +
> + /* should only be called after backend is connected */
> + assert(hdev->vhost_ops);
> +
> + if (hdev->vhost_ops->vhost_reset_device) {
> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
> + }
> +
> + return ret;
> +}
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 58a73e7..b8b7c20 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> void vhost_dev_cleanup(struct vhost_dev *hdev);
> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> +int vhost_dev_reset(struct vhost_dev *hdev);
> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>
> --
> 1.8.3.1
>
>
>
On 3/31/2022 7:53 PM, Jason Wang wrote:
> On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
>> Currently, when VM poweroff, it will trigger vdpa
>> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
>> queue pair and one control queue, triggered 3 times), this
>> leads to below issue:
>>
>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>
>> This because in vhost_net_stop(), it will stop all vhost device bind to
>> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
>> , then stop the queue: vhost_virtqueue_stop().
>>
>> In vhost_dev_stop(), it resets the device, which clear some flags
>> in low level driver, and in next loop(stop other vhost backends),
>> qemu try to stop the queue corresponding to the vhost backend,
>> the driver finds that the VQ is invalied, this is the root cause.
>>
>> To solve the issue, vdpa should set vring unready, and
>> remove reset ops in device stop: vhost_dev_start(hdev, false).
>>
>> and implement a new function vhost_dev_reset, only reset backend
>> device after all vhost(per-queue) stoped.
> Typo.
>
>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
> Rethink this patch, consider there're devices that don't support
> set_vq_ready(). I wonder if we need
>
> 1) uAPI to tell the user space whether or not it supports set_vq_ready()
I guess what's more relevant here is to define the uAPI semantics for
unready i.e. set_vq_ready(0) for resuming/stopping virtqueue processing,
as starting vq is comparatively less ambiguous. Considering the
likelihood that this interface may be used for live migration, it would
be nice to come up with variants such as 1) discard inflight request
v.s. 2) waiting for inflight processing to be done, and 3) timeout in
waiting.
> 2) userspace will call SET_VRING_ENABLE() when the device supports
> otherwise it will use RESET.
Are you looking to making virtqueue resume-able through the new
SET_VRING_ENABLE() uAPI?
I think RESET is inevitable in some case, i.e. when guest initiates
device reset by writing 0 to the status register. For suspend/resume and
live migration use cases, indeed RESET can be substituted with
SET_VRING_ENABLE. Again, it'd need quite some code refactoring to
accommodate this change. Although I'm all for it, it'd be the best to
lay out the plan for multiple phases rather than overload this single
patch too much. You can count my time on this endeavor if you don't mind. :)
>
> And for safety, I suggest tagging this as 7.1.
+1
Regards,
-Siwei
>
>> ---
>> v4 --> v3
>> Nothing changed, becasue of issue with mimecast,
>> when the From: tag is different from the sender,
>> the some mail client will take the patch as an
>> attachment, RESEND v3 does not work, So resend
>> the patch as v4
>>
>> v3 --> v2:
>> Call vhost_dev_reset() at the end of vhost_net_stop().
>>
>> Since the vDPA device need re-add the status bit
>> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
>> simply, add them inside vhost_vdpa_reset_device, and
>> the only way calling vhost_vdpa_reset_device is in
>> vhost_net_stop(), so it keeps the same behavior as before.
>>
>> v2 --> v1:
>> Implement a new function vhost_dev_reset,
>> reset the backend kernel device at last.
>> ---
>> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
>> hw/virtio/vhost-vdpa.c | 15 +++++++++------
>> hw/virtio/vhost.c | 15 ++++++++++++++-
>> include/hw/virtio/vhost.h | 1 +
>> 4 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 30379d2..422c9bf 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> VirtIONet *n = VIRTIO_NET(dev);
>> int nvhosts = data_queue_pairs + cvq;
>> - struct vhost_net *net;
>> + struct vhost_net *net = NULL;
>> int r, e, i, index_end = data_queue_pairs * 2;
>> NetClientState *peer;
>>
>> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> err_start:
>> while (--i >= 0) {
>> peer = qemu_get_peer(ncs , i);
>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>> +
>> + net = get_vhost_net(peer);
>> +
>> + vhost_net_stop_one(net, dev);
>> }
>> +
>> + /* We only reset backend vdpa device */
>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>> + vhost_dev_reset(&net->dev);
>> + }
>> +
>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>> if (e < 0) {
>> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
>> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>> VirtIONet *n = VIRTIO_NET(dev);
>> NetClientState *peer;
>> + struct vhost_net *net = NULL;
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> int nvhosts = data_queue_pairs + cvq;
>> int i, r;
>> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> } else {
>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>> }
>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>> +
>> + net = get_vhost_net(peer);
>> +
>> + vhost_net_stop_one(net, dev);
>> + }
>> +
>> + /* We only reset backend vdpa device */
>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>> + vhost_dev_reset(&net->dev);
>> }
> So we've already reset the device in vhost_vdpa_dev_start(), any
> reason we need to do it again here?
>
>> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index c5ed7a3..3ef0199 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>>
>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>> trace_vhost_vdpa_reset_device(dev, status);
>> +
>> + /* Add back this status, so that the device could work next time*/
>> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> + VIRTIO_CONFIG_S_DRIVER);
> This seems to contradict the semantic of reset.
>
>> +
>> return ret;
>> }
>>
>> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>> return idx;
>> }
>>
>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
>> {
>> int i;
>> trace_vhost_vdpa_set_vring_ready(dev);
>> for (i = 0; i < dev->nvqs; ++i) {
>> struct vhost_vring_state state = {
>> .index = dev->vq_index + i,
>> - .num = 1,
>> + .num = ready,
>> };
>> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>> }
>> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> if (unlikely(!ok)) {
>> return -1;
>> }
>> - vhost_vdpa_set_vring_ready(dev);
>> + vhost_vdpa_set_vring_ready(dev, 1);
>> } else {
>> + vhost_vdpa_set_vring_ready(dev, 0);
>> ok = vhost_vdpa_svqs_stop(dev);
>> if (unlikely(!ok)) {
>> return -1;
>> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> memory_listener_register(&v->listener, &address_space_memory);
>> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> } else {
>> - vhost_vdpa_reset_device(dev);
>> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> - VIRTIO_CONFIG_S_DRIVER);
>> memory_listener_unregister(&v->listener);
>>
>> return 0;
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index b643f42..7e0cdb6 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1820,7 +1820,6 @@ fail_features:
>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>> {
>> int i;
>> -
> Unnecessary changes.
>
>> /* should only be called after backend is connected */
>> assert(hdev->vhost_ops);
>>
>> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>
>> return -ENOSYS;
>> }
>> +
>> +int vhost_dev_reset(struct vhost_dev *hdev)
>> +{
> Let's use a separate patch for this.
>
> Thanks
>
>> + int ret = 0;
>> +
>> + /* should only be called after backend is connected */
>> + assert(hdev->vhost_ops);
>> +
>> + if (hdev->vhost_ops->vhost_reset_device) {
>> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index 58a73e7..b8b7c20 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> void vhost_dev_cleanup(struct vhost_dev *hdev);
>> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>> +int vhost_dev_reset(struct vhost_dev *hdev);
>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>
>> --
>> 1.8.3.1
>>
>>
>>
Adding Michael.
On Sat, Apr 2, 2022 at 7:08 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 3/31/2022 7:53 PM, Jason Wang wrote:
> > On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
> >> Currently, when VM poweroff, it will trigger vdpa
> >> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
> >> queue pair and one control queue, triggered 3 times), this
> >> leads to below issue:
> >>
> >> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
> >>
> >> This because in vhost_net_stop(), it will stop all vhost device bind to
> >> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
> >> , then stop the queue: vhost_virtqueue_stop().
> >>
> >> In vhost_dev_stop(), it resets the device, which clear some flags
> >> in low level driver, and in next loop(stop other vhost backends),
> >> qemu try to stop the queue corresponding to the vhost backend,
> >> the driver finds that the VQ is invalied, this is the root cause.
> >>
> >> To solve the issue, vdpa should set vring unready, and
> >> remove reset ops in device stop: vhost_dev_start(hdev, false).
> >>
> >> and implement a new function vhost_dev_reset, only reset backend
> >> device after all vhost(per-queue) stoped.
> > Typo.
> >
> >> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
> >> Acked-by: Jason Wang <jasowang@redhat.com>
> > Rethink this patch, consider there're devices that don't support
> > set_vq_ready(). I wonder if we need
> >
> > 1) uAPI to tell the user space whether or not it supports set_vq_ready()
> I guess what's more relevant here is to define the uAPI semantics for
> unready i.e. set_vq_ready(0) for resuming/stopping virtqueue processing,
> as starting vq is comparatively less ambiguous.
Yes.
> Considering the
> likelihood that this interface may be used for live migration, it would
> be nice to come up with variants such as 1) discard inflight request
> v.s. 2) waiting for inflight processing to be done,
Or inflight descriptor reporting (which seems to be tricky). But we
can start from net that a discarding may just work.
>and 3) timeout in
> waiting.
Actually, that's the plan and Eugenio is proposing something like this
via virtio spec:
https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
>
> > 2) userspace will call SET_VRING_ENABLE() when the device supports
> > otherwise it will use RESET.
> Are you looking to making virtqueue resume-able through the new
> SET_VRING_ENABLE() uAPI?
>
> I think RESET is inevitable in some case, i.e. when guest initiates
> device reset by writing 0 to the status register.
Yes, that's all my plan.
> For suspend/resume and
> live migration use cases, indeed RESET can be substituted with
> SET_VRING_ENABLE. Again, it'd need quite some code refactoring to
> accommodate this change. Although I'm all for it, it'd be the best to
> lay out the plan for multiple phases rather than overload this single
> patch too much. You can count my time on this endeavor if you don't mind. :)
You're welcome, I agree we should choose a way to go first:
1) manage to use SET_VRING_ENABLE (more like a workaround anyway)
2) go with virtio-spec (may take a while)
3) don't wait for the spec, have a vDPA specific uAPI first. Note that
I've chatted with most of the vendors and they seem to be fine with
the _S_STOP. If we go this way, we can still provide the forward
compatibility of _S_STOP
4) or do them all (in parallel)
Any thoughts?
Thanks
>
> >
> > And for safety, I suggest tagging this as 7.1.
> +1
>
> Regards,
> -Siwei
>
> >
> >> ---
> >> v4 --> v3
> >> Nothing changed, becasue of issue with mimecast,
> >> when the From: tag is different from the sender,
> >> the some mail client will take the patch as an
> >> attachment, RESEND v3 does not work, So resend
> >> the patch as v4
> >>
> >> v3 --> v2:
> >> Call vhost_dev_reset() at the end of vhost_net_stop().
> >>
> >> Since the vDPA device need re-add the status bit
> >> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
> >> simply, add them inside vhost_vdpa_reset_device, and
> >> the only way calling vhost_vdpa_reset_device is in
> >> vhost_net_stop(), so it keeps the same behavior as before.
> >>
> >> v2 --> v1:
> >> Implement a new function vhost_dev_reset,
> >> reset the backend kernel device at last.
> >> ---
> >> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
> >> hw/virtio/vhost-vdpa.c | 15 +++++++++------
> >> hw/virtio/vhost.c | 15 ++++++++++++++-
> >> include/hw/virtio/vhost.h | 1 +
> >> 4 files changed, 45 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index 30379d2..422c9bf 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >> int total_notifiers = data_queue_pairs * 2 + cvq;
> >> VirtIONet *n = VIRTIO_NET(dev);
> >> int nvhosts = data_queue_pairs + cvq;
> >> - struct vhost_net *net;
> >> + struct vhost_net *net = NULL;
> >> int r, e, i, index_end = data_queue_pairs * 2;
> >> NetClientState *peer;
> >>
> >> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >> err_start:
> >> while (--i >= 0) {
> >> peer = qemu_get_peer(ncs , i);
> >> - vhost_net_stop_one(get_vhost_net(peer), dev);
> >> +
> >> + net = get_vhost_net(peer);
> >> +
> >> + vhost_net_stop_one(net, dev);
> >> }
> >> +
> >> + /* We only reset backend vdpa device */
> >> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
> >> + vhost_dev_reset(&net->dev);
> >> + }
> >> +
> >> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> >> if (e < 0) {
> >> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
> >> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> >> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >> VirtIONet *n = VIRTIO_NET(dev);
> >> NetClientState *peer;
> >> + struct vhost_net *net = NULL;
> >> int total_notifiers = data_queue_pairs * 2 + cvq;
> >> int nvhosts = data_queue_pairs + cvq;
> >> int i, r;
> >> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> >> } else {
> >> peer = qemu_get_peer(ncs, n->max_queue_pairs);
> >> }
> >> - vhost_net_stop_one(get_vhost_net(peer), dev);
> >> +
> >> + net = get_vhost_net(peer);
> >> +
> >> + vhost_net_stop_one(net, dev);
> >> + }
> >> +
> >> + /* We only reset backend vdpa device */
> >> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
> >> + vhost_dev_reset(&net->dev);
> >> }
> > So we've already reset the device in vhost_vdpa_dev_start(), any
> > reason we need to do it again here?
> >
> >> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index c5ed7a3..3ef0199 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
> >>
> >> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
> >> trace_vhost_vdpa_reset_device(dev, status);
> >> +
> >> + /* Add back this status, so that the device could work next time*/
> >> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> + VIRTIO_CONFIG_S_DRIVER);
> > This seems to contradict the semantic of reset.
> >
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> >> return idx;
> >> }
> >>
> >> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> >> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
> >> {
> >> int i;
> >> trace_vhost_vdpa_set_vring_ready(dev);
> >> for (i = 0; i < dev->nvqs; ++i) {
> >> struct vhost_vring_state state = {
> >> .index = dev->vq_index + i,
> >> - .num = 1,
> >> + .num = ready,
> >> };
> >> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> >> }
> >> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >> if (unlikely(!ok)) {
> >> return -1;
> >> }
> >> - vhost_vdpa_set_vring_ready(dev);
> >> + vhost_vdpa_set_vring_ready(dev, 1);
> >> } else {
> >> + vhost_vdpa_set_vring_ready(dev, 0);
> >> ok = vhost_vdpa_svqs_stop(dev);
> >> if (unlikely(!ok)) {
> >> return -1;
> >> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >> memory_listener_register(&v->listener, &address_space_memory);
> >> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> } else {
> >> - vhost_vdpa_reset_device(dev);
> >> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >> - VIRTIO_CONFIG_S_DRIVER);
> >> memory_listener_unregister(&v->listener);
> >>
> >> return 0;
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index b643f42..7e0cdb6 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -1820,7 +1820,6 @@ fail_features:
> >> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> >> {
> >> int i;
> >> -
> > Unnecessary changes.
> >
> >> /* should only be called after backend is connected */
> >> assert(hdev->vhost_ops);
> >>
> >> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> >>
> >> return -ENOSYS;
> >> }
> >> +
> >> +int vhost_dev_reset(struct vhost_dev *hdev)
> >> +{
> > Let's use a separate patch for this.
> >
> > Thanks
> >
> >> + int ret = 0;
> >> +
> >> + /* should only be called after backend is connected */
> >> + assert(hdev->vhost_ops);
> >> +
> >> + if (hdev->vhost_ops->vhost_reset_device) {
> >> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index 58a73e7..b8b7c20 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> void vhost_dev_cleanup(struct vhost_dev *hdev);
> >> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> >> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> >> +int vhost_dev_reset(struct vhost_dev *hdev);
> >> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> >> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
> >>
> >> --
> >> 1.8.3.1
> >>
> >>
> >>
>
On 4/1/2022 7:20 PM, Jason Wang wrote:
> Adding Michael.
>
> On Sat, Apr 2, 2022 at 7:08 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 3/31/2022 7:53 PM, Jason Wang wrote:
>>> On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
>>>> Currently, when VM poweroff, it will trigger vdpa
>>>> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
>>>> queue pair and one control queue, triggered 3 times), this
>>>> leads to below issue:
>>>>
>>>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>>>
>>>> This because in vhost_net_stop(), it will stop all vhost device bind to
>>>> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
>>>> , then stop the queue: vhost_virtqueue_stop().
>>>>
>>>> In vhost_dev_stop(), it resets the device, which clear some flags
>>>> in low level driver, and in next loop(stop other vhost backends),
>>>> qemu try to stop the queue corresponding to the vhost backend,
>>>> the driver finds that the VQ is invalied, this is the root cause.
>>>>
>>>> To solve the issue, vdpa should set vring unready, and
>>>> remove reset ops in device stop: vhost_dev_start(hdev, false).
>>>>
>>>> and implement a new function vhost_dev_reset, only reset backend
>>>> device after all vhost(per-queue) stoped.
>>> Typo.
>>>
>>>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>> Rethink this patch, consider there're devices that don't support
>>> set_vq_ready(). I wonder if we need
>>>
>>> 1) uAPI to tell the user space whether or not it supports set_vq_ready()
>> I guess what's more relevant here is to define the uAPI semantics for
>> unready i.e. set_vq_ready(0) for resuming/stopping virtqueue processing,
>> as starting vq is comparatively less ambiguous.
> Yes.
>
>> Considering the
>> likelihood that this interface may be used for live migration, it would
>> be nice to come up with variants such as 1) discard inflight request
>> v.s. 2) waiting for inflight processing to be done,
> Or inflight descriptor reporting (which seems to be tricky). But we
> can start from net that a discarding may just work.
>
>> and 3) timeout in
>> waiting.
> Actually, that's the plan and Eugenio is proposing something like this
> via virtio spec:
>
> https://urldefense.com/v3/__https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html__;!!ACWV5N9M2RV99hQ!bcX6i6_atR-6Gcl-4q5Tekab_xDuXr7lDAMw2E1hilZ_1cZIX1c5mztQtvsnjiiy$
Thanks for the pointer, I seem to recall I saw it some time back though
I wonder if there's follow-up for the v3? My impression was that this is
still a work-in-progress spec proposal, while the semantics of various
F_STOP scenario is unclear yet and not all of the requirements (ex:
STOP_FAILED, rewind & !IN_ORDER) for live migration do seem to get
accommodated?
>
>>> 2) userspace will call SET_VRING_ENABLE() when the device supports
>>> otherwise it will use RESET.
>> Are you looking to making virtqueue resume-able through the new
>> SET_VRING_ENABLE() uAPI?
>>
>> I think RESET is inevitable in some case, i.e. when guest initiates
>> device reset by writing 0 to the status register.
> Yes, that's all my plan.
>
>> For suspend/resume and
>> live migration use cases, indeed RESET can be substituted with
>> SET_VRING_ENABLE. Again, it'd need quite some code refactoring to
>> accommodate this change. Although I'm all for it, it'd be the best to
>> lay out the plan for multiple phases rather than overload this single
>> patch too much. You can count my time on this endeavor if you don't mind. :)
> You're welcome, I agree we should choose a way to go first:
>
> 1) manage to use SET_VRING_ENABLE (more like a workaround anyway)
For networking device and the vq suspend/resume and live migration use
cases to support, I thought it might suffice? We may drop inflight or
unused ones for Ethernet... What other part do you think may limit its
extension to become a general uAPI or add new uAPI to address similar VQ
stop requirement if need be? Or we might well define subsystem specific
uAPI to stop the virtqueue, for vdpa device specifically? I think the
point here is given that we would like to avoid guest side modification
to support live migration, we can define specific uAPI for specific live
migration requirement without having to involve guest driver change.
It'd be easy to get started this way and generalize them all to a full
blown _S_STOP when things are eventually settled.
> 2) go with virtio-spec (may take a while)
I feel it might be still quite early for now to get to a full blown
_S_STOP spec level amendment that works for all types of virtio (vendor)
devices. Generally there can be very specific subsystem-dependent ways
to stop each type of virtio devices that satisfies the live migration of
virtio subsystem devices. For now the discussion mostly concerns with vq
index rewind, inflight handling, notification interrupt and
configuration space such kind of virtio level things, but real device
backend has implication on the other parts such as the order of IO/DMA
quiescing and interrupt masking. If the subsystem virtio guest drivers
today somehow don't support any of those _S_STOP new behaviors, I guess
it's with little point to introduce the same or similar _S_STOP
functionality to the guest driver to effectively support live migration.
Thanks,
-Siwei
> 3) don't wait for the spec, have a vDPA specific uAPI first. Note that
> I've chatted with most of the vendors and they seem to be fine with
> the _S_STOP. If we go this way, we can still provide the forward
> compatibility of _S_STOP
> 4) or do them all (in parallel)
>
> Any thoughts?
>
> Thanks
>
>>> And for safety, I suggest tagging this as 7.1.
>> +1
>>
>> Regards,
>> -Siwei
>>
>>>> ---
>>>> v4 --> v3
>>>> Nothing changed, becasue of issue with mimecast,
>>>> when the From: tag is different from the sender,
>>>> the some mail client will take the patch as an
>>>> attachment, RESEND v3 does not work, So resend
>>>> the patch as v4
>>>>
>>>> v3 --> v2:
>>>> Call vhost_dev_reset() at the end of vhost_net_stop().
>>>>
>>>> Since the vDPA device need re-add the status bit
>>>> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
>>>> simply, add them inside vhost_vdpa_reset_device, and
>>>> the only way calling vhost_vdpa_reset_device is in
>>>> vhost_net_stop(), so it keeps the same behavior as before.
>>>>
>>>> v2 --> v1:
>>>> Implement a new function vhost_dev_reset,
>>>> reset the backend kernel device at last.
>>>> ---
>>>> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
>>>> hw/virtio/vhost-vdpa.c | 15 +++++++++------
>>>> hw/virtio/vhost.c | 15 ++++++++++++++-
>>>> include/hw/virtio/vhost.h | 1 +
>>>> 4 files changed, 45 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index 30379d2..422c9bf 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> - struct vhost_net *net;
>>>> + struct vhost_net *net = NULL;
>>>> int r, e, i, index_end = data_queue_pairs * 2;
>>>> NetClientState *peer;
>>>>
>>>> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> err_start:
>>>> while (--i >= 0) {
>>>> peer = qemu_get_peer(ncs , i);
>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> +
>>>> + net = get_vhost_net(peer);
>>>> +
>>>> + vhost_net_stop_one(net, dev);
>>>> }
>>>> +
>>>> + /* We only reset backend vdpa device */
>>>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>>>> + vhost_dev_reset(&net->dev);
>>>> + }
>>>> +
>>>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>>>> if (e < 0) {
>>>> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
>>>> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> NetClientState *peer;
>>>> + struct vhost_net *net = NULL;
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> int i, r;
>>>> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> } else {
>>>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>> }
>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> +
>>>> + net = get_vhost_net(peer);
>>>> +
>>>> + vhost_net_stop_one(net, dev);
>>>> + }
>>>> +
>>>> + /* We only reset backend vdpa device */
>>>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>>>> + vhost_dev_reset(&net->dev);
>>>> }
>>> So we've already reset the device in vhost_vdpa_dev_start(), any
>>> reason we need to do it again here?
>>>
>>>> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>> index c5ed7a3..3ef0199 100644
>>>> --- a/hw/virtio/vhost-vdpa.c
>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>>>>
>>>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>>>> trace_vhost_vdpa_reset_device(dev, status);
>>>> +
>>>> + /* Add back this status, so that the device could work next time*/
>>>> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>> + VIRTIO_CONFIG_S_DRIVER);
>>> This seems to contradict the semantic of reset.
>>>
>>>> +
>>>> return ret;
>>>> }
>>>>
>>>> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>>>> return idx;
>>>> }
>>>>
>>>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>>>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
>>>> {
>>>> int i;
>>>> trace_vhost_vdpa_set_vring_ready(dev);
>>>> for (i = 0; i < dev->nvqs; ++i) {
>>>> struct vhost_vring_state state = {
>>>> .index = dev->vq_index + i,
>>>> - .num = 1,
>>>> + .num = ready,
>>>> };
>>>> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>>>> }
>>>> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>>> if (unlikely(!ok)) {
>>>> return -1;
>>>> }
>>>> - vhost_vdpa_set_vring_ready(dev);
>>>> + vhost_vdpa_set_vring_ready(dev, 1);
>>>> } else {
>>>> + vhost_vdpa_set_vring_ready(dev, 0);
>>>> ok = vhost_vdpa_svqs_stop(dev);
>>>> if (unlikely(!ok)) {
>>>> return -1;
>>>> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>>> memory_listener_register(&v->listener, &address_space_memory);
>>>> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>>> } else {
>>>> - vhost_vdpa_reset_device(dev);
>>>> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>> - VIRTIO_CONFIG_S_DRIVER);
>>>> memory_listener_unregister(&v->listener);
>>>>
>>>> return 0;
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index b643f42..7e0cdb6 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1820,7 +1820,6 @@ fail_features:
>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>>>> {
>>>> int i;
>>>> -
>>> Unnecessary changes.
>>>
>>>> /* should only be called after backend is connected */
>>>> assert(hdev->vhost_ops);
>>>>
>>>> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>>>
>>>> return -ENOSYS;
>>>> }
>>>> +
>>>> +int vhost_dev_reset(struct vhost_dev *hdev)
>>>> +{
>>> Let's use a separate patch for this.
>>>
>>> Thanks
>>>
>>>> + int ret = 0;
>>>> +
>>>> + /* should only be called after backend is connected */
>>>> + assert(hdev->vhost_ops);
>>>> +
>>>> + if (hdev->vhost_ops->vhost_reset_device) {
>>>> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>>> index 58a73e7..b8b7c20 100644
>>>> --- a/include/hw/virtio/vhost.h
>>>> +++ b/include/hw/virtio/vhost.h
>>>> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>>>> void vhost_dev_cleanup(struct vhost_dev *hdev);
>>>> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> +int vhost_dev_reset(struct vhost_dev *hdev);
>>>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>>>
在 2022/4/6 上午8:56, Si-Wei Liu 写道:
>
>
> On 4/1/2022 7:20 PM, Jason Wang wrote:
>> Adding Michael.
>>
>> On Sat, Apr 2, 2022 at 7:08 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>
>>>
>>> On 3/31/2022 7:53 PM, Jason Wang wrote:
>>>> On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com>
>>>> wrote:
>>>>> Currently, when VM poweroff, it will trigger vdpa
>>>>> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
>>>>> queue pair and one control queue, triggered 3 times), this
>>>>> leads to below issue:
>>>>>
>>>>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>>>>
>>>>> This because in vhost_net_stop(), it will stop all vhost device
>>>>> bind to
>>>>> this virtio device, and in vhost_dev_stop(), qemu tries to stop
>>>>> the device
>>>>> , then stop the queue: vhost_virtqueue_stop().
>>>>>
>>>>> In vhost_dev_stop(), it resets the device, which clear some flags
>>>>> in low level driver, and in next loop(stop other vhost backends),
>>>>> qemu try to stop the queue corresponding to the vhost backend,
>>>>> the driver finds that the VQ is invalied, this is the root cause.
>>>>>
>>>>> To solve the issue, vdpa should set vring unready, and
>>>>> remove reset ops in device stop: vhost_dev_start(hdev, false).
>>>>>
>>>>> and implement a new function vhost_dev_reset, only reset backend
>>>>> device after all vhost(per-queue) stoped.
>>>> Typo.
>>>>
>>>>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>> Rethink this patch, consider there're devices that don't support
>>>> set_vq_ready(). I wonder if we need
>>>>
>>>> 1) uAPI to tell the user space whether or not it supports
>>>> set_vq_ready()
>>> I guess what's more relevant here is to define the uAPI semantics for
>>> unready i.e. set_vq_ready(0) for resuming/stopping virtqueue
>>> processing,
>>> as starting vq is comparatively less ambiguous.
>> Yes.
>>
>>> Considering the
>>> likelihood that this interface may be used for live migration, it would
>>> be nice to come up with variants such as 1) discard inflight request
>>> v.s. 2) waiting for inflight processing to be done,
>> Or inflight descriptor reporting (which seems to be tricky). But we
>> can start from net that a discarding may just work.
>>
>>> and 3) timeout in
>>> waiting.
>> Actually, that's the plan and Eugenio is proposing something like this
>> via virtio spec:
>>
>> https://urldefense.com/v3/__https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html__;!!ACWV5N9M2RV99hQ!bcX6i6_atR-6Gcl-4q5Tekab_xDuXr7lDAMw2E1hilZ_1cZIX1c5mztQtvsnjiiy$
>>
> Thanks for the pointer, I seem to recall I saw it some time back
> though I wonder if there's follow-up for the v3? My impression was
> that this is still a work-in-progress spec proposal, while the
> semantics of various F_STOP scenario is unclear yet and not all of the
> requirements (ex: STOP_FAILED, rewind & !IN_ORDER) for live migration
> do seem to get accommodated?
My understanding is that, the reason for STOP_FAILED and IN_ORDER is
because we don't have a way to report inflight descriptors. We will try
to overcome those by allow the device to report inflight descriptors in
the next version.
>
>>
>>>> 2) userspace will call SET_VRING_ENABLE() when the device supports
>>>> otherwise it will use RESET.
>>> Are you looking to making virtqueue resume-able through the new
>>> SET_VRING_ENABLE() uAPI?
>>>
>>> I think RESET is inevitable in some case, i.e. when guest initiates
>>> device reset by writing 0 to the status register.
>> Yes, that's all my plan.
>>
>>> For suspend/resume and
>>> live migration use cases, indeed RESET can be substituted with
>>> SET_VRING_ENABLE. Again, it'd need quite some code refactoring to
>>> accommodate this change. Although I'm all for it, it'd be the best to
>>> lay out the plan for multiple phases rather than overload this single
>>> patch too much. You can count my time on this endeavor if you don't
>>> mind. :)
>> You're welcome, I agree we should choose a way to go first:
>>
>> 1) manage to use SET_VRING_ENABLE (more like a workaround anyway)
> For networking device and the vq suspend/resume and live migration use
> cases to support, I thought it might suffice?
Without config space change it would be sufficient. And anyhow the vDPA
parent can prevent the config change if all the virtqueue is disabled.
> We may drop inflight or unused ones for Ethernet...
Yes.
> What other part do you think may limit its extension to become a
> general uAPI or add new uAPI to address similar VQ stop requirement if
> need be?
For networking, we don't need other.
> Or we might well define subsystem specific uAPI to stop the virtqueue,
> for vdpa device specifically?
Anyhow we need a uAPI consider we have some parent that doesn't support
virtqueue stop. So this could be another way to go.
But if we decide to bother with new uAPI, I would rather go with a new
uAPI for stop the device. It can help for the config space change as well.
> I think the point here is given that we would like to avoid guest side
> modification to support live migration, we can define specific uAPI
> for specific live migration requirement without having to involve
> guest driver change. It'd be easy to get started this way and
> generalize them all to a full blown _S_STOP when things are eventually
> settled.
Yes, note that the status seen by guest is mediated by the hypervisor.
So the hypervisor can choose not to hide the _S_STOP from the guest to
keep the migration work without modifications in the guest driver.
>
>> 2) go with virtio-spec (may take a while)
> I feel it might be still quite early for now to get to a full blown
> _S_STOP spec level amendment that works for all types of virtio
> (vendor) devices. Generally there can be very specific
> subsystem-dependent ways to stop each type of virtio devices that
> satisfies the live migration of virtio subsystem devices.
Yes.
> For now the discussion mostly concerns with vq index rewind, inflight
> handling, notification interrupt and configuration space such kind of
> virtio level things, but real device backend has implication on the
> other parts such as the order of IO/DMA quiescing and interrupt masking.
It's the charge of the vDPA parent to perform any necessary quiescing to
satisfy the semantic of _S_STOP, it's an implementation detail which is
out of the scope of the spec.
> If the subsystem virtio guest drivers today somehow don't support any
> of those _S_STOP new behaviors, I guess it's with little point to
> introduce the same or similar _S_STOP functionality to the guest
> driver to effectively support live migration.
See above, the live migration is transparent to the guest. For the
driver that doesn't support _S_STOP, we can still live migrate it. The
only interesting part is nesting: if we want to live migrate a nested
guest we need to the guest driver must support _S_STOP.
Thanks
>
>
> Thanks,
> -Siwei
>> 3) don't wait for the spec, have a vDPA specific uAPI first. Note that
>> I've chatted with most of the vendors and they seem to be fine with
>> the _S_STOP. If we go this way, we can still provide the forward
>> compatibility of _S_STOP
>> 4) or do them all (in parallel)
>>
>> Any thoughts?
>>
>> Thanks
>>
>>>> And for safety, I suggest tagging this as 7.1.
>>> +1
>>>
>>> Regards,
>>> -Siwei
>>>
>>>>> ---
>>>>> v4 --> v3
>>>>> Nothing changed, becasue of issue with mimecast,
>>>>> when the From: tag is different from the sender,
>>>>> the some mail client will take the patch as an
>>>>> attachment, RESEND v3 does not work, So resend
>>>>> the patch as v4
>>>>>
>>>>> v3 --> v2:
>>>>> Call vhost_dev_reset() at the end of vhost_net_stop().
>>>>>
>>>>> Since the vDPA device need re-add the status bit
>>>>> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
>>>>> simply, add them inside vhost_vdpa_reset_device, and
>>>>> the only way calling vhost_vdpa_reset_device is in
>>>>> vhost_net_stop(), so it keeps the same behavior as before.
>>>>>
>>>>> v2 --> v1:
>>>>> Implement a new function vhost_dev_reset,
>>>>> reset the backend kernel device at last.
>>>>> ---
>>>>> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
>>>>> hw/virtio/vhost-vdpa.c | 15 +++++++++------
>>>>> hw/virtio/vhost.c | 15 ++++++++++++++-
>>>>> include/hw/virtio/vhost.h | 1 +
>>>>> 4 files changed, 45 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>> index 30379d2..422c9bf 100644
>>>>> --- a/hw/net/vhost_net.c
>>>>> +++ b/hw/net/vhost_net.c
>>>>> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev,
>>>>> NetClientState *ncs,
>>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>>> int nvhosts = data_queue_pairs + cvq;
>>>>> - struct vhost_net *net;
>>>>> + struct vhost_net *net = NULL;
>>>>> int r, e, i, index_end = data_queue_pairs * 2;
>>>>> NetClientState *peer;
>>>>>
>>>>> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev,
>>>>> NetClientState *ncs,
>>>>> err_start:
>>>>> while (--i >= 0) {
>>>>> peer = qemu_get_peer(ncs , i);
>>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>>> +
>>>>> + net = get_vhost_net(peer);
>>>>> +
>>>>> + vhost_net_stop_one(net, dev);
>>>>> }
>>>>> +
>>>>> + /* We only reset backend vdpa device */
>>>>> + if (net && net->dev.vhost_ops->backend_type ==
>>>>> VHOST_BACKEND_TYPE_VDPA) {
>>>>> + vhost_dev_reset(&net->dev);
>>>>> + }
>>>>> +
>>>>> e = k->set_guest_notifiers(qbus->parent, total_notifiers,
>>>>> false);
>>>>> if (e < 0) {
>>>>> fprintf(stderr, "vhost guest notifier cleanup failed:
>>>>> %d\n", e);
>>>>> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev,
>>>>> NetClientState *ncs,
>>>>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>>> NetClientState *peer;
>>>>> + struct vhost_net *net = NULL;
>>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>>> int nvhosts = data_queue_pairs + cvq;
>>>>> int i, r;
>>>>> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev,
>>>>> NetClientState *ncs,
>>>>> } else {
>>>>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>>> }
>>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>>> +
>>>>> + net = get_vhost_net(peer);
>>>>> +
>>>>> + vhost_net_stop_one(net, dev);
>>>>> + }
>>>>> +
>>>>> + /* We only reset backend vdpa device */
>>>>> + if (net && net->dev.vhost_ops->backend_type ==
>>>>> VHOST_BACKEND_TYPE_VDPA) {
>>>>> + vhost_dev_reset(&net->dev);
>>>>> }
>>>> So we've already reset the device in vhost_vdpa_dev_start(), any
>>>> reason we need to do it again here?
>>>>
>>>>> r = k->set_guest_notifiers(qbus->parent, total_notifiers,
>>>>> false);
>>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>>> index c5ed7a3..3ef0199 100644
>>>>> --- a/hw/virtio/vhost-vdpa.c
>>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>>> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct
>>>>> vhost_dev *dev)
>>>>>
>>>>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>>>>> trace_vhost_vdpa_reset_device(dev, status);
>>>>> +
>>>>> + /* Add back this status, so that the device could work next
>>>>> time*/
>>>>> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>>> + VIRTIO_CONFIG_S_DRIVER);
>>>> This seems to contradict the semantic of reset.
>>>>
>>>>> +
>>>>> return ret;
>>>>> }
>>>>>
>>>>> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct
>>>>> vhost_dev *dev, int idx)
>>>>> return idx;
>>>>> }
>>>>>
>>>>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>>>>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev,
>>>>> unsigned int ready)
>>>>> {
>>>>> int i;
>>>>> trace_vhost_vdpa_set_vring_ready(dev);
>>>>> for (i = 0; i < dev->nvqs; ++i) {
>>>>> struct vhost_vring_state state = {
>>>>> .index = dev->vq_index + i,
>>>>> - .num = 1,
>>>>> + .num = ready,
>>>>> };
>>>>> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>>>>> }
>>>>> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct
>>>>> vhost_dev *dev, bool started)
>>>>> if (unlikely(!ok)) {
>>>>> return -1;
>>>>> }
>>>>> - vhost_vdpa_set_vring_ready(dev);
>>>>> + vhost_vdpa_set_vring_ready(dev, 1);
>>>>> } else {
>>>>> + vhost_vdpa_set_vring_ready(dev, 0);
>>>>> ok = vhost_vdpa_svqs_stop(dev);
>>>>> if (unlikely(!ok)) {
>>>>> return -1;
>>>>> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct
>>>>> vhost_dev *dev, bool started)
>>>>> memory_listener_register(&v->listener,
>>>>> &address_space_memory);
>>>>> return vhost_vdpa_add_status(dev,
>>>>> VIRTIO_CONFIG_S_DRIVER_OK);
>>>>> } else {
>>>>> - vhost_vdpa_reset_device(dev);
>>>>> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>>> - VIRTIO_CONFIG_S_DRIVER);
>>>>> memory_listener_unregister(&v->listener);
>>>>>
>>>>> return 0;
>>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>>> index b643f42..7e0cdb6 100644
>>>>> --- a/hw/virtio/vhost.c
>>>>> +++ b/hw/virtio/vhost.c
>>>>> @@ -1820,7 +1820,6 @@ fail_features:
>>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>>>>> {
>>>>> int i;
>>>>> -
>>>> Unnecessary changes.
>>>>
>>>>> /* should only be called after backend is connected */
>>>>> assert(hdev->vhost_ops);
>>>>>
>>>>> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev
>>>>> *hdev,
>>>>>
>>>>> return -ENOSYS;
>>>>> }
>>>>> +
>>>>> +int vhost_dev_reset(struct vhost_dev *hdev)
>>>>> +{
>>>> Let's use a separate patch for this.
>>>>
>>>> Thanks
>>>>
>>>>> + int ret = 0;
>>>>> +
>>>>> + /* should only be called after backend is connected */
>>>>> + assert(hdev->vhost_ops);
>>>>> +
>>>>> + if (hdev->vhost_ops->vhost_reset_device) {
>>>>> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>>>> index 58a73e7..b8b7c20 100644
>>>>> --- a/include/hw/virtio/vhost.h
>>>>> +++ b/include/hw/virtio/vhost.h
>>>>> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev,
>>>>> void *opaque,
>>>>> void vhost_dev_cleanup(struct vhost_dev *hdev);
>>>>> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>>> +int vhost_dev_reset(struct vhost_dev *hdev);
>>>>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev,
>>>>> VirtIODevice *vdev);
>>>>> void vhost_dev_disable_notifiers(struct vhost_dev *hdev,
>>>>> VirtIODevice *vdev);
>>>>>
>>>>> --
>>>>> 1.8.3.1
>>>>>
>>>>>
>>>>>
>
On 2022/4/2 10:20, Jason Wang wrote:
> Adding Michael.
>
> On Sat, Apr 2, 2022 at 7:08 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>>
>> On 3/31/2022 7:53 PM, Jason Wang wrote:
>>> On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
>>>> Currently, when VM poweroff, it will trigger vdpa
>>>> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
>>>> queue pair and one control queue, triggered 3 times), this
>>>> leads to below issue:
>>>>
>>>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>>>
>>>> This because in vhost_net_stop(), it will stop all vhost device bind to
>>>> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
>>>> , then stop the queue: vhost_virtqueue_stop().
>>>>
>>>> In vhost_dev_stop(), it resets the device, which clear some flags
>>>> in low level driver, and in next loop(stop other vhost backends),
>>>> qemu try to stop the queue corresponding to the vhost backend,
>>>> the driver finds that the VQ is invalied, this is the root cause.
>>>>
>>>> To solve the issue, vdpa should set vring unready, and
>>>> remove reset ops in device stop: vhost_dev_start(hdev, false).
>>>>
>>>> and implement a new function vhost_dev_reset, only reset backend
>>>> device after all vhost(per-queue) stoped.
>>> Typo.
>>>
>>>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>> Rethink this patch, consider there're devices that don't support
>>> set_vq_ready(). I wonder if we need
>>>
>>> 1) uAPI to tell the user space whether or not it supports set_vq_ready()
>> I guess what's more relevant here is to define the uAPI semantics for
>> unready i.e. set_vq_ready(0) for resuming/stopping virtqueue processing,
>> as starting vq is comparatively less ambiguous.
>
> Yes.
>
>> Considering the
>> likelihood that this interface may be used for live migration, it would
>> be nice to come up with variants such as 1) discard inflight request
>> v.s. 2) waiting for inflight processing to be done,
>
> Or inflight descriptor reporting (which seems to be tricky). But we
> can start from net that a discarding may just work.
>
>> and 3) timeout in
>> waiting.
>
> Actually, that's the plan and Eugenio is proposing something like this
> via virtio spec:
>
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
>
>>
>>> 2) userspace will call SET_VRING_ENABLE() when the device supports
>>> otherwise it will use RESET.
>> Are you looking to making virtqueue resume-able through the new
>> SET_VRING_ENABLE() uAPI?
>>
>> I think RESET is inevitable in some case, i.e. when guest initiates
>> device reset by writing 0 to the status register.
>
> Yes, that's all my plan.
>
>> For suspend/resume and
>> live migration use cases, indeed RESET can be substituted with
>> SET_VRING_ENABLE. Again, it'd need quite some code refactoring to
>> accommodate this change. Although I'm all for it, it'd be the best to
>> lay out the plan for multiple phases rather than overload this single
>> patch too much. You can count my time on this endeavor if you don't mind. :)
>
> You're welcome, I agree we should choose a way to go first:
>
> 1) manage to use SET_VRING_ENABLE (more like a workaround anyway)
> 2) go with virtio-spec (may take a while)
> 3) don't wait for the spec, have a vDPA specific uAPI first. Note that
> I've chatted with most of the vendors and they seem to be fine with
> the _S_STOP. If we go this way, we can still provide the forward
> compatibility of _S_STOP
> 4) or do them all (in parallel)
>
> Any thoughts?
>
virtio-spec should be long-term, not only because the spec goes very
slowly, but also the hardware upgrade should be a problem.
For short-term, better to take the first one?
Thanks,
Michael
> Thanks
>
>>
>>>
>>> And for safety, I suggest tagging this as 7.1.
>> +1
>>
>> Regards,
>> -Siwei
>>
>>>
>>>> ---
>>>> v4 --> v3
>>>> Nothing changed, becasue of issue with mimecast,
>>>> when the From: tag is different from the sender,
>>>> the some mail client will take the patch as an
>>>> attachment, RESEND v3 does not work, So resend
>>>> the patch as v4
>>>>
>>>> v3 --> v2:
>>>> Call vhost_dev_reset() at the end of vhost_net_stop().
>>>>
>>>> Since the vDPA device need re-add the status bit
>>>> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
>>>> simply, add them inside vhost_vdpa_reset_device, and
>>>> the only way calling vhost_vdpa_reset_device is in
>>>> vhost_net_stop(), so it keeps the same behavior as before.
>>>>
>>>> v2 --> v1:
>>>> Implement a new function vhost_dev_reset,
>>>> reset the backend kernel device at last.
>>>> ---
>>>> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
>>>> hw/virtio/vhost-vdpa.c | 15 +++++++++------
>>>> hw/virtio/vhost.c | 15 ++++++++++++++-
>>>> include/hw/virtio/vhost.h | 1 +
>>>> 4 files changed, 45 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index 30379d2..422c9bf 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> - struct vhost_net *net;
>>>> + struct vhost_net *net = NULL;
>>>> int r, e, i, index_end = data_queue_pairs * 2;
>>>> NetClientState *peer;
>>>>
>>>> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> err_start:
>>>> while (--i >= 0) {
>>>> peer = qemu_get_peer(ncs , i);
>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> +
>>>> + net = get_vhost_net(peer);
>>>> +
>>>> + vhost_net_stop_one(net, dev);
>>>> }
>>>> +
>>>> + /* We only reset backend vdpa device */
>>>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>>>> + vhost_dev_reset(&net->dev);
>>>> + }
>>>> +
>>>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>>>> if (e < 0) {
>>>> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
>>>> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> NetClientState *peer;
>>>> + struct vhost_net *net = NULL;
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> int i, r;
>>>> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> } else {
>>>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>> }
>>>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> +
>>>> + net = get_vhost_net(peer);
>>>> +
>>>> + vhost_net_stop_one(net, dev);
>>>> + }
>>>> +
>>>> + /* We only reset backend vdpa device */
>>>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>>>> + vhost_dev_reset(&net->dev);
>>>> }
>>> So we've already reset the device in vhost_vdpa_dev_start(), any
>>> reason we need to do it again here?
>>>
>>>> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>> index c5ed7a3..3ef0199 100644
>>>> --- a/hw/virtio/vhost-vdpa.c
>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>>>>
>>>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>>>> trace_vhost_vdpa_reset_device(dev, status);
>>>> +
>>>> + /* Add back this status, so that the device could work next time*/
>>>> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>> + VIRTIO_CONFIG_S_DRIVER);
>>> This seems to contradict the semantic of reset.
>>>
>>>> +
>>>> return ret;
>>>> }
>>>>
>>>> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>>>> return idx;
>>>> }
>>>>
>>>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>>>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
>>>> {
>>>> int i;
>>>> trace_vhost_vdpa_set_vring_ready(dev);
>>>> for (i = 0; i < dev->nvqs; ++i) {
>>>> struct vhost_vring_state state = {
>>>> .index = dev->vq_index + i,
>>>> - .num = 1,
>>>> + .num = ready,
>>>> };
>>>> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>>>> }
>>>> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>>> if (unlikely(!ok)) {
>>>> return -1;
>>>> }
>>>> - vhost_vdpa_set_vring_ready(dev);
>>>> + vhost_vdpa_set_vring_ready(dev, 1);
>>>> } else {
>>>> + vhost_vdpa_set_vring_ready(dev, 0);
>>>> ok = vhost_vdpa_svqs_stop(dev);
>>>> if (unlikely(!ok)) {
>>>> return -1;
>>>> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>>> memory_listener_register(&v->listener, &address_space_memory);
>>>> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>>> } else {
>>>> - vhost_vdpa_reset_device(dev);
>>>> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>>>> - VIRTIO_CONFIG_S_DRIVER);
>>>> memory_listener_unregister(&v->listener);
>>>>
>>>> return 0;
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index b643f42..7e0cdb6 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1820,7 +1820,6 @@ fail_features:
>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>>>> {
>>>> int i;
>>>> -
>>> Unnecessary changes.
>>>
>>>> /* should only be called after backend is connected */
>>>> assert(hdev->vhost_ops);
>>>>
>>>> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>>>
>>>> return -ENOSYS;
>>>> }
>>>> +
>>>> +int vhost_dev_reset(struct vhost_dev *hdev)
>>>> +{
>>> Let's use a separate patch for this.
>>>
>>> Thanks
>>>
>>>> + int ret = 0;
>>>> +
>>>> + /* should only be called after backend is connected */
>>>> + assert(hdev->vhost_ops);
>>>> +
>>>> + if (hdev->vhost_ops->vhost_reset_device) {
>>>> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>>> index 58a73e7..b8b7c20 100644
>>>> --- a/include/hw/virtio/vhost.h
>>>> +++ b/include/hw/virtio/vhost.h
>>>> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>>>> void vhost_dev_cleanup(struct vhost_dev *hdev);
>>>> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> +int vhost_dev_reset(struct vhost_dev *hdev);
>>>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>>>>
>>
>
>
On 2022/4/1 10:53, Jason Wang wrote:
> On Fri, Apr 1, 2022 at 9:31 AM Michael Qiu <qiudayu@archeros.com> wrote:
>>
>> Currently, when VM poweroff, it will trigger vdpa
>> device(such as mlx bluefield2 VF) reset many times(with 1 datapath
>> queue pair and one control queue, triggered 3 times), this
>> leads to below issue:
>>
>> vhost VQ 2 ring restore failed: -22: Invalid argument (22)
>>
>> This because in vhost_net_stop(), it will stop all vhost device bind to
>> this virtio device, and in vhost_dev_stop(), qemu tries to stop the device
>> , then stop the queue: vhost_virtqueue_stop().
>>
>> In vhost_dev_stop(), it resets the device, which clear some flags
>> in low level driver, and in next loop(stop other vhost backends),
>> qemu try to stop the queue corresponding to the vhost backend,
>> the driver finds that the VQ is invalied, this is the root cause.
>>
>> To solve the issue, vdpa should set vring unready, and
>> remove reset ops in device stop: vhost_dev_start(hdev, false).
>>
>> and implement a new function vhost_dev_reset, only reset backend
>> device after all vhost(per-queue) stoped.
>
> Typo.
>
>>
>> Signed-off-by: Michael Qiu<qiudayu@archeros.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Rethink this patch, consider there're devices that don't support
> set_vq_ready(). I wonder if we need
>
> 1) uAPI to tell the user space whether or not it supports set_vq_ready()
> 2) userspace will call SET_VRING_ENABLE() when the device supports
> otherwise it will use RESET.
if the device does not support set_vq_ready() in kernel, it will trigger
kernel oops, at least in current kernel, it does not check where
set_vq_ready has been implemented.
And I checked all vdpa driver in kernel, all drivers has implemented
this ops.
So I think it is OK to call set_vq_ready without check.
>
> And for safety, I suggest tagging this as 7.1.
>
>> ---
>> v4 --> v3
>> Nothing changed, becasue of issue with mimecast,
>> when the From: tag is different from the sender,
>> the some mail client will take the patch as an
>> attachment, RESEND v3 does not work, So resend
>> the patch as v4
>>
>> v3 --> v2:
>> Call vhost_dev_reset() at the end of vhost_net_stop().
>>
>> Since the vDPA device need re-add the status bit
>> VIRTIO_CONFIG_S_ACKNOWLEDGE and VIRTIO_CONFIG_S_DRIVER,
>> simply, add them inside vhost_vdpa_reset_device, and
>> the only way calling vhost_vdpa_reset_device is in
>> vhost_net_stop(), so it keeps the same behavior as before.
>>
>> v2 --> v1:
>> Implement a new function vhost_dev_reset,
>> reset the backend kernel device at last.
>> ---
>> hw/net/vhost_net.c | 24 +++++++++++++++++++++---
>> hw/virtio/vhost-vdpa.c | 15 +++++++++------
>> hw/virtio/vhost.c | 15 ++++++++++++++-
>> include/hw/virtio/vhost.h | 1 +
>> 4 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 30379d2..422c9bf 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -325,7 +325,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> VirtIONet *n = VIRTIO_NET(dev);
>> int nvhosts = data_queue_pairs + cvq;
>> - struct vhost_net *net;
>> + struct vhost_net *net = NULL;
>> int r, e, i, index_end = data_queue_pairs * 2;
>> NetClientState *peer;
>>
>> @@ -391,8 +391,17 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> err_start:
>> while (--i >= 0) {
>> peer = qemu_get_peer(ncs , i);
>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>> +
>> + net = get_vhost_net(peer);
>> +
>> + vhost_net_stop_one(net, dev);
>> }
>> +
>> + /* We only reset backend vdpa device */
>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>> + vhost_dev_reset(&net->dev);
>> + }
>> +
>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>> if (e < 0) {
>> fprintf(stderr, "vhost guest notifier cleanup failed: %d\n", e);
>> @@ -410,6 +419,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>> VirtIONet *n = VIRTIO_NET(dev);
>> NetClientState *peer;
>> + struct vhost_net *net = NULL;
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> int nvhosts = data_queue_pairs + cvq;
>> int i, r;
>> @@ -420,7 +430,15 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> } else {
>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>> }
>> - vhost_net_stop_one(get_vhost_net(peer), dev);
>> +
>> + net = get_vhost_net(peer);
>> +
>> + vhost_net_stop_one(net, dev);
>> + }
>> +
>> + /* We only reset backend vdpa device */
>> + if (net && net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA) {
>> + vhost_dev_reset(&net->dev);
>> }
>
> So we've already reset the device in vhost_vdpa_dev_start(), any
> reason we need to do it again here?
reset device in vhost_vdpa_dev_start if there is some error with start.
>
>>
>> r = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index c5ed7a3..3ef0199 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -708,6 +708,11 @@ static int vhost_vdpa_reset_device(struct vhost_dev *dev)
>>
>> ret = vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &status);
>> trace_vhost_vdpa_reset_device(dev, status);
>> +
>> + /* Add back this status, so that the device could work next time*/
>> + vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> + VIRTIO_CONFIG_S_DRIVER);
>
> This seems to contradict the semantic of reset
Yes, but it's hard to put it in other place, seems only vhost-vdpa need
it, and for VM shutdown, qemu_del_nic() will do cleanup this like close
vhost fds, which will call reset in kernel space without set those features.
So at last I put it here with no other inpact.
Thanks,
Michael
>
>> +
>> return ret;
>> }
>>
>> @@ -719,14 +724,14 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>> return idx;
>> }
>>
>> -static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>> +static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, unsigned int ready)
>> {
>> int i;
>> trace_vhost_vdpa_set_vring_ready(dev);
>> for (i = 0; i < dev->nvqs; ++i) {
>> struct vhost_vring_state state = {
>> .index = dev->vq_index + i,
>> - .num = 1,
>> + .num = ready,
>> };
>> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>> }
>> @@ -1088,8 +1093,9 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> if (unlikely(!ok)) {
>> return -1;
>> }
>> - vhost_vdpa_set_vring_ready(dev);
>> + vhost_vdpa_set_vring_ready(dev, 1);
>> } else {
>> + vhost_vdpa_set_vring_ready(dev, 0);
>> ok = vhost_vdpa_svqs_stop(dev);
>> if (unlikely(!ok)) {
>> return -1;
>> @@ -1105,9 +1111,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> memory_listener_register(&v->listener, &address_space_memory);
>> return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> } else {
>> - vhost_vdpa_reset_device(dev);
>> - vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>> - VIRTIO_CONFIG_S_DRIVER);
>> memory_listener_unregister(&v->listener);
>>
>> return 0;
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index b643f42..7e0cdb6 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1820,7 +1820,6 @@ fail_features:
>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>> {
>> int i;
>> -
>
> Unnecessary changes.
>
>> /* should only be called after backend is connected */
>> assert(hdev->vhost_ops);
>>
>> @@ -1854,3 +1853,17 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>
>> return -ENOSYS;
>> }
>> +
>> +int vhost_dev_reset(struct vhost_dev *hdev)
>> +{
>
> Let's use a separate patch for this.
>
> Thanks
>
>> + int ret = 0;
>> +
>> + /* should only be called after backend is connected */
>> + assert(hdev->vhost_ops);
>> +
>> + if (hdev->vhost_ops->vhost_reset_device) {
>> + ret = hdev->vhost_ops->vhost_reset_device(hdev);
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index 58a73e7..b8b7c20 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -114,6 +114,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> void vhost_dev_cleanup(struct vhost_dev *hdev);
>> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
>> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
>> +int vhost_dev_reset(struct vhost_dev *hdev);
>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>> void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev);
>>
>> --
>> 1.8.3.1
>>
>>
>>
>
>
>
© 2016 - 2026 Red Hat, Inc.