[PATCH 0/6] vfio/migration: Block VFIO migration with postcopy and background snapshot

[PATCH 0/6] vfio/migration: Block VFIO migration with postcopy and background snapshot

[Avihai Horon]

Hello,

Recently added VFIO migration is not compatible with some of the
pre-existing migration features. This was overlooked and today these
combinations are not blocked by QEMU. This series fixes it.

Postcopy migration:
VFIO migration is not compatible with postcopy migration. A VFIO device
in the destination can't handle page faults for pages that have not been
sent yet. Doing such migration will cause the VM to crash in the
destination.

Background snapshot:
Background snapshot allows creating a snapshot of the VM while it's
running and keeping it small by not including dirty RAM pages.

The way it works is by first stopping the VM, saving the non-iterable
devices' state and then starting the VM and saving the RAM while write
protecting it with UFFD. The resulting snapshot represents the VM state
at snapshot start.

VFIO migration is not compatible with background snapshot.
First of all, VFIO device state is not even saved in background snapshot
because only non-iterable device state is saved. But even if it was
saved, after starting the VM, a VFIO device could dirty pages without it
being detected by UFFD write protection. This would corrupt the
snapshot, as the RAM in it would not represent the RAM at snapshot
start.

This series blocks these combinations explicitly:
If a VFIO device is added when postcopy or background snapshot are on,
a migration blocker will be added. If a VFIO device is present, setting
postcopy or background snapshot capabilities will fail with an
appropriate error message.

Note that this series is based on the P2P series [1] sent a few weeks
ago.

Comments and suggestions will be greatly appreciated.

Thanks.

[1]
https://lore.kernel.org/qemu-devel/20230802081449.2528-1-avihaih@nvidia.com/

Avihai Horon (6):
  migration: Add migration prefix to functions in target.c
  vfio/migration: Fail adding device with enable-migration=on and
    existing blocker
  vfio/migration: Add vfio_migratable_devices_num()
  vfio/migration: Change vfio_mig_active() semantics
  vfio/migration: Block VFIO migration with postcopy migration
  vfio/migration: Block VFIO migration with background snapshot

 include/hw/vfio/vfio-common.h |   4 ++
 migration/migration.h         |   7 +-
 hw/vfio/common.c              | 120 +++++++++++++++++++++++++++++-----
 hw/vfio/migration.c           |  12 ++++
 migration/migration.c         |   6 +-
 migration/options.c           |  26 ++++++++
 migration/savevm.c            |   2 +-
 migration/target.c            |  36 ++++++++--
 8 files changed, 187 insertions(+), 26 deletions(-)

-- 
2.26.3

[PATCH 1/6] migration: Add migration prefix to functions in target.c

[Avihai Horon]

The functions in target.c are not static, yet they don't have a proper
migration prefix. Add such prefix.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.h | 4 ++--
 migration/migration.c | 6 +++---
 migration/savevm.c    | 2 +-
 migration/target.c    | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 6eea18db36..c5695de214 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -512,8 +512,8 @@ void migration_consume_urgent_request(void);
 bool migration_rate_limit(void);
 void migration_cancel(const Error *error);
 
-void populate_vfio_info(MigrationInfo *info);
-void reset_vfio_bytes_transferred(void);
+void migration_populate_vfio_info(MigrationInfo *info);
+void migration_reset_vfio_bytes_transferred(void);
 void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 5528acb65e..92866a8f49 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1039,7 +1039,7 @@ static void fill_source_migration_info(MigrationInfo *info)
         populate_time_info(info, s);
         populate_ram_info(info, s);
         populate_disk_info(info);
-        populate_vfio_info(info);
+        migration_populate_vfio_info(info);
         break;
     case MIGRATION_STATUS_COLO:
         info->has_status = true;
@@ -1048,7 +1048,7 @@ static void fill_source_migration_info(MigrationInfo *info)
     case MIGRATION_STATUS_COMPLETED:
         populate_time_info(info, s);
         populate_ram_info(info, s);
-        populate_vfio_info(info);
+        migration_populate_vfio_info(info);
         break;
     case MIGRATION_STATUS_FAILED:
         info->has_status = true;
@@ -1641,7 +1641,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
      */
     memset(&mig_stats, 0, sizeof(mig_stats));
     memset(&compression_counters, 0, sizeof(compression_counters));
-    reset_vfio_bytes_transferred();
+    migration_reset_vfio_bytes_transferred();
 
     return true;
 }
diff --git a/migration/savevm.c b/migration/savevm.c
index a2cb8855e2..5bf8b59a7d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1622,7 +1622,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     migrate_init(ms);
     memset(&mig_stats, 0, sizeof(mig_stats));
     memset(&compression_counters, 0, sizeof(compression_counters));
-    reset_vfio_bytes_transferred();
+    migration_reset_vfio_bytes_transferred();
     ms->to_dst_file = f;
 
     qemu_mutex_unlock_iothread();
diff --git a/migration/target.c b/migration/target.c
index f39c9a8d88..a6ffa9a5ce 100644
--- a/migration/target.c
+++ b/migration/target.c
@@ -15,7 +15,7 @@
 #endif
 
 #ifdef CONFIG_VFIO
-void populate_vfio_info(MigrationInfo *info)
+void migration_populate_vfio_info(MigrationInfo *info)
 {
     if (vfio_mig_active()) {
         info->vfio = g_malloc0(sizeof(*info->vfio));
@@ -23,16 +23,16 @@ void populate_vfio_info(MigrationInfo *info)
     }
 }
 
-void reset_vfio_bytes_transferred(void)
+void migration_reset_vfio_bytes_transferred(void)
 {
     vfio_reset_bytes_transferred();
 }
 #else
-void populate_vfio_info(MigrationInfo *info)
+void migration_populate_vfio_info(MigrationInfo *info)
 {
 }
 
-void reset_vfio_bytes_transferred(void)
+void migration_reset_vfio_bytes_transferred(void)
 {
 }
 #endif
-- 
2.26.3

[PATCH 2/6] vfio/migration: Fail adding device with enable-migration=on and existing blocker

[Avihai Horon]

If a device with enable-migration=on is added and it causes a migration
blocker, adding the device should fail with a proper error.

This is not the case with multiple device migration blocker when the
blocker already exists. If the blocker already exists and a device with
enable-migration=on is added which causes a migration blocker, adding
the device will succeed.

Fix it by failing adding the device in such case.

