[PATCH 0/3] VFIO multifd device state transfer patches for QEMU 10.1

[PATCH 0/3] VFIO multifd device state transfer patches for QEMU 10.1

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

When VFIO multifd device state transfer support was merged in QEMU 10.0
some of patches were separated for the future QEMU release after the
spring cleanup.

Since QEMU 10.1 code freeze is getting closer let's have them reviewed
now.

The most important patch from this patch set is the one adding
"x-migration-load-config-after-iter" VFIO property since it is a
correctness fix for an ARM64 platform idiosyncrasy with respect
to VFIO load dependency on interrupt controller being loaded first.

I've also CCed the ARM maintainer and qemu-arm as suggested previously
by Cédric.

Maciej S. Szmigiero (3):
  vfio/migration: Max in-flight VFIO device state buffer count limit
  vfio/migration: Add x-migration-load-config-after-iter VFIO property
  vfio/migration: Add also max in-flight VFIO device state buffers size
    limit

 docs/devel/migration/vfio.rst     |  21 +++++
 hw/core/machine.c                 |   1 +
 hw/vfio/migration-multifd.c       | 125 +++++++++++++++++++++++++++++-
 hw/vfio/migration-multifd.h       |   3 +
 hw/vfio/migration.c               |  10 ++-
 hw/vfio/pci.c                     |  27 +++++++
 hw/vfio/vfio-migration-internal.h |   1 +
 include/hw/vfio/vfio-device.h     |   3 +
 8 files changed, 188 insertions(+), 3 deletions(-)

[PATCH 1/3] vfio/migration: Max in-flight VFIO device state buffer count limit

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

Allow capping the maximum count of in-flight VFIO device state buffers
queued at the destination, otherwise a malicious QEMU source could
theoretically cause the target QEMU to allocate unlimited amounts of memory
for buffers-in-flight.

Since this is not expected to be a realistic threat in most of VFIO live
migration use cases and the right value depends on the particular setup
disable the limit by default by setting it to UINT64_MAX.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 docs/devel/migration/vfio.rst | 13 +++++++++++++
 hw/vfio/migration-multifd.c   | 16 ++++++++++++++++
 hw/vfio/pci.c                 |  9 +++++++++
 include/hw/vfio/vfio-device.h |  1 +
 4 files changed, 39 insertions(+)

diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
index 673e354754c8..f4a6bfa4619b 100644
--- a/docs/devel/migration/vfio.rst
+++ b/docs/devel/migration/vfio.rst
@@ -247,3 +247,16 @@ The multifd VFIO device state transfer is controlled by
 "x-migration-multifd-transfer" VFIO device property. This property defaults to
 AUTO, which means that VFIO device state transfer via multifd channels is
 attempted in configurations that otherwise support it.
+
+Since the target QEMU needs to load device state buffers in-order it needs to
+queue incoming buffers until they can be loaded into the device.
+This means that a malicious QEMU source could theoretically cause the target
+QEMU to allocate unlimited amounts of memory for such buffers-in-flight.
+
+The "x-migration-max-queued-buffers" property allows capping the maximum count
+of these VFIO device state buffers queued at the destination.
+
+Because a malicious QEMU source causing OOM on the target is not expected to be
+a realistic threat in most of VFIO live migration use cases and the right value
+depends on the particular setup by default this queued buffers limit is
+disabled by setting it to UINT64_MAX.
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index 850a31948878..f26c112090b4 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -56,6 +56,7 @@ typedef struct VFIOMultifd {
     QemuMutex load_bufs_mutex; /* Lock order: this lock -> BQL */
     uint32_t load_buf_idx;
     uint32_t load_buf_idx_last;
+    uint32_t load_buf_queued_pending_buffers;
 } VFIOMultifd;
 
 static void vfio_state_buffer_clear(gpointer data)
@@ -127,6 +128,17 @@ static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev,
 
     assert(packet->idx >= multifd->load_buf_idx);
 
+    multifd->load_buf_queued_pending_buffers++;
+    if (multifd->load_buf_queued_pending_buffers >
+        vbasedev->migration_max_queued_buffers) {
+        error_setg(errp,
+                   "%s: queuing state buffer %" PRIu32
+                   " would exceed the max of %" PRIu64,
+                   vbasedev->name, packet->idx,
+                   vbasedev->migration_max_queued_buffers);
+        return false;
+    }
+
     lb->data = g_memdup2(&packet->data, packet_total_size - sizeof(*packet));
     lb->len = packet_total_size - sizeof(*packet);
     lb->is_present = true;
@@ -387,6 +399,9 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
             goto thread_exit;
         }
 
+        assert(multifd->load_buf_queued_pending_buffers > 0);
+        multifd->load_buf_queued_pending_buffers--;
+
         if (multifd->load_buf_idx == multifd->load_buf_idx_last - 1) {
             trace_vfio_load_state_device_buffer_end(vbasedev->name);
         }
@@ -423,6 +438,7 @@ static VFIOMultifd *vfio_multifd_new(void)
 
     multifd->load_buf_idx = 0;
     multifd->load_buf_idx_last = UINT32_MAX;
+    multifd->load_buf_queued_pending_buffers = 0;
     qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
 
     multifd->load_bufs_thread_running = false;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fa25bded25c5..2765a39f9df1 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3524,6 +3524,8 @@ static const Property vfio_pci_dev_properties[] = {
                 vbasedev.migration_multifd_transfer,
                 vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
                 .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
+                       vbasedev.migration_max_queued_buffers, UINT64_MAX),
     DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
                      vbasedev.migration_events, false),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
@@ -3698,6 +3700,13 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
                                           "x-migration-multifd-transfer",
                                           "Transfer this device state via "
                                           "multifd channels when live migrating it");
