On Wed, 23 Sep 2020 04:54:06 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
Whether the VFIO device supports migration or not is decided based of
migration region query. If migration region query is successful and migration
region initialization is successful then migration is supported else
migration is blocked.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/vfio/meson.build | 1 +
hw/vfio/migration.c | 142 ++++++++++++++++++++++++++++++++++++++++++
hw/vfio/trace-events | 5 ++
include/hw/vfio/vfio-common.h | 9 +++
4 files changed, 157 insertions(+)
create mode 100644 hw/vfio/migration.c
diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
index 37efa74018bc..da9af297a0c5 100644
--- a/hw/vfio/meson.build
+++ b/hw/vfio/meson.build
@@ -2,6 +2,7 @@ vfio_ss = ss.source_set()
vfio_ss.add(files(
'common.c',
'spapr.c',
+ 'migration.c',
))
vfio_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files(
'display.c',
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
new file mode 100644
index 000000000000..2f760f1f9c47
--- /dev/null
+++ b/hw/vfio/migration.c
@@ -0,0 +1,142 @@
+/*
+ * Migration support for VFIO devices
+ *
+ * Copyright NVIDIA, Inc. 2020
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <linux/vfio.h>
+
+#include "hw/vfio/vfio-common.h"
+#include "cpu.h"
+#include "migration/migration.h"
+#include "migration/qemu-file.h"
+#include "migration/register.h"
+#include "migration/blocker.h"
+#include "migration/misc.h"
+#include "qapi/error.h"
+#include "exec/ramlist.h"
+#include "exec/ram_addr.h"
+#include "pci.h"
+#include "trace.h"
+
+static void vfio_migration_region_exit(VFIODevice *vbasedev)
+{
+ VFIOMigration *migration = vbasedev->migration;
+
+ if (!migration) {
+ return;
+ }
+
+ if (migration->region.size) {
+ vfio_region_exit(&migration->region);
+ vfio_region_finalize(&migration->region);
+ }
+}
+
+static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
+{
+ VFIOMigration *migration = vbasedev->migration;
+ Object *obj = NULL;
Unnecessary initialization.
+ int ret = -EINVAL;
return -EINVAL below, this doesn't need to be initialized, use it for
storing actual return values.
+
+ obj = vbasedev->ops->vfio_get_object(vbasedev);
+ if (!obj) {
+ return ret;
+ }
vfio_migration_init() tests whether the vbasedev->ops supports
vfio_get_object, then calls this, then calls vfio_get_object itself
(added in a later patch, with a strange inconsistency in failure modes).
Wouldn't it make more sense for vfio_migration_init() to pass the
Object since that function also needs it (eventually) and actually does
the existence test?
+
+ ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
+ "migration");
+ if (ret) {
+ error_report("%s: Failed to setup VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+ goto err;
+ }
+
+ if (!migration->region.size) {
+ ret = -EINVAL;
+ error_report("%s: Invalid region size of VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+ goto err;
+ }
If the caller were to pass obj, this is nothing more than a wrapper for
calling vfio_region_setup(), which suggests to me we might not even
need this as a separate function outside of vfio_migration_init().
+
+ return 0;
+
+err:
+ vfio_migration_region_exit(vbasedev);
+ return ret;
+}
+
+static int vfio_migration_init(VFIODevice *vbasedev,
+ struct vfio_region_info *info)
+{
+ int ret = -EINVAL;
+
+ if (!vbasedev->ops->vfio_get_object) {
+ return ret;
+ }
+
+ vbasedev->migration = g_new0(VFIOMigration, 1);
+
+ ret = vfio_migration_region_init(vbasedev, info->index);
+ if (ret) {
+ error_report("%s: Failed to initialise migration region",
+ vbasedev->name);
+ g_free(vbasedev->migration);
+ vbasedev->migration = NULL;
+ }
+
+ return ret;
+}
+
+/* ---------------------------------------------------------------------- */
+
+int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
+{
+ struct vfio_region_info *info = NULL;
Not sure this initialization is strictly necessary either, but it also
seems to be a common convention for this function, so either way.
Connie, does vfio_ccw_get_region() leak this? It appears to call
vfio_get_dev_region_info() and vfio_get_region_info() several times with
the same pointer without freeing it between uses.
Thanks,
Alex
+ Error *local_err = NULL;
+ int ret;
+
+ ret = vfio_get_dev_region_info(vbasedev, VFIO_REGION_TYPE_MIGRATION,
+ VFIO_REGION_SUBTYPE_MIGRATION, &info);
+ if (ret) {
+ goto add_blocker;
+ }
+
+ ret = vfio_migration_init(vbasedev, info);
+ if (ret) {
+ goto add_blocker;
+ }
+
+ g_free(info);
+ trace_vfio_migration_probe(vbasedev->name, info->index);
+ return 0;
+
+add_blocker:
+ error_setg(&vbasedev->migration_blocker,
+ "VFIO device doesn't support migration");
+ g_free(info);
+
+ ret = migrate_add_blocker(vbasedev->migration_blocker, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ error_free(vbasedev->migration_blocker);
+ vbasedev->migration_blocker = NULL;
+ }
+ return ret;
+}
+
+void vfio_migration_finalize(VFIODevice *vbasedev)
+{
+ if (vbasedev->migration_blocker) {
+ migrate_del_blocker(vbasedev->migration_blocker);
+ error_free(vbasedev->migration_blocker);
+ vbasedev->migration_blocker = NULL;
+ }
+
+ vfio_migration_region_exit(vbasedev);
+ g_free(vbasedev->migration);
+}
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index a0c7b49a2ebc..8fe913175d85 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -145,3 +145,8 @@ vfio_display_edid_link_up(void) ""
vfio_display_edid_link_down(void) ""
vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%ux%u"
vfio_display_edid_write_error(void) ""
+
+
+# migration.c
+vfio_migration_probe(const char *name, uint32_t index) " (%s) Region %d"
+
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ba6169cd926e..8275c4c68f45 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -57,6 +57,10 @@ typedef struct VFIORegion {
uint8_t nr; /* cache the region number for debug */
} VFIORegion;
+typedef struct VFIOMigration {
+ VFIORegion region;
+} VFIOMigration;
+
typedef struct VFIOAddressSpace {
AddressSpace *as;
QLIST_HEAD(, VFIOContainer) containers;
@@ -113,6 +117,8 @@ typedef struct VFIODevice {
unsigned int num_irqs;
unsigned int num_regions;
unsigned int flags;
+ VFIOMigration *migration;
+ Error *migration_blocker;
} VFIODevice;
struct VFIODeviceOps {
@@ -204,4 +210,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
int vfio_spapr_remove_window(VFIOContainer *container,
hwaddr offset_within_address_space);
+int vfio_migration_probe(VFIODevice *vbasedev, Error **errp);
+void vfio_migration_finalize(VFIODevice *vbasedev);
+
#endif /* HW_VFIO_VFIO_COMMON_H */