[PATCH QEMU HCI-8.1 v4 00/31] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860

[PATCH QEMU HCI-8.1 v4 00/31] vfio: support qemu-update with passed through GPU's and vGPU's #VSTOR-103860

[Andrey Drobyshev]

v3 -> v4:

- Patch 31 (NEW) ("VZ: vfio/container: lift the CPR blocker for mdev
  devices"): CPR for mdev vGPUs, needs a kernel which reverts ef3a3f6a294b

Fixes of the issues found by Sashiko AI review.

- Patch 4 (NEW) ("migration/cpr-exec: require a file or exec migration URI"):
  a save or a migration issued with the mode parameter left at cpr-exec
  after a failed qemu-update ran as cpr-exec and lost the VM

- Patch 5 (NEW) ("migration: close the CPR state if the outgoing channel
  fails to open"): the CPR state was leaked when the file or exec URI
  couldn't be opened

- Patch 14 -> 16 ("migration: yank: Move register instance earlier"): the
  postcopy guard checked a state which is never reset once postcopy was
  advised, so every later migrate was refused; the instance is registered
  with errp instead of &error_abort

- Patch 18 -> 20 ("kvm: tolerate ioctls after kvm_close()"): kvm_state is
  kept allocated and the ioctls return 0 instead of -ENODEV, since
  kvm_set_irq() and friends dereference it and abort on an error

- Patch 20 -> 22 ("migration/cpr: add cpr_is_in_progress()"): the outgoing
  CPR is told by the open CPR state, not by the runstate, which a snapshot
  of a paused VM doesn't change

- Patch 25 -> 27 ("VZ: migration/cpr-exec: take the vhost-net devices back if
  exec fails"): the vring call and err fds are set again, VHOST_RESET_OWNER
  dropped them too

v3: http://lore.virtuozzo.com/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com
---
Akihiko Odaki (1):
      vfio/pci: Grow buffer in vfio_pci_host_match()

Andrey Drobyshev (17):
      VZ: migration/cpr-exec: fix Error leak in preserve_ram()
      migration/cpr-exec: restore RAM write access if exec fails
      vfio/region: create BAR dma-bufs with O_CLOEXEC
      migration/cpr-exec: require a file or exec migration URI
      migration: close the CPR state if the outgoing channel fails to open
      kvm: tolerate ioctls after kvm_close()
      kvm: fix coalesced MMIO flush racing with kvm_close()
      migration/cpr: add cpr_is_in_progress()
      vfio/migration: don't migrate device state during CPR
      vfio/listener: skip dirty page tracking during CPR
      VZ: vfio/cpr: don't close KVM on cpr-exec
      VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved
      VZ: migration/cpr-exec: take the vhost-net devices back if exec fails
      VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released
      VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860
      VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860
      VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

Arun Menon (1):
      migration: Fix regression of passing error_fatal into vmstate_load_state()

Cédric Le Goater (2):
      vfio/pci: Reject invalid MSI-X Table and PBA BIR values
      vfio/pci: Fix information leak in vfio_rom_read()

Fabiano Rosas (3):
      migration: yank: Move register instance earlier
      migration: Free cpr-transfer MigrationAddress along with gsource
      migration: Move CPR HUP watch to cpr-transfer.c

Igor Mammedov (1):
      q35: Fix migration of SMRAM state

Mario Casquero (1):
      vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

Markus Armbruster (1):
      migration: Plug memory leaks after migrate_set_error()

Peter Xu (1):
      migration: Fix crash on second migration when cancel early

Tycho Andersen (AMD) (1):
      hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

Vladimir Sementsov-Ogievskiy (1):
      migration: vmstate_save_state_v: fix double error_setg

Yanfei Xu (1):
      migration: ensure APIC is loaded prior to VFIO PCI devices

 accel/kvm/kvm-all.c           |  24 ++++++++-
 hw/display/virtio-gpu.c       |  22 ++++++---
 hw/intc/apic_common.c         |   1 +
 hw/net/vhost_net.c            |  62 +++++++++++++++++++++--
 hw/pci-host/q35.c             |  25 +++++-----
 hw/pci/pci.c                  |  15 +++++-
 hw/s390x/virtio-ccw.c         |  17 ++++++-
 hw/scsi/spapr_vscsi.c         |  10 +++-
 hw/vfio/container-legacy.c    |  13 -----
 hw/vfio/cpr.c                 |  11 +++--
 hw/vfio/device.c              |  29 ++++++++---
 hw/vfio/listener.c            |   6 +++
 hw/vfio/migration-multifd.c   |   2 +
 hw/vfio/migration.c           |  38 ++++++++++++++-
 hw/vfio/pci.c                 |  25 +++++++---
 hw/vfio/region.c              |  47 +++++++++++++++++-
 hw/virtio/virtio-mmio.c       |  15 +++++-
 hw/virtio/virtio-pci.c        |  15 +++++-
 hw/virtio/virtio.c            |  10 ++--
 include/hw/vfio/vfio-cpr.h    |   2 +-
 include/hw/vfio/vfio-device.h |   2 -
 include/hw/virtio/vhost.h     |   2 +
 include/migration/cpr.h       |   7 +++
 include/migration/vmstate.h   |   1 +
 include/net/vhost_net.h       |   3 +-
 migration/cpr-exec.c          |  40 +++++++++++++--
 migration/cpr-transfer.c      |  33 +++++++++++++
 migration/cpr.c               |  22 +++++++++
 migration/migration.c         | 111 +++++++++++++++++++++++-------------------
 migration/migration.h         |   5 ++
 migration/multifd.c           |   2 +
 migration/vmstate.c           |   7 ++-
 tests/qtest/q35-test.c        |   6 ---
 33 files changed, 492 insertions(+), 138 deletions(-)
---
base-commit: f3c1d9ef263edfab8d2a273078493bb766dcf8f8
change-id: 20261009-vfio_v1-a176cddd6b64

Best regards,
--  
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

[PATCH QEMU HCI-8.1 v4 01/31] VZ: migration/cpr-exec: fix Error leak in preserve_ram()

[Andrey Drobyshev]

migrate_set_error() stores its own copy of the error, so free ours.

Fixes: a3745506daf9 ("VZ: migration/cpr-exec: mark saved RAM blocks with MADV_DOEXEC before execve")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index edace3f0bd8..9fd3515753a 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -148,6 +148,7 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
         error_report_err(error_copy(err));
         migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
         migrate_set_error(s, err);
+        error_free(err);
 
         return false;
     }

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 02/31] migration/cpr-exec: restore RAM write access if exec fails

[Andrey Drobyshev]

With -cpr-ram-rdonly the RAM blocks are made read-only right before
execve().  If execve() fails, the VM resumes in the old QEMU with
read-only RAM and crashes on the first write.  Make it writable again.

Fixes: 91da25f20471 ("migration/cpr-exec: add '-cpr-ram-rdonly' option to mark RAM blocks as RO #VSTOR-101579")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 9fd3515753a..c4bdf2cb99f 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -224,6 +224,9 @@ static void cpr_exec_cb(void *opaque)
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
     g_clear_pointer(&argv, g_strfreev);
     cpr_exec_unpreserve_fds();
+    if (cpr_ram_rdonly) {
+        cpr_ram_restore_rw();
+    }
 
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 03/31] vfio/region: create BAR dma-bufs with O_CLOEXEC

[Andrey Drobyshev]

The dma-buf fds are created without O_CLOEXEC, so they leak into any
exec'ed program.  With cpr-exec, every update leaks one fd per mmapped
BAR, as new QEMU creates its own.

Fixes: 96163e0dbcb4 ("hw/vfio/region: Create dmabuf for PCI BAR per region")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/region.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index 1deaef12fd0..b0332f27fcd 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -258,7 +258,7 @@ static bool vfio_region_create_dma_buf(VFIORegion *region, Error **errp)
     dma_buf = (void *)feature->data;
     *dma_buf = (struct vfio_device_feature_dma_buf) {
         .region_index = region->nr,
-        .open_flags = O_RDWR,
+        .open_flags = O_RDWR | O_CLOEXEC,
         .nr_ranges = region->nr_mmaps,
     };
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 04/31] migration/cpr-exec: require a file or exec migration URI

[Andrey Drobyshev]

cpr-exec writes the device state to the migration URI and new QEMU
reads it back after the exec, so the URI must capture the data: a
file, or an exec command that stores it.  Nothing checks that.  With a
socket URI, e.g. a save or a live migration issued while the 'mode'
parameter is left at cpr-exec after a failed qemu-update, QEMU sends a
stream without the guest RAM to the peer, execs the stale cpr-exec
command, and the VM is lost.

Refuse cpr-exec with anything but a file or exec URI, the two types
the CPR documentation allows.

Fixes: a3eae205c601 ("migration: cpr-exec mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/migration.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 2f8ef20c036..39f70328fc7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
         return false;
     }
 
+    if (migrate_mode() == MIG_MODE_CPR_EXEC &&
+        addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
+        addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
+        error_setg(errp,
+                   "cpr-exec mode requires a file or exec migration URI");
+        return false;
+    }
+
     return true;
 }
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 05/31] migration: close the CPR state if the outgoing channel fails to open

[Andrey Drobyshev]

qmp_migrate() saves the CPR state before it opens the migration
channel.  If the channel is a file or an exec command and it can't be
opened, qmp_migrate_finish() returns right away and migration_cleanup()
never runs, so the CPR state stays open: the memfd for cpr-exec, the
state file for cpr-transfer.

Close it there, next to the yank instance, which has the same problem.

Fixes: 624e6e654e11 ("migration: cpr-transfer mode")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index 39f70328fc7..132f0d403b7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2412,6 +2412,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
         if (!resume_requested) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
+        cpr_state_close();
         migration_connect_set_error(s, local_err);
         error_propagate(errp, local_err);
         return;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 06/31] migration: Plug memory leaks after migrate_set_error()

[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 c4bdf2cb99f..d37293baec5 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -231,11 +231,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

[PATCH QEMU HCI-8.1 v4 07/31] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

[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

[PATCH QEMU HCI-8.1 v4 08/31] vfio/pci: Reject invalid MSI-X Table and PBA BIR values

[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

[PATCH QEMU HCI-8.1 v4 09/31] vfio/pci: Fix information leak in vfio_rom_read()

[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

[PATCH QEMU HCI-8.1 v4 10/31] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

[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

[PATCH QEMU HCI-8.1 v4 11/31] vfio/pci: Grow buffer in vfio_pci_host_match()

[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

[PATCH QEMU HCI-8.1 v4 12/31] q35: Fix migration of SMRAM state

[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

[PATCH QEMU HCI-8.1 v4 13/31] migration: ensure APIC is loaded prior to VFIO PCI devices

[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

[PATCH QEMU HCI-8.1 v4 14/31] migration: Fix regression of passing error_fatal into vmstate_load_state()

[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

[PATCH QEMU HCI-8.1 v4 15/31] migration: vmstate_save_state_v: fix double error_setg

[Andrey Drobyshev]

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

We may call error_setg twice on same errp if inner
vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
crash on assertion in error_setv().

andrey.drobyshev:
    This commit is cherry-picked as-is, with no modifications, from the
    upstream commit d41ce10d0f5a.

Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index aea41c09946..04903131257 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 } else {
                     ret = inner_field->info->put(f, curr_elem, size,
                                                  inner_field, vmdesc_loop);
+                    if (ret < 0) {
+                        error_setg(errp, "put failed");
+                    }
                 }
 
                 written_bytes = qemu_file_transferred(f) - old_offset;
@@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 if (ret) {
-                    error_setg(errp, "Save of field %s/%s failed",
-                                vmsd->name, field->name);
+                    error_prepend(errp, "Save of field %s/%s failed: ",
+                                  vmsd->name, field->name);
                     if (vmsd->post_save) {
                         vmsd->post_save(opaque);
                     }

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 16/31] migration: yank: Move register instance earlier

[Andrey Drobyshev]

From: Fabiano Rosas <farosas@suse.de>

Move the register_instance call to migrate_prepare() so it can be
paired with the unregister_instance at migration_cleanup(). Otherwise,
the cleanup cannot be run when cpr_state_save() fails because the
instance is registered only after it.

When resuming from a paused postcopy migration, migrate_prepare()
returns early, but migration_cleanup() doesn't run, so the yank will
remain paired.

andrey.drobyshev:
    This commit was cherry-picked from the upstream commit 7e714392437c,
    adapted for the hci-dev base, which predates the upstream
    migration_connect_set_error() -> migration_connect_error_propagate()
    rename (77c82edadbe0): the error propagation calls in qmp_migrate()
    and qmp_migrate_finish() keep the older spelling.

    Also fixed here: upstream left the yank instance registered when
    qmp_migrate_finish() fails before connecting (nothing calls
    migration_cleanup() then), so the next migrate command aborted on
    yank_register_instance(&error_abort); and the unregister at the out:
    label ran on a resume, which never registered the instance.  Upstream
    fixed both in 6b587af5ec9d ("migration: Fold migration_cleanup() into
    migration_connect_error_propagate()"), which needs the above rework;
    this is the equivalent for our base.

    Also register the instance with errp, before migrate_init(), instead
    of with &error_abort: a duplicate instance, e.g. a previous migration
    still being cleaned up, then fails the migrate command instead of
    aborting QEMU.  And refuse migrate while an incoming postcopy
    migration is still in progress: the guest already runs then, but the
    incoming side still holds the yank instance.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 132f0d403b7..802dc900c6e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2182,6 +2182,15 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         return false;
     }
 
+    /*
+     * The guest already runs during incoming postcopy, but the incoming
+     * migration still holds the yank instance registered below.
+     */
+    if (migration_in_incoming_postcopy()) {
+        error_setg(errp, "Incoming postcopy migration is still in progress");
+        return false;
+    }
+
     if (runstate_check(RUN_STATE_POSTMIGRATE)) {
         error_setg(errp, "Can't migrate the vm that was paused due to "
                    "previous migration");
@@ -2239,15 +2248,19 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         }
     }
 
+    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+        return false;
+    }
+
     if (migrate_init(s, errp)) {
+        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         return false;
     }
 
     return true;
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp);
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
 
 static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
                             void *opaque)
@@ -2272,7 +2285,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());
@@ -2284,7 +2297,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;
@@ -2337,8 +2349,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;
     }
@@ -2362,27 +2373,26 @@ void qmp_migrate(const char *uri, bool has_channels,
                         QAPI_CLONE(MigrationAddress, addr));
 
     } else {
-        qmp_migrate_finish(addr, resume_requested, errp);
+        qmp_migrate_finish(addr, errp);
     }
 
 out:
     if (local_err) {
+        /* A resume didn't register it, see migrate_prepare() */
+        if (s->state != MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP) {
+            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+        }
         migration_connect_set_error(s, local_err);
         error_propagate(errp, local_err);
     }
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp)
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
 {
     MigrationState *s = migrate_get_current();
     Error *local_err = NULL;
-
-    if (!resume_requested) {
-        if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
-            return;
-        }
-    }
+    /* Before the start functions below change the state */
+    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
 
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         SocketAddress *saddr = &addr->u.socket;
@@ -2409,7 +2419,8 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
     }
 
     if (local_err) {
-        if (!resume_requested) {
+        /* Nothing connected: migration_cleanup() won't unregister it */
+        if (!resume) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
         cpr_state_close();

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 17/31] migration: Free cpr-transfer MigrationAddress along with gsource

[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 802dc900c6e..63e9dd54805 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2265,9 +2265,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)
@@ -2289,7 +2291,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;
 }
 
