Re: [PATCH v2 07/11] vfio/migration: Implement VFIO migration protocol v2

[Avihai Horon]

On 6/14/2022 2:08 PM, Joao Martins wrote:
External email: Use caution opening links or attachments


On 5/30/22 18:07, Avihai Horon wrote:
+static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
+{
+    VFIODevice *vbasedev = opaque;
+    enum vfio_device_mig_state recover_state;
+    int ret;
+
+    /* We reach here with device state STOP or STOP_COPY only */
+    recover_state = VFIO_DEVICE_STATE_STOP;
+    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
+                                   recover_state);
+    if (ret) {
+        return ret;
+    }
+
+    do {
+        ret = vfio_save_block(f, vbasedev->migration);
+        if (ret < 0) {
+            return ret;
+        }
+    } while (!ret);
+
+    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
+    ret = qemu_file_get_error(f);
+    if (ret) {
+        return ret;
+    }
+
+    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
+                                   recover_state);
Is it expected that you are setting VFIO_DEVICE_STATE_STOP while
@recover_state is the same value (VFIO_DEVICE_STATE_STOP) ?


Yes.
Transitioning to any other state from STOP_COPY will first go through 
STOP state (this is done internally by kernel).
So there is no better option for the recover state but STOP.

+    if (ret) {
+        return ret;
+    }
+
+    trace_vfio_save_complete_precopy(vbasedev->name);
+
+    return 0;
+}
+
  static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque)
  {
      VFIODevice *vbasedev = opaque;
@@ -593,6 +775,14 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
      }
  }

+static int vfio_load_setup(QEMUFile *f, void *opaque)
+{
+    VFIODevice *vbasedev = opaque;
+
+    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
+                                   vbasedev->migration->device_state);
+}
+
  static int vfio_v1_load_setup(QEMUFile *f, void *opaque)
  {
      VFIODevice *vbasedev = opaque;
@@ -620,6 +810,15 @@ static int vfio_v1_load_setup(QEMUFile *f, void *opaque)
      return ret;
  }

+static int vfio_load_cleanup(void *opaque)
+{
+    VFIODevice *vbasedev = opaque;
+
+    vfio_migration_cleanup(vbasedev);
+    trace_vfio_load_cleanup(vbasedev->name);
+    return 0;
+}
+
  static int vfio_v1_load_cleanup(void *opaque)
  {
      VFIODevice *vbasedev = opaque;
@@ -662,7 +861,11 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int 
version_id)
              uint64_t data_size = qemu_get_be64(f);

              if (data_size) {
-                ret = vfio_v1_load_buffer(f, vbasedev, data_size);
+                if (vbasedev->migration->v2) {
+                    ret = vfio_load_buffer(f, vbasedev, data_size);
+                } else {
+                    ret = vfio_v1_load_buffer(f, vbasedev, data_size);
+                }
                  if (ret < 0) {
                      return ret;
                  }
@@ -683,6 +886,16 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int 
version_id)
      return ret;
  }