+    object_class_property_set_description(klass, /* 10.1 */
+                                          "x-migration-max-queued-buffers",
+                                          "Maximum count of in-flight VFIO "
+                                          "device state buffers queued at the "
+                                          "destination when doing live "
+                                          "migration of device state via "
+                                          "multifd channels");
 }
 
 static const TypeInfo vfio_pci_dev_info = {
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index d45e5a68a24e..0ee34aaf668b 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -66,6 +66,7 @@ typedef struct VFIODevice {
     bool ram_block_discard_allowed;
     OnOffAuto enable_migration;
     OnOffAuto migration_multifd_transfer;
+    uint64_t migration_max_queued_buffers;
     bool migration_events;
     bool use_region_fds;
     VFIODeviceOps *ops;

[PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

This property allows configuring whether to start the config load only
after all iterables were loaded.
Such interlocking is required for ARM64 due to this platform VFIO
dependency on interrupt controller being loaded first.

The property defaults to AUTO, which means ON for ARM, OFF for other
platforms.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 docs/devel/migration/vfio.rst     |  6 +++
 hw/core/machine.c                 |  1 +
 hw/vfio/migration-multifd.c       | 88 +++++++++++++++++++++++++++++++
 hw/vfio/migration-multifd.h       |  3 ++
 hw/vfio/migration.c               | 10 +++-
 hw/vfio/pci.c                     |  9 ++++
 hw/vfio/vfio-migration-internal.h |  1 +
 include/hw/vfio/vfio-device.h     |  1 +
 8 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
index f4a6bfa4619b..7c9cb7bdbf87 100644
--- a/docs/devel/migration/vfio.rst
+++ b/docs/devel/migration/vfio.rst
@@ -260,3 +260,9 @@ Because a malicious QEMU source causing OOM on the target is not expected to be
 a realistic threat in most of VFIO live migration use cases and the right value
 depends on the particular setup by default this queued buffers limit is
 disabled by setting it to UINT64_MAX.
+
+Some host platforms (like ARM64) require that VFIO device config is loaded only
+after all iterables were loaded.
+Such interlocking is controlled by "x-migration-load-config-after-iter" VFIO
+device property, which in its default setting (AUTO) does so only on platforms
+that actually require it.
diff --git a/hw/core/machine.c b/hw/core/machine.c
index e869821b2246..16640b700f2e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -39,6 +39,7 @@
 
 GlobalProperty hw_compat_10_0[] = {
     { "scsi-hd", "dpofua", "off" },
+    { "vfio-pci", "x-migration-load-config-after-iter", "off" },
 };
 const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
 
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index f26c112090b4..a12ec1ead74a 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -17,6 +17,7 @@
 #include "qemu/error-report.h"
 #include "qemu/lockable.h"
 #include "qemu/main-loop.h"
+#include "qemu/target-info.h"
 #include "qemu/thread.h"
 #include "io/channel-buffer.h"
 #include "migration/qemu-file.h"
@@ -35,6 +36,27 @@ typedef struct VFIODeviceStatePacket {
     uint8_t data[0];
 } QEMU_PACKED VFIODeviceStatePacket;
 
+bool vfio_load_config_after_iter(VFIODevice *vbasedev)
+{
+    if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
+        return true;
+    } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
+        return false;
+    }
+
+    assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
+
+    /*
+     * Starting the config load only after all iterables were loaded is required
+     * for ARM64 due to this platform VFIO dependency on interrupt controller
+     * being loaded first.
+     *
+     * See commit d329f5032e17 ("vfio: Move the saving of the config space to
+     * the right place in VFIO migration").
+     */
+    return strcmp(target_name(), "aarch64") == 0;
+}
+
 /* type safety */
 typedef struct VFIOStateBuffers {
     GArray *array;
@@ -50,6 +72,9 @@ typedef struct VFIOMultifd {
     bool load_bufs_thread_running;
     bool load_bufs_thread_want_exit;
 
+    bool load_bufs_iter_done;
+    QemuCond load_bufs_iter_done_cond;
+
     VFIOStateBuffers load_bufs;
     QemuCond load_bufs_buffer_ready_cond;
     QemuCond load_bufs_thread_finished_cond;
@@ -409,6 +434,22 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
         multifd->load_buf_idx++;
     }
 
+    if (vfio_load_config_after_iter(vbasedev)) {
+        while (!multifd->load_bufs_iter_done) {
+            qemu_cond_wait(&multifd->load_bufs_iter_done_cond,
+                           &multifd->load_bufs_mutex);
+
+            /*
+             * Need to re-check cancellation immediately after wait in case
+             * cond was signalled by vfio_load_cleanup_load_bufs_thread().
+             */
+            if (vfio_load_bufs_thread_want_exit(multifd, should_quit)) {
+                error_setg(errp, "operation cancelled");
+                goto thread_exit;
+            }
+        }
+    }
+
     if (!vfio_load_bufs_thread_load_config(vbasedev, errp)) {
         goto thread_exit;
     }
@@ -428,6 +469,48 @@ thread_exit:
     return ret;
 }
 
+int vfio_load_state_config_load_ready(VFIODevice *vbasedev)
+{
+    VFIOMigration *migration = vbasedev->migration;
+    VFIOMultifd *multifd = migration->multifd;
+    int ret = 0;
+
+    if (!vfio_multifd_transfer_enabled(vbasedev)) {
+        error_report("%s: got DEV_CONFIG_LOAD_READY outside multifd transfer",
+                     vbasedev->name);
+        return -EINVAL;
+    }
+
+    if (!vfio_load_config_after_iter(vbasedev)) {
+        error_report("%s: got DEV_CONFIG_LOAD_READY but was disabled",
+                     vbasedev->name);
+        return -EINVAL;
+    }
+
+    assert(multifd);
+
+    /* The lock order is load_bufs_mutex -> BQL so unlock BQL here first */
+    bql_unlock();
+    WITH_QEMU_LOCK_GUARD(&multifd->load_bufs_mutex) {
+        if (multifd->load_bufs_iter_done) {
+            /* Can't print error here as we're outside BQL */
+            ret = -EINVAL;
+            break;
+        }
+
+        multifd->load_bufs_iter_done = true;
+        qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
+    }
+    bql_lock();
+
+    if (ret) {
+        error_report("%s: duplicate DEV_CONFIG_LOAD_READY",
+                     vbasedev->name);
+    }
+
+    return ret;
+}
+
 static VFIOMultifd *vfio_multifd_new(void)
 {
     VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
@@ -441,6 +524,9 @@ static VFIOMultifd *vfio_multifd_new(void)
     multifd->load_buf_queued_pending_buffers = 0;
     qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
 
+    multifd->load_bufs_iter_done = false;
+    qemu_cond_init(&multifd->load_bufs_iter_done_cond);
+
     multifd->load_bufs_thread_running = false;
     multifd->load_bufs_thread_want_exit = false;
     qemu_cond_init(&multifd->load_bufs_thread_finished_cond);
@@ -464,6 +550,7 @@ static void vfio_load_cleanup_load_bufs_thread(VFIOMultifd *multifd)
             multifd->load_bufs_thread_want_exit = true;
 
             qemu_cond_signal(&multifd->load_bufs_buffer_ready_cond);
+            qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
             qemu_cond_wait(&multifd->load_bufs_thread_finished_cond,
                            &multifd->load_bufs_mutex);
         }
@@ -476,6 +563,7 @@ static void vfio_multifd_free(VFIOMultifd *multifd)
     vfio_load_cleanup_load_bufs_thread(multifd);
 
     qemu_cond_destroy(&multifd->load_bufs_thread_finished_cond);
+    qemu_cond_destroy(&multifd->load_bufs_iter_done_cond);
     vfio_state_buffers_destroy(&multifd->load_bufs);
     qemu_cond_destroy(&multifd->load_bufs_buffer_ready_cond);
     qemu_mutex_destroy(&multifd->load_bufs_mutex);
diff --git a/hw/vfio/migration-multifd.h b/hw/vfio/migration-multifd.h
index 0bab63211d30..487f457282df 100644
--- a/hw/vfio/migration-multifd.h
+++ b/hw/vfio/migration-multifd.h
@@ -20,9 +20,12 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev);
 bool vfio_multifd_transfer_supported(void);
 bool vfio_multifd_transfer_enabled(VFIODevice *vbasedev);
 
+bool vfio_load_config_after_iter(VFIODevice *vbasedev);
 bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size,
                                     Error **errp);
 
+int vfio_load_state_config_load_ready(VFIODevice *vbasedev);
+
 void vfio_multifd_emit_dummy_eos(VFIODevice *vbasedev, QEMUFile *f);
 
 bool
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index b76697bd1a23..7c6436d4c344 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -675,7 +675,11 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
     int ret;
 
     if (vfio_multifd_transfer_enabled(vbasedev)) {
-        vfio_multifd_emit_dummy_eos(vbasedev, f);
+        if (vfio_load_config_after_iter(vbasedev)) {
+            qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
+        } else {
+            vfio_multifd_emit_dummy_eos(vbasedev, f);
+        }
         return;
     }
 
@@ -784,6 +788,10 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
 
             return ret;
         }
+        case VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY:
+        {
+            return vfio_load_state_config_load_ready(vbasedev);
+        }
         default:
             error_report("%s: Unknown tag 0x%"PRIx64, vbasedev->name, data);
             return -EINVAL;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2765a39f9df1..01e48e39de75 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3524,6 +3524,9 @@ static const Property vfio_pci_dev_properties[] = {
                 vbasedev.migration_multifd_transfer,
                 vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
                 .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("x-migration-load-config-after-iter", VFIOPCIDevice,
+                            vbasedev.migration_load_config_after_iter,
+                            ON_OFF_AUTO_AUTO),
     DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
                        vbasedev.migration_max_queued_buffers, UINT64_MAX),
     DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
@@ -3700,6 +3703,12 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
                                           "x-migration-multifd-transfer",
                                           "Transfer this device state via "
                                           "multifd channels when live migrating it");