@@ -2370,7 +2371,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

[PATCH QEMU HCI-8.1 v4 18/31] migration: Move CPR HUP watch to cpr-transfer.c

[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 63e9dd54805..b7db5c94b0d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,7 +110,6 @@ static bool migration_object_check(MigrationState *ms, Error **errp);
 static bool migration_switchover_start(MigrationState *s, Error **errp);
 static bool close_return_path_on_source(MigrationState *s);
 static void migration_completion_end(MigrationState *s);
-static void migrate_hup_delete(MigrationState *s);
 
 static void migration_downtime_start(MigrationState *s)
 {
@@ -1508,7 +1507,7 @@ static void migration_cleanup(MigrationState *s)
 
     qemu_savevm_state_cleanup();
     cpr_state_close();
-    migrate_hup_delete(s);
+    cpr_transfer_source_destroy(s);
 
     close_return_path_on_source(s);
 
@@ -1669,7 +1668,7 @@ void migration_cancel(void)
         migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                           MIGRATION_STATUS_CANCELLED);
         cpr_state_close();
-        migrate_hup_delete(s);
+        cpr_transfer_source_destroy(s);
     }
 }
 
@@ -2262,25 +2261,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)
@@ -2290,7 +2270,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;
 }
 
@@ -2370,8 +2350,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

[PATCH QEMU HCI-8.1 v4 19/31] migration: Fix crash on second migration when cancel early

[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 b7db5c94b0d..3732751e66c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1661,14 +1661,19 @@ void migration_cancel(void)
     }
 
     /*
-     * If qmp_migrate_finish has not been called, then there is no path that
-     * will complete the cancellation.  Do it now.
+     * This is cpr-transfer specific processing.
+     *
+     * If this is true, it means cpr-transfer migration is waiting for the
+     * destination to send HUP event on CPR channel to continue the next
+     * phase.  If so, do the cleanup proactively to avoid get stuck in
+     * CANCELLING state.
      */
-    if (setup && !s->to_dst_file) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
-                          MIGRATION_STATUS_CANCELLED);
-        cpr_state_close();
-        cpr_transfer_source_destroy(s);
+    if (cpr_transfer_source_active(s)) {
+        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
+        assert(setup && !s->to_dst_file);
+        migration_cleanup(s);
+        /* Now all things should have been released */
+        assert(!cpr_transfer_source_active(s));
     }
 }
 
@@ -2266,11 +2271,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

[PATCH QEMU HCI-8.1 v4 20/31] kvm: tolerate ioctls after kvm_close()

[Andrey Drobyshev]

Since commit 7ed0919119b0 ("migration: close kvm after cpr") a source
QEMU that has handed its VFIO devices over via cpr-transfer (or
cpr-exec) calls kvm_close() once the migration completes, so that the
destination can register its irq bypass consumers.  kvm_close() closes
the VM and vCPU descriptors and sets kvm_state to NULL, but the source
process keeps living with all its device models intact until the
management layer terminates it.

Nothing prevents those device models from raising interrupts in the
meantime, and the regular exit path even guarantees one: net_cleanup()
deletes the netdev behind an emulated virtio-net, the NIC sees the link
go down and calls virtio_notify_config(), MSI-X delivers through the
in-kernel APIC, and kvm_irqchip_send_msi() dereferences the NULL
kvm_state:

  #0  kvm_vm_ioctl (s=0x0, type=KVM_SIGNAL_MSI)   accel/kvm/kvm-all.c
  #1  kvm_irqchip_send_msi (s=0x0)
  #2  kvm_send_msi                                hw/i386/kvm/apic.c
  #3  kvm_apic_mem_write
  ...
  #9  pci_msi_trigger
  #11 msix_notify
  #12 virtio_pci_notify
  #14 virtio_notify_config
  #15 virtio_net_set_link_status
  #16 qemu_del_net_client
  #17 net_cleanup
  #18 qemu_cleanup

Every retired cpr-transfer source with a VFIO device thus dies with
SIGSEGV on SIGTERM/quit and leaves a core dump behind.  Without a VFIO
device kvm_close() never runs and the same exit is harmless.

Make the ioctl wrappers honour the closed state instead of assuming a
live KVMState: keep kvm_state allocated, with its closed descriptors as
the mark, and let kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl()
return 0 once kvm_close() has been called, so any late interrupt or
query on the retired source is a no-op.  Returning an error instead
would still kill it: kvm_set_irq() aborts and kvm_irqchip_commit_routes()
asserts on any failure, and the ioeventfd callers abort on a stale errno.

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 accel/kvm/kvm-all.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a39c1ba260a..8b5d342af52 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -644,7 +644,6 @@ void kvm_close(void)
         close(kvm_state->fd);
         kvm_state->fd = -1;
     }
-    kvm_state = NULL;
 }
 
 /*
@@ -3371,6 +3370,11 @@ int kvm_ioctl(KVMState *s, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: there is no KVM to talk to any more */
+    if (s->fd == -1) {
+        return 0;
+    }
+
     trace_kvm_ioctl(type, arg);
     ret = ioctl(s->fd, type, arg);
     if (ret == -1) {
@@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: there is no VM to talk to any more */
+    if (s->vmfd == -1) {
+        return 0;
+    }
+
     trace_kvm_vm_ioctl(type, arg);
     accel_ioctl_begin();
     ret = ioctl(s->vmfd, type, arg);
@@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
+    /* kvm_close() has been called: the vCPU fds are gone */
+    if (cpu->kvm_fd == -1) {
+        return 0;
+    }
+
     trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
     accel_cpu_ioctl_begin(cpu);
     ret = ioctl(cpu->kvm_fd, type, arg);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 21/31] kvm: fix coalesced MMIO flush racing with kvm_close()

[Andrey Drobyshev]

kvm_close() destroys the vCPUs, which unmaps their kvm_run areas, and the
coalesced MMIO ring lives in the kvm_run area of the first vCPU.  Nothing
stops the RCU thread from flushing that ring at the same time: finalizing
a memory region of a freed flatview calls memory_region_transaction_begin()
and thus kvm_flush_coalesced_mmio_buffer().  kvm_state is still set at
that point, so the flush reads the ring after it has been unmapped:

  #0  kvm_flush_coalesced_mmio_buffer   accel/kvm/kvm-all.c
  #1  qemu_flush_coalesced_mmio_buffer
  #2  memory_region_transaction_begin
  #3  memory_region_finalize
  ...
  #10 flatview_destroy
  #11 call_rcu_thread

while the main thread is in:

  kvm_close -> cpu_remove_sync
  vfio_kvm_device_close
  vfio_cpr_kvm_close_notifier (MIG_EVENT_PRECOPY_DONE)

Seen as a SIGSEGV of the old QEMU during repeated cpr-exec of a VM with
a VFIO device.

Clear the ring pointer first, so that new flushes skip it, and wait for
the pending RCU callbacks before destroying the vCPUs.

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 accel/kvm/kvm-all.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 8b5d342af52..294a87043ef 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -630,6 +630,14 @@ void kvm_close(void)
         return;
     }
 
+    /*
+     * Destroying vCPUs unmaps the coalesced MMIO ring, while the RCU thread
+     * may be flushing it from a memory region finalizer.  Stop new flushes
+     * and wait for the pending RCU callbacks first.
+     */
+    kvm_state->coalesced_mmio_ring = NULL;
+    drain_call_rcu();
+
     CPU_FOREACH(cpu) {
         cpu_remove_sync(cpu);
         close(cpu->kvm_fd);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 22/31] migration/cpr: add cpr_is_in_progress()

[Andrey Drobyshev]

cpr_incoming_needed() only looks at the migration mode.  That's fine for
vmstate .needed callbacks, which run during a save or load.  But code that
runs at any time, e.g. VM state change handlers, can't tell from it
whether a CPR migration is actually going on: on the source the mode is
just a parameter, set by migrate-set-parameters whether or not a migrate
follows.

Add cpr_is_in_progress(), which is true on the target until the incoming
migration is done, and on the source only while the outgoing one runs.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 include/migration/cpr.h |  1 +
 migration/cpr.c         | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 4dd397a7c13..e73c5da1d9c 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -64,6 +64,7 @@ MigMode cpr_get_incoming_mode(void);
 void cpr_set_incoming_mode(MigMode mode);
 bool cpr_is_incoming(void);
 bool cpr_exec_is_incoming(void);
+bool cpr_is_in_progress(void);
 
 bool cpr_state_save(MigrationChannel *channel, Error **errp);
 int cpr_state_load(MigrationChannel *channel, Error **errp);
diff --git a/migration/cpr.c b/migration/cpr.c
index a149e790c10..3ea5d86bab4 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -549,6 +549,28 @@ bool cpr_exec_is_incoming(void)
     return incoming_mode == MIG_MODE_CPR_EXEC;
 }
 
+bool cpr_is_in_progress(void)
+{
+    /* The incoming mode on the target, the 'mode' parameter on the source */
+    MigMode mode = migrate_mode();
+
+    if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
+        return false;
+    }
+
+    /* Target: the incoming mode is set until the incoming migration is done */
+    if (cpr_is_incoming()) {
+        return true;
+    }
+
+    /*
+     * Source: the parameter alone means nothing, it may just have been set.
+     * An outgoing CPR migration has the CPR state open, from
+     * cpr_state_save() to cpr_state_close().
+     */
+    return cpr_state_file != NULL;
+}
+
 bool cpr_state_save(MigrationChannel *channel, Error **errp)
 {
     int ret;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 23/31] vfio/migration: don't migrate device state during CPR

[Andrey Drobyshev]

cpr-transfer and cpr-exec hand the running VFIO device over to the new
QEMU as is: the device fd, its mappings and interrupts are preserved, and
the device itself is never stopped.

But if the device supports VFIO migration, its migration handlers still
run.  The old QEMU stops the device and saves its state, then the new
QEMU puts the very same device into RESUMING and loads that state back.
On top of that, vfio_pci_load_config() re-programs MSI-X and disables
vector 0.