+static SaveVMHandlers savevm_vfio_handlers = {
+    .save_setup = vfio_save_setup,
+    .save_cleanup = vfio_save_cleanup,
+    .save_live_complete_precopy = vfio_save_complete_precopy,
+    .save_state = vfio_save_state,
+    .load_setup = vfio_load_setup,
+    .load_cleanup = vfio_load_cleanup,
+    .load_state = vfio_load_state,
+};
+
  static SaveVMHandlers savevm_vfio_v1_handlers = {
      .save_setup = vfio_v1_save_setup,
      .save_cleanup = vfio_v1_save_cleanup,
@@ -697,6 +910,34 @@ static SaveVMHandlers savevm_vfio_v1_handlers = {

  /* ---------------------------------------------------------------------- */

+static void vfio_vmstate_change(void *opaque, bool running, RunState state)
+{
+    VFIODevice *vbasedev = opaque;
+    enum vfio_device_mig_state new_state;
+    int ret;
+
+    if (running) {
+        new_state = VFIO_DEVICE_STATE_RUNNING;
+    } else {
+        new_state = VFIO_DEVICE_STATE_STOP;
+    }
+
+    ret = vfio_migration_set_state(vbasedev, new_state,
+                                   VFIO_DEVICE_STATE_ERROR);
+    if (ret) {
+        /*
+         * Migration should be aborted in this case, but vm_state_notify()
+         * currently does not support reporting failures.
+         */
+        if (migrate_get_current()->to_dst_file) {
+            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
+        }
+    }
+
+    trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
+                              new_state);
+}
+
  static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state)
  {
      VFIODevice *vbasedev = opaque;
@@ -770,12 +1011,17 @@ static void vfio_migration_state_notifier(Notifier 
*notifier, void *data)
      case MIGRATION_STATUS_CANCELLED:
      case MIGRATION_STATUS_FAILED:
          bytes_transferred = 0;
-        ret = vfio_migration_v1_set_state(vbasedev,
-                                          ~(VFIO_DEVICE_STATE_V1_SAVING |
-                                            VFIO_DEVICE_STATE_V1_RESUMING),
-                                          VFIO_DEVICE_STATE_V1_RUNNING);
-        if (ret) {
-            error_report("%s: Failed to set state RUNNING", vbasedev->name);
+        if (migration->v2) {
+            vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
+                                     VFIO_DEVICE_STATE_ERROR);
Perhaps you are discarding the error?

Shouldn't it be:

         err =  vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
                                         VFIO_DEVICE_STATE_ERROR);

+        } else {
+            ret = vfio_migration_v1_set_state(vbasedev,
+                                              ~(VFIO_DEVICE_STATE_V1_SAVING |
+                                                VFIO_DEVICE_STATE_V1_RESUMING),
+                                              VFIO_DEVICE_STATE_V1_RUNNING);
+            if (ret) {
+                error_report("%s: Failed to set state RUNNING", 
vbasedev->name);
+            }
Perhaps this error_report and condition is in the wrong scope?

Shouldn't it be more like this:

if (migration->v2) {
         ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING,
                                  VFIO_DEVICE_STATE_ERROR);
} else {
         ret = vfio_migration_v1_set_state(vbasedev,
                                           ~(VFIO_DEVICE_STATE_V1_SAVING |
                                             VFIO_DEVICE_STATE_V1_RESUMING),
                                           VFIO_DEVICE_STATE_V1_RUNNING);
}


if (ret) {
     error_report("%s: Failed to set state RUNNING", vbasedev->name);
}


It was intentionally discarded.
The return value is used by v1 code to determine whether to print an 
error message or not.
In v2 code the error message print is done inside 
vfio_migration_set_state(), so there is no
need for the return value here.

          }
      }
  }
@@ -784,12 +1030,35 @@ static void vfio_migration_exit(VFIODevice *vbasedev)
  {
      VFIOMigration *migration = vbasedev->migration;

-    vfio_region_exit(&migration->region);
-    vfio_region_finalize(&migration->region);
+    if (migration->v2) {
+        g_free(migration->data_buffer);
+    } else {
+        vfio_region_exit(&migration->region);
+        vfio_region_finalize(&migration->region);
+    }
      g_free(vbasedev->migration);
      vbasedev->migration = NULL;
  }