Fixes: 8bbcb64a71d8 ("vfio/migration: Make VFIO migration non-experimental")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/common.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8a8d074e18..237101d038 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -394,8 +394,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
 {
     int ret;
 
-    if (multiple_devices_migration_blocker ||
-        vfio_multiple_devices_migration_is_supported()) {
+    if (vfio_multiple_devices_migration_is_supported()) {
         return 0;
     }
 
@@ -405,6 +404,10 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
         return -EINVAL;
     }
 
+    if (multiple_devices_migration_blocker) {
+        return 0;
+    }
+
     error_setg(&multiple_devices_migration_blocker,
                "Multiple VFIO devices migration is supported only if all of "
                "them support P2P migration");
-- 
2.26.3

[PATCH 3/6] vfio/migration: Add vfio_migratable_devices_num()

[Avihai Horon]

Add vfio_migratable_devices_num() function, which returns the number of
VFIO devices that are using VFIO migration, and use it in
vfio_multiple_devices_migration_is_supported().

This is done in preparation for next patches which will block VFIO
migration with postcopy migration or background snapshot, as they are
not compatible together.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/common.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 237101d038..57a76feab1 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -363,6 +363,23 @@ bool vfio_mig_active(void)
 
 static Error *multiple_devices_migration_blocker;
 
+static unsigned int vfio_migratable_devices_num(void)
+{
+    VFIOGroup *group;
+    VFIODevice *vbasedev;
+    unsigned int device_num = 0;
+
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+        QLIST_FOREACH(vbasedev, &group->device_list, next) {
+            if (vbasedev->migration) {
+                device_num++;
+            }
+        }
+    }
+
+    return device_num;
+}
+
 /*
  * Multiple devices migration is allowed only if all devices support P2P
  * migration. Single device migration is allowed regardless of P2P migration
@@ -372,14 +389,11 @@ static bool vfio_multiple_devices_migration_is_supported(void)
 {
     VFIOGroup *group;
     VFIODevice *vbasedev;
-    unsigned int device_num = 0;
     bool all_support_p2p = true;
 
     QLIST_FOREACH(group, &vfio_group_list, next) {
         QLIST_FOREACH(vbasedev, &group->device_list, next) {
             if (vbasedev->migration) {
-                device_num++;
-
                 if (!(vbasedev->migration->mig_flags & VFIO_MIGRATION_P2P)) {
                     all_support_p2p = false;
                 }
@@ -387,7 +401,7 @@ static bool vfio_multiple_devices_migration_is_supported(void)
         }
     }
 
-    return all_support_p2p || device_num <= 1;
+    return all_support_p2p || vfio_migratable_devices_num() <= 1;
 }
 
 int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
-- 
2.26.3

[PATCH 4/6] vfio/migration: Change vfio_mig_active() semantics

[Avihai Horon]

vfio_mig_active() is used by migration_populate_vfio_info() to populate
VFIO migration info when it is active. Currently, VFIO migration is
considered active if there are VFIO devices and none of them has a
migration blocker.

Change that and consider VFIO migration to be active if there is a VFIO
device that is using VFIO migration, regardless of whether a device has
migration blocker or not.

This is done in preparation for next patches which will block VFIO
migration with postcopy migration or background snapshot, as they are
not compatible together. It will allow adding a migration blocker for
such cases even if the VFIO device already has a blocker.

Note that migration_populate_vfio_info() still behaves correctly, as if
there is a VFIO device with migration blocker, migration can't be
started and thus migration_populate_vfio_info() will never be called.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/common.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 57a76feab1..373f6e5932 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -342,25 +342,6 @@ static int vfio_bitmap_alloc(VFIOBitmap *vbmap, hwaddr size)
 static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
                                  uint64_t size, ram_addr_t ram_addr);
 
-bool vfio_mig_active(void)
-{
-    VFIOGroup *group;
-    VFIODevice *vbasedev;
-
-    if (QLIST_EMPTY(&vfio_group_list)) {
-        return false;
-    }
-
-    QLIST_FOREACH(group, &vfio_group_list, next) {
-        QLIST_FOREACH(vbasedev, &group->device_list, next) {
-            if (vbasedev->migration_blocker) {
-                return false;
-            }
-        }
-    }
-    return true;
-}
-
 static Error *multiple_devices_migration_blocker;
 
 static unsigned int vfio_migratable_devices_num(void)
@@ -446,6 +427,11 @@ void vfio_unblock_multiple_devices_migration(void)
     multiple_devices_migration_blocker = NULL;
 }
 
+bool vfio_mig_active(void)
+{
+    return vfio_migratable_devices_num();
+}
+
 bool vfio_viommu_preset(VFIODevice *vbasedev)
 {
     return vbasedev->group->container->space->as != &address_space_memory;
-- 
2.26.3

[PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

VFIO migration is not compatible with postcopy migration. A VFIO device
in the destination can't handle page faults for pages that have not been
sent yet.

Doing such migration will cause the VM to crash in the destination:

qemu-system-x86_64: VFIO_MAP_DMA failed: Bad address
qemu-system-x86_64: vfio_dma_map(0x55a28c7659d0, 0xc0000, 0xb000, 0x7f1b11a00000) = -14 (Bad address)
qemu: hardware error: vfio: DMA mapping failed, unable to continue

To prevent this and to be explicit about supported features, block VFIO
migration with postcopy migration: Fail setting postcopy capability if a
VFIO device is present, and add a migration blocker if a VFIO device is
added when postcopy capability is on.

Reported-by: Yanghang Liu <yanghliu@redhat.com>
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 include/hw/vfio/vfio-common.h |  2 ++
 migration/migration.h         |  2 ++
 hw/vfio/common.c              | 43 +++++++++++++++++++++++++++++++++++
 hw/vfio/migration.c           |  6 +++++
 migration/options.c           | 19 ++++++++++++++++
 migration/target.c            | 19 ++++++++++++++++
 6 files changed, 91 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index e9b8954595..c0b58f2bb7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -227,6 +227,8 @@ extern VFIOGroupList vfio_group_list;
 bool vfio_mig_active(void);
 int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_multiple_devices_migration(void);
+int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp);
+void vfio_unblock_postcopy_migration(void);
 bool vfio_viommu_preset(VFIODevice *vbasedev);
 int64_t vfio_mig_bytes_transferred(void);
 void vfio_reset_bytes_transferred(void);
diff --git a/migration/migration.h b/migration/migration.h
index c5695de214..21a6423408 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -514,6 +514,8 @@ void migration_cancel(const Error *error);
 
 void migration_populate_vfio_info(MigrationInfo *info);
 void migration_reset_vfio_bytes_transferred(void);
+bool migration_vfio_mig_active(void);
+void migration_vfio_unblock_postcopy_migration(void);
 void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
 
 #endif
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 373f6e5932..7461194b2b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -40,6 +40,7 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "migration/migration.h"
+#include "migration/options.h"
 #include "migration/misc.h"
 #include "migration/blocker.h"
 #include "migration/qemu-file.h"
@@ -343,6 +344,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
                                  uint64_t size, ram_addr_t ram_addr);
 
 static Error *multiple_devices_migration_blocker;
+static Error *postcopy_migration_blocker;
 
 static unsigned int vfio_migratable_devices_num(void)
 {
@@ -427,6 +429,47 @@ void vfio_unblock_multiple_devices_migration(void)
     multiple_devices_migration_blocker = NULL;
 }
 
+int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp)
+{
+    int ret;
+
+    if (!migrate_postcopy_ram()) {
+        return 0;
+    }
+
+    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
+        error_setg(errp,
+                   "VFIO migration is not compatible with postcopy migration");
+        return -EINVAL;
+    }
+
+    if (postcopy_migration_blocker) {
+        return 0;
+    }
+
+    error_setg(&postcopy_migration_blocker,
+               "VFIO migration is not compatible with postcopy migration");
+    ret = migrate_add_blocker(postcopy_migration_blocker, errp);
+    if (ret < 0) {
+        error_free(postcopy_migration_blocker);
+        postcopy_migration_blocker = NULL;
+    }
+
+    return ret;
+}
+
+void vfio_unblock_postcopy_migration(void)
+{
+    if (!postcopy_migration_blocker ||
+        (vfio_migratable_devices_num() && migrate_postcopy_ram())) {
+        return;
+    }
+
+    migrate_del_blocker(postcopy_migration_blocker);
+    error_free(postcopy_migration_blocker);
+    postcopy_migration_blocker = NULL;
+}
+
 bool vfio_mig_active(void)
 {
     return vfio_migratable_devices_num();
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 71855468fe..76406e9ae9 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -856,6 +856,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
     unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
     vfio_migration_free(vbasedev);
     vfio_unblock_multiple_devices_migration();
+    vfio_unblock_postcopy_migration();
 }
 
 static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
@@ -939,6 +940,11 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
         goto out_deinit;
     }
 
+    ret = vfio_block_postcopy_migration(vbasedev, errp);
+    if (ret) {
+        goto out_deinit;
+    }
+
     if (vfio_viommu_preset(vbasedev)) {
         error_setg(&err, "%s: Migration is currently not supported "
                    "with vIOMMU enabled", vbasedev->name);
diff --git a/migration/options.c b/migration/options.c
index 1d1e1321b0..e201053563 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
             error_setg(errp, "Postcopy is not yet compatible with multifd");
             return false;
         }
+
+        if (migration_vfio_mig_active()) {
+            error_setg(errp, "Postcopy is not compatible with VFIO migration");
+            return false;
+        }
     }
 
     if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
@@ -612,6 +617,16 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     return true;
 }
 
+/*
+ * Devices might have added migration blockers based on migration capabilities
+ * values when those devices were added. Remove such blockers according to new
+ * changes in migration capabilities.
+ */
+static void migration_caps_remove_blockers(void)
+{
+    migration_vfio_unblock_postcopy_migration();
+}
+
 bool migrate_cap_set(int cap, bool value, Error **errp)
 {
     MigrationState *s = migrate_get_current();
@@ -629,6 +644,8 @@ bool migrate_cap_set(int cap, bool value, Error **errp)
         return false;
     }
     s->capabilities[cap] = value;
+    migration_caps_remove_blockers();
+
     return true;
 }
 
@@ -678,6 +695,8 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     for (cap = params; cap; cap = cap->next) {
         s->capabilities[cap->value->capability] = cap->value->state;
     }
+
+    migration_caps_remove_blockers();
 }
 
 /* parameters */
diff --git a/migration/target.c b/migration/target.c
index a6ffa9a5ce..690ecb4dd5 100644
--- a/migration/target.c
+++ b/migration/target.c
@@ -27,6 +27,16 @@ void migration_reset_vfio_bytes_transferred(void)
 {
     vfio_reset_bytes_transferred();
 }
+
+bool migration_vfio_mig_active(void)
+{
+    return vfio_mig_active();
+}
+
+void migration_vfio_unblock_postcopy_migration(void)
+{
+    vfio_unblock_postcopy_migration();
+}
 #else
 void migration_populate_vfio_info(MigrationInfo *info)
 {
@@ -35,4 +45,13 @@ void migration_populate_vfio_info(MigrationInfo *info)
 void migration_reset_vfio_bytes_transferred(void)
 {
 }
+
+bool migration_vfio_mig_active(void)
+{
+    return false;
+}
+
+void migration_vfio_unblock_postcopy_migration()
+{
+}
 #endif
-- 
2.26.3

[PATCH 6/6] vfio/migration: Block VFIO migration with background snapshot

[Avihai Horon]

Background snapshot allows creating a snapshot of the VM while it's
running and keeping it small by not including dirty RAM pages.

The way it works is by first stopping the VM, saving the non-iterable
devices' state and then starting the VM and saving the RAM while write
protecting it with UFFD. The resulting snapshot represents the VM state
at snapshot start.

VFIO migration is not compatible with background snapshot.
First of all, VFIO device state is not even saved in background snapshot
because only non-iterable device state is saved. But even if it was
saved, after starting the VM, a VFIO device could dirty pages without it
being detected by UFFD write protection. This would corrupt the
snapshot, as the RAM in it would not represent the RAM at snapshot
start.

To prevent this and to be explicit about supported features, block VFIO
migration with background snapshot: Fail setting background snapshot
capability if a VFIO device is present, and add a migration blocker if a
VFIO device is added when background snapshot capability is on.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 include/hw/vfio/vfio-common.h |  2 ++
 migration/migration.h         |  1 +
 hw/vfio/common.c              | 42 +++++++++++++++++++++++++++++++++++
 hw/vfio/migration.c           |  6 +++++
 migration/options.c           |  7 ++++++
 migration/target.c            |  9 ++++++++
 6 files changed, 67 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index c0b58f2bb7..bb94f320f1 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -229,6 +229,8 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_multiple_devices_migration(void);
 int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_postcopy_migration(void);
+int vfio_block_background_snapshot(VFIODevice *vbasedev, Error **errp);
+void vfio_unblock_background_snapshot(void);
 bool vfio_viommu_preset(VFIODevice *vbasedev);
 int64_t vfio_mig_bytes_transferred(void);
 void vfio_reset_bytes_transferred(void);
diff --git a/migration/migration.h b/migration/migration.h
index 21a6423408..3077ed430b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -516,6 +516,7 @@ void migration_populate_vfio_info(MigrationInfo *info);
 void migration_reset_vfio_bytes_transferred(void);
 bool migration_vfio_mig_active(void);
 void migration_vfio_unblock_postcopy_migration(void);
+void migration_vfio_unblock_background_snapshot(void);
 void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
 
 #endif
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7461194b2b..4f6bc40cc0 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -345,6 +345,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
 
 static Error *multiple_devices_migration_blocker;
 static Error *postcopy_migration_blocker;
+static Error *background_snapshot_blocker;
 
 static unsigned int vfio_migratable_devices_num(void)
 {
@@ -470,6 +471,47 @@ void vfio_unblock_postcopy_migration(void)
     postcopy_migration_blocker = NULL;
 }
 
+int vfio_block_background_snapshot(VFIODevice *vbasedev, Error **errp)
+{
+    int ret;
+
+    if (!migrate_background_snapshot()) {
+        return 0;
+    }
+
+    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
+        error_setg(errp,
+                   "VFIO migration is not compatible with background snapshot");
+        return -EINVAL;
+    }
+
+    if (background_snapshot_blocker) {
+        return 0;
+    }
+
+    error_setg(&background_snapshot_blocker,
+               "VFIO migration is not compatible with background snapshot");
+    ret = migrate_add_blocker(background_snapshot_blocker, errp);
+    if (ret < 0) {
+        error_free(background_snapshot_blocker);
+        background_snapshot_blocker = NULL;
+    }
+
+    return ret;
+}
+
+void vfio_unblock_background_snapshot(void)
+{
+    if (!background_snapshot_blocker ||
+        (vfio_migratable_devices_num() && migrate_background_snapshot())) {
+        return;
+    }
+
+    migrate_del_blocker(background_snapshot_blocker);
+    error_free(background_snapshot_blocker);
+    background_snapshot_blocker = NULL;
+}
+
 bool vfio_mig_active(void)
 {
     return vfio_migratable_devices_num();
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 76406e9ae9..adf98ac8e3 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -857,6 +857,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
     vfio_migration_free(vbasedev);
     vfio_unblock_multiple_devices_migration();
     vfio_unblock_postcopy_migration();
+    vfio_unblock_background_snapshot();
 }
 
 static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
@@ -945,6 +946,11 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
         goto out_deinit;
     }
 