For an NVIDIA vGPU this breaks the device after qemu-update: restoring
the state into the live vGPU fails and the guest loses the device.
Devices without migration support aren't affected, since no migration
handlers are registered for them.

Skip VFIO migration while a CPR migration is in progress: don't save or
load the device state, and don't change the device state on VM
stop/start.  All of it is gated on cpr_is_in_progress(), not on the mode
alone: the mode may merely be set, and then a plain stop/cont must still
change the device state, and a snapshot must still save it.

Also reset multifd_transfer in vfio_multifd_cleanup(): it's only set in
vfio_multifd_setup(), which CPR now skips, so a stale value from an
earlier migration would still let the multifd save thread run.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/migration-multifd.c |  2 ++
 hw/vfio/migration.c         | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index e4785031a73..452f772dd7a 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -567,6 +567,8 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev)
     VFIOMigration *migration = vbasedev->migration;
 
     g_clear_pointer(&migration->multifd, vfio_multifd_free);
+    /* Decided anew by vfio_multifd_setup() of the next migration */
+    migration->multifd_transfer = false;
 }
 
 bool vfio_multifd_transfer_supported(void)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 4c06e3db936..b142c9f2289 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -19,6 +19,7 @@
 #include "hw/vfio/vfio-device.h"
 #include "hw/vfio/vfio-migration.h"
 #include "migration/misc.h"
+#include "migration/cpr.h"
 #include "migration/savevm.h"
 #include "migration/vmstate.h"
 #include "migration/qemu-file.h"
@@ -513,7 +514,6 @@ static void vfio_save_cleanup(void *opaque)
     Error *local_err = NULL;
     int ret;
 
-    /* Currently a NOP, done for symmetry with load_cleanup() */
     vfio_multifd_cleanup(vbasedev);
 
     /*
@@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
     Error *local_err = NULL;
     int ret;
 
+    /*
+     * .is_active doesn't cover the non-iterable stage, so the section is
+     * still sent: leave it empty, or new QEMU would load the config into
+     * the preserved device and re-program its MSI-X.
+     */
+    if (cpr_is_in_progress()) {
+        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+        return;
+    }
+
     if (vfio_multifd_transfer_enabled(vbasedev)) {
         if (vfio_load_config_after_iter(vbasedev)) {
             qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
@@ -810,6 +820,11 @@ static bool vfio_switchover_ack_needed(void *opaque)
 {
     VFIODevice *vbasedev = opaque;
 
+    /* No device state is sent during CPR, so no ACK to wait for */
+    if (cpr_is_in_progress()) {
+        return false;
+    }
+
     return vfio_precopy_supported(vbasedev);
 }
 
@@ -824,7 +839,19 @@ static int vfio_switchover_start(void *opaque)
     return 0;
 }
 
+/*
+ * cpr-transfer and cpr-exec hand the running device over to the new QEMU
+ * as is: its fds, mappings and interrupts are preserved and it never stops.
+ * Device state migration must stay out of it, else the device is stopped,
+ * its state saved and then resumed into the very same device.
+ */
+static bool vfio_is_active(void *opaque)
+{
+    return !cpr_is_in_progress();
+}
+
 static const SaveVMHandlers savevm_vfio_handlers = {
+    .is_active = vfio_is_active,
     .save_prepare = vfio_save_prepare,
     .save_setup = vfio_save_setup,
     .save_cleanup = vfio_save_cleanup,
@@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
     Error *local_err = NULL;
     int ret;
 
+    /* The device is preserved, not a plain stop/cont: keep its state */
+    if (cpr_is_in_progress()) {
+        return;
+    }
+
     new_state = migration->device_state == VFIO_DEVICE_STATE_PRE_COPY ?
                     VFIO_DEVICE_STATE_PRE_COPY_P2P :
                     VFIO_DEVICE_STATE_RUNNING_P2P;
@@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
     Error *local_err = NULL;
     int ret;
 
+    if (cpr_is_in_progress()) {
+        return;
+    }
+
     if (running) {
         new_state = VFIO_DEVICE_STATE_RUNNING;
     } else {

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 24/31] vfio/listener: skip dirty page tracking during CPR

[Andrey Drobyshev]

During cpr-transfer and cpr-exec guest RAM is preserved in place, not
copied, so there's no need to track which pages the device writes to.

Yet starting the migration also starts VFIO dirty tracking, on the
container or on the device itself (DMA logging).  With cpr-exec it isn't
stopped before exec, so e.g. the DMA page tracker of an NVIDIA vGPU is
left running after the update.

Don't start dirty tracking while a CPR migration is in progress.  Gate
it on cpr_is_in_progress(), not on the mode alone, so that a snapshot
taken while the mode parameter is set to a CPR mode still tracks dirty
pages.  Stopping stays unconditional: it's a no-op if tracking wasn't
started, and the mode may have changed in between.

Known gap: the memory core starts the listeners only for the first dirty
log user.  If another one (e.g. calc-dirty-rate) appears during a CPR
that then fails, VFIO tracking stays off until QEMU restarts.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/listener.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index afcf518f567..1270b9d5f3a 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -41,6 +41,7 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "migration/misc.h"
+#include "migration/cpr.h"
 #include "migration/qemu-file.h"
 #include "system/tcg.h"
 #include "system/tpm.h"
@@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
                                              listener);
     bool ret;
 
+    /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
+    if (cpr_is_in_progress()) {
+        return true;
+    }
+
     if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
         ret = vfio_devices_dma_logging_start(bcontainer, errp);
     } else {

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 25/31] VZ: vfio/cpr: don't close KVM on cpr-exec

[Andrey Drobyshev]

With VFIO devices, KVM is closed on the source when the migration
completes, so that the new QEMU can create its own KVM VM with the same
devices.  That is done from a migration notifier, which for cpr-exec
runs before cpr_exec_cb().  But cpr_exec_cb() may still give up, when
the memory can't be preserved or execve() fails, and resume the VM in
the old QEMU.  A VM without KVM can't resume: kvm_close() removes the
vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
(KVM_SET_CLOCK in kvmclock_vm_state_change()).

cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
kernel closes them during execve(), before the new QEMU runs, and
kvm_vm_release() tears down the irqfds then.  Keep the notifier for
cpr-transfer only, where the source process lives on.

Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
preserve_ram() and one with a bogus cpr-exec-command both left the VM
running with no lost pings, and a later cpr-exec completed.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/cpr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index db462aabcbf..7ca761d5bf5 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -192,13 +192,16 @@ static int vfio_cpr_kvm_close_notifier(NotifierWithReturn *notifier,
     return 0;
 }
 
+/*
+ * Not for cpr-exec: the exec closes the KVM fds anyway, and until then
+ * cpr_exec_cb() may still give up and resume the VM, which needs KVM.
+ */
 void vfio_cpr_add_kvm_notifier(void)
 {
     if (!kvm_close_notifier.notify) {
-        migration_add_notifier_modes(&kvm_close_notifier,
-                                     vfio_cpr_kvm_close_notifier,
-                                     MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-                                     -1);
+        migration_add_notifier_mode(&kvm_close_notifier,
+                                    vfio_cpr_kvm_close_notifier,
+                                    MIG_MODE_CPR_TRANSFER);
     }
 }
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 26/31] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

[Andrey Drobyshev]

cpr_exec_cb() marks all saved ramblocks with MADV_DOEXEC before exec, but
ignores the result.  If madvise() fails for some block, the walk stops
there and the remaining blocks are not marked, yet execvp() still runs.
The new QEMU then reuses addresses of guest RAM that wasn't preserved,
and the guest is lost.

Treat that as a failed cpr-exec: go to the same failure path as when
execvp() fails, so the VM keeps running in the old QEMU.

On that path also clear MADV_DOEXEC from all saved ramblocks again, just
like the preserved fds get close-on-exec back.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 migration/cpr-exec.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index d37293baec5..dd2ff9b26ff 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -155,6 +155,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)
 {
     /*
@@ -203,7 +216,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);
     }
@@ -222,18 +238,20 @@ static void cpr_exec_cb(void *opaque)
      * or the system is very short on resources.
      */
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
-    g_clear_pointer(&argv, g_strfreev);
-    cpr_exec_unpreserve_fds();
-    if (cpr_ram_rdonly) {
-        cpr_ram_restore_rw();
-    }
-
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);
     error_free(err);
     err = NULL;
 
+fail:
+    g_clear_pointer(&argv, g_strfreev);
+    cpr_exec_unpreserve_fds();
+    cpr_exec_unpreserve_ram();
+    if (cpr_ram_rdonly) {
+        cpr_ram_restore_rw();
+    }
+
     /* Note, we can go from state COMPLETED to FAILED */
     migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
 

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 27/31] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[Andrey Drobyshev]

Right before execve(), cpr_exec_cb() releases the kernel vhost-net
devices with VHOST_RESET_OWNER, so that the new QEMU can take them.  If
execve() fails, the VM goes on in the old QEMU, but nobody takes the
devices back: the kernel refuses the ioctls of the next
vhost_net_start(), and the guest is left without network.

Take them back with VHOST_SET_OWNER when the exec fails, and set the
vring call and err fds again: VHOST_RESET_OWNER dropped them too, and
vhost_dev_init() registers them only once.  The devices are stopped at
that point, and vhost_net_start() sets up the rest again when the VM
resumes.  Only the devices that were actually released are taken back:
vhost_net_cpr_reset_owners() marks them.  vhost-vsock does the same in
its CPR notifier.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/net/vhost_net.c        | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost.h |  2 ++
 include/net/vhost_net.h   |  1 +
 migration/cpr-exec.c      |  2 ++
 4 files changed, 54 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 56c697f4bcd..fae6b060c6f 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -594,6 +594,55 @@ void vhost_net_cpr_reset_owners(void)
             if (r < 0 && r != -ENOSYS) {
                 warn_report("cpr: failed to reset vhost-net owner for %s: %s",
                             nc->name, strerror(-r));
+            } else if (r == 0) {
+                net->cpr_owner_reset = true;
+            }
+        }
+    }
+}
+
+/*
+ * Take the ownership back when the exec fails and the VM goes on in the
+ * old QEMU.  VHOST_RESET_OWNER has reset the kernel device, but the devices
+ * are stopped, and vhost_net_start() sets everything up again when the VM
+ * resumes.
+ */
+void vhost_net_cpr_set_owners(void)
+{
+    NetClientState *nc;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        VHostNetState *net = get_vhost_net(nc);
+
+        if (net && net->cpr_owner_reset) {
+            struct vhost_dev *dev = &net->dev;
+            int i, r = vhost_dev_set_owner(dev);
+
+            if (r < 0) {
+                warn_report("cpr: failed to set vhost-net owner for %s: %s",
+                            nc->name, strerror(-r));
+                continue;
+            }
+            net->cpr_owner_reset = false;
+
+            /* RESET_OWNER also dropped the vring fds set by vhost_dev_init() */
+            for (i = 0; i < dev->nvqs; i++) {
+                const VhostOps *ops = dev->vhost_ops;
+                struct vhost_virtqueue *vq = &dev->vqs[i];
+                struct vhost_vring_file file = {
+                    .index = ops->vhost_get_vq_index(dev, dev->vq_index + i),
+                    .fd = event_notifier_get_wfd(&vq->masked_notifier),
+                };
+
+                r = ops->vhost_set_vring_call(dev, &file);
+                if (r == 0 && ops->vhost_set_vring_err) {
+                    file.fd = event_notifier_get_fd(&vq->error_notifier);
+                    r = ops->vhost_set_vring_err(dev, &file);
+                }
+                if (r < 0) {
+                    warn_report("cpr: failed to set vring fds of %s: %s",
+                                nc->name, strerror(-r));
+                }
             }
         }
     }
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a92439d3443..421f7139cfb 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -151,6 +151,8 @@ struct vhost_net {
     SaveAcketFeatures *save_acked_features;
     bool is_vhost_user;
     NetClientState *nc;
+    /* released for cpr-exec, take back if it fails */
+    bool cpr_owner_reset;
 };
 
 /**
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 39ce7ad0e3d..cdfefa81500 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -90,4 +90,5 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
 void vhost_net_save_acked_features(NetClientState *nc);
 
 void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_set_owners(void);
 #endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index dd2ff9b26ff..21caa614a13 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -251,6 +251,8 @@ fail:
     if (cpr_ram_rdonly) {
         cpr_ram_restore_rw();
     }
+    /* Still the same mm, take back the vhost-net devices we released */
+    vhost_net_cpr_set_owners();
 
     /* Note, we can go from state COMPLETED to FAILED */
     migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 28/31] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released

[Andrey Drobyshev]

