* [PATCH QEMU HCI-8.1 v2 01/24] migration: Plug memory leaks after migrate_set_error()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:30 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 02/24] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
` (22 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 edace3f0bd8..7b7609e19cd 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -227,11 +227,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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 02/24] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 01/24] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:30 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 03/24] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
` (21 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 03/24] vfio/pci: Reject invalid MSI-X Table and PBA BIR values
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 01/24] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 02/24] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:31 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 04/24] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
` (20 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 04/24] vfio/pci: Fix information leak in vfio_rom_read()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (2 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 03/24] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:31 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 05/24] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
` (19 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 05/24] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (3 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 04/24] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:33 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 06/24] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
` (18 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 06/24] vfio/pci: Grow buffer in vfio_pci_host_match()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (4 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 05/24] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:32 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 07/24] q35: Fix migration of SMRAM state Andrey Drobyshev
` (17 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 07/24] q35: Fix migration of SMRAM state
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (5 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 06/24] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:35 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 08/24] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
` (16 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 07/24] q35: Fix migration of SMRAM state
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 07/24] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-13 15:35 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:35 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] q35: mch_update_smbase_smram() skips region disable on IN_RAM post_load
- [Low] q35: mch_update_smbase_smram() does not restore wmask for loaded 0x00 SMBASE
--
commit fa09770dda8724554b3e3a42f4619ca6364af58c
Author: Igor Mammedov <imammedo@redhat.com>
q35: Fix migration of SMRAM state
This change drops the check in mch_update_smbase_smram() that cleared the
SMBASE register while wmask was 0xff. It resyncs wmask from the register
value instead, so a locked SMRAM state is preserved across migration. It
also removes the test hunk that wrote junk to the register before
negotiation.
Link: https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- 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;
> }
[Severity: Medium]
Can this early return leave the SMBASE regions enabled? Before this patch,
a register value of 0x01 fell through to lck = false, which disabled both
smbase_blackhole and smbase_window. Now only wmask is updated, and
memory_region_set_enabled(false) is never called for 0x01.
Consider mch_post_load() -> mch_update() -> mch_update_smbase_smram() on a
live MCHPCIState that is currently locked (regions enabled) and has not been
through mch_reset(). If the incoming SMBASE value is 0x01, the regions stay
enabled while config[SMBASE] reads 0x01 (unlocked). The guest then sees a
black-holed SMBASE although the register says the range is unlocked. This
contradicts the comment "update wmask to be in sync with it regardless of
source".
The snapshot-load path calls qemu_system_reset() and a fresh migration
destination has the regions disabled, so neither is affected. The path that
looks reachable is COLO, where colo_process_incoming_thread() calls
qemu_loadvm_state_main() repeatedly on a running secondary VM without
qemu_system_reset(). If the primary locks SMRAM, reboots, and negotiates
0x01 within one checkpoint interval, the secondary loads 0x01 while its
regions are still enabled from the earlier lock. This needs COLO and a
machine with has_smram_at_smbase.
The final series state does not change hw/pci-host/q35.c, so this is not
addressed later in the series. Should the IN_RAM branch also disable
smbase_blackhole and smbase_window before returning?
> 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();
[Severity: Low]
Does this code restore wmask when a loaded SMBASE value is 0x00 or a junk
value without bit 1 set? wmask is resynced only for QUERY, IN_RAM and
values with the LCK bit. For other values wmask is left unchanged.
mch_reset() is the only place that sets wmask back to 0xff:
d->config[MCH_HOST_BRIDGE_F_SMBASE] = 0;
d->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0xff;
On the COLO secondary, colo_incoming_process_checkpoint() calls
qemu_load_device_state() -> qemu_loadvm_state_main() -> mch_post_load() ->
mch_update() -> mch_update_smbase_smram() without a device reset. If the
secondary's MCH is locked (wmask 0) and a later checkpoint carries
SMBASE=0x00, wmask stays 0. The register is then read-only and the guest can
no longer negotiate or lock SMRAM at SMBASE. The primary could carry 0x00
after a guest reset, before firmware negotiation.
This only shows up if the secondary takes over after such a checkpoint
without a guest reset, and the final series state does not change this file.
Is the comment "update wmask to be in sync with it regardless of source"
fully accurate here, and should wmask be set to 0xff for values that do not
match the QUERY, IN_RAM or LCK cases?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=7
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 08/24] migration: ensure APIC is loaded prior to VFIO PCI devices
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (6 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 07/24] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:33 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 09/24] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
` (15 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 09/24] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (7 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 08/24] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:36 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 10/24] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
` (14 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 09/24] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 09/24] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-13 15:36 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:36 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: vmstate save/load callers only report Error when ret < 0
--
Subject: Re: [PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()
commit 7359b43733a10dec8030b1d9e924f7eb62045b03
Author: Arun Menon <armenon@redhat.com>
migration: Fix regression of passing error_fatal into vmstate_load_state()
This patch replaces &error_fatal with a local Error object in the callers
of vmstate_save_state() and vmstate_load_state(). It reports the error with
error_report_err() when the call fails, and returns the result where the
caller can use it.
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);
> + }
[Severity: Low]
This is a speculative concern, but the same pattern appears in all the new
callers, so I am raising it once here.
vmstate_save_state_v() treats any nonzero return as a failure, for example
in the pre_save, post_save and put paths:
migration/vmstate.c:vmstate_save_state_v() {
...
if (ret) {
error_prepend(errp, "Save of field %s/%s failed: ", ...
...
return ret;
}
...
}
The new callers only report the Error when ret < 0. These callers are
pci_device_save(), virtio_save() (final subsection save), virtio_gpu_save(),
virtio_ccw_save_config(), vscsi_save_request(),
virtio_mmio_save_extra_state() and virtio_pci_save_extra_state().
If a callback ever returned a positive nonzero value, would local_err be
allocated but neither reported nor freed? The void wrappers would also
silently ignore the failure, and virtio_save() and virtio_gpu_save()
would return the positive value unchanged. Before this patch, error_fatal
would have exited on any nonzero return.
The load-side callers have the same exposure through pre_load() (returned
as-is) and post_load().
[ ... ]
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
[ ... ]
> 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;
> }
[Severity: Low]
Within virtio_save(), the vdc->vmsd save a few lines earlier checks
"if (ret)", while this final subsection save checks "if (ret < 0)". Should
the two checks be consistent?
With "ret < 0", a positive return from vmstate_save_state() leaves
local_err unreported and unfreed, and virtio_save() returns the positive
value.
The later series commit "migration: vmstate_save_state_v: fix double
error_setg" does not appear to address this. It sets an Error in the put
branch only when ret < 0, but the following "if (ret)" block still calls
error_prepend(errp, ...). Could a positive return from a put callback then
reach error_prepend() with *errp still NULL when a caller passes
&local_err, as these callers now do? The callers are unchanged at the end of
that series.
In practice callbacks conventionally return negative errnos, so this is
likely a cold path.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=9
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 10/24] migration: vmstate_save_state_v: fix double error_setg
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (8 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 09/24] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:38 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 11/24] migration: yank: Move register instance earlier Andrey Drobyshev
` (13 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 10/24] migration: vmstate_save_state_v: fix double error_setg
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 10/24] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
@ 2026-10-13 15:38 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:38 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() sets Error only for ret<0 but prepends for any ret
--
commit 8108694e711b23e4f49933352e133e7c3dec993c
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 when a nested
vmstate_save_state() or vmstate_save_state_v() call fails. It sets the Error
only for leaf put() failures, and it changes the field-level error_setg()
into error_prepend().
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> --- 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");
> + }
> }
[Severity: Medium]
Can a positive return from put() leave *errp unset here?
The new error_setg() runs only for ret < 0, but the failure check further
down in vmstate_save_state_v() is a plain "if (ret)", which also runs for
positive values.
In-tree, put_power() in target/arm/machine.c (vmstate_powered_off) returns 1
when cpu->power_state is neither PSCI_ON nor PSCI_OFF:
} else {
return 1;
}
With that return value, no Error is created at this point.
> @@ -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);
Is it safe to call error_prepend() on this path when *errp is NULL?
error_vprepend() in util/error.c checks only for a NULL errp, then reads the
message:
if (!errp) {
return;
}
...
g_string_append(newmsg, (*errp)->msg);
If a caller passed a non-NULL errp, such as a local Error pointer or
&error_fatal, and put_power() returned 1, would this dereference a NULL
*errp and crash QEMU during migration save?
Before this patch, error_setg() ran for every non-zero ret, so this case was
handled. Checking the series up to its final commit, I found no change to
migration/vmstate.c or migration/vmstate-types.c that addresses it.
Would it work to make the leaf-put condition "if (ret)" instead of
"if (ret < 0)", or otherwise ensure an Error is set before error_prepend()
runs?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=10
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 11/24] migration: yank: Move register instance earlier
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (9 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 10/24] migration: vmstate_save_state_v: fix double error_setg Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:41 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 12/24] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
` (12 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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.
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 | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2f8ef20c036..fb83e8bc695 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2235,11 +2235,12 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
return false;
}
+ yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);
+
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)
@@ -2264,7 +2265,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());
@@ -2276,7 +2277,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;
@@ -2329,8 +2329,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;
}
@@ -2354,27 +2353,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;
@@ -2401,7 +2399,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);
}
migration_connect_set_error(s, local_err);
--
2.47.1
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 11/24] migration: yank: Move register instance earlier
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 11/24] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-13 15:41 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:41 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: yank: migrate_prepare() aborts QEMU on duplicate yank instance
--
commit d6ce8a94f0289aba78580f9f74c0003c88eebd49
Author: Fabiano Rosas <farosas@suse.de>
migration: yank: Move register instance earlier
This patch moves yank_register_instance() from qmp_migrate_finish() into
migrate_prepare(), so that it pairs with the unregister in
migration_cleanup(). The resume case is now detected from the migration
state instead of a resume_requested argument.
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
> diff --git a/migration/migration.c b/migration/migration.c
> index 2f8ef20c0369..fb83e8bc695d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2235,11 +2235,12 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
> return false;
> }
>
> + yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);
> +
> return true;
> }
[Severity: Medium]
Can this abort QEMU when a yank instance is already registered?
Before this patch, a duplicate registration was reported through errp
from qmp_migrate_finish() as "duplicate yank instance". Now
migrate_prepare() passes &error_abort, so the duplicate check in
yank_register_instance() ends in abort():
util/yank.c:yank_register_instance() {
if (yank_find_entry(instance)) {
error_setg(errp, "duplicate yank instance");
return false;
}
}
The incoming side registers the same global MIGRATION_YANK_INSTANCE in
qmp_migrate_incoming(), and it keeps it until
migration_incoming_state_destroy():
migration/migration.c:migration_incoming_state_destroy() {
...
cpr_set_incoming_mode(MIG_MODE_NONE);
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
For postcopy, that destroy happens at the end of postcopy from the
listen thread. If the incoming side is in POSTCOPY_PAUSED, it can wait
indefinitely. By then the guest is running, so the runstate is no
longer RUN_STATE_INMIGRATE.
The guards earlier in migrate_prepare() are migration_is_running(), which
only looks at the outgoing current_migration, RUN_STATE_INMIGRATE, and
RUN_STATE_POSTMIGRATE. None of them looks at incoming state.
A QMP migrate issued in that window would pass all of them, run
migrate_init(), and then hit yank_register_instance() with &error_abort.
This needs a nested or chained migration setup and a privileged QMP
command, so it should be rare. The upstream commit 7e714392437c has the
same &error_abort. I did not find a guard against the overlap, and
nothing later in this series adds one. The final series state still has
&error_abort in migrate_prepare(), while qmp_migrate_incoming() and
load_snapshot() still pass errp.
Could the registration here report the failure through errp instead, with
migrate_prepare() returning false? Alternatively, could the incoming
state be checked before registering?
The commit message also does not mention that a duplicate registration
changes from a recoverable error into an abort.
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=11
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 12/24] migration: Free cpr-transfer MigrationAddress along with gsource
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (10 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 11/24] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:39 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 13/24] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
` (11 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 fb83e8bc695..61d458b2562 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2245,9 +2245,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)
@@ -2269,7 +2271,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;
}
@@ -2350,7 +2351,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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 12/24] migration: Free cpr-transfer MigrationAddress along with gsource
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 12/24] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
@ 2026-10-13 15:39 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:39 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +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>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=12
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 13/24] migration: Move CPR HUP watch to cpr-transfer.c
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (11 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 12/24] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:40 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early Andrey Drobyshev
` (10 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 61d458b2562..14ff16fa6a4 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)
{
@@ -1500,7 +1499,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);
@@ -1661,7 +1660,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);
}
}
@@ -2242,25 +2241,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)
@@ -2270,7 +2250,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;
}
@@ -2350,8 +2330,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] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (12 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 13/24] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:45 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 15/24] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
` (9 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 14ff16fa6a4..2e4be2db65d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1653,14 +1653,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));
}
}
@@ -2246,11 +2251,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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-13 15:45 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:45 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] migration: cancel in CPR HUP wait fires FAILED without SETUP, tripping notify assert
- [Medium] migration: migration_cancel() asserts on live migrate_mode() while hup_source is set
--
commit e9dea0ff5e4d16a3a4c776bef4c26e24060a56d5
Author: Peter Xu <peterx@redhat.com>
migration: Fix crash on second migration when cancel early
This patch changes migration_cancel() so that, when a cpr-transfer
migration is waiting for the HUP event on the CPR channel, it calls
migration_cleanup() instead of directly setting the CANCELLED state. It
also detaches hup_source at the start of qmp_migrate_finish_cb() so the
wait window can be detected, and 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
> index 14ff16fa6a4e7..2e4be2db65ddd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -1653,14 +1653,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? migrate_mode() returns the live value of
migrate_get_current()->parameters.mode, since cpr_get_incoming_mode() is
NONE on the source.
Consider this sequence:
qmp_migrate() with mode=cpr-transfer
cpr_transfer_add_hup_watch() /* sets s->hup_source, state is SETUP */
migrate-set-parameters mode=normal
qmp_migrate_set_parameters()->migrate_params_apply()
s->parameters.mode = params->mode;
migrate-cancel
migration_cancel()
cpr_transfer_source_active(s) is true
assert(migrate_mode() == MIG_MODE_CPR_TRANSFER) fails
As far as I can tell, none of migrate_params_test_apply(),
migrate_params_check() or migrate_params_apply() reject a mode change
while a migration is running.
The same assert can be reached at exit, because migration_shutdown()
calls migration_cancel().
Since hup_source being set already implies that cpr-transfer was chosen
when the migration started, is this assertion on the mutable global mode
needed? Before this patch, migration_cancel() had no such assertion.
The second assertion, assert(setup && !s->to_dst_file), did not appear
to fire in the cases I checked.
> + assert(setup && !s->to_dst_file);
> + migration_cleanup(s);
[Severity: High]
Can this path abort QEMU when a VFIO PCI device is present?
migration_cleanup() ends with:
migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
but in the HUP-wait window migration_connect() has not run yet, so
MIG_EVENT_PRECOPY_SETUP was never delivered:
qmp_migrate()
cpr_transfer_add_hup_watch() /* no SETUP notification yet */
migrate_cancel
migration_cancel()
migration_cleanup()
migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL)
vfio_cpr_pci_notifier()
vfio_cpr_set_msi_virq(vdev, errp, true)
The FAILED handler in hw/vfio/cpr.c re-adds irqfds that only the SETUP
handler removes:
if (e->type == MIG_EVENT_PRECOPY_SETUP) {
return vfio_cpr_set_msi_virq(vdev, errp, false);
} else if (e->type == MIG_EVENT_PRECOPY_FAILED) {
return vfio_cpr_set_msi_virq(vdev, errp, true);
}
With SETUP skipped, the irqfds are still assigned, so a second
KVM_IRQFD assign may fail. I did not find this in the repository. It
is from my recollection that KVM rejects a duplicate eventfd with
-EBUSY.
If so, the notifier returns nonzero and migration_call_notifiers() hits:
ret = notifier->notify(notifier, &e, errp);
if (ret) {
assert(type == MIG_EVENT_PRECOPY_SETUP);
return ret;
}
The type is FAILED, so the assert fails.
Can the cancel path avoid sending FAILED to notifiers when SETUP was
never sent? Other notifiers (virtio-net failover, scsi-block,
vfio-migration) seem to tolerate an unpaired FAILED and do not return
errors.
The same unpaired FAILED already exists in the migration_connect()
error_in path. This patch adds a new route to it that a user can
trigger easily.
The later patches in the series do not appear to address this. The
series later removes the VFIO CPR block in migrate_cpr_is_blocked(),
which makes the VFIO plus cpr-transfer configuration reachable.
> + /* Now all things should have been released */
> + assert(!cpr_transfer_source_active(s));
> }
> }
[ ... ]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=14
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 15/24] kvm: tolerate ioctls after kvm_close()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (13 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:44 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 16/24] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
` (8 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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: kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return
-ENODEV when kvm_close() has already been called, so any late interrupt
or query on the retired source fails gracefully. Teach kvm_send_msi()
that -ENODEV in this situation is expected and not worth the "MSI lost"
warning on every such interrupt.
Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
accel/kvm/kvm-all.c | 15 +++++++++++++++
hw/i386/kvm/apic.c | 6 +++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a39c1ba260a..ea027c37829 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3371,6 +3371,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 || s->fd == -1) {
+ return -ENODEV;
+ }
+
trace_kvm_ioctl(type, arg);
ret = ioctl(s->fd, type, arg);
if (ret == -1) {
@@ -3389,6 +3394,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 || s->vmfd == -1) {
+ return -ENODEV;
+ }
+
trace_kvm_vm_ioctl(type, arg);
accel_ioctl_begin();
ret = ioctl(s->vmfd, type, arg);
@@ -3409,6 +3419,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 -ENODEV;
+ }
+
trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
accel_cpu_ioctl_begin(cpu);
ret = ioctl(cpu->kvm_fd, type, arg);
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 1be9bfe36e9..0c9f7759b6e 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -196,7 +196,11 @@ static void kvm_send_msi(MSIMessage *msg)
msg->address = kvm_swizzle_msi_ext_dest_id(msg->address);
ret = kvm_irqchip_send_msi(kvm_state, *msg);
- if (ret < 0) {
+ /*
+ * -ENODEV means kvm_close() has already run (the VM was handed over
+ * via CPR); the interrupt is meaningless here and not worth a warning.
+ */
+ if (ret < 0 && ret != -ENODEV) {
fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
strerror(-ret));
}
--
2.47.1
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 15/24] kvm: tolerate ioctls after kvm_close()
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 15/24] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-13 15:44 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:44 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: kvm_set_irq() and listeners still deref NULL kvm_state after kvm_close()
- [Medium] kvm: -ENODEV from closed-state guards still aborts in fail-fast callers; stale errno in ioeventfd
- [Low] kvm: closed-state sentinel reuses -ENODEV, which kvm_send_msi() cannot tell from a kernel -ENODEV
--
commit 32c3566efef848fa0bd62b2fabb9b95d82114443
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
kvm: tolerate ioctls after kvm_close()
This patch makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return
-ENODEV once kvm_close() has run. kvm_send_msi() no longer warns on
-ENODEV. The aim is to stop a retired cpr-transfer source from crashing
on late interrupts.
> 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 +3394,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 || s->vmfd == -1) {
> + return -ENODEV;
> + }
> +
[Severity: Medium]
Can callers that treat any negative return as fatal still bring down the
retired source? After kvm_close() the memory listeners are still
registered and kvm_enabled() is still true, so these callers now receive
-ENODEV:
kvm_mem_ioeventfd_del()
if (r < 0) {
fprintf(stderr, "%s: error deleting ioeventfd: %s (%d)\n",
__func__, strerror(-r), -r);
abort();
}
kvm_set_irq()
ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
if (ret < 0) {
perror("kvm_set_irq");
abort();
}
kvm_irqchip_commit_routes()
ret = kvm_vm_ioctl(s, KVM_SET_GSI_ROUTING, s->irq_routes);
assert(ret == 0);
kvm_update_clock() in hw/i386/kvm/clock.c
ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
if (ret < 0) {
fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(-ret));
abort();
The same applies to kvm_mem_ioeventfd_add(), the PIO ioeventfd variants and
kvm_log_clear().
Would a late legacy IRQ, a memory region update, a routing commit or a
clock query on the retired source still end in abort() or assert()? The
commit message says these cases now "fail gracefully".
Also, the new early return skips ioctl(), so errno is not updated. In
kvm_set_ioeventfd_mmio():
ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &iofd);
if (ret < 0) {
return -errno;
}
Doesn't this return a stale errno, which may be 0 or an unrelated value,
when kvm_vm_ioctl() returns -ENODEV from the new guard?
[Severity: Medium]
This isn't fully fixed for legacy interrupt callers either. The guard
helps only if the call reaches kvm_vm_ioctl(), but kvm_set_irq()
evaluates s->irq_set_ioctl as an argument before the call:
kvm_set_irq(s, irq, level)
ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
With s == NULL this is a NULL dereference before the !s check can run. It
is reached with kvm_state == NULL from:
hw/i386/kvm/ioapic.c:kvm_ioapic_set_irq()
delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
hw/i386/kvm/i8259.c:kvm_pic_set_irq()
delivered = kvm_set_irq(kvm_state, irq, level);
The final return in kvm_set_irq() also reads s->irq_set_ioctl. Memory
listener callbacks such as kvm_coalesce_pio_del() also use kvm_state
directly:
KVMState *s = kvm_state;
if (s->coalesced_pio) {
kvm_close() sets kvm_state = NULL but does not unregister the memory
listeners or the irqchip users.
Can an emulated INTx device raising a legacy interrupt, or a memory
region removed during qemu_cleanup(), still hit the SIGSEGV this patch
is meant to remove?
The later patches in the series change only coalesced-MMIO ring handling
in kvm_close() and the CPR-exec and VFIO code. They do not address these
callers.
[Severity: Low]
Is -ENODEV distinct enough to mean "closed"? The check in kvm_vm_ioctl()
tests s->vmfd == -1, and kvm_accel_instance_init() also sets:
s->fd = -1;
s->vmfd = -1;
so callers that run before the VM is opened now get -ENODEV instead of
-EBADF. I did not find any caller in accel/kvm, target/i386 or hw/ that
depends on -EBADF.
[ ... ]
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -196,7 +196,11 @@ static void kvm_send_msi(MSIMessage *msg)
> msg->address = kvm_swizzle_msi_ext_dest_id(msg->address);
>
> ret = kvm_irqchip_send_msi(kvm_state, *msg);
> - if (ret < 0) {
> + /*
> + * -ENODEV means kvm_close() has already run (the VM was handed over
> + * via CPR); the interrupt is meaningless here and not worth a warning.
> + */
> + if (ret < 0 && ret != -ENODEV) {
[Severity: Low]
This isn't a bug I could confirm, but the check suppresses the "MSI lost"
warning for any -ENODEV, not only the post-kvm_close() case. Since the
same value is also an errno the kernel could return from KVM_SIGNAL_MSI,
the two cases cannot be told apart here. I could not confirm that the
kernel returns -ENODEV for that ioctl.
Would testing kvm_state == NULL in kvm_send_msi() before the call be a
more precise way to detect the closed state?
> fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
> strerror(-ret));
> }
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=15
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 16/24] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (14 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 15/24] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:47 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 17/24] migration/cpr: add cpr_is_in_progress() Andrey Drobyshev
` (7 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 ea027c37829..c29d467f9f5 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 16/24] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 16/24] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-13 15:47 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:47 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +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>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=16
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 17/24] migration/cpr: add cpr_is_in_progress()
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (15 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 16/24] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:47 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
` (6 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 | 21 +++++++++++++++++++++
2 files changed, 22 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..bbfc362de2c 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -549,6 +549,27 @@ 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.
+ * But a running outgoing migration uses it.
+ */
+ return migration_is_running();
+}
+
bool cpr_state_save(MigrationChannel *channel, Error **errp)
{
int ret;
--
2.47.1
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (16 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 17/24] migration/cpr: add cpr_is_in_progress() Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:53 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 19/24] vfio/listener: skip dirty page tracking " Andrey Drobyshev
` (5 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 in CPR modes: don't save or load the device state,
and don't change the device state on VM stop/start. The former is gated
on cpr_incoming_needed(), as it only runs during a save or load. The
latter runs on any stop/cont, so it is gated on cpr_is_in_progress()
instead: the mode alone would also skip a plain stop/cont of a VM which
merely has it set.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/migration.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4c06e3db936..23e083853da 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"
@@ -674,6 +675,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_incoming_needed(NULL)) {
+ 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);
@@ -824,7 +835,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_incoming_needed(NULL);
+}
+
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 +880,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 +910,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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-13 15:53 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:53 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] vfio: migration save/load gated on mode parameter, not CPR in progress
- [Medium] vfio/migration: load_state not gated for CPR, unpatched source still hits live device
- [Medium] vfio/migration: switchover_ack_needed not gated for CPR, hangs migration
- [Medium] vfio/migration: stale multifd_transfer lets save thread stop device during CPR
--
commit 27c407560d6e12fc61d8fd9e47b3508c1eaa3fcb
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/migration: don't migrate device state during CPR
This patch skips VFIO device state migration in the cpr-transfer and
cpr-exec modes. It adds a vfio_is_active() callback and an early return in
vfio_save_state(), both gated on cpr_incoming_needed(). It also makes the
VM state change handlers return early when cpr_is_in_progress().
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
[ ... ]
> @@ -674,6 +675,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_incoming_needed(NULL)) {
> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> + return;
> + }
> +
[Severity: Medium]
cpr_incoming_needed() only tests migrate_mode(), that is, the configured
mode parameter, and not whether a CPR operation is running.
If the mode parameter is left at cpr-transfer or cpr-exec (for example
after an earlier or failed CPR attempt) and a non-CPR save such as
snapshot-save runs, what happens?
save_snapshot()
vm_stop(RUN_STATE_SAVE_VM)
qemu_savevm_state()
vfio_save_state()
cpr_incoming_needed(NULL) is true -> only END_OF_STATE is written
In that case vfio_vmstate_change() is gated on cpr_is_in_progress(),
which requires migration_is_running() on the source, and that is false
for savevm. The device is therefore still moved to STOP, but no state is
written, so a later load restores nothing.
The commit message says the stop/cont hook uses cpr_is_in_progress()
because "the mode alone would also skip a plain stop/cont of a VM which
merely has it set". Does the same reasoning apply to vfio_save_state()
and vfio_is_active()? Should they use the same predicate as the
vmstate handlers?
[ ... ]
> @@ -824,7 +835,19 @@ static int vfio_switchover_start(void *opaque)
> return 0;
> }
>
> +static bool vfio_is_active(void *opaque)
> +{
> + return !cpr_incoming_needed(NULL);
> +}
> +
> 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,
[Severity: Medium]
This isn't limited to the case above: vfio_is_active() also returns false
on the mode parameter alone, so all VFIO save handlers are disabled
whenever the mode is cpr-transfer or cpr-exec, even with no CPR handover
running.
Separately, is the load side covered? .is_active only suppresses setup,
save and pending-estimate handling, while .load_state is still called on
the destination for any section the source sends.
If the source is a QEMU without this patch, vfio_load_state() would still
see these sections:
vfio_load_state()
case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
return vfio_load_device_config_state(f, opaque);
That would apply the config to the preserved device and re-program MSI-X,
which is the failure the commit message describes. A DATA_STATE section
would reach vfio_load_buffer()->qemu_file_get_to_fd() with data_fd still
-1, because vfio_load_setup() is skipped, and the incoming migration
would fail.
Is a version requirement on the source intended here, and should
vfio_load_state() have its own CPR check?
[Severity: Medium]
Can this hang a CPR migration when switchover-ack is enabled?
vfio_switchover_ack_needed() still returns vfio_precopy_supported() and
is not gated by is_active. On the destination,
qemu_loadvm_state_switchover_ack_needed() does:
if (se->ops->switchover_ack_needed(se->opaque)) {
mis->switchover_ack_pending_num++;
}
without consulting is_active, so the VFIO device is counted as awaiting
an ACK. The fallback ACK on MIG_CMD_OPEN_RETURN_PATH then does not fire.
On the source, vfio_is_active() now returns false, so vfio_save_iterate()
is never called. That is the only place this is emitted:
qemu_put_be64(f, VFIO_MIG_FLAG_DEV_INIT_DATA_SENT);
The destination therefore never calls qemu_loadvm_approve_switchover().
migration_can_switchover() then keeps returning s->switchover_acked
(false) while the VM is running:
if (!runstate_is_running()) {
return true;
}
return s->switchover_acked;
migrate_caps_check() only requires return-path for switchover-ack and
does not reject it in CPR modes. Should vfio_switchover_ack_needed() also
return false in CPR mode, or should the capability be rejected there?
[ ... ]
> @@ -857,6 +880,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 +910,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> Error *local_err = NULL;
> int ret;
>
> + if (cpr_is_in_progress()) {
> + return;
> + }
> +
[Severity: Medium]
Can the multifd save thread still move the preserved device to STOP_COPY
during CPR?
qemu_savevm_state_complete_precopy_iterable() spawns
save_complete_precopy_thread for every handler that has one, without
checking .is_active. The thread's only guard is
vfio_multifd_transfer_enabled():
hw/vfio/migration-multifd.c:vfio_multifd_save_complete_precopy_thread() {
if (!vfio_multifd_transfer_enabled(vbasedev)) {
/* Nothing to do, vfio_save_complete_precopy() does the transfer. */
return true;
}
...
if (vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
VFIO_DEVICE_STATE_STOP, errp)) {
migration->multifd_transfer is written only in vfio_multifd_setup(), and
vfio_multifd_cleanup() does not reset it. With this patch
vfio_save_setup() is skipped in CPR modes, so the flag is no longer
refreshed.
If an earlier non-CPR migration attempt ran vfio_save_setup() with
multifd transfer enabled and then failed or was cancelled, and a later
cpr-transfer or cpr-exec is run in the same process, would the flag still
be true and the thread stop the preserved device and stream its state?
The same flag also gates vfio_switchover_start() without an is_active
check, though that is a destination-side hook in a fresh process and is
less exposed.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=18
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 19/24] vfio/listener: skip dirty page tracking during CPR
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (17 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 18/24] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:50 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 20/24] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
` (4 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 in CPR modes. Stopping it stays unconditional:
it's a no-op if tracking wasn't started, and the mode may have changed
in between.
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..a58a3778489 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_incoming_needed(NULL)) {
+ 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 19/24] vfio/listener: skip dirty page tracking during CPR
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 19/24] vfio/listener: skip dirty page tracking " Andrey Drobyshev
@ 2026-10-13 15:50 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:50 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio: log_global_start skipped in CPR mode is never retried when mode changes
--
commit 29c28bad6094fa55255321af5424128d0b541ec1
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/listener: skip dirty page tracking during CPR
This commit makes vfio_listener_log_global_start() return early without
starting VFIO dirty page tracking when the migration mode is cpr-transfer
or cpr-exec. Stopping dirty tracking is left unconditional.
> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> --- 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_incoming_needed(NULL)) {
> + return true;
> + }
> +
[Severity: Medium]
Can this early return leave VFIO dirty tracking permanently off for a later
live migration?
The skip is decided from the current migrate_mode(), which is mutable
state, rather than from the reason global dirty logging was requested.
The listener callback runs only on the first transition of the global
dirty flags:
system/memory.c:memory_global_dirty_log_start() {
...
flags &= ~global_dirty_tracking;
if (!flags) {
return true;
}
old_flags = global_dirty_tracking;
global_dirty_tracking |= flags;
...
if (!old_flags) {
if (!memory_global_dirty_log_do_start(errp)) {
...
}
Consider this sequence:
1. While migrate_mode() is cpr-transfer or cpr-exec, some user takes a
global dirty flag. calc-dirty-rate in dirty-bitmap mode does this via
global_dirty_log_change() calling memory_global_dirty_log_start(). A
CPR migration that is still running, or whose stop is postponed, also
qualifies. vfio_listener_log_global_start() returns true and starts
nothing.
2. The mode parameter is changed through migrate-set-parameters, or
incoming_mode is reset by cpr_set_incoming_mode(MIG_MODE_NONE) in
migration cleanup.
3. A normal migration calls
memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION). Since the other
flag is still held, old_flags is non-zero, so
memory_global_dirty_log_do_start() is not run again and
vfio_listener_log_global_start() is never called for this migration.
Would VFIO device DMA writes then go unsynced during that live migration,
which could corrupt guest memory on the destination?
The path needs an unusual mode switch while a global dirty flag is held,
but it is reachable through QMP. The series-final state does not change
hw/vfio/listener.c or system/memory.c, so this is not addressed by later
patches.
As a secondary effect, calc-dirty-rate while in a CPR mode silently
excludes VFIO device writes. Would it be better to base the decision on
why logging was requested, or to make the start retriable when the mode
changes?
> if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
> ret = vfio_devices_dma_logging_start(bcontainer, errp);
> } else {
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=19
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 20/24] VZ: vfio/cpr: don't close KVM on cpr-exec
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (18 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 19/24] vfio/listener: skip dirty page tracking " Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:53 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 21/24] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
` (3 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 20/24] VZ: vfio/cpr: don't close KVM on cpr-exec
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 20/24] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
@ 2026-10-13 15:53 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:53 UTC (permalink / raw)
To: Andrey Drobyshev, 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>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=20
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 21/24] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (19 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 20/24] VZ: vfio/cpr: don't close KVM on cpr-exec Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:55 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 22/24] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
` (2 subsequent siblings)
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 7b7609e19cd..cb962ce7252 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -154,6 +154,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)
{
/*
@@ -202,7 +215,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);
}
@@ -221,15 +237,17 @@ 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();
-
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();
+
/* 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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 21/24] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 21/24] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
@ 2026-10-13 15:55 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:55 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] migration: preserve_ram() leaks original Error after migrate_set_error()
- [Low] migration: cpr-exec fail path leaves RAM read-only after execvp() failure
--
commit 02a023bc5ba37bcad09a7328d397c6b940edac14
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
This patch makes cpr_exec_cb() treat a failed MADV_DOEXEC on any saved
ramblock as a failed cpr-exec. It jumps to a shared fail: label, which is
also reached when execvp() fails. That label restores close-on-exec on the
fds and clears MADV_DOEXEC on the ramblocks, so the VM keeps running in the
old QEMU.
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index 7b7609e19cd9..cb962ce72529 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
[ ... ]
> @@ -202,7 +215,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;
> + }
[Severity: Low]
Does this new goto fail path leak the original Error object?
preserve_ram() builds the error like this:
error_setg_errno(&err, errno, "MADV_DOEXEC failed on memory region %s:"
" addr=%p length=%zu", name, (void *) ram.addr,
ram.length);
error_report_err(error_copy(err));
migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
migrate_set_error(s, err);
return false;
error_report_err(error_copy(err)) frees only the copy, and
migrate_set_error() stores its own error_copy() without taking ownership.
No error_free(err) is reached before return false, so the original Error
(the struct and its msg string) is never freed.
The execvp() failure path in cpr_exec_cb() does call error_free(err) after
migrate_set_error(). The leak in preserve_ram() predates this patch, but
this patch now relies on that path to handle the failure. Would an
error_free(err) before return false in preserve_ram() be appropriate?
> if (cpr_ram_rdonly) {
> cpr_walk_ramblock(mark_ram_rdonly);
> }
[ ... ]
> @@ -221,15 +237,17 @@ 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();
> -
> 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();
> +
[Severity: Low]
Can the guest be resumed on read-only RAM when execvp() fails?
If cpr_ram_rdonly is set, cpr_exec_cb() calls:
if (cpr_ram_rdonly) {
cpr_walk_ramblock(mark_ram_rdonly);
}
before execvp(). If execvp() then fails, the code reaches fail:, which
clears MADV_DOEXEC and restores close-on-exec, but nothing restores the
RAM protection. cpr_ram_restore_rw() exists:
void cpr_ram_restore_rw(void)
{
cpr_walk_ramblock(mark_ram_rw);
}
but its only caller is system/vl.c, which runs in the new QEMU after a
successful exec.
The code after the label then runs:
if (runstate_is_live(s->vm_old_state)) {
vm_start();
}
With RAM still PROT_READ, can the first guest write, or a device emulation
write into guest memory, raise SIGSEGV and kill the VM? That seems contrary
to the commit message's statement that "the VM keeps running in the old
QEMU".
The preserve_ram() failure path jumps to fail: before mark_ram_rdonly() and
vhost_net_cpr_reset_owners() run, so it is not affected. The execvp()
failure path is affected. cpr_ram_rdonly is documented as a debug-only
option, which limits the exposure. Should the fail: path call
cpr_ram_restore_rw() when cpr_ram_rdonly is set?
Later patches in the series (the one that takes the vhost-net devices back
if exec fails) add a vhost_net_cpr_set_owners() call before the fail:
label. None of the later patches in the series restore RAM to read-write
on this path.
> /* Note, we can go from state COMPLETED to FAILED */
> migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
>
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=21
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 22/24] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (20 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 21/24] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:55 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 24/24] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 right after the exec failure. The
devices are stopped at that point, and vhost_net_start() sets up the
rest again when the VM resumes. vhost-vsock does the same in its CPR
notifier.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/net/vhost_net.c | 25 +++++++++++++++++++++++++
include/net/vhost_net.h | 1 +
migration/cpr-exec.c | 3 +++
3 files changed, 29 insertions(+)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 56c697f4bcd..58233788ebb 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -599,6 +599,31 @@ void vhost_net_cpr_reset_owners(void)
}
}
+/*
+ * 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->dev.vhost_ops &&
+ net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
+ int r = vhost_dev_set_owner(&net->dev);
+
+ if (r < 0 && r != -ENOSYS) {
+ warn_report("cpr: failed to set vhost-net owner for %s: %s",
+ nc->name, strerror(-r));
+ }
+ }
+ }
+}
+
VHostNetState *get_vhost_net(NetClientState *nc)
{
if (!nc) {
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 cb962ce7252..9a428b43093 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -243,6 +243,9 @@ static void cpr_exec_cb(void *opaque)
error_free(err);
err = NULL;
+ /* Still the same mm, take the vhost-net devices back */
+ vhost_net_cpr_set_owners();
+
fail:
g_clear_pointer(&argv, g_strfreev);
cpr_exec_unpreserve_fds();
--
2.47.1
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 22/24] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 22/24] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
@ 2026-10-13 15:55 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 15:55 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-net: cpr_set_owners() retries SET_OWNER on devices whose reset failed
--
commit 5dfb07a07fff9bf62ccc97d74b7cefc2e5892991
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 issues VHOST_SET_OWNER on
every kernel-backend vhost-net device. cpr_exec_cb() calls it after a failed
execvp(), so the old QEMU takes the devices back that it released with
VHOST_RESET_OWNER before the exec.
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
[ ... ]
> +void vhost_net_cpr_set_owners(void)
> +{
> + NetClientState *nc;
> +
> + QTAILQ_FOREACH(nc, &net_clients, next) {
> + VHostNetState *net = get_vhost_net(nc);
> +
> + if (net && net->dev.vhost_ops &&
> + net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
> + int r = vhost_dev_set_owner(&net->dev);
> +
> + if (r < 0 && r != -ENOSYS) {
> + warn_report("cpr: failed to set vhost-net owner for %s: %s",
> + nc->name, strerror(-r));
> + }
> + }
> + }
> +}
[Severity: Low]
Can this issue VHOST_SET_OWNER on a device that was never released?
vhost_net_cpr_reset_owners() only warns when vhost_dev_reset_owner() fails
and does not record which devices were actually reset:
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));
}
A device whose reset failed is still owned by this mm. After a failed
execvp(), vhost_net_cpr_set_owners() then calls vhost_dev_set_owner() on it
as well. From my recollection of the kernel code, which I have not
verified, VHOST_SET_OWNER returns -EBUSY for a device that already has an
owner. If so, the rollback would log a second, misleading "cpr: failed to
set vhost-net owner" warning for that device. The device would still be
owned by the old QEMU, so I don't expect functional loss.
The vhost-vsock sibling avoids this. In vhost_vsock_pre_save(), owner_reset
is set only after a successful vhost_dev_reset_owner(), and
vhost_vsock_cpr_notifier() restores ownership only when it is set:
if (e->type == MIG_EVENT_PRECOPY_FAILED && vsock->owner_reset) {
ret = vhost_dev_set_owner(&vvc->vhost_dev);
Would it make sense to track per device whether the reset succeeded in
vhost_net_cpr_reset_owners(), and have vhost_net_cpr_set_owners() restore
only those devices?
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
[ ... ]
> @@ -243,6 +243,9 @@ static void cpr_exec_cb(void *opaque)
> error_free(err);
> err = NULL;
>
> + /* Still the same mm, take the vhost-net devices back */
> + vhost_net_cpr_set_owners();
> +
This is the only rollback call site. It runs after a failed execvp() and
has no per-device information about which devices were released.
> fail:
> g_clear_pointer(&argv, g_strfreev);
> cpr_exec_unpreserve_fds();
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=22
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (21 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 22/24] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 16:00 ` no-reply
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 24/24] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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().
The ID is what cpr_name() would give to the mmap subregion:
"<device path>/<region name> mmaps[<index>]".
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 devices with mmapped regions.
New QEMU checks the saved mappings like ram_block_add() checks RAM
blocks: on cpr-exec an entry must exist, otherwise an entry is a bug.
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 | 1 +
hw/vfio/region.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
include/hw/vfio/vfio-cpr.h | 1 +
3 files changed, 74 insertions(+)
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 5a14a7d5c0a..88b9e8752cd 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -514,6 +514,7 @@ void vfio_device_unprepare(VFIODevice *vbasedev)
g_clear_pointer(&vbasedev->reginfo, g_free);
g_clear_pointer(&vbasedev->region_fds, g_free);
+ migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
QLIST_REMOVE(vbasedev, container_next);
QLIST_REMOVE(vbasedev, global_next);
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 1deaef12fd0..fce09a0d17b 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -30,6 +30,8 @@
#include "qemu/units.h"
#include "monitor/monitor.h"
#include "system/ramblock.h"
+#include "migration/blocker.h"
+#include "migration/cpr.h"
#include "vfio-helpers.h"
/*
@@ -227,8 +229,51 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
return 0;
}
+static char *vfio_region_cpr_name(VFIORegion *region, int index)
+{
+ g_autofree char *path = qdev_get_dev_path(region->vbasedev->dev);
+ return g_strdup_printf("%s%s%s mmaps[%d]", path ?: "", path ? "/" : "",
+ memory_region_name(region->mem), index);
+}
+
+/* Same checks as for RAM blocks in ram_block_add() */
+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 (cpr_exec_is_incoming()) {
+ if (!ram.addr || !ram.length) {
+ error_report("CPR mode=exec is requested, but mapping %s can't "
+ "be reused: addr=%p length=%zu. Aborting",
+ name, (void *)ram.addr, ram.length);
+ exit(1);
+ }
+ assert(ram.length == region->mmaps[index].size);
+ } else if (ram.addr || ram.length) {
+ error_report("CPR mode=exec wasn't requested, but mapping %s was "
+ "saved: addr=%p length=%zu. Aborting",
+ name, (void *)ram.addr, ram.length);
+ 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 +
@@ -307,6 +352,19 @@ int vfio_region_mmap(VFIORegion *region)
return 0;
}
+ /*
+ * Only cpr-exec keeps the address space, a cpr-transfer target would
+ * have to map the regions anew, which some drivers (NVIDIA vGPU) refuse
+ * while the old mappings exist. Not supported.
+ */
+ if (!region->vbasedev->cpr.mmap_blocker) {
+ error_setg(®ion->vbasedev->cpr.mmap_blocker,
+ "cpr-transfer is not supported with mmapped VFIO regions "
+ "(%s); use the cpr-exec mode", region->vbasedev->name);
+ migrate_add_blocker_modes(®ion->vbasedev->cpr.mmap_blocker,
+ &error_abort, MIG_MODE_CPR_TRANSFER, -1);
+ }
+
prot |= region->flags & VFIO_REGION_INFO_FLAG_READ ? PROT_READ : 0;
prot |= region->flags & VFIO_REGION_INFO_FLAG_WRITE ? PROT_WRITE : 0;
@@ -314,6 +372,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 +410,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,
@@ -410,6 +475,13 @@ void vfio_region_exit(VFIORegion *region)
for (i = 0; i < region->nr_mmaps; i++) {
if (region->mmaps[i].mmap) {
+ /*
+ * The mapping is unmapped by vfio_region_finalize(), but by
+ * then the device is unparented and its path, part of the CPR
+ * name, is gone. So drop the CPR entry here.
+ */
+ g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
+ cpr_delete_ramblock(cpr_name);
memory_region_del_subregion(region->mem, ®ion->mmaps[i].mem);
}
}
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] 49+ messages in thread* Re: [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-13 16:00 ` no-reply
0 siblings, 0 replies; 49+ messages in thread
From: no-reply @ 2026-10-13 16:00 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core; +Cc: andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] vfio: vfio_region_cpr_find() exits QEMU for lazily mmapped display region after incoming mode reset
- [Medium] vfio: vfio_region_mmap() adds cpr-transfer blocker with error_abort
- [Medium] vfio: cpr-exec new QEMU exit(1)s when a BAR was not mmapped by old QEMU
- [Medium] vfio: display region CPR entry leaked, vfio_region_exit() runs after unparent
- [Low] vfio: cpr-transfer blocker added before mmap and kept after failure
- [Low] vfio: cpr-transfer is blocked for every VFIO device with mmapped regions
--
commit 715bce8fddbc1bc88d530d34be1f40b7da065453
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 the
new QEMU can reuse the mapping after cpr-exec instead of calling mmap()
again. It also adds a cpr-transfer blocker for devices with mmapped
regions.
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 5a14a7d5c0a2e..88b9e8752cd93 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -514,6 +514,7 @@ void vfio_device_unprepare(VFIODevice *vbasedev)
>
> g_clear_pointer(&vbasedev->reginfo, g_free);
> g_clear_pointer(&vbasedev->region_fds, g_free);
> + migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
>
> QLIST_REMOVE(vbasedev, container_next);
> QLIST_REMOVE(vbasedev, global_next);
[ ... ]
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b8..fce09a0d17b1a 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
[ ... ]
> +static char *vfio_region_cpr_name(VFIORegion *region, int index)
> +{
> + g_autofree char *path = qdev_get_dev_path(region->vbasedev->dev);
> + return g_strdup_printf("%s%s%s mmaps[%d]", path ?: "", path ? "/" : "",
> + memory_region_name(region->mem), index);
> +}
[Severity: Medium]
The name built here depends on qdev_get_dev_path(), which returns NULL once
device_unparent() has cleared dev->parent_bus. Can the save-time and
delete-time names diverge for the display region?
The display region is mmapped lazily:
vfio_display_region_update()
vfio_region_setup()
vfio_region_mmap()
vfio_region_cpr_save() /* saved as "<path>/display mmaps[0]" */
On hot-unplug, vfio_exitfn() runs vfio_pci_bars_exit(), which covers only
the BAR regions. The display region is torn down later, from
vfio_pci_finalize():
vfio_pci_finalize()
vfio_pci_put_device()
vfio_display_finalize()
vfio_display_region_exit()
vfio_region_exit()
By then device_unparent() has run, so the name becomes "display mmaps[0]"
without the path prefix. Then cpr_delete_ramblock() in vfio_region_exit()
finds nothing, and the saved entry stays while vfio_region_finalize()
munmap()s the region.
Could this leave a stale entry that makes a later hot-replug of a device
with the same path hit exit(1) in vfio_region_cpr_find(), or makes a later
cpr-exec try MADV_DOEXEC on an unmapped range?
> +/* Same checks as for RAM blocks in ram_block_add() */
> +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 (cpr_exec_is_incoming()) {
> + if (!ram.addr || !ram.length) {
> + error_report("CPR mode=exec is requested, but mapping %s can't "
> + "be reused: addr=%p length=%zu. Aborting",
> + name, (void *)ram.addr, ram.length);
> + exit(1);
> + }
[Severity: Medium]
Can this exit(1) be reached for a region that old QEMU legitimately could
not mmap?
In old QEMU, vfio_bar_register() treats a vfio_region_mmap() failure as
non-fatal:
if (vfio_region_mmap(&bar->region)) {
error_report("Failed to mmap %s BAR %d. Performance may be slow",
vdev->vbasedev.name, nr);
}
The no_mmap path in vfio_region_mmap() calls vfio_subregion_unmap(), which
calls cpr_delete_ramblock(). A failed region therefore has no CPR entry.
The only blocker added by this patch is mmap_blocker, which applies to
cpr-transfer only. Nothing blocks cpr-exec in this state, so the new QEMU
runs vfio_region_mmap() again. Then vfio_region_cpr_find() sees
cpr_exec_is_incoming() true and no entry, and exits.
The comment says this mirrors ram_block_add(), but RAM blocks are always
allocated while BAR mmaps are optional. Should this fall back to mmap() or
the slow path when no entry exists, or should cpr-exec be blocked when a
region could not be mapped?
> + assert(ram.length == region->mmaps[index].size);
> + } else if (ram.addr || ram.length) {
> + error_report("CPR mode=exec wasn't requested, but mapping %s was "
> + "saved: addr=%p length=%zu. Aborting",
> + name, (void *)ram.addr, ram.length);
> + exit(1);
> + }
[Severity: High]
Can this second branch kill QEMU after a cpr-exec that had otherwise
succeeded, for the lazily mapped display region?
The sequence would be:
1. Old QEMU maps the display buffer from vfio_display_region_update(), and
vfio_region_cpr_save() adds "<path>/display mmaps[0]" to the ramblock
list. cpr_exec_cb() preserves it.
2. New QEMU loads the entry in cpr_state_load(), which also sets
incoming_mode to MIG_MODE_CPR_EXEC. Nothing maps the display region at
realize time, since vfio_display_region_init() only registers the
gfx_update callback.
3. migration_incoming_state_destroy() calls
cpr_set_incoming_mode(MIG_MODE_NONE).
4. The first gfx_update with an active plane after that calls
vfio_region_setup() and vfio_region_mmap(), and then
vfio_region_cpr_find().
The saved entry is still present, because only vfio_subregion_unmap() and
vfio_region_exit() delete it. cpr_exec_is_incoming() is now false, so the
"wasn't requested, but mapping %s was saved" branch calls exit(1).
The check was copied from ram_block_add(), which runs at realize time
while the incoming mode is still set. Is it valid for a mapping that can be
created after the mode is reset?
Even without the exit, the preserved mapping would be neither reused nor
released.
[ ... ]
> @@ -307,6 +352,19 @@ int vfio_region_mmap(VFIORegion *region)
> return 0;
> }
>
> + /*
> + * Only cpr-exec keeps the address space, a cpr-transfer target would
> + * have to map the regions anew, which some drivers (NVIDIA vGPU) refuse
> + * while the old mappings exist. Not supported.
> + */
> + if (!region->vbasedev->cpr.mmap_blocker) {
> + error_setg(®ion->vbasedev->cpr.mmap_blocker,
> + "cpr-transfer is not supported with mmapped VFIO regions "
> + "(%s); use the cpr-exec mode", region->vbasedev->name);
> + migrate_add_blocker_modes(®ion->vbasedev->cpr.mmap_blocker,
> + &error_abort, MIG_MODE_CPR_TRANSFER, -1);
> + }
[Severity: Medium]
Can passing &error_abort here abort QEMU at runtime?
migrate_add_blocker_modes() calls is_busy(), which fails with -EBUSY and
fills errp when the following holds:
if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
error_propagate_prepend(errp, *reasonp, ...
vfio_region_mmap() is not only a realize-time call. The gfx_update
callback reaches it lazily:
vfio_display_region_update()
vfio_region_mmap(&dpy->region.buffer)
For a device whose BARs have no mmap, or whose BAR mmaps failed, the first
blocker registration happens at runtime. If a migration or snapshot is
running at that moment, error_abort turns the -EBUSY into abort().
Since is_busy() does not depend on the mode, would this also hit ordinary
non-CPR migrations? Could the -EBUSY be handled instead of aborting?
[Severity: Low]
Is the blocker installed too early here? It is added before the mmap loop
and is not removed on the no_mmap error path, nor by vfio_subregion_unmap()
or vfio_region_exit().
The only removal is migrate_del_blocker() in vfio_device_unprepare(). The
commit message says the block applies "for devices with mmapped regions",
but a device whose first mmap() fails, or whose mappings were all undone,
still blocks cpr-transfer until vfio_device_unprepare().
[Severity: Low]
Is the blocker intended to apply to every device with mmapped regions?
It is registered unconditionally for any region with nr_mmaps > 0, so
nearly every vfio-pci device blocks cpr-transfer. Before this patch, VFIO
devices reached cpr-transfer through no such check, since
vfio_devices_present() was only consulted through
migration_is_blocked()->migrate_cpr_is_blocked() when migrate_mode() ==
MIG_MODE_CPR_EXEC.
The commit message gives the NVIDIA vGPU case as the reason, but the code
does not narrow the block to drivers that refuse to remap. Could the
blocker be limited to the affected drivers or devices?
[ ... ]
> @@ -314,6 +372,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;
> + }
> +
[ ... ]
> @@ -410,6 +475,13 @@ void vfio_region_exit(VFIORegion *region)
>
> for (i = 0; i < region->nr_mmaps; i++) {
> if (region->mmaps[i].mmap) {
> + /*
> + * The mapping is unmapped by vfio_region_finalize(), but by
> + * then the device is unparented and its path, part of the CPR
> + * name, is gone. So drop the CPR entry here.
> + */
> + g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> + cpr_delete_ramblock(cpr_name);
> memory_region_del_subregion(region->mem, ®ion->mmaps[i].mem);
> }
> }
This comment is correct for the BAR path through vfio_exitfn(), but is the
same ordering guaranteed when vfio_region_exit() is reached from
vfio_display_finalize()? As described above, the device is already
unparented there.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=23
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH QEMU HCI-8.1 v2 24/24] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
2026-10-13 15:28 [PATCH QEMU HCI-8.1 v2 00/24] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (22 preceding siblings ...)
2026-10-13 15:28 ` [PATCH QEMU HCI-8.1 v2 23/24] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-13 15:28 ` Andrey Drobyshev
2026-10-13 15:57 ` no-reply
23 siblings, 1 reply; 49+ messages in thread
From: Andrey Drobyshev @ 2026-10-13 15:28 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 88b9e8752cd..00012f0f2a9 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 2e4be2db65d..67f1fcea21e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2103,9 +2103,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] 49+ messages in thread