+    ret = vfio_block_background_snapshot(vbasedev, errp);
+    if (ret) {
+        goto out_deinit;
+    }
+
     if (vfio_viommu_preset(vbasedev)) {
         error_setg(&err, "%s: Migration is currently not supported "
                    "with vIOMMU enabled", vbasedev->name);
diff --git a/migration/options.c b/migration/options.c
index e201053563..2e13363de6 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -537,6 +537,12 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
                 return false;
             }
         }
+
+        if (migration_vfio_mig_active()) {
+            error_setg(errp, "Background-snapshot is not compatible with VFIO "
+                             "migration");
+            return false;
+        }
     }
 
 #ifdef CONFIG_LINUX
@@ -625,6 +631,7 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
 static void migration_caps_remove_blockers(void)
 {
     migration_vfio_unblock_postcopy_migration();
+    migration_vfio_unblock_background_snapshot();
 }
 
 bool migrate_cap_set(int cap, bool value, Error **errp)
diff --git a/migration/target.c b/migration/target.c
index 690ecb4dd5..c2be0b39db 100644
--- a/migration/target.c
+++ b/migration/target.c
@@ -37,6 +37,11 @@ void migration_vfio_unblock_postcopy_migration(void)
 {
     vfio_unblock_postcopy_migration();
 }
+
+void migration_vfio_unblock_background_snapshot(void)
+{
+    vfio_unblock_background_snapshot();
+}
 #else
 void migration_populate_vfio_info(MigrationInfo *info)
 {
@@ -54,4 +59,8 @@ bool migration_vfio_mig_active(void)
 void migration_vfio_unblock_postcopy_migration()
 {
 }
+
+void migration_vfio_unblock_background_snapshot(void)
+{
+}
 #endif
-- 
2.26.3

Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c

[Cédric Le Goater]

On 8/28/23 17:18, Avihai Horon wrote:
> The functions in target.c are not static, yet they don't have a proper
> migration prefix. Add such prefix.



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   migration/migration.h | 4 ++--
>   migration/migration.c | 6 +++---
>   migration/savevm.c    | 2 +-
>   migration/target.c    | 8 ++++----
>   4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 6eea18db36..c5695de214 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -512,8 +512,8 @@ void migration_consume_urgent_request(void);
>   bool migration_rate_limit(void);
>   void migration_cancel(const Error *error);
>   
> -void populate_vfio_info(MigrationInfo *info);
> -void reset_vfio_bytes_transferred(void);
> +void migration_populate_vfio_info(MigrationInfo *info);
> +void migration_reset_vfio_bytes_transferred(void);
>   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>   
>   #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 5528acb65e..92866a8f49 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1039,7 +1039,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>           populate_time_info(info, s);
>           populate_ram_info(info, s);
>           populate_disk_info(info);
> -        populate_vfio_info(info);
> +        migration_populate_vfio_info(info);
>           break;
>       case MIGRATION_STATUS_COLO:
>           info->has_status = true;
> @@ -1048,7 +1048,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>       case MIGRATION_STATUS_COMPLETED:
>           populate_time_info(info, s);
>           populate_ram_info(info, s);
> -        populate_vfio_info(info);
> +        migration_populate_vfio_info(info);
>           break;
>       case MIGRATION_STATUS_FAILED:
>           info->has_status = true;
> @@ -1641,7 +1641,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>        */
>       memset(&mig_stats, 0, sizeof(mig_stats));
>       memset(&compression_counters, 0, sizeof(compression_counters));
> -    reset_vfio_bytes_transferred();
> +    migration_reset_vfio_bytes_transferred();
>   
>       return true;
>   }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a2cb8855e2..5bf8b59a7d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1622,7 +1622,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>       migrate_init(ms);
>       memset(&mig_stats, 0, sizeof(mig_stats));
>       memset(&compression_counters, 0, sizeof(compression_counters));
> -    reset_vfio_bytes_transferred();
> +    migration_reset_vfio_bytes_transferred();
>       ms->to_dst_file = f;
>   
>       qemu_mutex_unlock_iothread();
> diff --git a/migration/target.c b/migration/target.c
> index f39c9a8d88..a6ffa9a5ce 100644
> --- a/migration/target.c
> +++ b/migration/target.c
> @@ -15,7 +15,7 @@
>   #endif
>   
>   #ifdef CONFIG_VFIO
> -void populate_vfio_info(MigrationInfo *info)
> +void migration_populate_vfio_info(MigrationInfo *info)
>   {
>       if (vfio_mig_active()) {
>           info->vfio = g_malloc0(sizeof(*info->vfio));
> @@ -23,16 +23,16 @@ void populate_vfio_info(MigrationInfo *info)
>       }
>   }
>   
> -void reset_vfio_bytes_transferred(void)
> +void migration_reset_vfio_bytes_transferred(void)
>   {
>       vfio_reset_bytes_transferred();
>   }
>   #else
> -void populate_vfio_info(MigrationInfo *info)
> +void migration_populate_vfio_info(MigrationInfo *info)
>   {
>   }
>   
> -void reset_vfio_bytes_transferred(void)
> +void migration_reset_vfio_bytes_transferred(void)
>   {
>   }
>   #endif

Re: [PATCH 2/6] vfio/migration: Fail adding device with enable-migration=on and existing blocker

[Cédric Le Goater]