vhost_net_cpr_reset_owners() only warns when VHOST_RESET_OWNER fails for
a device, and the exec goes on.  But the new QEMU can't take such a
device: it is still owned by the old mm, so VHOST_SET_OWNER fails with
EBUSY, and the guest is left without network after the update.

Stop at the first device that can't be released and abort the cpr-exec
instead.  The VM goes on in the old QEMU, with the devices released so
far taken back.  vhost-vsock already does that in its pre_save.

Fixes: 1278e4830052 ("VZ: migration/cpr-exec: vhost-net: send RESET_OWNER before cpr-exec")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/net/vhost_net.c      | 19 +++++++++++++------
 include/net/vhost_net.h |  2 +-
 migration/cpr-exec.c    |  7 ++++++-
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index fae6b060c6f..9110dc43d23 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -579,8 +579,12 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
  * EBUSY.  Called right before execve(), when the VM is stopped and the
  * devices have already been stopped, so nothing is in flight.  The device is
  * fully re-initialized from the migrated state by the new QEMU.
+ *
+ * Stops at the first device that can't be released, since the new QEMU
+ * couldn't take it.  The caller then takes back the ones already released
+ * with vhost_net_cpr_set_owners().
  */
-void vhost_net_cpr_reset_owners(void)
+void vhost_net_cpr_reset_owners(Error **errp)
 {
     NetClientState *nc;
 
@@ -591,12 +595,15 @@ void vhost_net_cpr_reset_owners(void)
             net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
             int r = vhost_dev_reset_owner(&net->dev);
 
-            if (r < 0 && r != -ENOSYS) {
-                warn_report("cpr: failed to reset vhost-net owner for %s: %s",
-                            nc->name, strerror(-r));
-            } else if (r == 0) {
-                net->cpr_owner_reset = true;
+            if (r == -ENOSYS) {
+                continue;
+            }
+            if (r < 0) {
+                error_setg_errno(errp, -r, "cpr: failed to reset vhost-net "
+                                 "owner for %s", nc->name);
+                return;
             }
+            net->cpr_owner_reset = true;
         }
     }
 }
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index cdfefa81500..679edefa1d3 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -89,6 +89,6 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
 
 void vhost_net_save_acked_features(NetClientState *nc);
 
-void vhost_net_cpr_reset_owners(void);
+void vhost_net_cpr_reset_owners(Error **errp);
 void vhost_net_cpr_set_owners(void);
 #endif
diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 21caa614a13..a206ca44a4b 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -228,7 +228,10 @@ static void cpr_exec_cb(void *opaque)
      * Hand the preserved vhost-net devices over to the new QEMU: they are
      * still owned by this mm, which is about to be replaced by execve().
      */
-    vhost_net_cpr_reset_owners();
+    vhost_net_cpr_reset_owners(&err);
+    if (err) {
+        goto fail_with_err;
+    }
 
     trace_cpr_exec();
     execvp(argv[0], argv);
@@ -238,6 +241,8 @@ static void cpr_exec_cb(void *opaque)
      * or the system is very short on resources.
      */
     error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
+
+fail_with_err:
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 29/31] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[Andrey Drobyshev]

After cpr-exec, new QEMU mmaps the device BARs again on the preserved
device fd.  Some drivers don't allow that: the NVIDIA vGPU VFIO driver
refuses to map a VF BAR that is still recorded as mapped by old QEMU,
and that record is dropped only when the device fd is closed.  So BAR1
and BAR3 of the vGPU can't be mapped, and the guest loses the device.

Keep the mappings instead.  Save each BAR mapping in the CPR ramblock
list, so cpr_exec_cb() marks it with MADV_DOEXEC together with guest RAM,
and new QEMU finds it there and reuses it instead of calling mmap().
Same find-or-create pattern as for other preserved objects, e.g. vector
eventfds in vfio_notifier_init().  No saved mapping means old QEMU didn't
map the region, e.g. its mmap failed or the region is mapped lazily, so
new QEMU maps it afresh.

The ID is "<device name> region <index> mmaps[<index>]".  It doesn't
depend on the device path, which is gone when the display region is
finalized, so the entry is dropped together with the mapping.

The saved addresses only make sense for cpr-exec, which keeps the
address space.  A cpr-transfer target is another process and would have
to map the regions anew, which the drivers above refuse while the old
mappings exist.  So block cpr-transfer for VFIO devices.

This needs a kernel which supports MADV_DOEXEC on shared PFN mappings of
device memory.  Without it cpr-exec of a VM with VFIO devices fails.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/device.c           | 24 ++++++++++++++++++++++--
 hw/vfio/region.c           | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-cpr.h |  1 +
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 5a14a7d5c0a..2a7073ccec8 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
                              TYPE_VFIO_IOMMU_IOMMUFD :
                              TYPE_VFIO_IOMMU_LEGACY;
 
-    return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
-                                            as, errp);
+    /*
+     * Only cpr-exec keeps the address space and so the region mappings,
+     * see vfio_region_mmap().  A cpr-transfer target would have to map the
+     * regions anew, which some drivers (NVIDIA vGPU) refuse.
+     */
+    error_setg(&vbasedev->cpr.mmap_blocker,
+               "cpr-transfer is not supported with VFIO devices; "
+               "use the cpr-exec mode");
+    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
+                                  MIG_MODE_CPR_TRANSFER, -1)) {
+        return false;
+    }
+
+    if (!vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
+                                          as, errp)) {
+        migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+        return false;
+    }
+
+    return true;
 }
 
 void vfio_device_detach(VFIODevice *vbasedev)
 {
+    migrate_del_blocker(&vbasedev->cpr.mmap_blocker);
+
     if (!vbasedev->bcontainer) {
         return;
     }
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index b0332f27fcd..4d58794b3cf 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,43 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
     return 0;
 }
 
