migration/cpr: support vhost-vsock devices
Subsystems (2)
v16 cover migration/cpr: support vhost-vsock devices
v15 -> v16:
* Patch 15:
- replace the errno filtering in vhost_dev_try_release_owner() with a new 'owner_handed_over' flag in struct vhost_dev. Release the ownership only if it was handed over to us, and report any failure;
* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.
v15: https://lore.kernel.org/qemu-devel/[email protected]
---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: add vhost_reset_owner op
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak in realize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor out vhost_dev_init_backend()
vhost: make vhost_dev_cleanup() safe on a partially initialized device
vhost: add vhost_dev_set_owner() / vhost_dev_reset_owner() helpers
vhost: add vhost_dev_is_initialized() helper
vhost: release ownership in vhost_dev_cleanup()
migration: add MIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsock
backends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)
---
base-commit: 615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1a
Best regards,
Signature · 1 line
Andrey Drobyshev <[email protected]>
PATCH v16 1/22 migration/cpr: forbid saving duplicate CprFds into hash table
In its current form, the mechanism for saving CPR FDs into hash table has flaws.
The CPR fd hash table uses the same CprFd for both key and value, and is created with a key destroy function. Inserting a second element with the same (name, id) key leads to a use after free. According to GLib docs [1], g_hash_table_insert() keeps the old key, replaces the value with the new element, and destroys the new key. Thus when inserting 2 CprFd elements fd1 and fd2 with equal keys we get:
g_hash_table_insert(table, fd1, fd1); /* table has fd1 -> fd1 */ g_hash_table_insert(table, fd2, fd2); /* table has fd1 -> fd2, fd2 freed */
Any subsequent cpr_find_fd() for that key then returns freed memory.
This is reachable whenever two CPR-aware objects register fds under the same name, and it happens long before an actual CPR, since cpr_save_fd() is typically called from .realize(), i.e. at boot time.
Apart from the UAF itself, when the user decides to perform CPR migration, on CPR target we'd get corrupted FD from the hash table and fail somewhere in .realize() for affected devices.
As a solution, let's altogether prohibit adding CprFds with duplicate keys. Since FDs are being registered for a subsequent CPR right at the startup in .realize(), that'll allow us to error out and exit QEMU (or fail a device hotplug op) gracefully instead of delaying potential problems for later. To do so, let's make cpr_save_fd() return 'bool' instead of 'void' and provide an 'errp' argument to it. That way caller becomes responsible for checking the return value and propagating the error.
Also update all the callers to use the adjusted interface to keep things compilable and bisectable.
[1] https://docs.gtk.org/glib/type_func.HashTable.insert.html
--- backends/hostmem-memfd.c | 5 +++- backends/hostmem-shm.c | 5 +++- backends/iommufd.c | 4 ++- hw/vfio/container-legacy.c | 7 +---- hw/vfio/cpr-legacy.c | 29 ++++++++++++++++----- hw/vfio/cpr.c | 6 ++--- hw/vfio/pci.c | 5 +++- include/hw/vfio/vfio-cpr.h | 7 ++--- include/migration/cpr.h | 2 +- migration/cpr.c | 64 +++++++++++++++++++++++++++++----------------- system/physmem.c | 5 ++-- 11 files changed, 91 insertions(+), 48 deletions(-)
backends/hostmem-memfd.c +4 -1
@@ -84,7 +84,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)if (fd <0 ) {return false; } -cpr_save_fd (name,0 , fd); +if (!cpr_save_fd (name,0 , fd, errp)) { +close (fd); +return false; + } have_fd: backend->aligned = true;
backends/hostmem-shm.c +4 -1
@@ -48,7 +48,10 @@ shm_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)if (fd <0 ) {return false; } -cpr_save_fd (backend_name,0 , fd); +if (!cpr_save_fd (backend_name,0 , fd, errp)) { +close (fd); +return false; + } have_fd:/* Let's do the same as memory-backend-ram,share=on would do. */
backends/iommufd.c +3 -1
@@ -85,7 +85,9 @@ static void iommufd_backend_complete(UserCreatable *uc, Error **errp)if (cpr_is_incoming ()) { be->fd =cpr_find_fd (name,0 ); }else { -cpr_save_fd (name,0 , be->fd); +if (!cpr_save_fd (name,0 , be->fd, errp)) { +return ; + } } }else if (!g_file_test ("/dev/iommu" ,G_FILE_TEST_EXISTS )) { error_setg(errp, "/dev/iommu does not exist"
hw/vfio/container-legacy.c +1 -6
@@ -847,13 +847,8 @@ static bool vfio_device_get(VFIOGroup *group, const char *name, g_autofree struct vfio_device_info *info = NULL;int fd; - fd =vfio_cpr_group_get_device_fd (group->fd, name); + fd =vfio_cpr_group_get_device_fd (group, name, errp);if (fd <0 ) { -error_setg_errno (errp, errno,"error getting device from group %d" , - group->groupid); -error_append_hint (errp, -"Verify all devices in group %d are bound to vfio-<bus> " -"or pci-stub and not already in use\n" , group->groupid);return false; }
hw/vfio/cpr-legacy.c +23 -6
@@ -254,17 +254,33 @@ bool vfio_cpr_ram_discard_replay_populated(VFIOContainer *bcontainer, &vrdl->listener) == 0; } -int vfio_cpr_group_get_device_fd (int d,const char *name) +int vfio_cpr_group_get_device_fd (VFIOGroup *group,const char *name, +Error **errp) { +ERRP_GUARD ();const int id =0 ;int fd =cpr_find_fd (name, id); +if (fd >=0 ) { +return fd; + } + + fd =ioctl (group->fd,VFIO_GROUP_GET_DEVICE_FD , name);if (fd <0 ) { - fd =ioctl (d,VFIO_GROUP_GET_DEVICE_FD , name); -if (fd >=0 ) { -cpr_save_fd (name, id, fd); - } +error_setg_errno (errp, errno,"error getting device from group %d" , + group->groupid); +error_append_hint (errp, +"Verify all devices in group %d are bound to " +"vfio-<bus> or pci-stub and not already in use\n" , + group->groupid); +return -1 ; } + +if (!cpr_save_fd (name, id, fd, errp)) { +close (fd); +return -1 ; + } +return fd; }
@@ -291,6 +307,7 @@ bool vfio_cpr_container_match(VFIOLegacyContainer *container, VFIOGroup *group,*/ cpr_delete_fd ("vfio_container_for_group" , group->groupid);close (fd); -cpr_save_fd ("vfio_container_for_group" , group->groupid, container->fd); +cpr_save_fd ("vfio_container_for_group" , group->groupid, container->fd, + &error_abort);return true; }
hw/vfio/cpr.c +3 -3
@@ -32,11 +32,11 @@ int vfio_cpr_reboot_notifier(NotifierWithReturn *notifier,#define STRDUP_VECTOR_FD_NAME (vdev, name) \ g_strdup_printf("%s_%s", (vdev)->vbasedev.name, (name)) -void vfio_cpr_save_vector_fd (VFIOPCIDevice *vdev,const char *name,int nr, -int fd) +bool vfio_cpr_save_vector_fd (VFIOPCIDevice *vdev,const char *name,int nr, +int fd,Error **errp) { g_autofree char *fdname = STRDUP_VECTOR_FD_NAME(vdev, name); -cpr_save_fd (fdname, nr, fd); +return cpr_save_fd (fdname, nr, fd, errp); }int vfio_cpr_load_vector_fd (VFIOPCIDevice *vdev,const char *name,int nr)
hw/vfio/pci.c +4 -1
@@ -76,7 +76,10 @@ static bool vfio_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e, } fd =event_notifier_get_fd (e); -vfio_cpr_save_vector_fd (vdev, name, nr, fd); +if (!vfio_cpr_save_vector_fd (vdev, name, nr, fd, errp)) { +event_notifier_cleanup (e); +return false; + }return true; }
include/hw/vfio/vfio-cpr.h +4 -3
@@ -60,7 +60,8 @@ void vfio_iommufd_cpr_register_device(struct VFIODevice *vbasedev);void vfio_iommufd_cpr_unregister_device (struct VFIODevice *vbasedev);void vfio_cpr_load_device (struct VFIODevice *vbasedev); -int vfio_cpr_group_get_device_fd (int d,const char *name); +int vfio_cpr_group_get_device_fd (struct VFIOGroup *group,const char *name, +Error **errp);bool vfio_cpr_container_match (struct VFIOLegacyContainer *container,struct VFIOGroup *group,int fd);
@@ -71,8 +72,8 @@ void vfio_cpr_giommu_remap(struct VFIOContainer *bcontainer,bool vfio_cpr_ram_discard_replay_populated (struct VFIOContainer *bcontainer,const MemoryRegionSection *section); -void vfio_cpr_save_vector_fd (struct VFIOPCIDevice *vdev,const char *name, -int nr,int fd); +bool vfio_cpr_save_vector_fd (struct VFIOPCIDevice *vdev,const char *name, +int nr,int fd,Error **errp);int vfio_cpr_load_vector_fd (struct VFIOPCIDevice *vdev,const char *name,int nr); void vfio_cpr_delete_vector_fd(struct VFIOPCIDevice *vdev, const char *name,
include/migration/cpr.h +1 -1
@@ -28,7 +28,7 @@ typedef struct CprState {extern CprState cpr_state; -void cpr_save_fd (const char *name,int id,int fd); +bool cpr_save_fd (const char *name,int id,int fd,Error **errp);void cpr_delete_fd (const char *name,int id);int cpr_find_fd (const char *name,int id);void cpr_resave_fd (const char *name,int id,int fd);
migration/cpr.c +41 -23
109 lines not on this page · raw message
system/physmem.c +3 -2
@@ -2456,8 +2456,9 @@ static int qemu_ram_get_shared_fd(const char *name, bool *reused, Error **errp) fd =qemu_shm_alloc (0 , errp); } -if (fd >=0 ) { -cpr_save_fd (name,0 , fd); +if (fd >=0 && !cpr_save_fd (name,0 , fd, errp)) { +close (fd); + fd =-1 ; } *reused = false;return fd;
Signature · 1 line
2.47.1
Re: [PATCH v16 01/22] migration/cpr: forbid saving duplicate CprFds into hash table
Quoted 35 lines · Andrey Drobyshev
In its current form, the mechanism for saving CPR FDs into hash table has flaws.
The CPR fd hash table uses the same CprFd for both key and value, and is created with a key destroy function. Inserting a second element with the same (name, id) key leads to a use after free. According to GLib docs [1],
g_hash_table_insert()keeps the old key, replaces the value with the new element, and destroys the new key. Thus when inserting 2 CprFd elements fd1 and fd2 with equal keys we get:g_hash_table_insert(table, fd1, fd1); /* table has fd1 -> fd1 */ g_hash_table_insert(table, fd2, fd2); /* table has fd1 -> fd2, fd2 freed */Any subsequent
cpr_find_fd()for that key then returns freed memory.This is reachable whenever two CPR-aware objects register fds under the same name, and it happens long before an actual CPR, since
cpr_save_fd()is typically called from .realize(), i.e. at boot time.Apart from the UAF itself, when the user decides to perform CPR migration, on CPR target we'd get corrupted FD from the hash table and fail somewhere in .realize() for affected devices.
As a solution, let's altogether prohibit adding CprFds with duplicate keys. Since FDs are being registered for a subsequent CPR right at the startup in .realize(), that'll allow us to error out and exit QEMU (or fail a device hotplug op) gracefully instead of delaying potential problems for later. To do so, let's make
cpr_save_fd()return 'bool' instead of 'void' and provide an 'errp' argument to it. That way caller becomes responsible for checking the return value and propagating the error.Also update all the callers to use the adjusted interface to keep things compilable and bisectable.
Quoted 6 lines
[1] https://docs.gtk.org/glib/type_func.HashTable.insert.html
Fixes a181df93bb3 ("migration/cpr: use hashtable for cpr fds")Reviewed-by Peter Xu <[email protected]>Reviewed-by Cédric Le Goater <[email protected]>Signed-off-by Andrey Drobyshev <[email protected]>
Hi everyone, FYI we've recently run into a regression on s390x where if we have a KVM guest with a PCI passthrough device using iommufd and we hot-unplug the device, the guest becomes fully unresponsive. In the host dmesg, we can see this message right when the hang begins:
[ 1618.849602] vfio-pci 0001:00:00.0: No device request channel
registered, blocked until released by user
Bisecting qemu leads us to this patch:
de80025348925ba9f56070ce4ec0a5a0c512d450 is the first 'bad' commit commit de80025348925ba9f56070ce4ec0a5a0c512d450
Author: Andrey Drobyshev <[email protected]> Date: Mon Oct 12 13:28:09 2026 +0300
migration/cpr: forbid saving duplicate CprFds into hash table
We tried to revert the commit to confirm the root cause, but then the master branch no longer compiles, so unfortunately we were not able to confirm that, at least without making many changes.
We're wondering if you might have any ideas what's going on there, or what we could do to try and resolve this issue. Any thoughts would be much appreciated and I'd be happy to share more info if needed.
Thanks in advance!
[...]
PATCH v16 2/22 tests/unit: add test for duplicate keys in CPR FD table
In reply to Andrey Drobyshevcpr_save_fd() now refuses to register an FD under a (name, id) key which is already taken. Add a test which checks that 2nd registration fails with an error while the 1st one is left intact, that different names or ids don't clash, and that the key can be reused once deleted.
--- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+)
tests/unit/meson.build +1 -0
@@ -140,6 +140,7 @@ if have_system 'test-bufferiszero': [], 'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c'], 'test-vmstate': [migration, io], + 'test-cpr': [migration, io, meson.project_source_root() / 'migration/cpr.c'], 'test-yank': ['socket-helpers.c', qom, io, chardev], 'test-ram-discard-manager': [ 'test-ram-discard-manager.c',
tests/unit/test-cpr.c new file +98 -0
98 lines not on this page · raw message
Signature · 1 line
2.47.1
PATCH v16 3/22 migration: forbid snapshots in CPR migration modes
In reply to Andrey DrobyshevThe savevm/loadvm operation doesn't make any sense in any of the CPR modes. Saving VM state implies saving its RAM entirely, while the whole point of CPR is avoiding that: in cpr-transfer and cpr-exec modes RAM is preserved in place and never put into the migration stream (see migrate_ram_is_ignored() in migration/ram.c). As a result, a snapshot taken with such a mode set has no RAM in it, and loading a snapshot with such a mode set fails in the RAM loader.
Currently nothing stops these operations from firing with a CPR mode still set, e.g. after an earlier CPR attempt. Reject them early in migrate_can_snapshot(), which guards 'savevm'/'loadvm' HMP commands as well as 'snapshot-save'/'snapshot-load' QMP commands.
--- migration/options.c | 7 +++++++ 1 file changed, 7 insertions(+)
migration/options.c +7 -0
@@ -560,8 +560,15 @@ static bool migrate_incoming_started(void)bool migrate_can_snapshot (Error **errp) {MigrationState *s =migrate_get_current (); +MigMode mode =migrate_mode ();int i; +if (mode !=MIG_MODE_NORMAL ) { +error_setg (errp,"Snapshots are not compatible with migration mode %s" , +MigMode_str (mode)); +return false; + } +for (i =0 ; i < check_caps_savevm.size; i++) {int incomp_cap = check_caps_savevm.caps[i];
Signature · 1 line
2.47.1
PATCH v16 4/22 migration: forbid snapshots under Xen
In reply to Andrey DrobyshevThe snapshot RAM-saving path does not support Xen's memory mappings. Refuse 'savevm' and 'loadvm' when running as a Xen device model, the same way they are refused for CPR migration modes.
--- migration/options.c | 6 ++++++ 1 file changed, 6 insertions(+)
migration/options.c +6 -0
@@ -37,6 +37,7 @@#include "ram.h" #include "options.h" #include "system/kvm.h" +#include "system/xen.h" /* Maximum migrate downtime set to 2000 seconds */ #define MAX_MIGRATE_DOWNTIME_SECONDS 2000
@@ -563,6 +564,11 @@ bool migrate_can_snapshot(Error **errp)MigMode mode =migrate_mode ();int i; +if (xen_enabled ()) { +error_setg (errp,"Snapshots are not compatible with Xen" ); +return false; + } +if (mode !=MIG_MODE_NORMAL ) {error_setg (errp,"Snapshots are not compatible with migration mode %s" ,MigMode_str (mode));
Signature · 1 line
2.47.1
PATCH v16 5/22 tests/qtest/migration: add test for snapshot rejection in CPR modes
In reply to Andrey DrobyshevSnapshots are refused whenever a CPR migration mode is set. Add a test which sets each of the CPR modes in turn and checks that 'savevm' and 'loadvm' fail with the corresponding error, then switches back to 'normal' mode and checks that the error is gone.
--- tests/qtest/migration/cpr-tests.c | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
tests/qtest/migration/cpr-tests.c +40 -0
@@ -290,6 +290,44 @@ static void test_mode_exec(char *name, MigrateCommon *args)test_cpr_exec (args); } +/* Snapshots must be refused while a CPR migration mode is set */ +static void test_mode_no_snapshot (char *name,MigrateCommon *args) +{ +static const char *const modes[] = { +"cpr-reboot" ,"cpr-transfer" ,"cpr-exec" , + }; +QTestState *vm =qtest_init ("-nodefaults -machine none" ); +char *out; +int i; + +for (i =0 ; i <ARRAY_SIZE (modes); i++) { + g_autofree char *expected = g_strdup_printf( +"Snapshots are not compatible with migration mode %s" , modes[i]); + +migrate_set_parameter_str (vm,"mode" , modes[i]); + + out =qtest_hmp (vm,"savevm snap" ); +g_assert_nonnull (strstr (out, expected)); +g_free (out); + + out =qtest_hmp (vm,"loadvm snap" ); +g_assert_nonnull (strstr (out, expected)); +g_free (out); + } + +/* +* The check should no longer fire in normal mode. savevm still fails +* (there's no block device for storing the snapshot). We just check +* that it's not the mode error any more. +*/ +migrate_set_parameter_str (vm,"mode" ,"normal" ); + out =qtest_hmp (vm,"savevm snap" ); +g_assert_null (strstr (out,"not compatible with migration mode" )); +g_free (out); + +qtest_quit (vm); +} +void migration_test_add_cpr (MigrationTestEnv *env) { tmpfs = env->tmpfs;
@@ -300,6 +338,8 @@ void migration_test_add_cpr(MigrationTestEnv *env)return ; } +migration_test_add ("/migration/mode/no-snapshot" , test_mode_no_snapshot); +/* * Our CI system has problems with shared memory. * Don't run this test until we find a workaround.
Signature · 1 line
2.47.1
PATCH v16 6/22 vhost-vsock: block CPR migration modes
In reply to Andrey DrobyshevCPR migration with a vhost-vsock device currently crashes the target QEMU. The device ownership is not handed over, so while the source QEMU still holds the guest CID hashed in the kernel, the target's freshly opened vhost-vsock device fails to set the same CID in realize():
qemu-system-x86_64: -device vhost-vsock-pci,id=vsock0,guest-cid=3:
vhost-vsock: unable to set guest cid: Address already in use
Add an unconditional migration blocker for the CPR modes, so that such a migration fails early and gracefully instead. The following patches implement the actual vhost-vsock CPR support, and the blocker is lifted once it's fully working.
--- hw/virtio/vhost-vsock.c | 27 +++++++++++++++++++++++---- include/hw/virtio/vhost-vsock.h | 1 + 2 files changed, 24 insertions(+), 4 deletions(-)
hw/virtio/vhost-vsock.c +23 -4
@@ -20,6 +20,8 @@#include "hw/core/qdev-properties.h" #include "hw/virtio/vhost-vsock.h" #include "monitor/monitor.h" +#include "migration/blocker.h" +#include "migration/misc.h" static void vhost_vsock_get_config (VirtIODevice *vdev,uint8_t *config) {
@@ -140,25 +142,38 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)return ; } +/* +* CPR migration of a vhost-vsock device is not supported yet: the +* device ownership is not handed over, so the target fails to set +* up its device. Fail the migration early and gracefully instead. +*/ +error_setg (&vsock->migration_blocker, +"vhost-vsock: CPR migration is not supported" ); +if (migrate_add_blocker_modes (&vsock->migration_blocker, +BIT (MIG_MODE_CPR_TRANSFER ) | +BIT (MIG_MODE_CPR_EXEC ), errp) <0 ) { +return ; + } +if (vsock->conf.vhostfd) { vhostfd =monitor_fd_param (monitor_cur (), vsock->conf.vhostfd, errp);if (vhostfd ==-1 ) {error_prepend (errp,"vhost-vsock: unable to parse vhostfd: " ); -return ; + goto err_blocker; }if (!qemu_set_blocking (vhostfd, false, errp)) { -return ; + goto err_blocker; } }else { vhostfd =open ("/dev/vhost-vsock" ,O_RDWR );if (vhostfd <0 ) {error_setg_file_open (errp, errno,"/dev/vhost-vsock" ); -return ; + goto err_blocker; }if (!qemu_set_blocking (vhostfd, false, errp)) { -return ; + goto err_blocker; } }
@@ -187,16 +202,20 @@ err_vhost_dev:vhost_dev_cleanup (&vvc->vhost_dev); err_virtio:vhost_vsock_common_unrealize (vdev); +err_blocker: +migrate_del_blocker (&vsock->migration_blocker); }static void vhost_vsock_device_unrealize (DeviceState *dev) {VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (dev);VirtIODevice *vdev =VIRTIO_DEVICE (dev); +VHostVSock *vsock =VHOST_VSOCK (dev);/* This will stop vhost backend if appropriate. */ vhost_vsock_set_status (vdev,0 ); +migrate_del_blocker (&vsock->migration_blocker);vhost_dev_cleanup (&vvc->vhost_dev);vhost_vsock_common_unrealize (vdev); }
include/hw/virtio/vhost-vsock.h +1 -0
@@ -29,6 +29,7 @@ struct VHostVSock {/*< private >*/ VHostVSockCommon parent;VHostVSockConf conf; +Error *migration_blocker;/* CPR migration is not supported */ /*< public >*/ };
Signature · 1 line
2.47.1
PATCH v16 7/22 vhost: add vhost_reset_owner op
In reply to Andrey DrobyshevAdd a VhostOps callback issuing VHOST_RESET_OWNER, wired up for the kernel backend. The CPR (checkpoint-restore) path uses it to release device ownership on the source so the destination can reclaim it later with VHOST_SET_OWNER.
--- hw/virtio/vhost-kernel.c | 6 ++++++ include/hw/virtio/vhost-backend.h | 2 ++ 2 files changed, 8 insertions(+)
hw/virtio/vhost-kernel.c +6 -0
@@ -261,6 +261,11 @@ static int vhost_kernel_set_owner(struct vhost_dev *dev)return vhost_kernel_call (dev,VHOST_SET_OWNER ,NULL ); } +static int vhost_kernel_reset_owner (struct vhost_dev *dev) +{ +return vhost_kernel_call (dev,VHOST_RESET_OWNER ,NULL ); +} +static int vhost_kernel_get_vq_index (struct vhost_dev *dev,int idx) {assert (idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
@@ -385,6 +390,7 @@ const VhostOps kernel_ops = { .vhost_get_features_ex = vhost_kernel_get_features, .vhost_set_backend_cap = vhost_kernel_set_backend_cap, .vhost_set_owner = vhost_kernel_set_owner, + .vhost_reset_owner = vhost_kernel_reset_owner, .vhost_get_vq_index = vhost_kernel_get_vq_index, .vhost_vsock_set_guest_cid = vhost_kernel_vsock_set_guest_cid, .vhost_vsock_set_running = vhost_kernel_vsock_set_running,
include/hw/virtio/vhost-backend.h +2 -0
@@ -104,6 +104,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,
@@ -205,6 +206,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;
Signature · 1 line
2.47.1
PATCH v16 8/22 vhost-vsock: don't reset connections during CPR
In reply to Andrey DrobyshevMigration target with a vhost-vsock device in the .post_load() hook issues VIRTIO_VSOCK_EVENT_TRANSPORT_RESET event to the guest. This results into the guest tearing down all its vsock connections. That reset exists because after a normal migration the cid may change.
However it's not true for CPR-style migration. In this case cid remains the same, and we want the connections to persist. Thus, let's customize the .post_load() hook to skip the common transport reset part in this case.
--- hw/virtio/vhost-vsock.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
hw/virtio/vhost-vsock.c +16 -1
@@ -20,6 +20,7 @@#include "hw/core/qdev-properties.h" #include "hw/virtio/vhost-vsock.h" #include "monitor/monitor.h" +#include "migration/cpr.h" #include "migration/blocker.h" #include "migration/misc.h"
@@ -110,6 +111,20 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,return vhost_vsock_common_get_features (vdev, requested_features, errp); } +static int vhost_vsock_post_load (void *opaque,int version_id) +{ +/* +* Only reset vsock connections for non-CPR migration. For CPR the +* guest cid is unchanged, and the cid-change reset would otherwise +* tear the vsock connections down. +*/ +if (!cpr_is_incoming ()) { +return vhost_vsock_common_post_load (opaque, version_id); + } + +return 0 ; +} +static const VMStateDescription vmstate_virtio_vhost_vsock = { .name ="virtio-vhost_vsock" , .minimum_version_id =VHOST_VSOCK_SAVEVM_VERSION ,
@@ -119,7 +134,7 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {VMSTATE_END_OF_LIST () }, .pre_save = vhost_vsock_common_pre_save, - .post_load = vhost_vsock_common_post_load, + .post_load = vhost_vsock_post_load, };static void vhost_vsock_device_realize (DeviceState *dev,Error **errp)
Signature · 1 line
2.47.1
PATCH v16 9/22 vhost-vsock: fix FD leak in realize()
In reply to Andrey DrobyshevIn vhost_vsock_device_realize(), after unsuccessful call to qemu_set_blocking(vhostfd, false), vhostfd gets leaked. Let's close it explicitly in this case. CPR-saved FDs don't get automatically closed, thus it is safe both for vhostfd obtained from cpr_find_fd() and from open(/dev/vhost-vsock).
--- hw/virtio/vhost-vsock.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
hw/virtio/vhost-vsock.c +4 -7
@@ -176,20 +176,17 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)error_prepend (errp,"vhost-vsock: unable to parse vhostfd: " ); goto err_blocker; } - -if (!qemu_set_blocking (vhostfd, false, errp)) { - goto err_blocker; - } }else { vhostfd =open ("/dev/vhost-vsock" ,O_RDWR );if (vhostfd <0 ) {error_setg_file_open (errp, errno,"/dev/vhost-vsock" ); goto err_blocker; } + } -if (!qemu_set_blocking (vhostfd, false, errp)) { - goto err_blocker; - } +if (!qemu_set_blocking (vhostfd, false, errp)) { +close (vhostfd); + goto err_blocker; }vhost_vsock_common_realize (vdev);
Signature · 1 line
2.47.1
PATCH v16 10/22 vhost-vsock: preserve vhost FD during CPR
In reply to Andrey DrobyshevDuring CPR (checkpoint-restore) migration the guest keeps running on the same host, so instead of reopening /dev/vhost-vsock on the destination, we should reuse the FD from the source. The FD is saved in the CPR namespace (hash table) with cpr_save_fd() and then reclaimed on the target via cpr_find_fd().
Use the (suffixed) device path as the key in CPR hash table. Device path is obtained with qdev_get_dev_path() and is essentially the device's address on the underlying bus. As any vhost-vsock device is attached to a virtio bus, whose .get_dev_path() forwards to the proxy device, and every proxy bus (PCI, MMIO, CCW) implements it, we should always be getting a unique non-empty path with qdev_get_dev_path().
vhost_dev_init() (and thus VHOST_SET_OWNER) still runs in realize() here. Deferring the ownership handoff to pre_save/post_load is done in a following patch. Until then CPR remains blocked, so the restore path added here is not reachable yet.
--- hw/virtio/vhost-vsock.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)
hw/virtio/vhost-vsock.c +36 -1
@@ -69,6 +69,18 @@ static int vhost_vsock_set_running(VirtIODevice *vdev, int start)return 0 ; } +/* +* Any vhost-vsock device is attached to a virtio bus, whose .get_dev_path() +* forwards to the proxy device, and every proxy bus (PCI, MMIO, CCW) +* implements it. Thus qdev_get_dev_path() always returns a non-empty string, +* which guarantees us a unique CPR name. +*/ +static char *vhost_vsock_cpr_name (DeviceState *dev) +{ + g_autofree char *path = qdev_get_dev_path(dev); +g_assert (path && path[0 ]); +return g_strdup_printf ("%s/vhost-vsock" , path); +}static int vhost_vsock_set_status (VirtIODevice *vdev,uint8_t status) {
@@ -143,6 +155,8 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (dev);VirtIODevice *vdev =VIRTIO_DEVICE (dev);VHostVSock *vsock =VHOST_VSOCK (dev); + g_autofree char *cpr_name = vhost_vsock_cpr_name(dev); +bool cpr_incoming =cpr_is_incoming ();int vhostfd;int ret;
@@ -170,7 +184,14 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)return ; } -if (vsock->conf.vhostfd) { +if (cpr_incoming) { +/* Reuse the fd handed over from the source QEMU */ + vhostfd =cpr_find_fd (cpr_name,0 ); +if (vhostfd <0 ) { +error_setg (errp,"vhost-vsock: could not find restored vhost FD" ); + goto err_blocker; + } + }else if (vsock->conf.vhostfd) { vhostfd =monitor_fd_param (monitor_cur (), vsock->conf.vhostfd, errp);if (vhostfd ==-1 ) {error_prepend (errp,"vhost-vsock: unable to parse vhostfd: " );
@@ -186,6 +207,9 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)if (!qemu_set_blocking (vhostfd, false, errp)) {close (vhostfd); +if (cpr_incoming) { +cpr_delete_fd (cpr_name,0 ); + } goto err_blocker; }
@@ -207,11 +231,20 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp) goto err_vhost_dev; } +/* Register the fd for a future CPR after a fully successful realize */ +if (!cpr_incoming && !cpr_save_fd (cpr_name,0 , vhostfd, errp)) { + goto err_vhost_dev; + } +return ; err_vhost_dev:/* vhost_dev_cleanup() closes the vhostfd passed to vhost_dev_init() */ vhost_dev_cleanup (&vvc->vhost_dev); +if (cpr_incoming) { +/* The fd came from cpr_find_fd() and is closed now, drop the entry */ +cpr_delete_fd (cpr_name,0 ); + } err_virtio:vhost_vsock_common_unrealize (vdev); err_blocker:
@@ -223,10 +256,12 @@ static void vhost_vsock_device_unrealize(DeviceState *dev)VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (dev);VirtIODevice *vdev =VIRTIO_DEVICE (dev);VHostVSock *vsock =VHOST_VSOCK (dev); + g_autofree char *cpr_name = vhost_vsock_cpr_name(dev);/* This will stop vhost backend if appropriate. */ vhost_vsock_set_status (vdev,0 ); +cpr_delete_fd (cpr_name,0 );migrate_del_blocker (&vsock->migration_blocker);vhost_dev_cleanup (&vvc->vhost_dev);vhost_vsock_common_unrealize (vdev);
Signature · 1 line
2.47.1
PATCH v16 11/22 vhost: factor out vhost_dev_init_backend()
In reply to Andrey DrobyshevSplit the first part of vhost_dev_init(): selecting the backend, calling its .vhost_init() and reading the supported features - into a new vhost_dev_init_backend() helper, and call it from vhost_dev_init().
This is in preparation for CPR restore of vhost-vsock, which needs to learn the backend's features at realize time to negotiate them when loading the incoming virtio state, but also must defer taking ownership of the device to post_load. vhost_dev_init_backend() does exactly the pre-ownership part.
As a result VHOST_SET_OWNER now follows the feature query rather than precedes it. This should be safe, as no backend requires ownership before VHOST_GET_FEATURES - the kernel and vdpa backends do not check ownership for it, and vhost-user already does query features from its .vhost_init() before set_owner().
--- hw/virtio/vhost.c | 33 +++++++++++++++++++++++---------- include/hw/virtio/vhost.h | 22 ++++++++++++++++++++++ 2 files changed, 45 insertions(+), 10 deletions(-)
hw/virtio/vhost.c +23 -10
@@ -1681,6 +1681,28 @@ static int vhost_dev_init_features(struct vhost_dev *hdev)return r; } +int vhost_dev_init_backend (struct vhost_dev *hdev,void *opaque, +VhostBackendType backend_type,Error **errp) +{ +int r; + + r =vhost_set_backend_type (hdev, backend_type); +assert (r >=0 ); + + r = hdev->vhost_ops->vhost_init (hdev, opaque, errp); +if (r <0 ) { +return r; + } + + r =vhost_dev_init_features (hdev); +if (r <0 ) { +error_setg_errno (errp, -r,"vhost_init_features failed" ); +return r; + } + +return 0 ; +} +int vhost_dev_init (struct vhost_dev *hdev,void *opaque,VhostBackendType backend_type,uint32_t busyloop_timeout,Error **errp)
@@ -1693,10 +1715,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, hdev->vdev =NULL ; hdev->migration_blocker =NULL ; - r =vhost_set_backend_type (hdev, backend_type); -assert (r >=0 ); - - r = hdev->vhost_ops->vhost_init (hdev, opaque, errp); + r =vhost_dev_init_backend (hdev, opaque, backend_type, errp);if (r <0 ) { goto fail; }
@@ -1707,12 +1726,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, goto fail; } - r =vhost_dev_init_features (hdev); -if (r <0 ) { -error_setg_errno (errp, -r,"vhost_init_features failed" ); - goto fail; - } - limit = hdev->vhost_ops->vhost_memslots_limit (hdev);if (limit <MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&memory_devices_memslot_auto_decision_active ()) {
include/hw/virtio/vhost.h +22 -0
@@ -156,6 +156,28 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,VhostBackendType backend_type, uint32_t busyloop_timeout, Error **errp); +/** +* vhost_dev_init_backend() - set up the backend and query its features +* @hdev: the common vhost_dev structure +* @opaque: opaque ptr passed to backend (vhost/vhost-user/vdpa) +* @backend_type: type of backend +* @errp: error handle +* +* Select the backend, initialise the backend instance and read its supported +* features into @hdev, without issuing VHOST_SET_OWNER, setting up the +* virtqueues or registering the memory listener. This is the part of +* vhost_dev_init() that precedes taking ownership; it can be used on its own +* so feature negotiation can happen before ownership is acquired (e.g. by CPR +* restore). +* +* On failure the backend may be left partially initialised; the caller must +* still call vhost_dev_cleanup() to release it. +* +* Return: 0 on success, non-zero on error while setting errp. +*/ +int vhost_dev_init_backend (struct vhost_dev *hdev,void *opaque, +VhostBackendType backend_type,Error **errp); +/** * vhost_dev_cleanup() - tear down and cleanup vhost interface * @hdev: the common vhost_dev structure
Signature · 1 line
2.47.1
PATCH v16 12/22 vhost: make vhost_dev_cleanup() safe on a partially initialized device
In reply to Andrey DrobyshevA device which only went through vhost_dev_init_backend() has its VQs not yet initialized, so vq->dev is NULL and vhost_virtqueue_cleanup() would crash dereferencing it. Check vq->dev before use, so that vhost_dev_cleanup() can be called to release a device whose full vhost_dev_init() never ran or failed along the way.
As of now, there's no way for the cleanup path to be reached with vq->dev uninitialized. However after a subsequent patch adding CPR support for vhost-vsock this'll become a possibility, so let's prevent such a crash beforehand.
--- hw/virtio/vhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/virtio/vhost.c +1 -1
@@ -1661,7 +1661,7 @@ fail_call:static void vhost_virtqueue_cleanup (struct vhost_virtqueue *vq) {event_notifier_cleanup (&vq->masked_notifier); -if (vq->dev->vhost_ops->vhost_set_vring_err) { +if (vq->dev && vq->dev->vhost_ops->vhost_set_vring_err) {event_notifier_set_handler (&vq->error_notifier,NULL );event_notifier_cleanup (&vq->error_notifier); }
Signature · 1 line
2.47.1
PATCH v16 13/22 vhost: add vhost_dev_set_owner() / vhost_dev_reset_owner() helpers
In reply to Andrey DrobyshevWrap the set_owner/reset_owner backend ops in dev-level helpers, matching other vhost_dev_* wrappers, so device code can take or release ownership without reaching into vhost_ops directly. vhost_dev_init() now uses vhost_dev_set_owner(). Both return -ENOSYS if the backend has no such op.
No functional change. These are used by the following vhost-vsock patch to hand a device between owners during CPR.
--- hw/virtio/vhost.c | 20 +++++++++++++++++++- include/hw/virtio/vhost.h | 25 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-)
hw/virtio/vhost.c +19 -1
@@ -1703,6 +1703,24 @@ int vhost_dev_init_backend(struct vhost_dev *hdev, void *opaque,return 0 ; } +int vhost_dev_set_owner (struct vhost_dev *hdev) +{ +assert (hdev->vhost_ops); +if (!hdev->vhost_ops->vhost_set_owner) { +return -ENOSYS ; + } +return hdev->vhost_ops->vhost_set_owner (hdev); +} + +int vhost_dev_reset_owner (struct vhost_dev *hdev) +{ +assert (hdev->vhost_ops); +if (!hdev->vhost_ops->vhost_reset_owner) { +return -ENOSYS ; + } +return hdev->vhost_ops->vhost_reset_owner (hdev); +} +int vhost_dev_init (struct vhost_dev *hdev,void *opaque,VhostBackendType backend_type,uint32_t busyloop_timeout,Error **errp)
@@ -1720,7 +1738,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, goto fail; } - r = hdev->vhost_ops->vhost_set_owner (hdev); + r =vhost_dev_set_owner (hdev);if (r <0 ) {error_setg_errno (errp, -r,"vhost_set_owner failed" ); goto fail;
include/hw/virtio/vhost.h +25 -0
@@ -178,6 +178,31 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,int vhost_dev_init_backend (struct vhost_dev *hdev,void *opaque,VhostBackendType backend_type,Error **errp); +/** +* vhost_dev_set_owner() - take ownership of a vhost device +* @hdev: the common vhost_dev structure +* +* Issue VHOST_SET_OWNER on a device that has already been set up, e.g. to +* complete a CPR handoff on the destination, or to take the device back on +* the source after a failed CPR. +* +* Return: 0 on success, -ENOSYS if the backend has no such op, negative +* errno on failure. +*/ +int vhost_dev_set_owner (struct vhost_dev *hdev); + +/** +* vhost_dev_reset_owner() - release ownership of a vhost device +* @hdev: the common vhost_dev structure +* +* Issue VHOST_RESET_OWNER so that another process can take the device over +* with vhost_dev_set_owner(), e.g. on the CPR source once the VM is stopped. +* +* Return: 0 on success, -ENOSYS if the backend has no such op, negative +* errno on failure. +*/ +int vhost_dev_reset_owner (struct vhost_dev *hdev); +/** * vhost_dev_cleanup() - tear down and cleanup vhost interface * @hdev: the common vhost_dev structure
Signature · 1 line
2.47.1
PATCH v16 14/22 vhost: add vhost_dev_is_initialized() helper
In reply to Andrey DrobyshevThe upcoming CPR support for vhost-vsock defers the full vhost_dev_init() to .post_load(). On an incoming CPR .realize() only sets up the backend, so we need a way to tell a fully initialized vhost device from a partially initialized one.
Add an explicit 'bool initialized' field, set once vhost_dev_init() succeeds, and cleared only in vhost_dev_cleanup() (by memset(0)). Also add a public vhost_dev_is_initialized() helper.
--- hw/virtio/vhost.c | 2 ++ include/hw/virtio/vhost.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)
hw/virtio/vhost.c +2 -0
@@ -1834,6 +1834,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, goto fail; } + hdev->initialized = true; +trace_vhost_dev_init_out (hdev);return 0 ;
include/hw/virtio/vhost.h +14 -0
@@ -109,6 +109,8 @@ struct vhost_dev {uint64_t backend_cap;/* @started: is the vhost device started? */ bool started; +/* @initialized: has the full vhost_dev_init() completed? */ +bool initialized;bool log_enabled;uint64_t log_size;Error *migration_blocker;
@@ -248,6 +250,18 @@ static inline bool vhost_dev_is_started(struct vhost_dev *hdev)return hdev->started; } +/** +* vhost_dev_is_initialized() - report init status of vhost device +* @hdev: common vhost_dev structure +* +* Return true if the full vhost_dev_init() has completed successfully +* (and vhost_dev_cleanup() has not run since). +*/ +static inline bool vhost_dev_is_initialized (struct vhost_dev *hdev) +{ +return hdev->initialized; +} +static inline int vhost_dev_set_vring_enable (struct vhost_dev *hdev,int enable) {if (!hdev->vhost_ops->vhost_set_vring_enable) {
Signature · 1 line
2.47.1
PATCH v16 15/22 vhost: release ownership in vhost_dev_cleanup()
In reply to Andrey Drobyshevvhost_dev_cleanup() closes the backend FD, but that doesn't release the device ownership taken with VHOST_SET_OWNER if the same file is open elsewhere. That's the case on a CPR target: the source keeps the FD, so if the target fails after SET_OWNER, either inside vhost_dev_init() or later in its caller, the ownership stays with the target. Once the source is resumed it can't take the device back, SET_OWNER fails with EBUSY.
Track the ownership with a new 'owner' flag in struct vhost_dev, set and cleared by vhost_dev_set_owner() and vhost_dev_reset_owner(), and issue VHOST_RESET_OWNER in vhost_dev_cleanup() as the common teardown for all those paths. Only do it if the ownership was handed over to us by another process, which is marked by the device with a new bool flag 'owner_handed_over'. Upon ownership release failure, we have nothing to unwind at this point (CPR target teardown), so we just report the failure.
--- hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++++++++++-- include/hw/virtio/vhost.h | 11 +++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-)
hw/virtio/vhost.c +38 -2
@@ -53,6 +53,25 @@ static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX]; static QLIST_HEAD(, vhost_dev) vhost_devices =QLIST_HEAD_INITIALIZER (vhost_devices); +/* +* Release the device ownership if it was handed over to us by another +* process, e.g. the CPR source. +*/ +static void vhost_dev_try_release_owner (struct vhost_dev *hdev, +const char *when) +{ +int r; + +if (!hdev->owner || !hdev->owner_handed_over) { +return ; + } + r =vhost_dev_reset_owner (hdev); +if (r <0 ) { +error_report ("vhost: cannot release device ownership on %s: %s" , + when,strerror (-r)); + } +} +unsigned int vhost_get_max_memslots (void ) {unsigned int max =UINT_MAX ;
@@ -1705,20 +1724,32 @@ int vhost_dev_init_backend(struct vhost_dev *hdev, void *opaque,int vhost_dev_set_owner (struct vhost_dev *hdev) { +int r; +assert (hdev->vhost_ops);if (!hdev->vhost_ops->vhost_set_owner) {return -ENOSYS ; } -return hdev->vhost_ops->vhost_set_owner (hdev); + r = hdev->vhost_ops->vhost_set_owner (hdev); +if (!r) { + hdev->owner = true; + } +return r; }int vhost_dev_reset_owner (struct vhost_dev *hdev) { +int r; +assert (hdev->vhost_ops);if (!hdev->vhost_ops->vhost_reset_owner) {return -ENOSYS ; } -return hdev->vhost_ops->vhost_reset_owner (hdev); + r = hdev->vhost_ops->vhost_reset_owner (hdev); +if (!r) { + hdev->owner = false; + } +return r; } int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
@@ -1864,6 +1895,11 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)g_free (hdev->mem);g_free (hdev->mem_sections);if (hdev->vhost_ops) { +/* +* Release the device ownership if we hold it. Needed e.g. for +* failing CPR target, so the source can re-acquire it. +*/ +vhost_dev_try_release_owner (hdev,"cleanup" ); hdev->vhost_ops->vhost_cleanup (hdev); }assert (!hdev->log);
include/hw/virtio/vhost.h +11 -0
@@ -111,6 +111,17 @@ struct vhost_dev {bool started;/* @initialized: has the full vhost_dev_init() completed? */ bool initialized; +/* +* @owner: has this vhost_dev acquired the device ownership? +* Updated only by vhost_dev_set_owner()/vhost_dev_reset_owner() +*/ +bool owner; +/* +* @owner_handed_over: CPR target side. The ownership was handed over +* to us by another process (the CPR source) and must be given back +* when this vhost_dev goes away. Set by the device. +*/ +bool owner_handed_over;bool log_enabled;uint64_t log_size;Error *migration_blocker;
Signature · 1 line
2.47.1
Re: [PATCH v16 15/22] vhost: release ownership in vhost_dev_cleanup()
Quoted 110 lines · Andrey Drobyshev
vhost_dev_cleanup()closes the backend FD, but that doesn't release the device ownership taken withVHOST_SET_OWNERif the same file is open elsewhere. That's the case on a CPR target: the source keeps the FD, so if the target fails afterSET_OWNER, either insidevhost_dev_init()or later in its caller, the ownership stays with the target. Once the source is resumed it can't take the device back,SET_OWNERfails with EBUSY.Track the ownership with a new 'owner' flag in struct
vhost_dev, set and cleared byvhost_dev_set_owner()andvhost_dev_reset_owner(), and issueVHOST_RESET_OWNERinvhost_dev_cleanup()as the common teardown for all those paths. Only do it if the ownership was handed over to us by another process, which is marked by the device with a new bool flag 'owner_handed_over'. Upon ownership release failure, we have nothing to unwind at this point (CPR target teardown), so we just report the failure.Reported-by Akihiko Odaki <[email protected]>Suggested-by Stefano Garzarella <[email protected]>Signed-off-by Andrey Drobyshev <[email protected]>---
hw/virtio/vhost.c| 40 ++++++++++++++++++++++++++++++++++++++--include/hw/virtio/vhost.h| 11 +++++++++++2 files changed, 49 insertions(+), 2 deletions(-)hw/virtio/vhost.c +0
-0@@ -53,6 +53,25 @@ static QLIST_HEAD(, vhost_dev) vhost_log_devs[VHOST_BACKEND_TYPE_MAX];static
QLIST_HEAD(, vhost_dev)vhost_devices=QLIST_HEAD_INITIALIZER(vhost_devices);hw/virtio/vhost.c +19
-0+/* +* Release the device ownership if it was handed over to us by another +* process, e.g. the CPR source. +*/ +static void vhost_dev_try_release_owner (struct vhost_dev *hdev, +const char *when) +{ +int r; + +if (!hdev->owner || !hdev->owner_handed_over) { +return ; + } + r =vhost_dev_reset_owner (hdev); +if (r <0 ) { +error_report ("vhost: cannot release device ownership on %s: %s" , + when,strerror (-r)); + } +} +unsigned int
vhost_get_max_memslots(void){ unsigned int max = UINT_MAX;hw/virtio/vhost.c +19
-2@@ -1705,20 +1724,32 @@ int vhost_dev_init_backend(struct vhost_dev *hdev, void *opaque,int vhost_dev_set_owner (struct vhost_dev *hdev) { +int r; +assert (hdev->vhost_ops);if (!hdev->vhost_ops->vhost_set_owner) {return -ENOSYS ; } -return hdev->vhost_ops->vhost_set_owner (hdev); + r = hdev->vhost_ops->vhost_set_owner (hdev); +if (!r) { + hdev->owner = true; + } +return r; }int vhost_dev_reset_owner (struct vhost_dev *hdev) { +int r; +assert (hdev->vhost_ops);if (!hdev->vhost_ops->vhost_reset_owner) {return -ENOSYS ; } -return hdev->vhost_ops->vhost_reset_owner (hdev); + r = hdev->vhost_ops->vhost_reset_owner (hdev); +if (!r) { + hdev->owner = false; + } +return r; } int vhost_dev_init(struct vhost_dev *hdev, void *opaque,@@ -1864,6 +1895,11 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)g_free (hdev->mem);g_free (hdev->mem_sections);if (hdev->vhost_ops) { +/* +* Release the device ownership if we hold it. Needed e.g. for +* failing CPR target, so the source can re-acquire it. +*/ +vhost_dev_try_release_owner (hdev,"cleanup" ); hdev->vhost_ops->vhost_cleanup (hdev); }assert (!hdev->log);include/hw/virtio/vhost.h +1
-0@@ -111,6 +111,17 @@ struct vhost_dev {bool started;/* @initialized: has the full vhost_dev_init() completed? */ bool initialized; +/*
include/hw/virtio/vhost.h struct vhost_dev { · line 124
Quoted 10 lines
include/hw/virtio/vhost.h +10
-0+* @owner: has this vhost_dev acquired the device ownership? +* Updated only by vhost_dev_set_owner()/vhost_dev_reset_owner() +*/ +bool owner; +/* +* @owner_handed_over: CPR target side. The ownership was handed over +* to us by another process (the CPR source) and must be given back +* when this vhost_dev goes away. Set by the device. +*/ +bool owner_handed_over;
I find the name a bit hard to get, but I don't have better suggestions.
Anyway:
Quoted 7 lines
include/hw/virtio/vhost.h +0
-0bool log_enabled;uint64_t log_size;Error *migration_blocker;Signature · 2 lines
-- 2.47.1
PATCH v16 16/22 migration: add MIG_EVENT_INCOMING_FAILED
In reply to Andrey DrobyshevA device may take over a resource from the source in post_load(), e.g. vhost-vsock acquires the device ownership on a CPR target. If the incoming migration fails afterwards, there's currently no way for the device to give that resource back before the failure is published, i.e. before the source is told about it over the return path and resumes, or the management sees the MIGRATION event on the target and resumes the source, or the target exits.
Add a migration event for that, sent to the migration notifiers from the failure path of process_incoming_migration_co() before the failure is published.
--- include/migration/misc.h | 5 +++++ migration/migration.c | 10 ++++++++++ 2 files changed, 15 insertions(+)
include/migration/misc.h +5 -0
@@ -65,12 +65,17 @@ bool migration_thread_is_self(void);* - MIG_EVENT_SETUP [-> MIG_EVENT_POSTCOPY_START] -> MIG_EVENT_DONE * - MIG_EVENT_SETUP [-> MIG_EVENT_POSTCOPY_START] -> MIG_EVENT_FAILED * - MIG_EVENT_FAILED +* +* MIG_EVENT_INCOMING_FAILED is currently the only event sent on the +* incoming side, when the incoming migration fails and before the failure +* is published. */ typedef enum MigrationEventType {MIG_EVENT_SETUP ,MIG_EVENT_POSTCOPY_START ,MIG_EVENT_DONE ,MIG_EVENT_FAILED , +MIG_EVENT_INCOMING_FAILED ,MIG_EVENT_MAX }MigrationEventType ;
migration/migration.c +10 -0
@@ -815,6 +815,16 @@ process_incoming_migration_co(void *opaque) goto out; fail: +/* +* Allow the devices which have registered their callbacks for +* migration target failure to undo the changes they may have +* done in post_load(). +* +* Do that before the failure is published in any way: the MIGRATION +* event emitted by migrate_set_state() below, the return path (if +* present) in migration_incoming_state_destroy(), or the exit(). +*/ +migration_call_notifiers (MIG_EVENT_INCOMING_FAILED ,NULL );migrate_set_state (&mis->state,MIGRATION_STATUS_ACTIVE ,MIGRATION_STATUS_FAILED );migrate_error_propagate (s, local_err);
Signature · 1 line
2.47.1
PATCH v16 17/22 vhost: release ownership when the incoming migration fails
In reply to Andrey DrobyshevA CPR target that has taken the device ownership in post_load() doesn't always reach vhost_dev_cleanup(): a subsequent load error, e.g. a truncated migration stream footer, makes QEMU exit() right away, and the ownership stays with the dead process. The source still holds the FD, so the kernel never releases the device, and the source can't take it back after being resumed -> SET_OWNER fails with EBUSY.
Releasing the ownership at exit isn't enough either: with the return-path capability the target reports the failure to the source before exiting, and the source resumes right away, racing with the target's exit for the ownership. And with exit-on-error=false the target doesn't exit at all.
Add a migration notifier to each vhost_dev, registered for its lifetime for the CPR modes, that releases the ownership if still held on MIG_EVENT_INCOMING_FAILED. It runs before the failure is published in any way.
--- hw/virtio/vhost.c | 23 +++++++++++++++++++++++ include/hw/virtio/vhost.h | 6 ++++++ 2 files changed, 29 insertions(+)
hw/virtio/vhost.c +23 -0
@@ -26,6 +26,7 @@#include "hw/virtio/vhost-user.h" #include "hw/mem/memory-device.h" #include "migration/blocker.h" +#include "migration/misc.h" #include "migration/qemu-file-types.h" #include "system/dma.h" #include "system/memory.h"
@@ -72,6 +73,23 @@ static void vhost_dev_try_release_owner(struct vhost_dev *hdev, } } +/* +* A CPR target takes the device ownership in post_load(). If the load +* fails afterwards, release it before the source learns about the failure +* and resumes, so that the source can take the device back. +*/ +static int vhost_migration_notify (NotifierWithReturn *notifier, +MigrationEvent *e,Error **errp) +{ + struct vhost_dev *hdev = container_of(notifier, struct vhost_dev, + migration_notifier); + +if (e->type ==MIG_EVENT_INCOMING_FAILED ) { +vhost_dev_try_release_owner (hdev,"incoming migration failure" ); + } +return 0 ; +} +unsigned int vhost_get_max_memslots (void ) {unsigned int max =UINT_MAX ;
@@ -1842,6 +1860,10 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, hdev->log_enabled = false; hdev->started = false;memory_listener_register (&hdev->memory_listener, &address_space_memory); +migration_add_notifier_modes (&hdev->migration_notifier, + vhost_migration_notify, +BIT (MIG_MODE_CPR_TRANSFER ) | +BIT (MIG_MODE_CPR_EXEC ));QLIST_INSERT_HEAD (&vhost_devices, hdev, entry);/*
@@ -1891,6 +1913,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)memory_listener_unregister (&hdev->memory_listener);QLIST_REMOVE (hdev, entry); } +migration_remove_notifier (&hdev->migration_notifier);migrate_del_blocker (&hdev->migration_blocker);g_free (hdev->mem);g_free (hdev->mem_sections);
include/hw/virtio/vhost.h +6 -0
@@ -3,6 +3,7 @@#include "net/vhost_net.h" #include "hw/virtio/vhost-backend.h" +#include "qemu/notify.h" #include "hw/virtio/virtio.h" #include "system/memory.h"
@@ -122,6 +123,11 @@ struct vhost_dev {* when this vhost_dev goes away. Set by the device. */ bool owner_handed_over; +/* +* @migration_notifier: releases the ownership when an incoming +* migration fails, vhost_migration_notify() +*/ +NotifierWithReturn migration_notifier;bool log_enabled;uint64_t log_size;Error *migration_blocker;
Signature · 1 line
2.47.1
Re: [PATCH v16 17/22] vhost: release ownership when the incoming migration fails
Quoted 18 lines · Andrey Drobyshev
A CPR target that has taken the device ownership in
post_load()doesn't always reachvhost_dev_cleanup(): a subsequent load error, e.g. a truncated migration stream footer, makes QEMUexit()right away, and the ownership stays with the dead process. The source still holds the FD, so the kernel never releases the device, and the source can't take it back after being resumed ->SET_OWNERfails with EBUSY.Releasing the ownership at exit isn't enough either: with the return-path capability the target reports the failure to the source before exiting, and the source resumes right away, racing with the target's exit for the ownership. And with exit-on-error=false the target doesn't exit at all.
Add a migration notifier to each
vhost_dev, registered for its lifetime for the CPR modes, that releases the ownership if still held onMIG_EVENT_INCOMING_FAILED. It runs before the failure is published in any way.
Quoted 6 lines
Reported-by Akihiko Odaki <[email protected]>Signed-off-by Andrey Drobyshev <[email protected]>---
hw/virtio/vhost.c| 23 +++++++++++++++++++++++include/hw/virtio/vhost.h| 6 ++++++2 files changed, 29 insertions(+)
PATCH v16 18/22 vhost: release ownership on exit
In reply to Andrey DrobyshevA CPR target that has taken the device ownership in post_load() doesn't always reach vhost_dev_cleanup(): a target which has loaded the state but hasn't been started yet, e.g. one started with -S, may be quit normally, with the source being resumed instead. Devices aren't unrealized on quit, so the ownership stays with the dead process. The source still holds the FD, so the kernel never releases the device, and the source can't take it back after being resumed -> SET_OWNER fails with EBUSY.
Add an exit notifier to each vhost_dev, registered for its lifetime, that releases the ownership if still held. This covers every orderly exit. A target killed with SIGKILL or crashing still leaves the device unrecoverable.
--- hw/virtio/vhost.c | 20 ++++++++++++++++++++ include/hw/virtio/vhost.h | 2 ++ 2 files changed, 22 insertions(+)
hw/virtio/vhost.c +20 -0
@@ -31,6 +31,7 @@#include "system/dma.h" #include "system/memory.h" #include "system/ramblock.h" +#include "system/system.h" #include "trace.h" /* enabled until disconnected backend stabilizes */
@@ -90,6 +91,19 @@ static int vhost_migration_notify(NotifierWithReturn *notifier,return 0 ; } +/* +* Release the ownership of the vhost devices still held at exit(), so that +* another process sharing the backend FDs (e.g. the CPR source) can take +* them over. +*/ +static void vhost_exit_notify (Notifier *notifier,void *data) +{ + struct vhost_dev *hdev = container_of(notifier, struct vhost_dev, + exit_notifier); + +vhost_dev_try_release_owner (hdev,"exit" ); +} +unsigned int vhost_get_max_memslots (void ) {unsigned int max =UINT_MAX ;
@@ -1860,6 +1874,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, hdev->log_enabled = false; hdev->started = false;memory_listener_register (&hdev->memory_listener, &address_space_memory); + hdev->exit_notifier.notify = vhost_exit_notify; +qemu_add_exit_notifier (&hdev->exit_notifier); migration_add_notifier_modes(&hdev->migration_notifier, vhost_migration_notify, BIT(MIG_MODE_CPR_TRANSFER) |
@@ -1913,6 +1929,10 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)memory_listener_unregister (&hdev->memory_listener);QLIST_REMOVE (hdev, entry); } +if (hdev->exit_notifier.notify) { +qemu_remove_exit_notifier (&hdev->exit_notifier); + hdev->exit_notifier.notify =NULL ; + }migration_remove_notifier (&hdev->migration_notifier);migrate_del_blocker (&hdev->migration_blocker);g_free (hdev->mem);
include/hw/virtio/vhost.h +2 -0
@@ -123,6 +123,8 @@ struct vhost_dev {* when this vhost_dev goes away. Set by the device. */ bool owner_handed_over; +/* @exit_notifier: releases the ownership at exit, vhost_exit_notify() */ +Notifier exit_notifier;/* * @migration_notifier: releases the ownership when an incoming * migration fails, vhost_migration_notify()
Signature · 1 line
2.47.1
Re: [PATCH v16 18/22] vhost: release ownership on exit
Quoted 14 lines · Andrey Drobyshev
A CPR target that has taken the device ownership in
post_load()doesn't always reachvhost_dev_cleanup(): a target which has loaded the state but hasn't been started yet, e.g. one started with -S, may be quit normally, with the source being resumed instead. Devices aren't unrealized on quit, so the ownership stays with the dead process. The source still holds the FD, so the kernel never releases the device, and the source can't take it back after being resumed ->SET_OWNERfails with EBUSY.Add an exit notifier to each
vhost_dev, registered for its lifetime, that releases the ownership if still held. This covers every orderly exit. A target killed with SIGKILL or crashing still leaves the device unrecoverable.
Quoted 6 lines
Reported-by Akihiko Odaki <[email protected]>Signed-off-by Andrey Drobyshev <[email protected]>---
hw/virtio/vhost.c| 20 ++++++++++++++++++++include/hw/virtio/vhost.h| 2 ++2 files changed, 22 insertions(+)
PATCH v16 19/22 vhost-vsock: hand off device ownership across CPR
In reply to Andrey DrobyshevThe previous patches reuse the source vhost FD on the destination, however both source and target still call vhost_dev_init() (VHOST_SET_OWNER) in realize(). For cpr-transfer the destination realizes while the source still owns the shared FD, so its SET_OWNER would fail - ownership has to be handed over explicitly.
Do this through the device's CPR vmstate hooks. Namely, release device ownership in pre_save, reclaim in post_load, re-acquire on restart:
- .pre_save() releases ownership on the source (VHOST_RESET_OWNER) once the VM is stopped, for the FD-preserving CPR modes (cpr-transfer and cpr-exec).
- .realize(), for an incoming CPR, only sets up the virtio device and queries the backend features by calling vhost_dev_init_backend(). It doesn't take device ownership and doesn't touch the VQs which still-running source might use. The full init is deferred to .post_load().
- .post_load() reclaims it on the destination: the full vhost_dev_init() (VHOST_SET_OWNER) on the preserved FD, plus sets the guest cid, before the device is started at vm_start. It also marks the ownership as handed over ('owner_handed_over'), so that it's released if this target goes away.
- .set_status() refuses to start a device whose init hasn't completed yet (incoming CPR before post_load).
- .set_status() re-acquires ownership before starting the device if pre_save released it and this QEMU is about to run the guest again:
the migration failed, or it completed and the management resumed the source (e.g. cpr-transfer target died).
With the handoff in place, lift the CPR blocker.
Note: this requires kernel side support of VHOST_RESET_OWNER operation for vhost-vsock. Otherwise CPR attempt fails in .pre_save().
--- hw/virtio/vhost-vsock.c | 157 ++++++++++++++++++++++++++++++++-------- include/hw/virtio/vhost-vsock.h | 1 - 2 files changed, 127 insertions(+), 31 deletions(-)
hw/virtio/vhost-vsock.c +127 -30
@@ -21,7 +21,6 @@#include "hw/virtio/vhost-vsock.h" #include "monitor/monitor.h" #include "migration/cpr.h" -#include "migration/blocker.h" #include "migration/misc.h" static void vhost_vsock_get_config (VirtIODevice *vdev,uint8_t *config)
@@ -88,11 +87,40 @@ static int vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)bool should_start =virtio_device_should_start (vdev, status);int ret; +/* +* During CPR, on target the full vhost_dev_init() is deferred to +* post_load. Refuse to start a half-initialised device rather than +* issue vhost ioctls on it. +*/ +if (should_start && !vhost_dev_is_initialized (&vvc->vhost_dev)) { +error_report ("vhost-vsock: refusing to start, device init incomplete" ); +return 0 ; + } +if (vhost_dev_is_started (&vvc->vhost_dev) == should_start) {return 0 ; }if (should_start) { +/* +* The ownership is released in .pre_save() on CPR migration +* source. If we end up here without it - that means that either +* CPR migration failed, or we got 'cont' cmd from the management +* (e.g. cpr-transfer target died). In either case, we need to +* re-acquire device ownership. +* +* On failure leave the device stopped. The next start attempt +* (guest driver reset, VM stop/cont) retries. +*/ +if (!vvc->vhost_dev.owner) { + ret =vhost_dev_set_owner (&vvc->vhost_dev); +if (ret <0 ) { +error_report ("vhost-vsock: cannot re-acquire device " +"ownership: %s" ,strerror (-ret)); +return 0 ; + } + } + ret =vhost_vsock_common_start (vdev);if (ret <0 ) {return 0 ;
@@ -123,8 +151,42 @@ static uint64_t vhost_vsock_get_features(VirtIODevice *vdev,return vhost_vsock_common_get_features (vdev, requested_features, errp); } +static int vhost_vsock_pre_save (void *opaque) +{ +VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (opaque); +int ret; + + ret =vhost_vsock_common_pre_save (opaque); +if (ret) { +return ret; + } + +/* +* Release the device ownership now for CPR migration. The device is +* already stopped at pre_save, and destination reclaims it by calling +* VHOST_SET_OWNER in post_load. +*/ +if (cpr_incoming_needed (NULL ) &&migration_is_running () && + vvc->vhost_dev.owner) { + ret =vhost_dev_reset_owner (&vvc->vhost_dev); +if (ret <0 ) { +error_report ("vhost-vsock: vhost_reset_owner failed: %s" , +strerror (-ret)); +return ret; + } + } + +return 0 ; +} +static int vhost_vsock_post_load (void *opaque,int version_id) { +VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (opaque); +VirtIODevice *vdev =VIRTIO_DEVICE (opaque); + g_autofree char *cpr_name = vhost_vsock_cpr_name(DEVICE(opaque)); +Error *local_err =NULL ; +int vhostfd, ret; +/* * Only reset vsock connections for non-CPR migration. For CPR the * guest cid is unchanged, and the cid-change reset would otherwise
@@ -134,6 +196,40 @@ static int vhost_vsock_post_load(void *opaque, int version_id)return vhost_vsock_common_post_load (opaque, version_id); } +/* +* CPR restore case. The source released device ownership in its +* pre_save. Complete the handoff here, before the device is started +* at vm_start. Init vhost device on preserved FD, issue +* VHOST_SET_OWNER on it, and restore the guest cid. +*/ + vhostfd =cpr_find_fd (cpr_name,0 ); +if (vhostfd <0 ) { +error_report ("vhost-vsock: could not find restored vhost FD" ); +return -1 ; + } + +/* +* The source has released the ownership on this FD for us, so give it +* back if this target goes away. +*/ + vvc->vhost_dev.owner_handed_over = true; + ret =vhost_dev_init (&vvc->vhost_dev, (void *)(uintptr_t )vhostfd, +VHOST_BACKEND_TYPE_KERNEL ,0 , &local_err); +if (ret <0 ) { +error_report_err (local_err); +cpr_delete_fd (cpr_name,0 ); +return ret; + } + + ret =vhost_vsock_set_guest_cid (vdev); +if (ret <0 ) { +error_report ("vhost-vsock: unable to set guest cid: %s" , +strerror (-ret)); +vhost_dev_cleanup (&vvc->vhost_dev); +cpr_delete_fd (cpr_name,0 ); +return ret; + } +return 0 ; }
@@ -145,7 +241,7 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = { VMSTATE_VIRTIO_DEVICE,VMSTATE_END_OF_LIST () }, - .pre_save = vhost_vsock_common_pre_save, + .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load, };
@@ -171,37 +267,24 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)return ; } -/* -* CPR migration of a vhost-vsock device is not supported yet: the -* device ownership is not handed over, so the target fails to set -* up its device. Fail the migration early and gracefully instead. -*/ -error_setg (&vsock->migration_blocker, -"vhost-vsock: CPR migration is not supported" ); -if (migrate_add_blocker_modes (&vsock->migration_blocker, -BIT (MIG_MODE_CPR_TRANSFER ) | -BIT (MIG_MODE_CPR_EXEC ), errp) <0 ) { -return ; - } -if (cpr_incoming) {/* Reuse the fd handed over from the source QEMU */ vhostfd =cpr_find_fd (cpr_name,0 );if (vhostfd <0 ) {error_setg (errp,"vhost-vsock: could not find restored vhost FD" ); - goto err_blocker; +return ; } }else if (vsock->conf.vhostfd) { vhostfd =monitor_fd_param (monitor_cur (), vsock->conf.vhostfd, errp);if (vhostfd ==-1 ) {error_prepend (errp,"vhost-vsock: unable to parse vhostfd: " ); - goto err_blocker; +return ; } }else { vhostfd =open ("/dev/vhost-vsock" ,O_RDWR );if (vhostfd <0 ) {error_setg_file_open (errp, errno,"/dev/vhost-vsock" ); - goto err_blocker; +return ; } }
@@ -210,19 +293,37 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)if (cpr_incoming) {cpr_delete_fd (cpr_name,0 ); } - goto err_blocker; +return ; }vhost_vsock_common_realize (vdev); - ret =vhost_dev_init (&vvc->vhost_dev, (void *)(uintptr_t )vhostfd, -VHOST_BACKEND_TYPE_KERNEL ,0 , errp); -if (ret <0 ) { +if (!cpr_incoming) { + ret =vhost_dev_init (&vvc->vhost_dev, (void *)(uintptr_t )vhostfd, +VHOST_BACKEND_TYPE_KERNEL ,0 , errp); +if (ret <0 ) { +/* +* vhostfd is closed by vhost_dev_cleanup, which is called +* by vhost_dev_init on initialization error. +*/ + goto err_virtio; + } + }else {/* -* vhostfd is closed by vhost_dev_cleanup, which is called -* by vhost_dev_init on initialization error. +* CPR restore case: only learn the backend feature set now, but +* defer taking ownership or touching VQs (the still-running source +* might be using them). The full vhost_dev_init()/VHOST_SET_OWNER +* is done later in post_load. */ - goto err_virtio; + ret =vhost_dev_init_backend (&vvc->vhost_dev, + (void *)(uintptr_t )vhostfd, +VHOST_BACKEND_TYPE_KERNEL , errp); +if (ret <0 ) { +/* vhost_dev_init_backend() does not close the fd on error */ + goto err_vhost_dev; + } + +return ; } ret =vhost_vsock_set_guest_cid (vdev);
@@ -232,7 +333,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp) }/* Register the fd for a future CPR after a fully successful realize */ -if (!cpr_incoming && !cpr_save_fd (cpr_name,0 , vhostfd, errp)) { +if (!cpr_save_fd (cpr_name,0 , vhostfd, errp)) { goto err_vhost_dev; }
@@ -247,22 +348,18 @@ err_vhost_dev: } err_virtio:vhost_vsock_common_unrealize (vdev); -err_blocker: -migrate_del_blocker (&vsock->migration_blocker); }static void vhost_vsock_device_unrealize (DeviceState *dev) {VHostVSockCommon *vvc =VHOST_VSOCK_COMMON (dev);VirtIODevice *vdev =VIRTIO_DEVICE (dev); -VHostVSock *vsock =VHOST_VSOCK (dev); g_autofree char *cpr_name = vhost_vsock_cpr_name(dev);/* This will stop vhost backend if appropriate. */ vhost_vsock_set_status (vdev,0 );cpr_delete_fd (cpr_name,0 ); -migrate_del_blocker (&vsock->migration_blocker);vhost_dev_cleanup (&vvc->vhost_dev);vhost_vsock_common_unrealize (vdev); }
include/hw/virtio/vhost-vsock.h +0 -1
@@ -29,7 +29,6 @@ struct VHostVSock {/*< private >*/ VHostVSockCommon parent;VHostVSockConf conf; -Error *migration_blocker;/* CPR migration is not supported */ /*< public >*/ };
Signature · 1 line
2.47.1
Re: [PATCH v16 19/22] vhost-vsock: hand off device ownership across CPR
Quoted 38 lines · Andrey Drobyshev
The previous patches reuse the source vhost FD on the destination, however both source and target still call
vhost_dev_init()(VHOST_SET_OWNER) inrealize(). For cpr-transfer the destination realizes while the source still owns the shared FD, so itsSET_OWNERwould fail - ownership has to be handed over explicitly.Do this through the device's CPR vmstate hooks. Namely, release device ownership in
pre_save, reclaim inpost_load, re-acquire on restart:- .pre_save() releases ownership on the source (
VHOST_RESET_OWNER) once the VM is stopped, for the FD-preserving CPR modes (cpr-transfer and cpr-exec).- .realize(), for an incoming CPR, only sets up the virtio device and queries the backend features by calling
vhost_dev_init_backend(). It doesn't take device ownership and doesn't touch the VQs which still-running source might use. The full init is deferred to .post_load().- .post_load() reclaims it on the destination: the full
vhost_dev_init()(VHOST_SET_OWNER) on the preserved FD, plus sets the guest cid, before the device is started atvm_start. It also marks the ownership as handed over ('owner_handed_over'), so that it's released if this target goes away.- .set_status() refuses to start a device whose init hasn't completed yet (incoming CPR before
post_load).- .set_status() re-acquires ownership before starting the device if
pre_savereleased it and this QEMU is about to run the guest again:
the migration failed, or it completed and the management resumed the source (e.g. cpr-transfer target died).With the handoff in place, lift the CPR blocker.
Note: this requires kernel side support of
VHOST_RESET_OWNERoperation for vhost-vsock. Otherwise CPR attempt fails in .pre_save().
Quoted 6 lines
Suggested-by Dongli Zhang <[email protected]>Signed-off-by Andrey Drobyshev <[email protected]>---
hw/virtio/vhost-vsock.c| 157 ++++++++++++++++++++++++++++++++--------include/hw/virtio/vhost-vsock.h| 1 -2 files changed, 127 insertions(+), 31 deletions(-)
Overall LGTM:
PATCH v16 20/22 tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec
In reply to Andrey DrobyshevRun the existing cpr-transfer and cpr-exec tests with a vhost-vsock-pci device attached. There's no guest involved which is driving the device, so here we only test the QEMU side of the handoff, i.e. the vhost FD is preserved, the device ownership is released on the source and re-acquired on the target.
The tests need /dev/vhost-vsock on the host and a kernel supporting VHOST_RESET_OWNER for vhost-vsock, they are skipped otherwise.
--- tests/qtest/migration/cpr-tests.c | 93 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 87 insertions(+), 6 deletions(-)
tests/qtest/migration/cpr-tests.c +87 -6
@@ -19,6 +19,13 @@#include "qobject/qjson.h" #include "qobject/qlist.h" +#ifdef CONFIG_LINUX +#include <sys/ioctl.h> +#include <linux/vhost.h> +#endif + +/* We expect this CID to be unassigned on the host */ +#define VHOST_VSOCK_TEST_DEV "vhost-vsock-pci,guest-cid=4000000000"static char *tmpfs;
@@ -101,14 +108,17 @@ static int test_transfer(MigrateCommon *args, const char *cpr_channel,* migration, and cannot connect synchronously to the monitor, so defer * the target connection. */ -static void test_mode_transfer_common (MigrateCommon *args,bool incoming_defer) +static void test_mode_transfer_common (MigrateCommon *args,bool incoming_defer, +const char *dev_opts) { g_autofree char *cpr_path = g_strdup_printf("%s/cpr.sock", tmpfs); g_autofree char *mig_path = g_strdup_printf("%s/migsocket", tmpfs); g_autofree char *uri = g_strdup_printf("unix:%s", mig_path); g_autofree char *opts_target = NULL; -const char *opts ="-machine aux-ram-share=on -nodefaults" ; + g_autofree char *opts = g_strdup_printf("-machine aux-ram-share=on " +"-nodefaults %s" , + dev_opts ? dev_opts :"" ); g_autofree const char *cpr_channel = g_strdup_printf("cpr,addr.transport=socket,addr.type=unix,addr.path=%s" , cpr_path);
@@ -154,12 +164,12 @@ static void test_mode_transfer_common(MigrateCommon *args, bool incoming_defer)static void test_mode_transfer (char *name,MigrateCommon *args) { -test_mode_transfer_common (args, false); +test_mode_transfer_common (args, false,NULL ); }static void test_mode_transfer_defer (char *name,MigrateCommon *args) { -test_mode_transfer_common (args, true); +test_mode_transfer_common (args, true,NULL ); }static void set_cpr_exec_args (QTestState *who,MigrateCommon *args)
@@ -276,20 +286,85 @@ static void *test_mode_exec_start(QTestState *from, QTestState *to)return NULL ; } -static void test_mode_exec (char *name,MigrateCommon *args) +static void test_mode_exec_common (MigrateCommon *args,const char *dev_opts) { g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,FILE_TEST_FILENAME ); + g_autofree char *opts = g_strdup_printf("-machine aux-ram-share=on " +"-nodefaults %s" , + dev_opts ? dev_opts :"" ); args->uri = uri; args->start_hook = test_mode_exec_start; args->start.only_source = true; - args->start.opts_source ="-machine aux-ram-share=on -nodefaults" ; + args->start.opts_source = opts; args->start.mem_type =MEM_TYPE_MEMFD ;test_cpr_exec (args); } +static void test_mode_exec (char *name,MigrateCommon *args) +{ +test_mode_exec_common (args,NULL ); +} + +/* +* A vhost-vsock device can be handed over across CPR only if the host +* exposes /dev/vhost-vsock and its kernel supports VHOST_RESET_OWNER +* for it. Skip the test otherwise. +*/ +static bool vhost_vsock_cpr_check (void ) +{ +#ifdef CONFIG_LINUX +bool set_owner, reset_owner; +int fd; + +if (!qtest_has_device ("vhost-vsock-pci" )) { +g_test_skip ("vhost-vsock-pci device is not available" ); +return false; + } + + fd =open ("/dev/vhost-vsock" ,O_RDWR ); +if (fd <0 ) { +g_test_skip ("/dev/vhost-vsock is not available" ); +return false; + } + + set_owner =ioctl (fd,VHOST_SET_OWNER ) ==0 ; + reset_owner =ioctl (fd,VHOST_RESET_OWNER ) ==0 ; +close (fd); + +if (!set_owner) { +g_test_skip ("VHOST_SET_OWNER is not supported for vhost-vsock" ); +return false; + } + +if (!reset_owner) { +g_test_skip ("VHOST_RESET_OWNER is not supported for vhost-vsock" ); +return false; + } + +return true; +#else +g_test_skip ("vhost-vsock is Linux only" ); +return false; +#endif +} + +static void test_mode_transfer_vhost_vsock (char *name,MigrateCommon *args) +{ +if (vhost_vsock_cpr_check ()) { +test_mode_transfer_common (args, false,"-device " VHOST_VSOCK_TEST_DEV ); + } +} + +static void test_mode_exec_vhost_vsock (char *name,MigrateCommon *args) +{ +if (vhost_vsock_cpr_check ()) { +test_mode_exec_common (args,"-device " VHOST_VSOCK_TEST_DEV ); + } +} +/* Snapshots must be refused while a CPR migration mode is set */ static void test_mode_no_snapshot (char *name,MigrateCommon *args) {
@@ -354,4 +429,10 @@ void migration_test_add_cpr(MigrationTestEnv *env) test_mode_transfer_defer);migration_test_add ("/migration/mode/exec" , test_mode_exec); } + +/* The vhost-vsock handoff doesn't need KVM */ +migration_test_add ("/migration/mode/transfer/vhost-vsock" , + test_mode_transfer_vhost_vsock); +migration_test_add ("/migration/mode/exec/vhost-vsock" , + test_mode_exec_vhost_vsock); }
Signature · 1 line
2.47.1
PATCH v16 21/22 tests/functional: add vhost-vsock CPR test with a live guest
In reply to Andrey DrobyshevThe qtests only cover the QEMU side of the vhost-vsock handoff. Add a functional test which runs an actual host<->guest vsock transfer across a series of CPR migrations, for both cpr-transfer and cpr-exec:
- the guest is booted on top of a qcow2 overlay shared by all the QEMU
instances it goes through;
- the host sends a 512M file of random data to the guest with socat, where it's stored on disk, then gets it back the same way; a CPR migration is performed every 0.2s while a transfer is in progress;
- with cpr-transfer each migration starts a fresh target QEMU and retires the source; with cpr-exec the same process is re-exec'ed over and over;
- the file received back must be identical to the one sent, and both socat instances in the guest must exit cleanly.
The test needs KVM, socat and /dev/vhost-vsock on the host, and a kernel supporting VHOST_RESET_OWNER for vhost-vsock. If any of those requirements are missing the test is skipped.
--- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 245 ++++++++++++++++++++++++ 2 files changed, 247 insertions(+)
tests/functional/x86_64/meson.build +2 -0
@@ -7,6 +7,7 @@ test_x86_64_timeouts = { 'netdev_ethtool' : 180, 'replay' : 480, 'virtio_balloon': 120, + 'vhost_vsock_cpr': 300, } tests_x86_64_system_quick = [
@@ -51,4 +52,5 @@ tests_x86_64_system_thorough = [ 'virtio_balloon', 'virtio_gpu', 'rebuild_vmfd', + 'vhost_vsock_cpr', ]
tests/functional/x86_64/test_vhost_vsock_cpr.py new file +245 -0
245 lines not on this page · raw message
Signature · 1 line
2.47.1
PATCH v16 22/22 tests/functional: cover a cpr-transfer target going away with vhost-vsock
In reply to Andrey DrobyshevA cpr-transfer target takes the vsock device over in post_load(). Make sure the source gets it back when the target goes away afterwards. Three cases, in the first two the target is made to fail the load by lacking a device the source has:
- the target exits on the failure (default exit-on-error); the source is stopped beforehand, so it stays paused until we 'cont' it once the target is gone;
- the target stays alive (exit-on-error=false) and the source resumes
on its own (return-path capability);
- the target loads the state while the source is stopped, stays paused and is quit normally; the source is resumed instead.
In all cases a host<->guest transfer must then work on the source.
The transfer helpers get a payload size parameter, since the plain checks don't need the 512M of the stress cases, and the target start code is moved into a helper shared with the existing cpr-transfer test.
--- tests/functional/x86_64/test_vhost_vsock_cpr.py | 178 +++++++++++++++++++----- 1 file changed, 140 insertions(+), 38 deletions(-)
tests/functional/x86_64/test_vhost_vsock_cpr.py +140 -38
@@ -24,6 +24,7 @@ VSOCK_PORT = 5000 TRANSFER_SIZE = 512 * 1024 * 1024 # file size for host<->guest transfer +CHECK_SIZE = 16 * 1024 * 1024 # enough to just check that vsock works CPR_INTERVAL = 0.2 # interval between CPR ops VHOST_SET_OWNER = 0xAF01 # _IO(VHOST_VIRTIO, 0x01)
@@ -71,11 +72,6 @@ def setUp(self): 'qcow2', '-b', self.ASSET_DISKIMAGE.fetch(), '-F', 'qcow2', self.disk]) - self.payload = self.scratch_file('payload') - with open(self.payload, 'wb') as f: - for _ in range(TRANSFER_SIZE >> 20): - f.write(os.urandom(1 << 20)) - self.sock_dir = self.socket_dir().name self.cpr_count = 0
@@ -121,12 +117,16 @@ def attempt(): return None return self.wait_for('QMP to come up', attempt) - def wait_migration(self, qmp): - def completed(): + def wait_migration(self, qmp, expect='completed'): + def finished(): status = qmp.cmd('query-migrate').get('status') - self.assertNotEqual(status, 'failed', 'migration failed') - return status == 'completed' - self.wait_for('migration completion', completed) + if status in ('completed', 'failed'): + if expect: + self.assertEqual(status, expect, + 'unexpected migration result') + return True + return False + self.wait_for('migration completion', finished) def wait_running(self, qmp_path): # Make sure we're dealing with the new QEMU after CPR
@@ -139,13 +139,37 @@ def wait_running(self, qmp_path): except QMP_GONE: pass - def boot(self): + def boot(self, *extra_args): self.guest = self.vm - self.qmp_path = self.prepare_vm(self.guest, 'source') + self.qmp_path = self.prepare_vm(self.guest, 'source', *extra_args) self.guest.launch() self.wait_for_console_pattern('login:') exec_command_and_wait_for_pattern(self, 'root', 'root@localhost:~#') + def start_target(self, name, main_incoming=None): + """ + Start a cpr-transfer target listening on the CPR channel. Return + the VM, its QMP socket path and the channels for 'migrate'. + """ + mig_sock = os.path.join(self.sock_dir, f'{name}-mig.sock') + cpr_sock = os.path.join(self.sock_dir, f'{name}-cpr.sock') + channels = [ + {'channel-type': 'main', + 'addr': {'transport': 'socket', 'type': 'unix', + 'path': mig_sock}}, + {'channel-type': 'cpr', + 'addr': {'transport': 'socket', 'type': 'unix', + 'path': cpr_sock}}] + + target = self.get_vm(name=name) + qmp_path = self.prepare_vm( + target, name, '-incoming', main_incoming or f'unix:{mig_sock}', + '-incoming', 'cpr,addr.transport=socket,addr.type=unix,' + f'addr.path={cpr_sock}') + target.launch() + self.wait_for('the CPR socket', lambda: os.path.exists(cpr_sock)) + return target, qmp_path, channels + def transfer(self, guest_cmd, host_cmd, cpr): """ Run a socat transfer between the guest and the host, and keep
@@ -160,21 +184,26 @@ def transfer(self, guest_cmd, host_cmd, cpr): exec_command_and_wait_for_pattern(self, 'wait %1; echo RC_$?', 'RC_0', vm=self.guest) - def transfer_both_ways(self, cpr): + def transfer_both_ways(self, cpr, size=TRANSFER_SIZE): """ - Send the payload to a file on the guest's disk, get it back into - another file on the host, then compare. + Send a random payload to a file on the guest's disk, get it back + into another file on the host, then compare. """ + payload = self.scratch_file('payload') + with open(payload, 'wb') as f: + for _ in range(size >> 20): + f.write(os.urandom(1 << 20)) + echo = self.scratch_file('echo') connect = f'VSOCK-CONNECT:{GUEST_CID}:{VSOCK_PORT},retry=10' self.transfer(f'socat -u VSOCK-LISTEN:{VSOCK_PORT} CREATE:/root/data', - ['socat', '-u', f'FILE:{self.payload}', connect], cpr) + ['socat', '-u', f'FILE:{payload}', connect], cpr) self.transfer(f'socat -u FILE:/root/data VSOCK-LISTEN:{VSOCK_PORT}', ['socat', '-u', connect, f'CREATE:{echo}'], cpr) self.log.info('%d CPR migrations performed', self.cpr_count) - self.assertTrue(filecmp.cmp(self.payload, echo, shallow=False), + self.assertTrue(filecmp.cmp(payload, echo, shallow=False), 'the data came back different') def test_cpr_transfer(self):
@@ -184,29 +213,11 @@ def test_cpr_transfer(self): it and shutting down the source """ def cpr(): - n = self.cpr_count - mig_sock = os.path.join(self.sock_dir, f'mig{n}.sock') - cpr_sock = os.path.join(self.sock_dir, f'cpr{n}.sock') - target = self.get_vm(name=f'target{n}') - target_qmp_path = self.prepare_vm( - target, f'target{n}', - '-incoming', f'unix:{mig_sock}', - '-incoming', 'cpr,addr.transport=socket,addr.type=unix,' - f'addr.path={cpr_sock}') - - # The target listens on the CPR channel: wait for it to be there - target.launch() - self.wait_for('the CPR socket', lambda: os.path.exists(cpr_sock)) - + target, target_qmp_path, channels = self.start_target( + f'target{self.cpr_count}') with self.qmp_connect(self.qmp_path) as qmp: qmp.cmd('migrate-set-parameters', mode='cpr-transfer') - qmp.cmd('migrate', channels=[ - {'channel-type': 'main', - 'addr': {'transport': 'socket', 'type': 'unix', - 'path': mig_sock}}, - {'channel-type': 'cpr', - 'addr': {'transport': 'socket', 'type': 'unix', - 'path': cpr_sock}}]) + qmp.cmd('migrate', channels=channels) self.wait_migration(qmp) self.wait_running(target_qmp_path)
@@ -217,6 +228,97 @@ def cpr(): self.boot() self.transfer_both_ways(cpr) + def test_cpr_transfer_dead_target(self): + """ + A cpr-transfer target which exits on a load error after having + taken the vsock device over must release it on exit, so that the + source can be resumed afterwards. The target lacks a device the + source has. The source is stopped beforehand, so it stays paused + on failure until we 'cont' it. + """ + self.boot('-device', 'virtio-rng-pci') + target, _, channels = self.start_target('target') + + with self.qmp_connect(self.qmp_path) as qmp: + qmp.cmd('stop') + qmp.cmd('migrate-set-parameters', mode='cpr-transfer') + qmp.cmd('migrate', channels=channels) + self.wait_migration(qmp, expect=None) + self.wait_for('the target to exit', + lambda: not target.is_running()) + self.assertEqual(target.exitcode(), 1, + 'the target did not exit on the load error') + qmp.cmd('cont') + + self.wait_running(self.qmp_path) + self.transfer_both_ways(lambda: None, size=CHECK_SIZE) + + def test_cpr_transfer_target_quit(self): + """ + A cpr-transfer target which has loaded the state but hasn't been + started yet may quit normally, and the source resume instead. + The target doesn't unrealize its devices on quit, so it must + release the vsock device on exit. + """ + self.boot() + target, target_qmp_path, channels = self.start_target( + 'target', main_incoming='defer') + + with self.qmp_connect(self.qmp_path) as qmp: + qmp.cmd('stop') + qmp.cmd('migrate-set-parameters', mode='cpr-transfer') + qmp.cmd('migrate', channels=channels) + + target_qmp = self.qmp_connect(target_qmp_path) + target_qmp.cmd('migrate-incoming', channels=channels[:1]) + self.wait_migration(qmp) + self.wait_migration(target_qmp) + self.assertEqual(target_qmp.cmd('query-status')['status'], + 'paused') + try: + target_qmp.cmd('quit') + target_qmp.close() + except QMP_GONE: + pass + + self.wait_for('the target to quit', + lambda: not target.is_running()) + self.assertEqual(target.exitcode(), 0, 'the target did not quit') + qmp.cmd('cont') + + self.wait_running(self.qmp_path) + self.transfer_both_ways(lambda: None, size=CHECK_SIZE) + + def test_cpr_transfer_failed_target(self): + """ + A cpr-transfer target which fails to load the state after having + taken the vsock device over must give it back to the source. + The target lacks a device the source has, stays alive on failure + (exit-on-error=false), and the source resumes on its own + (return-path). Then vsock must still work on the source. + """ + self.boot('-device', 'virtio-rng-pci') + target, target_qmp_path, channels = self.start_target( + 'target', main_incoming='defer') + + with self.qmp_connect(self.qmp_path) as qmp: + qmp.cmd('migrate-set-parameters', mode='cpr-transfer') + qmp.cmd('migrate-set-capabilities', capabilities=[ + {'capability': 'return-path', 'state': True}]) + qmp.cmd('migrate', channels=channels) + + # The target opens its monitor once it has read the CPR state + with self.qmp_connect(target_qmp_path) as target_qmp: + target_qmp.cmd('migrate-incoming', channels=channels[:1], + **{'exit-on-error': False}) + self.wait_migration(qmp, expect='failed') + self.wait_migration(target_qmp, expect='failed') + + # The source resumes by itself, and vsock must work on it + self.wait_running(self.qmp_path) + self.transfer_both_ways(lambda: None, size=CHECK_SIZE) + target.shutdown() + def test_cpr_exec(self): """ Perform consecutive cpr-exec migration during vsock file transfer,
Signature · 1 line
2.47.1
Quoted 7 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;
Quoted 8 lines
* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>
All comments I raised are now addressed, and other parts of the series also look good to me. For the whole series:
Regards,
Akihiko Odaki
Quoted 17 lines · Akihiko Odaki
Quoted 7 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;Quoted 8 lines
* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>
Quoted 7 lines
All comments I raised are now addressed, and other parts of the series also look good to me. For the whole series:
Reviewed-by Akihiko Odaki <[email protected]>Regards,
Akihiko Odaki
Thank you for your time and efforts!
Andrey
Quoted 65 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: addvhost_reset_ownerop
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak inrealize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor outvhost_dev_init_backend()
vhost: makevhost_dev_cleanup()safe on a partially initialized device
vhost: addvhost_dev_set_owner()/vhost_dev_reset_owner()helpers
vhost: addvhost_dev_is_initialized()helper
vhost: release ownership invhost_dev_cleanup()
migration: addMIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsockbackends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)
Quoted 8 lines
---
base-commit:615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1aBest regards,
Signature · 1 line
Andrey Drobyshev <[email protected]>
Friendly ping
Quoted 70 lines · Andrey Drobyshev
Quoted 69 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: addvhost_reset_ownerop
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak inrealize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor outvhost_dev_init_backend()
vhost: makevhost_dev_cleanup()safe on a partially initialized device
vhost: addvhost_dev_set_owner()/vhost_dev_reset_owner()helpers
vhost: addvhost_dev_is_initialized()helper
vhost: release ownership invhost_dev_cleanup()
migration: addMIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsockbackends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)---
base-commit:615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1a
Quoted 6 lines
Quoted 4 lines
Best regards,
Signature · 1 line
Andrey Drobyshev <[email protected]>Friendly ping
Ping
Quoted 71 lines · Andrey Drobyshev
Quoted 70 lines · Andrey Drobyshev
Quoted 69 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: addvhost_reset_ownerop
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak inrealize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor outvhost_dev_init_backend()
vhost: makevhost_dev_cleanup()safe on a partially initialized device
vhost: addvhost_dev_set_owner()/vhost_dev_reset_owner()helpers
vhost: addvhost_dev_is_initialized()helper
vhost: release ownership invhost_dev_cleanup()
migration: addMIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsockbackends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)---
base-commit:615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1a
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Best regards,
--
Andrey Drobyshev <[email protected]>Friendly ping
Ping
I wrote at v3 that I am fine with these patches CPR-wise, so if/once vsock maintainers are okay with them too it's only a question of which tree this should get into: vhost, migration or CPR?
Thanks,
Maciej
Quoted 78 lines · Maciej S. Szmigiero
Quoted 76 lines · Andrey Drobyshev
Quoted 75 lines · Andrey Drobyshev
Quoted 73 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: addvhost_reset_ownerop
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak inrealize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor outvhost_dev_init_backend()
vhost: makevhost_dev_cleanup()safe on a partially initialized device
vhost: addvhost_dev_set_owner()/vhost_dev_reset_owner()helpers
vhost: addvhost_dev_is_initialized()helper
vhost: release ownership invhost_dev_cleanup()
migration: addMIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsockbackends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)---
base-commit:615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1aBest regards,
--
Andrey Drobyshev <[email protected]>
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Friendly ping
Ping
I wrote at v3 that I am fine with these patches CPR-wise, so if/once vsock maintainers are okay with them too it's
For vhost and vsock, IIRC I acked most of the patches, but TBH I lost track, so if something is missing, please let me know.
Last time I took a look, it was in good shape, so it can go for me.
Thanks,
Stefano
Quoted 7 lines
only a question of which tree this should get into: vhost, migration or CPR?
Thanks,
Maciej
Quoted 81 lines · Maciej S. Szmigiero
Quoted 71 lines · Andrey Drobyshev
Quoted 70 lines · Andrey Drobyshev
Quoted 69 lines · Andrey Drobyshev
v15 -> v16:
* Patch 15:
- replace the errno filtering invhost_dev_try_release_owner()with a new 'owner_handed_over' flag in structvhost_dev. Release the ownership only if it was handed over to us, and report any failure;* Patch 19:
- set 'owner_handed_over' in .post_load() on the CPR target;
- drop the redundant 'owner_reset' flag, use vhost_dev.owner instead;
- drop Stefano's R-b because of that.v15: https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by Andrey Drobyshev <[email protected]>---
Andrey Drobyshev (22):
migration/cpr: forbid saving duplicate CprFds into hash table tests/unit: add test for duplicate keys in CPR FD table
migration: forbid snapshots in CPR migration modes
migration: forbid snapshots under Xen
tests/qtest/migration: add test for snapshot rejection in CPR modes
vhost-vsock: block CPR migration modes
vhost: addvhost_reset_ownerop
vhost-vsock: don't reset connections during CPR
vhost-vsock: fix FD leak inrealize()
vhost-vsock: preserve vhost FD during CPR
vhost: factor outvhost_dev_init_backend()
vhost: makevhost_dev_cleanup()safe on a partially initialized device
vhost: addvhost_dev_set_owner()/vhost_dev_reset_owner()helpers
vhost: addvhost_dev_is_initialized()helper
vhost: release ownership invhost_dev_cleanup()
migration: addMIG_EVENT_INCOMING_FAILED
vhost: release ownership when the incoming migration fails
vhost: release ownership on exit
vhost-vsock: hand off device ownership across CPR tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec tests/functional: add vhost-vsock CPR test with a live guest tests/functional: cover a cpr-transfer target going away with vhost-vsockbackends/hostmem-memfd.c | 5 +- backends/hostmem-shm.c | 5 +- backends/iommufd.c | 4 +- hw/vfio/container-legacy.c | 7 +- hw/vfio/cpr-legacy.c | 29 +- hw/vfio/cpr.c | 6 +- hw/vfio/pci.c | 5 +- hw/virtio/vhost-kernel.c | 6 + hw/virtio/vhost-vsock.c | 193 ++++++++++++- hw/virtio/vhost.c | 136 +++++++++- include/hw/vfio/vfio-cpr.h | 7 +- include/hw/virtio/vhost-backend.h | 2 + include/hw/virtio/vhost.h | 80 ++++++ include/migration/cpr.h | 2 +- include/migration/misc.h | 5 + migration/cpr.c | 64 +++-- migration/migration.c | 10 + migration/options.c | 13 + system/physmem.c | 5 +- tests/functional/x86_64/meson.build | 2 + tests/functional/x86_64/test_vhost_vsock_cpr.py | 347 ++++++++++++++++++++++++ tests/qtest/migration/cpr-tests.c | 133 ++++++++- tests/unit/meson.build | 1 + tests/unit/test-cpr.c | 98 +++++++ 24 files changed, 1084 insertions(+), 81 deletions(-)---
base-commit:615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1aQuoted 8 lines
Quoted 6 lines
Quoted 4 lines
Best regards,
--
Andrey Drobyshev <[email protected]>Friendly ping
Ping
Quoted 8 lines
I wrote at v3 that I am fine with these patches CPR-wise, so if/once vsock maintainers are okay with them too it's only a question of which tree this should get into: vhost, migration or CPR?
Thanks,
Maciej
Since the majority of the patches which introduce functional changes are vhost-specific - maybe vhost tree is the best candidate here?
@mst Michael, do you mind?
Thanks,
Andrey