+    object_class_property_set_description(klass, /* 10.1 */
+                                          "x-migration-load-config-after-iter",
+                                          "Start the config load only after "
+                                          "all iterables were loaded when doing "
+                                          "live migration of device state via "
+                                          "multifd channels");
     object_class_property_set_description(klass, /* 10.1 */
                                           "x-migration-max-queued-buffers",
                                           "Maximum count of in-flight VFIO "
diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
index a8b456b239df..54141e27e6b2 100644
--- a/hw/vfio/vfio-migration-internal.h
+++ b/hw/vfio/vfio-migration-internal.h
@@ -32,6 +32,7 @@
 #define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
 #define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)
 #define VFIO_MIG_FLAG_DEV_INIT_DATA_SENT (0xffffffffef100005ULL)
+#define VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY (0xffffffffef100006ULL)
 
 typedef struct VFIODevice VFIODevice;
 typedef struct VFIOMultifd VFIOMultifd;
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 0ee34aaf668b..359d553b916a 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -66,6 +66,7 @@ typedef struct VFIODevice {
     bool ram_block_discard_allowed;
     OnOffAuto enable_migration;
     OnOffAuto migration_multifd_transfer;
+    OnOffAuto migration_load_config_after_iter;
     uint64_t migration_max_queued_buffers;
     bool migration_events;
     bool use_region_fds;

[PATCH 3/3] vfio/migration: Add also max in-flight VFIO device state buffers size limit

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

There's already a max in-flight VFIO device state buffers *count* limit,
add also max queued buffers *size* limit.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 docs/devel/migration/vfio.rst |  8 +++++---
 hw/vfio/migration-multifd.c   | 21 +++++++++++++++++++--
 hw/vfio/pci.c                 |  9 +++++++++
 include/hw/vfio/vfio-device.h |  1 +
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
index 7c9cb7bdbf87..127a1db35949 100644
--- a/docs/devel/migration/vfio.rst
+++ b/docs/devel/migration/vfio.rst
@@ -254,12 +254,14 @@ This means that a malicious QEMU source could theoretically cause the target
 QEMU to allocate unlimited amounts of memory for such buffers-in-flight.
 
 The "x-migration-max-queued-buffers" property allows capping the maximum count
-of these VFIO device state buffers queued at the destination.
+of these VFIO device state buffers queued at the destination while
+"x-migration-max-queued-buffers-size" property allows capping their total queued
+size.
 
 Because a malicious QEMU source causing OOM on the target is not expected to be
 a realistic threat in most of VFIO live migration use cases and the right value
-depends on the particular setup by default this queued buffers limit is
-disabled by setting it to UINT64_MAX.
+depends on the particular setup by default these queued buffers limits are
+disabled by setting them to UINT64_MAX.
 
 Some host platforms (like ARM64) require that VFIO device config is loaded only
 after all iterables were loaded.
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index a12ec1ead74a..c76f1f2181f9 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -82,6 +82,7 @@ typedef struct VFIOMultifd {
     uint32_t load_buf_idx;
     uint32_t load_buf_idx_last;
     uint32_t load_buf_queued_pending_buffers;
+    size_t load_buf_queued_pending_buffers_size;
 } VFIOMultifd;
 
 static void vfio_state_buffer_clear(gpointer data)
@@ -138,6 +139,7 @@ static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev,
     VFIOMigration *migration = vbasedev->migration;
     VFIOMultifd *multifd = migration->multifd;
     VFIOStateBuffer *lb;
+    size_t data_size = packet_total_size - sizeof(*packet);
 
     vfio_state_buffers_assert_init(&multifd->load_bufs);
     if (packet->idx >= vfio_state_buffers_size_get(&multifd->load_bufs)) {
@@ -164,8 +166,19 @@ static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev,
         return false;
     }
 
-    lb->data = g_memdup2(&packet->data, packet_total_size - sizeof(*packet));
-    lb->len = packet_total_size - sizeof(*packet);
+    multifd->load_buf_queued_pending_buffers_size += data_size;
+    if (multifd->load_buf_queued_pending_buffers_size >
+        vbasedev->migration_max_queued_buffers_size) {
+        error_setg(errp,
+                   "%s: queuing state buffer %" PRIu32
+                   " would exceed the size max of %" PRIu64,
+                   vbasedev->name, packet->idx,
+                   vbasedev->migration_max_queued_buffers_size);
+        return false;
+    }
+
+    lb->data = g_memdup2(&packet->data, data_size);
+    lb->len = data_size;
     lb->is_present = true;
 
     return true;
@@ -349,6 +362,9 @@ static bool vfio_load_state_buffer_write(VFIODevice *vbasedev,
         assert(wr_ret <= buf_len);
         buf_len -= wr_ret;
         buf_cur += wr_ret;
+
+        assert(multifd->load_buf_queued_pending_buffers_size >= wr_ret);
+        multifd->load_buf_queued_pending_buffers_size -= wr_ret;
     }
 
     trace_vfio_load_state_device_buffer_load_end(vbasedev->name,
@@ -522,6 +538,7 @@ static VFIOMultifd *vfio_multifd_new(void)
     multifd->load_buf_idx = 0;
     multifd->load_buf_idx_last = UINT32_MAX;
     multifd->load_buf_queued_pending_buffers = 0;
+    multifd->load_buf_queued_pending_buffers_size = 0;
     qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
 
     multifd->load_bufs_iter_done = false;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 01e48e39de75..944813ee7bdb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3529,6 +3529,8 @@ static const Property vfio_pci_dev_properties[] = {
                             ON_OFF_AUTO_AUTO),
     DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
                        vbasedev.migration_max_queued_buffers, UINT64_MAX),
+    DEFINE_PROP_SIZE("x-migration-max-queued-buffers-size", VFIOPCIDevice,
+                     vbasedev.migration_max_queued_buffers_size, UINT64_MAX),
     DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
                      vbasedev.migration_events, false),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
@@ -3716,6 +3718,13 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
                                           "destination when doing live "
                                           "migration of device state via "
                                           "multifd channels");
+    object_class_property_set_description(klass, /* 10.1 */
+                                          "x-migration-max-queued-buffers-size",
+                                          "Maximum size of in-flight VFIO "
+                                          "device state buffers queued at the "
+                                          "destination when doing live "
+                                          "migration of device state via "
+                                          "multifd channels");
 }
 
 static const TypeInfo vfio_pci_dev_info = {
diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 359d553b916a..3e86d07347d6 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -68,6 +68,7 @@ typedef struct VFIODevice {
     OnOffAuto migration_multifd_transfer;
     OnOffAuto migration_load_config_after_iter;
     uint64_t migration_max_queued_buffers;
+    uint64_t migration_max_queued_buffers_size;
     bool migration_events;
     bool use_region_fds;
     VFIODeviceOps *ops;

Re: [PATCH 1/3] vfio/migration: Max in-flight VFIO device state buffer count limit

[Fabiano Rosas]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> Allow capping the maximum count of in-flight VFIO device state buffers
> queued at the destination, otherwise a malicious QEMU source could
> theoretically cause the target QEMU to allocate unlimited amounts of memory
> for buffers-in-flight.
>
> Since this is not expected to be a realistic threat in most of VFIO live
> migration use cases and the right value depends on the particular setup
> disable the limit by default by setting it to UINT64_MAX.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Fabiano Rosas]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> This property allows configuring whether to start the config load only
> after all iterables were loaded.
> Such interlocking is required for ARM64 due to this platform VFIO
> dependency on interrupt controller being loaded first.
>
> The property defaults to AUTO, which means ON for ARM, OFF for other
> platforms.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 3/3] vfio/migration: Add also max in-flight VFIO device state buffers size limit

[Fabiano Rosas]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> There's already a max in-flight VFIO device state buffers *count* limit,
> add also max queued buffers *size* limit.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Cédric Le Goater]

Adding more maintainers,

+Eric (ARM smmu),
+Peter (ARM, GIC, virt),

On 6/24/25 19:51, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> This property allows configuring whether to start the config load only
> after all iterables were loaded.
> Such interlocking is required for ARM64 due to this platform VFIO
> dependency on interrupt controller being loaded first.

Could you please a bit more ?
  
> The property defaults to AUTO, which means ON for ARM, OFF for other
> platforms.
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

As we've mentioned a couple of times, this is essentially a workaround
to help ARM support a migration optimization (multifd) for guests using
passthrough PCI devices. At the moment, this mainly for MLX5 VFs
(upstream) and NVIDIA vGPUs (not upstream).

It looks like the issue is related to the ordering of the vmstate during
load time.

Is there a different way we could address this ? Other virt machines like
x86 and ppc also deal with complex interrupt controllers, and those cases
have been handled cleanly AFAICT. So what’s the fundamental issue here that
makes it necessary to add more complexity to an already complex feature
(multif VFIO migration) for what seems to be a corner case on a single
architecture ?

d329f5032e17 is the turning point.

Using 'strcmp(target_name(), "aarch64")' is quite unique in the QEMU code
base, and to be honest, I’m not too keen on adding it unless there’s really
no other option.

Thanks,

C.