+static int vfio_migration_query_flags(VFIODevice *vbasedev, uint64_t 
*mig_flags)
+{
+    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
+                                  sizeof(struct vfio_device_feature_migration),
+                              sizeof(uint64_t))] = {};
+    struct vfio_device_feature *feature = (void *)buf;
+    struct vfio_device_feature_migration *mig = (void *)feature->data;
+
+    feature->argsz = sizeof(buf);
+    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_MIGRATION;
+    if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
+        return -EOPNOTSUPP;
+    }
+
+    *mig_flags = mig->flags;
+
+    return 0;
+}
+
  static int vfio_migration_init(VFIODevice *vbasedev)
  {
      int ret;
@@ -798,6 +1067,7 @@ static int vfio_migration_init(VFIODevice *vbasedev)
      char id[256] = "";
      g_autofree char *path = NULL, *oid = NULL;
      struct vfio_region_info *info = NULL;
+    uint64_t mig_flags;

      if (!vbasedev->ops->vfio_get_object) {
          return -EINVAL;
@@ -808,32 +1078,48 @@ static int vfio_migration_init(VFIODevice *vbasedev)
          return -EINVAL;
      }

-    ret = vfio_get_dev_region_info(vbasedev,
-                                   VFIO_REGION_TYPE_MIGRATION_DEPRECATED,
-                                   VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED,
-                                   &info);
-    if (ret) {
-        return ret;
-    }
+    ret = vfio_migration_query_flags(vbasedev, &mig_flags);
+    if (!ret) {
+        /* Migration v2 */
+        /* Basic migration functionality must be supported */
+        if (!(mig_flags & VFIO_MIGRATION_STOP_COPY)) {
+            return -EOPNOTSUPP;
+        }
+        vbasedev->migration = g_new0(VFIOMigration, 1);
+        vbasedev->migration->data_buffer_size = VFIO_MIG_DATA_BUFFER_SIZE;
+        vbasedev->migration->data_buffer =
+            g_malloc0(vbasedev->migration->data_buffer_size);
+        vbasedev->migration->data_fd = -1;
+        vbasedev->migration->v2 = true;
+    } else {
+        /* Migration v1 */
+        ret = vfio_get_dev_region_info(vbasedev,
+                                       VFIO_REGION_TYPE_MIGRATION_DEPRECATED,
+                                       
VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED,
+                                       &info);
+        if (ret) {
+            return ret;
+        }

-    vbasedev->migration = g_new0(VFIOMigration, 1);
+        vbasedev->migration = g_new0(VFIOMigration, 1);

-    ret = vfio_region_setup(obj, vbasedev, &vbasedev->migration->region,
-                            info->index, "migration");
-    if (ret) {
-        error_report("%s: Failed to setup VFIO migration region %d: %s",
-                     vbasedev->name, info->index, strerror(-ret));
-        goto err;
-    }
+        ret = vfio_region_setup(obj, vbasedev, &vbasedev->migration->region,
+                                info->index, "migration");
+        if (ret) {
+            error_report("%s: Failed to setup VFIO migration region %d: %s",
+                         vbasedev->name, info->index, strerror(-ret));
+            goto err;
+        }

-    if (!vbasedev->migration->region.size) {
-        error_report("%s: Invalid zero-sized VFIO migration region %d",
-                     vbasedev->name, info->index);
-        ret = -EINVAL;
-        goto err;
-    }
+        if (!vbasedev->migration->region.size) {
+            error_report("%s: Invalid zero-sized VFIO migration region %d",
+                         vbasedev->name, info->index);
+            ret = -EINVAL;
+            goto err;
+        }

-    g_free(info);
+        g_free(info);
+    }

      migration = vbasedev->migration;
      migration->vbasedev = vbasedev;
@@ -846,11 +1132,20 @@ static int vfio_migration_init(VFIODevice *vbasedev)
      }
      strpadcpy(id, sizeof(id), path, '\0');

-    register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1,
-                         &savevm_vfio_v1_handlers, vbasedev);
+    if (migration->v2) {
+        register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1,
+                             &savevm_vfio_handlers, vbasedev);
+
+        migration->vm_state = qdev_add_vm_change_state_handler(
+            vbasedev->dev, vfio_vmstate_change, vbasedev);
+    } else {
+        register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1,
+                             &savevm_vfio_v1_handlers, vbasedev);
+
+        migration->vm_state = qdev_add_vm_change_state_handler(
+            vbasedev->dev, vfio_v1_vmstate_change, vbasedev);
+    }

-    migration->vm_state = qdev_add_vm_change_state_handler(
-        vbasedev->dev, vfio_v1_vmstate_change, vbasedev);
      migration->migration_state.notify = vfio_migration_state_notifier;
      add_migration_state_change_notifier(&migration->migration_state);
      return 0;
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index ac8b04f52a..6e8c5958b9 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -163,6 +163,8 @@ vfio_save_complete_precopy(const char *name) " (%s)"
  vfio_load_device_config_state(const char *name) " (%s)"
  vfio_load_state(const char *name, uint64_t data) " (%s) data 0x%"PRIx64
  vfio_v1_load_state_device_data(const char *name, uint64_t data_offset, uint64_t data_size) " 
(%s) Offset 0x%"PRIx64" size 0x%"PRIx64
+vfio_load_state_device_data(const char *name, uint64_t data_size) " (%s) size 
0x%"PRIx64
  vfio_load_cleanup(const char *name) " (%s)"
  vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container 
fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
  vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 
0x%"PRIx64" - 0x%"PRIx64
+vfio_save_block(const char *name, int data_size) " (%s) data_size %d"
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index bbaf72ba00..2ec3346fea 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -66,6 +66,11 @@ typedef struct VFIOMigration {
      int vm_running;
      Notifier migration_state;
      uint64_t pending_bytes;
+    enum vfio_device_mig_state device_state;
+    int data_fd;
+    void *data_buffer;
+    size_t data_buffer_size;
+    bool v2;
  } VFIOMigration;

  typedef struct VFIOAddressSpace {