* [PATCH QEMU HCI-8.1 01/21] migration: Plug memory leaks after migrate_set_error()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:09 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 02/21] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
` (19 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 02/21] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 01/21] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:09 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 03/21] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
` (18 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 03/21] vfio/pci: Reject invalid MSI-X Table and PBA BIR values
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 01/21] migration: Plug memory leaks after migrate_set_error() Andrey Drobyshev
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 02/21] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:10 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 04/21] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
` (17 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 04/21] vfio/pci: Fix information leak in vfio_rom_read()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (2 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 03/21] vfio/pci: Reject invalid MSI-X Table and PBA BIR values Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:10 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 05/21] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
` (16 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 05/21] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (3 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 04/21] vfio/pci: Fix information leak in vfio_rom_read() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:13 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 06/21] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
` (15 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 06/21] vfio/pci: Grow buffer in vfio_pci_host_match()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (4 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 05/21] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:10 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 07/21] q35: Fix migration of SMRAM state Andrey Drobyshev
` (14 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 07/21] q35: Fix migration of SMRAM state
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (5 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 06/21] vfio/pci: Grow buffer in vfio_pci_host_match() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:12 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 08/21] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
` (13 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 07/21] q35: Fix migration of SMRAM state
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 07/21] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-09 15:12 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:12 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] q35: mch_update_smbase_smram() locks SMBASE on a pre-handshake write with bit 1 set
--
commit ba3fa220214fc6ee0a78056885c0b2f72b87e024
Author: Igor Mammedov <imammedo@redhat.com>
q35: Fix migration of SMRAM state
This patch stops mch_update_smbase_smram() from clearing the migrated
MCH_HOST_BRIDGE_F_SMBASE value. It re-derives wmask from the register
value instead, so a locked SMRAM state survives migration. It also
removes the qtest check that junk written before the handshake reads
back as 0.
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
[ ... ]
> /*
> - * 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();
[Severity: Low]
Can a guest now lock SMBASE without doing the 0xff handshake? After
mch_reset(), config[0x9c] is 0 and wmask[0x9c] is 0xff. A guest config
write of 0x02, 0x03 or 0xfe is stored by pci_default_write_config(),
because wmask is 0xff.
mch_write_config() then calls mch_update_smbase_smram(). The value is
neither 0xff nor 0x01, so it reaches this branch:
if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
*reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
}
wmask becomes 0 and *reg becomes 0x02. smbase_blackhole and
smbase_window are then enabled, so the 128K SMRAM window at SMBASE is
black-holed until machine reset.
The old wmask == 0xff check discarded such a write and set *reg to 0.
Other junk values are now stored as written, with no effect.
Only the writing guest is affected, and a reset clears it. Firmware such
as SeaBIOS and OVMF uses the 0xff query handshake first, so it is
unaffected. The commit message describes this trade-off, and the change
matches upstream QEMU 66cf169e29b0. Is this guest-visible change
intended?
The migration fix itself looks correct. mch_post_load() calls
mch_update(), which calls mch_update_smbase_smram() and re-derives wmask
from the migrated register value. A value of 0x01 gives wmask 0x02 with
no black hole. A value of 0x02 gives wmask 0 with the black hole
enabled. A value of 0 keeps the reset wmask of 0xff.
> diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
> --- 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);
> - }
> -
[Severity: Low]
This isn't a bug, but the removed check is not replaced by anything.
test_smram_smbase_lock() no longer covers a junk write before the
handshake, or the new lock-on-junk behaviour where a value with bit 1
set locks the register. No test covers migration of the SMRAM state
either.
Could a test be added for these cases?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=7
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 08/21] migration: ensure APIC is loaded prior to VFIO PCI devices
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (6 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 07/21] q35: Fix migration of SMRAM state Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:13 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 09/21] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
` (12 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 09/21] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (7 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 08/21] migration: ensure APIC is loaded prior to VFIO PCI devices Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:17 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier Andrey Drobyshev
` (11 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 09/21] migration: Fix regression of passing error_fatal into vmstate_load_state()
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 09/21] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-09 15:17 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:17 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() calls error_setg() on an errp already set by the nested call
- [Low] migration: void vmstate save helpers log failure but never flag the QEMUFile
--
Commit e12869dadb039e65c940e64b56a6161151d7d971
Author: Arun Menon <armenon@redhat.com>
migration: Fix regression of passing error_fatal into vmstate_load_state()
This replaces &error_fatal in the vmstate_save_state() and
vmstate_load_state() callers with a local Error object. Errors are now
reported with error_report_err() and the return value is propagated where
the caller has one.
Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
[ ... ]
> void pci_device_save(PCIDevice *s, QEMUFile *f)
> {
> + Error *local_err = NULL;
> + int ret;
> +
> /* Clear interrupt status bit: it is implicit
> * in irq_state which we are saving.
> * This makes us compatible with old devices
> * which never set or clear this bit. */
> s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> - vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal);
> + ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err);
> + if (ret < 0) {
> + error_report_err(local_err);
> + }
> /* Restore the interrupt status bit. */
> pci_update_irq_status(s);
> }
[Severity: Medium]
Can this abort QEMU through the assertion in error_setv()?
In vmstate_save_state_v(), a nested VMS_STRUCT, VMS_VSTRUCT or subsection
save is called with the same errp:
if (inner_field->flags & VMS_STRUCT) {
ret = vmstate_save_state(f, inner_field->vmsd, curr_elem,
vmdesc_loop, errp);
}
...
if (ret) {
error_setg(errp, "Save of field %s/%s failed",
vmsd->name, field->name);
If the nested call fails, it has already set *errp. The outer loop then
calls error_setg() on the same errp, which reaches:
assert(*errp == NULL);
With &error_fatal, QEMU exited before this point. With a real Error * as
passed here, the second error_setg() looks like it would hit the assertion
instead of propagating the error.
The same applies to the other converted save callers in this patch, such
as virtio_pci_save_extra_state() and virtio_save(). vmstate_save_state_v()
does not appear to use ERRP_GUARD or check for an already set error.
Could the outer error_setg() be skipped when *errp is already set, or could
error_prepend() be used there instead?
[Severity: Low]
Is logging the error here enough?
pci_device_save() returns void and does not call qemu_file_set_error().
vmstate_save_state_v() only does error_setg() and returns ret on failure,
unlike the load path, which calls qemu_file_set_error(). So the QEMUFile
error state stays clean after a failed save.
The source then continues writing the stream, and the failure would show
up later as a load error on the destination. Before this patch, QEMU
exited at the point of failure.
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
[ ... ]
> + rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
> + if (rc < 0) {
> + error_report_err(local_err);
> + return;
> + }
[Severity: Low]
Does vscsi_save_request() leave its caller unaware of the failed save?
This void function reports the error and returns. Nothing calls
qemu_file_set_error(f, rc), so the caller continues as if the save
succeeded.
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
[ ... ]
> 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);
> + }
> }
[Severity: Low]
Does this void save callback have the same gap as pci_device_save()?
The failure is only logged and the QEMUFile is not flagged with
qemu_file_set_error(). The same pattern appears in
virtio_mmio_save_extra_state() and virtio_ccw_save_config().
Would it make sense for vmstate_save_state_v() or these helpers to set the
QEMUFile error on failure?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=9
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (8 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 09/21] migration: Fix regression of passing error_fatal into vmstate_load_state() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:16 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 11/21] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
` (10 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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.
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 | 26 ++++++++------------------
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2f8ef20c036..638fa783ef2 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,28 +2353,22 @@ 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) {
+ 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;
- }
- }
-
if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
SocketAddress *saddr = &addr->u.socket;
if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
@@ -2401,9 +2394,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
}
if (local_err) {
- if (!resume_requested) {
- yank_unregister_instance(MIGRATION_YANK_INSTANCE);
- }
migration_connect_set_error(s, local_err);
error_propagate(errp, local_err);
return;
--
2.47.1
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-09 15:16 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:16 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] migration: yank instance leaked when qmp_migrate_finish() fails synchronously, aborting next migrate
- [Medium] migration: unconditional yank_unregister_instance() in qmp_migrate() out: path on resume
--
commit 8b6d573d4b1ea369ae6968e2938e2e1b1c4366c3
Author: Fabiano Rosas <farosas@suse.de>
migration: yank: Move register instance earlier
This commit moves yank_register_instance() from qmp_migrate_finish() into
migrate_prepare(), so that it can be paired with the unregister in
migration_cleanup(). It also drops the resume_requested argument from
qmp_migrate_finish() and adds a yank_unregister_instance() call to the
local_err path at the end of qmp_migrate().
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
> diff --git a/migration/migration.c b/migration/migration.c
> --- 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: High]
Can this call abort QEMU on a retry after a failed migrate command?
If a previous attempt failed synchronously in qmp_migrate_finish(), the
instance registered here is never unregistered (see the comments below).
The state is then FAILED, so migration_is_running() is false and the next
migrate passes the checks in migrate_prepare().
yank_register_instance() then finds the stale entry via yank_find_entry()
and fails with "duplicate yank instance". Because &error_abort is passed,
QEMU aborts.
A privileged QMP client can reach this with an fd: URI naming a
nonexistent fd name, an exec: command that cannot be spawned, or a file:
target that cannot be opened, followed by any further migrate.
[ ... ]
> @@ -2354,28 +2353,22 @@ 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) {
> + yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> migration_connect_set_error(s, local_err);
> error_propagate(errp, local_err);
> }
> }
[Severity: High]
Is this unregister reached when qmp_migrate_finish() fails?
qmp_migrate_finish() is passed errp rather than &local_err, so only a
cpr_state_save() failure sets local_err in qmp_migrate(). Failures from
qmp_migrate_finish() never reach this yank_unregister_instance() call.
[Severity: Medium]
Separately, this unregister is not conditioned on this call having
registered the instance. When resume is requested, migrate_prepare()
takes the resume branch and returns true before yank_register_instance().
cpr_state_save() looks only at migrate_mode() and not at the resume flag.
If mode is cpr-exec or cpr-transfer and cpr_state_save() fails (for
example cpr_exec_output() failing), the out: block calls
yank_unregister_instance() on the instance owned by the paused postcopy
migration.
Would the later migration_cleanup() then call yank_unregister_instance()
again and hit assert(entry)? The old code guarded both register and
unregister with !resume_requested. Should the unregister here be skipped
for resume?
> -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;
> - }
> - }
> -
> if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
[ ... ]
> if (local_err) {
> - if (!resume_requested) {
> - yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> - }
> migration_connect_set_error(s, local_err);
> error_propagate(errp, local_err);
> return;
[Severity: High]
Does this error path now leak the yank instance?
The removed yank_unregister_instance() was the only release on this path.
migration_connect_set_error() only calls migrate_set_state() to FAILED and
migrate_set_error(); it does not call migration_cleanup().
When the start function fails before any channel exists, for example
fd_start_outgoing_migration() returning after monitor_get_fd() == -1,
neither migration_channel_connect() nor migration_connect() runs, so
migration_cleanup() is never reached. Does the instance registered in
migrate_prepare() then stay registered?
The same applies to the cpr-transfer deferred path:
qmp_migrate_finish_cb()
qmp_migrate_finish(addr, NULL);
cpr_state_close();
migrate_hup_delete(migrate_get_current());
No yank unregister is done there after a start failure either.
The later patches in the series change the HUP watch handling and
migration_cancel(), but do not change the yank handling in
qmp_migrate_finish() or qmp_migrate_finish_cb(). The issue is therefore
not fixed within the series.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=10
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 11/21] migration: Free cpr-transfer MigrationAddress along with gsource
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (9 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:17 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
` (9 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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 638fa783ef2..b1112556a22 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] 43+ messages in thread* [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (10 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 11/21] migration: Free cpr-transfer MigrationAddress along with gsource Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:18 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early Andrey Drobyshev
` (8 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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 b1112556a22..81d5d4dd4c0 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
@ 2026-10-09 15:18 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:18 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: cpr_transfer_add_hup_watch() takes untyped opaque but needs MigrationAddress
--
commit c7d2819ddcd0f6868aa501a95d87ca2adba9d5d9
Author: Fabiano Rosas <farosas@suse.de>
migration: Move CPR HUP watch to cpr-transfer.c
This commit moves the CPR HUP watch helpers out of migration.c into
cpr-transfer.c and renames them cpr_transfer_add_hup_watch() and
cpr_transfer_source_destroy(). Callers in migration_cleanup(),
migration_cancel(), qmp_migrate_finish_cb() and qmp_migrate() are
updated to use the new names.
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-24-farosas@suse.de
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
[ ... ]
> @@ -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);
[Severity: Low]
This isn't a bug today, but the prototype of cpr_transfer_add_hup_watch()
takes a generic "void *opaque", while the implementation in
migration/cpr-transfer.c always does
QAPI_CLONE(MigrationAddress, opaque) and registers
qapi_free_MigrationAddress as the destroy notifier.
Could the parameter be declared as "MigrationAddress *addr" instead? A
future caller passing any other pointer type would compile cleanly, and
the function would then clone and free the wrong object.
The only caller is qmp_migrate(), which passes addr, a MigrationAddress *,
so behaviour is preserved.
> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
> --- a/migration/cpr-transfer.c
> +++ b/migration/cpr-transfer.c
[ ... ]
> +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);
> +}
The same untyped "void *opaque" appears here, and the contract that it
must be a MigrationAddress is only visible inside the function body.
The old migrate_hup_add() also took "void *opaque", and the series
through 3953515702de does not change this, so a typed parameter would
have to be done here.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=12
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (11 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 12/21] migration: Move CPR HUP watch to cpr-transfer.c Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:20 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 14/21] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
` (7 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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 81d5d4dd4c0..ab1f1641c21 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-09 15:20 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:20 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: stale comment in qmp_migrate_finish_cb() describes a removed call
--
Commit ea2a5dd391e35be2ede79c38ea1ba67a80a4cdb1
Author: Peter Xu <peterx@redhat.com>
migration: Fix crash on second migration when cancel early
This commit changes migration_cancel() so that it calls migration_cleanup()
only when the cpr-transfer HUP gsource is still pending. It also makes
qmp_migrate_finish_cb() destroy the HUP source on entry, and it adds
assertions around the hup_source handling.
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 81d5d4dd4c06..ab1f1641c217 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[ ... ]
> @@ -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;
> }
[Severity: Low]
This isn't a bug, but the comment placed just before return G_SOURCE_REMOVE
in qmp_migrate_finish_cb() describes code that isn't there. It says that a
redundant cpr_transfer_source_destroy() is done at entry and that glib will
skip the detach.
In this backport, the trailing cpr_transfer_source_destroy(migrate_get_current())
was removed. The only destroy call is now the one at function entry, and no
second call follows the comment.
Could the comment be dropped or reworded? It could say that the source was
already destroyed at entry, so returning G_SOURCE_REMOVE is harmless. As it
stands it may mislead readers. Runtime behaviour is correct.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=13
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 14/21] kvm: tolerate ioctls after kvm_close()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (12 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 13/21] migration: Fix crash on second migration when cancel early Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:22 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 15/21] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
` (6 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 14/21] kvm: tolerate ioctls after kvm_close()
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 14/21] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-09 15:22 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:22 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: kvm_set_irq() still crashes or aborts after kvm_close() on legacy IRQ paths
- [Medium] kvm: irq-route and accessor helpers still deref NULL kvm_state after kvm_close()
- [Medium] kvm: early -ENODEV in kvm_vm_ioctl() leaves errno stale for callers that use errno
--
commit a16b8da8a608436a2f83533cd5dc4b33bbf1644c
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() is also changed so that
-ENODEV does not print the "MSI lost" warning. 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;
> + }
> +
> trace_kvm_vm_ioctl(type, arg);
[Severity: Medium]
The early return here, and the matching ones in kvm_ioctl() and
kvm_vcpu_ioctl(), never call ioctl(), so errno is not set. Some callers
look at errno after a negative return instead of using the returned value.
kvm_set_ioeventfd_mmio() is one of them:
ret = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &iofd);
if (ret < 0) {
return -errno;
}
return 0;
After kvm_close(), can this return 0 (silent success) if errno happens to
be 0, or an unrelated stale errno otherwise? Its callers
kvm_mem_ioeventfd_add() and kvm_mem_ioeventfd_del() call abort() on any
negative result:
r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space,
data, false, int128_get64(section->size),
match_data);
if (r < 0) {
fprintf(stderr, "%s: error deleting ioeventfd: %s (%d)\n",
__func__, strerror(-r), -r);
abort();
So a late ioeventfd removal on the retired source, for example from a
memory region commit during device teardown, could still end in abort()
instead of failing gracefully.
kvm_create_guest_memfd() has the same pattern:
fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_GUEST_MEMFD, &guest_memfd);
if (fd < 0) {
error_setg_errno(errp, errno, "Error creating KVM guest_memfd");
This reports errno rather than -ENODEV. kvm_set_memory_attributes() uses
strerror(errno) in the same way.
The KVM_GET_STATS_FD callers test "== -1", which -ENODEV does not match, so
they would carry on with stats_fd set to -19.
Would setting errno = ENODEV before each early return, or changing these
callers to use the returned value, be more robust?
[Severity: Medium]
Separately, the commit message says "any late interrupt or query on the
retired source fails gracefully". Does that hold for the helpers that
dereference kvm_state before any ioctl wrapper runs? kvm_close() sets
kvm_state to NULL, but kvm_enabled() stays true:
accel/accel-irq.c:accel_irqchip_release_virq() {
if (kvm_enabled()) {
kvm_irqchip_release_virq(kvm_state, virq);
}
...
}
accel/kvm/kvm-all.c:kvm_irqchip_release_virq() {
...
for (i = 0; i < s->irq_routes->nr; i++) {
...
}
With s == NULL, this faults on s->irq_routes before the new "!s" check can
be reached. The only early return is kvm_gsi_direct_mapping().
The same applies to kvm_irqchip_commit_routes(), which writes
s->irq_routes->flags. It is reached via accel_irqchip_commit_routes() from
the virtio-pci unmask path and from the ioapic code.
kvm_irqchip_update_msi_route() ends in kvm_update_routing_entry(s, ...),
which reads s->irq_routes->nr.
The plain accessors kvm_kernel_irqchip_split(), kvm_kernel_irqchip_allowed(),
kvm_kernel_irqchip_required(), kvm_dirty_ring_size(), kvm_has_sync_mmu() and
kvm_has_vcpu_events() read kvm_state->field with no NULL check. Only
kvm_dirty_ring_enabled() has a "kvm_state &&" guard.
One reachable caller is the virtio-pci teardown path:
hw/virtio/virtio-pci.c:kvm_virtio_pci_vq_vector_release() {
...
accel_irqchip_release_virq(irqfd->virq);
...
}
This could be reached through set_guest_notifiers(false), device reset or
device_del on the retired source, since QMP is still served. I did not trace
one specific exit or QMP sequence that reaches an unguarded helper.
Nothing later in the series appears to guard these helpers. The follow-up
patch "kvm: fix coalesced MMIO flush racing with kvm_close()" only adds
"kvm_state->coalesced_mmio_ring = NULL; drain_call_rcu();" to kvm_close().
Should these helpers check for a closed kvm_state as well?
[Severity: Medium]
The commit message claim that any late interrupt fails gracefully also looks
too broad for the legacy IRQ path. kvm_set_irq() evaluates s->irq_set_ioctl
before kvm_vm_ioctl() is entered:
accel/kvm/kvm-all.c:kvm_set_irq() {
...
ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
if (ret < 0) {
perror("kvm_set_irq");
abort();
}
return (s->irq_set_ioctl == KVM_IRQ_LINE) ? 1 : event.status;
}
After kvm_close(), can a late level-IRQ change reach this through the
callers below, all of which pass kvm_state?
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);
hw/intc/ioapic.c:ioapic_update_kvm_routes()
kvm_set_irq(kvm_state, i, 1);
With s == NULL, the s->irq_set_ioctl read faults before the new guard runs.
If s were non-NULL, the new -ENODEV return would reach perror() followed by
abort(). Either way the retired source would still die with SIGSEGV or
SIGABRT and leave a core dump, as the commit message describes for the MSI
path.
I did not trace a concrete device that drives these legacy lines on a
retired source. Could kvm_set_irq() and its callers be made to tolerate the
closed state too?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=14
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 15/21] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (13 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 14/21] kvm: tolerate ioctls after kvm_close() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:22 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 16/21] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
` (5 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 15/21] kvm: fix coalesced MMIO flush racing with kvm_close()
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 15/21] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-09 15:22 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:22 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
> 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/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=15
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 16/21] vfio/migration: don't migrate device state during CPR
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (14 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 15/21] kvm: fix coalesced MMIO flush racing with kvm_close() Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:29 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 17/21] vfio/listener: skip dirty page tracking " Andrey Drobyshev
` (4 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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. Use
cpr_incoming_needed(), which tells exactly when the device is preserved
by CPR, both on the old and the new QEMU side.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/migration.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4c06e3db936..99d285e7b20 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,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
Error *local_err = NULL;
int ret;
+ if (cpr_incoming_needed(NULL)) {
+ return;
+ }
+
new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
VFIO_DEVICE_STATE_PRE_COPY_P2P :
VFIO_DEVICE_STATE_RUNNING_P2P;
@@ -882,6 +909,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
Error *local_err = NULL;
int ret;
+ if (cpr_incoming_needed(NULL)) {
+ return;
+ }
+
if (running) {
new_state = VFIO_DEVICE_STATE_RUNNING;
} else {
--
2.47.1
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 16/21] vfio/migration: don't migrate device state during CPR
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 16/21] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-09 15:29 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:29 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio/migration: CPR gating via cpr_incoming_needed() reads persistent parameters.mode
--
commit 1d6b3550509ad85a9b9898cb0af982dc9b67382f
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/migration: don't migrate device state during CPR
This patch skips VFIO device state migration when the migration mode is
cpr-transfer or cpr-exec. It gates the save_state handler, adds an
is_active callback, and returns early from the two vmstate-change
handlers, all using cpr_incoming_needed().
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
[ ... ]
> @@ -857,6 +880,10 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
> Error *local_err = NULL;
> int ret;
>
> + if (cpr_incoming_needed(NULL)) {
> + return;
> + }
> +
> new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
> VFIO_DEVICE_STATE_PRE_COPY_P2P :
> VFIO_DEVICE_STATE_RUNNING_P2P;
[Severity: Medium]
Can this early return skip the P2P quiesce step outside of an in-flight
CPR?
cpr_incoming_needed() only checks the configured mode:
migration/cpr.c:cpr_incoming_needed() {
MigMode mode = migrate_mode();
return mode == MIG_MODE_CPR_TRANSFER || mode == MIG_MODE_CPR_EXEC;
}
migrate_mode() uses the incoming mode when set, and otherwise falls back
to the persistent parameters.mode:
migration/options.c:migrate_mode() {
MigMode mode = cpr_get_incoming_mode();
if (mode == MIG_MODE_NONE) {
mode = migrate_get_current()->parameters.mode;
}
...
}
On the destination, vm_start() runs in process_incoming_migration_bh()
before migration_incoming_state_destroy() calls
cpr_set_incoming_mode(MIG_MODE_NONE). After that, only parameters.mode is
seen, and it stays cpr-transfer or cpr-exec. A later plain stop or cont on
the destination would then return early here and in vfio_vmstate_change().
In that case the device is never moved to STOP and could keep running,
and possibly DMAing, while the guest is paused.
The same applies if the mode is changed between a stop and the matching
start. It also applies if a normal migration is started while the VM is
already stopped, where the device would never be moved to STOP_COPY.
The gate works in the intended CPR flow. On the source the mode is set
before the migrate command, and on the destination incoming_mode is still
set during vm_start(). The concern is that these handlers run on every
stop and cont, not only during a CPR.
> @@ -882,6 +909,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
> Error *local_err = NULL;
> int ret;
>
> + if (cpr_incoming_needed(NULL)) {
> + return;
> + }
> +
> if (running) {
> new_state = VFIO_DEVICE_STATE_RUNNING;
> } else {
[Severity: Medium]
Does this early return leave the tracked migration->device_state out of
sync with the VM runstate?
The same persistent parameters.mode dependence applies here as in
vfio_vmstate_change_prepare(). A stop or cont while parameters.mode is
still cpr-transfer or cpr-exec would skip the RUNNING/STOP transition.
Is there a per-migration indicator that could be used instead of
cpr_incoming_needed()?
The commit message says "Use cpr_incoming_needed(), which tells exactly
when the device is preserved by CPR, both on the old and the new QEMU
side." Is this accurate?
cpr_incoming_needed() is used elsewhere for vmstate .needed callbacks,
which are evaluated only during a CPR save or load. Here it is used in
handlers that run on every stop and cont.
The series-final state (patches 17 to 21) leaves these gating sites
unchanged: hw/vfio/migration.c in vfio_save_state(), vfio_is_active(),
vfio_vmstate_change_prepare() and vfio_vmstate_change().
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=16
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 17/21] vfio/listener: skip dirty page tracking during CPR
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (15 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 16/21] vfio/migration: don't migrate device state during CPR Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:27 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860 Andrey Drobyshev
` (3 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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 or stop dirty tracking in CPR modes.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/listener.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index afcf518f567..5585d656c4a 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 {
@@ -1031,6 +1037,10 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
Error *local_err = NULL;
int ret = 0;
+ if (cpr_incoming_needed(NULL)) {
+ return;
+ }
+
if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
vfio_devices_dma_logging_stop(bcontainer);
} else {
--
2.47.1
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 17/21] vfio/listener: skip dirty page tracking during CPR
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 17/21] vfio/listener: skip dirty page tracking " Andrey Drobyshev
@ 2026-10-09 15:27 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:27 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio: log_global_stop() gated on live migrate_mode(), can leak dirty tracking
- [Low] vfio: log_global_start/stop skip tracking by configured mode, not by CPR migration
--
commit 6ce52b002b526df0523d3f2c394521c467727df4
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
vfio/listener: skip dirty page tracking during CPR
This patch makes vfio_listener_log_global_start() and
vfio_listener_log_global_stop() return early when cpr_incoming_needed()
is true. The goal is to avoid starting or stopping VFIO dirty tracking
in the cpr-transfer and cpr-exec modes, where guest RAM is preserved in
place.
> 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: Low]
Is the cpr_incoming_needed(NULL) check the right condition here?
cpr_incoming_needed() returns true whenever migrate_mode() is
MIG_MODE_CPR_TRANSFER or MIG_MODE_CPR_EXEC. migrate_mode() falls back to
the persistent parameter when no incoming mode is set:
migration/options.c:migrate_mode() {
MigMode mode = cpr_get_incoming_mode();
if (mode == MIG_MODE_NONE) {
mode = migrate_get_current()->parameters.mode;
}
...
}
So the check is true whenever a CPR mode is merely configured through
migrate-set-parameters, whether or not a CPR migration is running.
The listener callbacks also run for the other global dirty-log clients.
One example is calc-dirty-rate, and dirty-limit reaches them through
global_dirty_log_change() in migration/dirtyrate.c:
ret = memory_global_dirty_log_start(flag, &local_err);
With a CPR mode configured, those clients no longer start VFIO container
or device dirty tracking. Because vfio_log_sync_needed() requires
vfio_container_dirty_tracking_is_started(), log_sync is skipped too. Can
the dirty-rate and dirty-limit results then undercount writes made by VFIO
devices?
Would a check tied to a running CPR migration, or to the
GLOBAL_DIRTY_MIGRATION flag, be more precise? The commit message says
"during CPR" but does not mention this wider scope.
Also, cpr_incoming_needed() is the incoming-side VMState predicate, and
here it is reused for a source-side decision. That is not obvious to a
reader.
> @@ -1031,6 +1037,10 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
> Error *local_err = NULL;
> int ret = 0;
>
> + if (cpr_incoming_needed(NULL)) {
> + return;
> + }
> +
[Severity: Medium]
Can this early return leave VFIO dirty tracking running?
The stop is skipped based on the current migration mode, not on whether
tracking was actually started. Consider this sequence:
memory_global_dirty_log_start()
vfio_listener_log_global_start() /* mode is normal, tracking starts */
qmp migrate-set-parameters mode=cpr-transfer (or cpr-exec)
migrate_params_apply()
s->parameters.mode = params->mode; /* no check for active tracking */
memory_global_dirty_log_do_stop()
vfio_listener_log_global_stop()
cpr_incoming_needed(NULL) /* now true */
return;
Neither vfio_devices_dma_logging_stop() nor
vfio_container_set_dirty_page_tracking(bcontainer, false, ...) runs.
vbasedev->dirty_tracking and the container dirty_pages_started state stay
set, and DMA logging stays enabled on the device. That is the vGPU
situation the commit message wants to avoid.
The window can be wide. memory_global_dirty_log_stop() postpones the stop
when the VM is not running:
if (!runstate_is_running()) {
/* Postpone the dirty log stop, e.g., to when VM starts again */
so the mode can also change during that gap.
The reverse case (start skipped under CPR, stop run under a normal mode)
looks harmless, since vfio_devices_dma_logging_stop() checks
vbasedev->dirty_tracking and vfio_container_set_dirty_page_tracking()
returns early when the state already matches.
Could the cpr_incoming_needed() check be dropped from
vfio_listener_log_global_stop(), given that the stop helpers are
idempotent? Alternatively, could start record whether tracking was
actually started, with stop testing that state?
I did not find changes to hw/vfio/listener.c, migration/options.c,
system/memory.c or migration/cpr.c later in the series that address this.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=17
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (16 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 17/21] vfio/listener: skip dirty page tracking " Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:31 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
` (2 subsequent siblings)
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 UTC (permalink / raw)
To: svt-core; +Cc: Andrey Drobyshev
file_ram_alloc() aligns the mapping of an FD-backed RAM block to 4K for
memfd and tmpfs. Anon RAM by qemu_anon_ram_alloc() is aligned to
QEMU_VMALLOC_ALIGN instead, which is 2M on x86_64. Currently, for
FD-backed RAM that's only done on s390x KVM.
Without such alignment, shared memory can't be mapped with 2M
transparent huge pages. That makes "memory-backend-memfd" guest RAM
backed by 4K pages only, even with 'advice' set to
/sys/kernel/mm/transparent_hugepage/shmem_enabled.
This matters for VMs with VFIO devices and cpr-exec. For them, anon RAM
pages preserved via MADV_DOEXEC get pinned by VFIO_IOMMU_MAP_DMA and
then replaced by copies on first guest write after exec(), while the
device keeps doing DMA to the old pinned pages. Therefore we're going
to use "memory-backend-memfd" RAM backend for such VMs, and we don't
want to lose THP.
So align FD-backed RAM blocks to QEMU_VMALLOC_ALIGN on all hosts.
Nothing changes for hugetlbfs-backed RAM, whose page size is at least
2M already.
Measured on a 2G guest with a vfio-pci NIC, memory-backend-memfd with
share=on, and shmem_enabled=advise:
before after
mapping offset 0x1ff000 0x0
ShmemPmdMapped 0 kB 2097152 kB
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
system/physmem.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 6f89232d257..ae5aa534c60 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1476,11 +1476,14 @@ static void *file_ram_alloc(RAMBlock *block,
return NULL;
}
block->mr->align = MAX(block->page_size, block->mr->align);
-#if defined(__s390x__)
- if (kvm_enabled()) {
- block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
- }
-#endif
+
+ /*
+ * Align like anonymous RAM (qemu_anon_ram_alloc), so that shmem-backed
+ * RAM (memfd, tmpfs files) is eligible for THP.
+ *
+ * On s390x KVM this alignment is required.
+ */
+ block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
if (memory < block->page_size) {
error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
--
2.47.1
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 15:31 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:31 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] physmem: file_ram_alloc() raises shared mr->align to 2M for all fd RAM, with no compat
- [Medium] physmem: 2M alignment for fd-backed RAM shrinks NVDIMM PMEM size
- [Medium] physmem: 2M align forced on all fd-backed RAM, incl. non-shmem files and offsets
--
commit 06425618e51a0375322bf7c00e7205bad272313c
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860
This commit raises the alignment of fd-backed RAM blocks in
file_ram_alloc() to QEMU_VMALLOC_ALIGN on all hosts, instead of only on
s390x KVM. The goal is to make memfd and tmpfs backed guest RAM eligible
for 2M transparent huge pages.
> diff --git a/system/physmem.c b/system/physmem.c
> index 6f89232d257f..ae5aa534c60a 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1476,11 +1476,14 @@ static void *file_ram_alloc(RAMBlock *block,
> return NULL;
> }
> block->mr->align = MAX(block->page_size, block->mr->align);
> -#if defined(__s390x__)
> - if (kvm_enabled()) {
> - block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
> - }
> -#endif
> +
> + /*
> + * Align like anonymous RAM (qemu_anon_ram_alloc), so that shmem-backed
> + * RAM (memfd, tmpfs files) is eligible for THP.
> + *
> + * On s390x KVM this alignment is required.
> + */
> + block->mr->align = MAX(block->mr->align, QEMU_VMALLOC_ALIGN);
[Severity: Medium]
Can this unconditional bump of block->mr->align reject configurations
that were accepted before? mr->align is the same field returned by
memory_region_get_alignment(), so it is not only an mmap alignment.
memory_device_pre_plug() checks it against the backend size:
hw/mem/memory-device.c:memory_device_pre_plug() {
...
if (!QEMU_IS_ALIGNED(memory_region_size(mr),
memory_region_get_alignment(mr))) {
error_setg(errp, "backend memory size must be multiple of 0x%"
...
}
A memory-backend-file or memfd backend (not hugetlbfs) used with pc-dimm,
virtio-mem, nvdimm or hv-balloon, whose size is a 4K multiple but not a
2M multiple, was accepted before. Would it now fail at plug time with
"backend memory size must be multiple of 0x200000"?
The same function also does:
align = MAX(align, memory_region_get_alignment(mr));
and passes it to memory_device_get_free_addr(). Can this change the
auto-assigned guest-physical address of file-backed DIMMs once earlier
devices have odd sizes, or trigger the "memory fragmented due to
alignments" error?
virtio_mem_device_realize() (vmem->mr->align =
memory_region_get_alignment(&vmem->memdev->mr)) and hv-balloon inherit
the larger alignment as well.
No compat property or machine-type gating is added, and the commit
message does not mention these side effects. Is that intended?
[Severity: Medium]
Can this change shrink the guest-visible NVDIMM size, or make realize
fail, for file-backed NVDIMM backends?
hw/mem/nvdimm.c:nvdimm_prepare_memory_region() {
...
align = memory_region_get_alignment(mr);
...
pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
if (size <= nvdimm->label_size || !pmem_size) {
...
nvdimm->nvdimm_mr->align = align;
}
With align now 2M instead of 4K, a 1G backend with a 128K label would
expose 1G-2M rather than 1G-128K. A backend smaller than label plus 2M
would hit the "too small to contain nvdimm label and aligned PMEM" error.
The 2M alignment is also carried into nvdimm_mr->align and so into later
memory device placement. Could this break migration or compatibility
with guests expecting the old size? The commit message only discusses
memfd and THP and does not address NVDIMM sizing.
[Severity: Medium]
The commit message and comment justify this change for shmem-backed RAM
(memfd, tmpfs), but the bump applies to every fd, including regular
files, DAX/pmem files, and memory-backend-file with an offset. Should it
be limited to the shmem case?
Non-shmem fds gain nothing from the larger alignment, yet
qemu_ram_mmap() now reserves size + align of address space for them:
util/mmap-alloc.c:qemu_ram_mmap() {
total = size + align;
...
offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
ptr = mmap_activate(guardptr + offset, size, fd, qemu_map_flags,
map_offset);
...
}
Also, only the virtual address is aligned here. The file offset is
passed unchanged as map_offset, and file_ram_alloc() only requires the
offset to be a multiple of page_size:
area = qemu_ram_mmap(fd, memory, block->mr->align, qemu_map_flags, offset);
A PMD mapping needs the virtual address and file offset to be congruent
modulo 2M. With a memory-backend-file offset that is a 4K multiple but
not a 2M multiple, would this alignment still give no THP benefit?
Downstream, the memory_device_pre_plug() size check and the
nvdimm_prepare_memory_region() QEMU_ALIGN_DOWN(pmem_size, align) now
apply 2M constraints to these file-backed regions as well.
>
> if (memory < block->page_size) {
> error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=18
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (17 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 18/21] VZ: physmem: align fd-backed RAM like anonymous RAM #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:31 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
@ 2026-10-09 15:31 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:31 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] migration: cpr-exec failure rollback does not restore vhost-net ownership
- [Low] migration: unpreserve_ram() ignores MADV_DONTEXEC failure in cpr-exec rollback
--
commit e886be97f560362e9b11744697b4398676717c11
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
This commit makes cpr_exec_cb() jump to the failure path when
cpr_walk_ramblock(preserve_ram) fails, instead of calling execvp() anyway.
It adds unpreserve_ram() and cpr_exec_unpreserve_ram() to clear
MADV_DOEXEC on the saved ramblocks, and calls them from a shared fail:
label that execvp() failure also uses.
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
[ ... ]
> +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;
> +}
[Severity: Low]
Can a failed MADV_DONTEXEC go unnoticed here?
The return value of qemu_madvise() in unpreserve_ram() is discarded. If
the call fails for a block that preserve_ram() marked with MADV_DOEXEC, the
block keeps the flag in the surviving old QEMU, and nothing is logged.
By contrast, preserve_ram() reports a MADV_DOEXEC failure with
error_report_err(). The caller, cpr_exec_cb(), also drops the return value
of cpr_walk_ramblock(unpreserve_ram).
Would it be worth at least emitting an error_report() when the rollback
madvise fails?
[ ... ]
> @@ -221,15 +237,17 @@ static void cpr_exec_cb(void *opaque)
> trace_cpr_exec();
> execvp(argv[0], argv);
[ ... ]
> +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);
[Severity: Medium]
Does this failure path restore vhost-net ownership after an execvp()
failure?
Earlier in cpr_exec_cb(), just before execvp(), this is called:
vhost_net_cpr_reset_owners();
trace_cpr_exec();
execvp(argv[0], argv);
vhost_net_cpr_reset_owners() calls vhost_dev_reset_owner() for each kernel
vhost-net device, which issues VHOST_RESET_OWNER.
If execvp() fails, control reaches the fail: label. The cleanup there runs
cpr_exec_unpreserve_fds() and cpr_exec_unpreserve_ram(), but nothing
re-issues VHOST_SET_OWNER. The only caller of vhost_dev_set_owner() in the
net path is vhost_dev_init(), which runs once at device creation.
After vm_start(), virtio-net calls vhost_net_start() on devices that have
no owner. My recollection is that the kernel rejects the vring and memory
ioctls in that state, because vhost_dev_check_owner() requires
dev->mm == current->mm. I have not verified that against the kernel
source.
If that is right, virtio-net would log "unable to start vhost net ... falling
back on userspace virtio", or fail outright for vhost-force setups.
The new early goto fail after preserve_ram() failure runs before
vhost_net_cpr_reset_owners(), so it is not affected. Only the execvp()
failure path is.
Since the commit message says the VM keeps running in the old QEMU on this
path, should the rollback also re-take ownership of the vhost devices?
A diff of migration/cpr-exec.c and hw/net/vhost_net.c from this commit to
the end of the series is empty, so no later patch appears to address this.
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=19
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (18 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:36 ` no-reply
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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>]".
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, and
the VM keeps running in old QEMU.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
hw/vfio/region.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 1deaef12fd0..07027ee79f3 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -30,6 +30,7 @@
#include "qemu/units.h"
#include "monitor/monitor.h"
#include "system/ramblock.h"
+#include "migration/cpr.h"
#include "vfio-helpers.h"
/*
@@ -227,8 +228,39 @@ 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);
+}
+
+static void *vfio_region_cpr_find(VFIORegion *region, int index)
+{
+ g_autofree char *name = vfio_region_cpr_name(region, index);
+ cpr_ramblock_t ram = cpr_find_ramblock(name);
+
+ if (ram.addr) {
+ assert(ram.length == region->mmaps[index].size);
+ }
+
+ return (void *)(uintptr_t)ram.addr;
+}
+
+static void vfio_region_cpr_save(VFIORegion *region, int index)
+{
+ g_autofree char *name = vfio_region_cpr_name(region, index);
+ void *addr = region->mmaps[index].mmap;
+ size_t size = region->mmaps[index].size;
+
+ cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
+}
+
static void vfio_subregion_unmap(VFIORegion *region, int index)
{
+ g_autofree char *cpr_name = vfio_region_cpr_name(region, index);
+
+ cpr_delete_ramblock(cpr_name);
trace_vfio_region_unmap(memory_region_name(®ion->mmaps[index].mem),
region->mmaps[index].offset,
region->mmaps[index].offset +
@@ -314,6 +346,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 +384,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,
--
2.47.1
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 15:36 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:36 UTC (permalink / raw)
To: Andrey Drobyshev, svt-core
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vfio: region BAR reuse via CPR ramblock is not gated on cpr_exec_is_incoming()
- [Medium] vfio: stale CPR ramblock entry left after region finalize, reused by cpr_find
- [Low] vfio: BAR mappings in the CPR ramblock list get RAM-only mprotect handling
--
commit d9465df038b744893fa658afc7f09871b9b52f12
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
cpr_exec_cb() marks it with MADV_DOEXEC. The new QEMU then finds the entry
by name and reuses the mapping instead of calling mmap() again. This avoids
the NVIDIA vGPU driver refusing to map a BAR that old QEMU still has
recorded as mapped.
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b..07027ee79f3c 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);
> +}
> +
> +static void *vfio_region_cpr_find(VFIORegion *region, int index)
> +{
> + g_autofree char *name = vfio_region_cpr_name(region, index);
> + cpr_ramblock_t ram = cpr_find_ramblock(name);
> +
> + if (ram.addr) {
> + assert(ram.length == region->mmaps[index].size);
> + }
> +
> + return (void *)(uintptr_t)ram.addr;
> +}
[Severity: High]
Should vfio_region_cpr_find() be gated on cpr_exec_is_incoming()?
The lookup is by name only. cpr_state_save() serializes the ramblock list
for both cpr-transfer and cpr-exec, and cpr_state_load() loads it in both
modes:
migration/cpr.c:cpr_state_load() {
...
} else if (channel) {
mode = MIG_MODE_CPR_TRANSFER;
cpr_set_incoming_mode(mode);
f = cpr_transfer_input(channel, errp);
...
}
In transfer mode cpr_exec_is_incoming() is false, but the entry is still
found. The address from the old process would then be passed to
memory_region_init_ram_device_ptr() in vfio_region_mmap(). Guest BAR
accesses could then fault or hit unrelated memory.
In the opposite case, an exec-incoming QEMU with a missing entry silently
falls back to mmap(). That brings back the NVIDIA "BAR already mapped"
failure with no clear error.
ram_block_add() in system/physmem.c checks cpr_exec_is_incoming() and
reports an explicit error in both mismatch cases:
if (cpr_exec_is_incoming()) { ... } else {
if (ram.addr || ram.length) {
error_report("CPR mode=exec wasn't requested, but ram block %s was saved...");
exit(1);
Could the VFIO path have an equivalent check?
[Severity: Low]
Also, vfio_region_cpr_save() puts device MMIO BAR mappings into the generic
RAM preservation list:
> +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 });
> +}
Every cpr_walk_ramblock() user now treats these BARs as guest RAM. With the
debug option -cpr-ram-rdonly, cpr_exec_cb() runs:
if (cpr_ram_rdonly) {
cpr_walk_ramblock(mark_ram_rdonly);
}
This mprotects the BAR mappings read-only. Neither the fail: label nor the
execvp failure path restores RW. cpr_ram_restore_rw() only runs on the
incoming side. After a failed cpr-exec, the next guest MMIO store through a
ram_device pointer could SIGSEGV the old QEMU.
On the incoming side, mark_ram_rw() does this:
if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
...
abort();
}
Could that abort QEMU for a BAR mapped without PROT_WRITE? I did not
confirm that the kernel's VFIO mmap path rejects the upgrade, so this part
is speculative. The behavior is the same as guest RAM already has in this
debug mode, but the commit message does not mention the interaction.
> 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);
[Severity: Medium]
Can a stale CPR ramblock entry outlive its BAR mapping?
This is the only place the patch deletes the entry. It is not reached from
vfio_region_exit()->vfio_region_finalize(), which munmap()s the mappings
directly:
hw/vfio/region.c:vfio_region_finalize() {
...
for (i = 0; i < region->nr_mmaps; i++) {
if (region->mmaps[i].mmap) {
munmap(region->mmaps[i].mmap, region->mmaps[i].size);
}
}
...
}
Two paths seem affected.
First, vfio_display_region_update() calls:
vfio_region_exit(&dpy->region.buffer);
vfio_region_finalize(&dpy->region.buffer);
It then calls vfio_region_setup() and vfio_region_mmap() again under the
same name "display". vfio_region_cpr_find() returns the stale address, so
mmap() is skipped and a dead pointer goes to
memory_region_init_ram_device_ptr(). If the new size differs,
assert(ram.length == region->mmaps[index].size) aborts QEMU.
Second, PCI device unplug and re-hotplug goes through
vfio_pci_bars_exit()/vfio_bars_finalize()->vfio_region_finalize(). The
stale entry stays in cpr_state.ramblocks, and the next cpr-exec applies
MADV_DOEXEC to an unmapped or reused range.
The fallback cleanup in qemu_ram_free() does not seem to cover this. In the
display case, vfio_region_finalize() never unparents the mmaps[i].mem
child, so that MemoryRegion is not finalized and qemu_ram_free() never
runs. On unplug, qemu_ram_free() runs from object_property_del_all() during
object finalize. By then the qdev path is already unset (see the VSTOR-100254
comment in qemu_ram_unset_idstr()). cpr_name() then yields
"<region> mmaps[i]" without the device path, which does not match
vfio_region_cpr_name().
Should vfio_region_finalize() also call cpr_delete_ramblock() for each
mapping?
> int vfio_region_mmap(VFIORegion *region)
> {
[ ... ]
> + region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
> + if (region->mmaps[i].mmap) {
> + goto mapped;
> + }
The reuse decision here depends only on whether an entry is present. It
does not check the incoming CPR mode and does not check that the old
mapping still exists. See the comments above on vfio_region_cpr_find() and
vfio_subregion_unmap().
[ ... ]
> + vfio_region_cpr_save(region, i);
>
> +mapped:
> name = g_strdup_printf("%s mmaps[%d]",
> memory_region_name(region->mem), i);
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=20
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
2026-10-09 14:55 [PATCH QEMU HCI-8.1 00/21] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860 Andrey Drobyshev
` (19 preceding siblings ...)
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 14:56 ` Andrey Drobyshev
2026-10-09 15:34 ` no-reply
20 siblings, 1 reply; 43+ messages in thread
From: Andrey Drobyshev @ 2026-10-09 14:56 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 5a14a7d5c0a..9689f40daa1 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 ab1f1641c21..c513427df43 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] 43+ messages in thread* Re: [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
2026-10-09 14:56 ` [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860 Andrey Drobyshev
@ 2026-10-09 15:34 ` no-reply
0 siblings, 0 replies; 43+ messages in thread
From: no-reply @ 2026-10-09 15:34 UTC (permalink / raw)
To: svt-core, Andrey Drobyshev
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] migration: cpr-exec VFIO blocker removed with no check that RAM is shared/memfd-backed
- [Medium] vfio: removing vfio_devices_present() leaves vfio-user devices unblocked for cpr-exec
- [Low] migration: cpr-exec with VFIO has no up-front check for MADV_DOEXEC support
- [Low] migration: stale hw/vfio/vfio-device.h include left in migration.c
--
commit 3953515702dee287eb17eba476062c856bd30dd8
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
This patch removes the check in migrate_cpr_is_blocked() that rejected
cpr-exec migration whenever any VFIO device was present. It also removes
the now unused vfio_devices_present() helper and its declaration.
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 5a14a7d5c0a2e..9689f40daa172 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/migration/migration.c b/migration/migration.c
> index ab1f1641c2170..c513427df4367 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;
> }
[Severity: Medium]
Can this corrupt guest memory when the guest RAM is anonymous or private?
With the VFIO branch gone, migrate_cpr_is_blocked() accepts cpr-exec for
any RAM backend. cpr_exec_cb() then calls cpr_walk_ramblock(preserve_ram),
and preserve_ram() only does:
migration/cpr-exec.c:preserve_ram() {
if (qemu_madvise((void *) ram.addr, ram.length, QEMU_MADV_DOEXEC)) {
...
}
It does not check that the backing is shared or memfd. ram_block_add()
in system/physmem.c registers anonymous RAM through cpr_save_ramblock()
without any restriction, and cpr_exec_cb() then calls execvp() with the
VFIO mappings still pinned.
The sibling commit "VZ: physmem: align fd-backed RAM like anonymous RAM"
says anonymous RAM preserved via MADV_DOEXEC and pinned by
VFIO_IOMMU_MAP_DMA is replaced by copies on first guest write after
exec(), while the device keeps doing DMA to the old pinned pages. Would
that leave device DMA and guest memory incoherent?
The remaining VFIO blockers do not seem to cover this. In
hw/vfio/cpr-legacy.c, vfio_cpr_supported() only checks VFIO_UPDATE_VADDR
and VFIO_UNMAP_ALL. In hw/vfio/cpr-iommufd.c, it only checks
IOMMU_IOAS_CHANGE_PROCESS. The qemu_ram_is_shared() check in
hw/vfio/container.c only selects IOMMU_IOAS_MAP_FILE for iommufd, and the
legacy container has no equivalent.
The sibling commit relies on using memory-backend-memfd for such VMs, but
QEMU does not enforce that. Should the blanket VFIO blocker be replaced
with one that rejects cpr-exec when a VFIO device is present and any
RAMBlock is not shared and fd-backed?
[Severity: Low]
Is there an up-front check for MADV_DOEXEC support on the host kernel?
With the VFIO check removed, migrate_cpr_is_blocked() is the only gate
and it no longer verifies this. On a kernel without MADV_DOEXEC support
for shared PFN mappings of device memory, the failure shows up late:
migration/cpr-exec.c:cpr_exec_cb()
cpr_walk_ramblock(preserve_ram)
preserve_ram()
qemu_madvise(..., QEMU_MADV_DOEXEC) fails
migrate_set_state(..., MIGRATION_STATUS_FAILED)
By then the guest has been stopped and its state has been sent. The
rollback at the fail: label in cpr_exec_cb() looks correct, but could a
capability probe be added so that the migrate command is rejected up
front? The dependency appears to be documented only in the commit
message of "VZ: vfio/region: preserve BAR mappings across cpr-exec".
[Severity: Medium]
Does removing vfio_devices_present() leave vfio-user devices unblocked
for cpr-exec?
vfio_user_pci_realize() reaches vfio_device_prepare(), which inserts the
device into vfio_device_list. That list was what vfio_devices_present()
tested, so it used to reject vfio-user devices.
The other blockers do not appear to apply. vfio-user attaches through
vfio_device_attach_by_iommu_type(TYPE_VFIO_IOMMU_USER) rather than the
legacy container, so it never reaches the mdev blocker in
hw/vfio/container-legacy.c:
if (vbasedev->mdev) {
error_setg(&vbasedev->cpr.mdev_blocker,
"CPR does not support vfio mdev %s", vbasedev->name);
if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-1) < 0) {
A grep for blocker, cpr and CPR in hw/vfio-user/ finds no matches. Can a
cpr-exec migration now proceed with a vfio-user-pci device attached,
even though its connection to the external server has no CPR support?
Should a blocker be added for vfio-user devices?
[Severity: Low]
This isn't a bug, but is the include of hw/vfio/vfio-device.h in
migration/migration.c still needed?
It appears to have been used only for vfio_devices_present(). The
remaining VFIO helpers used in that file, migration_populate_vfio_info()
and migration_reset_vfio_bytes_transferred(), are declared in
migration/migration.h. This has not been confirmed with a build, so
could the include be dropped if it compiles cleanly?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=21
^ permalink raw reply [flat|nested] 43+ messages in thread