> ---
>   docs/devel/migration/vfio.rst     |  6 +++
>   hw/core/machine.c                 |  1 +
>   hw/vfio/migration-multifd.c       | 88 +++++++++++++++++++++++++++++++
>   hw/vfio/migration-multifd.h       |  3 ++
>   hw/vfio/migration.c               | 10 +++-
>   hw/vfio/pci.c                     |  9 ++++
>   hw/vfio/vfio-migration-internal.h |  1 +
>   include/hw/vfio/vfio-device.h     |  1 +
>   8 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
> index f4a6bfa4619b..7c9cb7bdbf87 100644
> --- a/docs/devel/migration/vfio.rst
> +++ b/docs/devel/migration/vfio.rst
> @@ -260,3 +260,9 @@ Because a malicious QEMU source causing OOM on the target is not expected to be
>   a realistic threat in most of VFIO live migration use cases and the right value
>   depends on the particular setup by default this queued buffers limit is
>   disabled by setting it to UINT64_MAX.
> +
> +Some host platforms (like ARM64) require that VFIO device config is loaded only
> +after all iterables were loaded.
> +Such interlocking is controlled by "x-migration-load-config-after-iter" VFIO
> +device property, which in its default setting (AUTO) does so only on platforms
> +that actually require it.
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e869821b2246..16640b700f2e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -39,6 +39,7 @@
>   
>   GlobalProperty hw_compat_10_0[] = {
>       { "scsi-hd", "dpofua", "off" },
> +    { "vfio-pci", "x-migration-load-config-after-iter", "off" },
>   };
>   const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
>   
> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
> index f26c112090b4..a12ec1ead74a 100644
> --- a/hw/vfio/migration-multifd.c
> +++ b/hw/vfio/migration-multifd.c
> @@ -17,6 +17,7 @@
>   #include "qemu/error-report.h"
>   #include "qemu/lockable.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/target-info.h"
>   #include "qemu/thread.h"
>   #include "io/channel-buffer.h"
>   #include "migration/qemu-file.h"
> @@ -35,6 +36,27 @@ typedef struct VFIODeviceStatePacket {
>       uint8_t data[0];
>   } QEMU_PACKED VFIODeviceStatePacket;
>   
> +bool vfio_load_config_after_iter(VFIODevice *vbasedev)
> +{
> +    if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
> +        return true;
> +    } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
> +        return false;
> +    }
> +
> +    assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
> +
> +    /*
> +     * Starting the config load only after all iterables were loaded is required
> +     * for ARM64 due to this platform VFIO dependency on interrupt controller
> +     * being loaded first.
> +     *
> +     * See commit d329f5032e17 ("vfio: Move the saving of the config space to
> +     * the right place in VFIO migration").
> +     */
> +    return strcmp(target_name(), "aarch64") == 0;
> +}
> +
>   /* type safety */
>   typedef struct VFIOStateBuffers {
>       GArray *array;
> @@ -50,6 +72,9 @@ typedef struct VFIOMultifd {
>       bool load_bufs_thread_running;
>       bool load_bufs_thread_want_exit;
>   
> +    bool load_bufs_iter_done;
> +    QemuCond load_bufs_iter_done_cond;
> +
>       VFIOStateBuffers load_bufs;
>       QemuCond load_bufs_buffer_ready_cond;
>       QemuCond load_bufs_thread_finished_cond;
> @@ -409,6 +434,22 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
>           multifd->load_buf_idx++;
>       }
>   
> +    if (vfio_load_config_after_iter(vbasedev)) {
> +        while (!multifd->load_bufs_iter_done) {
> +            qemu_cond_wait(&multifd->load_bufs_iter_done_cond,
> +                           &multifd->load_bufs_mutex);
> +
> +            /*
> +             * Need to re-check cancellation immediately after wait in case
> +             * cond was signalled by vfio_load_cleanup_load_bufs_thread().
> +             */
> +            if (vfio_load_bufs_thread_want_exit(multifd, should_quit)) {
> +                error_setg(errp, "operation cancelled");
> +                goto thread_exit;
> +            }
> +        }
> +    }
> +
>       if (!vfio_load_bufs_thread_load_config(vbasedev, errp)) {
>           goto thread_exit;
>       }
> @@ -428,6 +469,48 @@ thread_exit:
>       return ret;
>   }
>   
> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +    VFIOMultifd *multifd = migration->multifd;
> +    int ret = 0;
> +
> +    if (!vfio_multifd_transfer_enabled(vbasedev)) {
> +        error_report("%s: got DEV_CONFIG_LOAD_READY outside multifd transfer",
> +                     vbasedev->name);
> +        return -EINVAL;
> +    }
> +
> +    if (!vfio_load_config_after_iter(vbasedev)) {
> +        error_report("%s: got DEV_CONFIG_LOAD_READY but was disabled",
> +                     vbasedev->name);
> +        return -EINVAL;
> +    }
> +
> +    assert(multifd);
> +
> +    /* The lock order is load_bufs_mutex -> BQL so unlock BQL here first */
> +    bql_unlock();
> +    WITH_QEMU_LOCK_GUARD(&multifd->load_bufs_mutex) {
> +        if (multifd->load_bufs_iter_done) {
> +            /* Can't print error here as we're outside BQL */
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        multifd->load_bufs_iter_done = true;
> +        qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
> +    }
> +    bql_lock();
> +
> +    if (ret) {
> +        error_report("%s: duplicate DEV_CONFIG_LOAD_READY",
> +                     vbasedev->name);
> +    }
> +
> +    return ret;
> +}
> +
>   static VFIOMultifd *vfio_multifd_new(void)
>   {
>       VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
> @@ -441,6 +524,9 @@ static VFIOMultifd *vfio_multifd_new(void)
>       multifd->load_buf_queued_pending_buffers = 0;
>       qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
>   
> +    multifd->load_bufs_iter_done = false;
> +    qemu_cond_init(&multifd->load_bufs_iter_done_cond);
> +
>       multifd->load_bufs_thread_running = false;
>       multifd->load_bufs_thread_want_exit = false;
>       qemu_cond_init(&multifd->load_bufs_thread_finished_cond);
> @@ -464,6 +550,7 @@ static void vfio_load_cleanup_load_bufs_thread(VFIOMultifd *multifd)
>               multifd->load_bufs_thread_want_exit = true;
>   
>               qemu_cond_signal(&multifd->load_bufs_buffer_ready_cond);
> +            qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
>               qemu_cond_wait(&multifd->load_bufs_thread_finished_cond,
>                              &multifd->load_bufs_mutex);
>           }
> @@ -476,6 +563,7 @@ static void vfio_multifd_free(VFIOMultifd *multifd)
>       vfio_load_cleanup_load_bufs_thread(multifd);
>   
>       qemu_cond_destroy(&multifd->load_bufs_thread_finished_cond);
> +    qemu_cond_destroy(&multifd->load_bufs_iter_done_cond);
>       vfio_state_buffers_destroy(&multifd->load_bufs);
>       qemu_cond_destroy(&multifd->load_bufs_buffer_ready_cond);
>       qemu_mutex_destroy(&multifd->load_bufs_mutex);
> diff --git a/hw/vfio/migration-multifd.h b/hw/vfio/migration-multifd.h
> index 0bab63211d30..487f457282df 100644
> --- a/hw/vfio/migration-multifd.h
> +++ b/hw/vfio/migration-multifd.h
> @@ -20,9 +20,12 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev);
>   bool vfio_multifd_transfer_supported(void);
>   bool vfio_multifd_transfer_enabled(VFIODevice *vbasedev);
>   
> +bool vfio_load_config_after_iter(VFIODevice *vbasedev);
>   bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size,
>                                       Error **errp);
>   
> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev);
> +
>   void vfio_multifd_emit_dummy_eos(VFIODevice *vbasedev, QEMUFile *f);
>   
>   bool
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index b76697bd1a23..7c6436d4c344 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -675,7 +675,11 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>       int ret;
>   
>       if (vfio_multifd_transfer_enabled(vbasedev)) {
> -        vfio_multifd_emit_dummy_eos(vbasedev, f);
> +        if (vfio_load_config_after_iter(vbasedev)) {
> +            qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
> +        } else {
> +            vfio_multifd_emit_dummy_eos(vbasedev, f);
> +        }
>           return;
>       }
>   
> @@ -784,6 +788,10 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
>   
>               return ret;
>           }
> +        case VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY:
> +        {
> +            return vfio_load_state_config_load_ready(vbasedev);
> +        }
>           default:
>               error_report("%s: Unknown tag 0x%"PRIx64, vbasedev->name, data);
>               return -EINVAL;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2765a39f9df1..01e48e39de75 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3524,6 +3524,9 @@ static const Property vfio_pci_dev_properties[] = {
>                   vbasedev.migration_multifd_transfer,
>                   vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
>                   .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("x-migration-load-config-after-iter", VFIOPCIDevice,
> +                            vbasedev.migration_load_config_after_iter,
> +                            ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
>                          vbasedev.migration_max_queued_buffers, UINT64_MAX),
>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
> @@ -3700,6 +3703,12 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
>                                             "x-migration-multifd-transfer",
>                                             "Transfer this device state via "
>                                             "multifd channels when live migrating it");
> +    object_class_property_set_description(klass, /* 10.1 */
> +                                          "x-migration-load-config-after-iter",
> +                                          "Start the config load only after "
> +                                          "all iterables were loaded when doing "
> +                                          "live migration of device state via "
> +                                          "multifd channels");
>       object_class_property_set_description(klass, /* 10.1 */
>                                             "x-migration-max-queued-buffers",
>                                             "Maximum count of in-flight VFIO "
> diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
> index a8b456b239df..54141e27e6b2 100644
> --- a/hw/vfio/vfio-migration-internal.h
> +++ b/hw/vfio/vfio-migration-internal.h
> @@ -32,6 +32,7 @@
>   #define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
>   #define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)
>   #define VFIO_MIG_FLAG_DEV_INIT_DATA_SENT (0xffffffffef100005ULL)
> +#define VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY (0xffffffffef100006ULL)
>   
>   typedef struct VFIODevice VFIODevice;
>   typedef struct VFIOMultifd VFIOMultifd;
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 0ee34aaf668b..359d553b916a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -66,6 +66,7 @@ typedef struct VFIODevice {
>       bool ram_block_discard_allowed;
>       OnOffAuto enable_migration;
>       OnOffAuto migration_multifd_transfer;
> +    OnOffAuto migration_load_config_after_iter;
>       uint64_t migration_max_queued_buffers;
>       bool migration_events;
>       bool use_region_fds;
> 

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Cédric Le Goater]

On 7/2/25 08:27, Cédric Le Goater wrote:
> Adding more maintainers,
> 
> +Eric (ARM smmu),
> +Peter (ARM, GIC, virt),
> 
> On 6/24/25 19:51, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> This property allows configuring whether to start the config load only
>> after all iterables were loaded.
>> Such interlocking is required for ARM64 due to this platform VFIO
>> dependency on interrupt controller being loaded first.
> 
> Could you please a bit more ?
                   ^
                 explain

Still not fully awake ...

C.

>> The property defaults to AUTO, which means ON for ARM, OFF for other
>> platforms.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> As we've mentioned a couple of times, this is essentially a workaround
> to help ARM support a migration optimization (multifd) for guests using
> passthrough PCI devices. At the moment, this mainly for MLX5 VFs
> (upstream) and NVIDIA vGPUs (not upstream).
> 
> It looks like the issue is related to the ordering of the vmstate during
> load time.
> 
> Is there a different way we could address this ? Other virt machines like
> x86 and ppc also deal with complex interrupt controllers, and those cases
> have been handled cleanly AFAICT. So what’s the fundamental issue here that
> makes it necessary to add more complexity to an already complex feature
> (multif VFIO migration) for what seems to be a corner case on a single
> architecture ?
> 
> d329f5032e17 is the turning point.
> 
> Using 'strcmp(target_name(), "aarch64")' is quite unique in the QEMU code
> base, and to be honest, I’m not too keen on adding it unless there’s really
> no other option.
> 
> Thanks,
> 
> C.
> 
> 
>> ---
>>   docs/devel/migration/vfio.rst     |  6 +++
>>   hw/core/machine.c                 |  1 +
>>   hw/vfio/migration-multifd.c       | 88 +++++++++++++++++++++++++++++++
>>   hw/vfio/migration-multifd.h       |  3 ++
>>   hw/vfio/migration.c               | 10 +++-
>>   hw/vfio/pci.c                     |  9 ++++
>>   hw/vfio/vfio-migration-internal.h |  1 +
>>   include/hw/vfio/vfio-device.h     |  1 +
>>   8 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
>> index f4a6bfa4619b..7c9cb7bdbf87 100644
>> --- a/docs/devel/migration/vfio.rst
>> +++ b/docs/devel/migration/vfio.rst
>> @@ -260,3 +260,9 @@ Because a malicious QEMU source causing OOM on the target is not expected to be
>>   a realistic threat in most of VFIO live migration use cases and the right value
>>   depends on the particular setup by default this queued buffers limit is
>>   disabled by setting it to UINT64_MAX.
>> +
>> +Some host platforms (like ARM64) require that VFIO device config is loaded only
>> +after all iterables were loaded.
>> +Such interlocking is controlled by "x-migration-load-config-after-iter" VFIO
>> +device property, which in its default setting (AUTO) does so only on platforms
>> +that actually require it.
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index e869821b2246..16640b700f2e 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -39,6 +39,7 @@
>>   GlobalProperty hw_compat_10_0[] = {
>>       { "scsi-hd", "dpofua", "off" },
>> +    { "vfio-pci", "x-migration-load-config-after-iter", "off" },
>>   };
>>   const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
>> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
>> index f26c112090b4..a12ec1ead74a 100644
>> --- a/hw/vfio/migration-multifd.c
>> +++ b/hw/vfio/migration-multifd.c
>> @@ -17,6 +17,7 @@
>>   #include "qemu/error-report.h"
>>   #include "qemu/lockable.h"
>>   #include "qemu/main-loop.h"
>> +#include "qemu/target-info.h"
>>   #include "qemu/thread.h"
>>   #include "io/channel-buffer.h"
>>   #include "migration/qemu-file.h"
>> @@ -35,6 +36,27 @@ typedef struct VFIODeviceStatePacket {
>>       uint8_t data[0];
>>   } QEMU_PACKED VFIODeviceStatePacket;
>> +bool vfio_load_config_after_iter(VFIODevice *vbasedev)
>> +{
>> +    if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
>> +        return true;
>> +    } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
>> +        return false;
>> +    }
>> +
>> +    assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
>> +
>> +    /*
>> +     * Starting the config load only after all iterables were loaded is required
>> +     * for ARM64 due to this platform VFIO dependency on interrupt controller
>> +     * being loaded first.
>> +     *
>> +     * See commit d329f5032e17 ("vfio: Move the saving of the config space to
>> +     * the right place in VFIO migration").
>> +     */
>> +    return strcmp(target_name(), "aarch64") == 0;
>> +}
>> +
>>   /* type safety */
>>   typedef struct VFIOStateBuffers {
>>       GArray *array;
>> @@ -50,6 +72,9 @@ typedef struct VFIOMultifd {
>>       bool load_bufs_thread_running;
>>       bool load_bufs_thread_want_exit;
>> +    bool load_bufs_iter_done;
>> +    QemuCond load_bufs_iter_done_cond;
>> +
>>       VFIOStateBuffers load_bufs;
>>       QemuCond load_bufs_buffer_ready_cond;
>>       QemuCond load_bufs_thread_finished_cond;
>> @@ -409,6 +434,22 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
>>           multifd->load_buf_idx++;
>>       }
>> +    if (vfio_load_config_after_iter(vbasedev)) {
>> +        while (!multifd->load_bufs_iter_done) {
>> +            qemu_cond_wait(&multifd->load_bufs_iter_done_cond,
>> +                           &multifd->load_bufs_mutex);
>> +
>> +            /*
>> +             * Need to re-check cancellation immediately after wait in case
>> +             * cond was signalled by vfio_load_cleanup_load_bufs_thread().
>> +             */
>> +            if (vfio_load_bufs_thread_want_exit(multifd, should_quit)) {
>> +                error_setg(errp, "operation cancelled");
>> +                goto thread_exit;
>> +            }
>> +        }
>> +    }
>> +
>>       if (!vfio_load_bufs_thread_load_config(vbasedev, errp)) {
>>           goto thread_exit;
>>       }
>> @@ -428,6 +469,48 @@ thread_exit:
>>       return ret;
>>   }
>> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev)
>> +{
>> +    VFIOMigration *migration = vbasedev->migration;
>> +    VFIOMultifd *multifd = migration->multifd;
>> +    int ret = 0;
>> +
>> +    if (!vfio_multifd_transfer_enabled(vbasedev)) {
>> +        error_report("%s: got DEV_CONFIG_LOAD_READY outside multifd transfer",
>> +                     vbasedev->name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!vfio_load_config_after_iter(vbasedev)) {
>> +        error_report("%s: got DEV_CONFIG_LOAD_READY but was disabled",
>> +                     vbasedev->name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    assert(multifd);
>> +
>> +    /* The lock order is load_bufs_mutex -> BQL so unlock BQL here first */
>> +    bql_unlock();
>> +    WITH_QEMU_LOCK_GUARD(&multifd->load_bufs_mutex) {
>> +        if (multifd->load_bufs_iter_done) {
>> +            /* Can't print error here as we're outside BQL */
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        multifd->load_bufs_iter_done = true;
>> +        qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
>> +    }
>> +    bql_lock();
>> +
>> +    if (ret) {
>> +        error_report("%s: duplicate DEV_CONFIG_LOAD_READY",
>> +                     vbasedev->name);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static VFIOMultifd *vfio_multifd_new(void)
>>   {
>>       VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
>> @@ -441,6 +524,9 @@ static VFIOMultifd *vfio_multifd_new(void)
>>       multifd->load_buf_queued_pending_buffers = 0;
>>       qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
>> +    multifd->load_bufs_iter_done = false;
>> +    qemu_cond_init(&multifd->load_bufs_iter_done_cond);
>> +
>>       multifd->load_bufs_thread_running = false;
>>       multifd->load_bufs_thread_want_exit = false;
>>       qemu_cond_init(&multifd->load_bufs_thread_finished_cond);
>> @@ -464,6 +550,7 @@ static void vfio_load_cleanup_load_bufs_thread(VFIOMultifd *multifd)
>>               multifd->load_bufs_thread_want_exit = true;
>>               qemu_cond_signal(&multifd->load_bufs_buffer_ready_cond);
>> +            qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
>>               qemu_cond_wait(&multifd->load_bufs_thread_finished_cond,
>>                              &multifd->load_bufs_mutex);
>>           }
>> @@ -476,6 +563,7 @@ static void vfio_multifd_free(VFIOMultifd *multifd)
>>       vfio_load_cleanup_load_bufs_thread(multifd);
>>       qemu_cond_destroy(&multifd->load_bufs_thread_finished_cond);
>> +    qemu_cond_destroy(&multifd->load_bufs_iter_done_cond);
>>       vfio_state_buffers_destroy(&multifd->load_bufs);
>>       qemu_cond_destroy(&multifd->load_bufs_buffer_ready_cond);
>>       qemu_mutex_destroy(&multifd->load_bufs_mutex);
>> diff --git a/hw/vfio/migration-multifd.h b/hw/vfio/migration-multifd.h
>> index 0bab63211d30..487f457282df 100644
>> --- a/hw/vfio/migration-multifd.h
>> +++ b/hw/vfio/migration-multifd.h
>> @@ -20,9 +20,12 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev);
>>   bool vfio_multifd_transfer_supported(void);
>>   bool vfio_multifd_transfer_enabled(VFIODevice *vbasedev);
>> +bool vfio_load_config_after_iter(VFIODevice *vbasedev);
>>   bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size,
>>                                       Error **errp);
>> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev);
>> +
>>   void vfio_multifd_emit_dummy_eos(VFIODevice *vbasedev, QEMUFile *f);
>>   bool
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index b76697bd1a23..7c6436d4c344 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -675,7 +675,11 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>>       int ret;
>>       if (vfio_multifd_transfer_enabled(vbasedev)) {
>> -        vfio_multifd_emit_dummy_eos(vbasedev, f);
>> +        if (vfio_load_config_after_iter(vbasedev)) {
>> +            qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
>> +        } else {
>> +            vfio_multifd_emit_dummy_eos(vbasedev, f);
>> +        }
>>           return;
>>       }
>> @@ -784,6 +788,10 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
>>               return ret;
>>           }
>> +        case VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY:
>> +        {
>> +            return vfio_load_state_config_load_ready(vbasedev);
>> +        }
>>           default:
>>               error_report("%s: Unknown tag 0x%"PRIx64, vbasedev->name, data);
>>               return -EINVAL;
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 2765a39f9df1..01e48e39de75 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3524,6 +3524,9 @@ static const Property vfio_pci_dev_properties[] = {
>>                   vbasedev.migration_multifd_transfer,
>>                   vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
>>                   .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_ON_OFF_AUTO("x-migration-load-config-after-iter", VFIOPCIDevice,
>> +                            vbasedev.migration_load_config_after_iter,
>> +                            ON_OFF_AUTO_AUTO),
>>       DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
>>                          vbasedev.migration_max_queued_buffers, UINT64_MAX),
>>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>> @@ -3700,6 +3703,12 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
>>                                             "x-migration-multifd-transfer",
>>                                             "Transfer this device state via "
>>                                             "multifd channels when live migrating it");
>> +    object_class_property_set_description(klass, /* 10.1 */
>> +                                          "x-migration-load-config-after-iter",
>> +                                          "Start the config load only after "
>> +                                          "all iterables were loaded when doing "
>> +                                          "live migration of device state via "
>> +                                          "multifd channels");
>>       object_class_property_set_description(klass, /* 10.1 */
>>                                             "x-migration-max-queued-buffers",
>>                                             "Maximum count of in-flight VFIO "
>> diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
>> index a8b456b239df..54141e27e6b2 100644
>> --- a/hw/vfio/vfio-migration-internal.h
>> +++ b/hw/vfio/vfio-migration-internal.h
>> @@ -32,6 +32,7 @@
>>   #define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
>>   #define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)
>>   #define VFIO_MIG_FLAG_DEV_INIT_DATA_SENT (0xffffffffef100005ULL)
>> +#define VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY (0xffffffffef100006ULL)
>>   typedef struct VFIODevice VFIODevice;
>>   typedef struct VFIOMultifd VFIOMultifd;
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 0ee34aaf668b..359d553b916a 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -66,6 +66,7 @@ typedef struct VFIODevice {
>>       bool ram_block_discard_allowed;
>>       OnOffAuto enable_migration;
>>       OnOffAuto migration_multifd_transfer;
>> +    OnOffAuto migration_load_config_after_iter;
>>       uint64_t migration_max_queued_buffers;
>>       bool migration_events;
>>       bool use_region_fds;
>>
> 

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Maciej S. Szmigiero]

On 2.07.2025 08:27, Cédric Le Goater wrote:
> Adding more maintainers,
> 
> +Eric (ARM smmu),
> +Peter (ARM, GIC, virt),
> 
> On 6/24/25 19:51, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> This property allows configuring whether to start the config load only
>> after all iterables were loaded.
>> Such interlocking is required for ARM64 due to this platform VFIO
>> dependency on interrupt controller being loaded first.
> 
> Could you please a bit more ?

Any proposals what more you'd want to have written there?

Do you want to have the description of the issue being fixed from commit
d329f5032e17 copied into this fix commit message?

>> The property defaults to AUTO, which means ON for ARM, OFF for other
>> platforms.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> As we've mentioned a couple of times, this is essentially a workaround
> to help ARM support a migration optimization (multifd) for guests using
> passthrough PCI devices. At the moment, this mainly for MLX5 VFs
> (upstream) and NVIDIA vGPUs (not upstream).
> 
> It looks like the issue is related to the ordering of the vmstate during
> load time.
> 
> Is there a different way we could address this ? Other virt machines like
> x86 and ppc also deal with complex interrupt controllers, and those cases
> have been handled cleanly AFAICT. So what’s the fundamental issue here that
> makes it necessary to add more complexity to an already complex feature
> (multif VFIO migration) for what seems to be a corner case on a single
> architecture ?
> 
> d329f5032e17 is the turning point.
That commit says that restoring VFIO devices config space on an ARM target
needs to have the VGIC interrupt controller there already fully loaded,
otherwise that VFIO config space load operation may error out.

But here it would be good to have some feedback from ARM people, thanks for
CCing them.

I'm open to reasonable alternative proposals - as long as they can be tested,
preferably on other platforms.

The reason I implemented this ARM device loading ordering requirement this way
is that I can then test it on the VFIO setup I have and be reasonably certain
that it will work on the target platform too.

> Using 'strcmp(target_name(), "aarch64")' is quite unique in the QEMU code
> base, and to be honest, I’m not too keen on adding it unless there’s really
> no other option.
The previous versions simply tested the TARGET_ARM macro but commit
5731baee6c3c ("hw/vfio: Compile some common objects once") made the
migration-multifd.c file target-independent so it cannot use target-specific
macros now.

Another option would be to move vfio_load_config_after_iter() to helpers.c
since that file is target-dependent and can simply test TARGET_ARM
macro (#if defined(TARGET_ARM)) instead of doing strcmp(target_name(), "aarch64")
which I agree looks weird.

> Thanks,
> 
> C.
> 
>

Thanks,
Maciej

Re: [PATCH 1/3] vfio/migration: Max in-flight VFIO device state buffer count limit

[Avihai Horon]

On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
> External email: Use caution opening links or attachments
>
>
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> Allow capping the maximum count of in-flight VFIO device state buffers
> queued at the destination, otherwise a malicious QEMU source could
> theoretically cause the target QEMU to allocate unlimited amounts of memory
> for buffers-in-flight.
>
> Since this is not expected to be a realistic threat in most of VFIO live
> migration use cases and the right value depends on the particular setup
> disable the limit by default by setting it to UINT64_MAX.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Avihai Horon <avihaih@nvidia.com>

But do we really need both x-migration-max-queued-buffers and 
x-migration-max-queued-buffers-size?
I think one is sufficient.

I vote for x-migration-max-queued-buffers-size as the actual memory 
limit won't change depending on VFIO migration buffer size.

However, if you pick x-migration-max-queued-buffers, maybe it's worth 
mentioning in the docs what is the size of a buffer?

Thanks.

> ---
>   docs/devel/migration/vfio.rst | 13 +++++++++++++
>   hw/vfio/migration-multifd.c   | 16 ++++++++++++++++
>   hw/vfio/pci.c                 |  9 +++++++++
>   include/hw/vfio/vfio-device.h |  1 +
>   4 files changed, 39 insertions(+)
>
> diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
> index 673e354754c8..f4a6bfa4619b 100644
> --- a/docs/devel/migration/vfio.rst
> +++ b/docs/devel/migration/vfio.rst
> @@ -247,3 +247,16 @@ The multifd VFIO device state transfer is controlled by
>   "x-migration-multifd-transfer" VFIO device property. This property defaults to
>   AUTO, which means that VFIO device state transfer via multifd channels is
>   attempted in configurations that otherwise support it.
> +
> +Since the target QEMU needs to load device state buffers in-order it needs to
> +queue incoming buffers until they can be loaded into the device.
> +This means that a malicious QEMU source could theoretically cause the target
> +QEMU to allocate unlimited amounts of memory for such buffers-in-flight.
> +
> +The "x-migration-max-queued-buffers" property allows capping the maximum count
> +of these VFIO device state buffers queued at the destination.
> +
> +Because a malicious QEMU source causing OOM on the target is not expected to be
> +a realistic threat in most of VFIO live migration use cases and the right value
> +depends on the particular setup by default this queued buffers limit is
> +disabled by setting it to UINT64_MAX.
> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
> index 850a31948878..f26c112090b4 100644
> --- a/hw/vfio/migration-multifd.c
> +++ b/hw/vfio/migration-multifd.c
> @@ -56,6 +56,7 @@ typedef struct VFIOMultifd {
>       QemuMutex load_bufs_mutex; /* Lock order: this lock -> BQL */
>       uint32_t load_buf_idx;
>       uint32_t load_buf_idx_last;
> +    uint32_t load_buf_queued_pending_buffers;
>   } VFIOMultifd;
>
>   static void vfio_state_buffer_clear(gpointer data)
> @@ -127,6 +128,17 @@ static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev,
>
>       assert(packet->idx >= multifd->load_buf_idx);
>
> +    multifd->load_buf_queued_pending_buffers++;
> +    if (multifd->load_buf_queued_pending_buffers >
> +        vbasedev->migration_max_queued_buffers) {
> +        error_setg(errp,
> +                   "%s: queuing state buffer %" PRIu32
> +                   " would exceed the max of %" PRIu64,
> +                   vbasedev->name, packet->idx,
> +                   vbasedev->migration_max_queued_buffers);
> +        return false;
> +    }
> +
>       lb->data = g_memdup2(&packet->data, packet_total_size - sizeof(*packet));
>       lb->len = packet_total_size - sizeof(*packet);
>       lb->is_present = true;
> @@ -387,6 +399,9 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
>               goto thread_exit;
>           }
>
> +        assert(multifd->load_buf_queued_pending_buffers > 0);
> +        multifd->load_buf_queued_pending_buffers--;
> +
>           if (multifd->load_buf_idx == multifd->load_buf_idx_last - 1) {
>               trace_vfio_load_state_device_buffer_end(vbasedev->name);
>           }
> @@ -423,6 +438,7 @@ static VFIOMultifd *vfio_multifd_new(void)
>
>       multifd->load_buf_idx = 0;
>       multifd->load_buf_idx_last = UINT32_MAX;
> +    multifd->load_buf_queued_pending_buffers = 0;
>       qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
>
>       multifd->load_bufs_thread_running = false;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index fa25bded25c5..2765a39f9df1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3524,6 +3524,8 @@ static const Property vfio_pci_dev_properties[] = {
>                   vbasedev.migration_multifd_transfer,
>                   vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
>                   .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
> +                       vbasedev.migration_max_queued_buffers, UINT64_MAX),
>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>                        vbasedev.migration_events, false),
>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
> @@ -3698,6 +3700,13 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
>                                             "x-migration-multifd-transfer",
>                                             "Transfer this device state via "
>                                             "multifd channels when live migrating it");
> +    object_class_property_set_description(klass, /* 10.1 */
> +                                          "x-migration-max-queued-buffers",
> +                                          "Maximum count of in-flight VFIO "
> +                                          "device state buffers queued at the "
> +                                          "destination when doing live "
> +                                          "migration of device state via "
> +                                          "multifd channels");
>   }
>
>   static const TypeInfo vfio_pci_dev_info = {
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index d45e5a68a24e..0ee34aaf668b 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -66,6 +66,7 @@ typedef struct VFIODevice {
>       bool ram_block_discard_allowed;
>       OnOffAuto enable_migration;
>       OnOffAuto migration_multifd_transfer;
> +    uint64_t migration_max_queued_buffers;
>       bool migration_events;
>       bool use_region_fds;
>       VFIODeviceOps *ops;

Re: [PATCH 3/3] vfio/migration: Add also max in-flight VFIO device state buffers size limit

[Avihai Horon]

On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
> External email: Use caution opening links or attachments
>
>
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> There's already a max in-flight VFIO device state buffers *count* limit,
> add also max queued buffers *size* limit.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Reviewed-by: Avihai Horon <avihaih@nvidia.com>

Re: [PATCH 1/3] vfio/migration: Max in-flight VFIO device state buffer count limit

[Maciej S. Szmigiero]

On 7.07.2025 11:29, Avihai Horon wrote:
> 
> On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> Allow capping the maximum count of in-flight VFIO device state buffers
>> queued at the destination, otherwise a malicious QEMU source could
>> theoretically cause the target QEMU to allocate unlimited amounts of memory
>> for buffers-in-flight.
>>
>> Since this is not expected to be a realistic threat in most of VFIO live
>> migration use cases and the right value depends on the particular setup
>> disable the limit by default by setting it to UINT64_MAX.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>

Thanks.

> But do we really need both x-migration-max-queued-buffers and x-migration-max-queued-buffers-size?
> I think one is sufficient.
> 
> I vote for x-migration-max-queued-buffers-size as the actual memory limit won't change depending on VFIO migration buffer size.

If just one of these limits were to be implemented my vote would be also for the size limit rather than the count limit.

> However, if you pick x-migration-max-queued-buffers, maybe it's worth mentioning in the docs what is the size of a buffer?

I think it's not a constant number since it's a MIN(VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE, stop_copy_size)?
On the other hand, one could say it's "at most VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE".

> Thanks.
Thanks,
Maciej

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Avihai Horon]

On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
> External email: Use caution opening links or attachments
>
>
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> This property allows configuring whether to start the config load only
> after all iterables were loaded.

Nit: maybe, to be more accurate, mention that it is loaded as part of 
the non-iterables:
This property allows configuring whether to start the config load only 
after all iterables were loaded, during non-iterables loading phase.
(We can also mention this in docs/code comment)

Anyway:

Reviewed-by: Avihai Horon <avihaih@nvidia.com>

> Such interlocking is required for ARM64 due to this platform VFIO
> dependency on interrupt controller being loaded first.
>
> The property defaults to AUTO, which means ON for ARM, OFF for other
> platforms.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>   docs/devel/migration/vfio.rst     |  6 +++
>   hw/core/machine.c                 |  1 +
>   hw/vfio/migration-multifd.c       | 88 +++++++++++++++++++++++++++++++
>   hw/vfio/migration-multifd.h       |  3 ++
>   hw/vfio/migration.c               | 10 +++-
>   hw/vfio/pci.c                     |  9 ++++
>   hw/vfio/vfio-migration-internal.h |  1 +
>   include/hw/vfio/vfio-device.h     |  1 +
>   8 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
> index f4a6bfa4619b..7c9cb7bdbf87 100644
> --- a/docs/devel/migration/vfio.rst
> +++ b/docs/devel/migration/vfio.rst
> @@ -260,3 +260,9 @@ Because a malicious QEMU source causing OOM on the target is not expected to be
>   a realistic threat in most of VFIO live migration use cases and the right value
>   depends on the particular setup by default this queued buffers limit is
>   disabled by setting it to UINT64_MAX.
> +
> +Some host platforms (like ARM64) require that VFIO device config is loaded only
> +after all iterables were loaded.
> +Such interlocking is controlled by "x-migration-load-config-after-iter" VFIO
> +device property, which in its default setting (AUTO) does so only on platforms
> +that actually require it.
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e869821b2246..16640b700f2e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -39,6 +39,7 @@
>
>   GlobalProperty hw_compat_10_0[] = {
>       { "scsi-hd", "dpofua", "off" },
> +    { "vfio-pci", "x-migration-load-config-after-iter", "off" },
>   };
>   const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
>
> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
> index f26c112090b4..a12ec1ead74a 100644
> --- a/hw/vfio/migration-multifd.c
> +++ b/hw/vfio/migration-multifd.c
> @@ -17,6 +17,7 @@
>   #include "qemu/error-report.h"
>   #include "qemu/lockable.h"
>   #include "qemu/main-loop.h"
> +#include "qemu/target-info.h"
>   #include "qemu/thread.h"
>   #include "io/channel-buffer.h"
>   #include "migration/qemu-file.h"
> @@ -35,6 +36,27 @@ typedef struct VFIODeviceStatePacket {
>       uint8_t data[0];
>   } QEMU_PACKED VFIODeviceStatePacket;
>
> +bool vfio_load_config_after_iter(VFIODevice *vbasedev)
> +{
> +    if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
> +        return true;
> +    } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
> +        return false;
> +    }
> +
> +    assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
> +
> +    /*
> +     * Starting the config load only after all iterables were loaded is required
> +     * for ARM64 due to this platform VFIO dependency on interrupt controller
> +     * being loaded first.
> +     *
> +     * See commit d329f5032e17 ("vfio: Move the saving of the config space to
> +     * the right place in VFIO migration").
> +     */
> +    return strcmp(target_name(), "aarch64") == 0;
> +}
> +
>   /* type safety */
>   typedef struct VFIOStateBuffers {
>       GArray *array;
> @@ -50,6 +72,9 @@ typedef struct VFIOMultifd {
>       bool load_bufs_thread_running;
>       bool load_bufs_thread_want_exit;
>
> +    bool load_bufs_iter_done;
> +    QemuCond load_bufs_iter_done_cond;
> +
>       VFIOStateBuffers load_bufs;
>       QemuCond load_bufs_buffer_ready_cond;
>       QemuCond load_bufs_thread_finished_cond;
> @@ -409,6 +434,22 @@ static bool vfio_load_bufs_thread(void *opaque, bool *should_quit, Error **errp)
>           multifd->load_buf_idx++;
>       }
>
> +    if (vfio_load_config_after_iter(vbasedev)) {
> +        while (!multifd->load_bufs_iter_done) {
> +            qemu_cond_wait(&multifd->load_bufs_iter_done_cond,
> +                           &multifd->load_bufs_mutex);
> +
> +            /*
> +             * Need to re-check cancellation immediately after wait in case
> +             * cond was signalled by vfio_load_cleanup_load_bufs_thread().
> +             */
> +            if (vfio_load_bufs_thread_want_exit(multifd, should_quit)) {
> +                error_setg(errp, "operation cancelled");
> +                goto thread_exit;
> +            }
> +        }
> +    }
> +
>       if (!vfio_load_bufs_thread_load_config(vbasedev, errp)) {
>           goto thread_exit;
>       }
> @@ -428,6 +469,48 @@ thread_exit:
>       return ret;
>   }
>
> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +    VFIOMultifd *multifd = migration->multifd;
> +    int ret = 0;
> +
> +    if (!vfio_multifd_transfer_enabled(vbasedev)) {
> +        error_report("%s: got DEV_CONFIG_LOAD_READY outside multifd transfer",
> +                     vbasedev->name);
> +        return -EINVAL;
> +    }
> +
> +    if (!vfio_load_config_after_iter(vbasedev)) {
> +        error_report("%s: got DEV_CONFIG_LOAD_READY but was disabled",
> +                     vbasedev->name);
> +        return -EINVAL;
> +    }
> +
> +    assert(multifd);
> +
> +    /* The lock order is load_bufs_mutex -> BQL so unlock BQL here first */
> +    bql_unlock();
> +    WITH_QEMU_LOCK_GUARD(&multifd->load_bufs_mutex) {
> +        if (multifd->load_bufs_iter_done) {
> +            /* Can't print error here as we're outside BQL */
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        multifd->load_bufs_iter_done = true;
> +        qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
> +    }
> +    bql_lock();
> +
> +    if (ret) {
> +        error_report("%s: duplicate DEV_CONFIG_LOAD_READY",
> +                     vbasedev->name);
> +    }
> +
> +    return ret;
> +}
> +
>   static VFIOMultifd *vfio_multifd_new(void)
>   {
>       VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
> @@ -441,6 +524,9 @@ static VFIOMultifd *vfio_multifd_new(void)
>       multifd->load_buf_queued_pending_buffers = 0;
>       qemu_cond_init(&multifd->load_bufs_buffer_ready_cond);
>
> +    multifd->load_bufs_iter_done = false;
> +    qemu_cond_init(&multifd->load_bufs_iter_done_cond);
> +
>       multifd->load_bufs_thread_running = false;
>       multifd->load_bufs_thread_want_exit = false;
>       qemu_cond_init(&multifd->load_bufs_thread_finished_cond);
> @@ -464,6 +550,7 @@ static void vfio_load_cleanup_load_bufs_thread(VFIOMultifd *multifd)
>               multifd->load_bufs_thread_want_exit = true;
>
>               qemu_cond_signal(&multifd->load_bufs_buffer_ready_cond);
> +            qemu_cond_signal(&multifd->load_bufs_iter_done_cond);
>               qemu_cond_wait(&multifd->load_bufs_thread_finished_cond,
>                              &multifd->load_bufs_mutex);
>           }
> @@ -476,6 +563,7 @@ static void vfio_multifd_free(VFIOMultifd *multifd)
>       vfio_load_cleanup_load_bufs_thread(multifd);
>
>       qemu_cond_destroy(&multifd->load_bufs_thread_finished_cond);
> +    qemu_cond_destroy(&multifd->load_bufs_iter_done_cond);
>       vfio_state_buffers_destroy(&multifd->load_bufs);
>       qemu_cond_destroy(&multifd->load_bufs_buffer_ready_cond);
>       qemu_mutex_destroy(&multifd->load_bufs_mutex);
> diff --git a/hw/vfio/migration-multifd.h b/hw/vfio/migration-multifd.h
> index 0bab63211d30..487f457282df 100644
> --- a/hw/vfio/migration-multifd.h
> +++ b/hw/vfio/migration-multifd.h
> @@ -20,9 +20,12 @@ void vfio_multifd_cleanup(VFIODevice *vbasedev);
>   bool vfio_multifd_transfer_supported(void);
>   bool vfio_multifd_transfer_enabled(VFIODevice *vbasedev);
>
> +bool vfio_load_config_after_iter(VFIODevice *vbasedev);
>   bool vfio_multifd_load_state_buffer(void *opaque, char *data, size_t data_size,
>                                       Error **errp);
>
> +int vfio_load_state_config_load_ready(VFIODevice *vbasedev);
> +
>   void vfio_multifd_emit_dummy_eos(VFIODevice *vbasedev, QEMUFile *f);
>
>   bool
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index b76697bd1a23..7c6436d4c344 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -675,7 +675,11 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>       int ret;
>
>       if (vfio_multifd_transfer_enabled(vbasedev)) {
> -        vfio_multifd_emit_dummy_eos(vbasedev, f);
> +        if (vfio_load_config_after_iter(vbasedev)) {
> +            qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY);
> +        } else {
> +            vfio_multifd_emit_dummy_eos(vbasedev, f);
> +        }
>           return;
>       }
>
> @@ -784,6 +788,10 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
>
>               return ret;
>           }
> +        case VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY:
> +        {
> +            return vfio_load_state_config_load_ready(vbasedev);
> +        }
>           default:
>               error_report("%s: Unknown tag 0x%"PRIx64, vbasedev->name, data);
>               return -EINVAL;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2765a39f9df1..01e48e39de75 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3524,6 +3524,9 @@ static const Property vfio_pci_dev_properties[] = {
>                   vbasedev.migration_multifd_transfer,
>                   vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
>                   .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("x-migration-load-config-after-iter", VFIOPCIDevice,
> +                            vbasedev.migration_load_config_after_iter,
> +                            ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_UINT64("x-migration-max-queued-buffers", VFIOPCIDevice,
>                          vbasedev.migration_max_queued_buffers, UINT64_MAX),
>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
> @@ -3700,6 +3703,12 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, const void *data)
>                                             "x-migration-multifd-transfer",
>                                             "Transfer this device state via "
>                                             "multifd channels when live migrating it");
> +    object_class_property_set_description(klass, /* 10.1 */
> +                                          "x-migration-load-config-after-iter",
> +                                          "Start the config load only after "
> +                                          "all iterables were loaded when doing "
> +                                          "live migration of device state via "
> +                                          "multifd channels");
>       object_class_property_set_description(klass, /* 10.1 */
>                                             "x-migration-max-queued-buffers",
>                                             "Maximum count of in-flight VFIO "
> diff --git a/hw/vfio/vfio-migration-internal.h b/hw/vfio/vfio-migration-internal.h
> index a8b456b239df..54141e27e6b2 100644
> --- a/hw/vfio/vfio-migration-internal.h
> +++ b/hw/vfio/vfio-migration-internal.h
> @@ -32,6 +32,7 @@
>   #define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
>   #define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)
>   #define VFIO_MIG_FLAG_DEV_INIT_DATA_SENT (0xffffffffef100005ULL)
> +#define VFIO_MIG_FLAG_DEV_CONFIG_LOAD_READY (0xffffffffef100006ULL)
>
>   typedef struct VFIODevice VFIODevice;
>   typedef struct VFIOMultifd VFIOMultifd;
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 0ee34aaf668b..359d553b916a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -66,6 +66,7 @@ typedef struct VFIODevice {
>       bool ram_block_discard_allowed;
>       OnOffAuto enable_migration;
>       OnOffAuto migration_multifd_transfer;
> +    OnOffAuto migration_load_config_after_iter;
>       uint64_t migration_max_queued_buffers;
>       bool migration_events;
>       bool use_region_fds;

Re: [PATCH 0/3] VFIO multifd device state transfer patches for QEMU 10.1

[Maciej S. Szmigiero]

On 24.06.2025 19:51, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> When VFIO multifd device state transfer support was merged in QEMU 10.0
> some of patches were separated for the future QEMU release after the
> spring cleanup.
> 
> Since QEMU 10.1 code freeze is getting closer let's have them reviewed
> now.
> 

Hey Cédric,

I see these patches in your "vfio-10.1" GitHub tree but I don't see them
as a pull request on the qemu-devel mailing list.

Are you going to post them? - the soft code freeze is tomorrow.

Thanks,
Maciej

Re: [PATCH 1/3] vfio/migration: Max in-flight VFIO device state buffer count limit

[Cédric Le Goater]

>> But do we really need both x-migration-max-queued-buffers and x-migration-max-queued-buffers-size?
>> I think one is sufficient.
>>
>> I vote for x-migration-max-queued-buffers-size as the actual memory limit won't change depending on VFIO migration buffer size.
> 
> If just one of these limits were to be implemented my vote would be also for the size limit rather than the count limit.

Let's keep the size limit only.

Thanks,

C.

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Cédric Le Goater]

On 7/8/25 10:34, Avihai Horon wrote:
> 
> On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> This property allows configuring whether to start the config load only
>> after all iterables were loaded.
> 
> Nit: maybe, to be more accurate, mention that it is loaded as part of the non-iterables:
> This property allows configuring whether to start the config load only after all iterables were loaded, during non-iterables loading phase.
> (We can also mention this in docs/code comment)
> 
> Anyway:
> 
> Reviewed-by: Avihai Horon <avihaih@nvidia.com>


Avihai,

Have you had a chance to test this series on ARM?

Thanks,

C.

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Avihai Horon]