+static char *vfio_region_cpr_name(VFIORegion *region, int index)
+{
+    return g_strdup_printf("%s region %d mmaps[%d]", region->vbasedev->name,
+                           region->nr, index);
+}
+
+/*
+ * A saved mapping is one preserved by cpr-exec, reuse it.  None means old
+ * QEMU didn't map the region, e.g. its mmap failed or it's mapped lazily,
+ * so map it afresh.
+ */
+static void *vfio_region_cpr_find(VFIORegion *region, int index)
+{
+    g_autofree char *name = vfio_region_cpr_name(region, index);
+    cpr_ramblock_t ram = cpr_find_ramblock(name);
+
+    if (ram.addr) {
+        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(&region->mmaps[index].mem),
                             region->mmaps[index].offset,
                             region->mmaps[index].offset +
@@ -314,6 +350,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 +388,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(&region->mmaps[i].mem,
@@ -427,6 +470,8 @@ void vfio_region_finalize(VFIORegion *region)
 
     for (i = 0; i < region->nr_mmaps; i++) {
         if (region->mmaps[i].mmap) {
+            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
+            cpr_delete_ramblock(cpr_name);
             munmap(region->mmaps[i].mmap, region->mmaps[i].size);
         }
     }
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index 4606da500a7..fa1699857ca 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -34,6 +34,7 @@ typedef struct VFIOContainerCPR {
 typedef struct VFIODeviceCPR {
     Error *mdev_blocker;
     Error *id_blocker;
+    Error *mmap_blocker;
     uint32_t hwpt_id;
     uint32_t ioas_id;
 } VFIODeviceCPR;

-- 
2.47.1

[PATCH QEMU HCI-8.1 v4 30/31] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

[Andrey Drobyshev]

Commit bb8da93ddfc4 ("VZ: migration/cpr-exec: enable checks prohibiting
cpr-exec #VSTOR-108564") introduced restrictions for cpr-exec migration
mode.  Among them was prohibition to do such migration in case of
presence of any VFIO devices.  Now we want to allow migration with such
devices in scope of VSTOR-103860.  Therefore, let's lift that restiction.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/device.c              | 5 -----
 include/hw/vfio/vfio-device.h | 2 --
 migration/migration.c         | 3 ---
 3 files changed, 10 deletions(-)

diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 2a7073ccec8..d1b595adb71 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -36,11 +36,6 @@
 VFIODeviceList vfio_device_list =
     QLIST_HEAD_INITIALIZER(vfio_device_list);
 
-bool vfio_devices_present(void)
-{
-    return !QLIST_EMPTY(&vfio_device_list);
-}
-
 /*
  * We want to differentiate hot reset of multiple in-use devices vs
  * hot reset of a single in-use device. VFIO_DEVICE_RESET will already
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 935000f0074..fc28f580ba3 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -162,8 +162,6 @@ VFIODevice *vfio_get_vfio_device(Object *obj);
 typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
 extern VFIODeviceList vfio_device_list;
 
-bool vfio_devices_present(void);
-
 #ifdef CONFIG_LINUX
 /*
  * How devices communicate with the server.  The default option is through
diff --git a/migration/migration.c b/migration/migration.c
index 3732751e66c..14c883b1bcd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2111,9 +2111,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

[PATCH QEMU HCI-8.1 v4 31/31] VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

[Andrey Drobyshev]

Upstream commit dac0dd68d9b ("vfio/container: mdev cpr blocker") blocks
cpr-transfer and cpr-exec for mdev devices on the legacy container, as
the kernel refuses VFIO_UPDATE_VADDR for them since ef3a3f6a294b
("vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR").  We want CPR for
mdev vGPUs, so lift the blocker.

This needs a kernel which reverts ef3a3f6a294b, as UEK does.  Without
it the VFIO_UPDATE_VADDR extension check in vfio_cpr_supported() still
blocks CPR.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/vfio/container-legacy.c | 13 -------------
 include/hw/vfio/vfio-cpr.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
index e437eb0c741..285e594b048 100644
--- a/hw/vfio/container-legacy.c
+++ b/hw/vfio/container-legacy.c
@@ -988,20 +988,8 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
         goto device_put_exit;
     }
 
-    if (vbasedev->mdev) {
-        error_setg(&vbasedev->cpr.mdev_blocker,
-                   "CPR does not support vfio mdev %s", vbasedev->name);
-        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
-                                      MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
-                                      -1) < 0) {
-            goto hiod_unref_exit;
-        }
-    }
-
     return true;
 
-hiod_unref_exit:
-    object_unref(vbasedev->hiod);
 device_put_exit:
     vfio_device_put(vbasedev);
 group_put_exit:
@@ -1017,7 +1005,6 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
 
     vfio_device_unprepare(vbasedev);
 
-    migrate_del_blocker(&vbasedev->cpr.mdev_blocker);
     object_unref(vbasedev->hiod);
     vfio_device_put(vbasedev);
     vfio_group_put(group);
diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index fa1699857ca..a486b3b2578 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -32,7 +32,6 @@ typedef struct VFIOContainerCPR {
 } VFIOContainerCPR;
 
 typedef struct VFIODeviceCPR {
-    Error *mdev_blocker;
     Error *id_blocker;
     Error *mmap_blocker;
     uint32_t hwpt_id;

-- 
2.47.1

Re: [PATCH QEMU HCI-8.1 v4 01/31] VZ: migration/cpr-exec: fix Error leak in preserve_ram()

[no-reply]

> migrate_set_error() stores its own copy of the error, so free ours.
> 
> Fixes: a3745506daf9 ("VZ: migration/cpr-exec: mark saved RAM blocks with MADV_DOEXEC before execve")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=1

Re: [PATCH QEMU HCI-8.1 v4 02/31] migration/cpr-exec: restore RAM write access if exec fails

[no-reply]

> With -cpr-ram-rdonly the RAM blocks are made read-only right before
> execve().  If execve() fails, the VM resumes in the old QEMU with
> read-only RAM and crashes on the first write.  Make it writable again.
> 
> Fixes: 91da25f20471 ("migration/cpr-exec: add '-cpr-ram-rdonly' option to mark RAM blocks as RO #VSTOR-101579")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=2

Re: [PATCH QEMU HCI-8.1 v4 03/31] vfio/region: create BAR dma-bufs with O_CLOEXEC

[no-reply]

> The dma-buf fds are created without O_CLOEXEC, so they leak into any
> exec'ed program.  With cpr-exec, every update leaks one fd per mmapped
> BAR, as new QEMU creates its own.
> 
> Fixes: 96163e0dbcb4 ("hw/vfio/region: Create dmabuf for PCI BAR per region")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=3

Re: [PATCH QEMU HCI-8.1 v4 04/31] migration/cpr-exec: require a file or exec migration URI

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: cpr-exec URI allow-list rejects fd: and leaves CPR.rst wording stale
--

commit fbc8a913b5e7d85aa60e1735bde2730f6a4684ad
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr-exec: require a file or exec migration URI

This patch makes migration_channels_and_transport_compatible() reject any
migration URI in cpr-exec mode unless its transport is file or exec. The aim
is to stop a socket URI from sending a stream without guest RAM and then
exec'ing the stale cpr-exec command.

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -258,6 +258,14 @@ migration_channels_and_transport_compatible(MigrationAddress *addr,
>          return false;
>      }
>  
> +    if (migrate_mode() == MIG_MODE_CPR_EXEC &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_FILE &&
> +        addr->transport != MIGRATION_ADDRESS_TYPE_EXEC) {
> +        error_setg(errp,
> +                   "cpr-exec mode requires a file or exec migration URI");
> +        return false;
> +    }
> +

[Severity: Low]
Can this check reject an fd: URI that was previously accepted?

An fd: URI has transport MIGRATION_ADDRESS_TYPE_SOCKET with
SOCKET_ADDRESS_TYPE_FD. It therefore fails the new check together with tcp,
unix and vsock, even when the fd is a pipe or a regular file.

fd_start_outgoing_migration() still accepts such an fd. It only warns:

    if (!migration_fd_valid(fd)) {
        warn_report("fd: migration to a file is deprecated."
                    " Use file: instead.");
    }

So "getfd NAME" followed by "migrate fd:NAME" with mode=cpr-exec now fails
in migration_channels_and_transport_compatible() with "cpr-exec mode
requires a file or exec migration URI".

It is unclear whether fd: ever worked with cpr-exec. A named monitor fd is
not among the fds that cpr_exec_preserve_fds() preserves. The new QEMU would
need a fresh getfd and "migrate_incoming fd:NAME".

Is the exclusion of fd: backed by a file or pipe intentional? If so, could
the commit message say so?

The commit message says the check allows "the two types the CPR
documentation allows". docs/devel/migration/CPR.rst, in the cpr-exec Usage
section for the outgoing side, says:

    It is recommended that the URI be a file type, but one can use other
    types such as exec, provided the command captures all the data from the
    outgoing side, and provides all the data to the incoming side.

This wording is not exclusive, and it does not say whether fd: is allowed.
It still implies a looser rule than the code now enforces. Could CPR.rst be
updated to state the file or exec restriction explicitly?

>      return true;
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=4

Re: [PATCH QEMU HCI-8.1 v4 05/31] migration: close the CPR state if the outgoing channel fails to open

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: cpr-exec dup'd memfd and env var leak when channel open fails
--

commit 50d6efeaa5d85c80481be5ee50b6cbe211d76382
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration: close the CPR state if the outgoing channel fails to open

This patch adds a cpr_state_close() call to qmp_migrate_finish(). The call
runs when the outgoing channel (file or exec) fails to open. Its purpose is
to release the CPR state that qmp_migrate() saved earlier.

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2412,6 +2412,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          if (!resume_requested) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> +        cpr_state_close();
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;

[Severity: Medium]
The commit message says that without this change "the CPR state stays open:
the memfd for cpr-exec, the state file for cpr-transfer". Does this call
close everything for cpr-exec?

cpr_state_close() only does qemu_fclose(cpr_state_file). That closes the
original memfd.

In cpr-exec mode, cpr_state_save() calls cpr_exec_persist_state(). That
function creates a second reference to the memfd and sets an environment
variable:

migration/cpr-exec.c:cpr_exec_persist_state() {
    int mfd = dup(fioc->fd);
    ...
    qemu_clear_cloexec(mfd);
    ...
    g_setenv(CPR_EXEC_STATE_NAME, val, 1)
}

Nothing on this error path releases the dup'd fd or unsets
QEMU_CPR_EXEC_STATE. Only cpr_exec_unpersist_state() does that:

migration/cpr-exec.c:cpr_exec_unpersist_state() {
    g_unsetenv(CPR_EXEC_STATE_NAME);
    ...
    close(mfd);
}

It is called only from cpr_exec_notifier() on MIG_EVENT_PRECOPY_FAILED. This
event is raised from migration_cleanup(), from the postcopy failure path,
and from cpr_exec_cb() after a failed exec. None of these runs when
qmp_migrate_finish() fails to open the channel, because migration_cleanup()
doesn't run. migration_connect_set_error() only sets the state and the error.

Can a cpr-exec migrate to an exec: or file: URI that fails to open leak the
dup'd memfd? Each retry would leak one more fd, with CLOEXEC already cleared.
It would also leave a stale QEMU_CPR_EXEC_STATE variable pointing at that fd.
cpr_exec_has_state() would then keep returning true.

Should this path also call cpr_exec_unpersist_state(), or raise
MIG_EVENT_PRECOPY_FAILED, so the cpr-exec cleanup runs?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=5

Re: [PATCH QEMU HCI-8.1 v4 06/31] migration: Plug memory leaks after migrate_set_error()

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=6

Re: [PATCH QEMU HCI-8.1 v4 07/31] hw/vfio: Fix liveness check in vfio_connect_kvm_msi_virq()

[no-reply]

> 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
> [ ... ]
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=7

Re: [PATCH QEMU HCI-8.1 v4 08/31] vfio/pci: Reject invalid MSI-X Table and PBA BIR values

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=8

Re: [PATCH QEMU HCI-8.1 v4 09/31] vfio/pci: Fix information leak in vfio_rom_read()

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=9

Re: [PATCH QEMU HCI-8.1 v4 11/31] vfio/pci: Grow buffer in vfio_pci_host_match()

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=11

Re: [PATCH QEMU HCI-8.1 v4 10/31] vfio/pci: Initialize rom_read_failed in vfio_pci_load_rom()

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=10

Re: [PATCH QEMU HCI-8.1 v4 13/31] migration: ensure APIC is loaded prior to VFIO PCI devices

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=13

Re: [PATCH QEMU HCI-8.1 v4 12/31] q35: Fix migration of SMRAM state

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] q35: SMBASE lock reachable from reset state without 0xff/0x01 negotiation
--

commit 910f528c422bd7c6087d0502ae62fa2f2c8cd852
Author: Igor Mammedov <imammedo@redhat.com>

q35: Fix migration of SMRAM state

This commit stops mch_update_smbase_smram() from clearing the SMBASE
register when wmask is still at its reset value. wmask is now derived from
the register value instead, so a locked state survives migration. The
junk-write check in test_smram_smbase_lock() is removed to match.

Link: https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c

[ ... ]

> @@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
>      }
>  
>      if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> -            MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
>          return;
>      }
>  
>      /*
> -     * default/reset state, discard written value
> -     * which will disable SMRAM balackhole at SMBASE
> +     * reg value can come from register write/reset/migration source,
> +     * update wmask to be in sync with it regardless of source
>       */
> -    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> -        *reg = 0x00;
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        return;
>      }
> -
> -    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]
After mch_reset(), config[MCH_HOST_BRIDGE_F_SMBASE] is 0 and wmask is
0xff, with no 0xff/0x01 negotiation having taken place. Can a guest now
lock the register directly from this state?

For example, a guest config write of 0x02, 0x06 or 0xfe is stored by
pci_default_write_config() because wmask is 0xff:

mch_write_config()
  mch_update_smbase_smram()
    /* value is neither QUERY (0xff) nor IN_RAM (0x01) */
    if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
        *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
    }
    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
    memory_region_set_enabled(&mch->smbase_blackhole, lck);
    memory_region_set_enabled(&mch->smbase_window, lck);

The removed check used to zero such writes while wmask was 0xff, so this
path was not reachable before negotiation. Now smbase_blackhole and
smbase_window can be enabled without it, and non-lock junk values are
stored as written.

The commit message acknowledges that junk values are now accepted, but it
says the register can only be locked after reaching the reserved 0x1 or
0x2 values. Should it also say that a plain write of 0x02 locks the
register directly from the reset state?

[ ... ]

> 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 test that checked the old "junk is ignored before
negotiation" behaviour is removed, and nothing replaces it. Could
test_smram_smbase_lock() get coverage for the new pre-negotiation
behaviour?

The same applies to the wmask resync that the patch adds for migrated
register values. Is there a test that covers the case where the register
is 0x02 or 0x01 after post_load?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=12

Re: [PATCH QEMU HCI-8.1 v4 14/31] migration: Fix regression of passing error_fatal into vmstate_load_state()

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=14

Re: [PATCH QEMU HCI-8.1 v4 15/31] migration: vmstate_save_state_v: fix double error_setg

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() prepends to unset errp when put() returns positive
--

commit 900cd58df40307b6407d20cc283e62cccfe53256
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

migration: vmstate_save_state_v: fix double error_setg

This change avoids calling error_setg() twice on the same errp in
vmstate_save_state_v() when a nested vmstate_save_state() or
vmstate_save_state_v() call fails. It sets the error only in the leaf put()
branch, and the outer failure path now uses error_prepend() instead of
error_setg().

Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

[ ... ]

> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else {
>                      ret = inner_field->info->put(f, curr_elem, size,
>                                                   inner_field, vmdesc_loop);
> +                    if (ret < 0) {
> +                        error_setg(errp, "put failed");
> +                    }
>                  }

[Severity: Medium]
Can a put() callback return a positive value here? The new error_setg() is
guarded by ret < 0, but the check further down in vmstate_save_state_v() is
if (ret), so any non-zero return is treated as failure.

target/arm/machine.c:put_power() is an in-tree example. It returns 1 without
setting an Error when cpu->power_state is neither PSCI_ON nor PSCI_OFF:

    } else {
        return 1;
    }

In that case ret > 0, errp is left unset, and the code continues into the
failure path below.

>  
>                  written_bytes = qemu_file_transferred(f) - old_offset;

[ ... ]

> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> +                    error_prepend(errp, "Save of field %s/%s failed: ",
> +                                  vmsd->name, field->name);

Can this error_prepend() dereference a NULL *errp? error_vprepend() only
returns early when errp itself is NULL:

    if (!errp) {
        return;
    }
    ...
    g_string_append(newmsg, (*errp)->msg);

If the caller passed a non-NULL errp, for example the one passed from
savevm.c, and put() returned a positive value, then *errp is still NULL and
(*errp)->msg is a NULL dereference. The old code called error_setg() here, so
this case did not crash before the patch.

The same thing can happen at each nesting level, since a positive ret
propagates up from the VMS_STRUCT and VMS_VSTRUCT branches with no Error set.

Would it be better to use if (ret) rather than if (ret < 0) around the
error_setg() after put()? Alternatively, the outer path could call
error_setg() when *errp is not yet set.

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=15

Re: [PATCH QEMU HCI-8.1 v4 17/31] migration: Free cpr-transfer MigrationAddress along with gsource

[no-reply]

> From: Fabiano Rosas <farosas@suse.de>
> 
> When setting a callback on a Glib source and giving it a data pointer,
> it's natural to also provide the destructor for the data in question.
> 
> Since migrate_hup_add() already needs to clone the MigrationAddress
> when setting the qmp_migrate_finish_cb callback, also pass the
> qapi_free_MigrationAddress as the GDestroyNotify callback.
> 
> With this the address doesn't need to be freed at the callback body,
> making the management of that memory slightly simpler.
> 
> andrey.drobyshev:
>     This commit was cherry-picked from the upstream commit 6a887119ce18,
>     adapted for the hci-dev base: qmp_migrate_finish_cb() here still
>     closes the cpr state and deletes the HUP source itself (upstream had
>     already moved that into migration_cleanup()), so only the explicit
>     qapi_free_MigrationAddress() call goes away from it.
> 
> Cc: Mark Kanda <mark.kanda@oracle.com>
> Cc: Ben Chaney <bchaney@akamai.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-23-farosas@suse.de
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=17

Re: [PATCH QEMU HCI-8.1 v4 16/31] migration: yank: Move register instance earlier

[no-reply]

> 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()
> [ ... ]
> 
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=16

Re: [PATCH QEMU HCI-8.1 v4 18/31] migration: Move CPR HUP watch to cpr-transfer.c

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=18

Re: [PATCH QEMU HCI-8.1 v4 20/31] kvm: tolerate ioctls after kvm_close()

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] kvm: fake 0 return from kvm_vm_ioctl() after kvm_close() is taken as a valid fd
- [Low] kvm: kvm_vm_ioctl() fake success leaves kvm_set_irq() reading uninit event.status
- [Low] kvm: kvm_close() sentinel checks in ioctl wrappers are not synchronized with non-BQL callers
- [Low] kvm: ioctl wrappers return 0 for every ioctl after kvm_close(), faking success
--

commit 09dbc4fd8a681d8276f3d8eda8630fee15bf5809
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: tolerate ioctls after kvm_close()

This patch keeps kvm_state allocated after kvm_close() and makes
kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0 once the
descriptors have been closed. The aim is to stop a retired cpr-transfer
source from crashing on a late interrupt, such as the MSI-X delivery
triggered by net_cleanup().

> 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

[ ... ]

> @@ -644,7 +644,6 @@ void kvm_close(void)
>          close(kvm_state->fd);
>          kvm_state->fd = -1;
>      }
> -    kvm_state = NULL;
>  }