On 8/28/23 17:18, Avihai Horon wrote:
> If a device with enable-migration=on is added and it causes a migration
> blocker, adding the device should fail with a proper error.
> 
> This is not the case with multiple device migration blocker when the
> blocker already exists. If the blocker already exists and a device with
> enable-migration=on is added which causes a migration blocker, adding
> the device will succeed.
> 
> Fix it by failing adding the device in such case.


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> 
> Fixes: 8bbcb64a71d8 ("vfio/migration: Make VFIO migration non-experimental")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   hw/vfio/common.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 8a8d074e18..237101d038 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -394,8 +394,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>   {
>       int ret;
>   
> -    if (multiple_devices_migration_blocker ||
> -        vfio_multiple_devices_migration_is_supported()) {
> +    if (vfio_multiple_devices_migration_is_supported()) {
>           return 0;
>       }
>   
> @@ -405,6 +404,10 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>           return -EINVAL;
>       }
>   
> +    if (multiple_devices_migration_blocker) {
> +        return 0;
> +    }
> +
>       error_setg(&multiple_devices_migration_blocker,
>                  "Multiple VFIO devices migration is supported only if all of "
>                  "them support P2P migration");

Re: [PATCH 3/6] vfio/migration: Add vfio_migratable_devices_num()

[Cédric Le Goater]

On 8/28/23 17:18, Avihai Horon wrote:
> Add vfio_migratable_devices_num() function, which returns the number of
> VFIO devices that are using VFIO migration, and use it in
> vfio_multiple_devices_migration_is_supported().
> 
> This is done in preparation for next patches which will block VFIO
> migration with postcopy migration or background snapshot, as they are
> not compatible together.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/common.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 237101d038..57a76feab1 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -363,6 +363,23 @@ bool vfio_mig_active(void)
>   
>   static Error *multiple_devices_migration_blocker;
>   
> +static unsigned int vfio_migratable_devices_num(void)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +    unsigned int device_num = 0;
> +
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (vbasedev->migration) {
> +                device_num++;
> +            }
> +        }
> +    }
> +
> +    return device_num;
> +}
> +
>   /*
>    * Multiple devices migration is allowed only if all devices support P2P
>    * migration. Single device migration is allowed regardless of P2P migration
> @@ -372,14 +389,11 @@ static bool vfio_multiple_devices_migration_is_supported(void)
>   {
>       VFIOGroup *group;
>       VFIODevice *vbasedev;
> -    unsigned int device_num = 0;
>       bool all_support_p2p = true;
>   
>       QLIST_FOREACH(group, &vfio_group_list, next) {
>           QLIST_FOREACH(vbasedev, &group->device_list, next) {
>               if (vbasedev->migration) {
> -                device_num++;
> -
>                   if (!(vbasedev->migration->mig_flags & VFIO_MIGRATION_P2P)) {
>                       all_support_p2p = false;
>                   }
> @@ -387,7 +401,7 @@ static bool vfio_multiple_devices_migration_is_supported(void)
>           }
>       }
>   
> -    return all_support_p2p || device_num <= 1;
> +    return all_support_p2p || vfio_migratable_devices_num() <= 1;
>   }
>   
>   int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Cédric Le Goater]

On 8/28/23 17:18, Avihai Horon wrote:
> VFIO migration is not compatible with postcopy migration. A VFIO device
> in the destination can't handle page faults for pages that have not been
> sent yet.
> 
> Doing such migration will cause the VM to crash in the destination:
> 
> qemu-system-x86_64: VFIO_MAP_DMA failed: Bad address
> qemu-system-x86_64: vfio_dma_map(0x55a28c7659d0, 0xc0000, 0xb000, 0x7f1b11a00000) = -14 (Bad address)
> qemu: hardware error: vfio: DMA mapping failed, unable to continue
> 
> To prevent this and to be explicit about supported features, block VFIO
> migration with postcopy migration: Fail setting postcopy capability if a
> VFIO device is present, and add a migration blocker if a VFIO device is
> added when postcopy capability is on.
> 
> Reported-by: Yanghang Liu <yanghliu@redhat.com>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   include/hw/vfio/vfio-common.h |  2 ++
>   migration/migration.h         |  2 ++
>   hw/vfio/common.c              | 43 +++++++++++++++++++++++++++++++++++
>   hw/vfio/migration.c           |  6 +++++
>   migration/options.c           | 19 ++++++++++++++++
>   migration/target.c            | 19 ++++++++++++++++
>   6 files changed, 91 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index e9b8954595..c0b58f2bb7 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -227,6 +227,8 @@ extern VFIOGroupList vfio_group_list;
>   bool vfio_mig_active(void);
>   int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
>   void vfio_unblock_multiple_devices_migration(void);
> +int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp);
> +void vfio_unblock_postcopy_migration(void);
>   bool vfio_viommu_preset(VFIODevice *vbasedev);
>   int64_t vfio_mig_bytes_transferred(void);
>   void vfio_reset_bytes_transferred(void);
> diff --git a/migration/migration.h b/migration/migration.h
> index c5695de214..21a6423408 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -514,6 +514,8 @@ void migration_cancel(const Error *error);
>   
>   void migration_populate_vfio_info(MigrationInfo *info);
>   void migration_reset_vfio_bytes_transferred(void);
> +bool migration_vfio_mig_active(void);
> +void migration_vfio_unblock_postcopy_migration(void);
>   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>   
>   #endif
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 373f6e5932..7461194b2b 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -40,6 +40,7 @@
>   #include "trace.h"
>   #include "qapi/error.h"
>   #include "migration/migration.h"
> +#include "migration/options.h"
>   #include "migration/misc.h"
>   #include "migration/blocker.h"
>   #include "migration/qemu-file.h"
> @@ -343,6 +344,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>                                    uint64_t size, ram_addr_t ram_addr);
>   
>   static Error *multiple_devices_migration_blocker;
> +static Error *postcopy_migration_blocker;
>   
>   static unsigned int vfio_migratable_devices_num(void)
>   {
> @@ -427,6 +429,47 @@ void vfio_unblock_multiple_devices_migration(void)
>       multiple_devices_migration_blocker = NULL;
>   }
>   
> +int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp)
> +{
> +    int ret;
> +
> +    if (!migrate_postcopy_ram()) {
> +        return 0;
> +    }
> +
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
> +        error_setg(errp,
> +                   "VFIO migration is not compatible with postcopy migration");
> +        return -EINVAL;
> +    }
> +
> +    if (postcopy_migration_blocker) {
> +        return 0;
> +    }
> +
> +    error_setg(&postcopy_migration_blocker,
> +               "VFIO migration is not compatible with postcopy migration");
> +    ret = migrate_add_blocker(postcopy_migration_blocker, errp);
> +    if (ret < 0) {
> +        error_free(postcopy_migration_blocker);
> +        postcopy_migration_blocker = NULL;
> +    }
> +
> +    return ret;
> +}
> +
> +void vfio_unblock_postcopy_migration(void)
> +{
> +    if (!postcopy_migration_blocker ||
> +        (vfio_migratable_devices_num() && migrate_postcopy_ram())) {
> +        return;
> +    }
> +
> +    migrate_del_blocker(postcopy_migration_blocker);
> +    error_free(postcopy_migration_blocker);
> +    postcopy_migration_blocker = NULL;
> +}
> +
>   bool vfio_mig_active(void)
>   {
>       return vfio_migratable_devices_num();
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 71855468fe..76406e9ae9 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -856,6 +856,7 @@ static void vfio_migration_deinit(VFIODevice *vbasedev)
>       unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
>       vfio_migration_free(vbasedev);
>       vfio_unblock_multiple_devices_migration();
> +    vfio_unblock_postcopy_migration();
>   }
>   
>   static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
> @@ -939,6 +940,11 @@ bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>           goto out_deinit;
>       }
>   
> +    ret = vfio_block_postcopy_migration(vbasedev, errp);
> +    if (ret) {
> +        goto out_deinit;
> +    }
> +
>       if (vfio_viommu_preset(vbasedev)) {
>           error_setg(&err, "%s: Migration is currently not supported "
>                      "with vIOMMU enabled", vbasedev->name);
> diff --git a/migration/options.c b/migration/options.c
> index 1d1e1321b0..e201053563 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>               error_setg(errp, "Postcopy is not yet compatible with multifd");
>               return false;
>           }
> +
> +        if (migration_vfio_mig_active()) {
> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
> +            return false;
> +        }
>       }
>   
>       if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> @@ -612,6 +617,16 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>       return true;
>   }
>   
> +/*
> + * Devices might have added migration blockers based on migration capabilities
> + * values when those devices were added. Remove such blockers according to new
> + * changes in migration capabilities.
> + */
> +static void migration_caps_remove_blockers(void)
> +{
> +    migration_vfio_unblock_postcopy_migration();
> +}
> +
>   bool migrate_cap_set(int cap, bool value, Error **errp)
>   {
>       MigrationState *s = migrate_get_current();
> @@ -629,6 +644,8 @@ bool migrate_cap_set(int cap, bool value, Error **errp)
>           return false;
>       }
>       s->capabilities[cap] = value;
> +    migration_caps_remove_blockers();
> +
>       return true;
>   }
>   
> @@ -678,6 +695,8 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>       for (cap = params; cap; cap = cap->next) {
>           s->capabilities[cap->value->capability] = cap->value->state;
>       }
> +
> +    migration_caps_remove_blockers();
>   }
>   
>   /* parameters */
> diff --git a/migration/target.c b/migration/target.c
> index a6ffa9a5ce..690ecb4dd5 100644
> --- a/migration/target.c
> +++ b/migration/target.c
> @@ -27,6 +27,16 @@ void migration_reset_vfio_bytes_transferred(void)
>   {
>       vfio_reset_bytes_transferred();
>   }
> +
> +bool migration_vfio_mig_active(void)
> +{
> +    return vfio_mig_active();
> +}
> +
> +void migration_vfio_unblock_postcopy_migration(void)
> +{
> +    vfio_unblock_postcopy_migration();
> +}
>   #else
>   void migration_populate_vfio_info(MigrationInfo *info)
>   {
> @@ -35,4 +45,13 @@ void migration_populate_vfio_info(MigrationInfo *info)
>   void migration_reset_vfio_bytes_transferred(void)
>   {
>   }
> +
> +bool migration_vfio_mig_active(void)
> +{
> +    return false;
> +}
> +
> +void migration_vfio_unblock_postcopy_migration()


Missing 'void' above.

Thanks,

C.


> +{
> +}
>   #endif

Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c

[Peter Xu]

On Mon, Aug 28, 2023 at 06:18:37PM +0300, Avihai Horon wrote:
> The functions in target.c are not static, yet they don't have a proper
> migration prefix. Add such prefix.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

No issue on the patch itself, but just noticed that we have hard-coded vfio
calls in migration paths.. that's slightly unfortunate. :(

> ---
>  migration/migration.h | 4 ++--
>  migration/migration.c | 6 +++---
>  migration/savevm.c    | 2 +-
>  migration/target.c    | 8 ++++----
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 6eea18db36..c5695de214 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -512,8 +512,8 @@ void migration_consume_urgent_request(void);
>  bool migration_rate_limit(void);
>  void migration_cancel(const Error *error);
>  
> -void populate_vfio_info(MigrationInfo *info);
> -void reset_vfio_bytes_transferred(void);
> +void migration_populate_vfio_info(MigrationInfo *info);
> +void migration_reset_vfio_bytes_transferred(void);
>  void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>  
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 5528acb65e..92866a8f49 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1039,7 +1039,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>          populate_time_info(info, s);
>          populate_ram_info(info, s);
>          populate_disk_info(info);
> -        populate_vfio_info(info);
> +        migration_populate_vfio_info(info);

(maybe in the future we should have SaveVMHandlers hooks for populating
 data for ram/disk/vfio/..., or some other way to not hard-code these)

>          break;
>      case MIGRATION_STATUS_COLO:
>          info->has_status = true;
> @@ -1048,7 +1048,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_COMPLETED:
>          populate_time_info(info, s);
>          populate_ram_info(info, s);
> -        populate_vfio_info(info);
> +        migration_populate_vfio_info(info);
>          break;
>      case MIGRATION_STATUS_FAILED:
>          info->has_status = true;
> @@ -1641,7 +1641,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>       */
>      memset(&mig_stats, 0, sizeof(mig_stats));
>      memset(&compression_counters, 0, sizeof(compression_counters));
> -    reset_vfio_bytes_transferred();
> +    migration_reset_vfio_bytes_transferred();

Could this already be done during vfio_save_setup(), to avoid calling an
vfio function directly in migration.c?

Again, not a request for this patchset, but more to see whether it'll work
to be moved there.

Thanks,

>  
>      return true;
>  }

-- 
Peter Xu

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Peter Xu]

On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
> diff --git a/migration/options.c b/migration/options.c
> index 1d1e1321b0..e201053563 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>              error_setg(errp, "Postcopy is not yet compatible with multifd");
>              return false;
>          }
> +
> +        if (migration_vfio_mig_active()) {
> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
> +            return false;
> +        }

Hmm.. this will add yet another vfio hard-coded line into migration/..

What will happen if the vfio device is hot plugged after enabling
postcopy-ram here?

Is it possible to do it in a generic way?

I was thinking the only unified place to do such check is when migration
starts, as long as we switch to SETUP all caps are locked and doesn't allow
any change until it finishes or fails.

So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
to fail the whole migration early?  For example, maybe we should have an
Error** passed in, then if it fails it calls migrate_set_error, so
reflected in query-migrate later too.

Thanks,

>      }
>  
>      if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> @@ -612,6 +617,16 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>      return true;
>  }

