* [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-28 14:26 ` Peter Xu
2026-09-18 13:46 ` [PATCH v7 02/16] tests/unit: add test for duplicate keys in CPR FD table Andrey Drobyshev
` (16 subsequent siblings)
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
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
Fixes: a181df93bb3 ("migration/cpr: use hashtable for cpr fds")
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
backends/hostmem-memfd.c | 5 ++++-
backends/hostmem-shm.c | 5 ++++-
backends/iommufd.c | 4 +++-
hw/vfio/container-legacy.c | 17 +++++++++-----
hw/vfio/cpr-legacy.c | 10 +++++----
hw/vfio/cpr.c | 6 ++---
hw/vfio/pci.c | 5 ++++-
include/hw/vfio/vfio-cpr.h | 6 ++---
include/migration/cpr.h | 2 +-
migration/cpr.c | 55 ++++++++++++++++++++++++++++++----------------
system/physmem.c | 5 +++--
11 files changed, 78 insertions(+), 42 deletions(-)
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index e21c5a3f2e2..a95a4368a9e 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -54,7 +54,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
if (fd == -1) {
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;
diff --git a/backends/hostmem-shm.c b/backends/hostmem-shm.c
index 806e2670e03..2fa7b823a62 100644
--- a/backends/hostmem-shm.c
+++ b/backends/hostmem-shm.c
@@ -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. */
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 15f2a513501..45fd2e44c7f 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -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"
diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
index 7ac7b372145..1d2c8356189 100644
--- a/hw/vfio/container-legacy.c
+++ b/hw/vfio/container-legacy.c
@@ -844,16 +844,21 @@ static void vfio_group_put(VFIOGroup *group)
static bool vfio_device_get(VFIOGroup *group, const char *name,
VFIODevice *vbasedev, Error **errp)
{
+ ERRP_GUARD();
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->fd, 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);
+ if (!*errp) {
+ 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;
}
diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
index 2d40d8baeaf..14a06a1fa59 100644
--- a/hw/vfio/cpr-legacy.c
+++ b/hw/vfio/cpr-legacy.c
@@ -254,15 +254,16 @@ 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(int d, const char *name, Error **errp)
{
const int id = 0;
int fd = cpr_find_fd(name, id);
if (fd < 0) {
fd = ioctl(d, VFIO_GROUP_GET_DEVICE_FD, name);
- if (fd >= 0) {
- cpr_save_fd(name, id, fd);
+ if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) {
+ close(fd);
+ fd = -1;
}
}
return fd;
@@ -291,6 +292,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;
}
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index ffa4f8e099d..6e1659dcddc 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -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)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 428ab2f0698..ad13f2306f1 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -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;
}
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index ecabe0c747d..e57766e7e2b 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -60,7 +60,7 @@ 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(int d, const char *name, Error **errp);
bool vfio_cpr_container_match(struct VFIOLegacyContainer *container,
struct VFIOGroup *group, int fd);
@@ -71,8 +71,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,
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 56fb67e6b4d..3ad084d5bab 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -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);
diff --git a/migration/cpr.c b/migration/cpr.c
index bca43e9bf35..aeb9ad12392 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -82,10 +82,27 @@ static GHashTable *get_cpr_fds_hash(void)
return cpr_fds_hash;
}
+static CprFd *find_fd(const char *name, int id)
+{
+ CprFd key = {
+ .name = (char *)name,
+ .id = id,
+ };
+
+ return g_hash_table_lookup(get_cpr_fds_hash(), &key);
+}
+
static void cpr_fd_hash_insert(CprFd *elem)
{
- /* Use the same CprFd as key and value. */
- g_hash_table_insert(get_cpr_fds_hash(), elem, elem);
+ GHashTable *hash = get_cpr_fds_hash();
+
+ /*
+ * The same CprFd is used as key and value, and the table owns the key.
+ * Inserting a duplicate would destroy the new element while keeping it
+ * as the value, so a duplicate key is a bug in the caller.
+ */
+ g_assert(!g_hash_table_contains(hash, elem));
+ g_hash_table_insert(hash, elem, elem);
}
static int cpr_fd_pre_save(void *opaque)
@@ -127,26 +144,24 @@ static int cpr_fd_post_load(void *opaque, int version_id)
return 0;
}
-void cpr_save_fd(const char *name, int id, int fd)
+bool cpr_save_fd(const char *name, int id, int fd, Error **errp)
{
- CprFd *elem = g_new0(CprFd, 1);
+ CprFd *elem;
+ if (find_fd(name, id)) {
+ error_setg(errp, "cpr fd '%s' id %d is already registered", name, id);
+ return false;
+ }
+
+ elem = g_new0(CprFd, 1);
trace_cpr_save_fd(name, id, fd);
elem->name = g_strdup(name);
elem->namelen = strlen(name) + 1;
elem->id = id;
elem->fd = fd;
cpr_fd_hash_insert(elem);
-}
-
-static CprFd *find_fd(const char *name, int id)
-{
- CprFd key = {
- .name = (char *)name,
- .id = id,
- };
- return g_hash_table_lookup(get_cpr_fds_hash(), &key);
+ return true;
}
void cpr_delete_fd(const char *name, int id)
@@ -176,7 +191,7 @@ void cpr_resave_fd(const char *name, int id, int fd)
int old_fd = elem ? elem->fd : -1;
if (old_fd < 0) {
- cpr_save_fd(name, id, fd);
+ cpr_save_fd(name, id, fd, &error_abort);
} else if (old_fd != fd) {
error_report("internal error: cpr fd '%s' id %d value %d "
"already saved with a different value %d",
@@ -192,8 +207,9 @@ int cpr_open_fd(const char *path, int flags, const char *name, int id,
if (fd < 0) {
fd = qemu_open(path, flags, errp);
- if (fd >= 0) {
- cpr_save_fd(name, id, fd);
+ if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) {
+ close(fd);
+ fd = -1;
}
}
return fd;
@@ -400,10 +416,11 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index,
}
} else {
fd = monitor_fd_param(monitor_cur(), fdname, errp);
- if (fd >= 0) {
- cpr_save_fd(name, index, fd);
- } else {
+ if (fd < 0) {
error_prepend(errp, "Could not parse object fd %s:", fdname);
+ } else if (!cpr_save_fd(name, index, fd, errp)) {
+ close(fd);
+ fd = -1;
}
}
return fd;
diff --git a/system/physmem.c b/system/physmem.c
index a4a6a4710c6..e94fe5d4a4a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2445,8 +2445,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;
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table
2026-09-18 13:46 ` [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table Andrey Drobyshev
@ 2026-09-28 14:26 ` Peter Xu
2026-09-29 13:28 ` Andrey Drobyshev
0 siblings, 1 reply; 35+ messages in thread
From: Peter Xu @ 2026-09-28 14:26 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Fri, Sep 18, 2026 at 04:46:52PM +0300, Andrey Drobyshev wrote:
> 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
>
> Fixes: a181df93bb3 ("migration/cpr: use hashtable for cpr fds")
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> backends/hostmem-memfd.c | 5 ++++-
> backends/hostmem-shm.c | 5 ++++-
> backends/iommufd.c | 4 +++-
> hw/vfio/container-legacy.c | 17 +++++++++-----
> hw/vfio/cpr-legacy.c | 10 +++++----
> hw/vfio/cpr.c | 6 ++---
> hw/vfio/pci.c | 5 ++++-
> include/hw/vfio/vfio-cpr.h | 6 ++---
> include/migration/cpr.h | 2 +-
> migration/cpr.c | 55 ++++++++++++++++++++++++++++++----------------
> system/physmem.c | 5 +++--
> 11 files changed, 78 insertions(+), 42 deletions(-)
>
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index e21c5a3f2e2..a95a4368a9e 100644
> --- a/backends/hostmem-memfd.c
> +++ b/backends/hostmem-memfd.c
> @@ -54,7 +54,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> if (fd == -1) {
> 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;
> diff --git a/backends/hostmem-shm.c b/backends/hostmem-shm.c
> index 806e2670e03..2fa7b823a62 100644
> --- a/backends/hostmem-shm.c
> +++ b/backends/hostmem-shm.c
> @@ -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. */
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 15f2a513501..45fd2e44c7f 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -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"
> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
> index 7ac7b372145..1d2c8356189 100644
> --- a/hw/vfio/container-legacy.c
> +++ b/hw/vfio/container-legacy.c
> @@ -844,16 +844,21 @@ static void vfio_group_put(VFIOGroup *group)
> static bool vfio_device_get(VFIOGroup *group, const char *name,
> VFIODevice *vbasedev, Error **errp)
> {
> + ERRP_GUARD();
> 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->fd, 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);
> + if (!*errp) {
IIUC this implies something not done right. If we add errp into
vfio_cpr_group_get_device_fd(), then when fd<0 (failure case), we should
set *errp always. Maybe this should be moved into it?
> + 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;
> }
>
> diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
> index 2d40d8baeaf..14a06a1fa59 100644
> --- a/hw/vfio/cpr-legacy.c
> +++ b/hw/vfio/cpr-legacy.c
> @@ -254,15 +254,16 @@ 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(int d, const char *name, Error **errp)
> {
> const int id = 0;
> int fd = cpr_find_fd(name, id);
>
> if (fd < 0) {
> fd = ioctl(d, VFIO_GROUP_GET_DEVICE_FD, name);
> - if (fd >= 0) {
> - cpr_save_fd(name, id, fd);
> + if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) {
IMHO it's slightly error prone to do oneliner like this, this might be
relevant to the above !*errp question.
> + close(fd);
> + fd = -1;
> }
> }
> return fd;
> @@ -291,6 +292,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;
> }
> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
> index ffa4f8e099d..6e1659dcddc 100644
> --- a/hw/vfio/cpr.c
> +++ b/hw/vfio/cpr.c
> @@ -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)
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 428ab2f0698..ad13f2306f1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -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;
> }
>
> diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
> index ecabe0c747d..e57766e7e2b 100644
> --- a/include/hw/vfio/vfio-cpr.h
> +++ b/include/hw/vfio/vfio-cpr.h
> @@ -60,7 +60,7 @@ 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(int d, const char *name, Error **errp);
>
> bool vfio_cpr_container_match(struct VFIOLegacyContainer *container,
> struct VFIOGroup *group, int fd);
> @@ -71,8 +71,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,
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> index 56fb67e6b4d..3ad084d5bab 100644
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
> @@ -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);
> diff --git a/migration/cpr.c b/migration/cpr.c
> index bca43e9bf35..aeb9ad12392 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -82,10 +82,27 @@ static GHashTable *get_cpr_fds_hash(void)
> return cpr_fds_hash;
> }
>
> +static CprFd *find_fd(const char *name, int id)
> +{
> + CprFd key = {
> + .name = (char *)name,
> + .id = id,
> + };
> +
> + return g_hash_table_lookup(get_cpr_fds_hash(), &key);
> +}
> +
> static void cpr_fd_hash_insert(CprFd *elem)
> {
> - /* Use the same CprFd as key and value. */
> - g_hash_table_insert(get_cpr_fds_hash(), elem, elem);
> + GHashTable *hash = get_cpr_fds_hash();
> +
> + /*
> + * The same CprFd is used as key and value, and the table owns the key.
> + * Inserting a duplicate would destroy the new element while keeping it
> + * as the value, so a duplicate key is a bug in the caller.
IIUC this comment is slightly misleading. It almost says "the caller can't
do this because the impl of the API does that", but logically the caller
shouldn't care about the internal impl..
There're two relevant issues, IMHO:
(1) the memory crash described in this patch, solid, needs fixing one way
or another
(2) if there's real use case of injecting the same (name, id) fd twice, or
should we forbid it?
For (1), fixing it like this patch would work, but then the comment is
wrong here, and it means we already said YES to (2), hence (1) relies on
(2).
But I do see the existance of cpr_resave_fd(). I confess I don't know why
it existed and didn't check. That looks exactly like sometimes we can save
the same fd twice, if the 1st one is fd=-1. Any quick answer?
Thanks,
> + */
> + g_assert(!g_hash_table_contains(hash, elem));
> + g_hash_table_insert(hash, elem, elem);
> }
>
> static int cpr_fd_pre_save(void *opaque)
> @@ -127,26 +144,24 @@ static int cpr_fd_post_load(void *opaque, int version_id)
> return 0;
> }
>
> -void cpr_save_fd(const char *name, int id, int fd)
> +bool cpr_save_fd(const char *name, int id, int fd, Error **errp)
> {
> - CprFd *elem = g_new0(CprFd, 1);
> + CprFd *elem;
>
> + if (find_fd(name, id)) {
> + error_setg(errp, "cpr fd '%s' id %d is already registered", name, id);
> + return false;
> + }
> +
> + elem = g_new0(CprFd, 1);
> trace_cpr_save_fd(name, id, fd);
> elem->name = g_strdup(name);
> elem->namelen = strlen(name) + 1;
> elem->id = id;
> elem->fd = fd;
> cpr_fd_hash_insert(elem);
> -}
> -
> -static CprFd *find_fd(const char *name, int id)
> -{
> - CprFd key = {
> - .name = (char *)name,
> - .id = id,
> - };
>
> - return g_hash_table_lookup(get_cpr_fds_hash(), &key);
> + return true;
> }
>
> void cpr_delete_fd(const char *name, int id)
> @@ -176,7 +191,7 @@ void cpr_resave_fd(const char *name, int id, int fd)
> int old_fd = elem ? elem->fd : -1;
>
> if (old_fd < 0) {
> - cpr_save_fd(name, id, fd);
> + cpr_save_fd(name, id, fd, &error_abort);
> } else if (old_fd != fd) {
> error_report("internal error: cpr fd '%s' id %d value %d "
> "already saved with a different value %d",
> @@ -192,8 +207,9 @@ int cpr_open_fd(const char *path, int flags, const char *name, int id,
>
> if (fd < 0) {
> fd = qemu_open(path, flags, errp);
> - if (fd >= 0) {
> - cpr_save_fd(name, id, fd);
> + if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) {
> + close(fd);
> + fd = -1;
> }
> }
> return fd;
> @@ -400,10 +416,11 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index,
> }
> } else {
> fd = monitor_fd_param(monitor_cur(), fdname, errp);
> - if (fd >= 0) {
> - cpr_save_fd(name, index, fd);
> - } else {
> + if (fd < 0) {
> error_prepend(errp, "Could not parse object fd %s:", fdname);
> + } else if (!cpr_save_fd(name, index, fd, errp)) {
> + close(fd);
> + fd = -1;
> }
> }
> return fd;
> diff --git a/system/physmem.c b/system/physmem.c
> index a4a6a4710c6..e94fe5d4a4a 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2445,8 +2445,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;
>
> --
> 2.47.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table
2026-09-28 14:26 ` Peter Xu
@ 2026-09-29 13:28 ` Andrey Drobyshev
2026-09-29 15:37 ` Peter Xu
0 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-29 13:28 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On 9/28/26 5:26 PM, Peter Xu wrote:
> On Fri, Sep 18, 2026 at 04:46:52PM +0300, Andrey Drobyshev wrote:
>> 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
>>
>> Fixes: a181df93bb3 ("migration/cpr: use hashtable for cpr fds")
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> backends/hostmem-memfd.c | 5 ++++-
>> backends/hostmem-shm.c | 5 ++++-
>> backends/iommufd.c | 4 +++-
>> hw/vfio/container-legacy.c | 17 +++++++++-----
>> hw/vfio/cpr-legacy.c | 10 +++++----
>> hw/vfio/cpr.c | 6 ++---
>> hw/vfio/pci.c | 5 ++++-
>> include/hw/vfio/vfio-cpr.h | 6 ++---
>> include/migration/cpr.h | 2 +-
>> migration/cpr.c | 55 ++++++++++++++++++++++++++++++----------------
>> system/physmem.c | 5 +++--
>> 11 files changed, 78 insertions(+), 42 deletions(-)
>>
>> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
>> index e21c5a3f2e2..a95a4368a9e 100644
>> --- a/backends/hostmem-memfd.c
>> +++ b/backends/hostmem-memfd.c
>> @@ -54,7 +54,10 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>> if (fd == -1) {
>> 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;
>> diff --git a/backends/hostmem-shm.c b/backends/hostmem-shm.c
>> index 806e2670e03..2fa7b823a62 100644
>> --- a/backends/hostmem-shm.c
>> +++ b/backends/hostmem-shm.c
>> @@ -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. */
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 15f2a513501..45fd2e44c7f 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -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"
>> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
>> index 7ac7b372145..1d2c8356189 100644
>> --- a/hw/vfio/container-legacy.c
>> +++ b/hw/vfio/container-legacy.c
>> @@ -844,16 +844,21 @@ static void vfio_group_put(VFIOGroup *group)
>> static bool vfio_device_get(VFIOGroup *group, const char *name,
>> VFIODevice *vbasedev, Error **errp)
>> {
>> + ERRP_GUARD();
>> 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->fd, 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);
>> + if (!*errp) {
>
> IIUC this implies something not done right. If we add errp into
> vfio_cpr_group_get_device_fd(), then when fd<0 (failure case), we should
> set *errp always. Maybe this should be moved into it?
Agreed. I'll just move error handling into
vfio_cpr_group_get_device_fd(). Should look cleaner.
>> + 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;
>> }
>>
>> diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
>> index 2d40d8baeaf..14a06a1fa59 100644
>> --- a/hw/vfio/cpr-legacy.c
>> +++ b/hw/vfio/cpr-legacy.c
>> @@ -254,15 +254,16 @@ 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(int d, const char *name, Error **errp)
>> {
>> const int id = 0;
>> int fd = cpr_find_fd(name, id);
>>
>> if (fd < 0) {
>> fd = ioctl(d, VFIO_GROUP_GET_DEVICE_FD, name);
>> - if (fd >= 0) {
>> - cpr_save_fd(name, id, fd);
>> + if (fd >= 0 && !cpr_save_fd(name, id, fd, errp)) {
>
> IMHO it's slightly error prone to do oneliner like this, this might be
> relevant to the above !*errp question.
Ditto.
>> + close(fd);
>> + fd = -1;
>> }
>> }
>> return fd;
>> @@ -291,6 +292,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;
>> }
>> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
>> index ffa4f8e099d..6e1659dcddc 100644
>> --- a/hw/vfio/cpr.c
>> +++ b/hw/vfio/cpr.c
>> @@ -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)
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 428ab2f0698..ad13f2306f1 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -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;
>> }
>>
>> diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
>> index ecabe0c747d..e57766e7e2b 100644
>> --- a/include/hw/vfio/vfio-cpr.h
>> +++ b/include/hw/vfio/vfio-cpr.h
>> @@ -60,7 +60,7 @@ 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(int d, const char *name, Error **errp);
>>
>> bool vfio_cpr_container_match(struct VFIOLegacyContainer *container,
>> struct VFIOGroup *group, int fd);
>> @@ -71,8 +71,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,
>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>> index 56fb67e6b4d..3ad084d5bab 100644
>> --- a/include/migration/cpr.h
>> +++ b/include/migration/cpr.h
>> @@ -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);
>> diff --git a/migration/cpr.c b/migration/cpr.c
>> index bca43e9bf35..aeb9ad12392 100644
>> --- a/migration/cpr.c
>> +++ b/migration/cpr.c
>> @@ -82,10 +82,27 @@ static GHashTable *get_cpr_fds_hash(void)
>> return cpr_fds_hash;
>> }
>>
>> +static CprFd *find_fd(const char *name, int id)
>> +{
>> + CprFd key = {
>> + .name = (char *)name,
>> + .id = id,
>> + };
>> +
>> + return g_hash_table_lookup(get_cpr_fds_hash(), &key);
>> +}
>> +
>> static void cpr_fd_hash_insert(CprFd *elem)
>> {
>> - /* Use the same CprFd as key and value. */
>> - g_hash_table_insert(get_cpr_fds_hash(), elem, elem);
>> + GHashTable *hash = get_cpr_fds_hash();
>> +
>> + /*
>> + * The same CprFd is used as key and value, and the table owns the key.
>> + * Inserting a duplicate would destroy the new element while keeping it
>> + * as the value, so a duplicate key is a bug in the caller.
>
> IIUC this comment is slightly misleading. It almost says "the caller can't
> do this because the impl of the API does that", but logically the caller
> shouldn't care about the internal impl..
It's not the implementation we care about, but the documented behaviour.
And the docs say the key_destroy_func will be called on duplicated
keys, if provided. The "bug" on the caller's side here is that caller
doesn't take it into account, yet providing same object for both key and
value.
> There're two relevant issues, IMHO:
>
> (1) the memory crash described in this patch, solid, needs fixing one way
> or another
>
> (2) if there's real use case of injecting the same (name, id) fd twice, or
> should we forbid it?
>
> For (1), fixing it like this patch would work, but then the comment is
> wrong here, and it means we already said YES to (2), hence (1) relies on
> (2).
>
> But I do see the existance of cpr_resave_fd(). I confess I don't know why
> it existed and didn't check. That looks exactly like sometimes we can save
> the same fd twice, if the 1st one is fd=-1. Any quick answer?
AFAICT the sole purpose of cpr_resave_fd() is to be called from
different sites with the same (name, id, fd), to make sure the key is
registered in the table. What it does:
- no entry - save it into hash table;
- same FD already there - do nothing, table isn't updated;
- different FD present - abort as an internal error.
That means there's no case of injecting the same (name, id) twice into
the hash table. That is actually consistent with our patch here.
I'd leave the code as-is and only reword the comment, if that's fine
with you.
Thanks,
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table
2026-09-29 13:28 ` Andrey Drobyshev
@ 2026-09-29 15:37 ` Peter Xu
2026-09-29 17:09 ` Andrey Drobyshev
0 siblings, 1 reply; 35+ messages in thread
From: Peter Xu @ 2026-09-29 15:37 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Tue, Sep 29, 2026 at 04:28:19PM +0300, Andrey Drobyshev wrote:
> > But I do see the existance of cpr_resave_fd(). I confess I don't know why
> > it existed and didn't check. That looks exactly like sometimes we can save
> > the same fd twice, if the 1st one is fd=-1. Any quick answer?
>
> AFAICT the sole purpose of cpr_resave_fd() is to be called from
> different sites with the same (name, id, fd), to make sure the key is
> registered in the table. What it does:
>
> - no entry - save it into hash table;
> - same FD already there - do nothing, table isn't updated;
> - different FD present - abort as an internal error.
>
> That means there's no case of injecting the same (name, id) twice into
> the hash table. That is actually consistent with our patch here.
>
> I'd leave the code as-is and only reword the comment, if that's fine
> with you.
The whole cpr_resave_fd() API is confusing, I still don't understand why we
need it: we should make sure the fd is always properly inserted, and only
once somehow.. but I agree it's not an issue for the current patch.
Said that, for this patch, IIUC we still should need something like this:
diff --git a/migration/cpr.c b/migration/cpr.c
index bca43e9bf3..d26656c5c1 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -173,14 +173,13 @@ int cpr_find_fd(const char *name, int id)
void cpr_resave_fd(const char *name, int id, int fd)
{
CprFd *elem = find_fd(name, id);
- int old_fd = elem ? elem->fd : -1;
- if (old_fd < 0) {
+ if (!elem) {
cpr_save_fd(name, id, fd);
- } else if (old_fd != fd) {
+ } else if (elem->fd != fd) {
error_report("internal error: cpr fd '%s' id %d value %d "
"already saved with a different value %d",
- name, id, fd, old_fd);
+ name, id, fd, elem->fd);
g_assert_not_reached();
}
}
Otherwise at least from API level some user can inject one fd=-1 entry,
then resave fd with something else causing cpr_save_fd() done twice on the
same (name, id) index, triggering the new assert.
Thanks,
--
Peter Xu
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table
2026-09-29 15:37 ` Peter Xu
@ 2026-09-29 17:09 ` Andrey Drobyshev
0 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-29 17:09 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On 9/29/26 6:37 PM, Peter Xu wrote:
> On Tue, Sep 29, 2026 at 04:28:19PM +0300, Andrey Drobyshev wrote:
>>> But I do see the existance of cpr_resave_fd(). I confess I don't know why
>>> it existed and didn't check. That looks exactly like sometimes we can save
>>> the same fd twice, if the 1st one is fd=-1. Any quick answer?
>>
>> AFAICT the sole purpose of cpr_resave_fd() is to be called from
>> different sites with the same (name, id, fd), to make sure the key is
>> registered in the table. What it does:
>>
>> - no entry - save it into hash table;
>> - same FD already there - do nothing, table isn't updated;
>> - different FD present - abort as an internal error.
>>
>> That means there's no case of injecting the same (name, id) twice into
>> the hash table. That is actually consistent with our patch here.
>>
>> I'd leave the code as-is and only reword the comment, if that's fine
>> with you.
>
> The whole cpr_resave_fd() API is confusing, I still don't understand why we
> need it: we should make sure the fd is always properly inserted, and only
> once somehow.. but I agree it's not an issue for the current patch.
>
> Said that, for this patch, IIUC we still should need something like this:
>
> diff --git a/migration/cpr.c b/migration/cpr.c
> index bca43e9bf3..d26656c5c1 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -173,14 +173,13 @@ int cpr_find_fd(const char *name, int id)
> void cpr_resave_fd(const char *name, int id, int fd)
> {
> CprFd *elem = find_fd(name, id);
> - int old_fd = elem ? elem->fd : -1;
>
> - if (old_fd < 0) {
> + if (!elem) {
> cpr_save_fd(name, id, fd);
> - } else if (old_fd != fd) {
> + } else if (elem->fd != fd) {
> error_report("internal error: cpr fd '%s' id %d value %d "
> "already saved with a different value %d",
> - name, id, fd, old_fd);
> + name, id, fd, elem->fd);
> g_assert_not_reached();
> }
> }
>
> Otherwise at least from API level some user can inject one fd=-1 entry,
> then resave fd with something else causing cpr_save_fd() done twice on the
> same (name, id) index, triggering the new assert.
>
> Thanks,
AFAIU this bug (injecting fd=-1) is non-existing at the moment, but
surely adding some API hardening won't hurt. I'll include your hunk in
the patch, thanks.
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 02/16] tests/unit: add test for duplicate keys in CPR FD table
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes Andrey Drobyshev
` (15 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
cpr_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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/unit/meson.build | 1 +
tests/unit/test-cpr.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+)
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index e47bc7225ab..8d229ff60e6 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -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',
diff --git a/tests/unit/test-cpr.c b/tests/unit/test-cpr.c
new file mode 100644
index 00000000000..dfc6b1f2ec0
--- /dev/null
+++ b/tests/unit/test-cpr.c
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Unit tests for the CPR fd table
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "migration/cpr.h"
+#include "migration/vmstate.h"
+#include "monitor/monitor.h"
+#include "hw/vfio/vfio-cpr.h"
+
+/* Stubs for the parts of migration/cpr.c that these tests don't exercise */
+
+const VMStateDescription vmstate_cpr_vfio_devices = {
+ .name = "cpr vfio devices",
+ .fields = (const VMStateField[]) {
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
+{
+ g_assert_not_reached();
+}
+
+QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
+{
+ g_assert_not_reached();
+}
+
+QEMUFile *cpr_exec_output(Error **errp)
+{
+ g_assert_not_reached();
+}
+
+QEMUFile *cpr_exec_input(Error **errp)
+{
+ g_assert_not_reached();
+}
+
+bool cpr_exec_persist_state(QEMUFile *f, Error **errp)
+{
+ g_assert_not_reached();
+}
+
+bool cpr_exec_has_state(void)
+{
+ return false;
+}
+
+void cpr_exec_unpreserve_fds(void)
+{
+}
+
+int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
+{
+ g_assert_not_reached();
+}
+
+static void test_cpr_fd_duplicate(void)
+{
+ Error *err = NULL;
+
+ g_assert_true(cpr_save_fd("dup", 0, 10, &error_abort));
+
+ /* A second registration of the same (name, id) key must be refused */
+ g_assert_false(cpr_save_fd("dup", 0, 11, &err));
+ g_assert_nonnull(err);
+ g_assert_nonnull(strstr(error_get_pretty(err), "already registered"));
+ error_free(err);
+
+ /* and the first registration must be left intact */
+ g_assert_cmpint(cpr_find_fd("dup", 0), ==, 10);
+
+ /* A different id or name is a different key */
+ g_assert_true(cpr_save_fd("dup", 1, 12, &error_abort));
+ g_assert_true(cpr_save_fd("other", 0, 13, &error_abort));
+ g_assert_cmpint(cpr_find_fd("dup", 1), ==, 12);
+ g_assert_cmpint(cpr_find_fd("other", 0), ==, 13);
+
+ /* Once deleted, the key can be registered again */
+ cpr_delete_fd("dup", 0);
+ g_assert_cmpint(cpr_find_fd("dup", 0), ==, -1);
+ g_assert_true(cpr_save_fd("dup", 0, 14, &error_abort));
+ g_assert_cmpint(cpr_find_fd("dup", 0), ==, 14);
+
+ cpr_delete_fd("dup", 0);
+ cpr_delete_fd("dup", 1);
+ cpr_delete_fd("other", 0);
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/cpr/fd/duplicate", test_cpr_fd_duplicate);
+ return g_test_run();
+}
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 01/16] migration/cpr: forbid saving duplicate CprFds into hash table Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 02/16] tests/unit: add test for duplicate keys in CPR FD table Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-28 14:29 ` Peter Xu
2026-09-28 14:54 ` Fabiano Rosas
2026-09-18 13:46 ` [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes Andrey Drobyshev
` (14 subsequent siblings)
17 siblings, 2 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
The 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/options.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/migration/options.c b/migration/options.c
index 05b026f4bf2..f648be1302b 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -555,8 +555,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];
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes
2026-09-18 13:46 ` [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes Andrey Drobyshev
@ 2026-09-28 14:29 ` Peter Xu
2026-09-28 14:54 ` Fabiano Rosas
1 sibling, 0 replies; 35+ messages in thread
From: Peter Xu @ 2026-09-28 14:29 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Fri, Sep 18, 2026 at 04:46:54PM +0300, Andrey Drobyshev wrote:
> The 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.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes
2026-09-18 13:46 ` [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes Andrey Drobyshev
2026-09-28 14:29 ` Peter Xu
@ 2026-09-28 14:54 ` Fabiano Rosas
2026-09-28 17:39 ` Akihiko Odaki
1 sibling, 1 reply; 35+ messages in thread
From: Fabiano Rosas @ 2026-09-28 14:54 UTC (permalink / raw)
To: Andrey Drobyshev, qemu-devel
Cc: bchaney, mark.kanda, philmd, vsementsov, andrey.drobyshev,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den, sgarzare,
Akihiko Odaki
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> writes:
> The 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.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> migration/options.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/migration/options.c b/migration/options.c
> index 05b026f4bf2..f648be1302b 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -555,8 +555,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];
+CC Akihiko Odaki
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes
2026-09-28 14:54 ` Fabiano Rosas
@ 2026-09-28 17:39 ` Akihiko Odaki
2026-09-29 13:28 ` Andrey Drobyshev
0 siblings, 1 reply; 35+ messages in thread
From: Akihiko Odaki @ 2026-09-28 17:39 UTC (permalink / raw)
To: Fabiano Rosas, Andrey Drobyshev, qemu-devel
Cc: bchaney, mark.kanda, philmd, vsementsov, maciej.szmigiero,
dongli.zhang, mst, clg, peterx, den, sgarzare
On 2026/09/28 23:54, Fabiano Rosas wrote:
> Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> writes:
>
>> The 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.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> migration/options.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index 05b026f4bf2..f648be1302b 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -555,8 +555,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];
>
> +CC Akihiko Odaki
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
Thanks for CCing me. This is also relevant to my recent work on Xen
migration. The snapshot RAM-saving path does not support Xen's memory
mappings, so migrate_can_snapshot() should reject Xen as well.
If you send another revision, could you include that check? This is a
pre-existing issue, though, so it need not hold up this patch.
Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes
2026-09-28 17:39 ` Akihiko Odaki
@ 2026-09-29 13:28 ` Andrey Drobyshev
0 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-29 13:28 UTC (permalink / raw)
To: Akihiko Odaki, Fabiano Rosas, qemu-devel
Cc: bchaney, mark.kanda, philmd, vsementsov, maciej.szmigiero,
dongli.zhang, mst, clg, peterx, den, sgarzare
On 9/28/26 8:39 PM, Akihiko Odaki wrote:
> On 2026/09/28 23:54, Fabiano Rosas wrote:
>> Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> writes:
>>
>>> The 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.
>>>
>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>> ---
>>> migration/options.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 05b026f4bf2..f648be1302b 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -555,8 +555,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];
>>
>> +CC Akihiko Odaki
>>
>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>
> Thanks for CCing me. This is also relevant to my recent work on Xen
> migration. The snapshot RAM-saving path does not support Xen's memory
> mappings, so migrate_can_snapshot() should reject Xen as well.
>
> If you send another revision, could you include that check? This is a
> pre-existing issue, though, so it need not hold up this patch.
>
> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>
> Regards,
> Akihiko Odaki
Hello Akihiko,
Sure, I can add the check. It's of course slightly beyond the scope of
this series, but since the patch is trivial there shouldn't be any
problems. IIUC adding this should be enough:
if (xen_enabled()) {
error_setg(errp, "Snapshots are not compatible with Xen");
return false;
}
I'll make it a separate patch on the next respin.
Thanks,
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (2 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 03/16] migration: forbid snapshots in CPR migration modes Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-28 14:33 ` Peter Xu
2026-09-18 13:46 ` [PATCH v7 05/16] vhost-vsock: block CPR migration modes Andrey Drobyshev
` (13 subsequent siblings)
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Snapshots 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/qtest/migration/cpr-tests.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
index 0bec753b4ab..a5255193101 100644
--- a/tests/qtest/migration/cpr-tests.c
+++ b/tests/qtest/migration/cpr-tests.c
@@ -290,11 +290,45 @@ 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");
+ 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);
+ }
+
+ /* Back in normal mode the mode check no longer fires */
+ 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;
- /* no tests in the smoke set for now */
+ migration_test_add("/migration/mode/no-snapshot", test_mode_no_snapshot);
if (!env->full_set) {
return;
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes
2026-09-18 13:46 ` [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes Andrey Drobyshev
@ 2026-09-28 14:33 ` Peter Xu
2026-09-29 13:28 ` Andrey Drobyshev
0 siblings, 1 reply; 35+ messages in thread
From: Peter Xu @ 2026-09-28 14:33 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Fri, Sep 18, 2026 at 04:46:55PM +0300, Andrey Drobyshev wrote:
> Snapshots 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.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> tests/qtest/migration/cpr-tests.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
> index 0bec753b4ab..a5255193101 100644
> --- a/tests/qtest/migration/cpr-tests.c
> +++ b/tests/qtest/migration/cpr-tests.c
> @@ -290,11 +290,45 @@ 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");
> + 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);
> + }
> +
> + /* Back in normal mode the mode check no longer fires */
> + 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;
>
> - /* no tests in the smoke set for now */
> + migration_test_add("/migration/mode/no-snapshot", test_mode_no_snapshot);
We should justify each test to be added into default set because it bloats
the test matrix.
This is a negative test, I think it should be only for --full. Note that
this will be covered by each PULL as long as when KVM enabled (IIUC when
top maintainers collect PRs these will be covered), and we also run --full
at least when preparing migration pulls.
Thanks,
>
> if (!env->full_set) {
> return;
>
> --
> 2.47.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes
2026-09-28 14:33 ` Peter Xu
@ 2026-09-29 13:28 ` Andrey Drobyshev
0 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-29 13:28 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On 9/28/26 5:33 PM, Peter Xu wrote:
> On Fri, Sep 18, 2026 at 04:46:55PM +0300, Andrey Drobyshev wrote:
>> Snapshots 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.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> tests/qtest/migration/cpr-tests.c | 36 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
>> index 0bec753b4ab..a5255193101 100644
>> --- a/tests/qtest/migration/cpr-tests.c
>> +++ b/tests/qtest/migration/cpr-tests.c
>> @@ -290,11 +290,45 @@ 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");
>> + 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);
>> + }
>> +
>> + /* Back in normal mode the mode check no longer fires */
>> + 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;
>>
>> - /* no tests in the smoke set for now */
>> + migration_test_add("/migration/mode/no-snapshot", test_mode_no_snapshot);
>
> We should justify each test to be added into default set because it bloats
> the test matrix.
>
> This is a negative test, I think it should be only for --full. Note that
> this will be covered by each PULL as long as when KVM enabled (IIUC when
> top maintainers collect PRs these will be covered), and we also run --full
> at least when preparing migration pulls.
Sounds reasonable to me. I'll move it under the --full category.
Thanks,
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 05/16] vhost-vsock: block CPR migration modes
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (3 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 04/16] tests/qtest/migration: add test for snapshot rejection in CPR modes Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 06/16] vhost: add vhost_reset_owner op Andrey Drobyshev
` (12 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
CPR 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
---
hw/virtio/vhost-vsock.c | 27 +++++++++++++++++++++++----
include/hw/virtio/vhost-vsock.h | 1 +
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index da244eb1657..503d248a617 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -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);
}
diff --git a/include/hw/virtio/vhost-vsock.h b/include/hw/virtio/vhost-vsock.h
index 84f4e727c70..a964d57e1bc 100644
--- a/include/hw/virtio/vhost-vsock.h
+++ b/include/hw/virtio/vhost-vsock.h
@@ -29,6 +29,7 @@ struct VHostVSock {
/*< private >*/
VHostVSockCommon parent;
VHostVSockConf conf;
+ Error *migration_blocker; /* CPR migration is not supported */
/*< public >*/
};
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 06/16] vhost: add vhost_reset_owner op
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (4 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 05/16] vhost-vsock: block CPR migration modes Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 07/16] vhost-vsock: don't reset connections during CPR Andrey Drobyshev
` (11 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Add 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.
Originally-by: Mark Kanda <mark.kanda@oracle.com>
Originally-by: Steve Sistare <steven.sistare@oracle.com>
Originally-by: Ben Chaney <bchaney@akamai.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost-kernel.c | 6 ++++++
include/hw/virtio/vhost-backend.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/hw/virtio/vhost-kernel.c b/hw/virtio/vhost-kernel.c
index 3390b48c6f1..55d316b88e9 100644
--- a/hw/virtio/vhost-kernel.c
+++ b/hw/virtio/vhost-kernel.c
@@ -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,
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index daa979a1aae..fb580aa417e 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -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;
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 07/16] vhost-vsock: don't reset connections during CPR
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (5 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 06/16] vhost: add vhost_reset_owner op Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-18 13:46 ` [PATCH v7 08/16] vhost-vsock: fix FD leak in realize() Andrey Drobyshev
` (10 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Migration 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.
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost-vsock.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 503d248a617..c582ac9fcc8 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -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)
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 08/16] vhost-vsock: fix FD leak in realize()
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (6 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 07/16] vhost-vsock: don't reset connections during CPR Andrey Drobyshev
@ 2026-09-18 13:46 ` Andrey Drobyshev
2026-09-18 13:47 ` [PATCH v7 09/16] vhost-vsock: preserve vhost FD during CPR Andrey Drobyshev
` (9 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:46 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
In 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).
Fixes: 384c2561bddf ("vhost-vsock: set vhostfd to non-blocking mode")
Fixes: 701544cfaf46 ("hw: replace qemu_set_nonblock()")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost-vsock.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index c582ac9fcc8..3ec20d87606 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -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);
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 09/16] vhost-vsock: preserve vhost FD during CPR
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (7 preceding siblings ...)
2026-09-18 13:46 ` [PATCH v7 08/16] vhost-vsock: fix FD leak in realize() Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-22 9:07 ` Stefano Garzarella
2026-09-18 13:47 ` [PATCH v7 10/16] vhost: factor out vhost_dev_init_backend() Andrey Drobyshev
` (8 subsequent siblings)
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
During 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost-vsock.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 3ec20d87606..6e89007b2ce 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -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);
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 09/16] vhost-vsock: preserve vhost FD during CPR
2026-09-18 13:47 ` [PATCH v7 09/16] vhost-vsock: preserve vhost FD during CPR Andrey Drobyshev
@ 2026-09-22 9:07 ` Stefano Garzarella
0 siblings, 0 replies; 35+ messages in thread
From: Stefano Garzarella @ 2026-09-22 9:07 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den
On Fri, Sep 18, 2026 at 04:47:00PM +0300, Andrey Drobyshev wrote:
>During 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.
>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
> hw/virtio/vhost-vsock.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 10/16] vhost: factor out vhost_dev_init_backend()
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (8 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 09/16] vhost-vsock: preserve vhost FD during CPR Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-18 13:47 ` [PATCH v7 11/16] vhost: make vhost_dev_cleanup() safe on a partially initialized device Andrey Drobyshev
` (7 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Split 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().
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost.c | 33 +++++++++++++++++++++++----------
include/hw/virtio/vhost.h | 22 ++++++++++++++++++++++
2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 68a8e404818..99561ac5868 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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()) {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 0a322efec1d..b2b644c0d1c 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -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
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 11/16] vhost: make vhost_dev_cleanup() safe on a partially initialized device
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (9 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 10/16] vhost: factor out vhost_dev_init_backend() Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-18 13:47 ` [PATCH v7 12/16] vhost: add vhost_dev_set_owner() / vhost_dev_reset_owner() helpers Andrey Drobyshev
` (6 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
A 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.
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 99561ac5868..dfdfd49da3f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 12/16] vhost: add vhost_dev_set_owner() / vhost_dev_reset_owner() helpers
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (10 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 11/16] vhost: make vhost_dev_cleanup() safe on a partially initialized device Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-18 13:47 ` [PATCH v7 13/16] vhost: add vhost_dev_is_initialized() helper Andrey Drobyshev
` (5 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Wrap 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.
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost.c | 20 +++++++++++++++++++-
include/hw/virtio/vhost.h | 25 +++++++++++++++++++++++++
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index dfdfd49da3f..56fa4345886 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index b2b644c0d1c..8127a9a5a2c 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -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
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 13/16] vhost: add vhost_dev_is_initialized() helper
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (11 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 12/16] vhost: add vhost_dev_set_owner() / vhost_dev_reset_owner() helpers Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-18 13:47 ` [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR Andrey Drobyshev
` (4 subsequent siblings)
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
The 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.
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost.c | 2 ++
include/hw/virtio/vhost.h | 14 ++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 56fa4345886..c01ccc800cb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -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;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 8127a9a5a2c..73ea130d7ee 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -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) {
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (12 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 13/16] vhost: add vhost_dev_is_initialized() helper Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-22 9:26 ` Stefano Garzarella
2026-09-18 13:47 ` [PATCH v7 15/16] tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec Andrey Drobyshev
` (3 subsequent siblings)
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
The 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.
- .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().
Suggested-by: Dongli Zhang <dongli.zhang@oracle.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/virtio/vhost-vsock.c | 157 ++++++++++++++++++++++++++++++++--------
include/hw/virtio/vhost-vsock.h | 2 +-
2 files changed, 128 insertions(+), 31 deletions(-)
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 6e89007b2ce..65ba573da4d 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -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)
@@ -85,14 +84,46 @@ static char *vhost_vsock_cpr_name(DeviceState *dev)
static int vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
+ VHostVSock *vsock = VHOST_VSOCK(vdev);
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) {
+ /*
+ * vsock->owner_reset is only set in .pre_save() on CPR migration
+ * source, after a successful RESET_OWNER op. If we end up here
+ * with this flag set - 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 (vsock->owner_reset) {
+ 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;
+ }
+ vsock->owner_reset = false;
+ }
+
ret = vhost_vsock_common_start(vdev);
if (ret < 0) {
return 0;
@@ -123,8 +154,44 @@ 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);
+ VHostVSock *vsock = VHOST_VSOCK(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() &&
+ !vsock->owner_reset) {
+ ret = vhost_dev_reset_owner(&vvc->vhost_dev);
+ if (ret < 0) {
+ error_report("vhost-vsock: vhost_reset_owner failed: %s",
+ strerror(-ret));
+ return ret;
+ }
+ vsock->owner_reset = true;
+ }
+
+ 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 +201,35 @@ 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;
+ }
+
+ 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);
}
diff --git a/include/hw/virtio/vhost-vsock.h b/include/hw/virtio/vhost-vsock.h
index a964d57e1bc..6e1c39383ef 100644
--- a/include/hw/virtio/vhost-vsock.h
+++ b/include/hw/virtio/vhost-vsock.h
@@ -29,7 +29,7 @@ struct VHostVSock {
/*< private >*/
VHostVSockCommon parent;
VHostVSockConf conf;
- Error *migration_blocker; /* CPR migration is not supported */
+ bool owner_reset; /* CPR released ownership; needs re-acquire */
/*< public >*/
};
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR
2026-09-18 13:47 ` [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR Andrey Drobyshev
@ 2026-09-22 9:26 ` Stefano Garzarella
2026-09-23 11:09 ` Andrey Drobyshev
0 siblings, 1 reply; 35+ messages in thread
From: Stefano Garzarella @ 2026-09-22 9:26 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den
On Fri, Sep 18, 2026 at 04:47:05PM +0300, Andrey Drobyshev wrote:
>The 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.
>
> - .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().
>
>Suggested-by: Dongli Zhang <dongli.zhang@oracle.com>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
> hw/virtio/vhost-vsock.c | 157 ++++++++++++++++++++++++++++++++--------
> include/hw/virtio/vhost-vsock.h | 2 +-
> 2 files changed, 128 insertions(+), 31 deletions(-)
>
>diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>index 6e89007b2ce..65ba573da4d 100644
>--- a/hw/virtio/vhost-vsock.c
>+++ b/hw/virtio/vhost-vsock.c
>@@ -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)
>@@ -85,14 +84,46 @@ static char *vhost_vsock_cpr_name(DeviceState *dev)
> static int vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
> {
> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>+ VHostVSock *vsock = VHOST_VSOCK(vdev);
> 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) {
>+ /*
>+ * vsock->owner_reset is only set in .pre_save() on CPR migration
>+ * source, after a successful RESET_OWNER op. If we end up here
>+ * with this flag set - 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 (vsock->owner_reset) {
>+ 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;
>+ }
>+ vsock->owner_reset = false;
>+ }
>+
> ret = vhost_vsock_common_start(vdev);
> if (ret < 0) {
> return 0;
>@@ -123,8 +154,44 @@ 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);
>+ VHostVSock *vsock = VHOST_VSOCK(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() &&
>+ !vsock->owner_reset) {
>+ ret = vhost_dev_reset_owner(&vvc->vhost_dev);
>+ if (ret < 0) {
>+ error_report("vhost-vsock: vhost_reset_owner failed: %s",
>+ strerror(-ret));
>+ return ret;
>+ }
>+ vsock->owner_reset = true;
>+ }
>+
>+ 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 +201,35 @@ 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;
>+ }
>+
>+ 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);
Why we need to set the CID? Isn't it preserved?
That said, I don't think it is wrong, it seems just superfluous, but
maybe there is a reason.
The rest LGTM:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR
2026-09-22 9:26 ` Stefano Garzarella
@ 2026-09-23 11:09 ` Andrey Drobyshev
0 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-23 11:09 UTC (permalink / raw)
To: Stefano Garzarella
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den
On 9/22/26 12:26 PM, Stefano Garzarella wrote:
> On Fri, Sep 18, 2026 at 04:47:05PM +0300, Andrey Drobyshev wrote:
>> The 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.
>>
>> - .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().
>>
>> Suggested-by: Dongli Zhang <dongli.zhang@oracle.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> hw/virtio/vhost-vsock.c | 157 ++++++++++++++++++++++++++++++++--------
>> include/hw/virtio/vhost-vsock.h | 2 +-
>> 2 files changed, 128 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>> index 6e89007b2ce..65ba573da4d 100644
>> --- a/hw/virtio/vhost-vsock.c
>> +++ b/hw/virtio/vhost-vsock.c
>> @@ -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)
>> @@ -85,14 +84,46 @@ static char *vhost_vsock_cpr_name(DeviceState *dev)
>> static int vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
>> + VHostVSock *vsock = VHOST_VSOCK(vdev);
>> 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) {
>> + /*
>> + * vsock->owner_reset is only set in .pre_save() on CPR migration
>> + * source, after a successful RESET_OWNER op. If we end up here
>> + * with this flag set - 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 (vsock->owner_reset) {
>> + 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;
>> + }
>> + vsock->owner_reset = false;
>> + }
>> +
>> ret = vhost_vsock_common_start(vdev);
>> if (ret < 0) {
>> return 0;
>> @@ -123,8 +154,44 @@ 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);
>> + VHostVSock *vsock = VHOST_VSOCK(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() &&
>> + !vsock->owner_reset) {
>> + ret = vhost_dev_reset_owner(&vvc->vhost_dev);
>> + if (ret < 0) {
>> + error_report("vhost-vsock: vhost_reset_owner failed: %s",
>> + strerror(-ret));
>> + return ret;
>> + }
>> + vsock->owner_reset = true;
>> + }
>> +
>> + 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 +201,35 @@ 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;
>> + }
>> +
>> + 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);
>
> Why we need to set the CID? Isn't it preserved?
>
> That said, I don't think it is wrong, it seems just superfluous, but
> maybe there is a reason.
>
> The rest LGTM:
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
You're right that the CID itself is preserved across CPR, the kernel
keeps it hashed. But the ownership of the device is NOT preserved. The
ownership must be claimed by the new QEMU. After we issue RESET_OWNER
in .pre_save() (old QEMU), it must be followed by SET_OWNER in
.post_load() (new QEMU). So setting the same CID value in this call is
indeed redundant, but that's not the main goal here.
Thanks,
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 15/16] tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (13 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 14/16] vhost-vsock: hand off device ownership across CPR Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-28 14:59 ` Peter Xu
2026-09-18 13:47 ` [PATCH v7 16/16] tests/functional: add vhost-vsock CPR test with a live guest Andrey Drobyshev
` (2 subsequent siblings)
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
Run 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/qtest/migration/cpr-tests.c | 93 ++++++++++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
index a5255193101..245ab39f54b 100644
--- a/tests/qtest/migration/cpr-tests.c
+++ b/tests/qtest/migration/cpr-tests.c
@@ -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)
{
@@ -348,4 +423,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);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 15/16] tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec
2026-09-18 13:47 ` [PATCH v7 15/16] tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec Andrey Drobyshev
@ 2026-09-28 14:59 ` Peter Xu
0 siblings, 0 replies; 35+ messages in thread
From: Peter Xu @ 2026-09-28 14:59 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Fri, Sep 18, 2026 at 04:47:06PM +0300, Andrey Drobyshev wrote:
> Run 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.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 16/16] tests/functional: add vhost-vsock CPR test with a live guest
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (14 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 15/16] tests/qtest/migration: add vhost-vsock tests for cpr-transfer and cpr-exec Andrey Drobyshev
@ 2026-09-18 13:47 ` Andrey Drobyshev
2026-09-28 15:01 ` Peter Xu
2026-09-22 9:30 ` [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Stefano Garzarella
2026-09-28 10:44 ` Andrey Drobyshev
17 siblings, 1 reply; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-18 13:47 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
andrey.drobyshev, maciej.szmigiero, dongli.zhang, mst, clg,
peterx, den, sgarzare
The 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.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/functional/x86_64/meson.build | 2 +
tests/functional/x86_64/test_vhost_vsock_cpr.py | 245 ++++++++++++++++++++++++
2 files changed, 247 insertions(+)
diff --git a/tests/functional/x86_64/meson.build b/tests/functional/x86_64/meson.build
index 0353b2af8e1..718bdc5195c 100644
--- a/tests/functional/x86_64/meson.build
+++ b/tests/functional/x86_64/meson.build
@@ -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',
]
diff --git a/tests/functional/x86_64/test_vhost_vsock_cpr.py b/tests/functional/x86_64/test_vhost_vsock_cpr.py
new file mode 100755
index 00000000000..035307fc82f
--- /dev/null
+++ b/tests/functional/x86_64/test_vhost_vsock_cpr.py
@@ -0,0 +1,245 @@
+#!/usr/bin/env python3
+#
+# Functional test for vhost-vsock over CPR (cpr-transfer and cpr-exec):
+# a host<->guest vsock transfer must survive repeated CPR migrations, in
+# either direction.
+#
+# Copyright (c) 2026 Virtuozzo International GmbH.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import fcntl
+import filecmp
+import os
+import subprocess
+import time
+
+from qemu.qmp import ConnectError, ExecInterruptedError
+from qemu.qmp.legacy import QEMUMonitorProtocol
+from qemu_test import (Asset, LinuxKernelTest, exec_command,
+ exec_command_and_wait_for_pattern, get_qemu_img,
+ skipIfMissingCommands)
+
+GUEST_CID = 4000000010 # we expect this CID to be free on the host
+VSOCK_PORT = 5000
+
+TRANSFER_SIZE = 512 * 1024 * 1024 # file size for host<->guest transfer
+CPR_INTERVAL = 0.2 # interval between CPR ops
+
+VHOST_SET_OWNER = 0xAF01 # _IO(VHOST_VIRTIO, 0x01)
+VHOST_RESET_OWNER = 0xAF02 # _IO(VHOST_VIRTIO, 0x02)
+
+# What a QMP connection raises when its QEMU goes away underneath it
+QMP_GONE = (OSError, EOFError, ConnectError, ExecInterruptedError)
+
+
+@skipIfMissingCommands('socat')
+class VhostVsockCPR(LinuxKernelTest):
+ # This should have passwordless root login, vsock support + socat
+ ASSET_DISKIMAGE = Asset(
+ ('https://cloud.debian.org/images/cloud/bookworm/20231210-1590/'
+ 'debian-12-nocloud-amd64-20231210-1590.qcow2'),
+ 'b94e3f34db59988a815b33dffc24f5a58502540efece2cc1ed15e0ac8749bb2e')
+
+ def require_vhost_vsock_cpr(self):
+ try:
+ fd = os.open('/dev/vhost-vsock', os.O_RDWR)
+ except OSError:
+ self.skipTest('/dev/vhost-vsock is not available')
+ try:
+ for name, request in (('VHOST_SET_OWNER', VHOST_SET_OWNER),
+ ('VHOST_RESET_OWNER', VHOST_RESET_OWNER)):
+ try:
+ fcntl.ioctl(fd, request)
+ except OSError:
+ self.skipTest(f'{name} is not supported for vhost-vsock')
+ finally:
+ os.close(fd)
+
+ def setUp(self):
+ super().setUp()
+ self.require_accelerator('kvm')
+ self.require_device('vhost-vsock-pci')
+ self.require_vhost_vsock_cpr()
+ self.set_machine('q35')
+
+ # Use Debian cloud image as a backing. During host<->guest
+ # transfer the guest will write data to its disk, so we start
+ # it with an overlay
+ self.disk = self.scratch_file('disk.qcow2')
+ subprocess.check_call([get_qemu_img(self), 'create', '-q', '-f',
+ '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
+
+ self.guest = None
+ self.qmp_path = None
+
+ def prepare_vm(self, vm, name, *extra_args):
+ # We don't want QEMUMachine.launch() set up QMP for us. In this
+ # case launch() waits for greeting - which won't be sent cause
+ # CPR target is reading CPR migration channel before QMP
+ qmp_path = os.path.join(self.sock_dir, f'{name}-qmp.sock')
+ vm.set_qmp_monitor(False)
+ vm.set_console()
+ vm.add_args('-accel', 'kvm', '-m', '1G',
+ '-object', 'memory-backend-memfd,id=ram,share=on,size=1G',
+ '-machine', 'memory-backend=ram,aux-ram-share=on',
+ '-drive', f'file={self.disk},if=none,id=disk,format=qcow2',
+ '-device', 'virtio-blk-pci,drive=disk',
+ '-device', f'vhost-vsock-pci,guest-cid={GUEST_CID}',
+ '-qmp', f'unix:{qmp_path},server=on,wait=off',
+ *extra_args)
+ return qmp_path
+
+ def wait_for(self, what, cond, timeout=60):
+ """Poll cond() until it returns something true, and return that"""
+ deadline = time.monotonic() + timeout
+ while True:
+ value = cond()
+ if value:
+ return value
+ self.assertLess(time.monotonic(), deadline,
+ f'timed out waiting for {what}')
+ time.sleep(0.2)
+
+ def qmp_connect(self, qmp_path):
+ """Connect to a QEMU which may still be starting up"""
+ def attempt():
+ try:
+ qmp = QEMUMonitorProtocol(qmp_path)
+ qmp.connect()
+ return qmp
+ except (OSError, ConnectError):
+ return None
+ return self.wait_for('QMP to come up', attempt)
+
+ def wait_migration(self, qmp):
+ def completed():
+ status = qmp.cmd('query-migrate').get('status')
+ self.assertNotEqual(status, 'failed', 'migration failed')
+ return status == 'completed'
+ self.wait_for('migration completion', completed)
+
+ def wait_running(self, qmp_path):
+ # Make sure we're dealing with the new QEMU after CPR
+ while True:
+ try:
+ with self.qmp_connect(qmp_path) as qmp:
+ self.wait_for('the VM to resume', lambda: qmp.cmd(
+ 'query-status')['status'] == 'running')
+ return
+ except QMP_GONE:
+ pass
+
+ def boot(self):
+ self.guest = self.vm
+ self.qmp_path = self.prepare_vm(self.guest, 'source')
+ self.guest.launch()
+ self.wait_for_console_pattern('login:')
+ exec_command_and_wait_for_pattern(self, 'root', 'root@localhost:~#')
+
+ def transfer(self, guest_cmd, host_cmd, cpr):
+ """
+ Run a socat transfer between the guest and the host, and keep
+ performing CPR migrations as long as it's in progress
+ """
+ exec_command(self, f'{guest_cmd} &', vm=self.guest)
+ with subprocess.Popen(host_cmd) as socat:
+ while socat.poll() is None:
+ time.sleep(CPR_INTERVAL)
+ cpr()
+ self.assertEqual(socat.returncode, 0, 'host socat failed')
+ exec_command_and_wait_for_pattern(self, 'wait %1; echo RC_$?',
+ 'RC_0', vm=self.guest)
+
+ def transfer_both_ways(self, cpr):
+ """
+ Send the payload to a file on the guest's disk, get it back into
+ another file on the host, then compare.
+ """
+ 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)
+ 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),
+ 'the data came back different')
+
+ def test_cpr_transfer(self):
+ """
+ Perform consecutive cpr-transfer migrations during vsock file
+ transfer, starting a new QEMU instance on each one, migrating into
+ 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))
+
+ 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}}])
+ self.wait_migration(qmp)
+ self.wait_running(target_qmp_path)
+
+ self.guest.shutdown()
+ self.guest, self.qmp_path = target, target_qmp_path
+ self.cpr_count += 1
+
+ self.boot()
+ self.transfer_both_ways(cpr)
+
+ def test_cpr_exec(self):
+ """
+ Perform consecutive cpr-exec migration during vsock file transfer,
+ saving devices' state to a file
+ """
+ state = self.scratch_file('cpr.state')
+
+ def cpr():
+ try:
+ with self.qmp_connect(self.qmp_path) as qmp:
+ qmp.cmd('migrate-set-parameters', **{
+ 'mode': 'cpr-exec', 'cpr-exec-command': exec_cmd})
+ qmp.cmd('migrate', uri=f'file:{state}')
+ self.wait_migration(qmp)
+ except QMP_GONE:
+ pass
+ self.wait_running(self.qmp_path)
+ self.cpr_count += 1
+
+ self.boot()
+ exec_cmd = [*self.guest._qemu_full_args, '-incoming', f'file:{state}']
+ self.transfer_both_ways(cpr)
+
+
+if __name__ == '__main__':
+ LinuxKernelTest.main()
--
2.47.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v7 16/16] tests/functional: add vhost-vsock CPR test with a live guest
2026-09-18 13:47 ` [PATCH v7 16/16] tests/functional: add vhost-vsock CPR test with a live guest Andrey Drobyshev
@ 2026-09-28 15:01 ` Peter Xu
0 siblings, 0 replies; 35+ messages in thread
From: Peter Xu @ 2026-09-28 15:01 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, den, sgarzare
On Fri, Sep 18, 2026 at 04:47:07PM +0300, Andrey Drobyshev wrote:
> The 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.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Acked-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 00/16] migration/cpr: support vhost-vsock devices
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (15 preceding siblings ...)
2026-09-18 13:47 ` [PATCH v7 16/16] tests/functional: add vhost-vsock CPR test with a live guest Andrey Drobyshev
@ 2026-09-22 9:30 ` Stefano Garzarella
2026-09-23 11:09 ` Andrey Drobyshev
2026-09-28 10:44 ` Andrey Drobyshev
17 siblings, 1 reply; 35+ messages in thread
From: Stefano Garzarella @ 2026-09-22 9:30 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den
On Fri, Sep 18, 2026 at 04:46:51PM +0300, Andrey Drobyshev wrote:
>v6 -> v7:
>
> * Patch 9:
> - cleanup closed vhostfd here rather than in #14;
> - add !cpr_incoming guard before saving CPR fd;
>
> * Patch 14:
> - remove closed vhostfd cleanup (moved to #9);
> - lift !cpr_incoming guard before saving CPR fd - not needed with an
> early return;
>
> * Patches 15-16 (NEW):
> - add qtests running cpr-transfer/cpr-exec with a vhost-vsock-pci
> device attached;
> - add functional test with a live guest doing host<->guest vsock
> transfers across a series of CPR migrations.
>
>v6: https://lore.kernel.org/qemu-devel/20260915-up-vsock-vhost-v5-v6-0-4d31b2dc6afd@virtuozzo.com
>
>Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>---
>Andrey Drobyshev (16):
> 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
> 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-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
I think I sent my A-b/R-b to all vhost/vsock related patches (if not
please let me know). As for the other patches, I don't think I know
enough about CPR.
So for vhost/vsock everything LGTM, I left just a comment more for my
curiosity.
Thanks for the patience and the work!
Stefano
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v7 00/16] migration/cpr: support vhost-vsock devices
2026-09-22 9:30 ` [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Stefano Garzarella
@ 2026-09-23 11:09 ` Andrey Drobyshev
0 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-23 11:09 UTC (permalink / raw)
To: Stefano Garzarella
Cc: qemu-devel, farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den
On 9/22/26 12:30 PM, Stefano Garzarella wrote:
> On Fri, Sep 18, 2026 at 04:46:51PM +0300, Andrey Drobyshev wrote:
>> v6 -> v7:
>>
>> * Patch 9:
>> - cleanup closed vhostfd here rather than in #14;
>> - add !cpr_incoming guard before saving CPR fd;
>>
>> * Patch 14:
>> - remove closed vhostfd cleanup (moved to #9);
>> - lift !cpr_incoming guard before saving CPR fd - not needed with an
>> early return;
>>
>> * Patches 15-16 (NEW):
>> - add qtests running cpr-transfer/cpr-exec with a vhost-vsock-pci
>> device attached;
>> - add functional test with a live guest doing host<->guest vsock
>> transfers across a series of CPR migrations.
>>
>> v6: https://lore.kernel.org/qemu-devel/20260915-up-vsock-vhost-v5-v6-0-4d31b2dc6afd@virtuozzo.com
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> Andrey Drobyshev (16):
>> 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
>> 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-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
>
> I think I sent my A-b/R-b to all vhost/vsock related patches (if not
> please let me know). As for the other patches, I don't think I know
> enough about CPR.
>
> So for vhost/vsock everything LGTM, I left just a comment more for my
> curiosity.
>
> Thanks for the patience and the work!
> Stefano
Thanks for your patience and your reviews!
Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 00/16] migration/cpr: support vhost-vsock devices
2026-09-18 13:46 [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Andrey Drobyshev
` (16 preceding siblings ...)
2026-09-22 9:30 ` [PATCH v7 00/16] migration/cpr: support vhost-vsock devices Stefano Garzarella
@ 2026-09-28 10:44 ` Andrey Drobyshev
17 siblings, 0 replies; 35+ messages in thread
From: Andrey Drobyshev @ 2026-09-28 10:44 UTC (permalink / raw)
To: qemu-devel
Cc: farosas, bchaney, mark.kanda, philmd, vsementsov,
maciej.szmigiero, dongli.zhang, mst, clg, peterx, den, sgarzare
On 9/18/26 4:46 PM, Andrey Drobyshev wrote:
> v6 -> v7:
>
> * Patch 9:
> - cleanup closed vhostfd here rather than in #14;
> - add !cpr_incoming guard before saving CPR fd;
>
> * Patch 14:
> - remove closed vhostfd cleanup (moved to #9);
> - lift !cpr_incoming guard before saving CPR fd - not needed with an
> early return;
>
> * Patches 15-16 (NEW):
> - add qtests running cpr-transfer/cpr-exec with a vhost-vsock-pci
> device attached;
> - add functional test with a live guest doing host<->guest vsock
> transfers across a series of CPR migrations.
>
> v6: https://lore.kernel.org/qemu-devel/20260915-up-vsock-vhost-v5-v6-0-4d31b2dc6afd@virtuozzo.com
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> Andrey Drobyshev (16):
> 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
> 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-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
>
> backends/hostmem-memfd.c | 5 +-
> backends/hostmem-shm.c | 5 +-
> backends/iommufd.c | 4 +-
> hw/vfio/container-legacy.c | 17 +-
> hw/vfio/cpr-legacy.c | 10 +-
> 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 | 57 ++++--
> include/hw/vfio/vfio-cpr.h | 6 +-
> include/hw/virtio/vhost-backend.h | 2 +
> include/hw/virtio/vhost-vsock.h | 1 +
> include/hw/virtio/vhost.h | 61 ++++++
> include/migration/cpr.h | 2 +-
> migration/cpr.c | 55 ++++--
> migration/options.c | 7 +
> system/physmem.c | 5 +-
> tests/functional/x86_64/meson.build | 2 +
> tests/functional/x86_64/test_vhost_vsock_cpr.py | 245 ++++++++++++++++++++++++
> tests/qtest/migration/cpr-tests.c | 129 ++++++++++++-
> tests/unit/meson.build | 1 +
> tests/unit/test-cpr.c | 98 ++++++++++
> 23 files changed, 846 insertions(+), 76 deletions(-)
> ---
> base-commit: 5f664cd37aec17e8145aa117d8da68f507edc8f1
> change-id: 20260908-up-vsock-vhost-v5-c781b39f1c1a
>
> Best regards,
> --
> Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Friendly ping
Thanks, Andrey
^ permalink raw reply [flat|nested] 35+ messages in thread