[Severity: Low]
Is kvm_close() synchronized with non-BQL callers of the ioctl wrappers?

kvm_close() is called from vfio_cpr_kvm_close_notifier() on
MIG_EVENT_PRECOPY_DONE, through vfio_kvm_device_close(). It does not call
accel_ioctl_inhibit_begin(). The fds are closed with plain stores and no
barrier:

    close(kvm_state->vmfd);
    kvm_state->vmfd = -1;
    close(kvm_state->fd);
    kvm_state->fd = -1;

The later patch "kvm: fix coalesced MMIO flush racing with kvm_close()"
only clears coalesced_mmio_ring and calls drain_call_rcu(). It does not
add any synchronization to the fd close or to the new checks.

> @@ -3371,6 +3370,11 @@ int kvm_ioctl(KVMState *s, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no KVM to talk to any more */
> +    if (s->fd == -1) {
> +        return 0;
> +    }
> +

[Severity: Low]
Can returning 0 for every system-level ioctl cause callers to act on a
success that never happened?

After kvm_close(), kvm_ioctl() fills in no output. For example,
kvm_check_extension() sees 0 for KVM_CHECK_EXTENSION and so reports every
capability as unsupported.

>      trace_kvm_ioctl(type, arg);
>      ret = ioctl(s->fd, type, arg);

[ ... ]

> @@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no VM to talk to any more */
> +    if (s->vmfd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Can this fake 0 be mistaken for a valid file descriptor?

Some kvm_vm_ioctl() requests return a new fd on success, and 0 is
indistinguishable from fd 0 (normally stdin). Two paths are affected after
kvm_close().

query_stats_cb() and query_stats_schemas_cb() only test for -1:

    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
    if (stats_fd == -1) {
        ...
    }
    query_stats(result, target, names, stats_fd, NULL, errp);
    close(stats_fd);

Both callbacks are still registered after kvm_close(), so a QMP query-stats
or query-stats-schemas would pass fd 0 to query_stats() and then call
close(0).

vfio_kvm_device_add_fd() in hw/vfio/helpers.c has the same problem:

    if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
        ...
    }

    vfio_kvm_device_fd = cd.fd;

cd is initialized with only .type, so cd.fd stays 0. If a VFIO device is
attached after kvm_close(), vfio_kvm_device_fd becomes 0, the next
ioctl(0, KVM_SET_DEVICE_ATTR) fails with ENOTTY, and vfio_kvm_device_close()
would later call close(0).

kvm_create_guest_memfd() and kvm_create_vcpu() check only fd < 0, so they
would also accept 0. I did not establish that either of them can run after
kvm_close() on the source.

The commit message justifies returning 0 only for interrupt and ioeventfd
callers. Were the ioctls that return an fd audited?

[Severity: Low]
Can kvm_set_irq() return an uninitialized value after kvm_close()?

kvm_set_irq() declares struct kvm_irq_level event; without an initializer
and writes only level and irq:

    event.level = level;
    event.irq = 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;

With the new guard, kvm_vm_ioctl() returns 0 without running the ioctl, so
the abort() check is skipped. On x86, irq_set_ioctl is KVM_IRQ_LINE_STATUS
when KVM_CAP_IRQ_INJECT_STATUS is present, so event.status is returned
uninitialized.

kvm_ioapic_set_irq() reaches this path after kvm_close(), because kvm_state
is no longer NULL:

    delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
    kvm_report_irq_delivered(delivered);

The legacy PIC path, kvm_pic_set_irq(), does the same. Should event be
zero-initialized, or should kvm_set_irq() return early when the VM is
closed?

The same shape applies to other getters such as KVM_GET_CLOCK,
KVM_GET_IRQCHIP and KVM_GET_DIRTY_LOG, which would report success without
filling the caller's buffer. I could not establish that any of them runs
after kvm_close().

[Severity: Low]
Is there a window between the new check and the ioctl() call?

s->vmfd is read once for the check and again for ioctl(), and the check sits
outside the accel_ioctl_begin()/accel_ioctl_end() region:

    if (s->vmfd == -1) {
        return 0;
    }

    trace_kvm_vm_ioctl(type, arg);
    accel_ioctl_begin();
    ret = ioctl(s->vmfd, type, arg);

If a non-BQL thread passes the check just as kvm_close() closes the fds, the
ioctl() can fail with EBADF. Callers such as kvm_set_irq() and
kvm_irqchip_commit_routes() treat that as fatal. If the fd number has been
reused, the ioctl lands on an unrelated fd.

vCPU threads are stopped by cpu_remove_sync(), and the dirty-ring reaper
takes the BQL. I could not show that every other non-BQL caller is
excluded. This is speculative, and the window is narrow.

>      trace_kvm_vm_ioctl(type, arg);
>      accel_ioctl_begin();
>      ret = ioctl(s->vmfd, type, arg);

[ ... ]

> @@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: the vCPU fds are gone */
> +    if (cpu->kvm_fd == -1) {
> +        return 0;
> +    }
> +

[Severity: Low]
Is cpu->kvm_fd == -1 a reliable marker for the closed state?

The guard returns success for any vCPU whose kvm_fd is -1, whether it was
closed or never created. kvm_fd is set to -1 only in kvm_close(), so the
guard is not tied to a dedicated closed state. Vector getters on this path
would also report success without filling the caller's buffer.

I did not establish that this path runs after kvm_close().

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=20

Re: [PATCH QEMU HCI-8.1 v4 19/31] migration: Fix crash on second migration when cancel early

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] migration: cancel in cpr-transfer HUP window sends FAILED without SETUP, can abort via VFIO notifier
- [Medium] migration: migration_cancel() asserts on mode that migrate-set-parameters can change
- [Low] migration: qmp_migrate_finish_cb() comment refers to a nonexistent redundant destroy
--

commit e43a55e4327f60b3f762f7e2bead9f8ce8624d43
Author: Peter Xu <peterx@redhat.com>

migration: Fix crash on second migration when cancel early

This commit makes migration_cancel() call migration_cleanup() when the
cpr-transfer HUP gsource is still pending. It also detaches hup_source
at entry to qmp_migrate_finish_cb(), and adds some assertions. This
fixes a crash when a cancelled early-setup migration is retried.

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 b7db5c94b0d9..3732751e66c7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -1661,14 +1661,19 @@ void migration_cancel(void)
>      }
>  
>      /*
> -     * If qmp_migrate_finish has not been called, then there is no path that
> -     * will complete the cancellation.  Do it now.
> +     * This is cpr-transfer specific processing.
> +     *
> +     * If this is true, it means cpr-transfer migration is waiting for the
> +     * destination to send HUP event on CPR channel to continue the next
> +     * phase.  If so, do the cleanup proactively to avoid get stuck in
> +     * CANCELLING state.
>       */
> -    if (setup && !s->to_dst_file) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
> -                          MIGRATION_STATUS_CANCELLED);
> -        cpr_state_close();
> -        cpr_transfer_source_destroy(s);
> +    if (cpr_transfer_source_active(s)) {
> +        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);
> +        assert(setup && !s->to_dst_file);
> +        migration_cleanup(s);
> +        /* Now all things should have been released */
> +        assert(!cpr_transfer_source_active(s));
>      }
>  }

[Severity: High]
Can this migration_cleanup() call abort QEMU through a VFIO notifier?

In the cpr-transfer HUP-wait window, qmp_migrate() has only called
cpr_transfer_add_hup_watch() and returned. migration_connect() has not
run, so migration_call_notifiers(MIG_EVENT_PRECOPY_SETUP) has not been
delivered. The SETUP handler vfio_cpr_pci_notifier(), which disables the
MSI/INTx irqfds, has therefore not run.

If migrate_cancel arrives in this window, migration_cancel() calls
migration_cleanup(), which ends with:

    type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
                                     MIG_EVENT_PRECOPY_DONE;
    migration_call_notifiers(type, NULL);

For MIG_EVENT_PRECOPY_FAILED, vfio_cpr_pci_notifier() calls
vfio_cpr_set_msi_virq(vdev, errp, true). That re-adds irqfds which were
never removed. If KVM_IRQFD assign rejects the duplicate, the notifier
returns non-zero. I have not verified the kernel behaviour for a
duplicate assign.

migration_call_notifiers() then reaches:

    ret = notifier->notify(notifier, &e, errp);
    if (ret) {
        assert(type == MIG_EVENT_PRECOPY_SETUP);

That assert would fire for a FAILED event and abort the running source
QEMU. The old code in this path sent no notifier events.

Is it intended that FAILED is delivered here without a preceding SETUP,
and is vfio_cpr_pci_notifier() expected to tolerate that? The commit
message does not mention this side effect.

[Severity: Medium]
Can assert(migrate_mode() == MIG_MODE_CPR_TRANSFER) fire here?

migrate_mode() reads the live s->parameters.mode. migrate_params_apply()
applies has_mode with no check for a running migration:

    if (params->has_mode) {
        s->parameters.mode = params->mode;
    }

qmp_migrate() samples the mode only once, when it calls
cpr_transfer_add_hup_watch(), and does not record it. Consider this
sequence:

    qmp_migrate()                      /* cpr-transfer, hup_source set */
    migrate-set-parameters mode=normal
    migrate_cancel  (or migration_shutdown() at exit)
      migration_cancel()
        cpr_transfer_source_active(s)  /* true */
        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER)  /* fails */

That would abort QEMU in a case where the old code just cancelled and
cleaned up. The cleanup itself does not depend on the mode. Is this
assertion needed?

> @@ -2266,11 +2271,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 new comment before return G_SOURCE_REMOVE says
"This is redundant as we do cpr_transfer_source_destroy() at the entry".
No second cpr_transfer_source_destroy() call remains in
qmp_migrate_finish_cb(), because the patch removes the trailing one.

The comment seems to describe code that is not present in this backport.
Could it be reworded or dropped?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=19

Re: [PATCH QEMU HCI-8.1 v4 21/31] kvm: fix coalesced MMIO flush racing with kvm_close()

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] kvm: coalesced_mmio_ring cleared with plain store, read twice without READ_ONCE
--

commit 3b43e13d8cef028cfae52bf9fd0d5ec297cf51f8
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: fix coalesced MMIO flush racing with kvm_close()

This patch clears kvm_state->coalesced_mmio_ring and calls drain_call_rcu() at the start of kvm_close(). The aim is to stop the RCU thread from flushing the coalesced MMIO ring after cpu_remove_sync() has unmapped it.

> 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

[ ... ]

> @@ -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;