-- 
Peter Xu

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 29/08/2023 16:24, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/28/23 17:18, Avihai Horon wrote:
>> VFIO migration is not compatible with postcopy migration. A VFIO device
>> in the destination can't handle page faults for pages that have not been
>> sent yet.
>>
>> Doing such migration will cause the VM to crash in the destination:
>>
>> qemu-system-x86_64: VFIO_MAP_DMA failed: Bad address
>> qemu-system-x86_64: vfio_dma_map(0x55a28c7659d0, 0xc0000, 0xb000, 
>> 0x7f1b11a00000) = -14 (Bad address)
>> qemu: hardware error: vfio: DMA mapping failed, unable to continue
>>
>> To prevent this and to be explicit about supported features, block VFIO
>> migration with postcopy migration: Fail setting postcopy capability if a
>> VFIO device is present, and add a migration blocker if a VFIO device is
>> added when postcopy capability is on.
>>
>> Reported-by: Yanghang Liu <yanghliu@redhat.com>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   include/hw/vfio/vfio-common.h |  2 ++
>>   migration/migration.h         |  2 ++
>>   hw/vfio/common.c              | 43 +++++++++++++++++++++++++++++++++++
>>   hw/vfio/migration.c           |  6 +++++
>>   migration/options.c           | 19 ++++++++++++++++
>>   migration/target.c            | 19 ++++++++++++++++
>>   6 files changed, 91 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h 
>> b/include/hw/vfio/vfio-common.h
>> index e9b8954595..c0b58f2bb7 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -227,6 +227,8 @@ extern VFIOGroupList vfio_group_list;
>>   bool vfio_mig_active(void);
>>   int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, 
>> Error **errp);
>>   void vfio_unblock_multiple_devices_migration(void);
>> +int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp);
>> +void vfio_unblock_postcopy_migration(void);
>>   bool vfio_viommu_preset(VFIODevice *vbasedev);
>>   int64_t vfio_mig_bytes_transferred(void);
>>   void vfio_reset_bytes_transferred(void);
>> diff --git a/migration/migration.h b/migration/migration.h
>> index c5695de214..21a6423408 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -514,6 +514,8 @@ void migration_cancel(const Error *error);
>>
>>   void migration_populate_vfio_info(MigrationInfo *info);
>>   void migration_reset_vfio_bytes_transferred(void);
>> +bool migration_vfio_mig_active(void);
>> +void migration_vfio_unblock_postcopy_migration(void);
>>   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>>
>>   #endif
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 373f6e5932..7461194b2b 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -40,6 +40,7 @@
>>   #include "trace.h"
>>   #include "qapi/error.h"
>>   #include "migration/migration.h"
>> +#include "migration/options.h"
>>   #include "migration/misc.h"
>>   #include "migration/blocker.h"
>>   #include "migration/qemu-file.h"
>> @@ -343,6 +344,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer 
>> *container, uint64_t iova,
>>                                    uint64_t size, ram_addr_t ram_addr);
>>
>>   static Error *multiple_devices_migration_blocker;
>> +static Error *postcopy_migration_blocker;
>>
>>   static unsigned int vfio_migratable_devices_num(void)
>>   {
>> @@ -427,6 +429,47 @@ void vfio_unblock_multiple_devices_migration(void)
>>       multiple_devices_migration_blocker = NULL;
>>   }
>>
>> +int vfio_block_postcopy_migration(VFIODevice *vbasedev, Error **errp)
>> +{
>> +    int ret;
>> +
>> +    if (!migrate_postcopy_ram()) {
>> +        return 0;
>> +    }
>> +
>> +    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>> +        error_setg(errp,
>> +                   "VFIO migration is not compatible with postcopy 
>> migration");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (postcopy_migration_blocker) {
>> +        return 0;
>> +    }
>> +
>> +    error_setg(&postcopy_migration_blocker,
>> +               "VFIO migration is not compatible with postcopy 
>> migration");
>> +    ret = migrate_add_blocker(postcopy_migration_blocker, errp);
>> +    if (ret < 0) {
>> +        error_free(postcopy_migration_blocker);
>> +        postcopy_migration_blocker = NULL;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +void vfio_unblock_postcopy_migration(void)
>> +{
>> +    if (!postcopy_migration_blocker ||
>> +        (vfio_migratable_devices_num() && migrate_postcopy_ram())) {
>> +        return;
>> +    }
>> +
>> +    migrate_del_blocker(postcopy_migration_blocker);
>> +    error_free(postcopy_migration_blocker);
>> +    postcopy_migration_blocker = NULL;
>> +}
>> +
>>   bool vfio_mig_active(void)
>>   {
>>       return vfio_migratable_devices_num();
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 71855468fe..76406e9ae9 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -856,6 +856,7 @@ static void vfio_migration_deinit(VFIODevice 
>> *vbasedev)
>>       unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
>>       vfio_migration_free(vbasedev);
>>       vfio_unblock_multiple_devices_migration();
>> +    vfio_unblock_postcopy_migration();
>>   }
>>
>>   static int vfio_block_migration(VFIODevice *vbasedev, Error *err, 
>> Error **errp)
>> @@ -939,6 +940,11 @@ bool vfio_migration_realize(VFIODevice 
>> *vbasedev, Error **errp)
>>           goto out_deinit;
>>       }
>>
>> +    ret = vfio_block_postcopy_migration(vbasedev, errp);
>> +    if (ret) {
>> +        goto out_deinit;
>> +    }
>> +
>>       if (vfio_viommu_preset(vbasedev)) {
>>           error_setg(&err, "%s: Migration is currently not supported "
>>                      "with vIOMMU enabled", vbasedev->name);
>> diff --git a/migration/options.c b/migration/options.c
>> index 1d1e1321b0..e201053563 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool 
>> *new_caps, Error **errp)
>>               error_setg(errp, "Postcopy is not yet compatible with 
>> multifd");
>>               return false;
>>           }
>> +
>> +        if (migration_vfio_mig_active()) {
>> +            error_setg(errp, "Postcopy is not compatible with VFIO 
>> migration");
>> +            return false;
>> +        }
>>       }
>>
>>       if (new_caps[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
>> @@ -612,6 +617,16 @@ bool migrate_caps_check(bool *old_caps, bool 
>> *new_caps, Error **errp)
>>       return true;
>>   }
>>
>> +/*
>> + * Devices might have added migration blockers based on migration 
>> capabilities
>> + * values when those devices were added. Remove such blockers 
>> according to new
>> + * changes in migration capabilities.
>> + */
>> +static void migration_caps_remove_blockers(void)
>> +{
>> +    migration_vfio_unblock_postcopy_migration();
>> +}
>> +
>>   bool migrate_cap_set(int cap, bool value, Error **errp)
>>   {
>>       MigrationState *s = migrate_get_current();
>> @@ -629,6 +644,8 @@ bool migrate_cap_set(int cap, bool value, Error 
>> **errp)
>>           return false;
>>       }
>>       s->capabilities[cap] = value;
>> +    migration_caps_remove_blockers();
>> +
>>       return true;
>>   }
>>
>> @@ -678,6 +695,8 @@ void 
>> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>       for (cap = params; cap; cap = cap->next) {
>>           s->capabilities[cap->value->capability] = cap->value->state;
>>       }
>> +
>> +    migration_caps_remove_blockers();
>>   }
>>
>>   /* parameters */
>> diff --git a/migration/target.c b/migration/target.c
>> index a6ffa9a5ce..690ecb4dd5 100644
>> --- a/migration/target.c
>> +++ b/migration/target.c
>> @@ -27,6 +27,16 @@ void migration_reset_vfio_bytes_transferred(void)
>>   {
>>       vfio_reset_bytes_transferred();
>>   }
>> +
>> +bool migration_vfio_mig_active(void)
>> +{
>> +    return vfio_mig_active();
>> +}
>> +
>> +void migration_vfio_unblock_postcopy_migration(void)
>> +{
>> +    vfio_unblock_postcopy_migration();
>> +}
>>   #else
>>   void migration_populate_vfio_info(MigrationInfo *info)
>>   {
>> @@ -35,4 +45,13 @@ void migration_populate_vfio_info(MigrationInfo 
>> *info)
>>   void migration_reset_vfio_bytes_transferred(void)
>>   {
>>   }
>> +
>> +bool migration_vfio_mig_active(void)
>> +{
>> +    return false;
>> +}
>> +
>> +void migration_vfio_unblock_postcopy_migration()
>
>
> Missing 'void' above.
>
Oh, right. Next time I will also test compilation without VFIO config.

Thanks.

Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c

[Avihai Horon]

On 29/08/2023 17:04, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Aug 28, 2023 at 06:18:37PM +0300, Avihai Horon wrote:
>> The functions in target.c are not static, yet they don't have a proper
>> migration prefix. Add such prefix.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> No issue on the patch itself, but just noticed that we have hard-coded vfio
> calls in migration paths.. that's slightly unfortunate. :(
>
>> ---
>>   migration/migration.h | 4 ++--
>>   migration/migration.c | 6 +++---
>>   migration/savevm.c    | 2 +-
>>   migration/target.c    | 8 ++++----
>>   4 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 6eea18db36..c5695de214 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -512,8 +512,8 @@ void migration_consume_urgent_request(void);
>>   bool migration_rate_limit(void);
>>   void migration_cancel(const Error *error);
>>
>> -void populate_vfio_info(MigrationInfo *info);
>> -void reset_vfio_bytes_transferred(void);
>> +void migration_populate_vfio_info(MigrationInfo *info);
>> +void migration_reset_vfio_bytes_transferred(void);
>>   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>>
>>   #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5528acb65e..92866a8f49 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1039,7 +1039,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>>           populate_time_info(info, s);
>>           populate_ram_info(info, s);
>>           populate_disk_info(info);
>> -        populate_vfio_info(info);
>> +        migration_populate_vfio_info(info);
> (maybe in the future we should have SaveVMHandlers hooks for populating
>   data for ram/disk/vfio/..., or some other way to not hard-code these)

This sounds like a good idea.

>
>>           break;
>>       case MIGRATION_STATUS_COLO:
>>           info->has_status = true;
>> @@ -1048,7 +1048,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>>       case MIGRATION_STATUS_COMPLETED:
>>           populate_time_info(info, s);
>>           populate_ram_info(info, s);
>> -        populate_vfio_info(info);
>> +        migration_populate_vfio_info(info);
>>           break;
>>       case MIGRATION_STATUS_FAILED:
>>           info->has_status = true;
>> @@ -1641,7 +1641,7 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
>>        */
>>       memset(&mig_stats, 0, sizeof(mig_stats));
>>       memset(&compression_counters, 0, sizeof(compression_counters));
>> -    reset_vfio_bytes_transferred();
>> +    migration_reset_vfio_bytes_transferred();
> Could this already be done during vfio_save_setup(), to avoid calling an
> vfio function directly in migration.c?
>
> Again, not a request for this patchset, but more to see whether it'll work
> to be moved there.

VFIO bytes_transferred is a global variable for all VFIO devices.
Resetting it in vfio_save_setup() means that if there are multiple VFIO 
devices, it will be reset multiple times and that doesn't seem clean IMHO.

But maybe it would be a good idea to extend the VFIO migration info: 
make it per device and maybe split it to pre-copy data, stop-copy data, etc.
Then we can reset it in vfio_save_setup().

Thanks.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 29/08/2023 17:53, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>> diff --git a/migration/options.c b/migration/options.c
>> index 1d1e1321b0..e201053563 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>               error_setg(errp, "Postcopy is not yet compatible with multifd");
>>               return false;
>>           }
>> +
>> +        if (migration_vfio_mig_active()) {
>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>> +            return false;
>> +        }
> Hmm.. this will add yet another vfio hard-coded line into migration/..
>
> What will happen if the vfio device is hot plugged after enabling
> postcopy-ram here?

In that case a migration blocker will be added.

>
> Is it possible to do it in a generic way?

What comes to my mind is to let devices register a handler for a "caps 
change" notification and allow them to object.
But maybe that's a bit of an overkill.

>
> I was thinking the only unified place to do such check is when migration
> starts, as long as we switch to SETUP all caps are locked and doesn't allow
> any change until it finishes or fails.
>
> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
> to fail the whole migration early?  For example, maybe we should have an
> Error** passed in, then if it fails it calls migrate_set_error, so
> reflected in query-migrate later too.

Yes, I think this could work and it will simplify things because we 
could also drop the VFIO migration blockers code.
The downside is that the user will know migration is blocked only when 
he tries to migrate, and migrate_caps_check() will not block setting 
postcopy when a VFIO device is already attached.
I don't have a strong opinion here, so if it's fine by you and everyone 
else, I could change that to what you suggested.

Thanks.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Peter Xu]

On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
> 
> On 29/08/2023 17:53, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
> > > diff --git a/migration/options.c b/migration/options.c
> > > index 1d1e1321b0..e201053563 100644
> > > --- a/migration/options.c
> > > +++ b/migration/options.c
> > > @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> > >               error_setg(errp, "Postcopy is not yet compatible with multifd");
> > >               return false;
> > >           }
> > > +
> > > +        if (migration_vfio_mig_active()) {
> > > +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
> > > +            return false;
> > > +        }
> > Hmm.. this will add yet another vfio hard-coded line into migration/..
> > 
> > What will happen if the vfio device is hot plugged after enabling
> > postcopy-ram here?
> 
> In that case a migration blocker will be added.
> 
> > 
> > Is it possible to do it in a generic way?
> 
> What comes to my mind is to let devices register a handler for a "caps
> change" notification and allow them to object.
> But maybe that's a bit of an overkill.

This one also sounds better than hard-codes to me.

