| Message ID | 20221103161620.13120-13-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/migration: Implement VFIO migration protocol v2 | expand |
On Thu, 3 Nov 2022 18:16:15 +0200 Avihai Horon <avihaih@nvidia.com> wrote: > Add implementation of VFIO migration protocol v2. The two protocols, v1 > and v2, will co-exist and in next patch v1 protocol will be removed. > > There are several main differences between v1 and v2 protocols: > - VFIO device state is now represented as a finite state machine instead > of a bitmap. > > - Migration interface with kernel is now done using VFIO_DEVICE_FEATURE > ioctl and normal read() and write() instead of the migration region. > > - VFIO migration protocol v2 currently doesn't support the pre-copy > phase of migration. > > Detailed information about VFIO migration protocol v2 and difference > compared to v1 can be found here [1]. > > [1] > https://lore.kernel.org/all/20220224142024.147653-10-yishaih@nvidia.com/ > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > --- > hw/vfio/common.c | 19 +- > hw/vfio/migration.c | 386 ++++++++++++++++++++++++++++++---- > hw/vfio/trace-events | 4 + > include/hw/vfio/vfio-common.h | 5 + > 4 files changed, 375 insertions(+), 39 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 617e6cd901..0bdbd1586b 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -355,10 +355,18 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) > return false; > } > > - if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && > + if (!migration->v2 && > + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && > (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING)) { > return false; > } > + > + if (migration->v2 && > + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && > + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { > + return false; > + } > } > } > return true; > @@ -385,7 +393,14 @@ static bool vfio_devices_all_running_and_mig_active(VFIOContainer *container) > return false; > } > > - if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { > + if (!migration->v2 && > + migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { > + continue; > + } > + > + if (migration->v2 && > + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { > continue; > } else { > return false; > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index e784374453..62afc23a8c 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -44,8 +44,84 @@ > #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) > #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) > > +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) Add comment explaining heuristic of this size. > + > static int64_t bytes_transferred; > > +static const char *mig_state_to_str(enum vfio_device_mig_state state) > +{ > + switch (state) { > + case VFIO_DEVICE_STATE_ERROR: > + return "ERROR"; > + case VFIO_DEVICE_STATE_STOP: > + return "STOP"; > + case VFIO_DEVICE_STATE_RUNNING: > + return "RUNNING"; > + case VFIO_DEVICE_STATE_STOP_COPY: > + return "STOP_COPY"; > + case VFIO_DEVICE_STATE_RESUMING: > + return "RESUMING"; > + case VFIO_DEVICE_STATE_RUNNING_P2P: > + return "RUNNING_P2P"; > + default: > + return "UNKNOWN STATE"; > + } > +} > + > +static int vfio_migration_set_state(VFIODevice *vbasedev, > + enum vfio_device_mig_state new_state, > + enum vfio_device_mig_state recover_state) > +{ > + VFIOMigration *migration = vbasedev->migration; > + uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) + > + sizeof(struct vfio_device_feature_mig_state), > + sizeof(uint64_t))] = {}; > + struct vfio_device_feature *feature = (void *)buf; > + struct vfio_device_feature_mig_state *mig_state = (void *)feature->data; We can cast to the actual types rather than void* here. > + > + feature->argsz = sizeof(buf); > + feature->flags = > + VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE; > + mig_state->device_state = new_state; > + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { > + /* Try to set the device in some good state */ > + error_report( > + "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s", > + vbasedev->name, mig_state_to_str(new_state), > + strerror(errno), mig_state_to_str(recover_state)); > + > + mig_state->device_state = recover_state; > + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { > + hw_error("%s: Failed setting device in recover state, err: %s", > + vbasedev->name, strerror(errno)); > + } > + > + migration->device_state = recover_state; > + > + return -1; We could preserve -errno to return here. > + } > + > + if (mig_state->data_fd != -1) { > + if (migration->data_fd != -1) { > + /* > + * This can happen if the device is asynchronously reset and > + * terminates a data transfer. > + */ > + error_report("%s: data_fd out of sync", vbasedev->name); > + close(mig_state->data_fd); > + > + return -1; Should we go to recover_state here? Is migration->device_state invalid? -EBADF? > + } > + > + migration->data_fd = mig_state->data_fd; > + } > + migration->device_state = new_state; > + > + trace_vfio_migration_set_state(vbasedev->name, mig_state_to_str(new_state)); > + > + return 0; > +} > + > static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count, > off_t off, bool iswrite) > { > @@ -260,6 +336,20 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t *size) > return ret; > } > > +static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, > + uint64_t data_size) > +{ > + VFIOMigration *migration = vbasedev->migration; > + int ret; > + > + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); > + if (!ret) { > + trace_vfio_load_state_device_data(vbasedev->name, data_size); > + } > + > + return ret; > +} > + > static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, > uint64_t data_size) > { > @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) > return qemu_file_get_error(f); > } > > +static void vfio_migration_cleanup(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + > + close(migration->data_fd); > + migration->data_fd = -1; > +} > + > static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > { > VFIOMigration *migration = vbasedev->migration; > @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > > /* ---------------------------------------------------------------------- */ > > +static int vfio_save_setup(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + trace_vfio_save_setup(vbasedev->name); > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + return qemu_file_get_error(f); > +} > + > static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > return 0; > } > > +static void vfio_save_cleanup(void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + vfio_migration_cleanup(vbasedev); > + trace_vfio_save_cleanup(vbasedev->name); > +} > + > static void vfio_v1_save_cleanup(void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) > trace_vfio_save_cleanup(vbasedev->name); > } > > +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) There's a comment below, but that gets deleted in a later patch while we still use this as a fallback size. Some explanation of how this size is derived would be useful. Is this an estimate for mlx5? It seems muchtoo small for a GPU. For a fallback, should we set something here so large that we don't risk failing any SLA, ex. 100G? > +static void vfio_save_pending(void *opaque, uint64_t threshold_size, > + uint64_t *res_precopy, uint64_t *res_postcopy) > +{ > + VFIODevice *vbasedev = opaque; > + > + /* > + * VFIO migration protocol v2 currently doesn't have an API to get pending > + * device state size. Until such API is introduced, report some big > + * arbitrary pending size so the device will be taken into account for > + * downtime limit calculations. > + */ > + *res_postcopy += VFIO_MIG_PENDING_SIZE; > + > + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); > +} > + > static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, > uint64_t *res_precopy, uint64_t *res_postcopy) > { > @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) > return 0; > } > > +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ > +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) > +{ > + ssize_t data_size; > + > + data_size = read(migration->data_fd, migration->data_buffer, > + migration->data_buffer_size); > + if (data_size < 0) { > + return -1; Appears this could return -errno, granted it'll get swallowed in qemu_savevm_state_complete_precopy_iterable(), but it seems a bit cleaner here. > + } > + if (data_size == 0) { > + return 1; > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + qemu_put_be64(f, data_size); > + qemu_put_buffer(f, migration->data_buffer, data_size); > + bytes_transferred += data_size; > + > + trace_vfio_save_block(migration->vbasedev->name, data_size); > + > + return qemu_file_get_error(f); > +} > + > +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 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; > + } > + > + recover_state = VFIO_DEVICE_STATE_ERROR; > + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, > + recover_state); > + if (!ret) { > + trace_vfio_save_complete_precopy(vbasedev->name); > + } > + > + return ret; > +} > + > static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -589,6 +785,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; > @@ -616,6 +820,16 @@ 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; > @@ -658,7 +872,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; > } > @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) > return ret; > } > > +static const SaveVMHandlers savevm_vfio_handlers = { > + .save_setup = vfio_save_setup, > + .save_cleanup = vfio_save_cleanup, > + .save_live_pending = vfio_save_pending, > + .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, > @@ -693,6 +922,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), > + mig_state_to_str(new_state)); > +} > + > static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) > { > VFIODevice *vbasedev = opaque; > @@ -766,12 +1023,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); > + } 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); > + } > } > } > } > @@ -780,12 +1042,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; > @@ -794,6 +1079,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; > @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; > + vbasedev->migration->data_buffer_size = VFIO_MIG_DATA_BUFFER_SIZE; > + vbasedev->migration->data_buffer = > + g_malloc0(vbasedev->migration->data_buffer_size); So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the later addition of estimated device data size make any changes here? I'd think we'd want to scale the buffer to the minimum of the reported data size and some well documented heuristic for an upper bound. > + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > - vbasedev->migration->vm_running = runstate_is_running(); > + vbasedev->migration = g_new0(VFIOMigration, 1); > + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > + vbasedev->migration->vm_running = runstate_is_running(); > > - 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); It would probably make sense to scope info within this branch, but it goes away in the next patch anyway, so this is fine. Thanks, Alex > + } > > migration = vbasedev->migration; > migration->vbasedev = vbasedev; > @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" > > # migration.c > vfio_migration_probe(const char *name) " (%s)" > +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" > vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" > +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" > vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" > vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" > vfio_save_setup(const char *name) " (%s)" > @@ -163,6 +165,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 {
On 16/11/2022 20:29, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Thu, 3 Nov 2022 18:16:15 +0200 > Avihai Horon <avihaih@nvidia.com> wrote: > >> Add implementation of VFIO migration protocol v2. The two protocols, v1 >> and v2, will co-exist and in next patch v1 protocol will be removed. >> >> There are several main differences between v1 and v2 protocols: >> - VFIO device state is now represented as a finite state machine instead >> of a bitmap. >> >> - Migration interface with kernel is now done using VFIO_DEVICE_FEATURE >> ioctl and normal read() and write() instead of the migration region. >> >> - VFIO migration protocol v2 currently doesn't support the pre-copy >> phase of migration. >> >> Detailed information about VFIO migration protocol v2 and difference >> compared to v1 can be found here [1]. >> >> [1] >> https://lore.kernel.org/all/20220224142024.147653-10-yishaih@nvidia.com/ >> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com> >> --- >> hw/vfio/common.c | 19 +- >> hw/vfio/migration.c | 386 ++++++++++++++++++++++++++++++---- >> hw/vfio/trace-events | 4 + >> include/hw/vfio/vfio-common.h | 5 + >> 4 files changed, 375 insertions(+), 39 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 617e6cd901..0bdbd1586b 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -355,10 +355,18 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) >> return false; >> } >> >> - if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && >> + if (!migration->v2 && >> + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && >> (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING)) { >> return false; >> } >> + >> + if (migration->v2 && >> + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && >> + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || >> + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { >> + return false; >> + } >> } >> } >> return true; >> @@ -385,7 +393,14 @@ static bool vfio_devices_all_running_and_mig_active(VFIOContainer *container) >> return false; >> } >> >> - if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { >> + if (!migration->v2 && >> + migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { >> + continue; >> + } >> + >> + if (migration->v2 && >> + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || >> + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { >> continue; >> } else { >> return false; >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index e784374453..62afc23a8c 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -44,8 +44,84 @@ >> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) >> #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) >> >> +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) > Add comment explaining heuristic of this size. This is an arbitrary size we picked with mlx5 state size in mind. Increasing this size to higher values (128M, 1G) didn't improve performance in our testing. How about this comment: This is an initial value that doesn't consume much memory and provides good performance. Do you have other suggestion? > >> + >> static int64_t bytes_transferred; >> >> +static const char *mig_state_to_str(enum vfio_device_mig_state state) >> +{ >> + switch (state) { >> + case VFIO_DEVICE_STATE_ERROR: >> + return "ERROR"; >> + case VFIO_DEVICE_STATE_STOP: >> + return "STOP"; >> + case VFIO_DEVICE_STATE_RUNNING: >> + return "RUNNING"; >> + case VFIO_DEVICE_STATE_STOP_COPY: >> + return "STOP_COPY"; >> + case VFIO_DEVICE_STATE_RESUMING: >> + return "RESUMING"; >> + case VFIO_DEVICE_STATE_RUNNING_P2P: >> + return "RUNNING_P2P"; >> + default: >> + return "UNKNOWN STATE"; >> + } >> +} >> + >> +static int vfio_migration_set_state(VFIODevice *vbasedev, >> + enum vfio_device_mig_state new_state, >> + enum vfio_device_mig_state recover_state) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) + >> + sizeof(struct vfio_device_feature_mig_state), >> + sizeof(uint64_t))] = {}; >> + struct vfio_device_feature *feature = (void *)buf; >> + struct vfio_device_feature_mig_state *mig_state = (void *)feature->data; > We can cast to the actual types rather than void* here. Sure, will do. > >> + >> + feature->argsz = sizeof(buf); >> + feature->flags = >> + VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE; >> + mig_state->device_state = new_state; >> + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { >> + /* Try to set the device in some good state */ >> + error_report( >> + "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s", >> + vbasedev->name, mig_state_to_str(new_state), >> + strerror(errno), mig_state_to_str(recover_state)); >> + >> + mig_state->device_state = recover_state; >> + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { >> + hw_error("%s: Failed setting device in recover state, err: %s", >> + vbasedev->name, strerror(errno)); >> + } >> + >> + migration->device_state = recover_state; >> + >> + return -1; > We could preserve -errno to return here. Sure, will do. > >> + } >> + >> + if (mig_state->data_fd != -1) { >> + if (migration->data_fd != -1) { >> + /* >> + * This can happen if the device is asynchronously reset and >> + * terminates a data transfer. >> + */ >> + error_report("%s: data_fd out of sync", vbasedev->name); >> + close(mig_state->data_fd); >> + >> + return -1; > Should we go to recover_state here? Is migration->device_state > invalid? -EBADF? Yes, we should. Although VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE ioctl above succeeded, setting the device state didn't *really* succeed, as the data_fd went out of sync. So we should go to recover_state and return -EBADF. I will change it. >> + } >> + >> + migration->data_fd = mig_state->data_fd; >> + } >> + migration->device_state = new_state; >> + >> + trace_vfio_migration_set_state(vbasedev->name, mig_state_to_str(new_state)); >> + >> + return 0; >> +} >> + >> static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count, >> off_t off, bool iswrite) >> { >> @@ -260,6 +336,20 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t *size) >> return ret; >> } >> >> +static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, >> + uint64_t data_size) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + int ret; >> + >> + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); >> + if (!ret) { >> + trace_vfio_load_state_device_data(vbasedev->name, data_size); >> + } >> + >> + return ret; >> +} >> + >> static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, >> uint64_t data_size) >> { >> @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) >> return qemu_file_get_error(f); >> } >> >> +static void vfio_migration_cleanup(VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + >> + close(migration->data_fd); >> + migration->data_fd = -1; >> +} >> + >> static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >> { >> VFIOMigration *migration = vbasedev->migration; >> @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >> >> /* ---------------------------------------------------------------------- */ >> >> +static int vfio_save_setup(QEMUFile *f, void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + trace_vfio_save_setup(vbasedev->name); >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >> + >> + return qemu_file_get_error(f); >> +} >> + >> static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >> return 0; >> } >> >> +static void vfio_save_cleanup(void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + vfio_migration_cleanup(vbasedev); >> + trace_vfio_save_cleanup(vbasedev->name); >> +} >> + >> static void vfio_v1_save_cleanup(void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) >> trace_vfio_save_cleanup(vbasedev->name); >> } >> >> +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) > There's a comment below, but that gets deleted in a later patch while > we still use this as a fallback size. I will keep some variant of the comment in the later patch. > Some explanation of how this > size is derived would be useful. Is this an estimate for mlx5? It > seems muchtoo small for a GPU. For a fallback, should we set something > here so large that we don't risk failing any SLA, ex. 100G? In the KVM call we talked about setting this to 1G. mlx5 state size, when heavily utilized, should be in the order of hundreds of MBs. So eventually we decided to go for 500MB. However, I think you are right, we should set it to 100G to cover the worst case. Anyway, this fallback value is used only if kernel doesn't support VFIO_DEVICE_FEATURE_MIG_DATA_SIZE ioctl, which should not be the usual case AFAIU. I will also add a comment explaining the size choice. >> +static void vfio_save_pending(void *opaque, uint64_t threshold_size, >> + uint64_t *res_precopy, uint64_t *res_postcopy) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + /* >> + * VFIO migration protocol v2 currently doesn't have an API to get pending >> + * device state size. Until such API is introduced, report some big >> + * arbitrary pending size so the device will be taken into account for >> + * downtime limit calculations. >> + */ >> + *res_postcopy += VFIO_MIG_PENDING_SIZE; >> + >> + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); >> +} >> + >> static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, >> uint64_t *res_precopy, uint64_t *res_postcopy) >> { >> @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) >> return 0; >> } >> >> +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ >> +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) >> +{ >> + ssize_t data_size; >> + >> + data_size = read(migration->data_fd, migration->data_buffer, >> + migration->data_buffer_size); >> + if (data_size < 0) { >> + return -1; > Appears this could return -errno, granted it'll get swallowed in > qemu_savevm_state_complete_precopy_iterable(), but it seems a bit > cleaner here. Sure, I will change it. >> + } >> + if (data_size == 0) { >> + return 1; >> + } >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >> + qemu_put_be64(f, data_size); >> + qemu_put_buffer(f, migration->data_buffer, data_size); >> + bytes_transferred += data_size; >> + >> + trace_vfio_save_block(migration->vbasedev->name, data_size); >> + >> + return qemu_file_get_error(f); >> +} >> + >> +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 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; >> + } >> + >> + recover_state = VFIO_DEVICE_STATE_ERROR; >> + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, >> + recover_state); >> + if (!ret) { >> + trace_vfio_save_complete_precopy(vbasedev->name); >> + } >> + >> + return ret; >> +} >> + >> static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -589,6 +785,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; >> @@ -616,6 +820,16 @@ 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; >> @@ -658,7 +872,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; >> } >> @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) >> return ret; >> } >> >> +static const SaveVMHandlers savevm_vfio_handlers = { >> + .save_setup = vfio_save_setup, >> + .save_cleanup = vfio_save_cleanup, >> + .save_live_pending = vfio_save_pending, >> + .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, >> @@ -693,6 +922,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), >> + mig_state_to_str(new_state)); >> +} >> + >> static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) >> { >> VFIODevice *vbasedev = opaque; >> @@ -766,12 +1023,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); >> + } 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); >> + } >> } >> } >> } >> @@ -780,12 +1042,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; >> @@ -794,6 +1079,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; >> @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; >> + vbasedev->migration->data_buffer_size = VFIO_MIG_DATA_BUFFER_SIZE; >> + vbasedev->migration->data_buffer = >> + g_malloc0(vbasedev->migration->data_buffer_size); > So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the > later addition of estimated device data size make any changes here? > I'd think we'd want to scale the buffer to the minimum of the reported > data size and some well documented heuristic for an upper bound. As I wrote above, increasing this size to higher values (128M, 1G) didn't improve performance in our testing. We can always change it later on if some other heuristics are proven to improve performance. Thanks! >> + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >> - vbasedev->migration->vm_running = runstate_is_running(); >> + vbasedev->migration = g_new0(VFIOMigration, 1); >> + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >> + vbasedev->migration->vm_running = runstate_is_running(); >> >> - 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); > > It would probably make sense to scope info within this branch, but it > goes away in the next patch anyway, so this is fine. Thanks, > > Alex > >> + } >> >> migration = vbasedev->migration; >> migration->vbasedev = vbasedev; >> @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 >> --- a/hw/vfio/trace-events >> +++ b/hw/vfio/trace-events >> @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" >> >> # migration.c >> vfio_migration_probe(const char *name) " (%s)" >> +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" >> vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" >> +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" >> vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" >> vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" >> vfio_save_setup(const char *name) " (%s)" >> @@ -163,6 +165,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 {
On Thu, Nov 17, 2022 at 07:07:10PM +0200, Avihai Horon wrote: > > > + } > > > + > > > + if (mig_state->data_fd != -1) { > > > + if (migration->data_fd != -1) { > > > + /* > > > + * This can happen if the device is asynchronously reset and > > > + * terminates a data transfer. > > > + */ > > > + error_report("%s: data_fd out of sync", vbasedev->name); > > > + close(mig_state->data_fd); > > > + > > > + return -1; > > Should we go to recover_state here? Is migration->device_state > > invalid? -EBADF? > > Yes, we should. > Although VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE ioctl above succeeded, setting > the device state didn't *really* succeed, as the data_fd went out of sync. > So we should go to recover_state and return -EBADF. The state did succeed and it is now "new_state". Getting an unexpected data_fd means it did something like RUNNING->PRE_COPY_P2P when the code was expecting PRE_COPY->PRE_COPY_P2P. It is actually in PRE_COPY_P2P but the in-progress migration must be stopped and the kernel would have made the migration->data_fd permanently return some error when it went async to RUNNING. The recovery is to resart the migration (of this device?) from the start. Jason
On Thu, 17 Nov 2022 19:07:10 +0200 Avihai Horon <avihaih@nvidia.com> wrote: > On 16/11/2022 20:29, Alex Williamson wrote: > > On Thu, 3 Nov 2022 18:16:15 +0200 > > Avihai Horon <avihaih@nvidia.com> wrote: > >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > >> index e784374453..62afc23a8c 100644 > >> --- a/hw/vfio/migration.c > >> +++ b/hw/vfio/migration.c > >> @@ -44,8 +44,84 @@ > >> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) > >> #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) > >> > >> +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) > > Add comment explaining heuristic of this size. > > This is an arbitrary size we picked with mlx5 state size in mind. > Increasing this size to higher values (128M, 1G) didn't improve > performance in our testing. > > How about this comment: > This is an initial value that doesn't consume much memory and provides > good performance. > > Do you have other suggestion? I'd lean more towards your description above, ex: /* * This is an arbitrary size based on migration of mlx5 devices, where * the worst case total device migration size is on the order of 100s * of MB. Testing with larger values, ex. 128MB and 1GB, did not show * a performance improvement. */ I think that provides sufficient information for someone who might come later to have an understanding of the basis if they want to try to optimize further. > >> @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; > >> + vbasedev->migration->data_buffer_size = VFIO_MIG_DATA_BUFFER_SIZE; > >> + vbasedev->migration->data_buffer = > >> + g_malloc0(vbasedev->migration->data_buffer_size); > > So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the > > later addition of estimated device data size make any changes here? > > I'd think we'd want to scale the buffer to the minimum of the reported > > data size and some well documented heuristic for an upper bound. > > As I wrote above, increasing this size to higher values (128M, 1G) > didn't improve performance in our testing. > We can always change it later on if some other heuristics are proven to > improve performance. Note that I'm asking about a minimum buffer size, for example if hisi_acc reports only 10s of KB for an estimated device size, why would we still allocate VFIO_MIG_DATA_BUFFER_SIZE here? Thanks, Alex
On 17/11/2022 19:24, Jason Gunthorpe wrote: > On Thu, Nov 17, 2022 at 07:07:10PM +0200, Avihai Horon wrote: >>>> + } >>>> + >>>> + if (mig_state->data_fd != -1) { >>>> + if (migration->data_fd != -1) { >>>> + /* >>>> + * This can happen if the device is asynchronously reset and >>>> + * terminates a data transfer. >>>> + */ >>>> + error_report("%s: data_fd out of sync", vbasedev->name); >>>> + close(mig_state->data_fd); >>>> + >>>> + return -1; >>> Should we go to recover_state here? Is migration->device_state >>> invalid? -EBADF? >> Yes, we should. >> Although VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE ioctl above succeeded, setting >> the device state didn't *really* succeed, as the data_fd went out of sync. >> So we should go to recover_state and return -EBADF. > The state did succeed and it is now "new_state". Getting an > unexpected data_fd means it did something like RUNNING->PRE_COPY_P2P > when the code was expecting PRE_COPY->PRE_COPY_P2P. > > It is actually in PRE_COPY_P2P but the in-progress migration must be > stopped and the kernel would have made the migration->data_fd > permanently return some error when it went async to RUNNING. > > The recovery is to resart the migration (of this device?) from the > start. Yes, and restart is what's happening here - the -EBADF that we return here will cause the migration to be aborted. I didn't mean that we should go to recover_state *instead* of returning an error. But rethinking about it, I think you are right - recover_state should be used only if we can't set the device in new_state (i.e., there is some error in device functionality). In the "data_fd out of sync" case, we did set the device in new_state (no error in device functionality), but data_fd got mixed up, so we should just abort migration and restart it again. So bottom line, I think we should just return -EBADF here to abort migration.
On 17/11/2022 19:38, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Thu, 17 Nov 2022 19:07:10 +0200 > Avihai Horon <avihaih@nvidia.com> wrote: >> On 16/11/2022 20:29, Alex Williamson wrote: >>> On Thu, 3 Nov 2022 18:16:15 +0200 >>> Avihai Horon <avihaih@nvidia.com> wrote: >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>> index e784374453..62afc23a8c 100644 >>>> --- a/hw/vfio/migration.c >>>> +++ b/hw/vfio/migration.c >>>> @@ -44,8 +44,84 @@ >>>> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) >>>> #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) >>>> >>>> +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) >>> Add comment explaining heuristic of this size. >> This is an arbitrary size we picked with mlx5 state size in mind. >> Increasing this size to higher values (128M, 1G) didn't improve >> performance in our testing. >> >> How about this comment: >> This is an initial value that doesn't consume much memory and provides >> good performance. >> >> Do you have other suggestion? > I'd lean more towards your description above, ex: > > /* > * This is an arbitrary size based on migration of mlx5 devices, where > * the worst case total device migration size is on the order of 100s > * of MB. Testing with larger values, ex. 128MB and 1GB, did not show > * a performance improvement. > */ > > I think that provides sufficient information for someone who might come > later to have an understanding of the basis if they want to try to > optimize further. OK, sounds good, I will add a comment like this. >>>> @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; >>>> + vbasedev->migration->data_buffer_size = VFIO_MIG_DATA_BUFFER_SIZE; >>>> + vbasedev->migration->data_buffer = >>>> + g_malloc0(vbasedev->migration->data_buffer_size); >>> So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the >>> later addition of estimated device data size make any changes here? >>> I'd think we'd want to scale the buffer to the minimum of the reported >>> data size and some well documented heuristic for an upper bound. >> As I wrote above, increasing this size to higher values (128M, 1G) >> didn't improve performance in our testing. >> We can always change it later on if some other heuristics are proven to >> improve performance. > Note that I'm asking about a minimum buffer size, for example if > hisi_acc reports only 10s of KB for an estimated device size, why would > we still allocate VFIO_MIG_DATA_BUFFER_SIZE here? Thanks, This buffer is rather small and has little memory footprint. Do you think it is worth the extra complexity of resizing the buffer?
* Avihai Horon (avihaih@nvidia.com) wrote: <snip> > + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); > + if (!ret) { > + trace_vfio_load_state_device_data(vbasedev->name, data_size); > + > + } I notice you had a few cases like that; I wouldn't bother making that conditional - just add 'ret' to the trace parameters; that way if it fails then you can see that in the trace, and it's simpler anyway. Dave > + > + return ret; > +} > + > static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, > uint64_t data_size) > { > @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) > return qemu_file_get_error(f); > } > > +static void vfio_migration_cleanup(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + > + close(migration->data_fd); > + migration->data_fd = -1; > +} > + > static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > { > VFIOMigration *migration = vbasedev->migration; > @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > > /* ---------------------------------------------------------------------- */ > > +static int vfio_save_setup(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + trace_vfio_save_setup(vbasedev->name); > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + return qemu_file_get_error(f); > +} > + > static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > return 0; > } > > +static void vfio_save_cleanup(void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + vfio_migration_cleanup(vbasedev); > + trace_vfio_save_cleanup(vbasedev->name); > +} > + > static void vfio_v1_save_cleanup(void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) > trace_vfio_save_cleanup(vbasedev->name); > } > > +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) > +static void vfio_save_pending(void *opaque, uint64_t threshold_size, > + uint64_t *res_precopy, uint64_t *res_postcopy) > +{ > + VFIODevice *vbasedev = opaque; > + > + /* > + * VFIO migration protocol v2 currently doesn't have an API to get pending > + * device state size. Until such API is introduced, report some big > + * arbitrary pending size so the device will be taken into account for > + * downtime limit calculations. > + */ > + *res_postcopy += VFIO_MIG_PENDING_SIZE; > + > + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); > +} > + > static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, > uint64_t *res_precopy, uint64_t *res_postcopy) > { > @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) > return 0; > } > > +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ > +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) > +{ > + ssize_t data_size; > + > + data_size = read(migration->data_fd, migration->data_buffer, > + migration->data_buffer_size); > + if (data_size < 0) { > + return -1; > + } > + if (data_size == 0) { > + return 1; > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + qemu_put_be64(f, data_size); > + qemu_put_buffer(f, migration->data_buffer, data_size); > + bytes_transferred += data_size; > + > + trace_vfio_save_block(migration->vbasedev->name, data_size); > + > + return qemu_file_get_error(f); > +} > + > +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 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; > + } > + > + recover_state = VFIO_DEVICE_STATE_ERROR; > + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, > + recover_state); > + if (!ret) { > + trace_vfio_save_complete_precopy(vbasedev->name); > + } > + > + return ret; > +} > + > static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) > { > VFIODevice *vbasedev = opaque; > @@ -589,6 +785,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; > @@ -616,6 +820,16 @@ 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; > @@ -658,7 +872,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; > } > @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) > return ret; > } > > +static const SaveVMHandlers savevm_vfio_handlers = { > + .save_setup = vfio_save_setup, > + .save_cleanup = vfio_save_cleanup, > + .save_live_pending = vfio_save_pending, > + .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, > @@ -693,6 +922,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), > + mig_state_to_str(new_state)); > +} > + > static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) > { > VFIODevice *vbasedev = opaque; > @@ -766,12 +1023,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); > + } 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); > + } > } > } > } > @@ -780,12 +1042,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; > @@ -794,6 +1079,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; > @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; > + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > - vbasedev->migration->vm_running = runstate_is_running(); > + vbasedev->migration = g_new0(VFIOMigration, 1); > + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > + vbasedev->migration->vm_running = runstate_is_running(); > > - 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; > @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" > > # migration.c > vfio_migration_probe(const char *name) " (%s)" > +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" > vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" > +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" > vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" > vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" > vfio_save_setup(const char *name) " (%s)" > @@ -163,6 +165,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 { > -- > 2.21.3 >
On 23/11/2022 20:59, Dr. David Alan Gilbert wrote: > External email: Use caution opening links or attachments > > > * Avihai Horon (avihaih@nvidia.com) wrote: > > <snip> > >> + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); >> + if (!ret) { >> + trace_vfio_load_state_device_data(vbasedev->name, data_size); >> + >> + } > I notice you had a few cases like that; I wouldn't bother making that > conditional - just add 'ret' to the trace parameters; that way if it > fails then you can see that in the trace, and it's simpler anyway. If we add ret to traces such as this, shouldn’t we add ret to the other traces as well, to keep consistent trace format? In that case, is it worth the trouble? Alternatively, we can print the traces regardless of success or failure of the function to better reflect the flow of execution. WDYT? Thanks. > Dave > >> + >> + return ret; >> +} >> + >> static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, >> uint64_t data_size) >> { >> @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) >> return qemu_file_get_error(f); >> } >> >> +static void vfio_migration_cleanup(VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + >> + close(migration->data_fd); >> + migration->data_fd = -1; >> +} >> + >> static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >> { >> VFIOMigration *migration = vbasedev->migration; >> @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >> >> /* ---------------------------------------------------------------------- */ >> >> +static int vfio_save_setup(QEMUFile *f, void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + trace_vfio_save_setup(vbasedev->name); >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >> + >> + return qemu_file_get_error(f); >> +} >> + >> static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >> return 0; >> } >> >> +static void vfio_save_cleanup(void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + vfio_migration_cleanup(vbasedev); >> + trace_vfio_save_cleanup(vbasedev->name); >> +} >> + >> static void vfio_v1_save_cleanup(void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) >> trace_vfio_save_cleanup(vbasedev->name); >> } >> >> +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) >> +static void vfio_save_pending(void *opaque, uint64_t threshold_size, >> + uint64_t *res_precopy, uint64_t *res_postcopy) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + /* >> + * VFIO migration protocol v2 currently doesn't have an API to get pending >> + * device state size. Until such API is introduced, report some big >> + * arbitrary pending size so the device will be taken into account for >> + * downtime limit calculations. >> + */ >> + *res_postcopy += VFIO_MIG_PENDING_SIZE; >> + >> + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); >> +} >> + >> static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, >> uint64_t *res_precopy, uint64_t *res_postcopy) >> { >> @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) >> return 0; >> } >> >> +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ >> +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) >> +{ >> + ssize_t data_size; >> + >> + data_size = read(migration->data_fd, migration->data_buffer, >> + migration->data_buffer_size); >> + if (data_size < 0) { >> + return -1; >> + } >> + if (data_size == 0) { >> + return 1; >> + } >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >> + qemu_put_be64(f, data_size); >> + qemu_put_buffer(f, migration->data_buffer, data_size); >> + bytes_transferred += data_size; >> + >> + trace_vfio_save_block(migration->vbasedev->name, data_size); >> + >> + return qemu_file_get_error(f); >> +} >> + >> +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 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; >> + } >> + >> + recover_state = VFIO_DEVICE_STATE_ERROR; >> + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, >> + recover_state); >> + if (!ret) { >> + trace_vfio_save_complete_precopy(vbasedev->name); >> + } >> + >> + return ret; >> +} >> + >> static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) >> { >> VFIODevice *vbasedev = opaque; >> @@ -589,6 +785,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; >> @@ -616,6 +820,16 @@ 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; >> @@ -658,7 +872,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; >> } >> @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) >> return ret; >> } >> >> +static const SaveVMHandlers savevm_vfio_handlers = { >> + .save_setup = vfio_save_setup, >> + .save_cleanup = vfio_save_cleanup, >> + .save_live_pending = vfio_save_pending, >> + .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, >> @@ -693,6 +922,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), >> + mig_state_to_str(new_state)); >> +} >> + >> static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) >> { >> VFIODevice *vbasedev = opaque; >> @@ -766,12 +1023,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); >> + } 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); >> + } >> } >> } >> } >> @@ -780,12 +1042,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; >> @@ -794,6 +1079,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; >> @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; >> + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >> - vbasedev->migration->vm_running = runstate_is_running(); >> + vbasedev->migration = g_new0(VFIOMigration, 1); >> + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >> + vbasedev->migration->vm_running = runstate_is_running(); >> >> - 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; >> @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 >> --- a/hw/vfio/trace-events >> +++ b/hw/vfio/trace-events >> @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" >> >> # migration.c >> vfio_migration_probe(const char *name) " (%s)" >> +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" >> vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" >> +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" >> vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" >> vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" >> vfio_save_setup(const char *name) " (%s)" >> @@ -163,6 +165,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 { >> -- >> 2.21.3 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On 20/11/2022 11:34, Avihai Horon wrote: > > On 17/11/2022 19:38, Alex Williamson wrote: >> External email: Use caution opening links or attachments >> >> >> On Thu, 17 Nov 2022 19:07:10 +0200 >> Avihai Horon <avihaih@nvidia.com> wrote: >>> On 16/11/2022 20:29, Alex Williamson wrote: >>>> On Thu, 3 Nov 2022 18:16:15 +0200 >>>> Avihai Horon <avihaih@nvidia.com> wrote: >>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>>> index e784374453..62afc23a8c 100644 >>>>> --- a/hw/vfio/migration.c >>>>> +++ b/hw/vfio/migration.c >>>>> @@ -44,8 +44,84 @@ >>>>> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) >>>>> #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) >>>>> >>>>> +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) >>>> Add comment explaining heuristic of this size. >>> This is an arbitrary size we picked with mlx5 state size in mind. >>> Increasing this size to higher values (128M, 1G) didn't improve >>> performance in our testing. >>> >>> How about this comment: >>> This is an initial value that doesn't consume much memory and provides >>> good performance. >>> >>> Do you have other suggestion? >> I'd lean more towards your description above, ex: >> >> /* >> * This is an arbitrary size based on migration of mlx5 devices, where >> * the worst case total device migration size is on the order of 100s >> * of MB. Testing with larger values, ex. 128MB and 1GB, did not show >> * a performance improvement. >> */ >> >> I think that provides sufficient information for someone who might come >> later to have an understanding of the basis if they want to try to >> optimize further. > > OK, sounds good, I will add a comment like this. > >>>>> @@ -804,34 +1090,51 @@ 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->device_state = >>>>> VFIO_DEVICE_STATE_RUNNING; >>>>> + vbasedev->migration->data_buffer_size = >>>>> VFIO_MIG_DATA_BUFFER_SIZE; >>>>> + vbasedev->migration->data_buffer = >>>>> + g_malloc0(vbasedev->migration->data_buffer_size); >>>> So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the >>>> later addition of estimated device data size make any changes here? >>>> I'd think we'd want to scale the buffer to the minimum of the reported >>>> data size and some well documented heuristic for an upper bound. >>> As I wrote above, increasing this size to higher values (128M, 1G) >>> didn't improve performance in our testing. >>> We can always change it later on if some other heuristics are proven to >>> improve performance. >> Note that I'm asking about a minimum buffer size, for example if >> hisi_acc reports only 10s of KB for an estimated device size, why would >> we still allocate VFIO_MIG_DATA_BUFFER_SIZE here? Thanks, > > This buffer is rather small and has little memory footprint. > Do you think it is worth the extra complexity of resizing the buffer? > Alex, WDYT? Note that the reported estimated size is dynamic and might change from query to the other, potentially leaving us with smaller buffer size. Also, as part of v4 I moved this allocation to vfio_save_setup(), so it will be allocated only during migration (when it's actually used) and only by src side. Thanks.
* Avihai Horon (avihaih@nvidia.com) wrote: > > On 23/11/2022 20:59, Dr. David Alan Gilbert wrote: > > External email: Use caution opening links or attachments > > > > > > * Avihai Horon (avihaih@nvidia.com) wrote: > > > > <snip> > > > > > + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); > > > + if (!ret) { > > > + trace_vfio_load_state_device_data(vbasedev->name, data_size); > > > + > > > + } > > I notice you had a few cases like that; I wouldn't bother making that > > conditional - just add 'ret' to the trace parameters; that way if it > > fails then you can see that in the trace, and it's simpler anyway. > > If we add ret to traces such as this, shouldn’t we add ret to the other > traces as well, to keep consistent trace format? > In that case, is it worth the trouble? Traces are for humans; it's nice to keep the same format, but not required. > > Alternatively, we can print the traces regardless of success or failure of > the function to better reflect the flow of execution. > WDYT? I'd just add ret to the ones you're changing. The important thing about traceing is that they've got what you need to debug things! Dave > > Thanks. > > > Dave > > > > > + > > > + return ret; > > > +} > > > + > > > static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, > > > uint64_t data_size) > > > { > > > @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) > > > return qemu_file_get_error(f); > > > } > > > > > > +static void vfio_migration_cleanup(VFIODevice *vbasedev) > > > +{ > > > + VFIOMigration *migration = vbasedev->migration; > > > + > > > + close(migration->data_fd); > > > + migration->data_fd = -1; > > > +} > > > + > > > static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > > > { > > > VFIOMigration *migration = vbasedev->migration; > > > @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) > > > > > > /* ---------------------------------------------------------------------- */ > > > > > > +static int vfio_save_setup(QEMUFile *f, void *opaque) > > > +{ > > > + VFIODevice *vbasedev = opaque; > > > + > > > + trace_vfio_save_setup(vbasedev->name); > > > + > > > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); > > > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > > > + > > > + return qemu_file_get_error(f); > > > +} > > > + > > > static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > > > { > > > VFIODevice *vbasedev = opaque; > > > @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) > > > return 0; > > > } > > > > > > +static void vfio_save_cleanup(void *opaque) > > > +{ > > > + VFIODevice *vbasedev = opaque; > > > + > > > + vfio_migration_cleanup(vbasedev); > > > + trace_vfio_save_cleanup(vbasedev->name); > > > +} > > > + > > > static void vfio_v1_save_cleanup(void *opaque) > > > { > > > VFIODevice *vbasedev = opaque; > > > @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) > > > trace_vfio_save_cleanup(vbasedev->name); > > > } > > > > > > +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) > > > +static void vfio_save_pending(void *opaque, uint64_t threshold_size, > > > + uint64_t *res_precopy, uint64_t *res_postcopy) > > > +{ > > > + VFIODevice *vbasedev = opaque; > > > + > > > + /* > > > + * VFIO migration protocol v2 currently doesn't have an API to get pending > > > + * device state size. Until such API is introduced, report some big > > > + * arbitrary pending size so the device will be taken into account for > > > + * downtime limit calculations. > > > + */ > > > + *res_postcopy += VFIO_MIG_PENDING_SIZE; > > > + > > > + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); > > > +} > > > + > > > static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, > > > uint64_t *res_precopy, uint64_t *res_postcopy) > > > { > > > @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) > > > return 0; > > > } > > > > > > +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ > > > +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) > > > +{ > > > + ssize_t data_size; > > > + > > > + data_size = read(migration->data_fd, migration->data_buffer, > > > + migration->data_buffer_size); > > > + if (data_size < 0) { > > > + return -1; > > > + } > > > + if (data_size == 0) { > > > + return 1; > > > + } > > > + > > > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > > > + qemu_put_be64(f, data_size); > > > + qemu_put_buffer(f, migration->data_buffer, data_size); > > > + bytes_transferred += data_size; > > > + > > > + trace_vfio_save_block(migration->vbasedev->name, data_size); > > > + > > > + return qemu_file_get_error(f); > > > +} > > > + > > > +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 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; > > > + } > > > + > > > + recover_state = VFIO_DEVICE_STATE_ERROR; > > > + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, > > > + recover_state); > > > + if (!ret) { > > > + trace_vfio_save_complete_precopy(vbasedev->name); > > > + } > > > + > > > + return ret; > > > +} > > > + > > > static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) > > > { > > > VFIODevice *vbasedev = opaque; > > > @@ -589,6 +785,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; > > > @@ -616,6 +820,16 @@ 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; > > > @@ -658,7 +872,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; > > > } > > > @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) > > > return ret; > > > } > > > > > > +static const SaveVMHandlers savevm_vfio_handlers = { > > > + .save_setup = vfio_save_setup, > > > + .save_cleanup = vfio_save_cleanup, > > > + .save_live_pending = vfio_save_pending, > > > + .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, > > > @@ -693,6 +922,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), > > > + mig_state_to_str(new_state)); > > > +} > > > + > > > static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) > > > { > > > VFIODevice *vbasedev = opaque; > > > @@ -766,12 +1023,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); > > > + } 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); > > > + } > > > } > > > } > > > } > > > @@ -780,12 +1042,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; > > > @@ -794,6 +1079,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; > > > @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; > > > + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > > > - vbasedev->migration->vm_running = runstate_is_running(); > > > + vbasedev->migration = g_new0(VFIOMigration, 1); > > > + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; > > > + vbasedev->migration->vm_running = runstate_is_running(); > > > > > > - 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; > > > @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 > > > --- a/hw/vfio/trace-events > > > +++ b/hw/vfio/trace-events > > > @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" > > > > > > # migration.c > > > vfio_migration_probe(const char *name) " (%s)" > > > +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" > > > vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" > > > +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" > > > vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" > > > vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" > > > vfio_save_setup(const char *name) " (%s)" > > > @@ -163,6 +165,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 { > > > -- > > > 2.21.3 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > >
On 24/11/2022 15:28, Dr. David Alan Gilbert wrote: > External email: Use caution opening links or attachments > > > * Avihai Horon (avihaih@nvidia.com) wrote: >> On 23/11/2022 20:59, Dr. David Alan Gilbert wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> * Avihai Horon (avihaih@nvidia.com) wrote: >>> >>> <snip> >>> >>>> + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); >>>> + if (!ret) { >>>> + trace_vfio_load_state_device_data(vbasedev->name, data_size); >>>> + >>>> + } >>> I notice you had a few cases like that; I wouldn't bother making that >>> conditional - just add 'ret' to the trace parameters; that way if it >>> fails then you can see that in the trace, and it's simpler anyway. >> If we add ret to traces such as this, shouldn’t we add ret to the other >> traces as well, to keep consistent trace format? >> In that case, is it worth the trouble? > Traces are for humans; it's nice to keep the same format, but not > required. >> Alternatively, we can print the traces regardless of success or failure of >> the function to better reflect the flow of execution. >> WDYT? > I'd just add ret to the ones you're changing. > The important thing about traceing is that they've got what you need to > debug things! OK, fair enough. I will add ret to these traces. Thanks. > Dave > >> Thanks. >> >>> Dave >>> >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, >>>> uint64_t data_size) >>>> { >>>> @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) >>>> return qemu_file_get_error(f); >>>> } >>>> >>>> +static void vfio_migration_cleanup(VFIODevice *vbasedev) >>>> +{ >>>> + VFIOMigration *migration = vbasedev->migration; >>>> + >>>> + close(migration->data_fd); >>>> + migration->data_fd = -1; >>>> +} >>>> + >>>> static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >>>> { >>>> VFIOMigration *migration = vbasedev->migration; >>>> @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) >>>> >>>> /* ---------------------------------------------------------------------- */ >>>> >>>> +static int vfio_save_setup(QEMUFile *f, void *opaque) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + >>>> + trace_vfio_save_setup(vbasedev->name); >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >>>> + >>>> + return qemu_file_get_error(f); >>>> +} >>>> + >>>> static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >>>> { >>>> VFIODevice *vbasedev = opaque; >>>> @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) >>>> return 0; >>>> } >>>> >>>> +static void vfio_save_cleanup(void *opaque) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + >>>> + vfio_migration_cleanup(vbasedev); >>>> + trace_vfio_save_cleanup(vbasedev->name); >>>> +} >>>> + >>>> static void vfio_v1_save_cleanup(void *opaque) >>>> { >>>> VFIODevice *vbasedev = opaque; >>>> @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) >>>> trace_vfio_save_cleanup(vbasedev->name); >>>> } >>>> >>>> +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) >>>> +static void vfio_save_pending(void *opaque, uint64_t threshold_size, >>>> + uint64_t *res_precopy, uint64_t *res_postcopy) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + >>>> + /* >>>> + * VFIO migration protocol v2 currently doesn't have an API to get pending >>>> + * device state size. Until such API is introduced, report some big >>>> + * arbitrary pending size so the device will be taken into account for >>>> + * downtime limit calculations. >>>> + */ >>>> + *res_postcopy += VFIO_MIG_PENDING_SIZE; >>>> + >>>> + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); >>>> +} >>>> + >>>> static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, >>>> uint64_t *res_precopy, uint64_t *res_postcopy) >>>> { >>>> @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) >>>> return 0; >>>> } >>>> >>>> +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ >>>> +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) >>>> +{ >>>> + ssize_t data_size; >>>> + >>>> + data_size = read(migration->data_fd, migration->data_buffer, >>>> + migration->data_buffer_size); >>>> + if (data_size < 0) { >>>> + return -1; >>>> + } >>>> + if (data_size == 0) { >>>> + return 1; >>>> + } >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >>>> + qemu_put_be64(f, data_size); >>>> + qemu_put_buffer(f, migration->data_buffer, data_size); >>>> + bytes_transferred += data_size; >>>> + >>>> + trace_vfio_save_block(migration->vbasedev->name, data_size); >>>> + >>>> + return qemu_file_get_error(f); >>>> +} >>>> + >>>> +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 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; >>>> + } >>>> + >>>> + recover_state = VFIO_DEVICE_STATE_ERROR; >>>> + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, >>>> + recover_state); >>>> + if (!ret) { >>>> + trace_vfio_save_complete_precopy(vbasedev->name); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) >>>> { >>>> VFIODevice *vbasedev = opaque; >>>> @@ -589,6 +785,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; >>>> @@ -616,6 +820,16 @@ 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; >>>> @@ -658,7 +872,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; >>>> } >>>> @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) >>>> return ret; >>>> } >>>> >>>> +static const SaveVMHandlers savevm_vfio_handlers = { >>>> + .save_setup = vfio_save_setup, >>>> + .save_cleanup = vfio_save_cleanup, >>>> + .save_live_pending = vfio_save_pending, >>>> + .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, >>>> @@ -693,6 +922,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), >>>> + mig_state_to_str(new_state)); >>>> +} >>>> + >>>> static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) >>>> { >>>> VFIODevice *vbasedev = opaque; >>>> @@ -766,12 +1023,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); >>>> + } 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); >>>> + } >>>> } >>>> } >>>> } >>>> @@ -780,12 +1042,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; >>>> @@ -794,6 +1079,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; >>>> @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; >>>> + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >>>> - vbasedev->migration->vm_running = runstate_is_running(); >>>> + vbasedev->migration = g_new0(VFIOMigration, 1); >>>> + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; >>>> + vbasedev->migration->vm_running = runstate_is_running(); >>>> >>>> - 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; >>>> @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 >>>> --- a/hw/vfio/trace-events >>>> +++ b/hw/vfio/trace-events >>>> @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" >>>> >>>> # migration.c >>>> vfio_migration_probe(const char *name) " (%s)" >>>> +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" >>>> vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" >>>> +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" >>>> vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" >>>> vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" >>>> vfio_save_setup(const char *name) " (%s)" >>>> @@ -163,6 +165,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 { >>>> -- >>>> 2.21.3 >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >>> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On Thu, 24 Nov 2022 14:41:00 +0200 Avihai Horon <avihaih@nvidia.com> wrote: > On 20/11/2022 11:34, Avihai Horon wrote: > > > > On 17/11/2022 19:38, Alex Williamson wrote: > >> External email: Use caution opening links or attachments > >> > >> > >> On Thu, 17 Nov 2022 19:07:10 +0200 > >> Avihai Horon <avihaih@nvidia.com> wrote: > >>> On 16/11/2022 20:29, Alex Williamson wrote: > >>>> On Thu, 3 Nov 2022 18:16:15 +0200 > >>>> Avihai Horon <avihaih@nvidia.com> wrote: > >>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > >>>>> index e784374453..62afc23a8c 100644 > >>>>> --- a/hw/vfio/migration.c > >>>>> +++ b/hw/vfio/migration.c > >>>>> @@ -44,8 +44,84 @@ > >>>>> #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) > >>>>> #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) > >>>>> > >>>>> +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) > >>>> Add comment explaining heuristic of this size. > >>> This is an arbitrary size we picked with mlx5 state size in mind. > >>> Increasing this size to higher values (128M, 1G) didn't improve > >>> performance in our testing. > >>> > >>> How about this comment: > >>> This is an initial value that doesn't consume much memory and provides > >>> good performance. > >>> > >>> Do you have other suggestion? > >> I'd lean more towards your description above, ex: > >> > >> /* > >> * This is an arbitrary size based on migration of mlx5 devices, where > >> * the worst case total device migration size is on the order of 100s > >> * of MB. Testing with larger values, ex. 128MB and 1GB, did not show > >> * a performance improvement. > >> */ > >> > >> I think that provides sufficient information for someone who might come > >> later to have an understanding of the basis if they want to try to > >> optimize further. > > > > OK, sounds good, I will add a comment like this. > > > >>>>> @@ -804,34 +1090,51 @@ 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->device_state = > >>>>> VFIO_DEVICE_STATE_RUNNING; > >>>>> + vbasedev->migration->data_buffer_size = > >>>>> VFIO_MIG_DATA_BUFFER_SIZE; > >>>>> + vbasedev->migration->data_buffer = > >>>>> + g_malloc0(vbasedev->migration->data_buffer_size); > >>>> So VFIO_MIG_DATA_BUFFER_SIZE is our chunk size, but why doesn't the > >>>> later addition of estimated device data size make any changes here? > >>>> I'd think we'd want to scale the buffer to the minimum of the reported > >>>> data size and some well documented heuristic for an upper bound. > >>> As I wrote above, increasing this size to higher values (128M, 1G) > >>> didn't improve performance in our testing. > >>> We can always change it later on if some other heuristics are proven to > >>> improve performance. > >> Note that I'm asking about a minimum buffer size, for example if > >> hisi_acc reports only 10s of KB for an estimated device size, why would > >> we still allocate VFIO_MIG_DATA_BUFFER_SIZE here? Thanks, > > > > This buffer is rather small and has little memory footprint. > > Do you think it is worth the extra complexity of resizing the buffer? > > > Alex, WDYT? > Note that the reported estimated size is dynamic and might change from > query to the other, potentially leaving us with smaller buffer size. > > Also, as part of v4 I moved this allocation to vfio_save_setup(), so it > will be allocated only during migration (when it's actually used) and > only by src side. There's a claim here about added complexity that I'm not really seeing. It looks like we simply make an ioctl call here and scale our buffer based on the minimum of the returned device estimate or our upper bound. The previous comments that exceptionally large buffers don't significantly affect migration performance seems like that also suggests that even if the device estimate later changes, we'll likely be ok with the initial device estimate anyway. Periodically re-checking the device estimate and re-allocating up to a high water mark could potentially be future work. Thanks, Alex
On Mon, Nov 28, 2022 at 11:50:03AM -0700, Alex Williamson wrote: > There's a claim here about added complexity that I'm not really seeing. > It looks like we simply make an ioctl call here and scale our buffer > based on the minimum of the returned device estimate or our upper > bound. I'm not keen on this, for something like mlx5 that has a small precopy size and large post-copy size it risks running with an under allocated buffer, which is harmful to performance. It is a mmap space, if we don't touch the pages they don't get allocated from the OS, I think this is micro-optimizing. Jason
On Mon, 28 Nov 2022 15:40:23 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Mon, Nov 28, 2022 at 11:50:03AM -0700, Alex Williamson wrote: > > > There's a claim here about added complexity that I'm not really seeing. > > It looks like we simply make an ioctl call here and scale our buffer > > based on the minimum of the returned device estimate or our upper > > bound. > > I'm not keen on this, for something like mlx5 that has a small precopy > size and large post-copy size it risks running with an under allocated > buffer, which is harmful to performance. I'm trying to weed out whether there are device assumptions in the implementation, seems like maybe we found one. MIG_DATA_SIZE specifies that it's an estimated data size for stop-copy, so shouldn't that provide the buffer size you're looking for? Thanks, Alex
On Mon, Nov 28, 2022 at 01:36:30PM -0700, Alex Williamson wrote: > On Mon, 28 Nov 2022 15:40:23 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Mon, Nov 28, 2022 at 11:50:03AM -0700, Alex Williamson wrote: > > > > > There's a claim here about added complexity that I'm not really seeing. > > > It looks like we simply make an ioctl call here and scale our buffer > > > based on the minimum of the returned device estimate or our upper > > > bound. > > > > I'm not keen on this, for something like mlx5 that has a small precopy > > size and large post-copy size it risks running with an under allocated > > buffer, which is harmful to performance. > > I'm trying to weed out whether there are device assumptions in the > implementation, seems like maybe we found one. I don't think there are assumptions. Any correct kernel driver should be able to do this transfer out of the FD byte-at-a-time. This buffer size is just a random selection for now until we get multi-fd and can sit down, benchmark and optimize this properly. The ideal realization of this has no buffer at all. > MIG_DATA_SIZE specifies that it's an estimated data size for > stop-copy, so shouldn't that provide the buffer size you're looking > for? Yes, it should, and it should be OK for mlx5 Jason
On Mon, 28 Nov 2022 16:56:39 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Mon, Nov 28, 2022 at 01:36:30PM -0700, Alex Williamson wrote: > > On Mon, 28 Nov 2022 15:40:23 -0400 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > On Mon, Nov 28, 2022 at 11:50:03AM -0700, Alex Williamson wrote: > > > > > > > There's a claim here about added complexity that I'm not really seeing. > > > > It looks like we simply make an ioctl call here and scale our buffer > > > > based on the minimum of the returned device estimate or our upper > > > > bound. > > > > > > I'm not keen on this, for something like mlx5 that has a small precopy > > > size and large post-copy size it risks running with an under allocated > > > buffer, which is harmful to performance. > > > > I'm trying to weed out whether there are device assumptions in the > > implementation, seems like maybe we found one. > > I don't think there are assumptions. Any correct kernel driver should > be able to do this transfer out of the FD byte-at-a-time. > > This buffer size is just a random selection for now until we get > multi-fd and can sit down, benchmark and optimize this properly. We can certainly still do that, but I'm still failing to see how buffer_size = min(MIG_DATA_SIZE, 1MB) is such an imposition on the complexity or over-eager optimization. Thanks, Alex
On 28/11/2022 23:10, Alex Williamson wrote: > External email: Use caution opening links or attachments > > > On Mon, 28 Nov 2022 16:56:39 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: > >> On Mon, Nov 28, 2022 at 01:36:30PM -0700, Alex Williamson wrote: >>> On Mon, 28 Nov 2022 15:40:23 -0400 >>> Jason Gunthorpe <jgg@nvidia.com> wrote: >>> >>>> On Mon, Nov 28, 2022 at 11:50:03AM -0700, Alex Williamson wrote: >>>> >>>>> There's a claim here about added complexity that I'm not really seeing. >>>>> It looks like we simply make an ioctl call here and scale our buffer >>>>> based on the minimum of the returned device estimate or our upper >>>>> bound. >>>> I'm not keen on this, for something like mlx5 that has a small precopy >>>> size and large post-copy size it risks running with an under allocated >>>> buffer, which is harmful to performance. >>> I'm trying to weed out whether there are device assumptions in the >>> implementation, seems like maybe we found one. >> I don't think there are assumptions. Any correct kernel driver should >> be able to do this transfer out of the FD byte-at-a-time. >> >> This buffer size is just a random selection for now until we get >> multi-fd and can sit down, benchmark and optimize this properly. > We can certainly still do that, but I'm still failing to see how > buffer_size = min(MIG_DATA_SIZE, 1MB) is such an imposition on the > complexity or over-eager optimization. OK, I will adjust the min buffer size based on MIG_DATA_SIZE ioctl. Thanks.
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 617e6cd901..0bdbd1586b 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -355,10 +355,18 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) return false; } - if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && + if (!migration->v2 && + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING)) { return false; } + + if (migration->v2 && + (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) && + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { + return false; + } } } return true; @@ -385,7 +393,14 @@ static bool vfio_devices_all_running_and_mig_active(VFIOContainer *container) return false; } - if (migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { + if (!migration->v2 && + migration->device_state_v1 & VFIO_DEVICE_STATE_V1_RUNNING) { + continue; + } + + if (migration->v2 && + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || + migration->device_state == VFIO_DEVICE_STATE_RUNNING_P2P)) { continue; } else { return false; diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index e784374453..62afc23a8c 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -44,8 +44,84 @@ #define VFIO_MIG_FLAG_DEV_SETUP_STATE (0xffffffffef100003ULL) #define VFIO_MIG_FLAG_DEV_DATA_STATE (0xffffffffef100004ULL) +#define VFIO_MIG_DATA_BUFFER_SIZE (1024 * 1024) + static int64_t bytes_transferred; +static const char *mig_state_to_str(enum vfio_device_mig_state state) +{ + switch (state) { + case VFIO_DEVICE_STATE_ERROR: + return "ERROR"; + case VFIO_DEVICE_STATE_STOP: + return "STOP"; + case VFIO_DEVICE_STATE_RUNNING: + return "RUNNING"; + case VFIO_DEVICE_STATE_STOP_COPY: + return "STOP_COPY"; + case VFIO_DEVICE_STATE_RESUMING: + return "RESUMING"; + case VFIO_DEVICE_STATE_RUNNING_P2P: + return "RUNNING_P2P"; + default: + return "UNKNOWN STATE"; + } +} + +static int vfio_migration_set_state(VFIODevice *vbasedev, + enum vfio_device_mig_state new_state, + enum vfio_device_mig_state recover_state) +{ + VFIOMigration *migration = vbasedev->migration; + uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) + + sizeof(struct vfio_device_feature_mig_state), + sizeof(uint64_t))] = {}; + struct vfio_device_feature *feature = (void *)buf; + struct vfio_device_feature_mig_state *mig_state = (void *)feature->data; + + feature->argsz = sizeof(buf); + feature->flags = + VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE; + mig_state->device_state = new_state; + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { + /* Try to set the device in some good state */ + error_report( + "%s: Failed setting device state to %s, err: %s. Setting device in recover state %s", + vbasedev->name, mig_state_to_str(new_state), + strerror(errno), mig_state_to_str(recover_state)); + + mig_state->device_state = recover_state; + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { + hw_error("%s: Failed setting device in recover state, err: %s", + vbasedev->name, strerror(errno)); + } + + migration->device_state = recover_state; + + return -1; + } + + if (mig_state->data_fd != -1) { + if (migration->data_fd != -1) { + /* + * This can happen if the device is asynchronously reset and + * terminates a data transfer. + */ + error_report("%s: data_fd out of sync", vbasedev->name); + close(mig_state->data_fd); + + return -1; + } + + migration->data_fd = mig_state->data_fd; + } + migration->device_state = new_state; + + trace_vfio_migration_set_state(vbasedev->name, mig_state_to_str(new_state)); + + return 0; +} + static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count, off_t off, bool iswrite) { @@ -260,6 +336,20 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t *size) return ret; } +static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev, + uint64_t data_size) +{ + VFIOMigration *migration = vbasedev->migration; + int ret; + + ret = qemu_file_get_to_fd(f, migration->data_fd, data_size); + if (!ret) { + trace_vfio_load_state_device_data(vbasedev->name, data_size); + } + + return ret; +} + static int vfio_v1_load_buffer(QEMUFile *f, VFIODevice *vbasedev, uint64_t data_size) { @@ -394,6 +484,14 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) return qemu_file_get_error(f); } +static void vfio_migration_cleanup(VFIODevice *vbasedev) +{ + VFIOMigration *migration = vbasedev->migration; + + close(migration->data_fd); + migration->data_fd = -1; +} + static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) { VFIOMigration *migration = vbasedev->migration; @@ -405,6 +503,18 @@ static void vfio_migration_v1_cleanup(VFIODevice *vbasedev) /* ---------------------------------------------------------------------- */ +static int vfio_save_setup(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + + trace_vfio_save_setup(vbasedev->name); + + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE); + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); + + return qemu_file_get_error(f); +} + static int vfio_v1_save_setup(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; @@ -448,6 +558,14 @@ static int vfio_v1_save_setup(QEMUFile *f, void *opaque) return 0; } +static void vfio_save_cleanup(void *opaque) +{ + VFIODevice *vbasedev = opaque; + + vfio_migration_cleanup(vbasedev); + trace_vfio_save_cleanup(vbasedev->name); +} + static void vfio_v1_save_cleanup(void *opaque) { VFIODevice *vbasedev = opaque; @@ -456,6 +574,23 @@ static void vfio_v1_save_cleanup(void *opaque) trace_vfio_save_cleanup(vbasedev->name); } +#define VFIO_MIG_PENDING_SIZE (512 * 1024 * 1024) +static void vfio_save_pending(void *opaque, uint64_t threshold_size, + uint64_t *res_precopy, uint64_t *res_postcopy) +{ + VFIODevice *vbasedev = opaque; + + /* + * VFIO migration protocol v2 currently doesn't have an API to get pending + * device state size. Until such API is introduced, report some big + * arbitrary pending size so the device will be taken into account for + * downtime limit calculations. + */ + *res_postcopy += VFIO_MIG_PENDING_SIZE; + + trace_vfio_save_pending(vbasedev->name, *res_precopy, *res_postcopy); +} + static void vfio_v1_save_pending(void *opaque, uint64_t threshold_size, uint64_t *res_precopy, uint64_t *res_postcopy) { @@ -520,6 +655,67 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque) return 0; } +/* Returns 1 if end-of-stream is reached, 0 if more data and -1 if error */ +static int vfio_save_block(QEMUFile *f, VFIOMigration *migration) +{ + ssize_t data_size; + + data_size = read(migration->data_fd, migration->data_buffer, + migration->data_buffer_size); + if (data_size < 0) { + return -1; + } + if (data_size == 0) { + return 1; + } + + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); + qemu_put_be64(f, data_size); + qemu_put_buffer(f, migration->data_buffer, data_size); + bytes_transferred += data_size; + + trace_vfio_save_block(migration->vbasedev->name, data_size); + + return qemu_file_get_error(f); +} + +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 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; + } + + recover_state = VFIO_DEVICE_STATE_ERROR; + ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP, + recover_state); + if (!ret) { + trace_vfio_save_complete_precopy(vbasedev->name); + } + + return ret; +} + static int vfio_v1_save_complete_precopy(QEMUFile *f, void *opaque) { VFIODevice *vbasedev = opaque; @@ -589,6 +785,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; @@ -616,6 +820,16 @@ 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; @@ -658,7 +872,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; } @@ -679,6 +897,17 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id) return ret; } +static const SaveVMHandlers savevm_vfio_handlers = { + .save_setup = vfio_save_setup, + .save_cleanup = vfio_save_cleanup, + .save_live_pending = vfio_save_pending, + .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, @@ -693,6 +922,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), + mig_state_to_str(new_state)); +} + static void vfio_v1_vmstate_change(void *opaque, bool running, RunState state) { VFIODevice *vbasedev = opaque; @@ -766,12 +1023,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); + } 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); + } } } } @@ -780,12 +1042,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; @@ -794,6 +1079,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; @@ -804,34 +1090,51 @@ 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->device_state = VFIO_DEVICE_STATE_RUNNING; + 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->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; - vbasedev->migration->vm_running = runstate_is_running(); + vbasedev->migration = g_new0(VFIOMigration, 1); + vbasedev->migration->device_state_v1 = VFIO_DEVICE_STATE_V1_RUNNING; + vbasedev->migration->vm_running = runstate_is_running(); - 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; @@ -844,11 +1147,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 d88d2b4053..9ef84e24b2 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -149,7 +149,9 @@ vfio_display_edid_write_error(void) "" # migration.c vfio_migration_probe(const char *name) " (%s)" +vfio_migration_set_state(const char *name, const char *state) " (%s) state %s" vfio_migration_v1_set_state(const char *name, uint32_t state) " (%s) state %d" +vfio_vmstate_change(const char *name, int running, const char *reason, const char *dev_state) " (%s) running %d reason %s device state %s" vfio_v1_vmstate_change(const char *name, int running, const char *reason, uint32_t dev_state) " (%s) running %d reason %s device state %d" vfio_migration_state_notifier(const char *name, const char *state) " (%s) state %s" vfio_save_setup(const char *name) " (%s)" @@ -163,6 +165,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 {
Add implementation of VFIO migration protocol v2. The two protocols, v1 and v2, will co-exist and in next patch v1 protocol will be removed. There are several main differences between v1 and v2 protocols: - VFIO device state is now represented as a finite state machine instead of a bitmap. - Migration interface with kernel is now done using VFIO_DEVICE_FEATURE ioctl and normal read() and write() instead of the migration region. - VFIO migration protocol v2 currently doesn't support the pre-copy phase of migration. Detailed information about VFIO migration protocol v2 and difference compared to v1 can be found here [1]. [1] https://lore.kernel.org/all/20220224142024.147653-10-yishaih@nvidia.com/ Signed-off-by: Avihai Horon <avihaih@nvidia.com> --- hw/vfio/common.c | 19 +- hw/vfio/migration.c | 386 ++++++++++++++++++++++++++++++---- hw/vfio/trace-events | 4 + include/hw/vfio/vfio-common.h | 5 + 4 files changed, 375 insertions(+), 39 deletions(-)