[Severity: Low]
Is this plain store safe against the lockless reader in the RCU thread? The reader, kvm_flush_coalesced_mmio_buffer(), loads the same pointer twice with plain loads:

    if (s->coalesced_mmio_ring) {
        struct kvm_coalesced_mmio_ring *ring = s->coalesced_mmio_ring;
        while (ring->first != ring->last) {

The RCU thread runs flatview_destroy() -> memory_region_finalize() -> memory_region_transaction_begin() -> kvm_flush_coalesced_mmio_buffer() with no lock shared with kvm_close().

If the NULL store lands between the check and the reload, or if the compiler reloads the field, ring becomes NULL and ring->first faults. That is the same SIGSEGV this patch is meant to fix.

In practice no call or barrier sits between the two loads, so the compiler will most likely merge them. That makes the window small, but it is still a data race under the C memory model.

Would it be better to use qatomic_set() here in kvm_close(), and a single qatomic_read() into a local variable in kvm_flush_coalesced_mmio_buffer()?

drain_call_rcu() below closes the larger window, where a flush is already past the load. It queues its callback behind any in-flight flatview_destroy(), so that flush finishes before cpu_remove_sync() unmaps the ring. It does not close the double-load window described above.

> +    drain_call_rcu();
> +
>      CPU_FOREACH(cpu) {
>          cpu_remove_sync(cpu);
>          close(cpu->kvm_fd);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=21

Re: [PATCH QEMU HCI-8.1 v4 22/31] migration/cpr: add cpr_is_in_progress()

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration/cpr: cpr_is_in_progress() false on cpr-transfer source after HUP
--

commit 6f0604877a0fdc026e4268cdb035a9a9897fd0b4
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr: add cpr_is_in_progress()

This adds cpr_is_in_progress(). It returns true on the target until the
incoming migration is done. On the source it returns true only while the
CPR state file is open.

> diff --git a/migration/cpr.c b/migration/cpr.c
> --- a/migration/cpr.c
> +++ b/migration/cpr.c

[ ... ]

> +bool cpr_is_in_progress(void)
> +{
> +    /* The incoming mode on the target, the 'mode' parameter on the source */
> +    MigMode mode = migrate_mode();
> +
> +    if (mode != MIG_MODE_CPR_TRANSFER && mode != MIG_MODE_CPR_EXEC) {
> +        return false;
> +    }
> +
> +    /* Target: the incoming mode is set until the incoming migration is done */
> +    if (cpr_is_incoming()) {
> +        return true;
> +    }
> +
> +    /*
> +     * Source: the parameter alone means nothing, it may just have been set.
> +     * An outgoing CPR migration has the CPR state open, from
> +     * cpr_state_save() to cpr_state_close().
> +     */
> +    return cpr_state_file != NULL;

[Severity: Medium]
Can this return false on the source for almost the whole outgoing
migration in cpr-transfer mode?

The source-side check follows the lifetime of cpr_state_file, not the
lifetime of the outgoing migration. In cpr-transfer mode the file is
closed early:

qmp_migrate()
  cpr_state_save()                 /* sets cpr_state_file */
  cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr)

The target then closes the CPR socket, and the source runs:

qmp_migrate_finish_cb()
    ...
    qmp_migrate_finish(addr, NULL);   /* only starts the outgoing connection */

    cpr_state_close();                /* cpr_state_file = NULL */

After this point the RAM transfer, vm_stop, device state save and
completion all still run. During that time migrate_mode() is
MIG_MODE_CPR_TRANSFER, cpr_is_incoming() is false on the source, and
cpr_state_file is NULL. So cpr_is_in_progress() returns false.

This contradicts the commit message ("on the source only while the
outgoing one runs") and the comment above ("from cpr_state_save() to
cpr_state_close()"). The cpr-exec case is not affected, because there the
state is closed only in migration_cleanup().

The function has no callers in this patch. However, the later patches in
the series add source-side callers that run during the migration:
vfio_is_active(), the vfio VM state change handlers, save_prepare in
hw/vfio/migration.c, and the dirty-tracking start in hw/vfio/listener.c.
In cpr-transfer mode these would see false, so VFIO device state
migration, device stop/resume and dirty tracking would not be skipped.

A VM state change handler that fires at vm_stop during completion is the
use case the commit message names.

Could the source-side check use state that lasts for the whole outgoing
migration instead of cpr_state_file?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=22

Re: [PATCH QEMU HCI-8.1 v4 24/31] vfio/listener: skip dirty page tracking during CPR

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio/listener: CPR gate in log_global_start() is already false for cpr-transfer
- [Medium] vfio: skipping log_global_start during CPR leaves VFIO tracking off while global flag stays set
--

commit d3cf7e64855bb765e2ec55a6878e79a36d8d4b81
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/listener: skip dirty page tracking during CPR

This patch makes vfio_listener_log_global_start() return early without
starting VFIO dirty tracking while cpr_is_in_progress() is true. The goal
is to avoid starting container or device (DMA logging) dirty tracking
during cpr-transfer and cpr-exec migrations. Stopping stays unconditional.

> diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
> index afcf518f567f..1270b9d5f3aa 100644
> --- a/hw/vfio/listener.c
> +++ b/hw/vfio/listener.c

[ ... ]

> @@ -1012,6 +1013,11 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>                                               listener);
>      bool ret;
>  
> +    /* Preserved devices keep DMA-ing into preserved RAM, nothing to track */
> +    if (cpr_is_in_progress()) {
> +        return true;
> +    }
> +

[Severity: Medium]
Is this check effective for cpr-transfer on the source?

On the source, cpr_is_in_progress() ends with:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate_finish_cb() closes the state file right
after starting the outgoing connection:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
}

The dirty log is started later, from the migration thread, which takes the
BQL for setup:

migration_thread()
  bql_lock();
  ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
  ...
    ram_init_bitmaps()
      memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp)
        vfio_listener_log_global_start()

Since the migration thread cannot take the BQL until the main loop
callback has returned, cpr_state_file already appears to be NULL by the
time vfio_listener_log_global_start() runs. cpr_is_incoming() is false on
the source, so cpr_is_in_progress() would return false and dirty tracking
would still be started.

The comment in cpr_is_in_progress() describes the window as "from
cpr_state_save() to cpr_state_close()", which does not appear to cover
migration thread setup for cpr-transfer. The commit message says the patch
covers both cpr-transfer and cpr-exec, but the cpr-exec case seems to be
the only one where the state file stays open until migration_cleanup().

Could the gate use a predicate that remains true through setup for
cpr-transfer as well? I did not find a later change in the series that
alters cpr_is_in_progress(), the placement of cpr_state_close(), or this
listener.

[Severity: Medium]
Does returning true here leave VFIO tracking off while the global dirty
tracking flag stays set?

memory_global_dirty_log_start() only calls the listeners when the first
flag is set:

system/memory.c:memory_global_dirty_log_start() {
    old_flags = global_dirty_tracking;
    global_dirty_tracking |= flags;
    ...
    if (!old_flags) {
        if (!memory_global_dirty_log_do_start(errp)) {
    ...
}

Consider this sequence:

1. A CPR migration is the first dirty log user.
   vfio_listener_log_global_start() returns early, so VFIO tracking is
   not started.
2. calc-dirty-rate calls memory_global_dirty_log_start() with
   GLOBAL_DIRTY_DIRTY_RATE.  old_flags is non-zero, so no listener is
   called.
3. The CPR migration fails or is cancelled and drops its flag.
   global_dirty_tracking stays non-zero, so memory_global_dirty_log_do_stop()
   does not call log_global_stop.
4. A normal migration then starts while calc-dirty-rate is still active.
   old_flags is non-zero, so the VFIO start hook is not called.

In that state vfio_log_sync_needed() returns false:

hw/vfio/listener.c:vfio_log_sync_needed() {
    ...
    if (!vfio_container_dirty_tracking_is_started(bcontainer)) {
        return false;
    }
    ...
}

so DMA writes by VFIO devices would not be reported to later dirty log
users, including a subsequent live migration.

The commit message acknowledges this ("Known gap ... VFIO tracking stays
off until QEMU restarts"), and I did not find a later patch in the series
that addresses it. Would it be possible to make the decision in the
migration layer, or with a per-listener opt-out, so that the global flag
and the VFIO device state stay consistent?

>      if (vfio_container_devices_dirty_tracking_is_supported(bcontainer)) {
>          ret = vfio_devices_dma_logging_start(bcontainer, errp);
>      } else {

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=24

Re: [PATCH QEMU HCI-8.1 v4 23/31] vfio/migration: don't migrate device state during CPR

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio/migration: cpr_is_in_progress() is false on cpr-transfer source, gates ineffective
- [Medium] vfio/migration: cached device_state goes stale when a paused VM is CPR-transferred
--

commit ef9b082e80c829a57ff650c4bacfd6d71dd9e216
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: don't migrate device state during CPR

This patch makes the VFIO migration handlers skip saving and loading
device state, and skip device state changes on VM stop/start, while
cpr_is_in_progress() is true. It also resets multifd_transfer in
vfio_multifd_cleanup().

> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c

[ ... ]

> @@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /*
> +     * .is_active doesn't cover the non-iterable stage, so the section is
> +     * still sent: leave it empty, or new QEMU would load the config into
> +     * the preserved device and re-program its MSI-X.
> +     */
> +    if (cpr_is_in_progress()) {
> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> +        return;
> +    }
> +

[ ... ]

> +/*
> + * cpr-transfer and cpr-exec hand the running device over to the new QEMU
> + * as is: its fds, mappings and interrupts are preserved and it never stops.
> + * Device state migration must stay out of it, else the device is stopped,
> + * its state saved and then resumed into the very same device.
> + */
> +static bool vfio_is_active(void *opaque)
> +{
> +    return !cpr_is_in_progress();
> +}
> +

[Severity: High]
Is cpr_is_in_progress() actually true on the cpr-transfer source while
these handlers run?

On the source it ends with:

migration/cpr.c:cpr_is_in_progress() {
    ...
    return cpr_state_file != NULL;
}

For cpr-transfer, qmp_migrate() calls cpr_state_save(), which sets
cpr_state_file. When the target closes the CPR socket,
qmp_migrate_finish_cb() runs:

migration/migration.c:qmp_migrate_finish_cb() {
    ...
    qmp_migrate_finish(addr, NULL);

    cpr_state_close();
}

qmp_migrate_finish() only starts the outgoing connection, and
cpr_state_close() then sets cpr_state_file to NULL. The migration thread,
savevm setup, the VM stop at switchover and vfio_save_state() all run after
that.

At that point migrate_mode() is CPR_TRANSFER, cpr_is_incoming() is false and
cpr_state_file is NULL, so cpr_is_in_progress() returns false on the source.

If so, on the source vfio_is_active() returns true, so save_setup, iterate
and complete still run. The vmstate-change notifiers still move the device
to RUNNING_P2P and then STOP or STOP_COPY. vfio_save_state() still emits
the real config or data, and the switchover ACK is still required.

On the target, cpr_is_incoming() is true until
migration_incoming_state_destroy(), so vfio_is_active() is false there and
load_setup is skipped. That means no RESUMING state, and data_fd stays -1.

The loader does not consult is_active, so vfio_load_state() can still
receive the source's real DEV_SETUP, DATA or CONFIG sections. Could those
fail on data_fd == -1, or re-program MSI-X, which is what this patch is
meant to avoid?

cpr-exec looks unaffected, since there cpr_state_close() only runs in
migration_cleanup(). Does the cpr-transfer source need a different
predicate, or should the cpr_state_close() call be moved?

> @@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +
>      if (running) {
>          new_state = VFIO_DEVICE_STATE_RUNNING;
>      } else {

[Severity: Medium]
Can the cached migration->device_state go stale when a paused VM is
CPR-transferred?

On the paused source, vfio_vmstate_change(running=false) has already put
the kernel device into STOP before CPR starts. The target creates a fresh
VFIOMigration in vfio_migration_init():

    migration->device_state = VFIO_DEVICE_STATE_RUNNING;

Nothing queries the kernel for the real state, because the device fd is
inherited as is.

With this early return, the target's notifiers do nothing while
cpr_is_in_progress() is true. In process_incoming_migration_bh(), a paused
source takes this path:

    } else {
        runstate_set(RUN_STATE_PAUSED);
    }

so vm_start() is not called and no VFIO notifier runs. After that,
migration_incoming_state_destroy() resets the incoming mode and
cpr_is_in_progress() becomes false.

A later 'cont' then calls vfio_vmstate_change(running=true) with
new_state = RUNNING, and vfio_migration_set_state() hits:

    if (new_state == migration->device_state) {
        return 0;
    }

so the VFIO_DEVICE_FEATURE ioctl is never issued. Would the kernel device
then stay in STOP while QEMU believes it is RUNNING?

Should the cached device_state be reconciled with the real device state
when CPR skips the transition on the target?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=23

Re: [PATCH QEMU HCI-8.1 v4 25/31] VZ: vfio/cpr: don't close KVM on cpr-exec

[no-reply]

> With VFIO devices, KVM is closed on the source when the migration
> completes, so that the new QEMU can create its own KVM VM with the same
> devices.  That is done from a migration notifier, which for cpr-exec
> runs before cpr_exec_cb().  But cpr_exec_cb() may still give up, when
> the memory can't be preserved or execve() fails, and resume the VM in
> the old QEMU.  A VM without KVM can't resume: kvm_close() removes the
> vCPU threads, and the first KVM ioctl of vm_start() aborts QEMU
> (KVM_SET_CLOCK in kvmclock_vm_state_change()).
> 
> cpr-exec doesn't need it anyway: the KVM fds are close-on-exec, so the
> kernel closes them during execve(), before the new QEMU runs, and
> kvm_vm_release() tears down the irqfds then.  Keep the notifier for
> cpr-transfer only, where the source process lives on.
> 
> Tested on the vfio-net rig: cpr-exec 5/5; a cpr-exec aborted in
> preserve_ram() and one with a bogus cpr-exec-command both left the VM
> running with no lost pings, and a later cpr-exec completed.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=25

Re: [PATCH QEMU HCI-8.1 v4 26/31] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=26

Re: [PATCH QEMU HCI-8.1 v4 28/31] VZ: migration/cpr-exec: don't exec if a vhost-net device can't be released

[no-reply]

> vhost_net_cpr_reset_owners() only warns when VHOST_RESET_OWNER fails for
> a device, and the exec goes on.  But the new QEMU can't take such a
> device: it is still owned by the old mm, so VHOST_SET_OWNER fails with
> EBUSY, and the guest is left without network after the update.
> 
> Stop at the first device that can't be released and abort the cpr-exec
> instead.  The VM goes on in the old QEMU, with the devices released so
> far taken back.  vhost-vsock already does that in its pre_save.
> 
> Fixes: 1278e4830052 ("VZ: migration/cpr-exec: vhost-net: send RESET_OWNER before cpr-exec")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=28

Re: [PATCH QEMU HCI-8.1 v4 27/31] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-net: cpr-exec failure path does not re-apply busyloop timeout (poll-us)
--

commit 3c0903d7d2ea3ce9e525bbd0a16e4a5b7320f49d
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

This patch makes the cpr-exec failure path take the kernel vhost-net
devices back with VHOST_SET_OWNER after they were released with
VHOST_RESET_OWNER. It also re-applies the vring call and err fds, and
marks released devices with a new cpr_owner_reset flag.

> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c

[ ... ]

> +/*
> + * Take the ownership back when the exec fails and the VM goes on in the
> + * old QEMU.  VHOST_RESET_OWNER has reset the kernel device, but the devices
> + * are stopped, and vhost_net_start() sets everything up again when the VM
> + * resumes.
> + */
> +void vhost_net_cpr_set_owners(void)
> +{

[ ... ]

> +            /* RESET_OWNER also dropped the vring fds set by vhost_dev_init() */
> +            for (i = 0; i < dev->nvqs; i++) {
> +                const VhostOps *ops = dev->vhost_ops;
> +                struct vhost_virtqueue *vq = &dev->vqs[i];
> +                struct vhost_vring_file file = {
> +                    .index = ops->vhost_get_vq_index(dev, dev->vq_index + i),
> +                    .fd = event_notifier_get_wfd(&vq->masked_notifier),
> +                };
> +
> +                r = ops->vhost_set_vring_call(dev, &file);
> +                if (r == 0 && ops->vhost_set_vring_err) {
> +                    file.fd = event_notifier_get_fd(&vq->error_notifier);
> +                    r = ops->vhost_set_vring_err(dev, &file);
> +                }

[Severity: Low]
Is the busyloop timeout also lost here?

vhost_dev_init() programs it only once at setup time:

hw/virtio/vhost.c:vhost_dev_init() {
    ...
    if (busyloop_timeout) {
        for (i = 0; i < hdev->nvqs; ++i) {
            r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
                                                     busyloop_timeout);
    ...
}

The value comes from the user-configurable poll-us tap option:

net/tap.c:net_init_tap_one() {
    ...
    if (tap->has_poll_us) {
        options.busyloop_timeout = tap->poll_us;
    }
    ...
}

The value is not stored in struct vhost_dev. As far as I can see, nothing in the vhost_dev_start()/vhost_net_start() path re-applies it.

So with tap,vhost=on,poll-us=N, after cpr_exec_cb() calls vhost_net_cpr_reset_owners() and execvp() fails, vhost_net_cpr_set_owners() restores only the vring call and err fds. Could the poll-us setting then be silently dropped on the old QEMU's device?

If so, the comment "vhost_net_start() sets everything up again when the VM resumes" does not hold for this setting.

I did not verify in the kernel whether VHOST_RESET_OWNER actually clears vq->busyloop_timeout, so this depends on that behavior. Could the timeout be saved and replayed here, or the comment and commit message be adjusted?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=27

Re: [PATCH QEMU HCI-8.1 v4 30/31] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

[no-reply]

> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=30

Re: [PATCH QEMU HCI-8.1 v4 31/31] VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio: mdev CPR blocker dropped; remaining UPDATE_VADDR check only covers the first group
- [Low] vfio: CPR mdev blocker removed for all mdev types, including vfio-ap and vfio-ccw
--

commit 7cbd800afda6ddbe0b7b6ef4fa513b3145de2ff4
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: vfio/container: lift the CPR blocker for mdev devices #VSTOR-103860

This patch removes the per-device CPR migration blocker for mdev devices
in vfio_legacy_attach_device() and the mdev_blocker field in
VFIODeviceCPR. The aim is to allow cpr-transfer and cpr-exec for mdev
vGPUs. The commit message says this needs a kernel that reverts
ef3a3f6a294b.

> diff --git a/hw/vfio/container-legacy.c b/hw/vfio/container-legacy.c
> --- a/hw/vfio/container-legacy.c
> +++ b/hw/vfio/container-legacy.c
> @@ -988,20 +988,8 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>          goto device_put_exit;
>      }
>  
> -    if (vbasedev->mdev) {
> -        error_setg(&vbasedev->cpr.mdev_blocker,
> -                   "CPR does not support vfio mdev %s", vbasedev->name);
> -        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
> -                                      MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
> -                                      -1) < 0) {
> -            goto hiod_unref_exit;
> -        }
> -    }
> -

[Severity: Medium]
Is the commit message accurate where it says "Without it the
VFIO_UPDATE_VADDR extension check in vfio_cpr_supported() still blocks
CPR"?

That check is container-wide.  It runs once, from
vfio_legacy_cpr_register_container(), right after the first group of an
address space has been attached:

    if (!vfio_cpr_supported(container, cpr_blocker)) {
        return migrate_add_blocker_modes(cpr_blocker, errp,
                                         MIG_MODE_CPR_TRANSFER,
                                         MIG_MODE_CPR_EXEC, -1) == 0;
    }

When a later group joins an existing container, vfio_container_connect()
returns early:

    QLIST_FOREACH(bcontainer, &space->containers, next) {
        container = VFIO_IOMMU_LEGACY(bcontainer);
        if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
            return vfio_container_group_add(container, group, errp);
        }
    }

This path never calls vfio_cpr_supported() again.

Consider a passthrough group that opens the container first, followed by
an mdev group that joins it. On a kernel that still carries ef3a3f6a294b,
the extension check passes before the mdev attaches. The removed
mdev_blocker was the only guard for that case.

Can CPR then be refused only late, from vfio_container_pre_save()
through vfio_dma_unmap_vaddr_all(), instead of being blocked up front?

Does the patch also leave QEMU silently dependent on a patched kernel,
with no runtime probe, opt-in property or per-device blocker? That also
differs from upstream commit dac0dd68d9b.

[Severity: Low]
Is it intended that this blocker is removed for every mdev type? The
removed check was keyed only on vbasedev->mdev, so it covered all
mdev-backed devices attached through vfio_legacy_attach_device().

vfio-ap (vfio_ap_realize() in hw/vfio/ap.c) and vfio-ccw
(vfio_ccw_realize() in hw/vfio/ccw.c) both set:

    vbasedev->mdev = true;

so they also lose the blocker. vfio-pci does the same when
vfio_device_is_mdev() matches.

The commit message gives the reason only for mdev vGPUs. The only
remaining gate is the VFIO_CHECK_EXTENSION(VFIO_UPDATE_VADDR) test in
vfio_cpr_supported(), which does not look at device type.

Has the VADDR-update flow been checked for vfio-ap and vfio-ccw? If not,
should the blocker be kept for those device types?

>      return true;
>  
> -hiod_unref_exit:
> -    object_unref(vbasedev->hiod);
>  device_put_exit:
>      vfio_device_put(vbasedev);
>  group_put_exit:

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=31

Re: [PATCH QEMU HCI-8.1 v4 29/31] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[no-reply]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] vfio: vfio_device_attach() now fails hotplug while migration/snapshot runs
- [Medium] vfio: unconditional cpr-transfer blocker in vfio_device_attach() for all VFIO devices
- [Low] vfio/region: vfio_region_cpr_find() aborts on mmap size mismatch via assert()
- [Low] vfio: BAR mappings joined to CPR ramblock list hit abort()-on-failure mprotect in cpr-ram-rdonly mode
- [Low] vfio: preserved display-region mapping not released if never remapped after cpr-exec
--

commit af431a92f0ea91854f19f143a605e436ef6246ae
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

This patch saves each VFIO BAR mmap in the CPR ramblock list so that
cpr_exec_cb() marks it with MADV_DOEXEC. New QEMU then reuses the saved
mapping instead of calling mmap() again. It also adds a blocker that stops
cpr-transfer for all VFIO devices.

> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -464,12 +464,32 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                               TYPE_VFIO_IOMMU_IOMMUFD :
>                               TYPE_VFIO_IOMMU_LEGACY;
>  
> -    return vfio_device_attach_by_iommu_type(iommu_type, name, vbasedev,
> -                                            as, errp);
> +    /*
> +     * Only cpr-exec keeps the address space and so the region mappings,
> +     * see vfio_region_mmap().  A cpr-transfer target would have to map the
> +     * regions anew, which some drivers (NVIDIA vGPU) refuse.
> +     */
> +    error_setg(&vbasedev->cpr.mmap_blocker,
> +               "cpr-transfer is not supported with VFIO devices; "
> +               "use the cpr-exec mode");

[Severity: Medium]
The blocker is installed here for every VFIO device, before no_mmap, the
device type or the region state is looked at. Does this also disable
cpr-transfer for devices that never map a BAR (no_mmap=on, vfio-ap,
vfio-ccw) and for drivers that accept a re-mmap?

The stated reason is the NVIDIA vGPU re-mmap problem, which only exists
once vfio_region_cpr_save() has actually saved a mapping. Could the
blocker be added only when a BAR mapping is saved, or be made opt-in per
device?

Also, docs/devel/migration/CPR.rst still says in the cpr-transfer mode
section that VFIO devices are supported:

    This mode supports VFIO devices provided the user first puts the

Should the documentation be updated along with this change?

> +    if (migrate_add_blocker_modes(&vbasedev->cpr.mmap_blocker, errp,
> +                                  MIG_MODE_CPR_TRANSFER, -1)) {
> +        return false;
> +    }

[Severity: Medium]
Can this make vfio_device_attach() fail during a migration or snapshot,
whatever the CPR mode? migrate_add_blocker_modes() runs is_busy() no
matter which mode mask is passed:

vfio_device_attach()
  migrate_add_blocker_modes()
    is_busy()
      if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
          error_propagate_prepend(errp, *reasonp,
              "disallowing migration blocker (migration/snapshot in progress) for: ");
          ...
          return true;
      }

This returns -EBUSY, so realizing or hotplugging any VFIO device (PCI,
AP, CCW; legacy or iommufd) while a migration is running or
RUN_STATE_SAVE_VM is set would now fail. Is that intended?

The later patches in the series do not change this call.

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +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);
> +    }