> 
> > 
> > I was thinking the only unified place to do such check is when migration
> > starts, as long as we switch to SETUP all caps are locked and doesn't allow
> > any change until it finishes or fails.
> > 
> > So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
> > to fail the whole migration early?  For example, maybe we should have an
> > Error** passed in, then if it fails it calls migrate_set_error, so
> > reflected in query-migrate later too.
> 
> Yes, I think this could work and it will simplify things because we could
> also drop the VFIO migration blockers code.
> The downside is that the user will know migration is blocked only when he
> tries to migrate, and migrate_caps_check() will not block setting postcopy
> when a VFIO device is already attached.
> I don't have a strong opinion here, so if it's fine by you and everyone
> else, I could change that to what you suggested.

Failing later would be fine in this case to me; my expectation is VFIO
users should be advanced already anyway (as the whole solution is still
pretty involved comparing to a generic VM migration) and shouldn't try to
trigger that at all in real life.  IOW I'd expect this check will be there
just for sanity, rather than being relied on to let people be aware of it
by the error message.

Meanwhile the blocker + caps check is slightly complicated to me to guard
both sides.  So I'd vote for failing at the QMP command.  But we can wait
and see whether there's other votes.

Thanks,

-- 
Peter Xu

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 29/08/2023 21:27, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>> On 29/08/2023 17:53, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>> diff --git a/migration/options.c b/migration/options.c
>>>> index 1d1e1321b0..e201053563 100644
>>>> --- a/migration/options.c
>>>> +++ b/migration/options.c
>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>                error_setg(errp, "Postcopy is not yet compatible with multifd");
>>>>                return false;
>>>>            }
>>>> +
>>>> +        if (migration_vfio_mig_active()) {
>>>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>>>> +            return false;
>>>> +        }
>>> Hmm.. this will add yet another vfio hard-coded line into migration/..
>>>
>>> What will happen if the vfio device is hot plugged after enabling
>>> postcopy-ram here?
>> In that case a migration blocker will be added.
>>
>>> Is it possible to do it in a generic way?
>> What comes to my mind is to let devices register a handler for a "caps
>> change" notification and allow them to object.
>> But maybe that's a bit of an overkill.
> This one also sounds better than hard-codes to me.
>
>>> I was thinking the only unified place to do such check is when migration
>>> starts, as long as we switch to SETUP all caps are locked and doesn't allow
>>> any change until it finishes or fails.
>>>
>>> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
>>> to fail the whole migration early?  For example, maybe we should have an
>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>> reflected in query-migrate later too.
>> Yes, I think this could work and it will simplify things because we could
>> also drop the VFIO migration blockers code.
>> The downside is that the user will know migration is blocked only when he
>> tries to migrate, and migrate_caps_check() will not block setting postcopy
>> when a VFIO device is already attached.
>> I don't have a strong opinion here, so if it's fine by you and everyone
>> else, I could change that to what you suggested.
> Failing later would be fine in this case to me; my expectation is VFIO
> users should be advanced already anyway (as the whole solution is still
> pretty involved comparing to a generic VM migration) and shouldn't try to
> trigger that at all in real life.  IOW I'd expect this check will be there
> just for sanity, rather than being relied on to let people be aware of it
> by the error message.

Yes, I agree with you.

>
> Meanwhile the blocker + caps check is slightly complicated to me to guard
> both sides.  So I'd vote for failing at the QMP command.  But we can wait
> and see whether there's other votes.

Sure.
So I will do the checking in vfio_save_setup(), unless someone else has 
a better idea.

Thanks.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Cédric Le Goater]

On 8/30/23 09:01, Avihai Horon wrote:
> 
> On 29/08/2023 21:27, Peter Xu wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>> index 1d1e1321b0..e201053563 100644
>>>>> --- a/migration/options.c
>>>>> +++ b/migration/options.c
>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>>                error_setg(errp, "Postcopy is not yet compatible with multifd");
>>>>>                return false;
>>>>>            }
>>>>> +
>>>>> +        if (migration_vfio_mig_active()) {
>>>>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>>>>> +            return false;
>>>>> +        }
>>>> Hmm.. this will add yet another vfio hard-coded line into migration/..
>>>>
>>>> What will happen if the vfio device is hot plugged after enabling
>>>> postcopy-ram here?
>>> In that case a migration blocker will be added.
>>>
>>>> Is it possible to do it in a generic way?
>>> What comes to my mind is to let devices register a handler for a "caps
>>> change" notification and allow them to object.
>>> But maybe that's a bit of an overkill.
>> This one also sounds better than hard-codes to me.
>>
>>>> I was thinking the only unified place to do such check is when migration
>>>> starts, as long as we switch to SETUP all caps are locked and doesn't allow
>>>> any change until it finishes or fails.
>>>>
>>>> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
>>>> to fail the whole migration early?  For example, maybe we should have an
>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>> reflected in query-migrate later too.
>>> Yes, I think this could work and it will simplify things because we could
>>> also drop the VFIO migration blockers code.
>>> The downside is that the user will know migration is blocked only when he
>>> tries to migrate, and migrate_caps_check() will not block setting postcopy
>>> when a VFIO device is already attached.
>>> I don't have a strong opinion here, so if it's fine by you and everyone
>>> else, I could change that to what you suggested.
>> Failing later would be fine in this case to me; my expectation is VFIO
>> users should be advanced already anyway (as the whole solution is still
>> pretty involved comparing to a generic VM migration) and shouldn't try to
>> trigger that at all in real life.  IOW I'd expect this check will be there
>> just for sanity, rather than being relied on to let people be aware of it
>> by the error message.
> 
> Yes, I agree with you.
> 
>>
>> Meanwhile the blocker + caps check is slightly complicated to me to guard
>> both sides.  So I'd vote for failing at the QMP command.  But we can wait
>> and see whether there's other votes.
> 
> Sure.
> So I will do the checking in vfio_save_setup(), unless someone else has a better idea.

Just to recap for my understanding,

vfio_save_setup() would test migrate_postcopy_ram() and update a new
'Error *err' parameter of the .save_setup() op which would be taken
into account in qemu_savevm_state_setup(). Is that correct ?


Thanks,

C.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 30/08/2023 11:37, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/30/23 09:01, Avihai Horon wrote:
>>
>> On 29/08/2023 21:27, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>> index 1d1e1321b0..e201053563 100644
>>>>>> --- a/migration/options.c
>>>>>> +++ b/migration/options.c
>>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool 
>>>>>> *new_caps, Error **errp)
>>>>>>                error_setg(errp, "Postcopy is not yet compatible 
>>>>>> with multifd");
>>>>>>                return false;
>>>>>>            }
>>>>>> +
>>>>>> +        if (migration_vfio_mig_active()) {
>>>>>> +            error_setg(errp, "Postcopy is not compatible with 
>>>>>> VFIO migration");
>>>>>> +            return false;
>>>>>> +        }
>>>>> Hmm.. this will add yet another vfio hard-coded line into 
>>>>> migration/..
>>>>>
>>>>> What will happen if the vfio device is hot plugged after enabling
>>>>> postcopy-ram here?
>>>> In that case a migration blocker will be added.
>>>>
>>>>> Is it possible to do it in a generic way?
>>>> What comes to my mind is to let devices register a handler for a "caps
>>>> change" notification and allow them to object.
>>>> But maybe that's a bit of an overkill.
>>> This one also sounds better than hard-codes to me.
>>>
>>>>> I was thinking the only unified place to do such check is when 
>>>>> migration
>>>>> starts, as long as we switch to SETUP all caps are locked and 
>>>>> doesn't allow
>>>>> any change until it finishes or fails.
>>>>>
>>>>> So, can we do this check inside vfio_save_setup(), allow 
>>>>> vfio_save_setup()
>>>>> to fail the whole migration early?  For example, maybe we should 
>>>>> have an
>>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>>> reflected in query-migrate later too.
>>>> Yes, I think this could work and it will simplify things because we 
>>>> could
>>>> also drop the VFIO migration blockers code.
>>>> The downside is that the user will know migration is blocked only 
>>>> when he
>>>> tries to migrate, and migrate_caps_check() will not block setting 
>>>> postcopy
>>>> when a VFIO device is already attached.
>>>> I don't have a strong opinion here, so if it's fine by you and 
>>>> everyone
>>>> else, I could change that to what you suggested.
>>> Failing later would be fine in this case to me; my expectation is VFIO
>>> users should be advanced already anyway (as the whole solution is still
>>> pretty involved comparing to a generic VM migration) and shouldn't 
>>> try to
>>> trigger that at all in real life.  IOW I'd expect this check will be 
>>> there
>>> just for sanity, rather than being relied on to let people be aware 
>>> of it
>>> by the error message.
>>
>> Yes, I agree with you.
>>
>>>
>>> Meanwhile the blocker + caps check is slightly complicated to me to 
>>> guard
>>> both sides.  So I'd vote for failing at the QMP command.  But we can 
>>> wait
>>> and see whether there's other votes.
>>
>> Sure.
>> So I will do the checking in vfio_save_setup(), unless someone else 
>> has a better idea.
>
> Just to recap for my understanding,
>
> vfio_save_setup() would test migrate_postcopy_ram() and update a new
> 'Error *err' parameter of the .save_setup() op which would be taken
> into account in qemu_savevm_state_setup(). Is that correct ?
>
Yes.
But I wonder if it would be simpler to call migrate_set_error() directly 
from vfio_save_setup() instead of adding "Error *err" argument to 
.save_setup() and changing all other users.
What do you prefer?

Thanks.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Cédric Le Goater]

On 8/30/23 11:21, Avihai Horon wrote:
> 
> On 30/08/2023 11:37, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 8/30/23 09:01, Avihai Horon wrote:
>>>
>>> On 29/08/2023 21:27, Peter Xu wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>>> index 1d1e1321b0..e201053563 100644
>>>>>>> --- a/migration/options.c
>>>>>>> +++ b/migration/options.c
>>>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>>>>                error_setg(errp, "Postcopy is not yet compatible with multifd");
>>>>>>>                return false;
>>>>>>>            }
>>>>>>> +
>>>>>>> +        if (migration_vfio_mig_active()) {
>>>>>>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>>>>>>> +            return false;
>>>>>>> +        }
>>>>>> Hmm.. this will add yet another vfio hard-coded line into migration/..
>>>>>>
>>>>>> What will happen if the vfio device is hot plugged after enabling
>>>>>> postcopy-ram here?
>>>>> In that case a migration blocker will be added.
>>>>>
>>>>>> Is it possible to do it in a generic way?
>>>>> What comes to my mind is to let devices register a handler for a "caps
>>>>> change" notification and allow them to object.
>>>>> But maybe that's a bit of an overkill.
>>>> This one also sounds better than hard-codes to me.
>>>>
>>>>>> I was thinking the only unified place to do such check is when migration
>>>>>> starts, as long as we switch to SETUP all caps are locked and doesn't allow
>>>>>> any change until it finishes or fails.
>>>>>>
>>>>>> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
>>>>>> to fail the whole migration early?  For example, maybe we should have an
>>>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>>>> reflected in query-migrate later too.
>>>>> Yes, I think this could work and it will simplify things because we could
>>>>> also drop the VFIO migration blockers code.
>>>>> The downside is that the user will know migration is blocked only when he
>>>>> tries to migrate, and migrate_caps_check() will not block setting postcopy
>>>>> when a VFIO device is already attached.
>>>>> I don't have a strong opinion here, so if it's fine by you and everyone
>>>>> else, I could change that to what you suggested.
>>>> Failing later would be fine in this case to me; my expectation is VFIO
>>>> users should be advanced already anyway (as the whole solution is still
>>>> pretty involved comparing to a generic VM migration) and shouldn't try to
>>>> trigger that at all in real life.  IOW I'd expect this check will be there
>>>> just for sanity, rather than being relied on to let people be aware of it
>>>> by the error message.
>>>
>>> Yes, I agree with you.
>>>
>>>>
>>>> Meanwhile the blocker + caps check is slightly complicated to me to guard
>>>> both sides.  So I'd vote for failing at the QMP command.  But we can wait
>>>> and see whether there's other votes.
>>>
>>> Sure.
>>> So I will do the checking in vfio_save_setup(), unless someone else has a better idea.
>>
>> Just to recap for my understanding,
>>
>> vfio_save_setup() would test migrate_postcopy_ram() and update a new
>> 'Error *err' parameter of the .save_setup() op which would be taken
>> into account in qemu_savevm_state_setup(). Is that correct ?
>>
> Yes.
> But I wonder if it would be simpler to call migrate_set_error() directly from vfio_save_setup() instead of adding "Error *err" argument to .save_setup() and changing all other users.
> What do you prefer?

