The backend maybe crash when vhost_dev_stop and GET_VRING_BASE
would fail, we can return failure to indicate the connection
with the backend is broken.
Signed-off-by: Haoqian He <haoqian.he@smartx.com>
---
hw/virtio/vhost.c | 27 +++++++++++++++------------
include/hw/virtio/vhost.h | 8 +++++---
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6aa72fd434..c82bbbe4cc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1368,23 +1368,23 @@ fail_alloc_desc:
return r;
}
-void vhost_virtqueue_stop(struct vhost_dev *dev,
- struct VirtIODevice *vdev,
- struct vhost_virtqueue *vq,
- unsigned idx)
+int vhost_virtqueue_stop(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_state state = {
.index = vhost_vq_index,
};
- int r;
+ int r = 0;
if (virtio_queue_get_desc_addr(vdev, idx) == 0) {
/* Don't stop the virtqueue which might have not been started */
- return;
+ return 0;
}
- r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
+ r |= dev->vhost_ops->vhost_get_vring_base(dev, &state);
if (r < 0) {
VHOST_OPS_DEBUG(r, "vhost VQ %u ring restore failed: %d", idx, r);
/* Connection to the backend is broken, so let's sync internal
@@ -1412,6 +1412,7 @@ void vhost_virtqueue_stop(struct vhost_dev *dev,
0, virtio_queue_get_avail_size(vdev, idx));
vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
0, virtio_queue_get_desc_size(vdev, idx));
+ return r;
}
static int vhost_virtqueue_set_busyloop_timeout(struct vhost_dev *dev,
@@ -2136,9 +2137,10 @@ fail_features:
}
/* Host notifiers must be enabled at this point. */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
+int vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
{
int i;
+ int rc = 0;
/* should only be called after backend is connected */
assert(hdev->vhost_ops);
@@ -2157,10 +2159,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
vhost_dev_set_vring_enable(hdev, false);
}
for (i = 0; i < hdev->nvqs; ++i) {
- vhost_virtqueue_stop(hdev,
- vdev,
- hdev->vqs + i,
- hdev->vq_index + i);
+ rc |= vhost_virtqueue_stop(hdev,
+ vdev,
+ hdev->vqs + i,
+ hdev->vq_index + i);
}
if (hdev->vhost_ops->vhost_reset_status) {
hdev->vhost_ops->vhost_reset_status(hdev);
@@ -2177,6 +2179,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
hdev->started = false;
vdev->vhost_started = false;
hdev->vdev = NULL;
+ return rc;
}
int vhost_net_set_backend(struct vhost_dev *hdev,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a9469d50bc..fd96ec9c39 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -232,8 +232,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
* Stop the vhost device. After the device is stopped the notifiers
* can be disabled (@vhost_dev_disable_notifiers) and the device can
* be torn down (@vhost_dev_cleanup).
+ *
+ * Return: 0 on success, != 0 on error when stopping dev.
*/
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
+int vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
/**
* DOC: vhost device configuration handling
@@ -333,8 +335,8 @@ int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
struct vhost_virtqueue *vq, unsigned idx);
-void vhost_virtqueue_stop(struct vhost_dev *dev, struct VirtIODevice *vdev,
- struct vhost_virtqueue *vq, unsigned idx);
+int vhost_virtqueue_stop(struct vhost_dev *dev, struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq, unsigned idx);
void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
void vhost_dev_free_inflight(struct vhost_inflight *inflight);
--
2.48.1Live migration should be terminated if backend crash.
Since the vhost device will be stopped when VM is stopped before
the end of the live migration, current implementation if vhost
backend died, vhost device's set_status() will not return failure,
live migration won't perceive the disconnection between qemu and
vhost backend, inflight io would be submitted in migration target
host, leading to IO error.
To fix this issue:
1. Add set_status_ext() which has return value for
VirtioDeviceClass and vhost-user-blk/scsi use the _ext version.
2. In set_status_ext(), return failure if the flag `connected`
is false or vhost_dev_stop return failure, which means qemu lost
connection with backend.
Hence migration_completion() will process failure, terminate
migration and restore VM.
Signed-off-by: Haoqian He <haoqian.he@smartx.com>
---
hw/block/vhost-user-blk.c | 29 +++++++++++++++------------
hw/scsi/vhost-scsi-common.c | 11 +++++-----
hw/scsi/vhost-user-scsi.c | 20 ++++++++++--------
hw/virtio/virtio.c | 20 +++++++++++++-----
include/hw/virtio/vhost-scsi-common.h | 2 +-
include/hw/virtio/virtio.h | 1 +
6 files changed, 51 insertions(+), 32 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index ae42327cf8..4865786c54 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -204,7 +204,7 @@ err_host_notifiers:
return ret;
}
-static void vhost_user_blk_stop(VirtIODevice *vdev)
+static int vhost_user_blk_stop(VirtIODevice *vdev)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -212,26 +212,26 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
int ret;
if (!s->started_vu) {
- return;
+ return 0;
}
s->started_vu = false;
if (!k->set_guest_notifiers) {
- return;
+ return 0;
}
- vhost_dev_stop(&s->dev, vdev, true);
+ ret = vhost_dev_stop(&s->dev, vdev, true);
- ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
- if (ret < 0) {
+ if (k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false) < 0) {
error_report("vhost guest notifier cleanup failed: %d", ret);
- return;
+ return -1;
}
vhost_dev_disable_notifiers(&s->dev, vdev);
+ return ret;
}
-static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
+static int vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
bool should_start = virtio_device_should_start(vdev, status);
@@ -239,11 +239,11 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
int ret;
if (!s->connected) {
- return;
+ return -1;
}
if (vhost_dev_is_started(&s->dev) == should_start) {
- return;
+ return 0;
}
if (should_start) {
@@ -253,9 +253,12 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
qemu_chr_fe_disconnect(&s->chardev);
}
} else {
- vhost_user_blk_stop(vdev);
+ ret = vhost_user_blk_stop(vdev);
+ if (ret < 0) {
+ return ret;
+ }
}
-
+ return 0;
}
static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
@@ -597,7 +600,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
vdc->get_config = vhost_user_blk_update_config;
vdc->set_config = vhost_user_blk_set_config;
vdc->get_features = vhost_user_blk_get_features;
- vdc->set_status = vhost_user_blk_set_status;
+ vdc->set_status_ext = vhost_user_blk_set_status;
vdc->reset = vhost_user_blk_reset;
vdc->get_vhost = vhost_user_blk_get_vhost;
}
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index 4c8637045d..bfeed0cb1b 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -101,24 +101,25 @@ err_host_notifiers:
return ret;
}
-void vhost_scsi_common_stop(VHostSCSICommon *vsc)
+int vhost_scsi_common_stop(VHostSCSICommon *vsc)
{
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret = 0;
- vhost_dev_stop(&vsc->dev, vdev, true);
+ ret = vhost_dev_stop(&vsc->dev, vdev, true);
if (k->set_guest_notifiers) {
- ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
- if (ret < 0) {
+ int r = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
+ if (r < 0) {
error_report("vhost guest notifier cleanup failed: %d", ret);
}
+ assert(r >= 0);
}
- assert(ret >= 0);
vhost_dev_disable_notifiers(&vsc->dev, vdev);
+ return ret;
}
uint64_t vhost_scsi_common_get_features(VirtIODevice *vdev, uint64_t features,
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index adb41b9816..8e7efc38f2 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -52,19 +52,19 @@ static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
return ret;
}
-static void vhost_user_scsi_stop(VHostUserSCSI *s)
+static int vhost_user_scsi_stop(VHostUserSCSI *s)
{
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
if (!s->started_vu) {
- return;
+ return 0;
}
s->started_vu = false;
- vhost_scsi_common_stop(vsc);
+ return vhost_scsi_common_stop(vsc);
}
-static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
+static int vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
DeviceState *dev = DEVICE(vdev);
@@ -75,11 +75,11 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
int ret;
if (!s->connected) {
- return;
+ return -1;
}
if (vhost_dev_is_started(&vsc->dev) == should_start) {
- return;
+ return 0;
}
if (should_start) {
@@ -91,8 +91,12 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
qemu_chr_fe_disconnect(&vs->conf.chardev);
}
} else {
- vhost_user_scsi_stop(s);
+ ret = vhost_user_scsi_stop(s);
+ if (ret) {
+ return ret;
+ }
}
+ return 0;
}
static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
@@ -399,7 +403,7 @@ static void vhost_user_scsi_class_init(ObjectClass *klass, void *data)
vdc->unrealize = vhost_user_scsi_unrealize;
vdc->get_features = vhost_scsi_common_get_features;
vdc->set_config = vhost_scsi_common_set_config;
- vdc->set_status = vhost_user_scsi_set_status;
+ vdc->set_status_ext = vhost_user_scsi_set_status;
fwc->get_dev_path = vhost_scsi_common_get_fw_dev_path;
vdc->reset = vhost_user_scsi_reset;
vdc->get_vhost = vhost_user_scsi_get_vhost;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5e8d4cab53..fff7cdb35d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2221,12 +2221,12 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
trace_virtio_set_status(vdev, val);
+ int ret = 0;
if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) &&
val & VIRTIO_CONFIG_S_FEATURES_OK) {
- int ret = virtio_validate_features(vdev);
-
+ ret = virtio_validate_features(vdev);
if (ret) {
return ret;
}
@@ -2238,12 +2238,18 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
}
- if (k->set_status) {
+ if (k->set_status_ext) {
+ ret = k->set_status_ext(vdev, val);
+ if (ret) {
+ qemu_log("set %s status to %d failed, old status: %d\n",
+ vdev->name, val, vdev->status);
+ }
+ } else if (k->set_status) {
k->set_status(vdev, val);
}
vdev->status = val;
- return 0;
+ return ret;
}
static enum virtio_device_endian virtio_default_endian(void)
@@ -3436,7 +3442,11 @@ static int virtio_vmstate_change(void *opaque, bool running, RunState state)
}
if (!backend_run) {
- virtio_set_status(vdev, vdev->status);
+ // the return value was used for stopping VM during migration
+ int ret = virtio_set_status(vdev, vdev->status);
+ if (ret) {
+ return ret;
+ }
}
return 0;
}
diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
index c5d2c09455..d54d9c916f 100644
--- a/include/hw/virtio/vhost-scsi-common.h
+++ b/include/hw/virtio/vhost-scsi-common.h
@@ -40,7 +40,7 @@ struct VHostSCSICommon {
};
int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
-void vhost_scsi_common_stop(VHostSCSICommon *vsc);
+int vhost_scsi_common_stop(VHostSCSICommon *vsc);
char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
DeviceState *dev);
void vhost_scsi_common_set_config(VirtIODevice *vdev, const uint8_t *config);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6386910280..c99d56f519 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -187,6 +187,7 @@ struct VirtioDeviceClass {
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
void (*set_status)(VirtIODevice *vdev, uint8_t val);
+ int (*set_status_ext)(VirtIODevice *vdev, uint8_t val);
/* Device must validate queue_index. */
void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
/* Device must validate queue_index. */
--
2.48.1I'll let others chime in on higher level device model changes you're
making. For the vhost-user-blk and vhost-user-scsi bits:
On Sun, Mar 9, 2025 at 11:41 AM Haoqian He <haoqian.he@smartx.com> wrote:
>
> Live migration should be terminated if backend crash.
nit: "...if the backend crashes before the migration completes."
>
> Since the vhost device will be stopped when VM is stopped before
> the end of the live migration, current implementation if vhost
> backend died, vhost device's set_status() will not return failure,
> live migration won't perceive the disconnection between qemu and
> vhost backend, inflight io would be submitted in migration target
> host, leading to IO error.
>
> To fix this issue:
> 1. Add set_status_ext() which has return value for
> VirtioDeviceClass and vhost-user-blk/scsi use the _ext version.
>
> 2. In set_status_ext(), return failure if the flag `connected`
> is false or vhost_dev_stop return failure, which means qemu lost
> connection with backend.
I have some concerns with [2]. See my later responses.
>
> Hence migration_completion() will process failure, terminate
> migration and restore VM.
>
> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
> ---
> hw/block/vhost-user-blk.c | 29 +++++++++++++++------------
> hw/scsi/vhost-scsi-common.c | 11 +++++-----
> hw/scsi/vhost-user-scsi.c | 20 ++++++++++--------
> hw/virtio/virtio.c | 20 +++++++++++++-----
> include/hw/virtio/vhost-scsi-common.h | 2 +-
> include/hw/virtio/virtio.h | 1 +
> 6 files changed, 51 insertions(+), 32 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index ae42327cf8..4865786c54 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -204,7 +204,7 @@ err_host_notifiers:
> return ret;
> }
>
> -static void vhost_user_blk_stop(VirtIODevice *vdev)
> +static int vhost_user_blk_stop(VirtIODevice *vdev)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> @@ -212,26 +212,26 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
> int ret;
>
> if (!s->started_vu) {
> - return;
> + return 0;
> }
> s->started_vu = false;
>
> if (!k->set_guest_notifiers) {
> - return;
> + return 0;
> }
>
> - vhost_dev_stop(&s->dev, vdev, true);
> + ret = vhost_dev_stop(&s->dev, vdev, true);
>
> - ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
> - if (ret < 0) {
> + if (k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false) < 0) {
> error_report("vhost guest notifier cleanup failed: %d", ret);
> - return;
> + return -1;
> }
>
> vhost_dev_disable_notifiers(&s->dev, vdev);
> + return ret;
> }
>
> -static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> +static int vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> bool should_start = virtio_device_should_start(vdev, status);
> @@ -239,11 +239,11 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> int ret;
>
Do we want to fail out in all cases where vhost_user_blk_set_status()
and the device is not connected? Could this impact power-on if the
backend is temporarily down?
> if (!s->connected) {
> - return;
> + return -1;
> }
>
> if (vhost_dev_is_started(&s->dev) == should_start) {
> - return;
> + return 0;
> }
>
> if (should_start) {
> @@ -253,9 +253,12 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> qemu_chr_fe_disconnect(&s->chardev);
> }
> } else {
> - vhost_user_blk_stop(vdev);
> + ret = vhost_user_blk_stop(vdev);
> + if (ret < 0) {
> + return ret;
> + }
> }
> -
> + return 0;
> }
>
> static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
> @@ -597,7 +600,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
> vdc->get_config = vhost_user_blk_update_config;
> vdc->set_config = vhost_user_blk_set_config;
> vdc->get_features = vhost_user_blk_get_features;
> - vdc->set_status = vhost_user_blk_set_status;
> + vdc->set_status_ext = vhost_user_blk_set_status;
> vdc->reset = vhost_user_blk_reset;
> vdc->get_vhost = vhost_user_blk_get_vhost;
> }
> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> index 4c8637045d..bfeed0cb1b 100644
> --- a/hw/scsi/vhost-scsi-common.c
> +++ b/hw/scsi/vhost-scsi-common.c
> @@ -101,24 +101,25 @@ err_host_notifiers:
> return ret;
> }
>
> -void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> +int vhost_scsi_common_stop(VHostSCSICommon *vsc)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> int ret = 0;
>
> - vhost_dev_stop(&vsc->dev, vdev, true);
> + ret = vhost_dev_stop(&vsc->dev, vdev, true);
>
> if (k->set_guest_notifiers) {
> - ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> - if (ret < 0) {
> + int r = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> + if (r < 0) {
> error_report("vhost guest notifier cleanup failed: %d", ret);
> }
Now that we return back a value the assert here should probably be dropped?
> + assert(r >= 0);
> }
> - assert(ret >= 0);
>
> vhost_dev_disable_notifiers(&vsc->dev, vdev);
> + return ret;
> }
>
> uint64_t vhost_scsi_common_get_features(VirtIODevice *vdev, uint64_t features,
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index adb41b9816..8e7efc38f2 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -52,19 +52,19 @@ static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
> return ret;
> }
>
> -static void vhost_user_scsi_stop(VHostUserSCSI *s)
> +static int vhost_user_scsi_stop(VHostUserSCSI *s)
> {
> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>
> if (!s->started_vu) {
> - return;
> + return 0;
> }
> s->started_vu = false;
>
> - vhost_scsi_common_stop(vsc);
> + return vhost_scsi_common_stop(vsc);
> }
>
> -static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> +static int vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> {
> VHostUserSCSI *s = (VHostUserSCSI *)vdev;
> DeviceState *dev = DEVICE(vdev);
> @@ -75,11 +75,11 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> int ret;
>
> if (!s->connected) {
> - return;
> + return -1;
> }
>
> if (vhost_dev_is_started(&vsc->dev) == should_start) {
> - return;
> + return 0;
> }
>
> if (should_start) {
> @@ -91,8 +91,12 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
> qemu_chr_fe_disconnect(&vs->conf.chardev);
> }
> } else {
> - vhost_user_scsi_stop(s);
> + ret = vhost_user_scsi_stop(s);
> + if (ret) {
> + return ret;
> + }
> }
> + return 0;
> }
>
> static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> @@ -399,7 +403,7 @@ static void vhost_user_scsi_class_init(ObjectClass *klass, void *data)
> vdc->unrealize = vhost_user_scsi_unrealize;
> vdc->get_features = vhost_scsi_common_get_features;
> vdc->set_config = vhost_scsi_common_set_config;
> - vdc->set_status = vhost_user_scsi_set_status;
> + vdc->set_status_ext = vhost_user_scsi_set_status;
> fwc->get_dev_path = vhost_scsi_common_get_fw_dev_path;
> vdc->reset = vhost_user_scsi_reset;
> vdc->get_vhost = vhost_user_scsi_get_vhost;
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5e8d4cab53..fff7cdb35d 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2221,12 +2221,12 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
> {
> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> trace_virtio_set_status(vdev, val);
> + int ret = 0;
>
> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) &&
> val & VIRTIO_CONFIG_S_FEATURES_OK) {
> - int ret = virtio_validate_features(vdev);
> -
> + ret = virtio_validate_features(vdev);
> if (ret) {
> return ret;
> }
> @@ -2238,12 +2238,18 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
> virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
> }
>
> - if (k->set_status) {
> + if (k->set_status_ext) {
> + ret = k->set_status_ext(vdev, val);
> + if (ret) {
> + qemu_log("set %s status to %d failed, old status: %d\n",
> + vdev->name, val, vdev->status);
> + }
> + } else if (k->set_status) {
> k->set_status(vdev, val);
> }
> vdev->status = val;
>
> - return 0;
> + return ret;
> }
>
> static enum virtio_device_endian virtio_default_endian(void)
> @@ -3436,7 +3442,11 @@ static int virtio_vmstate_change(void *opaque, bool running, RunState state)
> }
>
> if (!backend_run) {
> - virtio_set_status(vdev, vdev->status);
> + // the return value was used for stopping VM during migration
> + int ret = virtio_set_status(vdev, vdev->status);
> + if (ret) {
> + return ret;
> + }
> }
> return 0;
> }
> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
> index c5d2c09455..d54d9c916f 100644
> --- a/include/hw/virtio/vhost-scsi-common.h
> +++ b/include/hw/virtio/vhost-scsi-common.h
> @@ -40,7 +40,7 @@ struct VHostSCSICommon {
> };
>
> int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
> -void vhost_scsi_common_stop(VHostSCSICommon *vsc);
> +int vhost_scsi_common_stop(VHostSCSICommon *vsc);
> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
> DeviceState *dev);
> void vhost_scsi_common_set_config(VirtIODevice *vdev, const uint8_t *config);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 6386910280..c99d56f519 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -187,6 +187,7 @@ struct VirtioDeviceClass {
> void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
> void (*reset)(VirtIODevice *vdev);
> void (*set_status)(VirtIODevice *vdev, uint8_t val);
> + int (*set_status_ext)(VirtIODevice *vdev, uint8_t val);
> /* Device must validate queue_index. */
> void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> /* Device must validate queue_index. */
> --
> 2.48.1
>
Thanks for reviewing, I will fix this grammar issue in the next version.
>
>
> 2025年3月10日 00:30，Raphael Norwitz <raphael@enfabrica.net> 写道：
>
> I'll let others chime in on higher level device model changes you're
> making. For the vhost-user-blk and vhost-user-scsi bits:
>
> On Sun, Mar 9, 2025 at 11:41 AM Haoqian He <haoqian.he@smartx.com> wrote:
>>
>> Live migration should be terminated if backend crash.
>
> nit: "...if the backend crashes before the migration completes."
>
>>
>> Since the vhost device will be stopped when VM is stopped before
>> the end of the live migration, current implementation if vhost
>> backend died, vhost device's set_status() will not return failure,
>> live migration won't perceive the disconnection between qemu and
>> vhost backend, inflight io would be submitted in migration target
>> host, leading to IO error.
>>
>> To fix this issue:
>> 1. Add set_status_ext() which has return value for
>> VirtioDeviceClass and vhost-user-blk/scsi use the _ext version.
>>
>> 2. In set_status_ext(), return failure if the flag `connected`
>> is false or vhost_dev_stop return failure, which means qemu lost
>> connection with backend.
>
> I have some concerns with [2]. See my later responses.
>
>>
>> Hence migration_completion() will process failure, terminate
>> migration and restore VM.
>>
>> Signed-off-by: Haoqian He <haoqian.he@smartx.com>
>> ---
>> hw/block/vhost-user-blk.c | 29 +++++++++++++++------------
>> hw/scsi/vhost-scsi-common.c | 11 +++++-----
>> hw/scsi/vhost-user-scsi.c | 20 ++++++++++--------
>> hw/virtio/virtio.c | 20 +++++++++++++-----
>> include/hw/virtio/vhost-scsi-common.h | 2 +-
>> include/hw/virtio/virtio.h | 1 +
>> 6 files changed, 51 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index ae42327cf8..4865786c54 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -204,7 +204,7 @@ err_host_notifiers:
>> return ret;
>> }
>>
>> -static void vhost_user_blk_stop(VirtIODevice *vdev)
>> +static int vhost_user_blk_stop(VirtIODevice *vdev)
>> {
>> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> @@ -212,26 +212,26 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
>> int ret;
>>
>> if (!s->started_vu) {
>> - return;
>> + return 0;
>> }
>> s->started_vu = false;
>>
>> if (!k->set_guest_notifiers) {
>> - return;
>> + return 0;
>> }
>>
>> - vhost_dev_stop(&s->dev, vdev, true);
>> + ret = vhost_dev_stop(&s->dev, vdev, true);
>>
>> - ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
>> - if (ret < 0) {
>> + if (k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false) < 0) {
>> error_report("vhost guest notifier cleanup failed: %d", ret);
>> - return;
>> + return -1;
>> }
>>
>> vhost_dev_disable_notifiers(&s->dev, vdev);
>> + return ret;
>> }
>>
>> -static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>> +static int vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>> bool should_start = virtio_device_should_start(vdev, status);
>> @@ -239,11 +239,11 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>> int ret;
>>
>
The series of patches will not impact power-on if the backend is temporarily
down. The first patch ([PATCH 1/3] virtio: add VM state change cb with return
value) ignores the return value of VMChangeStateHandlerExt, which is
vhost_user_blk_set_status:
@@ -393,7 +398,12 @@ void vm_state_notify(bool running, RunState state)
}
QTAILQ_FOREACH_SAFE(e, &vm_change_state_head, entries, next) {
- e->cb(e->opaque, running, state);
+ if (e->cb) {
+ e->cb(e->opaque, running, state);
+ } else if (e->cb_ext) {
+ // no need to process the result when starting VM
+ e->cb_ext(e->opaque, running, state);
+ }
}
} else {
QTAILQ_FOREACH_REVERSE_SAFE(e, &vm_change_state_head, entries, next) {
@@ -403,9 +413,14 @@ void vm_state_notify(bool running, RunState state)
}
QTAILQ_FOREACH_REVERSE_SAFE(e, &vm_change_state_head, entries, next) {
- e->cb(e->opaque, running, state);
+ if (e->cb) {
+ e->cb(e->opaque, running, state);
+ } else if (e->cb_ext) {
+ ret |= e->cb_ext(e->opaque, running, state);
+ }
}
}
+ return ret;
}
>
>
> Do we want to fail out in all cases where vhost_user_blk_set_status()
> and the device is not connected? Could this impact power-on if the
> backend is temporarily down?
>
>> if (!s->connected) {
>> - return;
>> + return -1;
>> }
>>
>> if (vhost_dev_is_started(&s->dev) == should_start) {
>> - return;
>> + return 0;
>> }
>>
>> if (should_start) {
>> @@ -253,9 +253,12 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
>> qemu_chr_fe_disconnect(&s->chardev);
>> }
>> } else {
>> - vhost_user_blk_stop(vdev);
>> + ret = vhost_user_blk_stop(vdev);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> }
>> -
>> + return 0;
>> }
>>
>> static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
>> @@ -597,7 +600,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass, void *data)
>> vdc->get_config = vhost_user_blk_update_config;
>> vdc->set_config = vhost_user_blk_set_config;
>> vdc->get_features = vhost_user_blk_get_features;
>> - vdc->set_status = vhost_user_blk_set_status;
>> + vdc->set_status_ext = vhost_user_blk_set_status;
>> vdc->reset = vhost_user_blk_reset;
>> vdc->get_vhost = vhost_user_blk_get_vhost;
>> }
>> diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
>> index 4c8637045d..bfeed0cb1b 100644
>> --- a/hw/scsi/vhost-scsi-common.c
>> +++ b/hw/scsi/vhost-scsi-common.c
>> @@ -101,24 +101,25 @@ err_host_notifiers:
>> return ret;
>> }
>>
>> -void vhost_scsi_common_stop(VHostSCSICommon *vsc)
>> +int vhost_scsi_common_stop(VHostSCSICommon *vsc)
>> {
>> VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> int ret = 0;
>>
>> - vhost_dev_stop(&vsc->dev, vdev, true);
>> + ret = vhost_dev_stop(&vsc->dev, vdev, true);
>>
>> if (k->set_guest_notifiers) {
>> - ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> - if (ret < 0) {
>> + int r = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
>> + if (r < 0) {
>> error_report("vhost guest notifier cleanup failed: %d", ret);
>> }
>
Yes, we can remove the assert here and return the return value, which will
also be modified in the next version.
> Now that we return back a value the assert here should probably be dropped?
>
>> + assert(r >= 0);
>> }
>> - assert(ret >= 0);
>>
>> vhost_dev_disable_notifiers(&vsc->dev, vdev);
>> + return ret;
>> }
>>
>> uint64_t vhost_scsi_common_get_features(VirtIODevice *vdev, uint64_t features,
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index adb41b9816..8e7efc38f2 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -52,19 +52,19 @@ static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
>> return ret;
>> }
>>
>> -static void vhost_user_scsi_stop(VHostUserSCSI *s)
>> +static int vhost_user_scsi_stop(VHostUserSCSI *s)
>> {
>> VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>>
>> if (!s->started_vu) {
>> - return;
>> + return 0;
>> }
>> s->started_vu = false;
>>
>> - vhost_scsi_common_stop(vsc);
>> + return vhost_scsi_common_stop(vsc);
>> }
>>
>> -static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> +static int vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostUserSCSI *s = (VHostUserSCSI *)vdev;
>> DeviceState *dev = DEVICE(vdev);
>> @@ -75,11 +75,11 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> int ret;
>>
>> if (!s->connected) {
>> - return;
>> + return -1;
>> }
>>
>> if (vhost_dev_is_started(&vsc->dev) == should_start) {
>> - return;
>> + return 0;
>> }
>>
>> if (should_start) {
>> @@ -91,8 +91,12 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> qemu_chr_fe_disconnect(&vs->conf.chardev);
>> }
>> } else {
>> - vhost_user_scsi_stop(s);
>> + ret = vhost_user_scsi_stop(s);
>> + if (ret) {
>> + return ret;
>> + }
>> }
>> + return 0;
>> }
>>
>> static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>> @@ -399,7 +403,7 @@ static void vhost_user_scsi_class_init(ObjectClass *klass, void *data)
>> vdc->unrealize = vhost_user_scsi_unrealize;
>> vdc->get_features = vhost_scsi_common_get_features;
>> vdc->set_config = vhost_scsi_common_set_config;
>> - vdc->set_status = vhost_user_scsi_set_status;
>> + vdc->set_status_ext = vhost_user_scsi_set_status;
>> fwc->get_dev_path = vhost_scsi_common_get_fw_dev_path;
>> vdc->reset = vhost_user_scsi_reset;
>> vdc->get_vhost = vhost_user_scsi_get_vhost;
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 5e8d4cab53..fff7cdb35d 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2221,12 +2221,12 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>> {
>> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> trace_virtio_set_status(vdev, val);
>> + int ret = 0;
>>
>> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>> if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) &&
>> val & VIRTIO_CONFIG_S_FEATURES_OK) {
>> - int ret = virtio_validate_features(vdev);
>> -
>> + ret = virtio_validate_features(vdev);
>> if (ret) {
>> return ret;
>> }
>> @@ -2238,12 +2238,18 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>> virtio_set_started(vdev, val & VIRTIO_CONFIG_S_DRIVER_OK);
>> }
>>
>> - if (k->set_status) {
>> + if (k->set_status_ext) {
>> + ret = k->set_status_ext(vdev, val);
>> + if (ret) {
>> + qemu_log("set %s status to %d failed, old status: %d\n",
>> + vdev->name, val, vdev->status);
>> + }
>> + } else if (k->set_status) {
>> k->set_status(vdev, val);
>> }
>> vdev->status = val;
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static enum virtio_device_endian virtio_default_endian(void)
>> @@ -3436,7 +3442,11 @@ static int virtio_vmstate_change(void *opaque, bool running, RunState state)
>> }
>>
>> if (!backend_run) {
>> - virtio_set_status(vdev, vdev->status);
>> + // the return value was used for stopping VM during migration
>> + int ret = virtio_set_status(vdev, vdev->status);
>> + if (ret) {
>> + return ret;
>> + }
>> }
>> return 0;
>> }
>> diff --git a/include/hw/virtio/vhost-scsi-common.h b/include/hw/virtio/vhost-scsi-common.h
>> index c5d2c09455..d54d9c916f 100644
>> --- a/include/hw/virtio/vhost-scsi-common.h
>> +++ b/include/hw/virtio/vhost-scsi-common.h
>> @@ -40,7 +40,7 @@ struct VHostSCSICommon {
>> };
>>
>> int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp);
>> -void vhost_scsi_common_stop(VHostSCSICommon *vsc);
>> +int vhost_scsi_common_stop(VHostSCSICommon *vsc);
>> char *vhost_scsi_common_get_fw_dev_path(FWPathProvider *p, BusState *bus,
>> DeviceState *dev);
>> void vhost_scsi_common_set_config(VirtIODevice *vdev, const uint8_t *config);
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 6386910280..c99d56f519 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -187,6 +187,7 @@ struct VirtioDeviceClass {
>> void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
>> void (*reset)(VirtIODevice *vdev);
>> void (*set_status)(VirtIODevice *vdev, uint8_t val);
>> + int (*set_status_ext)(VirtIODevice *vdev, uint8_t val);
>> /* Device must validate queue_index. */
>> void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
>> /* Device must validate queue_index. */
>> --
>> 2.48.1
Thanks
--
Haoqian
© 2016 - 2025 Red Hat, Inc.