[Severity: Low]
If the preserved length differs from the size that new QEMU computes for
this mmap slot, this aborts after old QEMU has already exec'd. Could this
report an error with error_report() and exit(1), as the ramblock reuse
path in system/physmem.c does under cpr_exec_is_incoming()?

Only the length is compared here. Offset, prot and fd_offset are not
validated, so a layout change that keeps the same length would silently
reuse a mapping of the wrong window. Is a check of those intended?

[ ... ]

> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    void *addr = region->mmaps[index].mmap;
> +    size_t size = region->mmaps[index].size;
> +
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

[Severity: Low]
This puts device BAR mappings into the list that migration/cpr-exec.c
walks. vfio_region_mmap() can map a BAR PROT_READ only, depending on
region->flags. With the debug-only -cpr-ram-rdonly option, cpr_exec_cb()
calls mark_ram_rdonly() on every entry. On the exec-failure path, and when
cpr_exec_is_incoming() && cpr_ram_rdonly, mark_ram_rw() requests RW on
every entry:

    if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
        error_report("qemu_mprotect_rw() failed on memory region %s:"
        ...
        abort();
    }

Can this abort() QEMU if a driver refuses mprotect on a shared PFN
mapping, for example by clearing VM_MAYWRITE? Would it also make a
read-only BAR mapping writable?

I could not confirm the driver behaviour from the repository, so this is
a question about the interaction.

[ ... ]

> @@ -427,6 +470,8 @@ void vfio_region_finalize(VFIORegion *region)
>  
>      for (i = 0; i < region->nr_mmaps; i++) {
>          if (region->mmaps[i].mmap) {
> +            g_autofree char *cpr_name = vfio_region_cpr_name(region, i);
> +            cpr_delete_ramblock(cpr_name);
>              munmap(region->mmaps[i].mmap, region->mmaps[i].size);
>          }
>      }

[Severity: Low]
Can the preserved entry for the lazily mapped display region be leaked
here? After cpr-exec, if new QEMU has not yet set up and mapped the
display region, vfio_region_setup() and vfio_region_mmap() are never
called for it. region->mem and region->mmaps[i].mmap stay NULL:

hw/vfio/display.c:vfio_display_region_exit() {
    if (!dpy->region.buffer.size) {
        return;
    }

    vfio_region_exit(&dpy->region.buffer);
    vfio_region_finalize(&dpy->region.buffer);
}

Here the early return, and the early return in vfio_region_finalize() when
!region->mem, skip the cpr_delete_ramblock() call. The entry then stays
in the list, so cpr_exec_cb() marks the VMA with MADV_DOEXEC again on the
next cpr-exec. The VMA also keeps a reference on the device file.

A re-added device with the same name could also pick the stale mapping up
through vfio_region_cpr_find(). Should the preserved entry be released
when the region is never remapped?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=29