Well, with my downstreamer hat, I would prefer a simpler solution for the
VFIO postcopy limitation first. That said, there is value in adding
a 'Error *' parameter to the .save_setup() op and letting the top routine
qemu_savevm_state_setup() propagate. Other SaveVMhandler could start using
it. even VFIO has multiple error_report() in vfio_save_setup() which could
be propagated to the top callers.

Let's try that first. I will check your new series on top of 8.0

Thanks,

C.


> 
> Thanks.
> 

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 30/08/2023 12:53, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/30/23 11:21, Avihai Horon wrote:
>>
>> On 30/08/2023 11:37, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 8/30/23 09:01, Avihai Horon wrote:
>>>>
>>>> On 29/08/2023 21:27, Peter Xu wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>>>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>>>> index 1d1e1321b0..e201053563 100644
>>>>>>>> --- a/migration/options.c
>>>>>>>> +++ b/migration/options.c
>>>>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, 
>>>>>>>> bool *new_caps, Error **errp)
>>>>>>>>                error_setg(errp, "Postcopy is not yet compatible 
>>>>>>>> with multifd");
>>>>>>>>                return false;
>>>>>>>>            }
>>>>>>>> +
>>>>>>>> +        if (migration_vfio_mig_active()) {
>>>>>>>> +            error_setg(errp, "Postcopy is not compatible with 
>>>>>>>> VFIO migration");
>>>>>>>> +            return false;
>>>>>>>> +        }
>>>>>>> Hmm.. this will add yet another vfio hard-coded line into 
>>>>>>> migration/..
>>>>>>>
>>>>>>> What will happen if the vfio device is hot plugged after enabling
>>>>>>> postcopy-ram here?
>>>>>> In that case a migration blocker will be added.
>>>>>>
>>>>>>> Is it possible to do it in a generic way?
>>>>>> What comes to my mind is to let devices register a handler for a 
>>>>>> "caps
>>>>>> change" notification and allow them to object.
>>>>>> But maybe that's a bit of an overkill.
>>>>> This one also sounds better than hard-codes to me.
>>>>>
>>>>>>> I was thinking the only unified place to do such check is when 
>>>>>>> migration
>>>>>>> starts, as long as we switch to SETUP all caps are locked and 
>>>>>>> doesn't allow
>>>>>>> any change until it finishes or fails.
>>>>>>>
>>>>>>> So, can we do this check inside vfio_save_setup(), allow 
>>>>>>> vfio_save_setup()
>>>>>>> to fail the whole migration early?  For example, maybe we should 
>>>>>>> have an
>>>>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>>>>> reflected in query-migrate later too.
>>>>>> Yes, I think this could work and it will simplify things because 
>>>>>> we could
>>>>>> also drop the VFIO migration blockers code.
>>>>>> The downside is that the user will know migration is blocked only 
>>>>>> when he
>>>>>> tries to migrate, and migrate_caps_check() will not block setting 
>>>>>> postcopy
>>>>>> when a VFIO device is already attached.
>>>>>> I don't have a strong opinion here, so if it's fine by you and 
>>>>>> everyone
>>>>>> else, I could change that to what you suggested.
>>>>> Failing later would be fine in this case to me; my expectation is 
>>>>> VFIO
>>>>> users should be advanced already anyway (as the whole solution is 
>>>>> still
>>>>> pretty involved comparing to a generic VM migration) and shouldn't 
>>>>> try to
>>>>> trigger that at all in real life.  IOW I'd expect this check will 
>>>>> be there
>>>>> just for sanity, rather than being relied on to let people be 
>>>>> aware of it
>>>>> by the error message.
>>>>
>>>> Yes, I agree with you.
>>>>
>>>>>
>>>>> Meanwhile the blocker + caps check is slightly complicated to me 
>>>>> to guard
>>>>> both sides.  So I'd vote for failing at the QMP command. But we 
>>>>> can wait
>>>>> and see whether there's other votes.
>>>>
>>>> Sure.
>>>> So I will do the checking in vfio_save_setup(), unless someone else 
>>>> has a better idea.
>>>
>>> Just to recap for my understanding,
>>>
>>> vfio_save_setup() would test migrate_postcopy_ram() and update a new
>>> 'Error *err' parameter of the .save_setup() op which would be taken
>>> into account in qemu_savevm_state_setup(). Is that correct ?
>>>
>> Yes.
>> But I wonder if it would be simpler to call migrate_set_error() 
>> directly from vfio_save_setup() instead of adding "Error *err" 
>> argument to .save_setup() and changing all other users.
>> What do you prefer?
>
> Well, with my downstreamer hat, I would prefer a simpler solution for the
> VFIO postcopy limitation first. That said, there is value in adding
> a 'Error *' parameter to the .save_setup() op and letting the top routine
> qemu_savevm_state_setup() propagate. Other SaveVMhandler could start 
> using
> it. even VFIO has multiple error_report() in vfio_save_setup() which 
> could
> be propagated to the top callers.
>
> Let's try that first. I will check your new series on top of 8.0
>
OK, so just making sure, you want to add "Error *err" argument to 
.save_setup() first and see how it goes, right?

Thanks.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Cédric Le Goater]

On 8/30/23 12:12, Avihai Horon wrote:
> 
> On 30/08/2023 12:53, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 8/30/23 11:21, Avihai Horon wrote:
>>>
>>> On 30/08/2023 11:37, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 8/30/23 09:01, Avihai Horon wrote:
>>>>>
>>>>> On 29/08/2023 21:27, Peter Xu wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>>>>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>>>>> index 1d1e1321b0..e201053563 100644
>>>>>>>>> --- a/migration/options.c
>>>>>>>>> +++ b/migration/options.c
>>>>>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>>>>>>                error_setg(errp, "Postcopy is not yet compatible with multifd");
>>>>>>>>>                return false;
>>>>>>>>>            }
>>>>>>>>> +
>>>>>>>>> +        if (migration_vfio_mig_active()) {
>>>>>>>>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>>>>>>>>> +            return false;
>>>>>>>>> +        }
>>>>>>>> Hmm.. this will add yet another vfio hard-coded line into migration/..
>>>>>>>>
>>>>>>>> What will happen if the vfio device is hot plugged after enabling
>>>>>>>> postcopy-ram here?
>>>>>>> In that case a migration blocker will be added.
>>>>>>>
>>>>>>>> Is it possible to do it in a generic way?
>>>>>>> What comes to my mind is to let devices register a handler for a "caps
>>>>>>> change" notification and allow them to object.
>>>>>>> But maybe that's a bit of an overkill.
>>>>>> This one also sounds better than hard-codes to me.
>>>>>>
>>>>>>>> I was thinking the only unified place to do such check is when migration
>>>>>>>> starts, as long as we switch to SETUP all caps are locked and doesn't allow
>>>>>>>> any change until it finishes or fails.
>>>>>>>>
>>>>>>>> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
>>>>>>>> to fail the whole migration early?  For example, maybe we should have an
>>>>>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>>>>>> reflected in query-migrate later too.
>>>>>>> Yes, I think this could work and it will simplify things because we could
>>>>>>> also drop the VFIO migration blockers code.
>>>>>>> The downside is that the user will know migration is blocked only when he
>>>>>>> tries to migrate, and migrate_caps_check() will not block setting postcopy
>>>>>>> when a VFIO device is already attached.
>>>>>>> I don't have a strong opinion here, so if it's fine by you and everyone
>>>>>>> else, I could change that to what you suggested.
>>>>>> Failing later would be fine in this case to me; my expectation is VFIO
>>>>>> users should be advanced already anyway (as the whole solution is still
>>>>>> pretty involved comparing to a generic VM migration) and shouldn't try to
>>>>>> trigger that at all in real life.  IOW I'd expect this check will be there
>>>>>> just for sanity, rather than being relied on to let people be aware of it
>>>>>> by the error message.
>>>>>
>>>>> Yes, I agree with you.
>>>>>
>>>>>>
>>>>>> Meanwhile the blocker + caps check is slightly complicated to me to guard
>>>>>> both sides.  So I'd vote for failing at the QMP command. But we can wait
>>>>>> and see whether there's other votes.
>>>>>
>>>>> Sure.
>>>>> So I will do the checking in vfio_save_setup(), unless someone else has a better idea.
>>>>
>>>> Just to recap for my understanding,
>>>>
>>>> vfio_save_setup() would test migrate_postcopy_ram() and update a new
>>>> 'Error *err' parameter of the .save_setup() op which would be taken
>>>> into account in qemu_savevm_state_setup(). Is that correct ?
>>>>
>>> Yes.
>>> But I wonder if it would be simpler to call migrate_set_error() directly from vfio_save_setup() instead of adding "Error *err" argument to .save_setup() and changing all other users.
>>> What do you prefer?
>>
>> Well, with my downstreamer hat, I would prefer a simpler solution for the
>> VFIO postcopy limitation first. That said, there is value in adding
>> a 'Error *' parameter to the .save_setup() op and letting the top routine
>> qemu_savevm_state_setup() propagate. Other SaveVMhandler could start using
>> it. even VFIO has multiple error_report() in vfio_save_setup() which could
>> be propagated to the top callers.
>>
>> Let's try that first. I will check your new series on top of 8.0
>>
> OK, so just making sure, you want to add "Error *err" argument to .save_setup() first and see how it goes, right?

yes. Sorry. that was not clear.

Thanks

C.

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Peter Xu]

