* [PATCH QEMU HCI-8.1 v5 01/36] VZ: migration/cpr-exec: fix Error leak in preserve_ram()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 16:58 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 02/36] migration/cpr-exec: restore RAM write access if exec fails Andrey Drobyshev
` (34 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
migrate_set_error() stores its own copy of the error, so free ours.
Fixes: a3745506daf9 ("VZ: migration/cpr-exec: mark saved RAM blocks with MADV_DOEXEC before execve")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/cpr-exec.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index edace3f0bd8..9fd3515753a 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -148,6 +148,7 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
migrate_set_error(s, err);
+ error_free(err);
return false;
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 02/36] migration/cpr-exec: restore RAM write access if exec fails
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 01/36] VZ: migration/cpr-exec: fix Error leak in preserve_ram() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 16:58 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC Andrey Drobyshev
` (33 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
With -cpr-ram-rdonly the RAM blocks are made read-only right before
execve(). If execve() fails, the VM resumes in the old QEMU with
read-only RAM and crashes on the first write. Make it writable again.
Fixes: 91da25f20471 ("migration/cpr-exec: add '-cpr-ram-rdonly' option to mark RAM blocks as RO #VSTOR-101579")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/cpr-exec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 9fd3515753a..c4bdf2cb99f 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -224,6 +224,9 @@ static void cpr_exec_cb(void *opaque)
error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
g_clear_pointer(&argv, g_strfreev);
cpr_exec_unpreserve_fds();
+ if (cpr_ram_rdonly) {
+ cpr_ram_restore_rw();
+ }
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 01/36] VZ: migration/cpr-exec: fix Error leak in preserve_ram() Andrey Drobyshev
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 02/36] migration/cpr-exec: restore RAM write access if exec fails Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:00 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 04/36] migration/cpr-exec: require a file or exec migration URI Andrey Drobyshev
` (32 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
The dma-buf fds are created without O_CLOEXEC, so they leak into any
exec'ed program. With cpr-exec, every update leaks one fd per mmapped
BAR, as new QEMU creates its own.
Fixes: 96163e0dbcb4 ("hw/vfio/region: Create dmabuf for PCI BAR per region")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/region.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 1deaef12fd0..b0332f27fcd 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -258,7 +258,7 @@ static bool vfio_region_create_dma_buf(VFIORegion *region, Error **errp)
dma_buf = (void *)feature->data;
*dma_buf = (struct vfio_device_feature_dma_buf) {
.region_index = region->nr,
- .open_flags = O_RDWR,
+ .open_flags = O_RDWR | O_CLOEXEC,
.nr_ranges = region->nr_mmaps,
};
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC Andrey Drobyshev
@ 2026-10-23 17:00 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:00 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio: BAR dma-buf fd stored in RAMBlock->fd is never closed
--
commit 73b2e092ad0c285255215af88f06299b94a431bd
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/region: create BAR dma-bufs with O_CLOEXEC
This patch adds O_CLOEXEC to the open_flags passed when
vfio_region_create_dma_buf() requests a dma-buf fd for a BAR region. The
aim is to stop those fds from leaking into exec'ed programs, for example
with cpr-exec.
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b8..b0332f27fcd79 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
[ ... ]
> @@ -258,7 +258,7 @@ static bool vfio_region_create_dma_buf(VFIORegion *region, Error **errp)
> dma_buf = (void *)feature->data;
> *dma_buf = (struct vfio_device_feature_dma_buf) {
> .region_index = region->nr,
> - .open_flags = O_RDWR,
> + .open_flags = O_RDWR | O_CLOEXEC,
> .nr_ranges = region->nr_mmaps,
> };
[Severity: Medium]
The commit message says "With cpr-exec, every update leaks one fd per
mmapped BAR". Does this change fix the leak completely? It looks like the
dma-buf fd is still never closed inside the running process.
vfio_region_create_dma_buf() stores the single returned fd in every
sub-region RAMBlock:
ram_block->fd = ret;
ram_block->fd_offset = region->mmaps[i].offset;
These RAMBlocks come from memory_region_init_ram_device_ptr() via
qemu_ram_alloc_from_ptr(), so they have RAM_PREALLOC set. In
reclaim_ramblock() that case takes the empty branch, so close(block->fd)
is never reached:
system/physmem.c:reclaim_ramblock() {
if (block->flags & RAM_PREALLOC) {
;
} else if (xen_enabled()) {
...
} else if (block->fd >= 0) {
qemu_ram_munmap(block->fd, block->host, block->max_length);
close(block->fd);
...
}
The region teardown paths do not close it either. vfio_subregion_unmap(),
vfio_region_exit() and vfio_region_finalize() only munmap() the BAR:
hw/vfio/region.c:vfio_region_finalize() {
...
if (region->mmaps[i].mmap) {
munmap(region->mmaps[i].mmap, region->mmaps[i].size);
}
...
}
hw/vfio/region.c has no close() call for this fd. Each mmap/unmap cycle,
device unplug or realize/unrealize therefore appears to leak one fd per
region, together with the kernel dma-buf object behind it. O_CLOEXEC does
not change that.
The one fd is also shared by all nr_mmaps RAMBlocks, so it has no single
owner. If RAM_PREALLOC were ever dropped, reclaim_ramblock() would call
close(block->fd) once per sub-region, which would be a double close when
nr_mmaps > 1.
Should the fd be closed explicitly when the region is unmapped or
finalized, and should the commit message be limited to the leak across
exec?
The series patch "VZ: vfio/region: preserve BAR mappings across cpr-exec"
only adds cpr ramblock save/delete of the mmap addresses. It does not
close the dma-buf fd either.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=3
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 04/36] migration/cpr-exec: require a file or exec migration URI
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (2 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:00 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 05/36] migration: close the CPR state if the outgoing channel fails to open Andrey Drobyshev
` (31 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
cpr-exec writes the device state to the migration URI and new QEMU
reads it back after the exec, so the URI must capture the data: a
file, or an exec command that stores it. Nothing checks that. With a
socket URI, e.g. a save or a live migration issued while the 'mode'
parameter is left at cpr-exec after a failed qemu-update, QEMU sends a
stream without the guest RAM to the peer, execs the stale cpr-exec
command, and the VM is lost.
Refuse cpr-exec with anything but a file or exec URI, the two types
the CPR documentation allows.
Fixes: a3eae205c601 ("migration: cpr-exec mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
docs/devel/migration/CPR.rst | 8 ++++----
migration/migration.c | 8 ++++++++
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/docs/devel/migration/CPR.rst b/docs/devel/migration/CPR.rst
index b6178568a85..2f34a6d7c9d 100644
--- a/docs/devel/migration/CPR.rst
+++ b/docs/devel/migration/CPR.rst
@@ -365,10 +365,10 @@ The VM must be started with the ``-machine aux-ram-share=on`` option.
Outgoing:
* Set the migration mode parameter to ``cpr-exec``.
* Set the ``cpr-exec-command`` parameter.
- * Issue the ``migrate`` command. It is recommended that the URI be
- a ``file`` type, but one can use other types such as ``exec``,
- provided the command captures all the data from the outgoing side,
- and provides all the data to the incoming side.
+ * Issue the ``migrate`` command. The URI must be a ``file`` type,
+ or an ``exec`` type, provided the command captures all the data
+ from the outgoing side, and provides all the data to the incoming
+ side. Other types are refused.
Incoming:
* You do not need to explicitly start new QEMU. It is started as
diff --git a/migration/migration.c b/migration/migration.c
index 2f8ef20c036..39f70328fc7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
return false;
}
+ if (migrate_mode() == MIG_MODE_CPR_EXEC &&
+ addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
+ addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
+ error_setg(errp,
+ "cpr-exec mode requires a file or exec migration URI");
+ return false;
+ }
+
return true;
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 05/36] migration: close the CPR state if the outgoing channel fails to open
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (3 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 04/36] migration/cpr-exec: require a file or exec migration URI Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:01 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 06/36] migration/cpr-exec: close the persisted state if the " Andrey Drobyshev
` (30 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
qmp_migrate() saves the CPR state before it opens the migration
channel. If the channel is a file or an exec command and it can't be
opened, qmp_migrate_finish() returns right away and migration_cleanup()
never runs, so the CPR state file stays open.
Close it there, next to the yank instance, which has the same problem.
Fixes: 624e6e654e11 ("migration: cpr-transfer mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/migration.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/migration/migration.c b/migration/migration.c
index 39f70328fc7..132f0d403b7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2412,6 +2412,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
if (!resume_requested) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
+ cpr_state_close();
migration_connect_set_error(s, local_err);
error_propagate(errp, local_err);
return;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 06/36] migration/cpr-exec: close the persisted state if the channel fails to open
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (4 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 05/36] migration: close the CPR state if the outgoing channel fails to open Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:01 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode Andrey Drobyshev
` (29 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
cpr_exec_persist_state() dups the memfd holding the CPR state so that
the dup survives exec(), and remembers it in the environment. Both
are released by cpr_exec_unpersist_state() alone, which runs only on
MIG_EVENT_PRECOPY_FAILED. The migration channel failing to open
doesn't raise it, so the dup is leaked and the stale environment
variable is left behind, once per attempt.
Release them there, next to the CPR state. That path runs in any
migration mode, so make cpr_exec_unpersist_state() a no-op if there's
nothing to release.
Fixes: a3eae205c60 ("migration: cpr-exec mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/cpr-exec.c | 4 +++-
migration/migration.c | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index c4bdf2cb99f..d4214e6b379 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -83,8 +83,10 @@ void cpr_exec_unpersist_state(void)
int mfd;
const char *val = g_getenv(CPR_EXEC_STATE_NAME);
+ if (!val) {
+ return;
+ }
g_unsetenv(CPR_EXEC_STATE_NAME);
- assert(val);
assert(!qemu_strtoi(val, NULL, 10, &mfd));
close(mfd);
}
diff --git a/migration/migration.c b/migration/migration.c
index 132f0d403b7..7c2fd704890 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2413,6 +2413,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
cpr_state_close();
+ cpr_exec_unpersist_state();
migration_connect_set_error(s, local_err);
error_propagate(errp, local_err);
return;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (5 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 06/36] migration/cpr-exec: close the persisted state if the " Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:03 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 08/36] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
` (28 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
migrate_add_blocker_modes() refuses a blocker while a migration is
running, whatever modes the blocker is for. So, while e.g. a normal
live migration is running, adding a blocker for cpr-transfer only is
refused as well, and whatever operation adds it fails, although the
blocker doesn't concern that migration at all.
Only refuse a blocker if the running migration is of one of its modes.
Snapshots are checked as before.
Fixes: fa3673e497a ("migration: per-mode blockers")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/migration.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 7c2fd704890..d5a21da3129 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1866,12 +1866,16 @@ int migrate_init(MigrationState *s, Error **errp)
return 0;
}
-static bool is_busy(Error **reasonp, Error **errp)
+static bool is_busy(Error **reasonp, Error **errp, int modes)
{
ERRP_GUARD();
- /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
- if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
+ /*
+ * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
+ * A running migration only matters if the blocker applies to its mode.
+ */
+ if (runstate_check(RUN_STATE_SAVE_VM) ||
+ (migration_is_running() && (modes & BIT(migrate_mode())))) {
error_propagate_prepend(errp, *reasonp,
"disallowing migration blocker "
"(migration/snapshot in progress) for: ");
@@ -1942,7 +1946,7 @@ int migrate_add_blocker_modes(Error **reasonp, Error **errp, MigMode mode, ...)
if (is_only_migratable(reasonp, errp, modes)) {
return -EACCES;
- } else if (is_busy(reasonp, errp)) {
+ } else if (is_busy(reasonp, errp, modes)) {
return -EBUSY;
}
return add_blockers(reasonp, errp, modes);
@@ -1952,7 +1956,7 @@ int migrate_add_blocker_internal(Error **reasonp, Error **errp)
{
int modes = BIT(MIG_MODE__MAX) - 1;
- if (is_busy(reasonp, errp)) {
+ if (is_busy(reasonp, errp, modes)) {
return -EBUSY;
}
return add_blockers(reasonp, errp, modes);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode Andrey Drobyshev
@ 2026-10-23 17:03 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:03 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: is_busy() infers running migration mode from mutable migrate_mode()
--
commit 7374a894384272ece1d26f3fa1c922acfae8eea7
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
migration: don't refuse a blocker over a migration of another mode
This patch changes is_busy() so that a running migration only causes a
migration blocker to be refused if the blocker applies to the mode of that
migration. A running snapshot (RUN_STATE_SAVE_VM) is still checked as
before.
> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> -static bool is_busy(Error **reasonp, Error **errp)
> +static bool is_busy(Error **reasonp, Error **errp, int modes)
> {
> ERRP_GUARD();
>
> - /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> - if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
> + /*
> + * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
> + * A running migration only matters if the blocker applies to its mode.
> + */
> + if (runstate_check(RUN_STATE_SAVE_VM) ||
> + (migration_is_running() && (modes & BIT(migrate_mode())))) {
[Severity: Low]
Can this check pick the wrong mode for the running migration?
migrate_mode() does not return the mode the running migration was
started with:
migration/options.c:migrate_mode() {
MigMode mode = cpr_get_incoming_mode();
if (mode == MIG_MODE_NONE) {
mode = migrate_get_current()->parameters.mode;
}
...
}
It returns either the CPR incoming mode or the live parameter value.
Nothing latches the mode in MigrationState at migrate_init().
The parameter can also be changed while a migration is running:
qmp_migrate_set_parameters()->migrate_params_apply()
s->parameters.mode = params->mode;
I did not see a migration_is_running() guard on this path.
qmp_migrate_set_capabilities() has such a guard, but the parameters path
does not.
Consider a normal-mode migration that is running when the user sets mode
to cpr-transfer. If migrate_add_blocker_modes() or
migrate_add_blocker_internal() is then called:
- A blocker for normal mode only is accepted, even though a normal
migration is running.
- A blocker for cpr-transfer only is refused with -EBUSY, even though
the running migration is not a cpr-transfer one.
If cpr_get_incoming_mode() is set while an outgoing migration is running,
migrate_mode() returns the incoming mode, and the same mismatch can occur.
Before this patch is_busy() ignored the mode, so this exposure is new.
migration_is_blocked() already reads migrate_mode() live, so the
convention is not new.
Would it be better to record the mode in MigrationState in
migrate_init() and use that value in is_busy()? Alternatively, could
changes to the mode parameter be rejected while migration_is_running()?
> error_propagate_prepend(errp, *reasonp,
> "disallowing migration blocker "
> "(migration/snapshot in progress) for: ");
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=7
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 08/36] migration: Plug memory leaks after migrate_set_error()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (6 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:02 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 09/36] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
` (27 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Markus Armbruster <armbru@redhat.com>
migrate_set_error(s, err) stores a copy of @err in @s. The original
@err is not freed. Most callers free it immediately. Some callers
free it later, or pass it on. And some leak it. Fix those.
Perhaps migrate_set_error(s, err) should take ownership of @err. The
callers that free it immediately would become simpler, and avoid a
copy and a deallocation. The others would have to pass
error_copy(err).
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit 93817ec39656.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Link: https://lore.kernel.org/r/20251115083500.2753895-2-armbru@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/cpr-exec.c | 3 ++-
migration/multifd.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index d4214e6b379..5a885422154 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -233,11 +233,12 @@ static void cpr_exec_cb(void *opaque)
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
migrate_set_error(s, err);
+ error_free(err);
+ err = NULL;
/* Note, we can go from state COMPLETED to FAILED */
migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
- err = NULL;
if (!migration_block_activate(&err)) {
/* error was already reported */
error_free(err);
diff --git a/migration/multifd.c b/migration/multifd.c
index 5a723ae25d2..e369421b3ff 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -966,6 +966,7 @@ bool multifd_send_setup(void)
if (!multifd_new_send_channel_create(p, &local_err)) {
migrate_set_error(s, local_err);
+ error_free(local_err);
ret = -1;
}
}
@@ -990,6 +991,7 @@ bool multifd_send_setup(void)
ret = multifd_send_state->ops->send_setup(p, &local_err);
if (ret) {
migrate_set_error(s, local_err);
+ error_free(local_err);
goto err;
}
assert(p->iov);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 09/36] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (7 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 08/36] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:04 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 10/36] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
` (26 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: "Tycho Andersen (AMD)" <tycho@kernel.org>
While working on savevm/loadvm for a new vfio device, I encountered the
crash below. Since vfio_connect_kvm_msi_virq() didn't check the ->use flag
for the vector, it would pass an unused vector down to
vfio_cpr_load_vector_fd() which would crash.
Fix this by checking the ->use flag along with the virq number to detect
whether a vector is valid or not.
Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x0000555555a891ef in vfio_cpr_load_vector_fd (vdev=vdev@entry=0x0,
name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1) at ../hw/vfio/cpr.c:44
44 g_autofree char *fdname = STRDUP_VECTOR_FD_NAME(vdev, name);
(gdb) bt
(vdev=vdev@entry=0x0, name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1)
at ../hw/vfio/cpr.c:44
(vdev=0x0, e=e@entry=0x5555586971b4, name=name@entry=0x555555eeb27e "kvm_interrupt", nr=nr@entry=1, errp=errp@entry=0x0) at ../hw/vfio/pci.c:79
at ../hw/vfio/pci.c:601
at ../hw/vfio/pci.c:597
at ../hw/vfio/pci.c:3088
at ../hw/vfio/migration.c:278
(f=f@entry=0x5555574a43d0, se=se@entry=0x5555591edd40, errp=errp@entry=0x7fffffffe130)
at ../migration/savevm.c:971
(f=f@entry=0x5555574a43d0, type=type@entry=4 '\004', errp=errp@entry=0x7fffffffe130)
at ../migration/savevm.c:2654
(f=f@entry=0x5555574a43d0, mis=mis@entry=0x5555571de5a0, errp=0x7fffffffe130,
errp@entry=0x555557157c10 <error_fatal>) at ../migration/savevm.c:2973
(f=f@entry=0x5555574a43d0, errp=errp@entry=0x555557157c10 <error_fatal>)
at ../migration/savevm.c:3058
(name=0x7fffffffecc9 "foo", vmstate=vmstate@entry=0x0, has_devices=has_devices@entry=false, devices=devices@entry=0x0, errp=errp@entry=0x555557157c10 <error_fatal>) at ../migration/savevm.c:3452
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit f498c21b3595.
Fixes: 30edcb4d4e7a ("vfio-pci: preserve MSI")
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260727150038.2684512-1-tycho@kernel.org
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fd73a0fdf87..5e281b2e1d9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -589,7 +589,7 @@ static void vfio_connect_kvm_msi_virq(VFIOMSIVector *vector, int nr)
{
const char *name = "kvm_interrupt";
- if (vector->virq < 0) {
+ if (!vector->use || vector->virq < 0) {
return;
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 10/36] vfio/pci: Reject invalid MSI-X Table and PBA BIR values
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (8 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 09/36] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:03 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 11/36] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
` (25 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Cédric Le Goater <clg@redhat.com>
The 3-bit MSI-X BIR fields permit values 0-7, but VFIOPCIDevice::bars[]
only contains BAR0-BAR5 (6 entries). An invalid BIR value of 6 or 7
can cause an out-of-bounds array access (CWE-129) in vfio_msix_early_setup().
Add range checks immediately after extracting the BIR values.
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit 9daa7347ff25.
Reported-by: Feifan Qian <bea1e@proton.me>
Fixes: 65501a745dba ("vfio: vfio-pci device assignment driver")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3878
Reviewed-by: Alex Williamson <alex@shazbot.org>
Link: https://lore.kernel.org/qemu-devel/20260706161334.2165482-1-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5e281b2e1d9..d930ef46309 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1791,6 +1791,14 @@ static bool vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
+ if (msix->table_bar >= ARRAY_SIZE(vdev->bars) ||
+ msix->pba_bar >= ARRAY_SIZE(vdev->bars)) {
+ error_setg(errp, "invalid MSI-X BIR, table_bar=%d pba_bar=%d",
+ msix->table_bar, msix->pba_bar);
+ g_free(msix);
+ return false;
+ }
+
ret = vfio_device_get_irq_info(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX,
&irq_info);
if (ret < 0) {
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 11/36] vfio/pci: Fix information leak in vfio_rom_read()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (9 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 10/36] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:04 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 12/36] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
` (24 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Cédric Le Goater <clg@redhat.com>
vfio_rom_read() can leak uninitialized stack data to the guest when a
read occurs at the end boundary of the ROM BAR.
Fix this by initializing the val union to 0xff, matching the ROM
initialization pattern used in vfio_pci_load_rom().
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit 402fd5a75659.
Reviewed-by: Philippe Mathieu-Daudé <philmd@mailo.com>
Reviewed-by: Mario Casquero <mcasquer@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260602074446.1206892-1-clg@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d930ef46309..fa2ca0d779d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1153,7 +1153,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
uint16_t word;
uint32_t dword;
uint64_t qword;
- } val;
+ } val = { .qword = ~0ULL };
uint64_t data = 0;
/* Load the ROM lazily when the guest tries to read it */
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 12/36] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (10 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 11/36] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:05 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 13/36] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
` (23 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Mario Casquero <mcasquer@redhat.com>
When vfio_device_get_region_info() fails in vfio_pci_load_rom(),
the function returns without setting vdev->rom_read_failed to true,
and without allocating vdev->rom. This leaves vdev->rom as NULL.
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit c822ecd80661.
Signed-off-by: Mario Casquero <mcasquer@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260602070857.356526-1-mcasquer@redhat.com
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/pci.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fa2ca0d779d..bea478d521f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1039,7 +1039,7 @@ static void vfio_update_msi(VFIOPCIDevice *vdev)
}
}
-static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
+static bool vfio_pci_load_rom(VFIOPCIDevice *vdev)
{
VFIODevice *vbasedev = &vdev->vbasedev;
struct vfio_region_info *reg_info = NULL;
@@ -1053,7 +1053,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
if (ret != 0) {
error_report("vfio: Error getting ROM info: %s", strerror(-ret));
- return;
+ return false;
}
trace_vfio_pci_load_rom(vbasedev->name, (unsigned long)reg_info->size,
@@ -1064,12 +1064,11 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
vdev->rom_offset = reg_info->offset;
if (!vdev->rom_size) {
- vdev->rom_read_failed = true;
error_report("vfio-pci: Cannot read device rom at %s", vbasedev->name);
error_printf("Device option ROM contents are probably invalid "
"(check dmesg).\nSkip option ROM probe with rombar=0, "
"or load from file with romfile=\n");
- return;
+ return false;
}
vdev->rom = g_malloc(size);
@@ -1125,6 +1124,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
data[6] = -csum;
}
}
+
+ return true;
}
/* "Raw" read of underlying config space. */
@@ -1158,7 +1159,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
/* Load the ROM lazily when the guest tries to read it */
if (unlikely(!vdev->rom && !vdev->rom_read_failed)) {
- vfio_pci_load_rom(vdev);
+ vdev->rom_read_failed = !vfio_pci_load_rom(vdev);
}
memcpy(&val, vdev->rom + addr,
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 13/36] vfio/pci: Grow buffer in vfio_pci_host_match()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (11 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 12/36] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:05 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 14/36] q35: Fix migration of SMRAM state Andrey Drobyshev
` (22 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Each field of PCIHostDeviceAddress is an unsigned int, therefore
while a valid address is limited to 13 characters, an invalid
address could exceed the specified format, up to:
ffffffff:ffffffff:ffffffff.ffffffff<NUL>
This requires 36 characters with the terminator.
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit 1785bf9685ec.
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>
Message-ID: <20260305-nvme-v4-2-b65b9de1839f@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bea478d521f..50498148275 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2691,7 +2691,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
{
- char tmp[13];
+ char tmp[36];
sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
addr->bus, addr->slot, addr->function);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 14/36] q35: Fix migration of SMRAM state
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (12 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 13/36] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:07 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 15/36] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
` (21 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Igor Mammedov <imammedo@redhat.com>
When migrating, dst QEMU by default has SMRAM unlocked,
and since wmask is not migrated, the migrated value of
MCH_HOST_BRIDGE_F_SMBASE in config space fall to prey of
mch_update_smbase_smram()
...
if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
*reg = 0x00;
and is getting cleared and leads to unlocked smram
on dst even if on source it's been locked.
As Andrey has pointed out [1], we should derive wmask
from config and not other way around.
Drop offending chunk and resync wmask based on MCH_HOST_BRIDGE_F_SMBASE
register value. That would preserve the register during
migration and set smram regions into corresponding state.
What that changes is:
that it would let guest write junk values in register
(with no apparent effect) until it's stumbles upon
reserved 0x1 [|] 0x2 values, at which point it
would be only possible to lock register and trigger
switch to SMRAM blackhole in CPU AS.
While at it, fix up test by removing junk discard before negotiation hunk.
PS2:
Instead of adding a dedicated post_load handler for it,
reuse mch_update->mch_update_smbase_smram call chain
that is called on write/reset/post_load to be consistent
with how we handle mch registers.
PS3:
for prosterity here is erro message Andrey got due to this bug:
qemu: vfio_container_dma_map(0x..., 0x0, 0xa0000, 0x....) = -22 (Invalid argument)
qemu: hardware error: vfio: DMA mapping failed, unable to continue
1) https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit 66cf169e29b0.
Fixes: f404220e279c ("q35: implement 128K SMRAM at default SMBASE address")
Reported-by: Andrey Ryabinin <arbn@yandex-team.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andrey Ryabinin <arbn@yandex-team.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20251211165454.288476-1-imammedo@redhat.com>
---
hw/pci-host/q35.c | 25 +++++++++++--------------
tests/qtest/q35-test.c | 6 ------
2 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index a708758d361..946342ba585 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
}
if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
- pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
- MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
*reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
return;
}
/*
- * default/reset state, discard written value
- * which will disable SMRAM balackhole at SMBASE
+ * reg value can come from register write/reset/migration source,
+ * update wmask to be in sync with it regardless of source
*/
- if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
- *reg = 0x00;
+ if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ return;
}
-
- memory_region_transaction_begin();
if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
- /* disable all writes */
- pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
- ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ /* lock register at 0x2 and disable all writes */
+ pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
*reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
- lck = true;
- } else {
- lck = false;
}
+
+ lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
+ memory_region_transaction_begin();
memory_region_set_enabled(&mch->smbase_blackhole, lck);
memory_region_set_enabled(&mch->smbase_window, lck);
memory_region_transaction_commit();
diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
index 62fff49fc89..4e3a4457f62 100644
--- a/tests/qtest/q35-test.c
+++ b/tests/qtest/q35-test.c
@@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
- /* check that writing junk to 0x9c before before negotiating is ignored */
- for (i = 0; i < 0xff; i++) {
- qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
- g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
- }
-
/* enable SMRAM at SMBASE */
qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 14/36] q35: Fix migration of SMRAM state
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 14/36] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-23 17:07 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:07 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
> From: Igor Mammedov <imammedo@redhat.com>
>
> When migrating, dst QEMU by default has SMRAM unlocked,
> and since wmask is not migrated, the migrated value of
> MCH_HOST_BRIDGE_F_SMBASE in config space fall to prey of
>
> mch_update_smbase_smram()
> ...
> if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> *reg = 0x00;
>
> and is getting cleared and leads to unlocked smram
> on dst even if on source it's been locked.
>
> As Andrey has pointed out [1], we should derive wmask
> [ ... ]
> Reported-by: Andrey Ryabinin <arbn@yandex-team.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Andrey Ryabinin <arbn@yandex-team.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=14
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 15/36] migration: ensure APIC is loaded prior to VFIO PCI devices
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (13 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 14/36] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:06 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 16/36] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
` (20 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Yanfei Xu <yanfei.xu@bytedance.com>
The load procedure of VFIO PCI devices involves setting up IRT
for each VFIO PCI devices. This requires determining whether an
interrupt is single-destination interrupt to decide between
Posted Interrupt(PI) or remapping mode for the IRTE. However,
determining this may require accessing the VM's APIC registers.
For example:
ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irqs)
...
kvm_arch_irq_bypass_add_producer
kvm_x86_call(pi_update_irte)
vmx_pi_update_irte
kvm_intr_is_single_vcpu
If the LAPIC has not been loaded yet, interrupts will use remapping
mode. To prevent the fallback of interrupt mode, keep APIC is always
loaded prior to VFIO PCI devices.
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit d943cef76090.
Signed-off-by: Yicong Shen <shenyicong.1023@bytedance.com>
Signed-off-by: Yanfei Xu <yanfei.xu@bytedance.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/r/20250818131127.1021648-1-yanfei.xu@bytedance.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/intc/apic_common.c | 1 +
include/migration/vmstate.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 37a7a7019d3..394fe020134 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -379,6 +379,7 @@ static const VMStateDescription vmstate_apic_common = {
.pre_load = apic_pre_load,
.pre_save = apic_dispatch_pre_save,
.post_load = apic_dispatch_post_load,
+ .priority = MIG_PRI_APIC,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(apicbase, APICCommonState),
VMSTATE_UINT8(id, APICCommonState),
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 6839117be87..c3d156c514f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -163,6 +163,7 @@ typedef enum {
MIG_PRI_IOMMU, /* Must happen before PCI devices */
MIG_PRI_PCI_BUS, /* Must happen before IOMMU */
MIG_PRI_VIRTIO_MEM, /* Must happen before IOMMU */
+ MIG_PRI_APIC, /* Must happen before PCI devices */
MIG_PRI_GICV3_ITS, /* Must happen before PCI devices */
MIG_PRI_GICV3, /* Must happen before the ITS */
MIG_PRI_MAX,
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 16/36] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (14 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 15/36] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:09 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
` (19 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Arun Menon <armenon@redhat.com>
error_fatal is passed to vmstate_load_state() and vmstate_save_state()
functions. This was introduced in commit c632ffbd74. This would exit(1)
on error, and therefore does not allow to propagate the error back to
the caller.
To maintain consistency with prior error handling i.e. either propagating
the error to the caller or reporting it, we must set the error within a
local Error object instead of using error_fatal.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Arun Menon <armenon@redhat.com>
Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com
[peterx: always uninit var ret, per Akihiko]
[peterx: touchups on line ordering, spacings etc.]
andrey.drobyshev:
This commit was cherry-picked from the upstream commit 986c3292c6b8,
adapted for the hci-dev base, which predates the upstream commit
bcb536cabe10 ("error: Kill @error_warn"): virtio_gpu_load() has no
'err' local there yet, so it is declared here.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
hw/display/virtio-gpu.c | 22 +++++++++++++++-------
hw/pci/pci.c | 15 +++++++++++++--
hw/s390x/virtio-ccw.c | 17 +++++++++++++++--
hw/scsi/spapr_vscsi.c | 10 ++++++++--
hw/virtio/virtio-mmio.c | 15 +++++++++++++--
hw/virtio/virtio-pci.c | 15 +++++++++++++--
hw/virtio/virtio.c | 10 +++++++---
7 files changed, 84 insertions(+), 20 deletions(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 477ec700a13..4bc8c778508 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1223,7 +1223,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
{
VirtIOGPU *g = opaque;
struct virtio_gpu_simple_resource *res;
- int i;
+ Error *err = NULL;
+ int i, ret;
/* in 2d mode we should never find unprocessed commands here */
assert(QTAILQ_EMPTY(&g->cmdq));
@@ -1246,8 +1247,12 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
}
qemu_put_be32(f, 0); /* end of list */
- return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL,
- &error_fatal);
+ ret = vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL,
+ &err);
+ if (ret < 0) {
+ error_report_err(err);
+ }
+ return ret;
}
static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g,
@@ -1283,9 +1288,10 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
const VMStateField *field)
{
VirtIOGPU *g = opaque;
+ Error *err = NULL;
struct virtio_gpu_simple_resource *res;
uint32_t resource_id, pformat;
- int i;
+ int i, ret;
g->hostmem = 0;
@@ -1344,9 +1350,11 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
}
/* load & apply scanout state */
- vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
-
- return 0;
+ ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
+ if (ret < 0) {
+ error_report_err(err);
+ }
+ return ret;
}
static int virtio_gpu_blob_save(QEMUFile *f, void *opaque, size_t size,
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 5d7d17f6529..50f062045a9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -926,21 +926,32 @@ const VMStateDescription vmstate_pci_device = {
void pci_device_save(PCIDevice *s, QEMUFile *f)
{
+ Error *local_err = NULL;
+ int ret;
+
/* Clear interrupt status bit: it is implicit
* in irq_state which we are saving.
* This makes us compatible with old devices
* which never set or clear this bit. */
s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
- vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal);
+ ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
/* Restore the interrupt status bit. */
pci_update_irq_status(s);
}
int pci_device_load(PCIDevice *s, QEMUFile *f)
{
+ Error *local_err = NULL;
int ret;
+
ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
- &error_fatal);
+ &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
/* Restore the interrupt status bit. */
pci_update_irq_status(s);
return ret;
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 4cb1ced001a..4a3ffb84f8f 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1130,13 +1130,26 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f)
static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &error_fatal);
+ Error *local_err = NULL;
+ int ret;
+
+ ret = vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
}
static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
+ Error *local_err = NULL;
+ int ret;
+
+ ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
+ return ret;
}
static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index f0a7dd2b882..a6591319db7 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -628,10 +628,16 @@ static const VMStateDescription vmstate_spapr_vscsi_req = {
static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq)
{
vscsi_req *req = sreq->hba_private;
- assert(req->active);
+ Error *local_err = NULL;
+ int rc;
- vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal);
+ assert(req->active);
+ rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
+ if (rc < 0) {
+ error_report_err(local_err);
+ return;
+ }
trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
req->cur_desc_offset);
}
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index fb58c364527..c05c00bcd4a 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -612,15 +612,26 @@ static const VMStateDescription vmstate_virtio_mmio = {
static void virtio_mmio_save_extra_state(DeviceState *opaque, QEMUFile *f)
{
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+ Error *local_err = NULL;
+ int ret;
- vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &error_fatal);
+ ret = vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
}
static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
{
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+ Error *local_err = NULL;
+ int ret;
- return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
+ ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
+ return ret;
}
static bool virtio_mmio_has_extra_state(DeviceState *opaque)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7273da4c018..c276fbb017c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -189,15 +189,26 @@ static bool virtio_pci_has_extra_state(DeviceState *d)
static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f)
{
VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+ Error *local_err = NULL;
+ int ret;
- vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &error_fatal);
+ ret = vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
}
static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
{
VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+ Error *local_err = NULL;
+ int ret;
- return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
+ ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
+ return ret;
}
static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a7063d5555e..d4b2e67c274 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3016,7 +3016,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff);
- int i;
+ int i, ret;
Error *local_err = NULL;
if (k->save_config) {
@@ -3061,7 +3061,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
}
if (vdc->vmsd) {
- int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
+ ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
if (ret) {
error_report_err(local_err);
return ret;
@@ -3069,7 +3069,11 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
}
/* Subsections */
- return vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &error_fatal);
+ ret = vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &local_err);
+ if (ret < 0) {
+ error_report_err(local_err);
+ }
+ return ret;
}
/* A wrapper for use as a VMState .put function */
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 16/36] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 16/36] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-23 17:09 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:09 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: void save helpers swallow vmstate_save_state() errors
--
commit b01216a0ff02c1cecda9723eaf8284b060c9fe21
Author: Arun Menon <armenon@redhat.com>
migration: Fix regression of passing error_fatal into vmstate_load_state()
This commit replaces error_fatal with a local Error object in the callers
of vmstate_load_state() and vmstate_save_state(). On failure it reports
the error with error_report_err() and, where the function returns a value,
propagates that value to the caller. This is a cherry-pick of upstream
commit 986c3292c6b8.
Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
[ ... ]
> void pci_device_save(PCIDevice *s, QEMUFile *f)
> {
> + Error *local_err = NULL;
> + int ret;
> +
> /* Clear interrupt status bit: it is implicit
> * in irq_state which we are saving.
> * This makes us compatible with old devices
> * which never set or clear this bit. */
> s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> - vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal);
> + ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + }
> /* Restore the interrupt status bit. */
> pci_update_irq_status(s);
> }
[Severity: Medium]
If vmstate_save_state() fails here, pci_device_save() is a void function,
so the failure is only printed and then dropped. Can the source keep
writing the stream and carry on with the migration or savevm?
Previously error_fatal would exit the process. Now the caller has no way to
learn of the error.
Looking at vmstate_save_state_v(), the put path only calls error_setg()
and returns ret:
migration/vmstate.c:vmstate_save_state_v() {
...
ret = inner_field->info->put(f, curr_elem, size,
inner_field, vmdesc_loop);
if (ret < 0) {
error_setg(errp, "put failed");
}
...
}
It never calls qemu_file_set_error(), so the QEMUFile does not record the
failure either. Is anything downstream able to detect the failed save?
The destination may later fail, or it may load inconsistent device state.
The commit message says the goal is "either propagating the error to the
caller or reporting it". For this void helper the failure is only
reported. Should the commit message mention that the process no longer
exits on a failed save?
The same pattern appears in virtio_ccw_save_config(),
virtio_mmio_save_extra_state() and virtio_pci_save_extra_state(). Their
save_config and save_extra_state callbacks return void, so virtio_save()
cannot see the failure.
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
[ ... ]
> + rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
> + if (rc < 0) {
> + error_report_err(local_err);
> + return;
> + }
> trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
> req->cur_desc_offset);
[Severity: Low]
In vscsi_save_request(), the failure is not propagated to the caller
because the function returns void. Is that intended?
The early return also skips trace_spapr_vscsi_save_request() on the error
path.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=16
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (15 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 16/36] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:11 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier Andrey Drobyshev
` (18 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We may call error_setg twice on same errp if inner
vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
crash on assertion in error_setv().
andrey.drobyshev:
This commit is cherry-picked as-is, with no modifications, from the
upstream commit d41ce10d0f5a.
Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/vmstate.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index aea41c09946..04903131257 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
} else {
ret = inner_field->info->put(f, curr_elem, size,
inner_field, vmdesc_loop);
+ if (ret < 0) {
+ error_setg(errp, "put failed");
+ }
}
written_bytes = qemu_file_transferred(f) - old_offset;
@@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
}
if (ret) {
- error_setg(errp, "Save of field %s/%s failed",
- vmsd->name, field->name);
+ error_prepend(errp, "Save of field %s/%s failed: ",
+ vmsd->name, field->name);
if (vmsd->post_save) {
vmsd->post_save(opaque);
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
@ 2026-10-23 17:11 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:11 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() error_prepend() on unset errp if put() returns >0
--
commit 02288718e44ad995a2dd7f4f453ddcf9d949e513
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
migration: vmstate_save_state_v: fix double error_setg
This commit avoids calling error_setg() twice on the same errp in
vmstate_save_state_v(). It does this by setting the Error only when
info->put() fails with a negative return, and by changing the later
failure path to use error_prepend() instead of error_setg().
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index aea41c099463..04903131257b 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
[ ... ]
> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> } else {
> ret = inner_field->info->put(f, curr_elem, size,
> inner_field, vmdesc_loop);
> + if (ret < 0) {
> + error_setg(errp, "put failed");
> + }
> }
>
> written_bytes = qemu_file_transferred(f) - old_offset;
[ ... ]
> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> if (ret) {
> - error_setg(errp, "Save of field %s/%s failed",
> - vmsd->name, field->name);
> + error_prepend(errp, "Save of field %s/%s failed: ",
> + vmsd->name, field->name);
> if (vmsd->post_save) {
> vmsd->post_save(opaque);
> }
[Severity: Medium]
Can error_prepend() run here with *errp still unset?
The Error is only set after put() when ret < 0, but this failure block
runs for any non-zero ret. If info->put() returns a positive value,
nothing sets *errp, and error_prepend() ends up in error_vprepend(),
which checks only !errp before reading (*errp)->msg:
util/error.c:error_vprepend() {
...
if (!errp) {
return;
}
...
g_string_append(newmsg, (*errp)->msg);
}
Before this patch, error_setg() ran for every non-zero ret, so a
positive return was handled.
A positive ret can also come from the nested path. put_tmp() in
migration/vmstate-types.c calls vmstate_save_state() with a private
local_err and returns the raw inner ret:
ret = vmstate_save_state(f, vmsd, tmp, vmdesc, &local_err);
if (ret) {
error_report_err(local_err);
}
g_free(tmp);
return ret;
put_qtailq(), put_qlist() and put_gtree() follow the same pattern. If a
pre_save or post_save hook in such a nested vmsd returns a positive
value, vmstate_save_state_v() treats it as failure and the outer errp
stays unset.
Positive pre_save returns do exist in the tree, for example
qxl_pre_save() in hw/display/qxl.c returns 1. That hook is only on a
top-level vmsd today, where the pre-save failure path sets errp. I did
not find an in-tree put() or nested vmsd that currently triggers this,
so the problem looks latent.
If errp were &error_warn, error_setg() would report and free the Error
without storing it, and error_prepend() would then crash the same way.
I found no caller that passes it.
Would it be safer to set the Error for any non-zero ret from put(), for
example with if (ret), or to only call error_prepend() when *errp is
set?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=17
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (16 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 17/36] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:13 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 19/36] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
` (17 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Fabiano Rosas <farosas@suse.de>
Move the register_instance call to migrate_prepare() so it can be
paired with the unregister_instance at migration_cleanup(). Otherwise,
the cleanup cannot be run when cpr_state_save() fails because the
instance is registered only after it.
When resuming from a paused postcopy migration, migrate_prepare()
returns early, but migration_cleanup() doesn't run, so the yank will
remain paired.
andrey.drobyshev:
This commit was cherry-picked from the upstream commit 7e714392437c,
adapted for the hci-dev base, which predates the upstream
migration_connect_set_error() -> migration_connect_error_propagate()
rename (77c82edadbe0): the error propagation calls in qmp_migrate()
and qmp_migrate_finish() keep the older spelling.
Also fixed here: upstream left the yank instance registered when
qmp_migrate_finish() fails before connecting (nothing calls
migration_cleanup() then), so the next migrate command aborted on
yank_register_instance(&error_abort); and the unregister at the out:
label ran on a resume, which never registered the instance. Upstream
fixed both in 6b587af5ec9d ("migration: Fold migration_cleanup() into
migration_connect_error_propagate()"), which needs the above rework;
this is the equivalent for our base.
Also register the instance with errp, before migrate_init(), instead
of with &error_abort: a duplicate instance, e.g. a previous migration
still being cleaned up, then fails the migrate command instead of
aborting QEMU. And refuse migrate while an incoming postcopy
migration is still in progress: the guest already runs then, but the
incoming side still holds the yank instance.
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration.c | 43 +++++++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index d5a21da3129..cc88f70ae59 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2186,6 +2186,15 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
return false;
}
+ /*
+ * The guest already runs during incoming postcopy, but the incoming
+ * migration still holds the yank instance registered below.
+ */
+ if (migration_in_incoming_postcopy()) {
+ error_setg(errp, "Incoming postcopy migration is still in progress");
+ return false;
+ }
+
if (runstate_check(RUN_STATE_POSTMIGRATE)) {
error_setg(errp, "Can't migrate the vm that was paused due to "
"previous migration");
@@ -2243,15 +2252,19 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
}
}
+ if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+ return false;
+ }
+
if (migrate_init(s, errp)) {
+ yank_unregister_instance(MIGRATION_YANK_INSTANCE);
return false;
}
return true;
}
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
- Error **errp);
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
void *opaque)
@@ -2276,7 +2289,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
{
MigrationAddress *addr = opaque;
- qmp_migrate_finish(addr, false, NULL);
+ qmp_migrate_finish(addr, NULL);
cpr_state_close();
migrate_hup_delete(migrate_get_current());
@@ -2288,7 +2301,6 @@ void qmp_migrate(const char *uri, bool has_channels,
MigrationChannelList *channels, bool has_detach, bool detach,
bool has_resume, bool resume, Error **errp)
{
- bool resume_requested;
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
g_autoptr(MigrationChannel) channel = NULL;
@@ -2341,8 +2353,7 @@ void qmp_migrate(const char *uri, bool has_channels,
return;
}
- resume_requested = has_resume && resume;
- if (!migrate_prepare(s, resume_requested, errp)) {
+ if (!migrate_prepare(s, has_resume && resume, errp)) {
/* Error detected, put into errp */
return;
}
@@ -2366,27 +2377,26 @@ void qmp_migrate(const char *uri, bool has_channels,
QAPI_CLONE(MigrationAddress, addr));
} else {
- qmp_migrate_finish(addr, resume_requested, errp);
+ qmp_migrate_finish(addr, errp);
}
out:
if (local_err) {
+ /* A resume didn't register it, see migrate_prepare() */
+ if (s->state != MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP) {
+ yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+ }
migration_connect_set_error(s, local_err);
error_propagate(errp, local_err);
}
}
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
- Error **errp)
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
{
MigrationState *s = migrate_get_current();
Error *local_err = NULL;
-
- if (!resume_requested) {
- if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
- return;
- }
- }
+ /* Before the start functions below change the state */
+ bool resume = s->state == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
SocketAddress *saddr = &addr->u.socket;
@@ -2413,7 +2423,8 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
}
if (local_err) {
- if (!resume_requested) {
+ /* Nothing connected: migration_cleanup() won't unregister it */
+ if (!resume) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
cpr_state_close();
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-23 17:13 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:13 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: yank instance leaked when cpr-transfer is cancelled during HUP wait
--
commit 19f402394221e6bd3b3aceafbab4a2479e855a39
Author: Fabiano Rosas <farosas@suse.de>
migration: yank: Move register instance earlier
This moves yank_register_instance() from qmp_migrate_finish() into
migrate_prepare(), so that it can be paired with the unregister in
migration_cleanup(). It also adds unregister calls on the qmp_migrate()
error path and refuses migrate while an incoming postcopy is in progress.
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
> diff --git a/migration/migration.c b/migration/migration.c
> index d5a21da3129a..cc88f70ae593 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -2243,15 +2252,19 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
> }
> }
>
> + if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> + return false;
> + }
> +
> if (migrate_init(s, errp)) {
> + yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> return false;
> }
>
> return true;
> }
[ ... ]
> @@ -2366,27 +2377,26 @@ void qmp_migrate(const char *uri, bool has_channels,
> QAPI_CLONE(MigrationAddress, addr));
>
> } else {
> - qmp_migrate_finish(addr, resume_requested, errp);
> + qmp_migrate_finish(addr, errp);
> }
>
> out:
> if (local_err) {
> + /* A resume didn't register it, see migrate_prepare() */
> + if (s->state != MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP) {
> + yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> + }
> migration_connect_set_error(s, local_err);
> error_propagate(errp, local_err);
> }
> }
[Severity: Medium]
Can this leak the yank instance when a cpr-transfer migration is
cancelled while qmp_migrate() is waiting for the HUP event?
In cpr-transfer mode the call chain is:
qmp_migrate()
migrate_prepare()
yank_register_instance(MIGRATION_YANK_INSTANCE, errp)
migrate_init() /* state is now SETUP */
cpr_state_save()
migrate_hup_add(s, cpr_state_ioc(), qmp_migrate_finish_cb, ...)
/* returns, qmp_migrate_finish() not called yet */
The instance is now registered during the whole HUP wait. Before this
patch it was registered in qmp_migrate_finish(), so nothing was held
during that window.
If the user then issues migrate_cancel, migration_cancel() sees setup
true and s->to_dst_file == NULL, and takes this branch:
migration/migration.c:migration_cancel() {
...
if (setup && !s->to_dst_file) {
migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
MIGRATION_STATUS_CANCELLED);
cpr_state_close();
migrate_hup_delete(s);
}
...
}
This branch does not call migration_cleanup(), and migrate_hup_delete()
destroys the source, so qmp_migrate_finish_cb() never runs either. None
of the unregister sites reached from here apply: the migrate_init()
failure branch in migrate_prepare(), the out: label in qmp_migrate(), the
error branch in qmp_migrate_finish(), and the unregister in
migration_cleanup().
The registration then stays in place, so every later migrate command
would fail in migrate_prepare() with "duplicate yank instance" until QEMU
is restarted. This also means the pairing with migration_cleanup() that
the commit message describes does not hold on this path.
This appears to be fixed later in the series. At the series end,
migration_cancel() checks cpr_transfer_source_active(s) and calls
migration_cleanup(s), which ends with yank_unregister_instance(). That
comes from the follow-up patches "migration: Move CPR HUP watch to
cpr-transfer.c" and "migration: Fix crash on second migration when cancel
early".
At this commit, though, cpr-transfer cancel during the HUP wait leaks the
instance, which affects bisectability. Could the cancel path be made to
unregister the instance here, or could these patches be reordered?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=18
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 19/36] migration: Free cpr-transfer MigrationAddress along with gsource
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (17 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:12 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 20/36] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
` (16 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Fabiano Rosas <farosas@suse.de>
When setting a callback on a Glib source and giving it a data pointer,
it's natural to also provide the destructor for the data in question.
Since migrate_hup_add() already needs to clone the MigrationAddress
when setting the qmp_migrate_finish_cb callback, also pass the
qapi_free_MigrationAddress as the GDestroyNotify callback.
With this the address doesn't need to be freed at the callback body,
making the management of that memory slightly simpler.
andrey.drobyshev:
This commit was cherry-picked from the upstream commit 6a887119ce18,
adapted for the hci-dev base: qmp_migrate_finish_cb() here still
closes the cpr state and deletes the HUP source itself (upstream had
already moved that into migration_cleanup()), so only the explicit
qapi_free_MigrationAddress() call goes away from it.
Cc: Mark Kanda <mark.kanda@oracle.com>
Cc: Ben Chaney <bchaney@akamai.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-23-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index cc88f70ae59..b365e413a84 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2269,9 +2269,11 @@ static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
void *opaque)
{
- s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
- g_source_set_callback(s->hup_source, cb, opaque, NULL);
- g_source_attach(s->hup_source, NULL);
+ s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
+ g_source_set_callback(s->hup_source, cb,
+ QAPI_CLONE(MigrationAddress, opaque),
+ (GDestroyNotify)qapi_free_MigrationAddress);
+ g_source_attach(s->hup_source, NULL);
}
static void migrate_hup_delete(MigrationState *s)
@@ -2293,7 +2295,6 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
cpr_state_close();
migrate_hup_delete(migrate_get_current());
- qapi_free_MigrationAddress(addr);
return G_SOURCE_REMOVE;
}
@@ -2374,7 +2375,7 @@ void qmp_migrate(const char *uri, bool has_channels,
*/
if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
- QAPI_CLONE(MigrationAddress, addr));
+ addr);
} else {
qmp_migrate_finish(addr, errp);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 20/36] migration: Move CPR HUP watch to cpr-transfer.c
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (18 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 19/36] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:12 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 21/36] migration: Fix crash on second migration when cancel early Andrey Drobyshev
` (15 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Fabiano Rosas <farosas@suse.de>
Move this CPR-specific code into a cpr file. While here, give the
functions more significant names.
This makes the new idea (after cpr-transfer) of having two parts to
qmp_migrate slightly more obvious: either wait for the hangup or
continue directly.
andrey.drobyshev:
This commit was cherry-picked from the upstream commit f523e5aa5f56,
adapted for the hci-dev base, which predates the upstream qmp_migrate()
rework around MigrationChannel: the watch is still armed with the
plain MigrationAddress from qmp_migrate() rather than main_ch->addr,
and qmp_migrate_finish_cb() keeps closing the cpr state and destroying
the HUP source itself.
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-24-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
include/migration/cpr.h | 5 +++++
migration/cpr-transfer.c | 23 +++++++++++++++++++++++
migration/migration.c | 29 ++++-------------------------
3 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 8fb79eb0a28..ae441322ff5 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -9,6 +9,7 @@
#define MIGRATION_CPR_H
#include "qapi/qapi-types-migration.h"
+#include "io/channel.h"
#include "qemu/queue.h"
#define MIG_MODE_NONE -1
@@ -76,6 +77,10 @@ int cpr_get_fd_param(const char *name, const char *fdname, int index, bool cpr,
QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp);
QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
+void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
+ void *opaque);
+void cpr_transfer_source_destroy(MigrationState *s);
+
void cpr_exec_init(void);
QEMUFile *cpr_exec_output(Error **errp);
QEMUFile *cpr_exec_input(Error **errp);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index 00371d17c3a..61d5c9dce2b 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -6,7 +6,10 @@
*/
#include "qemu/osdep.h"
+#include "qapi/clone-visitor.h"
#include "qapi/error.h"
+#include "qapi/qapi-visit-migration.h"
+#include "io/channel.h"
#include "io/channel-file.h"
#include "io/channel-socket.h"
#include "io/net-listener.h"
@@ -72,3 +75,23 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
return NULL;
}
}
+
+void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
+ void *opaque)
+{
+ s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
+ g_source_set_callback(s->hup_source,
+ (GSourceFunc)func,
+ QAPI_CLONE(MigrationAddress, opaque),
+ (GDestroyNotify)qapi_free_MigrationAddress);
+ g_source_attach(s->hup_source, NULL);
+}
+
+void cpr_transfer_source_destroy(MigrationState *s)
+{
+ if (s->hup_source) {
+ g_source_destroy(s->hup_source);
+ g_source_unref(s->hup_source);
+ s->hup_source = NULL;
+ }
+}
diff --git a/migration/migration.c b/migration/migration.c
index b365e413a84..d892655a21e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,7 +110,6 @@ static bool migration_object_check(MigrationState *ms, Error **errp);
static bool migration_switchover_start(MigrationState *s, Error **errp);
static bool close_return_path_on_source(MigrationState *s);
static void migration_completion_end(MigrationState *s);
-static void migrate_hup_delete(MigrationState *s);
static void migration_downtime_start(MigrationState *s)
{
@@ -1508,7 +1507,7 @@ static void migration_cleanup(MigrationState *s)
qemu_savevm_state_cleanup();
cpr_state_close();
- migrate_hup_delete(s);
+ cpr_transfer_source_destroy(s);
close_return_path_on_source(s);
@@ -1669,7 +1668,7 @@ void migration_cancel(void)
migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
MIGRATION_STATUS_CANCELLED);
cpr_state_close();
- migrate_hup_delete(s);
+ cpr_transfer_source_destroy(s);
}
}
@@ -2266,25 +2265,6 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
-static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
- void *opaque)
-{
- s->hup_source = qio_channel_create_watch(ioc, G_IO_HUP);
- g_source_set_callback(s->hup_source, cb,
- QAPI_CLONE(MigrationAddress, opaque),
- (GDestroyNotify)qapi_free_MigrationAddress);
- g_source_attach(s->hup_source, NULL);
-}
-
-static void migrate_hup_delete(MigrationState *s)
-{
- if (s->hup_source) {
- g_source_destroy(s->hup_source);
- g_source_unref(s->hup_source);
- s->hup_source = NULL;
- }
-}
-
static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
GIOCondition cond,
void *opaque)
@@ -2294,7 +2274,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
qmp_migrate_finish(addr, NULL);
cpr_state_close();
- migrate_hup_delete(migrate_get_current());
+ cpr_transfer_source_destroy(migrate_get_current());
return G_SOURCE_REMOVE;
}
@@ -2374,8 +2354,7 @@ void qmp_migrate(const char *uri, bool has_channels,
* connection, so qmp_migrate_finish will fail to connect, and then recover.
*/
if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
- migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
- addr);
+ cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
} else {
qmp_migrate_finish(addr, errp);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 21/36] migration: Fix crash on second migration when cancel early
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (19 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 20/36] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:17 ` no-reply
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
` (14 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
From: Peter Xu <peterx@redhat.com>
Marc-André reported an issue on QEMU crash when retrying a cancelled
migration during early setup phase, see "Link:" for more information, and
also easy way to reproduce.
This patch is a replacement of the prior fix proposed by not only switching
to migration_cleanup(), but also fixing it from CPR side, so that we track
hup_source properly to know if src QEMU is waiting or the HUP signal.
To put it simple: this chunk of special casing in migration_cancel() should
not affect normal migration, but only cpr-transfer migration to cover the
small window when the src QEMU is waiting for a HUP signal on cpr
channel (so that src QEMU can continue the migration on the main channel).
To achieve that, we'll also need to remember to detach the hup_source
whenenver invoked: after that point, we should always be able to cleanup
the migration.
It's not a generic operation to explicitly detach a gsource from its
context while in its dispatch() function. But it should be safe, because
gsource disptch() will only happen with a boosted refcount for the
dispatcher so that the gsource will not be freed until the callback
completes. It's also safe to return G_SOURCE_REMOVE after the gsource is
detached, as glib will simply ignore the G_SOURCE_REMOVE.
One can refer to latest 2.86.5 glib code in g_main_dispatch() for that:
https://github.com/GNOME/glib/blob/2.86.5/glib/gmain.c#L3592
When at this, add a bunch of assertions to make sure nothing surprises us.
After this patch applied, the 2nd migration will not crash QEMU, instead
it'll be in CANCELLING until the socket connection times out (it will take
~2min on my Fedora default kernel). During this process no 2nd migration
will be allowed, and after it timed out migration can be restarted.
It's because so far we don't have control over socket_connect_outgoing(),
or anything yet managed by a task executed in qio_task_run_in_thread().
Speeding up the cancellation to be left for future.
I also tested cpr-transfer by only providing cpr channel not the main
channel (with -incoming defer), kickoff migration on source, then cancel it
on source directly without providing the main channel. It keeps working.
I wanted to add an unit test for that but it'll need to refactor current
cpr-transfer tests first; let's leave it for later.
andrey.drobyshev:
This commit was cherry-picked from the upstream commit 4f08b870f8f0,
adapted for the hci-dev base, which predates the upstream qmp_migrate()
rework: the HUP callback is still qmp_migrate_finish_cb() rather than
migration_connect_outgoing_cb(), so the early
cpr_transfer_source_destroy() call lands there, ahead of
qmp_migrate_finish() and the cpr_state_close() the callback keeps
doing itself.
Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/cpr.h | 1 +
migration/cpr-transfer.c | 10 ++++++++++
migration/migration.c | 32 ++++++++++++++++++++++++--------
migration/migration.h | 5 +++++
4 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index ae441322ff5..4dd397a7c13 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -80,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp);
void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
void *opaque);
void cpr_transfer_source_destroy(MigrationState *s);
+bool cpr_transfer_source_active(MigrationState *s);
void cpr_exec_init(void);
QEMUFile *cpr_exec_output(Error **errp);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index 61d5c9dce2b..9defe7bad78 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -6,6 +6,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
#include "qapi/clone-visitor.h"
#include "qapi/error.h"
#include "qapi/qapi-visit-migration.h"
@@ -79,6 +80,7 @@ QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
void *opaque)
{
+ assert(bql_locked());
s->hup_source = qio_channel_create_watch(cpr_state_ioc(), G_IO_HUP);
g_source_set_callback(s->hup_source,
(GSourceFunc)func,
@@ -89,9 +91,17 @@ void cpr_transfer_add_hup_watch(MigrationState *s, QIOChannelFunc func,
void cpr_transfer_source_destroy(MigrationState *s)
{
+ assert(bql_locked());
if (s->hup_source) {
g_source_destroy(s->hup_source);
g_source_unref(s->hup_source);
s->hup_source = NULL;
}
}
+
+bool cpr_transfer_source_active(MigrationState *s)
+{
+ /* Whenever the HUP gsource is available, it's active. */
+ assert(bql_locked());
+ return s->hup_source;
+}
diff --git a/migration/migration.c b/migration/migration.c
index d892655a21e..821764afae9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1661,14 +1661,19 @@ void migration_cancel(void)
}
/*
- * If qmp_migrate_finish has not been called, then there is no path that
- * will complete the cancellation. Do it now.
+ * This is cpr-transfer specific processing.
+ *
+ * If this is true, it means cpr-transfer migration is waiting for the
+ * destination to send HUP event on CPR channel to continue the next
+ * phase. If so, do the cleanup proactively to avoid get stuck in
+ * CANCELLING state.
*/
- if (setup && !s->to_dst_file) {
- migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
- MIGRATION_STATUS_CANCELLED);
- cpr_state_close();
- cpr_transfer_source_destroy(s);
+ if (cpr_transfer_source_active(s)) {
+ assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
+ assert(setup && !s->to_dst_file);
+ migration_cleanup(s);
+ /* Now all things should have been released */
+ assert(!cpr_transfer_source_active(s));
}
}
@@ -2270,11 +2275,22 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
void *opaque)
{
MigrationAddress *addr = opaque;
+ MigrationState *s = migrate_get_current();
+
+ /*
+ * Detach and release the GSource right after use. We rely on this to
+ * detect this small cpr-transfer window of "waiting for HUP event".
+ */
+ cpr_transfer_source_destroy(s);
qmp_migrate_finish(addr, NULL);
cpr_state_close();
- cpr_transfer_source_destroy(migrate_get_current());
+
+ /*
+ * This is redundant as we do cpr_transfer_source_destroy() at the
+ * entry, but it's benign; glib will just skip the detach.
+ */
return G_SOURCE_REMOVE;
}
diff --git a/migration/migration.h b/migration/migration.h
index a7ea43e925d..ee15ca88461 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -509,6 +509,11 @@ struct MigrationState {
/* Is this a rdma migration */
bool rdma_migration;
+ /*
+ * When set, it means cpr-transfer is waiting for the HUP signal from
+ * destination to continue the 2nd step of migration via the main
+ * channel.
+ */
GSource *hup_source;
};
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 21/36] migration: Fix crash on second migration when cancel early
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 21/36] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-23 17:17 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:17 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: migration_cancel() asserts migrate_mode()==CPR_TRANSFER, but mode is mutable
--
commit d8e2c80dd170f3a8f2343c0a4b7abbebb7c94fc5
Author: Peter Xu <peterx@redhat.com>
migration: Fix crash on second migration when cancel early
This patch makes migration_cancel() call migration_cleanup() when the
cpr-transfer HUP gsource is still armed. Before, it used a special case
that checked setup && !s->to_dst_file. It also makes
qmp_migrate_finish_cb() detach the HUP gsource on entry, and it adds
several assertions.
Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com
> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -1661,14 +1661,19 @@ void migration_cancel(void)
> }
>
> /*
> - * If qmp_migrate_finish has not been called, then there is no path that
> - * will complete the cancellation. Do it now.
> + * This is cpr-transfer specific processing.
> + *
> + * If this is true, it means cpr-transfer migration is waiting for the
> + * destination to send HUP event on CPR channel to continue the next
> + * phase. If so, do the cleanup proactively to avoid get stuck in
> + * CANCELLING state.
> */
> - if (setup && !s->to_dst_file) {
> - migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
> - MIGRATION_STATUS_CANCELLED);
> - cpr_state_close();
> - cpr_transfer_source_destroy(s);
> + if (cpr_transfer_source_active(s)) {
> + assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
[Severity: Medium]
Can this assert abort QEMU on a legitimate cancel? assert() is g_assert()
here, and migrate_mode() reads s->parameters.mode, which can be changed
while the migration is running.
migrate_params_apply() writes s->parameters.mode whenever has_mode is set.
Neither qmp_migrate_set_parameters() nor migrate_params_check() rejects
that while a migration is running.
The hup_source is armed in qmp_migrate() while migrate_mode() is
MIG_MODE_CPR_TRANSFER, and the mode is not latched at that point:
if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
}
A possible sequence is:
migrate (mode=cpr-transfer) -> hup_source armed, state is SETUP
migrate-set-parameters mode=normal (or cpr-exec)
migrate_cancel -> migration_cancel()
migration_cancel() sees cpr_transfer_source_active(s) true, but
migrate_mode() no longer returns MIG_MODE_CPR_TRANSFER, so the assert
fails.
The same assert would also run from migration_shutdown() at QEMU exit,
and from the ram.c resize-during-precopy path, if the mode was changed
in the window. The window lasts until the destination closes the CPR
channel.
The old check was only setup && !s->to_dst_file. The new check makes the
cancel path depend on a user-mutable parameter.
Would it be better to drop this assert, or to record the mode when
hup_source is armed and check that instead?
> + assert(setup && !s->to_dst_file);
> + migration_cleanup(s);
> + /* Now all things should have been released */
> + assert(!cpr_transfer_source_active(s));
> }
> }
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=21
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (20 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 21/36] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-23 16:53 ` Andrey Drobyshev
2026-10-23 17:17 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 23/36] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
` (13 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:53 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
Since commit 7ed0919119b0 ("migration: close kvm after cpr") a source
QEMU that has handed its VFIO devices over via cpr-transfer (or
cpr-exec) calls kvm_close() once the migration completes, so that the
destination can register its irq bypass consumers. kvm_close() closes
the VM and vCPU descriptors and sets kvm_state to NULL, but the source
process keeps living with all its device models intact until the
management layer terminates it.
Nothing prevents those device models from raising interrupts in the
meantime, and the regular exit path even guarantees one: net_cleanup()
deletes the netdev behind an emulated virtio-net, the NIC sees the link
go down and calls virtio_notify_config(), MSI-X delivers through the
in-kernel APIC, and kvm_irqchip_send_msi() dereferences the NULL
kvm_state:
#0 kvm_vm_ioctl (s=0x0, type=KVM_SIGNAL_MSI) accel/kvm/kvm-all.c
#1 kvm_irqchip_send_msi (s=0x0)
#2 kvm_send_msi hw/i386/kvm/apic.c
#3 kvm_apic_mem_write
...
#9 pci_msi_trigger
#11 msix_notify
#12 virtio_pci_notify
#14 virtio_notify_config
#15 virtio_net_set_link_status
#16 qemu_del_net_client
#17 net_cleanup
#18 qemu_cleanup
Every retired cpr-transfer source with a VFIO device thus dies with
SIGSEGV on SIGTERM/quit and leaves a core dump behind. Without a VFIO
device kvm_close() never runs and the same exit is harmless.
Make the ioctl wrappers honour the closed state instead of assuming a
live KVMState: keep kvm_state allocated, with its closed descriptors as
the mark, and let kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl()
return 0 once kvm_close() has been called, so any late interrupt or
query on the retired source is a no-op. Returning an error instead
would still kill it: kvm_set_irq() aborts and kvm_irqchip_commit_routes()
asserts on any failure, and the ioeventfd callers abort on a stale errno.
Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
accel/kvm/kvm-all.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a39c1ba260a..8b5d342af52 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -644,7 +644,6 @@ void kvm_close(void)
close(kvm_state->fd);
kvm_state->fd = -1;
}
- kvm_state = NULL;
}
/*
@@ -3371,6 +3370,11 @@ int kvm_ioctl(KVMState *s, unsigned long type, ...)
arg = va_arg(ap, void *);
va_end(ap);
+ /* kvm_close() has been called: there is no KVM to talk to any more */
+ if (s->fd == -1) {
+ return 0;
+ }
+
trace_kvm_ioctl(type, arg);
ret = ioctl(s->fd, type, arg);
if (ret == -1) {
@@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
arg = va_arg(ap, void *);
va_end(ap);
+ /* kvm_close() has been called: there is no VM to talk to any more */
+ if (s->vmfd == -1) {
+ return 0;
+ }
+
trace_kvm_vm_ioctl(type, arg);
accel_ioctl_begin();
ret = ioctl(s->vmfd, type, arg);
@@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
arg = va_arg(ap, void *);
va_end(ap);
+ /* kvm_close() has been called: the vCPU fds are gone */
+ if (cpu->kvm_fd == -1) {
+ return 0;
+ }
+
trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
accel_cpu_ioctl_begin(cpu);
ret = ioctl(cpu->kvm_fd, type, arg);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close()
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-23 17:17 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:17 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] kvm: ioctl wrappers return 0 after kvm_close(), faking fd 0 for fd-returning ioctls
- [Medium] kvm: fd==-1 early return makes vfio_kvm_device_add_fd() store bogus fd 0
--
commit 78cd49a51c86e49590c679c12aaf6d28efd634ee
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
kvm: tolerate ioctls after kvm_close()
This patch keeps kvm_state allocated after kvm_close() and makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0 once the descriptors are closed. The goal is to stop a retired cpr-transfer source from crashing on a late interrupt, such as kvm_irqchip_send_msi() with kvm_state set to NULL.
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
[ ... ]
> @@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
> arg = va_arg(ap, void *);
> va_end(ap);
>
> + /* kvm_close() has been called: there is no VM to talk to any more */
> + if (s->vmfd == -1) {
> + return 0;
> + }
> +
[Severity: Medium]
Can this early return of 0 be mistaken for a valid file descriptor? Several kvm_vm_ioctl() requests return a new fd on success, and callers treat 0 as success.
query_stats_cb() and query_stats_schemas_cb() only treat -1 as an error:
stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
if (stats_fd == -1) {
...
}
query_stats(result, target, names, stats_fd, NULL, errp);
close(stats_fd);
After kvm_close(), kvm_state is non-NULL and vmfd is -1. The fake 0 then passes the check, the stats are read through fd 0, and close(stats_fd) closes stdin.
kvm_create_guest_memfd() checks only "if (fd < 0)", so it would return 0 as a valid guest_memfd. kvm_create_vcpu() checks only "if (kvm_fd < 0)", so it would store 0 in cpu->kvm_fd. Later vCPU ioctls would then go to stdin.
The vCPU stats path is not affected, because kvm_close() sets kvm_vcpu_stats_fd to -1 and query_stats_vcpu() checks for it. I did not verify that the QMP stats commands are reachable on a retired source. I also did not find a kvm_enabled() or closed-state guard that would block these callers.
Would it be better to have these fd-returning requests fail explicitly after kvm_close(), or to have the callers check for the closed state?
[Severity: Medium]
Can the same early return cause vfio_kvm_device_add_fd() to store a bogus fd? After vfio_kvm_device_close() calls kvm_close(), it resets vfio_kvm_device_fd to -1, and kvm_enabled() stays true. A later VFIO add on the retired source takes this path in hw/vfio/helpers.c:
if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
error_setg_errno(errp, errno, "Failed to create KVM VFIO device");
return -errno;
}
vfio_kvm_device_fd = cd.fd;
kvm_vm_ioctl() now returns 0 without running the ioctl. cd.fd is still 0 because cd is zero-initialised apart from .type, so vfio_kvm_device_fd becomes 0, which is stdin.
The following KVM_SET_DEVICE_ATTR ioctl on fd 0 fails with ENOTTY, so the add reports an error with a stale errno. vfio_kvm_device_fd is left at 0. Later add and del calls then skip creation and issue ioctls on fd 0, instead of reporting that no KVM VFIO device exists.
Is this the intended behaviour for VFIO hotplug on a retired source?
A related case is kvm_vm_check_extension(). It skips its "ret < 0" fallback to the global check after close. That looks benign, because kvm_check_extension() also returns 0 after close. It does show that callers cannot tell the closed state from success.
The commit message describes only the interrupt and ioeventfd callers. It does not mention callers that use the return value as an fd or that depend on the ioctl filling an output buffer. Examples are KVM_CREATE_DEVICE, KVM_GET_STATS_FD and KVM_GET_DIRTY_LOG, which would report success with an unfilled bitmap.
The final series state (f62ee5bb) has the same code in query_stats_cb(), query_stats_schemas_cb(), kvm_create_vcpu() and kvm_create_guest_memfd(). The later patch "kvm: fix coalesced MMIO flush racing with kvm_close()" and the other follow-ups do not change this. hw/vfio/helpers.c is also unchanged by the series.
> @@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
> arg = va_arg(ap, void *);
> va_end(ap);
>
> + /* kvm_close() has been called: the vCPU fds are gone */
> + if (cpu->kvm_fd == -1) {
> + return 0;
> + }
> +
[Severity: Medium]
This is the same fake-success behaviour as in kvm_vm_ioctl(). Any vCPU ioctl that fills a buffer, such as KVM_GET_REGS, now returns 0 with the output left unfilled once cpu->kvm_fd is -1. Can callers that consume that output be affected?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=22
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 23/36] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (21 preceding siblings ...)
2026-10-23 16:53 ` [PATCH QEMU HCI-8.1 v5 22/36] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:19 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress() Andrey Drobyshev
` (12 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
kvm_close() destroys the vCPUs, which unmaps their kvm_run areas, and the
coalesced MMIO ring lives in the kvm_run area of the first vCPU. Nothing
stops the RCU thread from flushing that ring at the same time: finalizing
a memory region of a freed flatview calls memory_region_transaction_begin()
and thus kvm_flush_coalesced_mmio_buffer(). kvm_state is still set at
that point, so the flush reads the ring after it has been unmapped:
#0 kvm_flush_coalesced_mmio_buffer accel/kvm/kvm-all.c
#1 qemu_flush_coalesced_mmio_buffer
#2 memory_region_transaction_begin
#3 memory_region_finalize
...
#10 flatview_destroy
#11 call_rcu_thread
while the main thread is in:
kvm_close -> cpu_remove_sync
vfio_kvm_device_close
vfio_cpr_kvm_close_notifier (MIG_EVENT_PRECOPY_DONE)
Seen as a SIGSEGV of the old QEMU during repeated cpr-exec of a VM with
a VFIO device.
Clear the ring pointer first, so that new flushes skip it, and wait for
the pending RCU callbacks before destroying the vCPUs.
Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
accel/kvm/kvm-all.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 8b5d342af52..294a87043ef 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -630,6 +630,14 @@ void kvm_close(void)
return;
}
+ /*
+ * Destroying vCPUs unmaps the coalesced MMIO ring, while the RCU thread
+ * may be flushing it from a memory region finalizer. Stop new flushes
+ * and wait for the pending RCU callbacks first.
+ */
+ kvm_state->coalesced_mmio_ring = NULL;
+ drain_call_rcu();
+
CPU_FOREACH(cpu) {
cpu_remove_sync(cpu);
close(cpu->kvm_fd);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 23/36] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 23/36] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-23 17:19 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:19 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
> kvm_close() destroys the vCPUs, which unmaps their kvm_run areas, and the
> coalesced MMIO ring lives in the kvm_run area of the first vCPU. Nothing
> stops the RCU thread from flushing that ring at the same time: finalizing
> a memory region of a freed flatview calls memory_region_transaction_begin()
> and thus kvm_flush_coalesced_mmio_buffer(). kvm_state is still set at
> that point, so the flush reads the ring after it has been unmapped:
>
> #0 kvm_flush_coalesced_mmio_buffer accel/kvm/kvm-all.c
> #1 qemu_flush_coalesced_mmio_buffer
> #2 memory_region_transaction_begin
> #3 memory_region_finalize
> ...
> #10 flatview_destroy
> #11 call_rcu_thread
>
> while the main thread is in:
>
> kvm_close -> cpu_remove_sync
> vfio_kvm_device_close
> vfio_cpr_kvm_close_notifier (MIG_EVENT_PRECOPY_DONE)
>
> Seen as a SIGSEGV of the old QEMU during repeated cpr-exec of a VM with
> a VFIO device.
>
> Clear the ring pointer first, so that new flushes skip it, and wait for
> the pending RCU callbacks before destroying the vCPUs.
>
> Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=23
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress()
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (22 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 23/36] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:19 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 25/36] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
` (11 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
cpr_incoming_needed() only looks at the migration mode. That's fine for
vmstate .needed callbacks, which run during a save or load. But code that
runs at any time, e.g. VM state change handlers, can't tell from it
whether a CPR migration is actually going on: on the source the mode is
just a parameter, set by migrate-set-parameters whether or not a migrate
follows.
Add cpr_is_in_progress(), which is true on the target until the incoming
migration is done, and on the source only while the outgoing one runs.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
include/migration/cpr.h | 1 +
migration/cpr.c | 22 ++++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 4dd397a7c13..e73c5da1d9c 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -64,6 +64,7 @@ MigMode cpr_get_incoming_mode(void);
void cpr_set_incoming_mode(MigMode mode);
bool cpr_is_incoming(void);
bool cpr_exec_is_incoming(void);
+bool cpr_is_in_progress(void);
bool cpr_state_save(MigrationChannel *channel, Error **errp);
int cpr_state_load(MigrationChannel *channel, Error **errp);
diff --git a/migration/cpr.c b/migration/cpr.c
index a149e790c10..3ea5d86bab4 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -549,6 +549,28 @@ bool cpr_exec_is_incoming(void)
return incoming_mode == MIG_MODE_CPR_EXEC;
}
+bool cpr_is_in_progress(void)
+{
+ /* The incoming mode on the target, the 'mode' parameter on the source */
+ MigMode mode = migrate_mode();
+
+ if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
+ return false;
+ }
+
+ /* Target: the incoming mode is set until the incoming migration is done */
+ if (cpr_is_incoming()) {
+ return true;
+ }
+
+ /*
+ * Source: the parameter alone means nothing, it may just have been set.
+ * An outgoing CPR migration has the CPR state open, from
+ * cpr_state_save() to cpr_state_close().
+ */
+ return cpr_state_file != NULL;
+}
+
bool cpr_state_save(MigrationChannel *channel, Error **errp)
{
int ret;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress()
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress() Andrey Drobyshev
@ 2026-10-23 17:19 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:19 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] migration/cpr: cpr_is_in_progress() false on cpr-transfer source after early cpr_state_close()
--
Commit 453b7d2ec8b4fa272ccd4231894792f7b5e92c8a
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
migration/cpr: add cpr_is_in_progress()
This commit adds cpr_is_in_progress(). It returns true on the target until the incoming migration is done. On the source it returns true only while the outgoing migration runs, which it detects by checking whether cpr_state_file is open.
> diff --git a/migration/cpr.c b/migration/cpr.c
> index a149e790c103..3ea5d86bab48 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
[ ... ]
> +bool cpr_is_in_progress(void)
> +{
> + /* The incoming mode on the target, the 'mode' parameter on the source */
> + MigMode mode = migrate_mode();
> +
> + if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
> + return false;
> + }
> +
> + /* Target: the incoming mode is set until the incoming migration is done */
> + if (cpr_is_incoming()) {
> + return true;
> + }
> +
> + /*
> + * Source: the parameter alone means nothing, it may just have been set.
> + * An outgoing CPR migration has the CPR state open, from
> + * cpr_state_save() to cpr_state_close().
> + */
> + return cpr_state_file != NULL;
[Severity: High]
Can this return false for most of an outgoing cpr-transfer migration?
The commit message says the function is true "on the source only while
the outgoing one runs", but cpr_state_file does not appear to stay open
that long in cpr-transfer mode.
In qmp_migrate(), cpr-transfer defers the real migration to a HUP
callback:
if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
When the target closes the CPR socket, qmp_migrate_finish_cb() starts the
outgoing migration and then closes the CPR state right away:
qmp_migrate_finish(addr, NULL);
cpr_state_close();
cpr_state_close() does qemu_fclose(cpr_state_file) and sets
cpr_state_file to NULL.
From then on, for the migration thread, precopy, the vm_stop state change
handlers and save_complete, the predicate evaluates as follows:
migrate_mode() == MIG_MODE_CPR_TRANSFER
cpr_is_incoming() == false (source)
cpr_state_file == NULL
=> cpr_is_in_progress() returns false
In cpr-exec mode there is no HUP callback. The file stays open until
migration_cleanup() calls cpr_state_close(), so the result is correct
there. The two CPR modes therefore give different answers.
There is a second window where it returns false. cpr_state_file is
assigned only at the end of cpr_state_save():
cpr_state_file = f;
return true;
So the predicate is also false while the CPR state is still being
written.
Later patches in this series call cpr_is_in_progress() from
vfio_vmstate_change(), vfio_is_active(), vfio_save_state() and
vfio_listener_log_global_start(). For example, vfio_vmstate_change() has:
/* CPR source only: the device keeps running across the exec */
if (cpr_is_in_progress()) {
return;
}
On a cpr-transfer source this check sees false. The VM state change
handler would then stop the device, save and send its state, and start
dirty page tracking. The series is meant to avoid exactly that.
No later patch in the series changes cpr_is_in_progress(),
cpr_state_close() or the cpr_state_file lifetime. The series also removes
the "CPR is prohibited: passthrough (VFIO) devices present" blocker, so
this path becomes reachable with cpr-transfer.
Should the source-side check use something that lasts for the whole
outgoing migration, rather than cpr_state_file? Alternatively, could the
cpr_state_close() call in qmp_migrate_finish_cb() be moved so that it
matches the lifetime the commit message describes?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=24
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 25/36] vfio/migration: don't migrate device state during CPR
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (23 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 24/36] migration/cpr: add cpr_is_in_progress() Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:26 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming Andrey Drobyshev
` (10 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
cpr-transfer and cpr-exec hand the running VFIO device over to the new
QEMU as is: the device fd, its mappings and interrupts are preserved, and
the device itself is never stopped.
But if the device supports VFIO migration, its migration handlers still
run. The old QEMU stops the device and saves its state, then the new
QEMU puts the very same device into RESUMING and loads that state back.
On top of that, vfio_pci_load_config() re-programs MSI-X and disables
vector 0.
For an NVIDIA vGPU this breaks the device after qemu-update: restoring
the state into the live vGPU fails and the guest loses the device.
Devices without migration support aren't affected, since no migration
handlers are registered for them.
Skip VFIO migration while a CPR migration is in progress: don't save or
load the device state, and don't change the device state on VM
stop/start. All of it is gated on cpr_is_in_progress(), not on the mode
alone: the mode may merely be set, and then a plain stop/cont must still
change the device state, and a snapshot must still save it.
Also reset multifd_transfer in vfio_multifd_cleanup(): it's only set in
vfio_multifd_setup(), which CPR now skips, so a stale value from an
earlier migration would still let the multifd save thread run.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/migration-multifd.c | 2 ++
hw/vfio/migration.c | 38 +++++++++++++++++++++++++++++++++++++-
2 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index e4785031a73..452f772dd7a 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -567,6 +567,8 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev)
VFIOMigration *migration = vbasedev->migration;
g_clear_pointer(&migration->multifd, vfio_multifd_free);
+ /* Decided anew by vfio_multifd_setup() of the next migration */
+ migration->multifd_transfer = false;
}
bool vfio_multifd_transfer_supported(void)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4c06e3db936..b142c9f2289 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -19,6 +19,7 @@
#include "hw/vfio/vfio-device.h"
#include "hw/vfio/vfio-migration.h"
#include "migration/misc.h"
+#include "migration/cpr.h"
#include "migration/savevm.h"
#include "migration/vmstate.h"
#include "migration/qemu-file.h"
@@ -513,7 +514,6 @@ static void vfio_save_cleanup(void *opaque)
Error *local_err = NULL;
int ret;
- /* Currently a NOP, done for symmetry with load_cleanup() */
vfio_multifd_cleanup(vbasedev);
/*
@@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
Error *local_err = NULL;
int ret;
+ /*
+ * .is_active doesn't cover the non-iterable stage, so the section is
+ * still sent: leave it empty, or new QEMU would load the config into
+ * the preserved device and re-program its MSI-X.
+ */
+ if (cpr_is_in_progress()) {
+ qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+ return;
+ }
+
if (vfio_multifd_transfer_enabled(vbasedev)) {
if (vfio_load_config_after_iter(vbasedev)) {
qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
@@ -810,6 +820,11 @@ static bool vfio_switchover_ack_needed(void *opaque)
{
VFIODevice *vbasedev = opaque;
+ /* No device state is sent during CPR, so no ACK to wait for */
+ if (cpr_is_in_progress()) {
+ return false;
+ }
+
return vfio_precopy_supported(vbasedev);
}
@@ -824,7 +839,19 @@ static int vfio_switchover_start(void *opaque)
return 0;
}
+/*
+ * cpr-transfer and cpr-exec hand the running device over to the new QEMU
+ * as is: its fds, mappings and interrupts are preserved and it never stops.
+ * Device state migration must stay out of it, else the device is stopped,
+ * its state saved and then resumed into the very same device.
+ */
+static bool vfio_is_active(void *opaque)
+{
+ return !cpr_is_in_progress();
+}
+
static const SaveVMHandlers savevm_vfio_handlers = {
+ .is_active = vfio_is_active,
.save_prepare = vfio_save_prepare,
.save_setup = vfio_save_setup,
.save_cleanup = vfio_save_cleanup,
@@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
Error *local_err = NULL;
int ret;
+ /* The device is preserved, not a plain stop/cont: keep its state */
+ if (cpr_is_in_progress()) {
+ return;
+ }
+
new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
VFIO_DEVICE_STATE_PRE_COPY_P2P :
VFIO_DEVICE_STATE_RUNNING_P2P;
@@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
Error *local_err = NULL;
int ret;
+ if (cpr_is_in_progress()) {
+ return;
+ }
+
if (running) {
new_state = VFIO_DEVICE_STATE_RUNNING;
} else {
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 25/36] vfio/migration: don't migrate device state during CPR
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 25/36] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-23 17:26 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:26 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vfio/migration: cpr_is_in_progress() is false for cpr-transfer source after HUP, so the gating is bypassed
- [Medium] vfio/cpr: cpr_is_in_progress() is false on the cpr-transfer source during stream
- [Medium] vfio/migration: vfio_load_state() lacks a CPR guard, so a non-empty stream re-programs MSI-X
--
commit d5eeadfa806615694344ddad118c4979064a63b2
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/migration: don't migrate device state during CPR
This patch makes the VFIO migration handlers do nothing while a CPR
migration (cpr-transfer or cpr-exec) is in progress. It gates the save,
load and VM stop/start paths on cpr_is_in_progress(). It also resets
multifd_transfer in vfio_multifd_cleanup().
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
[ ... ]
> @@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
> Error *local_err = NULL;
> int ret;
>
> + /*
> + * .is_active doesn't cover the non-iterable stage, so the section is
> + * still sent: leave it empty, or new QEMU would load the config into
> + * the preserved device and re-program its MSI-X.
> + */
> + if (cpr_is_in_progress()) {
> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> + return;
> + }
> +
[Severity: High]
Is cpr_is_in_progress() still true on the cpr-transfer source when this
runs?
On the source the predicate reduces to cpr_state_file != NULL:
migration/cpr.c:cpr_is_in_progress() {
...
return cpr_state_file != NULL;
}
For cpr-transfer, qmp_migrate() calls cpr_state_save() and only arms
cpr_transfer_add_hup_watch(). When the target closes the CPR socket:
qmp_migrate_finish_cb()
qmp_migrate_finish(addr, NULL); /* only starts an async connect */
cpr_state_close(); /* cpr_state_file = NULL */
The migration thread (vm_stop, the vmstate change notifiers,
qemu_savevm_state_setup(), vfio_save_state(), save_complete) runs only
after the connect completes. By then cpr_is_in_progress() returns false
on the source.
Would that make vfio_is_active(), vfio_save_state(),
vfio_switchover_ack_needed(), vfio_vmstate_change_prepare() and
vfio_vmstate_change() all take the normal migration path on a cpr-transfer
source? The old QEMU would then stop the live device and save its config
and state.
The target has cpr_is_incoming() true, so it skips load_setup but still
parses the stream. That looks like the vGPU breakage this patch is meant
to fix, together with a source/target mismatch.
The comment in cpr_is_in_progress() says "An outgoing CPR migration has
the CPR state open, from cpr_state_save() to cpr_state_close()", and the
commit message relies on this. Does that hold for cpr-transfer?
cpr-exec looks fine, since qmp_migrate_finish() is called directly there
and cpr_state_close() runs only on error or in migration_cleanup().
Nothing later in the series appears to change cpr_is_in_progress(),
cpr_state_save(), cpr_state_close() or qmp_migrate_finish_cb(). The later
patch "VZ: vfio/region: preserve BAR mappings across cpr-exec" adds an
mmap_blocker in vfio_device_attach() that blocks cpr-transfer for VFIO
devices. That makes the path unreachable at series end, but it doesn't
change the predicate.
[Severity: High]
Is the source-side predicate also an unlatched sample of mutable globals?
qmp_migrate_set_parameters() does not reject changing "mode" while a
migration is running. Could migrate_mode(), which cpr_is_in_progress()
reads, change between the VM stop and the later vm start or
vfio_save_state() call? The gates in vfio_vmstate_change_prepare(),
vfio_vmstate_change() and vfio_save_state() would then disagree with each
other.
[ ... ]
> @@ -810,6 +820,11 @@ static bool vfio_switchover_ack_needed(void *opaque)
> {
> VFIODevice *vbasedev = opaque;
>
> + /* No device state is sent during CPR, so no ACK to wait for */
> + if (cpr_is_in_progress()) {
> + return false;
> + }
> +
> return vfio_precopy_supported(vbasedev);
> }
[ ... ]
> +static bool vfio_is_active(void *opaque)
> +{
> + return !cpr_is_in_progress();
> +}
> +
> static const SaveVMHandlers savevm_vfio_handlers = {
> + .is_active = vfio_is_active,
> .save_prepare = vfio_save_prepare,
[Severity: Medium]
Should vfio_load_state() have a matching cpr_is_in_progress() guard?
The patch gates only the save side and load_setup (through is_active).
vfio_load_state() has no check:
hw/vfio/migration.c:vfio_load_state() {
...
case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
...
return vfio_load_device_config_state(f, opaque);
}
Nothing in the loadvm section path consults is_active before calling
load_state. If the target receives a non-empty VFIO section during CPR,
it reaches vfio_pci_load_config() on the preserved device even though
vfio_load_setup() never ran. That would re-program MSI-X and disable
vector 0.
A DEV_DATA_STATE section would reach vfio_load_buffer() with data_fd == -1
and fail with EBADF, aborting the migration.
A non-empty section is reachable when the source predicate returns false,
as described above for the cpr-transfer source. The later patch "vfio/migration: don't assume
the device is running on CPR incoming" only adds cpr_is_incoming()
handling in vfio_vmstate_change() and migration init, and adds no
load-side guard.
[ ... ]
> @@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
> Error *local_err = NULL;
> int ret;
>
> + /* The device is preserved, not a plain stop/cont: keep its state */
> + if (cpr_is_in_progress()) {
> + return;
> + }
> +
[ ... ]
> @@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> Error *local_err = NULL;
> int ret;
>
> + if (cpr_is_in_progress()) {
> + return;
> + }
> +
The same cpr_is_in_progress() question applies to both of these gates on
the cpr-transfer source, since cpr_state_close() has already cleared
cpr_state_file when the VM is stopped.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=25
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (24 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 25/36] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:23 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR Andrey Drobyshev
` (9 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
Since the device descriptor is preserved across CPR, the device keeps
whatever state the source left it in, while vfio_migration_init() on
the target still caches RUNNING. For a VM paused before CPR, the
device is in STOP, so on 'cont' vfio_migration_set_state() finds the
cached and the requested state equal and skips the ioctl: the guest
runs with a stopped device.
On CPR incoming, cache STOP instead, and have the VM state handler,
which doesn't touch the device during CPR, note where the VM leaves
it: RUNNING once the target starts the VM, STOP if it stays paused.
At CPR the device can only be in one of the two.
Fixes: c29a65ed685 ("vfio/container: preserve descriptors")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/migration.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index b142c9f2289..8f2722dc671 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -914,6 +914,14 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
Error *local_err = NULL;
int ret;
+ /* CPR target only: the device kept its state, note where the VM leaves it */
+ if (cpr_is_incoming()) {
+ vbasedev->migration->device_state = running ?
+ VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
+ return;
+ }
+
+ /* CPR source only: the device keeps running across the exec */
if (cpr_is_in_progress()) {
return;
}
@@ -1039,7 +1047,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
vbasedev->migration = g_new0(VFIOMigration, 1);
migration = vbasedev->migration;
migration->vbasedev = vbasedev;
- migration->device_state = VFIO_DEVICE_STATE_RUNNING;
+ /* A device inherited across CPR is in STOP unless the VM gets started */
+ migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
+ VFIO_DEVICE_STATE_RUNNING;
migration->data_fd = -1;
migration->mig_flags = mig_flags;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming Andrey Drobyshev
@ 2026-10-23 17:23 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:23 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vfio/migration: cached STOP on CPR incoming is wrong if target stays paused
--
commit 2d2dfcb91b4c065d544641a9b16c0991feb658d5
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/migration: don't assume the device is running on CPR incoming
This patch makes vfio_migration_init() cache STOP instead of RUNNING when
CPR is incoming. It also makes vfio_vmstate_change() record RUNNING or STOP
in the cached device_state on the CPR target, instead of leaving the device
untouched. The goal is to stop 'cont' from skipping the ioctl when the
source VM was paused.
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
[ ... ]
> @@ -914,6 +914,14 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> Error *local_err = NULL;
> int ret;
>
> + /* CPR target only: the device kept its state, note where the VM leaves it */
> + if (cpr_is_incoming()) {
> + vbasedev->migration->device_state = running ?
> + VFIO_DEVICE_STATE_RUNNING : VFIO_DEVICE_STATE_STOP;
> + return;
> + }
> +
[Severity: Low]
Can the cached STOP be left stale here when the target stays paused?
If the source VM was running, CPR skips the device stop under
cpr_is_in_progress(), so the kernel device stays RUNNING. On the target,
vfio_migration_init() then caches STOP unconditionally:
migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
VFIO_DEVICE_STATE_RUNNING;
If the target stays paused (-S, autostart=false, or a block-activate
failure), process_incoming_migration_bh() takes this path:
migration/migration.c:process_incoming_migration_bh() {
...
} else {
runstate_set(RUN_STATE_PAUSED);
}
...
}
runstate_set() does not call vm_state_notify(), so vfio_vmstate_change()
is never invoked. The new cpr_is_incoming() block above is therefore never
reached, and the cache stays at STOP while the device is really RUNNING.
The commit message says "STOP if it stays paused" and "At CPR the device
can only be in one of the two". Is that accurate for a running source
whose target stays paused?
The cached state is also consumed elsewhere, for example:
hw/vfio/listener.c:vfio_log_sync_needed() {
...
if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
(vfio_device_state_is_running(vbasedev) ||
vfio_device_state_is_precopy(vbasedev))) {
return false;
}
...
}
Could a cached STOP on a really RUNNING device give the wrong dirty-sync
decision there, if the still-paused target is migrated out again?
A later 'cont' would also issue a redundant STOP->RUNNING transition, and
vfio_device_state_is_running() would return the wrong answer until then.
Would it be possible to determine the real device state on CPR incoming,
rather than assuming STOP?
> + /* CPR source only: the device keeps running across the exec */
> if (cpr_is_in_progress()) {
> return;
> }
[ ... ]
> @@ -1039,7 +1047,9 @@ static int vfio_migration_init(VFIODevice *vbasedev)
> - migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> + /* A device inherited across CPR is in STOP unless the VM gets started */
> + migration->device_state = cpr_is_incoming() ? VFIO_DEVICE_STATE_STOP :
> + VFIO_DEVICE_STATE_RUNNING;
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=26
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (25 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 26/36] vfio/migration: don't assume the device is running on CPR incoming Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:27 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 28/36] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
` (8 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
During cpr-transfer and cpr-exec guest RAM is preserved in place, not
copied, so there's no need to track which pages the device writes to.
Yet starting the migration also starts VFIO dirty tracking, on the
container or on the device itself (DMA logging). With cpr-exec it isn't
stopped before exec, so e.g. the DMA page tracker of an NVIDIA vGPU is
left running after the update.
Don't start dirty tracking while a CPR migration is in progress. Gate
it on cpr_is_in_progress(), not on the mode alone, so that a snapshot
taken while the mode parameter is set to a CPR mode still tracks dirty
pages. Stopping stays unconditional: it's a no-op if tracking wasn't
started, and the mode may have changed in between.
Known gap: the memory core starts the listeners only for the first dirty
log user. If another one (e.g. calc-dirty-rate) appears during a CPR
that then fails, VFIO tracking stays off until QEMU restarts.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/listener.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index afcf518f567..1270b9d5f3a 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -41,6 +41,7 @@
#include "trace.h"
#include "qapi/error.h"
#include "migration/misc.h"
+#include "migration/cpr.h"
#include "migration/qemu-file.h"
#include "system/tcg.h"
#include "system/tpm.h"
@@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
listener);
bool ret;
+ /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
+ if (cpr_is_in_progress()) {
+ return true;
+ }
+
if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
ret = vfio_devices_dma_logging_start(bcontainer, errp);
} else {
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR Andrey Drobyshev
@ 2026-10-23 17:27 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:27 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio: CPR skip in log_global_start() leaves VFIO dirty tracking off if dirty log stays on
- [Medium] vfio: CPR dirty-tracking skip is ineffective on the cpr-transfer source
--
commit b54c3b90944a2a7cd1a56e7739cf8ea66b19d6c9
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/listener: skip dirty page tracking during CPR
This patch makes vfio_listener_log_global_start() return early without
starting VFIO dirty page tracking while cpr_is_in_progress() is true. The
aim is to avoid leaving DMA logging running after cpr-transfer or cpr-exec.
Stopping tracking is left unconditional.
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index afcf518f567f..1270b9d5f3aa 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c
[ ... ]
> @@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
> listener);
> bool ret;
>
> + /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
> + if (cpr_is_in_progress()) {
> + return true;
> + }
> +
[Severity: Medium]
Is this skip effective on the cpr-transfer source?
On the source, cpr_is_in_progress() reduces to this check, because
cpr_is_incoming() is false there:
migration/cpr.c:cpr_is_in_progress() {
...
return cpr_state_file != NULL;
}
For cpr-transfer, qmp_migrate() only arms a HUP watch after
cpr_state_save(). When the target closes the CPR socket, the main loop
runs qmp_migrate_finish_cb() with the BQL held:
migration/migration.c:qmp_migrate_finish_cb() {
...
qmp_migrate_finish(addr, NULL);
cpr_state_close();
}
So cpr_state_file is already NULL by the time the migration thread gets
the BQL and runs setup:
migration_thread()
bql_lock();
ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
bql_unlock();
The call chain from there is:
qemu_savevm_state_setup()
ram_init_bitmaps()
memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp)
vfio_listener_log_global_start()
When that chain reaches the new check, cpr_is_in_progress() returns false.
VFIO dirty tracking would then still start on the cpr-transfer source, even
though the commit message says it is skipped for cpr-transfer. cpr-exec
does not appear to be affected.
I did not find a later change in the series that alters the timing of
cpr_state_close() or the predicate. Could the check be based on something
that is still set when setup runs on the cpr-transfer source?
[Severity: High]
Can VFIO dirty tracking stay off after a failed CPR when another dirty log
user is active?
The early return sets neither vbasedev->dirty_tracking nor
bcontainer->dirty_pages_started. The memory core calls the listeners only on
the transition from no dirty log users to one:
system/memory.c:memory_global_dirty_log_start() {
old_flags = global_dirty_tracking;
global_dirty_tracking |= flags;
...
if (!old_flags) {
if (!memory_global_dirty_log_do_start(errp)) {
...
}
A possible sequence is:
1. A CPR migration starts and global_dirty_tracking goes from 0 to
GLOBAL_DIRTY_MIGRATION. vfio_listener_log_global_start() returns early.
2. calc-dirty-rate starts and adds GLOBAL_DIRTY_DIRTY_RATE.
3. The CPR fails or is cancelled. Only the migration flag is cleared, so
global_dirty_tracking stays non-zero and log_global_stop is not called.
4. A later live migration calls
memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION). old_flags is
non-zero, so VFIO log_global_start() is not invoked again.
After that, vfio_log_sync_needed() returns false:
hw/vfio/listener.c:vfio_log_sync_needed() {
...
if (!vfio_container_dirty_tracking_is_started(bcontainer)) {
return false;
}
...
}
The VFIO dirty bitmap sync is then skipped without any warning. Could that
cause a later live migration to send stale guest RAM for pages dirtied by
the device?
The commit message acknowledges this as a "Known gap", but I did not find
anything in the series that restarts tracking. Would it be possible to
restart VFIO tracking when the CPR ends or fails, or to avoid the early
return in a way that does not depend on the first-user transition?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=27
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 28/36] VZ: vfio/cpr: don't close KVM on cpr-exec
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (26 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 27/36] vfio/listener: skip dirty page tracking during CPR Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:29 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 29/36] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
` (7 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
With VFIO devices, KVM is closed on the source when the migration
completes, so that the new QEMU can create its own KVM VM with the same
devices. That is done from a migration notifier, which for cpr-exec
runs before cpr_exec_cb(). But cpr_exec_cb() may still give up, when
the memory can't be preserved or execve() fails, and resume the VM in
the old QEMU. A VM without KVM can't resume: kvm_close() removes the
vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
(KVM_SET_CLOCK in kvmclock_vm_state_change()).
cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
kernel closes them during execve(), before the new QEMU runs, and
kvm_vm_release() tears down the irqfds then. Keep the notifier for
cpr-transfer only, where the source process lives on.
Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
preserve_ram() and one with a bogus cpr-exec-command both left the VM
running with no lost pings, and a later cpr-exec completed.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/cpr.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index db462aabcbf..7ca761d5bf5 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -192,13 +192,16 @@ static int vfio_cpr_kvm_close_notifier(NotifierWithReturn *notifier,
return 0;
}
+/*
+ * Not for cpr-exec: the exec closes the KVM fds anyway, and until then
+ * cpr_exec_cb() may still give up and resume the VM, which needs KVM.
+ */
void vfio_cpr_add_kvm_notifier(void)
{
if (!kvm_close_notifier.notify) {
- migration_add_notifier_modes(&kvm_close_notifier,
- vfio_cpr_kvm_close_notifier,
- MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
- -1);
+ migration_add_notifier_mode(&kvm_close_notifier,
+ vfio_cpr_kvm_close_notifier,
+ MIG_MODE_CPR_TRANSFER);
}
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 28/36] VZ: vfio/cpr: don't close KVM on cpr-exec
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 28/36] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
@ 2026-10-23 17:29 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:29 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
> With VFIO devices, KVM is closed on the source when the migration
> completes, so that the new QEMU can create its own KVM VM with the same
> devices. That is done from a migration notifier, which for cpr-exec
> runs before cpr_exec_cb(). But cpr_exec_cb() may still give up, when
> the memory can't be preserved or execve() fails, and resume the VM in
> the old QEMU. A VM without KVM can't resume: kvm_close() removes the
> vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
> (KVM_SET_CLOCK in kvmclock_vm_state_change()).
>
> cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
> kernel closes them during execve(), before the new QEMU runs, and
> kvm_vm_release() tears down the irqfds then. Keep the notifier for
> cpr-transfer only, where the source process lives on.
>
> Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
> preserve_ram() and one with a bogus cpr-exec-command both left the VM
> running with no lost pings, and a later cpr-exec completed.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=28
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 29/36] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (27 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 28/36] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:29 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 30/36] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
` (6 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
cpr_exec_cb() marks all saved ramblocks with MADV_DOEXEC before exec, but
ignores the result. If madvise() fails for some block, the walk stops
there and the remaining blocks are not marked, yet execvp() still runs.
The new QEMU then reuses addresses of guest RAM that wasn't preserved,
and the guest is lost.
Treat that as a failed cpr-exec: go to the same failure path as when
execvp() fails, so the VM keeps running in the old QEMU.
On that path also clear MADV_DOEXEC from all saved ramblocks again, just
like the preserved fds get close-on-exec back.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/cpr-exec.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 5a885422154..df11b0c7633 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -157,6 +157,19 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
return true;
}
+static bool unpreserve_ram(cpr_ramblock_t ram, char *name)
+{
+#ifdef MADV_DONTEXEC
+ qemu_madvise((void *) ram.addr, ram.length, MADV_DONTEXEC);
+#endif
+ return true;
+}
+
+static void cpr_exec_unpreserve_ram(void)
+{
+ cpr_walk_ramblock(unpreserve_ram);
+}
+
static bool mark_ram_rdonly(cpr_ramblock_t ram, char *name)
{
/*
@@ -205,7 +218,10 @@ static void cpr_exec_cb(void *opaque)
*/
cpr_exec_preserve_fds();
- cpr_walk_ramblock(preserve_ram);
+ if (!cpr_walk_ramblock(preserve_ram)) {
+ /* preserve_ram() has already reported the error and failed migration */
+ goto fail;
+ }
if (cpr_ram_rdonly) {
cpr_walk_ramblock(mark_ram_rdonly);
}
@@ -224,18 +240,20 @@ static void cpr_exec_cb(void *opaque)
* or the system is very short on resources.
*/
error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
- g_clear_pointer(&argv, g_strfreev);
- cpr_exec_unpreserve_fds();
- if (cpr_ram_rdonly) {
- cpr_ram_restore_rw();
- }
-
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
migrate_set_error(s, err);
error_free(err);
err = NULL;
+fail:
+ g_clear_pointer(&argv, g_strfreev);
+ cpr_exec_unpreserve_fds();
+ cpr_exec_unpreserve_ram();
+ if (cpr_ram_rdonly) {
+ cpr_ram_restore_rw();
+ }
+
/* Note, we can go from state COMPLETED to FAILED */
migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 30/36] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (28 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 29/36] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:31 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 31/36] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released Andrey Drobyshev
` (5 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
Right before execve(), cpr_exec_cb() releases the kernel vhost-net
devices with VHOST_RESET_OWNER, so that the new QEMU can take them. If
execve() fails, the VM goes on in the old QEMU, but nobody takes the
devices back: the kernel refuses the ioctls of the next
vhost_net_start(), and the guest is left without network.
Take them back with VHOST_SET_OWNER when the exec fails, and set the
vring call and err fds again: VHOST_RESET_OWNER dropped them too, and
vhost_dev_init() registers them only once. The devices are stopped at
that point, and vhost_net_start() sets up the rest again when the VM
resumes. Only the devices that were actually released are taken back:
vhost_net_cpr_reset_owners() marks them. vhost-vsock does the same in
its CPR notifier.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/net/vhost_net.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/virtio/vhost.h | 2 ++
include/net/vhost_net.h | 1 +
migration/cpr-exec.c | 2 ++
4 files changed, 54 insertions(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 56c697f4bcd..fae6b060c6f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -594,6 +594,55 @@ void vhost_net_cpr_reset_owners(void)
if (r < 0 && r != -ENOSYS) {
warn_report("cpr: failed to reset vhost-net owner for %s: %s",
nc->name, strerror(-r));
+ } else if (r == 0) {
+ net->cpr_owner_reset = true;
+ }
+ }
+ }
+}
+
+/*
+ * Take the ownership back when the exec fails and the VM goes on in the
+ * old QEMU. VHOST_RESET_OWNER has reset the kernel device, but the devices
+ * are stopped, and vhost_net_start() sets everything up again when the VM
+ * resumes.
+ */
+void vhost_net_cpr_set_owners(void)
+{
+ NetClientState *nc;
+
+ QTAILQ_FOREACH(nc, &net_clients, next) {
+ VHostNetState *net = get_vhost_net(nc);
+
+ if (net && net->cpr_owner_reset) {
+ struct vhost_dev *dev = &net->dev;
+ int i, r = vhost_dev_set_owner(dev);
+
+ if (r < 0) {
+ warn_report("cpr: failed to set vhost-net owner for %s: %s",
+ nc->name, strerror(-r));
+ continue;
+ }
+ net->cpr_owner_reset = false;
+
+ /* RESET_OWNER also dropped the vring fds set by vhost_dev_init() */
+ for (i = 0; i < dev->nvqs; i++) {
+ const VhostOps *ops = dev->vhost_ops;
+ struct vhost_virtqueue *vq = &dev->vqs[i];
+ struct vhost_vring_file file = {
+ .index = ops->vhost_get_vq_index(dev, dev->vq_index + i),
+ .fd = event_notifier_get_wfd(&vq->masked_notifier),
+ };
+
+ r = ops->vhost_set_vring_call(dev, &file);
+ if (r == 0 && ops->vhost_set_vring_err) {
+ file.fd = event_notifier_get_fd(&vq->error_notifier);
+ r = ops->vhost_set_vring_err(dev, &file);
+ }
+ if (r < 0) {
+ warn_report("cpr: failed to set vring fds of %s: %s",
+ nc->name, strerror(-r));
+ }
}
}
}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a92439d3443..421f7139cfb 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -151,6 +151,8 @@ struct vhost_net {
SaveAcketFeatures *save_acked_features;
bool is_vhost_user;
NetClientState *nc;
+ /* released for cpr-exec, take back if it fails */
+ bool cpr_owner_reset;
};
/**
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 39ce7ad0e3d..cdfefa81500 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -90,4 +90,5 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
void vhost_net_save_acked_features(NetClientState *nc);
void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_set_owners(void);
#endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index df11b0c7633..e4ea160698c 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -253,6 +253,8 @@ fail:
if (cpr_ram_rdonly) {
cpr_ram_restore_rw();
}
+ /* Still the same mm, take back the vhost-net devices we released */
+ vhost_net_cpr_set_owners();
/* Note, we can go from state COMPLETED to FAILED */
migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 30/36] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 30/36] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
@ 2026-10-23 17:31 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:31 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-net: missing vhost_net_cpr_set_owners() stub breaks link without vhost-net
--
commit 254c845f4346569ec4ac6196d59e129392d054ac
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
This patch adds vhost_net_cpr_set_owners(), which calls VHOST_SET_OWNER
again and re-registers the vring call and err fds for vhost-net devices
that were released before execve(). It is called from the fail path in
cpr_exec_cb() when exec fails. A new cpr_owner_reset flag in struct
vhost_net records which devices were actually released.
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index df11b0c7633cb..e4ea160698cd4 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
> @@ -253,6 +253,8 @@ fail:
> if (cpr_ram_rdonly) {
> cpr_ram_restore_rw();
> }
> + /* Still the same mm, take back the vhost-net devices we released */
> + vhost_net_cpr_set_owners();
>
> /* Note, we can go from state COMPLETED to FAILED */
> migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
[Severity: Low]
Will this link in configurations that build hw/net/vhost_net-stub.c
instead of hw/net/vhost_net.c?
cpr_exec_cb() in migration/cpr-exec.c is built unconditionally. It now
calls vhost_net_cpr_set_owners() right after the existing call to
vhost_net_cpr_reset_owners().
hw/net/meson.build builds vhost_net.c only when vhost-net and
CONFIG_VIRTIO_NET are enabled:
system_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('vhost_net.c'), if_false: files('vhost_net-stub.c'))
Other configurations get vhost_net-stub.c. Examples are
--disable-vhost-net, non-Linux hosts, and builds without VIRTIO_NET.
That file ends after vhost_net_save_acked_features() and defines neither
vhost_net_cpr_reset_owners() nor vhost_net_cpr_set_owners().
I found both functions only in hw/net/vhost_net.c and
include/net/vhost_net.h, including at the end of the series. This patch
therefore adds a second undefined reference in stub builds. The missing
stub for vhost_net_cpr_reset_owners() was already there before this
patch.
Could stubs for both functions be added to vhost_net-stub.c?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=30
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 31/36] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (29 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 30/36] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:30 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
` (4 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
vhost_net_cpr_reset_owners() only warns when VHOST_RESET_OWNER fails for
a device, and the exec goes on. But the new QEMU can't take such a
device: it is still owned by the old mm, so VHOST_SET_OWNER fails with
EBUSY, and the guest is left without network after the update.
Stop at the first device that can't be released and abort the cpr-exec
instead. The VM goes on in the old QEMU, with the devices released so
far taken back. vhost-vsock already does that in its pre_save.
Fixes: 1278e4830052 ("VZ: migration/cpr-exec: vhost-net: send RESET_OWNER before cpr-exec")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/net/vhost_net.c | 19 +++++++++++++------
include/net/vhost_net.h | 2 +-
migration/cpr-exec.c | 7 ++++++-
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index fae6b060c6f..9110dc43d23 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -579,8 +579,12 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
* EBUSY. Called right before execve(), when the VM is stopped and the
* devices have already been stopped, so nothing is in flight. The device is
* fully re-initialized from the migrated state by the new QEMU.
+ *
+ * Stops at the first device that can't be released, since the new QEMU
+ * couldn't take it. The caller then takes back the ones already released
+ * with vhost_net_cpr_set_owners().
*/
-void vhost_net_cpr_reset_owners(void)
+void vhost_net_cpr_reset_owners(Error **errp)
{
NetClientState *nc;
@@ -591,12 +595,15 @@ void vhost_net_cpr_reset_owners(void)
net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
int r = vhost_dev_reset_owner(&net->dev);
- if (r < 0 && r != -ENOSYS) {
- warn_report("cpr: failed to reset vhost-net owner for %s: %s",
- nc->name, strerror(-r));
- } else if (r == 0) {
- net->cpr_owner_reset = true;
+ if (r == -ENOSYS) {
+ continue;
+ }
+ if (r < 0) {
+ error_setg_errno(errp, -r, "cpr: failed to reset vhost-net "
+ "owner for %s", nc->name);
+ return;
}
+ net->cpr_owner_reset = true;
}
}
}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index cdfefa81500..679edefa1d3 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -89,6 +89,6 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
void vhost_net_save_acked_features(NetClientState *nc);
-void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_reset_owners(Error **errp);
void vhost_net_cpr_set_owners(void);
#endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index e4ea160698c..b41dcc46494 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -230,7 +230,10 @@ static void cpr_exec_cb(void *opaque)
* Hand the preserved vhost-net devices over to the new QEMU: they are
* still owned by this mm, which is about to be replaced by execve().
*/
- vhost_net_cpr_reset_owners();
+ vhost_net_cpr_reset_owners(&err);
+ if (err) {
+ goto fail_with_err;
+ }
trace_cpr_exec();
execvp(argv[0], argv);
@@ -240,6 +243,8 @@ static void cpr_exec_cb(void *opaque)
* or the system is very short on resources.
*/
error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
+
+fail_with_err:
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
migrate_set_error(s, err);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (30 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 31/36] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:36 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 33/36] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
` (3 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
After cpr-exec, new QEMU mmaps the device BARs again on the preserved
device fd. Some drivers don't allow that: the NVIDIA vGPU VFIO driver
refuses to map a VF BAR that is still recorded as mapped by old QEMU,
and that record is dropped only when the device fd is closed. So BAR1
and BAR3 of the vGPU can't be mapped, and the guest loses the device.
Keep the mappings instead. Save each BAR mapping in the CPR ramblock
list, so cpr_exec_cb() marks it with MADV_DOEXEC together with guest RAM,
and new QEMU finds it there and reuses it instead of calling mmap().
Same find-or-create pattern as for other preserved objects, e.g. vector
eventfds in vfio_notifier_init(). No saved mapping means old QEMU didn't
map the region, e.g. its mmap failed or the region is mapped lazily, so
new QEMU maps it afresh.
The ID is "<device name> region <index> mmaps[<index>]". It doesn't
depend on the device path, which is gone when the display region is
finalized, so the entry is dropped together with the mapping.
The saved addresses only make sense for cpr-exec, which keeps the
address space. A cpr-transfer target is another process and would have
to map the regions anew, which the drivers above refuse while the old
mappings exist. So block cpr-transfer for VFIO devices.
This needs a kernel which supports MADV_DOEXEC on shared PFN mappings of
device memory. Without it cpr-exec of a VM with VFIO devices fails.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/device.c | 24 +++++++++++++++++++++--
hw/vfio/region.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
include/hw/vfio/vfio-cpr.h | 1 +
3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 5a14a7d5c0a..2a7073ccec8 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
TYPE_VFIO_IOMMU_IOMMUFD :
TYPE_VFIO_IOMMU_LEGACY;
- return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
- as, errp);
+ /*
+ * Only cpr-exec keeps the address space and so the region mappings,
+ * see vfio_region_mmap(). A cpr-transfer target would have to map the
+ * regions anew, which some drivers (NVIDIA vGPU) refuse.
+ */
+ error_setg(&vbasedev->cpr.mmap_blocker,
+ "cpr-transfer is not supported with VFIO devices; "
+ "use the cpr-exec mode");
+ if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
+ MIG_MODE_CPR_TRANSFER, -1)) {
+ return false;
+ }
+
+ if (!vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
+ as, errp)) {
+ migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+ return false;
+ }
+
+ return true;
}
void vfio_device_detach(VFIODevice *vbasedev)
{
+ migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+
if (!vbasedev->bcontainer) {
return;
}
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index b0332f27fcd..86a34b42c57 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -30,6 +30,7 @@
#include "qemu/units.h"
#include "monitor/monitor.h"
#include "system/ramblock.h"
+#include "migration/cpr.h"
#include "vfio-helpers.h"
/*
@@ -227,8 +228,47 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
return 0;
}
+static char *vfio_region_cpr_name(VFIORegion *region, int index)
+{
+ return g_strdup_printf("%s region %d mmaps[%d]", region->vbasedev->name,
+ region->nr, index);
+}
+
+/*
+ * A saved mapping is one preserved by cpr-exec, reuse it. None means old
+ * QEMU didn't map the region, e.g. its mmap failed or it's mapped lazily,
+ * so map it afresh.
+ */
+static void *vfio_region_cpr_find(VFIORegion *region, int index)
+{
+ g_autofree char *name = vfio_region_cpr_name(region, index);
+ cpr_ramblock_t ram = cpr_find_ramblock(name);
+
+ if (ram.addr && ram.length != region->mmaps[index].size) {
+ error_report("CPR mode=exec is requested, but the mapping of %s "
+ "can't be reused: addr=%p length=%zu, expected "
+ "length=%zu. Aborting", name, (void *)ram.addr,
+ ram.length, region->mmaps[index].size);
+ exit(1);
+ }
+
+ return (void *)(uintptr_t)ram.addr;
+}
+
+static void vfio_region_cpr_save(VFIORegion *region, int index)
+{
+ g_autofree char *name = vfio_region_cpr_name(region, index);
+ void *addr = region->mmaps[index].mmap;
+ size_t size = region->mmaps[index].size;
+
+ cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
+}
+
static void vfio_subregion_unmap(VFIORegion *region, int index)
{
+ g_autofree char *cpr_name = vfio_region_cpr_name(region, index);
+
+ cpr_delete_ramblock(cpr_name);
trace_vfio_region_unmap(memory_region_name(®ion->mmaps[index].mem),
region->mmaps[index].offset,
region->mmaps[index].offset +
@@ -314,6 +354,11 @@ int vfio_region_mmap(VFIORegion *region)
size_t align = MIN(1ULL << ctz64(region->mmaps[i].size), 1 * GiB);
void *map_base, *map_align;
+ region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
+ if (region->mmaps[i].mmap) {
+ goto mapped;
+ }
+
/*
* Align the mmap for more efficient mapping in the kernel. Ideally
* we'd know the PMD and PUD mapping sizes to use as discrete alignment
@@ -347,7 +392,9 @@ int vfio_region_mmap(VFIORegion *region)
ret = -errno;
goto no_mmap;
}
+ vfio_region_cpr_save(region, i);
+mapped:
name = g_strdup_printf("%s mmaps[%d]",
memory_region_name(region->mem), i);
memory_region_init_ram_device_ptr(®ion->mmaps[i].mem,
@@ -427,6 +474,8 @@ void vfio_region_finalize(VFIORegion *region)
for (i = 0; i < region->nr_mmaps; i++) {
if (region->mmaps[i].mmap) {
+ g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
+ cpr_delete_ramblock(cpr_name);
munmap(region->mmaps[i].mmap, region->mmaps[i].size);
}
}
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index 4606da500a7..fa1699857ca 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -34,6 +34,7 @@ typedef struct VFIOContainerCPR {
typedef struct VFIODeviceCPR {
Error *mdev_blocker;
Error *id_blocker;
+ Error *mmap_blocker;
uint32_t hwpt_id;
uint32_t ioas_id;
} VFIODeviceCPR;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-23 17:36 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:36 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] vfio: unclaimed CPR BAR mmap entries are never reconciled after cpr-exec
- [Low] vfio: BAR mmaps in the CPR ramblock list get RAM-only mprotect in cpr-ram-rdonly
--
commit a087f4249b17de71aa8260f7340069a28fcc6353
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
This patch saves each VFIO BAR mmap in the CPR ramblock list so that
cpr_exec_cb() preserves it with MADV_DOEXEC. After cpr-exec, new QEMU finds
the saved mapping and reuses it instead of calling mmap() again. It also
blocks cpr-transfer for VFIO devices.
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
[ ... ]
> +/*
> + * A saved mapping is one preserved by cpr-exec, reuse it. None means old
> + * QEMU didn't map the region, e.g. its mmap failed or it's mapped lazily,
> + * so map it afresh.
> + */
> +static void *vfio_region_cpr_find(VFIORegion *region, int index)
> +{
> + g_autofree char *name = vfio_region_cpr_name(region, index);
> + cpr_ramblock_t ram = cpr_find_ramblock(name);
> +
> + if (ram.addr && ram.length != region->mmaps[index].size) {
> + error_report("CPR mode=exec is requested, but the mapping of %s "
> + "can't be reused: addr=%p length=%zu, expected "
> + "length=%zu. Aborting", name, (void *)ram.addr,
> + ram.length, region->mmaps[index].size);
> + exit(1);
> + }
> +
> + return (void *)(uintptr_t)ram.addr;
> +}
[Severity: Low]
Is there a path that removes an unclaimed entry after cpr-exec?
vfio_region_cpr_find() only reads the entry with cpr_find_ramblock(). It
never consumes it or marks it as claimed. The entry is removed only by
vfio_subregion_unmap() and vfio_region_finalize().
Suppose new QEMU never calls vfio_region_mmap() for a region that old QEMU
had saved. Examples are a different device configuration, a no-mmap
configuration, or a region that new QEMU maps lazily. The entry then stays
in cpr_state.ramblocks.
cpr_exec_cb() calls cpr_walk_ramblock(preserve_ram), which walks every
entry in the list. A stale BAR entry would be marked MADV_DOEXEC again at
the next cpr-exec.
If a stale entry is later found by name with a different length, this code
calls exit(1) instead of falling back to a fresh mmap.
I could not find any code in the series that reconciles unclaimed entries.
Should unclaimed entries be dropped after the new QEMU finishes device
setup, and should a size mismatch fall back to a fresh mmap?
> +
> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> + g_autofree char *name = vfio_region_cpr_name(region, index);
> + void *addr = region->mmaps[index].mmap;
> + size_t size = region->mmaps[index].size;
> +
> + cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}
[Severity: Low]
Can putting device BAR mappings into the generic CPR ramblock list
interfere with the -cpr-ram-rdonly debug option?
vfio_region_cpr_save() adds each BAR mmap to the same list that
migration/cpr-exec.c treats as guest RAM. When cpr_ram_rdonly is set,
cpr_exec_cb() runs cpr_walk_ramblock(mark_ram_rdonly) over this list, and
that now includes the BAR mappings.
mark_ram_rw() runs on the exec-failure path and in the new QEMU. It calls
qemu_mprotect_rw() with PROT_READ|PROT_WRITE on every entry, and it calls
abort() on failure:
migration/cpr-exec.c:mark_ram_rw() {
if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
...
abort();
}
}
vfio_region_mmap() derives prot from region->flags (READ and WRITE). A BAR
mapped read-only or with no access would be widened to read-write. If the
kernel or a driver rejects the mprotect(), QEMU aborts.
Typical vfio-pci BARs are read-write, and the option is debug-only. Should
BAR entries be excluded from the mark_ram_rdonly and mark_ram_rw walks?
[ ... ]
> @@ -427,6 +474,8 @@ void vfio_region_finalize(VFIORegion *region)
>
> for (i = 0; i < region->nr_mmaps; i++) {
> if (region->mmaps[i].mmap) {
> + g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> + cpr_delete_ramblock(cpr_name);
> munmap(region->mmaps[i].mmap, region->mmaps[i].size);
> }
> }
This is the second of only two places that delete a BAR entry, as noted in
the vfio_region_cpr_find() comment above.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=32
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 33/36] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (31 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 32/36] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:32 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 34/36] VZ: vfio/container: lift the CPR blocker for mdev " Andrey Drobyshev
` (2 subsequent siblings)
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
Commit bb8da93ddfc4 ("VZ: migration/cpr-exec: enable checks prohibiting
cpr-exec #VSTOR-108564") introduced restrictions for cpr-exec migration
mode. Among them was prohibition to do such migration in case of
presence of any VFIO devices. Now we want to allow migration with such
devices in scope of VSTOR-103860. Therefore, let's lift that restiction.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/device.c | 5 -----
include/hw/vfio/vfio-device.h | 2 --
migration/migration.c | 3 ---
3 files changed, 10 deletions(-)
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 2a7073ccec8..d1b595adb71 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -36,11 +36,6 @@
VFIODeviceList vfio_device_list =
QLIST_HEAD_INITIALIZER(vfio_device_list);
-bool vfio_devices_present(void)
-{
- return !QLIST_EMPTY(&vfio_device_list);
-}
-
/*
* We want to differentiate hot reset of multiple in-use devices vs
* hot reset of a single in-use device. VFIO_DEVICE_RESET will already
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 935000f0074..fc28f580ba3 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -162,8 +162,6 @@ VFIODevice *vfio_get_vfio_device(Object *obj);
typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
extern VFIODeviceList vfio_device_list;
-bool vfio_devices_present(void);
-
#ifdef CONFIG_LINUX
/*
* How devices communicate with the server. The default option is through
diff --git a/migration/migration.c b/migration/migration.c
index 821764afae9..57aa0dea264 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2115,9 +2115,6 @@ static bool migrate_cpr_is_blocked(Error **errp)
} else if (cbw_filter_present()) {
error_setg(errp, "CPR is prohibited: active block snapshots (CBW "
"filter) present");
- } else if (vfio_devices_present()) {
- error_setg(errp, "CPR is prohibited: passthrough (VFIO) "
- "devices present");
} else {
res = false;
}
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* [PATCH QEMU HCI-8.1 v5 34/36] VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (32 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 33/36] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:34 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595 Andrey Drobyshev
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595 Andrey Drobyshev
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
Upstream commit dac0dd68d9b ("vfio/container: mdev cpr blocker") blocks
cpr-transfer and cpr-exec for mdev devices on the legacy container, as
the kernel refuses VFIO_UPDATE_VADDR for them since ef3a3f6a294b
("vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR"). We want CPR for
mdev vGPUs, so lift the blocker.
This needs a kernel which reverts ef3a3f6a294b, as UEK does. Without
it the VFIO_UPDATE_VADDR extension check in vfio_cpr_supported() still
blocks CPR.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/container-legacy.c | 13 -------------
include/hw/vfio/vfio-cpr.h | 1 -
2 files changed, 14 deletions(-)
diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
index e437eb0c741..285e594b048 100644
--- a/hw/vfio/container-legacy.c
+++ b/hw/vfio/container-legacy.c
@@ -988,20 +988,8 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
goto device_put_exit;
}
- if (vbasedev->mdev) {
- error_setg(&vbasedev->cpr.mdev_blocker,
- "CPR does not support vfio mdev %s", vbasedev->name);
- if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
- MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
- -1) < 0) {
- goto hiod_unref_exit;
- }
- }
-
return true;
-hiod_unref_exit:
- object_unref(vbasedev->hiod);
device_put_exit:
vfio_device_put(vbasedev);
group_put_exit:
@@ -1017,7 +1005,6 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
vfio_device_unprepare(vbasedev);
- migrate_del_blocker(&vbasedev->cpr.mdev_blocker);
object_unref(vbasedev->hiod);
vfio_device_put(vbasedev);
vfio_group_put(group);
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index fa1699857ca..a486b3b2578 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -32,7 +32,6 @@ typedef struct VFIOContainerCPR {
} VFIOContainerCPR;
typedef struct VFIODeviceCPR {
- Error *mdev_blocker;
Error *id_blocker;
Error *mmap_blocker;
uint32_t hwpt_id;
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 34/36] VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 34/36] VZ: vfio/container: lift the CPR blocker for mdev " Andrey Drobyshev
@ 2026-10-23 17:34 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:34 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio: mdev CPR blocker removed; kernel capability check only runs once per container
--
commit df2c9f0ff7e7012bfa39fe116e7f04b6a2a0cd4e
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860
This patch removes the per-device mdev CPR blocker from the legacy VFIO
container code. It also drops the now-unused mdev_blocker field from
VFIODeviceCPR. The commit message says a kernel that reverts ef3a3f6a294b
is required, and that the VFIO_UPDATE_VADDR extension check in
vfio_cpr_supported() still blocks CPR otherwise.
> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
> index e437eb0c7417..285e594b048b 100644
> --- a/hw/vfio/container-legacy.c
> +++ b/hw/vfio/container-legacy.c
> @@ -988,20 +988,8 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
> goto device_put_exit;
> }
>
> - if (vbasedev->mdev) {
> - error_setg(&vbasedev->cpr.mdev_blocker,
> - "CPR does not support vfio mdev %s", vbasedev->name);
> - if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
> - MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
> - -1) < 0) {
> - goto hiod_unref_exit;
> - }
> - }
> -
> return true;
[Severity: Medium]
Can CPR now be accepted at command time and then fail partway through on
a host kernel that still carries ef3a3f6a294b?
The commit message relies on the VFIO_UPDATE_VADDR extension check in
vfio_cpr_supported() as the remaining gate:
hw/vfio/cpr-legacy.c:vfio_cpr_supported() {
...
if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR)) {
error_setg(errp, "VFIO container does not support VFIO_UPDATE_VADDR");
...
}
That check is per container. It is reached only through
vfio_legacy_cpr_register_container(), which vfio_container_connect()
calls when it creates a new container:
if (!vfio_legacy_cpr_register_container(container, errp)) {
goto fail;
}
A group or device that joins an existing container goes through
vfio_container_group_add() and skips that check. The removed per-device
blocker was the only check that covered this case.
For example, an mdev could be added to a container that was already
registered for a physical device in the same address space. Nothing
would re-check it. If the kernel refuses VADDR unmap for that mdev,
the failure shows up late, in vfio_container_pre_save():
if (!vfio_dma_unmap_vaddr_all(container, &local_err)) {
error_report_err(local_err);
return -1;
}
That would fail the migration mid-CPR instead of blocking it up front.
I did not check the kernel source, so how ef3a3f6a294b treats an mdev
added after container creation is inferred, not verified.
Separately, the blocker is now lifted for every mdev type, not only
vGPUs. Would it make sense to limit it, or to add a per-device
capability check?
> -hiod_unref_exit:
> - object_unref(vbasedev->hiod);
> device_put_exit:
[ ... ]
> @@ -1017,7 +1005,6 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
>
> vfio_device_unprepare(vbasedev);
>
> - migrate_del_blocker(&vbasedev->cpr.mdev_blocker);
> object_unref(vbasedev->hiod);
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=34
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (33 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 34/36] VZ: vfio/container: lift the CPR blocker for mdev " Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:38 ` no-reply
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595 Andrey Drobyshev
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
QEMU estimates the switchover bandwidth from the last update window.
Once the guest RAM is mostly clean, the sender runs out of data within
a window and the estimate collapses to the trickle it carried (13 MiB/s
on a 25 Gbit/s link). Device state which only moves once the guest is
stopped, like that of an NVIDIA vGPU, then never fits into the downtime
limit and the migration never converges. avail-switchover-bandwidth
would help, but nothing sets it for us. Meanwhile the migration thread
loops without waiting, re-syncing the dirty logs about once a
millisecond.
Keep the previous estimate over a window in which every device ran out
of data, and wait such a window out, as is done when the rate limit is
exceeded. Migrations which converge today don't see such a window
before the switchover, so they're unaffected.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
migration/migration.c | 21 +++++++++++++++++----
migration/migration.h | 4 ++++
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 57aa0dea264..5f488836ab1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1839,6 +1839,7 @@ int migrate_init(MigrationState *s, Error **errp)
s->state = MIGRATION_STATUS_NONE;
s->rp_state.from_dst_file = NULL;
s->mbps = 0.0;
+ s->switchover_bw_per_ms = 0.0;
s->pages_per_second = 0.0;
s->downtime = 0;
s->expected_downtime = 0;
@@ -3452,6 +3453,7 @@ static void update_iteration_initial_status(MigrationState *s)
s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
s->iteration_initial_bytes = migration_transferred_bytes();
s->iteration_initial_pages = ram_get_total_transferred_pages();
+ s->iteration_starved = false;
}
static void migration_update_counters(MigrationState *s,
@@ -3481,8 +3483,15 @@ static void migration_update_counters(MigrationState *s,
*/
expected_bw_per_ms = switchover_bw / 1000;
} else {
- /* If the user doesn't specify bandwidth, we use the estimated */
- expected_bw_per_ms = bandwidth;
+ /*
+ * If the user doesn't specify bandwidth, we use the estimated.
+ * Keep it over a window the sender ran out of data in: such a
+ * window only shows how little was left to send.
+ */
+ if (!s->iteration_starved || !s->switchover_bw_per_ms) {
+ s->switchover_bw_per_ms = bandwidth;
+ }
+ expected_bw_per_ms = s->switchover_bw_per_ms;
}
s->threshold_size = expected_bw_per_ms * migrate_downtime_limit();
@@ -3601,7 +3610,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
}
/* Just another iteration step */
- qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
+ if (qemu_savevm_state_iterate(s->to_dst_file, in_postcopy) > 0) {
+ /* Every iterable device had nothing more to send */
+ s->iteration_starved = true;
+ }
return MIG_ITERATE_RESUME;
}
@@ -3719,7 +3731,8 @@ bool migration_rate_limit(void)
bool urgent = false;
migration_update_counters(s, now);
- if (migration_rate_exceeded(s->to_dst_file)) {
+ /* Also wait out the window if the sender ran out of data */
+ if (migration_rate_exceeded(s->to_dst_file) || s->iteration_starved) {
if (qemu_file_get_error(s->to_dst_file)) {
return false;
diff --git a/migration/migration.h b/migration/migration.h
index ee15ca88461..e2cdb087321 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -350,6 +350,10 @@ struct MigrationState {
} rp_state;
double mbps;
+ /* Switchover bandwidth estimate (bytes/ms), kept over starved windows */
+ double switchover_bw_per_ms;
+ /* The sender ran out of data during the current update window */
+ bool iteration_starved;
/* Timestamp when recent migration starts (ms) */
int64_t start_time;
/* Total time used by latest migration (ms) */
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595 Andrey Drobyshev
@ 2026-10-23 17:38 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:38 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] migration: iteration_starved set from iterate() 'done' even after a busy window
- [Low] migration: starved-window wait in migration_rate_limit() delays completion up to 100ms
--
commit b0713b8ba87d723db3d095914dcfe10764c0314e
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595
This patch keeps the previous switchover bandwidth estimate over an update
window in which every iterable device reported it had nothing more to send.
It also makes migration_rate_limit() wait out such a window, the same way it
does when the rate limit is exceeded.
> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -3481,8 +3483,15 @@ static void migration_update_counters(MigrationState *s,
> */
> expected_bw_per_ms = switchover_bw / 1000;
> } else {
> - /* If the user doesn't specify bandwidth, we use the estimated */
> - expected_bw_per_ms = bandwidth;
> + /*
> + * If the user doesn't specify bandwidth, we use the estimated.
> + * Keep it over a window the sender ran out of data in: such a
> + * window only shows how little was left to send.
> + */
> + if (!s->iteration_starved || !s->switchover_bw_per_ms) {
> + s->switchover_bw_per_ms = bandwidth;
> + }
> + expected_bw_per_ms = s->switchover_bw_per_ms;
> }
[Severity: Low]
The cached switchover_bw_per_ms is maintained only in this branch and is
reset only in migrate_init().
If avail-switchover-bandwidth is set and later cleared during a migration,
could the cached estimate be stale, or zero?
In the zero case the code falls back to the current window bandwidth, so
this looks minor.
[ ... ]
> @@ -3601,7 +3610,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
> }
>
> /* Just another iteration step */
> - qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
> + if (qemu_savevm_state_iterate(s->to_dst_file, in_postcopy) > 0) {
> + /* Every iterable device had nothing more to send */
> + s->iteration_starved = true;
> + }
> return MIG_ITERATE_RESUME;
> }
[Severity: Medium]
Does a return value greater than 0 from qemu_savevm_state_iterate() really
mean that the window was starved?
qemu_savevm_state_iterate() returns all_finished, which stays true as long
as no device returned 0:
migration/savevm.c:qemu_savevm_state_iterate() {
bool all_finished = true;
...
} else if (!ret) {
all_finished = false;
}
...
return all_finished;
}
That value says nothing about how much data was sent in the call or in the
window. ram_save_iterate() can send many pages in its loop and then hit:
migration/ram.c:ram_save_iterate() {
...
pages = ram_find_and_save_block(rs);
/* no more pages to sent */
if (pages == 0) {
done = 1;
break;
}
...
}
So it can return 1 right after a large send. The result is also 1 when
every device was skipped as inactive or non-postcopy.
iteration_starved is cleared only in update_iteration_initial_status(),
when the window rolls over, so later productive iterate calls in the same
window do not clear it.
Then in migration_update_counters():
if (!s->iteration_starved || !s->switchover_bw_per_ms) {
s->switchover_bw_per_ms = bandwidth;
}
would skip the update for a window that carried, say, 80% of line rate and
then ran dry.
Is the comment "such a window only shows how little was left to send"
(and the corresponding statement in the commit message) accurate for such
a mostly busy window?
Could the flag be set only when the window actually transferred little
data?
> @@ -3719,7 +3731,8 @@ bool migration_rate_limit(void)
>
> bool urgent = false;
> migration_update_counters(s, now);
> - if (migration_rate_exceeded(s->to_dst_file)) {
> + /* Also wait out the window if the sender ran out of data */
> + if (migration_rate_exceeded(s->to_dst_file) || s->iteration_starved) {
[Severity: Low]
Can this wait delay completion by up to BUFFER_DELAY (100 ms)?
The call chain is:
migration_thread()
migration_iteration_run()
qemu_savevm_state_iterate() /* returns 1, sets iteration_starved */
migration_rate_limit()
qemu_sem_timedwait(&s->rate_limit_sem, ms)
The next migration_iteration_run(), where complete_ready is evaluated, is
then postponed until iteration_start_time + BUFFER_DELAY unless an urgent
request wakes the semaphore.
Examples are an idle guest whose pending data was just above the threshold
and was fully drained by this iterate, or the case where can_switchover is
false because the destination has not yet sent the switchover ack. Before
the patch the loop re-evaluated completion immediately.
The commit message says "Migrations which converge today don't see such a
window before the switchover, so they're unaffected." Is that strictly
true?
The same wait also applies in postcopy, where complete_ready is
!pending_size. It also applies when avail-switchover-bandwidth is set,
where the estimate is unused and the wait seems to serve no purpose. Should
the starved condition be limited to the case where the estimate is in use?
The later patch "VZ: migration: don't wait for stop-copy-only device state"
does not change iteration_starved, migration_rate_limit() or this
condition, so the behavior remains at the end of the series.
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=35
^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595
2026-10-23 16:53 [PATCH QEMU HCI-8.1 v5 00/36] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (34 preceding siblings ...)
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 35/36] VZ: migration: keep the estimate and wait over starved windows #VSTOR-150595 Andrey Drobyshev
@ 2026-10-23 16:54 ` Andrey Drobyshev
2026-10-23 17:40 ` no-reply
35 siblings, 1 reply; 73+ messages in thread
From: Andrey Drobyshev @ 2026-10-23 16:54 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
A VFIO device reports its stop-copy state as pending data. Once it
has nothing left to pre-copy, or doesn't pre-copy at all, nothing done
while the guest runs makes it smaller. If it doesn't fit into
bandwidth x downtime-limit on its own, the migration iterates until
the limit is raised, which Nova does in steps of minutes. An NVIDIA
vGPU reports ~2 GiB, so at the usual limits it never fits.
Have state_pending_exact handlers report such irreducible data and,
when it alone exceeds the threshold, switch over once the rest is
within the threshold plus as much again: waiting can't do better, and
squeezing the rest further only keeps the guest throttled. Migrations
whose pending data fits the threshold are unaffected.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/migration.c | 9 ++++++++-
migration/migration.c | 17 +++++++++++++++--
migration/migration.h | 2 ++
migration/savevm.c | 15 +++++++++++++--
migration/savevm.h | 5 +++--
5 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 8f2722dc671..c7312d9cb58 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -569,18 +569,25 @@ static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
uint64_t stop_copy_size = VFIO_MIG_STOP_COPY_SIZE;
+ int ret;
/*
* If getting pending migration size fails, VFIO_MIG_STOP_COPY_SIZE is
* reported so downtime limit won't be violated.
*/
- vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
+ ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
*must_precopy += stop_copy_size;
if (vfio_device_state_is_precopy(vbasedev)) {
vfio_query_precopy_size(migration);
}
+ /* Once there's nothing left to pre-copy, iterating can't shrink it */
+ if (!ret && !migration->precopy_init_size &&
+ !migration->precopy_dirty_size) {
+ qemu_savevm_state_pending_add_irreducible(stop_copy_size);
+ }
+
trace_vfio_state_pending_exact(vbasedev->name, *must_precopy, *can_postcopy,
stop_copy_size, migration->precopy_init_size,
migration->precopy_dirty_size);
diff --git a/migration/migration.c b/migration/migration.c
index 5f488836ab1..4a7da07fa05 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1840,6 +1840,7 @@ int migrate_init(MigrationState *s, Error **errp)
s->rp_state.from_dst_file = NULL;
s->mbps = 0.0;
s->switchover_bw_per_ms = 0.0;
+ s->irreducible_pending = 0;
s->pages_per_second = 0.0;
s->downtime = 0;
s->expected_downtime = 0;
@@ -3576,8 +3577,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
* postcopy started, so ESTIMATE should always match with EXACT
* during postcopy phase.
*/
- if (pending_size < s->threshold_size) {
- qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
+ /* Allow for what the estimate lacks, see the irreducible rule below */
+ if (pending_size < s->threshold_size + 2 * s->irreducible_pending) {
+ s->irreducible_pending =
+ qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
pending_size = must_precopy + can_postcopy;
trace_migrate_pending_exact(pending_size, must_precopy,
can_postcopy);
@@ -3601,6 +3604,16 @@ static MigIterateState migration_iteration_run(MigrationState *s)
* (which was calculated from expected downtime)
*/
complete_ready = can_switchover && (pending_size <= s->threshold_size);
+
+ /*
+ * Data which only moves once the guest is stopped can't shrink by
+ * iterating: if it alone exceeds the threshold, switch over once
+ * the rest is within the threshold plus as much again.
+ */
+ if (s->irreducible_pending > s->threshold_size) {
+ complete_ready = can_switchover &&
+ pending_size <= s->threshold_size + 2 * s->irreducible_pending;
+ }
}
if (complete_ready) {
diff --git a/migration/migration.h b/migration/migration.h
index e2cdb087321..d5df6ce495f 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -316,6 +316,8 @@ struct MigrationState {
* measured bandwidth, or avail-switchover-bandwidth if specified.
*/
uint64_t threshold_size;
+ /* Pending data which only moves once stopped, from the last exact query */
+ uint64_t irreducible_pending;
/* params from 'migrate-set-parameters' */
MigrationParameters parameters;
diff --git a/migration/savevm.c b/migration/savevm.c
index 3b768f4f4e0..f4df8308c2d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1752,13 +1752,23 @@ void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
}
}
-void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
- uint64_t *can_postcopy)
+/* Exact pending data which only moves once the guest is stopped */
+static uint64_t savevm_pending_irreducible;
+
+void qemu_savevm_state_pending_add_irreducible(uint64_t size)
+{
+ savevm_pending_irreducible += size;
+}
+
+/* Returns the part of the pending data which only moves once stopped */
+uint64_t qemu_savevm_state_pending_exact(uint64_t *must_precopy,
+ uint64_t *can_postcopy)
{
SaveStateEntry *se;
*must_precopy = 0;
*can_postcopy = 0;
+ savevm_pending_irreducible = 0;
QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
if (!se->ops || !se->ops->state_pending_exact) {
@@ -1771,6 +1781,7 @@ void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
}
se->ops->state_pending_exact(se->opaque, must_precopy, can_postcopy);
}
+ return savevm_pending_irreducible;
}
void qemu_savevm_state_cleanup(void)
diff --git a/migration/savevm.h b/migration/savevm.h
index 2d5e9c71668..dc37a4ba194 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -40,10 +40,11 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
void qemu_savevm_state_cleanup(void);
void qemu_savevm_state_complete_postcopy(QEMUFile *f);
int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only);
-void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
- uint64_t *can_postcopy);
+uint64_t qemu_savevm_state_pending_exact(uint64_t *must_precopy,
+ uint64_t *can_postcopy);
void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
uint64_t *can_postcopy);
+void qemu_savevm_state_pending_add_irreducible(uint64_t size);
int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
--
2.47.1
^ permalink raw reply [flat|nested] 73+ messages in thread* Re: [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595
2026-10-23 16:54 ` [PATCH QEMU HCI-8.1 v5 36/36] VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595 Andrey Drobyshev
@ 2026-10-23 17:40 ` no-reply
0 siblings, 0 replies; 73+ messages in thread
From: no-reply @ 2026-10-23 17:40 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] vfio: vfio_state_pending_exact() ignores vfio_query_precopy_size() failure
- [Medium] migration: irreducible-state switchover bound exceeds downtime-limit and commit message
- [Medium] migration: irreducible_pending only learned after RAM estimate drops below plain threshold
- [Medium] migration: irreducible-data switchover overrides a pending migrate-start-postcopy request
- [Low] migration: irreducible size passed via file-static global in savevm.c
--
commit f62ee5bb362c0d23b60102c51ee61dedc887ca84
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration: don't wait for stop-copy-only device state #VSTOR-150595
This patch lets state_pending_exact handlers report pending data that only
moves once the guest is stopped (the VFIO stop-copy size). When that data
alone exceeds the threshold, migration_iteration_run() switches over once
the rest is within the threshold plus as much again.
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -569,18 +569,25 @@ static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
> VFIODevice *vbasedev = opaque;
> VFIOMigration *migration = vbasedev->migration;
> uint64_t stop_copy_size = VFIO_MIG_STOP_COPY_SIZE;
> + int ret;
>
> /*
> * If getting pending migration size fails, VFIO_MIG_STOP_COPY_SIZE is
> * reported so downtime limit won't be violated.
> */
> - vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> + ret = vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> *must_precopy += stop_copy_size;
>
> if (vfio_device_state_is_precopy(vbasedev)) {
> vfio_query_precopy_size(migration);
> }
>
> + /* Once there's nothing left to pre-copy, iterating can't shrink it */
> + if (!ret && !migration->precopy_init_size &&
> + !migration->precopy_dirty_size) {
> + qemu_savevm_state_pending_add_irreducible(stop_copy_size);
> + }
[Severity: Medium]
Can a failed precopy query be treated as "nothing left to pre-copy" here?
The return value of vfio_query_precopy_size() is dropped. That function
zeroes both sizes before the ioctl and returns -errno on failure:
migration->precopy_init_size = 0;
migration->precopy_dirty_size = 0;
if (ioctl(migration->data_fd, VFIO_MIG_GET_PRECOPY_INFO, &precopy)) {
return -errno;
}
So if VFIO_MIG_GET_PRECOPY_INFO fails while the device is in PRE_COPY or
PRE_COPY_P2P, the new test "!ret && !migration->precopy_init_size &&
!migration->precopy_dirty_size" still passes, since ret only comes from
vfio_query_stop_copy_size(). The whole stop-copy size is then reported
through qemu_savevm_state_pending_add_irreducible(), even though pre-copy
data may remain that iterating would drain.
The existing code fails closed when the stop-copy size query fails, but this
path fails open. Should the return value of vfio_query_precopy_size() be
captured, and irreducible data reported only when it succeeded (or when the
device is not in a pre-copy state)?
The same logic also seems to treat a transient dirty_bytes == 0 on a
precopy-capable device as irreducible.
> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -3576,8 +3577,10 @@ static MigIterateState migration_iteration_run(MigrationState *s)
> * postcopy started, so ESTIMATE should always match with EXACT
> * during postcopy phase.
> */
> - if (pending_size < s->threshold_size) {
> - qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> + /* Allow for what the estimate lacks, see the irreducible rule below */
> + if (pending_size < s->threshold_size + 2 * s->irreducible_pending) {
> + s->irreducible_pending =
> + qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
[Severity: Medium]
Is irreducible_pending only learned after the RAM estimate has dropped
below the plain threshold_size?
irreducible_pending is set to 0 in migrate_init(), so the gate above is
the old "pending_size < s->threshold_size" until an exact query has run.
The pending_size here comes from qemu_savevm_state_pending_estimate().
vfio_state_pending_estimate() returns early when the device is not in a
precopy state, and otherwise reports only:
*must_precopy +=
migration->precopy_init_size + migration->precopy_dirty_size;
It never includes the stop-copy size. So the estimate is roughly the RAM
dirty estimate from ram_state_pending_estimate():
uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
If RAM dirtying keeps that at or above threshold_size, the exact query is
never made and irreducible_pending stays 0. The relaxed completion check
is guarded by "s->irreducible_pending > s->threshold_size", so it is never
evaluated.
Would a guest whose RAM remainder sits between threshold and threshold plus
the irreducible size, which is the case the commit message targets, then
keep iterating as before until the estimate happens to dip below the
threshold?
> pending_size = must_precopy + can_postcopy;
> trace_migrate_pending_exact(pending_size, must_precopy,
> can_postcopy);
[ ... ]
> /* Should we switch to postcopy now? */
> if (must_precopy <= s->threshold_size &&
> can_switchover && qatomic_read(&s->start_postcopy)) {
[Severity: Medium]
Can the new override below discard a pending migrate-start-postcopy
request?
must_precopy includes the VFIO stop-copy size, so when that size exceeds
threshold_size, the postcopy gate above is never satisfied and
postcopy_start() is skipped.
> @@ -3601,6 +3604,16 @@ static MigIterateState migration_iteration_run(MigrationState *s)
> * (which was calculated from expected downtime)
> */
> complete_ready = can_switchover && (pending_size <= s->threshold_size);
> +
> + /*
> + * Data which only moves once the guest is stopped can't shrink by
> + * iterating: if it alone exceeds the threshold, switch over once
> + * the rest is within the threshold plus as much again.
> + */
> + if (s->irreducible_pending > s->threshold_size) {
> + complete_ready = can_switchover &&
> + pending_size <= s->threshold_size + 2 * s->irreducible_pending;
> + }
Before the patch, pending_size <= threshold_size implied must_precopy <=
threshold_size, so a requested postcopy always took precedence over precopy
completion. The new override does not look at s->start_postcopy. Once it
becomes true, migration_completion() runs with s->state == ACTIVE and takes
the migration_completion_precopy() branch, stopping the VM and sending the
whole device state during downtime.
Should this override be skipped when start_postcopy is set (or postcopy is
enabled)?
[Severity: Medium]
Does this bound match the commit message and the comment?
The commit message says "switch over once the rest is within the threshold
plus as much again", and the comment says the same. But pending_size
already includes the irreducible bytes, since vfio_state_pending_exact()
does "*must_precopy += stop_copy_size" and then also calls
qemu_savevm_state_pending_add_irreducible(stop_copy_size). So the
condition
pending_size <= s->threshold_size + 2 * s->irreducible_pending
is equivalent to "rest <= threshold + irreducible", not roughly twice the
threshold. For a ~2 GiB vGPU, around 2 GiB plus the threshold of RAM and
other state may still be pending at switchover, so about 6 GiB in total.
This replaces the bandwidth x downtime-limit criterion from
migration_update_counters():
s->threshold_size = expected_bw_per_ms * migrate_downtime_limit();
with one that scales with the device-reported size. s->expected_downtime
is not updated, there is no capability, parameter or compat property, and
the downtime-limit documentation does not mention it. Is that intended,
and can the wording be made to match the actual bound?
> diff --git a/migration/savevm.c b/migration/savevm.c
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
[ ... ]
> +/* Exact pending data which only moves once the guest is stopped */
> +static uint64_t savevm_pending_irreducible;
> +
> +void qemu_savevm_state_pending_add_irreducible(uint64_t size)
> +{
> + savevm_pending_irreducible += size;
> +}
[Severity: Low]
This isn't a bug, but the irreducible amount is passed from device handlers
to the migration core through a file-static global.
It is reset only inside qemu_savevm_state_pending_exact(). Nothing checks
that an exact query is in progress, so a call from any other context would
be wiped by the next query or leak into it. The state_pending_exact
documentation in include/migration/register.h was not updated to describe
the new contract either, and the global is not tied to MigrationState.
Would it be cleaner to extend the handler interface, or at least document
that the function may only be called from within a state_pending_exact
handler?
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=36
^ permalink raw reply [flat|nested] 73+ messages in thread