On 15/07/2025 12:08, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 7/8/25 10:34, Avihai Horon wrote:
>>
>> On 24/06/2025 20:51, Maciej S. Szmigiero wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> This property allows configuring whether to start the config load only
>>> after all iterables were loaded.
>>
>> Nit: maybe, to be more accurate, mention that it is loaded as part of 
>> the non-iterables:
>> This property allows configuring whether to start the config load 
>> only after all iterables were loaded, during non-iterables loading 
>> phase.
>> (We can also mention this in docs/code comment)
>>
>> Anyway:
>>
>> Reviewed-by: Avihai Horon <avihaih@nvidia.com>
>
>
> Avihai,
>
> Have you had a chance to test this series on ARM?

Unfortunately no, I don't have such setup. I did test it on x86 though.

Re: [PATCH 2/3] vfio/migration: Add x-migration-load-config-after-iter VFIO property

[Cédric Le Goater]

>> Using 'strcmp(target_name(), "aarch64")' is quite unique in the QEMU code
>> base, and to be honest, I’m not too keen on adding it unless there’s really
>> no other option.
>
> The previous versions simply tested the TARGET_ARM macro but commit
> 5731baee6c3c ("hw/vfio: Compile some common objects once") made the
> migration-multifd.c file target-independent so it cannot use target-specific
> macros now.
> 
> Another option would be to move vfio_load_config_after_iter() to helpers.c
> since that file is target-dependent and can simply test TARGET_ARM
> macro (#if defined(TARGET_ARM)) instead of doing strcmp(target_name(), "aarch64")
> which I agree looks weird.

This would look better than testing target_name(). Let's do that.

Thanks,

C.

Re: [PATCH 0/3] VFIO multifd device state transfer patches for QEMU 10.1

[Cédric Le Goater]

On 7/14/25 11:46, Maciej S. Szmigiero wrote:
> On 24.06.2025 19:51, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> When VFIO multifd device state transfer support was merged in QEMU 10.0
>> some of patches were separated for the future QEMU release after the
>> spring cleanup.
>>
>> Since QEMU 10.1 code freeze is getting closer let's have them reviewed
>> now.
>>
> 
> Hey Cédric,
> 
> I see these patches in your "vfio-10.1" GitHub tree

vfio-x.y is the branch I use for vfio-next candidates.


> but I don't see them
> as a pull request on the qemu-devel mailing list.
> > Are you going to post them? - the soft code freeze is tomorrow.

We still have some time. Let's see how v2 looks like.

I would prefer to have some feedback from the virt-arm team or
a Tested-by at least.

Thanks,

C.