On Wed, Aug 30, 2023 at 01:17:55PM +0200, Cédric Le Goater wrote:
> On 8/30/23 12:12, Avihai Horon wrote:
> > 
> > On 30/08/2023 12:53, Cédric Le Goater wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On 8/30/23 11:21, Avihai Horon wrote:
> > > > 
> > > > On 30/08/2023 11:37, Cédric Le Goater wrote:
> > > > > External email: Use caution opening links or attachments
> > > > > 
> > > > > 
> > > > > On 8/30/23 09:01, Avihai Horon wrote:
> > > > > > 
> > > > > > On 29/08/2023 21:27, Peter Xu wrote:
> > > > > > > External email: Use caution opening links or attachments
> > > > > > > 
> > > > > > > 
> > > > > > > On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
> > > > > > > > On 29/08/2023 17:53, Peter Xu wrote:
> > > > > > > > > External email: Use caution opening links or attachments
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
> > > > > > > > > > diff --git a/migration/options.c b/migration/options.c
> > > > > > > > > > index 1d1e1321b0..e201053563 100644
> > > > > > > > > > --- a/migration/options.c
> > > > > > > > > > +++ b/migration/options.c
> > > > > > > > > > @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> > > > > > > > > >                error_setg(errp, "Postcopy is not yet compatible with multifd");
> > > > > > > > > >                return false;
> > > > > > > > > >            }
> > > > > > > > > > +
> > > > > > > > > > +        if (migration_vfio_mig_active()) {
> > > > > > > > > > +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
> > > > > > > > > > +            return false;
> > > > > > > > > > +        }
> > > > > > > > > Hmm.. this will add yet another vfio hard-coded line into migration/..
> > > > > > > > > 
> > > > > > > > > What will happen if the vfio device is hot plugged after enabling
> > > > > > > > > postcopy-ram here?
> > > > > > > > In that case a migration blocker will be added.
> > > > > > > > 
> > > > > > > > > Is it possible to do it in a generic way?
> > > > > > > > What comes to my mind is to let devices register a handler for a "caps
> > > > > > > > change" notification and allow them to object.
> > > > > > > > But maybe that's a bit of an overkill.
> > > > > > > This one also sounds better than hard-codes to me.
> > > > > > > 
> > > > > > > > > I was thinking the only unified place to do such check is when migration
> > > > > > > > > starts, as long as we switch to SETUP all caps are locked and doesn't allow
> > > > > > > > > any change until it finishes or fails.
> > > > > > > > > 
> > > > > > > > > So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
> > > > > > > > > to fail the whole migration early?  For example, maybe we should have an
> > > > > > > > > Error** passed in, then if it fails it calls migrate_set_error, so
> > > > > > > > > reflected in query-migrate later too.
> > > > > > > > Yes, I think this could work and it will simplify things because we could
> > > > > > > > also drop the VFIO migration blockers code.
> > > > > > > > The downside is that the user will know migration is blocked only when he
> > > > > > > > tries to migrate, and migrate_caps_check() will not block setting postcopy
> > > > > > > > when a VFIO device is already attached.
> > > > > > > > I don't have a strong opinion here, so if it's fine by you and everyone
> > > > > > > > else, I could change that to what you suggested.
> > > > > > > Failing later would be fine in this case to me; my expectation is VFIO
> > > > > > > users should be advanced already anyway (as the whole solution is still
> > > > > > > pretty involved comparing to a generic VM migration) and shouldn't try to
> > > > > > > trigger that at all in real life.  IOW I'd expect this check will be there
> > > > > > > just for sanity, rather than being relied on to let people be aware of it
> > > > > > > by the error message.
> > > > > > 
> > > > > > Yes, I agree with you.
> > > > > > 
> > > > > > > 
> > > > > > > Meanwhile the blocker + caps check is slightly complicated to me to guard
> > > > > > > both sides.  So I'd vote for failing at the QMP command. But we can wait
> > > > > > > and see whether there's other votes.
> > > > > > 
> > > > > > Sure.
> > > > > > So I will do the checking in vfio_save_setup(), unless someone else has a better idea.
> > > > > 
> > > > > Just to recap for my understanding,
> > > > > 
> > > > > vfio_save_setup() would test migrate_postcopy_ram() and update a new
> > > > > 'Error *err' parameter of the .save_setup() op which would be taken
> > > > > into account in qemu_savevm_state_setup(). Is that correct ?
> > > > > 
> > > > Yes.
> > > > But I wonder if it would be simpler to call migrate_set_error() directly from vfio_save_setup() instead of adding "Error *err" argument to .save_setup() and changing all other users.
> > > > What do you prefer?
> > > 
> > > Well, with my downstreamer hat, I would prefer a simpler solution for the
> > > VFIO postcopy limitation first. That said, there is value in adding
> > > a 'Error *' parameter to the .save_setup() op and letting the top routine
> > > qemu_savevm_state_setup() propagate. Other SaveVMhandler could start using
> > > it. even VFIO has multiple error_report() in vfio_save_setup() which could
> > > be propagated to the top callers.
> > > 
> > > Let's try that first. I will check your new series on top of 8.0
> > > 
> > OK, so just making sure, you want to add "Error *err" argument to .save_setup() first and see how it goes, right?
> 
> yes. Sorry. that was not clear.

I just remembered one pity of failing at save_setup() is it won't fail qmp
command "migrate" itself, but only reflected in query-migrate later.

If we want to make it even better (no strong opinion here, of now), we can
have it separate from save_setup(), e.g., SaveVMHandlers.save_prepare(), so
that it can be called even at migrate_prepare() and fail the QMP command
with proper errors.

Thanks,

-- 
Peter Xu

Re: [PATCH 5/6] vfio/migration: Block VFIO migration with postcopy migration

[Avihai Horon]

On 30/08/2023 17:22, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Aug 30, 2023 at 01:17:55PM +0200, Cédric Le Goater wrote:
>> On 8/30/23 12:12, Avihai Horon wrote:
>>> On 30/08/2023 12:53, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 8/30/23 11:21, Avihai Horon wrote:
>>>>> On 30/08/2023 11:37, Cédric Le Goater wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> On 8/30/23 09:01, Avihai Horon wrote:
>>>>>>> On 29/08/2023 21:27, Peter Xu wrote:
>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
>>>>>>>>> On 29/08/2023 17:53, Peter Xu wrote:
>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
>>>>>>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>>>>>>> index 1d1e1321b0..e201053563 100644
>>>>>>>>>>> --- a/migration/options.c
>>>>>>>>>>> +++ b/migration/options.c
>>>>>>>>>>> @@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>>>>>>>>                 error_setg(errp, "Postcopy is not yet compatible with multifd");
>>>>>>>>>>>                 return false;
>>>>>>>>>>>             }
>>>>>>>>>>> +
>>>>>>>>>>> +        if (migration_vfio_mig_active()) {
>>>>>>>>>>> +            error_setg(errp, "Postcopy is not compatible with VFIO migration");
>>>>>>>>>>> +            return false;
>>>>>>>>>>> +        }
>>>>>>>>>> Hmm.. this will add yet another vfio hard-coded line into migration/..
>>>>>>>>>>
>>>>>>>>>> What will happen if the vfio device is hot plugged after enabling
>>>>>>>>>> postcopy-ram here?
>>>>>>>>> In that case a migration blocker will be added.
>>>>>>>>>
>>>>>>>>>> Is it possible to do it in a generic way?
>>>>>>>>> What comes to my mind is to let devices register a handler for a "caps
>>>>>>>>> change" notification and allow them to object.
>>>>>>>>> But maybe that's a bit of an overkill.
>>>>>>>> This one also sounds better than hard-codes to me.
>>>>>>>>
>>>>>>>>>> I was thinking the only unified place to do such check is when migration
>>>>>>>>>> starts, as long as we switch to SETUP all caps are locked and doesn't allow
>>>>>>>>>> any change until it finishes or fails.
>>>>>>>>>>
>>>>>>>>>> So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
>>>>>>>>>> to fail the whole migration early?  For example, maybe we should have an
>>>>>>>>>> Error** passed in, then if it fails it calls migrate_set_error, so
>>>>>>>>>> reflected in query-migrate later too.
>>>>>>>>> Yes, I think this could work and it will simplify things because we could
>>>>>>>>> also drop the VFIO migration blockers code.
>>>>>>>>> The downside is that the user will know migration is blocked only when he
>>>>>>>>> tries to migrate, and migrate_caps_check() will not block setting postcopy
>>>>>>>>> when a VFIO device is already attached.
>>>>>>>>> I don't have a strong opinion here, so if it's fine by you and everyone
>>>>>>>>> else, I could change that to what you suggested.
>>>>>>>> Failing later would be fine in this case to me; my expectation is VFIO
>>>>>>>> users should be advanced already anyway (as the whole solution is still
>>>>>>>> pretty involved comparing to a generic VM migration) and shouldn't try to
>>>>>>>> trigger that at all in real life.  IOW I'd expect this check will be there
>>>>>>>> just for sanity, rather than being relied on to let people be aware of it
>>>>>>>> by the error message.
>>>>>>> Yes, I agree with you.
>>>>>>>
>>>>>>>> Meanwhile the blocker + caps check is slightly complicated to me to guard
>>>>>>>> both sides.  So I'd vote for failing at the QMP command. But we can wait
>>>>>>>> and see whether there's other votes.
>>>>>>> Sure.
>>>>>>> So I will do the checking in vfio_save_setup(), unless someone else has a better idea.
>>>>>> Just to recap for my understanding,
>>>>>>
>>>>>> vfio_save_setup() would test migrate_postcopy_ram() and update a new
>>>>>> 'Error *err' parameter of the .save_setup() op which would be taken
>>>>>> into account in qemu_savevm_state_setup(). Is that correct ?
>>>>>>
>>>>> Yes.
>>>>> But I wonder if it would be simpler to call migrate_set_error() directly from vfio_save_setup() instead of adding "Error *err" argument to .save_setup() and changing all other users.
>>>>> What do you prefer?
>>>> Well, with my downstreamer hat, I would prefer a simpler solution for the
>>>> VFIO postcopy limitation first. That said, there is value in adding
>>>> a 'Error *' parameter to the .save_setup() op and letting the top routine
>>>> qemu_savevm_state_setup() propagate. Other SaveVMhandler could start using
>>>> it. even VFIO has multiple error_report() in vfio_save_setup() which could
>>>> be propagated to the top callers.
>>>>
>>>> Let's try that first. I will check your new series on top of 8.0
>>>>
>>> OK, so just making sure, you want to add "Error *err" argument to .save_setup() first and see how it goes, right?
>> yes. Sorry. that was not clear.
> I just remembered one pity of failing at save_setup() is it won't fail qmp
> command "migrate" itself, but only reflected in query-migrate later.
>
> If we want to make it even better (no strong opinion here, of now), we can
> have it separate from save_setup(), e.g., SaveVMHandlers.save_prepare(), so
> that it can be called even at migrate_prepare() and fail the QMP command
> with proper errors.

Sounds good.
I will do it and send v2, hopefully tomorrow.

Thanks.