| Message ID | 1585084154-29461-10-git-send-email-kwankhede@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add migration support for VFIO devices | expand |
On Wed, 25 Mar 2020 02:39:07 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy > functions. These functions handles pre-copy and stop-and-copy phase. > > In _SAVING|_RUNNING device state or pre-copy phase: > - read pending_bytes. If pending_bytes > 0, go through below steps. > - read data_offset - indicates kernel driver to write data to staging > buffer. > - read data_size - amount of data in bytes written by vendor driver in > migration region. > - read data_size bytes of data from data_offset in the migration region. > - Write data packet to file stream as below: > {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, > VFIO_MIG_FLAG_END_OF_STATE } > > In _SAVING device state or stop-and-copy phase > a. read config space of device and save to migration file stream. This > doesn't need to be from vendor driver. Any other special config state > from driver can be saved as data in following iteration. > b. read pending_bytes. If pending_bytes > 0, go through below steps. > c. read data_offset - indicates kernel driver to write data to staging > buffer. > d. read data_size - amount of data in bytes written by vendor driver in > migration region. > e. read data_size bytes of data from data_offset in the migration region. > f. Write data packet as below: > {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} > g. iterate through steps b to f while (pending_bytes > 0) > h. Write {VFIO_MIG_FLAG_END_OF_STATE} > > When data region is mapped, its user's responsibility to read data from > data_offset of data_size before moving to next steps. > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > Reviewed-by: Neo Jia <cjia@nvidia.com> > --- > hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- > hw/vfio/trace-events | 6 ++ > include/hw/vfio/vfio-common.h | 1 + > 3 files changed, 251 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 033f76526e49..ecbeed5182c2 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, > return 0; > } > > +static void *find_data_region(VFIORegion *region, > + uint64_t data_offset, > + uint64_t data_size) > +{ > + void *ptr = NULL; > + int i; > + > + for (i = 0; i < region->nr_mmaps; i++) { > + if ((data_offset >= region->mmaps[i].offset) && > + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && > + (data_size <= region->mmaps[i].size)) { (data_offset - region->mmaps[i].offset) can be non-zero, so this test is invalid. Additionally the uapi does not require that a give data chunk fits exclusively within an mmap'd area, it may overlap one or more mmap'd sections of the region, possibly with non-mmap'd areas included. > + ptr = region->mmaps[i].mmap + (data_offset - > + region->mmaps[i].offset); > + break; > + } > + } > + return ptr; > +} > + > +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + VFIORegion *region = &migration->region; > + uint64_t data_offset = 0, data_size = 0; > + int ret; > + > + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + data_offset)); > + if (ret != sizeof(data_offset)) { > + error_report("%s: Failed to get migration buffer data offset %d", > + vbasedev->name, ret); > + return -EINVAL; > + } > + > + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + data_size)); > + if (ret != sizeof(data_size)) { > + error_report("%s: Failed to get migration buffer data size %d", > + vbasedev->name, ret); > + return -EINVAL; > + } > + > + if (data_size > 0) { > + void *buf = NULL; > + bool buffer_mmaped; > + > + if (region->mmaps) { > + buf = find_data_region(region, data_offset, data_size); > + } > + > + buffer_mmaped = (buf != NULL) ? true : false; The ternary is unnecessary, "? true : false" is redundant. > + > + if (!buffer_mmaped) { > + buf = g_try_malloc0(data_size); Why do we need zero'd memory? > + if (!buf) { > + error_report("%s: Error allocating buffer ", __func__); > + return -ENOMEM; > + } > + > + ret = pread(vbasedev->fd, buf, data_size, > + region->fd_offset + data_offset); > + if (ret != data_size) { > + error_report("%s: Failed to get migration data %d", > + vbasedev->name, ret); > + g_free(buf); > + return -EINVAL; > + } > + } > + > + qemu_put_be64(f, data_size); > + qemu_put_buffer(f, buf, data_size); This can segfault when mmap'd given the above assumptions about size and layout. > + > + if (!buffer_mmaped) { > + g_free(buf); > + } > + } else { > + qemu_put_be64(f, data_size); We insert a zero? Couldn't we add the section header and end here and skip it entirely? > + } > + > + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, > + migration->pending_bytes); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + return data_size; > +} > + > +static int vfio_update_pending(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + VFIORegion *region = &migration->region; > + uint64_t pending_bytes = 0; > + int ret; > + > + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + pending_bytes)); > + if ((ret < 0) || (ret != sizeof(pending_bytes))) { > + error_report("%s: Failed to get pending bytes %d", > + vbasedev->name, ret); > + migration->pending_bytes = 0; > + return (ret < 0) ? ret : -EINVAL; > + } > + > + migration->pending_bytes = pending_bytes; > + trace_vfio_update_pending(vbasedev->name, pending_bytes); > + return 0; > +} > + > +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); > + > + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { > + vbasedev->ops->vfio_save_config(vbasedev, f); > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + trace_vfio_save_device_config_state(vbasedev->name); > + > + return qemu_file_get_error(f); > +} > + > /* ---------------------------------------------------------------------- */ > > static int vfio_save_setup(QEMUFile *f, void *opaque) > @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > qemu_mutex_unlock_iothread(); > if (ret) { > error_report("%s: Failed to mmap VFIO migration region %d: %s", > - vbasedev->name, migration->region.index, > + vbasedev->name, migration->region.nr, > strerror(-ret)); > return ret; > } > @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) > trace_vfio_save_cleanup(vbasedev->name); > } > > +static void vfio_save_pending(QEMUFile *f, void *opaque, > + uint64_t threshold_size, > + uint64_t *res_precopy_only, > + uint64_t *res_compatible, > + uint64_t *res_postcopy_only) > +{ > + VFIODevice *vbasedev = opaque; > + VFIOMigration *migration = vbasedev->migration; > + int ret; > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return; > + } > + > + *res_precopy_only += migration->pending_bytes; > + > + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, > + *res_postcopy_only, *res_compatible); > +} > + > +static int vfio_save_iterate(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + int ret, data_size; > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + > + data_size = vfio_save_buffer(f, vbasedev); > + > + if (data_size < 0) { > + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, > + strerror(errno)); > + return data_size; > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + trace_vfio_save_iterate(vbasedev->name, data_size); > + if (data_size == 0) { > + /* indicates data finished, goto complete phase */ > + return 1; But it's pending_bytes not data_size that indicates we're done. How do we get away with ignoring pending_bytes for the save_live_iterate phase? > + } > + > + return 0; > +} > + > +static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + VFIOMigration *migration = vbasedev->migration; > + int ret; > + > + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, > + VFIO_DEVICE_STATE_SAVING); > + if (ret) { > + error_report("%s: Failed to set state STOP and SAVING", > + vbasedev->name); > + return ret; > + } > + > + ret = vfio_save_device_config_state(f, opaque); > + if (ret) { > + return ret; > + } > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return ret; > + } > + > + while (migration->pending_bytes > 0) { > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + ret = vfio_save_buffer(f, vbasedev); > + if (ret < 0) { > + error_report("%s: Failed to save buffer", vbasedev->name); > + return ret; > + } else if (ret == 0) { > + break; > + } > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return ret; > + } > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); > + if (ret) { > + error_report("%s: Failed to set state STOPPED", vbasedev->name); > + return ret; > + } > + > + trace_vfio_save_complete_precopy(vbasedev->name); > + return ret; > +} > + > static SaveVMHandlers savevm_vfio_handlers = { > .save_setup = vfio_save_setup, > .save_cleanup = vfio_save_cleanup, > + .save_live_pending = vfio_save_pending, > + .save_live_iterate = vfio_save_iterate, > + .save_live_complete_precopy = vfio_save_complete_precopy, > }; > > /* ---------------------------------------------------------------------- */ > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index 4bb43f18f315..bdf40ba368c7 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -151,3 +151,9 @@ vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_st > vfio_migration_state_notifier(char *name, int state) " (%s) state %d" > vfio_save_setup(char *name) " (%s)" > vfio_save_cleanup(char *name) " (%s)" > +vfio_save_buffer(char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64 > +vfio_update_pending(char *name, uint64_t pending) " (%s) pending 0x%"PRIx64 > +vfio_save_device_config_state(char *name) " (%s)" > +vfio_save_pending(char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64 > +vfio_save_iterate(char *name, int data_size) " (%s) data_size %d" > +vfio_save_complete_precopy(char *name) " (%s)" > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 28f55f66d019..c78033e4149d 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -60,6 +60,7 @@ typedef struct VFIORegion { > > typedef struct VFIOMigration { > VFIORegion region; > + uint64_t pending_bytes; > } VFIOMigration; > > typedef struct VFIOAddressSpace {
On 3/26/2020 3:33 AM, Alex Williamson wrote: > On Wed, 25 Mar 2020 02:39:07 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > >> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy >> functions. These functions handles pre-copy and stop-and-copy phase. >> >> In _SAVING|_RUNNING device state or pre-copy phase: >> - read pending_bytes. If pending_bytes > 0, go through below steps. >> - read data_offset - indicates kernel driver to write data to staging >> buffer. >> - read data_size - amount of data in bytes written by vendor driver in >> migration region. >> - read data_size bytes of data from data_offset in the migration region. >> - Write data packet to file stream as below: >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, >> VFIO_MIG_FLAG_END_OF_STATE } >> >> In _SAVING device state or stop-and-copy phase >> a. read config space of device and save to migration file stream. This >> doesn't need to be from vendor driver. Any other special config state >> from driver can be saved as data in following iteration. >> b. read pending_bytes. If pending_bytes > 0, go through below steps. >> c. read data_offset - indicates kernel driver to write data to staging >> buffer. >> d. read data_size - amount of data in bytes written by vendor driver in >> migration region. >> e. read data_size bytes of data from data_offset in the migration region. >> f. Write data packet as below: >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} >> g. iterate through steps b to f while (pending_bytes > 0) >> h. Write {VFIO_MIG_FLAG_END_OF_STATE} >> >> When data region is mapped, its user's responsibility to read data from >> data_offset of data_size before moving to next steps. >> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >> Reviewed-by: Neo Jia <cjia@nvidia.com> >> --- >> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- >> hw/vfio/trace-events | 6 ++ >> include/hw/vfio/vfio-common.h | 1 + >> 3 files changed, 251 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 033f76526e49..ecbeed5182c2 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, >> return 0; >> } >> >> +static void *find_data_region(VFIORegion *region, >> + uint64_t data_offset, >> + uint64_t data_size) >> +{ >> + void *ptr = NULL; >> + int i; >> + >> + for (i = 0; i < region->nr_mmaps; i++) { >> + if ((data_offset >= region->mmaps[i].offset) && >> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && >> + (data_size <= region->mmaps[i].size)) { > > (data_offset - region->mmaps[i].offset) can be non-zero, so this test > is invalid. Additionally the uapi does not require that a give data > chunk fits exclusively within an mmap'd area, it may overlap one or > more mmap'd sections of the region, possibly with non-mmap'd areas > included. > What's the advantage of having mmap and non-mmap overlapped regions? Isn't it better to have data section either mapped or trapped? >> + ptr = region->mmaps[i].mmap + (data_offset - >> + region->mmaps[i].offset); >> + break; >> + } >> + } >> + return ptr; >> +} >> + >> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + VFIORegion *region = &migration->region; >> + uint64_t data_offset = 0, data_size = 0; >> + int ret; >> + >> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), >> + region->fd_offset + offsetof(struct vfio_device_migration_info, >> + data_offset)); >> + if (ret != sizeof(data_offset)) { >> + error_report("%s: Failed to get migration buffer data offset %d", >> + vbasedev->name, ret); >> + return -EINVAL; >> + } >> + >> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), >> + region->fd_offset + offsetof(struct vfio_device_migration_info, >> + data_size)); >> + if (ret != sizeof(data_size)) { >> + error_report("%s: Failed to get migration buffer data size %d", >> + vbasedev->name, ret); >> + return -EINVAL; >> + } >> + >> + if (data_size > 0) { >> + void *buf = NULL; >> + bool buffer_mmaped; >> + >> + if (region->mmaps) { >> + buf = find_data_region(region, data_offset, data_size); >> + } >> + >> + buffer_mmaped = (buf != NULL) ? true : false; > > The ternary is unnecessary, "? true : false" is redundant. > Removing it. >> + >> + if (!buffer_mmaped) { >> + buf = g_try_malloc0(data_size); > > Why do we need zero'd memory? > Zeroed memory not required, removing 0 >> + if (!buf) { >> + error_report("%s: Error allocating buffer ", __func__); >> + return -ENOMEM; >> + } >> + >> + ret = pread(vbasedev->fd, buf, data_size, >> + region->fd_offset + data_offset); >> + if (ret != data_size) { >> + error_report("%s: Failed to get migration data %d", >> + vbasedev->name, ret); >> + g_free(buf); >> + return -EINVAL; >> + } >> + } >> + >> + qemu_put_be64(f, data_size); >> + qemu_put_buffer(f, buf, data_size); > > This can segfault when mmap'd given the above assumptions about size > and layout. > >> + >> + if (!buffer_mmaped) { >> + g_free(buf); >> + } >> + } else { >> + qemu_put_be64(f, data_size); > > We insert a zero? Couldn't we add the section header and end here and > skip it entirely? > This is used during resuming, data_size 0 indicates end of data. >> + } >> + >> + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, >> + migration->pending_bytes); >> + >> + ret = qemu_file_get_error(f); >> + if (ret) { >> + return ret; >> + } >> + >> + return data_size; >> +} >> + >> +static int vfio_update_pending(VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + VFIORegion *region = &migration->region; >> + uint64_t pending_bytes = 0; >> + int ret; >> + >> + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), >> + region->fd_offset + offsetof(struct vfio_device_migration_info, >> + pending_bytes)); >> + if ((ret < 0) || (ret != sizeof(pending_bytes))) { >> + error_report("%s: Failed to get pending bytes %d", >> + vbasedev->name, ret); >> + migration->pending_bytes = 0; >> + return (ret < 0) ? ret : -EINVAL; >> + } >> + >> + migration->pending_bytes = pending_bytes; >> + trace_vfio_update_pending(vbasedev->name, pending_bytes); >> + return 0; >> +} >> + >> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); >> + >> + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { >> + vbasedev->ops->vfio_save_config(vbasedev, f); >> + } >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >> + >> + trace_vfio_save_device_config_state(vbasedev->name); >> + >> + return qemu_file_get_error(f); >> +} >> + >> /* ---------------------------------------------------------------------- */ >> >> static int vfio_save_setup(QEMUFile *f, void *opaque) >> @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) >> qemu_mutex_unlock_iothread(); >> if (ret) { >> error_report("%s: Failed to mmap VFIO migration region %d: %s", >> - vbasedev->name, migration->region.index, >> + vbasedev->name, migration->region.nr, >> strerror(-ret)); >> return ret; >> } >> @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) >> trace_vfio_save_cleanup(vbasedev->name); >> } >> >> +static void vfio_save_pending(QEMUFile *f, void *opaque, >> + uint64_t threshold_size, >> + uint64_t *res_precopy_only, >> + uint64_t *res_compatible, >> + uint64_t *res_postcopy_only) >> +{ >> + VFIODevice *vbasedev = opaque; >> + VFIOMigration *migration = vbasedev->migration; >> + int ret; >> + >> + ret = vfio_update_pending(vbasedev); >> + if (ret) { >> + return; >> + } >> + >> + *res_precopy_only += migration->pending_bytes; >> + >> + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, >> + *res_postcopy_only, *res_compatible); >> +} >> + >> +static int vfio_save_iterate(QEMUFile *f, void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + int ret, data_size; >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >> + >> + data_size = vfio_save_buffer(f, vbasedev); >> + >> + if (data_size < 0) { >> + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, >> + strerror(errno)); >> + return data_size; >> + } >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >> + >> + ret = qemu_file_get_error(f); >> + if (ret) { >> + return ret; >> + } >> + >> + trace_vfio_save_iterate(vbasedev->name, data_size); >> + if (data_size == 0) { >> + /* indicates data finished, goto complete phase */ >> + return 1; > > But it's pending_bytes not data_size that indicates we're done. How do > we get away with ignoring pending_bytes for the save_live_iterate phase? > This is requirement mentioned above qemu_savevm_state_iterate() which calls .save_live_iterate. /* * this function has three return values: * negative: there was one error, and we have -errno. * 0 : We haven't finished, caller have to go again * 1 : We have finished, we can go to complete phase */ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy) This is to serialize savevm_state.handlers (or in other words devices). Thanks, Kirti >> + } >> + >> + return 0; >> +} >> + >> +static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) >> +{ >> + VFIODevice *vbasedev = opaque; >> + VFIOMigration *migration = vbasedev->migration; >> + int ret; >> + >> + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, >> + VFIO_DEVICE_STATE_SAVING); >> + if (ret) { >> + error_report("%s: Failed to set state STOP and SAVING", >> + vbasedev->name); >> + return ret; >> + } >> + >> + ret = vfio_save_device_config_state(f, opaque); >> + if (ret) { >> + return ret; >> + } >> + >> + ret = vfio_update_pending(vbasedev); >> + if (ret) { >> + return ret; >> + } >> + >> + while (migration->pending_bytes > 0) { >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >> + ret = vfio_save_buffer(f, vbasedev); >> + if (ret < 0) { >> + error_report("%s: Failed to save buffer", vbasedev->name); >> + return ret; >> + } else if (ret == 0) { >> + break; >> + } >> + >> + ret = vfio_update_pending(vbasedev); >> + if (ret) { >> + return ret; >> + } >> + } >> + >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >> + >> + ret = qemu_file_get_error(f); >> + if (ret) { >> + return ret; >> + } >> + >> + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); >> + if (ret) { >> + error_report("%s: Failed to set state STOPPED", vbasedev->name); >> + return ret; >> + } >> + >> + trace_vfio_save_complete_precopy(vbasedev->name); >> + return ret; >> +} >> + >> static SaveVMHandlers savevm_vfio_handlers = { >> .save_setup = vfio_save_setup, >> .save_cleanup = vfio_save_cleanup, >> + .save_live_pending = vfio_save_pending, >> + .save_live_iterate = vfio_save_iterate, >> + .save_live_complete_precopy = vfio_save_complete_precopy, >> }; >> >> /* ---------------------------------------------------------------------- */ >> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events >> index 4bb43f18f315..bdf40ba368c7 100644 >> --- a/hw/vfio/trace-events >> +++ b/hw/vfio/trace-events >> @@ -151,3 +151,9 @@ vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_st >> vfio_migration_state_notifier(char *name, int state) " (%s) state %d" >> vfio_save_setup(char *name) " (%s)" >> vfio_save_cleanup(char *name) " (%s)" >> +vfio_save_buffer(char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64 >> +vfio_update_pending(char *name, uint64_t pending) " (%s) pending 0x%"PRIx64 >> +vfio_save_device_config_state(char *name) " (%s)" >> +vfio_save_pending(char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64 >> +vfio_save_iterate(char *name, int data_size) " (%s) data_size %d" >> +vfio_save_complete_precopy(char *name) " (%s)" >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index 28f55f66d019..c78033e4149d 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -60,6 +60,7 @@ typedef struct VFIORegion { >> >> typedef struct VFIOMigration { >> VFIORegion region; >> + uint64_t pending_bytes; >> } VFIOMigration; >> >> typedef struct VFIOAddressSpace { >
On Tue, 5 May 2020 04:48:14 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 3/26/2020 3:33 AM, Alex Williamson wrote: > > On Wed, 25 Mar 2020 02:39:07 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > >> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy > >> functions. These functions handles pre-copy and stop-and-copy phase. > >> > >> In _SAVING|_RUNNING device state or pre-copy phase: > >> - read pending_bytes. If pending_bytes > 0, go through below steps. > >> - read data_offset - indicates kernel driver to write data to staging > >> buffer. > >> - read data_size - amount of data in bytes written by vendor driver in > >> migration region. > >> - read data_size bytes of data from data_offset in the migration region. > >> - Write data packet to file stream as below: > >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, > >> VFIO_MIG_FLAG_END_OF_STATE } > >> > >> In _SAVING device state or stop-and-copy phase > >> a. read config space of device and save to migration file stream. This > >> doesn't need to be from vendor driver. Any other special config state > >> from driver can be saved as data in following iteration. > >> b. read pending_bytes. If pending_bytes > 0, go through below steps. > >> c. read data_offset - indicates kernel driver to write data to staging > >> buffer. > >> d. read data_size - amount of data in bytes written by vendor driver in > >> migration region. > >> e. read data_size bytes of data from data_offset in the migration region. > >> f. Write data packet as below: > >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} > >> g. iterate through steps b to f while (pending_bytes > 0) > >> h. Write {VFIO_MIG_FLAG_END_OF_STATE} > >> > >> When data region is mapped, its user's responsibility to read data from > >> data_offset of data_size before moving to next steps. > >> > >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > >> Reviewed-by: Neo Jia <cjia@nvidia.com> > >> --- > >> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- > >> hw/vfio/trace-events | 6 ++ > >> include/hw/vfio/vfio-common.h | 1 + > >> 3 files changed, 251 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > >> index 033f76526e49..ecbeed5182c2 100644 > >> --- a/hw/vfio/migration.c > >> +++ b/hw/vfio/migration.c > >> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, > >> return 0; > >> } > >> > >> +static void *find_data_region(VFIORegion *region, > >> + uint64_t data_offset, > >> + uint64_t data_size) > >> +{ > >> + void *ptr = NULL; > >> + int i; > >> + > >> + for (i = 0; i < region->nr_mmaps; i++) { > >> + if ((data_offset >= region->mmaps[i].offset) && > >> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && > >> + (data_size <= region->mmaps[i].size)) { > > > > (data_offset - region->mmaps[i].offset) can be non-zero, so this test > > is invalid. Additionally the uapi does not require that a give data > > chunk fits exclusively within an mmap'd area, it may overlap one or > > more mmap'd sections of the region, possibly with non-mmap'd areas > > included. > > > > What's the advantage of having mmap and non-mmap overlapped regions? > Isn't it better to have data section either mapped or trapped? The spec allows for it, therefore we need to support it. A vendor driver might choose to include a header with sequence and checksum information for each transaction, they might accomplish this by setting data_offset to a trapped area backed by kernel memory followed by an area supporting direct mmap to the device. The target end could then fault on writing the header if the sequence information is incorrect. A trapped area at the end of the transaction could allow the vendor driver to validate a checksum. > >> + ptr = region->mmaps[i].mmap + (data_offset - > >> + region->mmaps[i].offset); > >> + break; > >> + } > >> + } > >> + return ptr; > >> +} > >> + > >> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) > >> +{ > >> + VFIOMigration *migration = vbasedev->migration; > >> + VFIORegion *region = &migration->region; > >> + uint64_t data_offset = 0, data_size = 0; > >> + int ret; > >> + > >> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), > >> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >> + data_offset)); > >> + if (ret != sizeof(data_offset)) { > >> + error_report("%s: Failed to get migration buffer data offset %d", > >> + vbasedev->name, ret); > >> + return -EINVAL; > >> + } > >> + > >> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), > >> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >> + data_size)); > >> + if (ret != sizeof(data_size)) { > >> + error_report("%s: Failed to get migration buffer data size %d", > >> + vbasedev->name, ret); > >> + return -EINVAL; > >> + } > >> + > >> + if (data_size > 0) { > >> + void *buf = NULL; > >> + bool buffer_mmaped; > >> + > >> + if (region->mmaps) { > >> + buf = find_data_region(region, data_offset, data_size); > >> + } > >> + > >> + buffer_mmaped = (buf != NULL) ? true : false; > > > > The ternary is unnecessary, "? true : false" is redundant. > > > > Removing it. > > >> + > >> + if (!buffer_mmaped) { > >> + buf = g_try_malloc0(data_size); > > > > Why do we need zero'd memory? > > > > Zeroed memory not required, removing 0 > > >> + if (!buf) { > >> + error_report("%s: Error allocating buffer ", __func__); > >> + return -ENOMEM; > >> + } > >> + > >> + ret = pread(vbasedev->fd, buf, data_size, > >> + region->fd_offset + data_offset); > >> + if (ret != data_size) { > >> + error_report("%s: Failed to get migration data %d", > >> + vbasedev->name, ret); > >> + g_free(buf); > >> + return -EINVAL; > >> + } > >> + } > >> + > >> + qemu_put_be64(f, data_size); > >> + qemu_put_buffer(f, buf, data_size); > > > > This can segfault when mmap'd given the above assumptions about size > > and layout. > > > >> + > >> + if (!buffer_mmaped) { > >> + g_free(buf); > >> + } > >> + } else { > >> + qemu_put_be64(f, data_size); > > > > We insert a zero? Couldn't we add the section header and end here and > > skip it entirely? > > > > This is used during resuming, data_size 0 indicates end of data. > > >> + } > >> + > >> + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, > >> + migration->pending_bytes); > >> + > >> + ret = qemu_file_get_error(f); > >> + if (ret) { > >> + return ret; > >> + } > >> + > >> + return data_size; > >> +} > >> + > >> +static int vfio_update_pending(VFIODevice *vbasedev) > >> +{ > >> + VFIOMigration *migration = vbasedev->migration; > >> + VFIORegion *region = &migration->region; > >> + uint64_t pending_bytes = 0; > >> + int ret; > >> + > >> + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), > >> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >> + pending_bytes)); > >> + if ((ret < 0) || (ret != sizeof(pending_bytes))) { > >> + error_report("%s: Failed to get pending bytes %d", > >> + vbasedev->name, ret); > >> + migration->pending_bytes = 0; > >> + return (ret < 0) ? ret : -EINVAL; > >> + } > >> + > >> + migration->pending_bytes = pending_bytes; > >> + trace_vfio_update_pending(vbasedev->name, pending_bytes); > >> + return 0; > >> +} > >> + > >> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) > >> +{ > >> + VFIODevice *vbasedev = opaque; > >> + > >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); > >> + > >> + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { > >> + vbasedev->ops->vfio_save_config(vbasedev, f); > >> + } > >> + > >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > >> + > >> + trace_vfio_save_device_config_state(vbasedev->name); > >> + > >> + return qemu_file_get_error(f); > >> +} > >> + > >> /* ---------------------------------------------------------------------- */ > >> > >> static int vfio_save_setup(QEMUFile *f, void *opaque) > >> @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > >> qemu_mutex_unlock_iothread(); > >> if (ret) { > >> error_report("%s: Failed to mmap VFIO migration region %d: %s", > >> - vbasedev->name, migration->region.index, > >> + vbasedev->name, migration->region.nr, > >> strerror(-ret)); > >> return ret; > >> } > >> @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) > >> trace_vfio_save_cleanup(vbasedev->name); > >> } > >> > >> +static void vfio_save_pending(QEMUFile *f, void *opaque, > >> + uint64_t threshold_size, > >> + uint64_t *res_precopy_only, > >> + uint64_t *res_compatible, > >> + uint64_t *res_postcopy_only) > >> +{ > >> + VFIODevice *vbasedev = opaque; > >> + VFIOMigration *migration = vbasedev->migration; > >> + int ret; > >> + > >> + ret = vfio_update_pending(vbasedev); > >> + if (ret) { > >> + return; > >> + } > >> + > >> + *res_precopy_only += migration->pending_bytes; > >> + > >> + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, > >> + *res_postcopy_only, *res_compatible); > >> +} > >> + > >> +static int vfio_save_iterate(QEMUFile *f, void *opaque) > >> +{ > >> + VFIODevice *vbasedev = opaque; > >> + int ret, data_size; > >> + > >> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > >> + > >> + data_size = vfio_save_buffer(f, vbasedev); > >> + > >> + if (data_size < 0) { > >> + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, > >> + strerror(errno)); > >> + return data_size; > >> + } > >> + > >> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > >> + > >> + ret = qemu_file_get_error(f); > >> + if (ret) { > >> + return ret; > >> + } > >> + > >> + trace_vfio_save_iterate(vbasedev->name, data_size); > >> + if (data_size == 0) { > >> + /* indicates data finished, goto complete phase */ > >> + return 1; > > > > But it's pending_bytes not data_size that indicates we're done. How do > > we get away with ignoring pending_bytes for the save_live_iterate phase? > > > > This is requirement mentioned above qemu_savevm_state_iterate() which > calls .save_live_iterate. > > /* > * this function has three return values: > * negative: there was one error, and we have -errno. > * 0 : We haven't finished, caller have to go again > * 1 : We have finished, we can go to complete phase > */ > int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy) > > This is to serialize savevm_state.handlers (or in other words devices). I've lost all context on this question in the interim, but I think this highlights my question. We use pending_bytes to know how close we are to the end of the stream and data_size to iterate each transaction within that stream. So how does data_size == 0 indicate we've completed the current phase? It seems like pending_bytes should indicate that. Thanks, Alex
On Wed, Mar 25, 2020 at 05:09:07AM +0800, Kirti Wankhede wrote: > Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy > functions. These functions handles pre-copy and stop-and-copy phase. > > In _SAVING|_RUNNING device state or pre-copy phase: > - read pending_bytes. If pending_bytes > 0, go through below steps. > - read data_offset - indicates kernel driver to write data to staging > buffer. > - read data_size - amount of data in bytes written by vendor driver in > migration region. I think we should change the sequence of reading data_size and data_offset. see the next comment below. > - read data_size bytes of data from data_offset in the migration region. > - Write data packet to file stream as below: > {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, > VFIO_MIG_FLAG_END_OF_STATE } > > In _SAVING device state or stop-and-copy phase > a. read config space of device and save to migration file stream. This > doesn't need to be from vendor driver. Any other special config state > from driver can be saved as data in following iteration. > b. read pending_bytes. If pending_bytes > 0, go through below steps. > c. read data_offset - indicates kernel driver to write data to staging > buffer. > d. read data_size - amount of data in bytes written by vendor driver in > migration region. > e. read data_size bytes of data from data_offset in the migration region. > f. Write data packet as below: > {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} > g. iterate through steps b to f while (pending_bytes > 0) > h. Write {VFIO_MIG_FLAG_END_OF_STATE} > > When data region is mapped, its user's responsibility to read data from > data_offset of data_size before moving to next steps. > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > Reviewed-by: Neo Jia <cjia@nvidia.com> > --- > hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- > hw/vfio/trace-events | 6 ++ > include/hw/vfio/vfio-common.h | 1 + > 3 files changed, 251 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index 033f76526e49..ecbeed5182c2 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, > return 0; > } > > +static void *find_data_region(VFIORegion *region, > + uint64_t data_offset, > + uint64_t data_size) > +{ > + void *ptr = NULL; > + int i; > + > + for (i = 0; i < region->nr_mmaps; i++) { > + if ((data_offset >= region->mmaps[i].offset) && > + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && > + (data_size <= region->mmaps[i].size)) { > + ptr = region->mmaps[i].mmap + (data_offset - > + region->mmaps[i].offset); > + break; > + } > + } > + return ptr; > +} > + > +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + VFIORegion *region = &migration->region; > + uint64_t data_offset = 0, data_size = 0; > + int ret; > + > + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + data_offset)); > + if (ret != sizeof(data_offset)) { > + error_report("%s: Failed to get migration buffer data offset %d", > + vbasedev->name, ret); > + return -EINVAL; > + } > + > + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + data_size)); > + if (ret != sizeof(data_size)) { > + error_report("%s: Failed to get migration buffer data size %d", > + vbasedev->name, ret); > + return -EINVAL; > + } data_size should be read first, and if it's 0, data_offset will not be read further. the reasons are below: 1. if there's no data region provided by vendor driver, there's no reason to get a valid data_offset, so reading/writing of data_offset should fail. And this should not be treated as a migration error. 2. even if pending_bytes is 0, vfio_save_iterate() is still possible to be called and therefore vfio_save_buffer() is called. Thanks Yan > + > + if (data_size > 0) { > + void *buf = NULL; > + bool buffer_mmaped; > + > + if (region->mmaps) { > + buf = find_data_region(region, data_offset, data_size); > + } > + > + buffer_mmaped = (buf != NULL) ? true : false; > + > + if (!buffer_mmaped) { > + buf = g_try_malloc0(data_size); > + if (!buf) { > + error_report("%s: Error allocating buffer ", __func__); > + return -ENOMEM; > + } > + > + ret = pread(vbasedev->fd, buf, data_size, > + region->fd_offset + data_offset); > + if (ret != data_size) { > + error_report("%s: Failed to get migration data %d", > + vbasedev->name, ret); > + g_free(buf); > + return -EINVAL; > + } > + } > + > + qemu_put_be64(f, data_size); > + qemu_put_buffer(f, buf, data_size); > + > + if (!buffer_mmaped) { > + g_free(buf); > + } > + } else { > + qemu_put_be64(f, data_size); > + } > + > + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, > + migration->pending_bytes); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + return data_size; > +} > + > +static int vfio_update_pending(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + VFIORegion *region = &migration->region; > + uint64_t pending_bytes = 0; > + int ret; > + > + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), > + region->fd_offset + offsetof(struct vfio_device_migration_info, > + pending_bytes)); > + if ((ret < 0) || (ret != sizeof(pending_bytes))) { > + error_report("%s: Failed to get pending bytes %d", > + vbasedev->name, ret); > + migration->pending_bytes = 0; > + return (ret < 0) ? ret : -EINVAL; > + } > + > + migration->pending_bytes = pending_bytes; > + trace_vfio_update_pending(vbasedev->name, pending_bytes); > + return 0; > +} > + > +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); > + > + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { > + vbasedev->ops->vfio_save_config(vbasedev, f); > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + trace_vfio_save_device_config_state(vbasedev->name); > + > + return qemu_file_get_error(f); > +} > + > /* ---------------------------------------------------------------------- */ > > static int vfio_save_setup(QEMUFile *f, void *opaque) > @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > qemu_mutex_unlock_iothread(); > if (ret) { > error_report("%s: Failed to mmap VFIO migration region %d: %s", > - vbasedev->name, migration->region.index, > + vbasedev->name, migration->region.nr, > strerror(-ret)); > return ret; > } > @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) > trace_vfio_save_cleanup(vbasedev->name); > } > > +static void vfio_save_pending(QEMUFile *f, void *opaque, > + uint64_t threshold_size, > + uint64_t *res_precopy_only, > + uint64_t *res_compatible, > + uint64_t *res_postcopy_only) > +{ > + VFIODevice *vbasedev = opaque; > + VFIOMigration *migration = vbasedev->migration; > + int ret; > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return; > + } > + > + *res_precopy_only += migration->pending_bytes; > + > + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, > + *res_postcopy_only, *res_compatible); > +} > + > +static int vfio_save_iterate(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + int ret, data_size; > + > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + > + data_size = vfio_save_buffer(f, vbasedev); > + > + if (data_size < 0) { > + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, > + strerror(errno)); > + return data_size; > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + trace_vfio_save_iterate(vbasedev->name, data_size); > + if (data_size == 0) { > + /* indicates data finished, goto complete phase */ > + return 1; > + } > + > + return 0; > +} > + > +static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) > +{ > + VFIODevice *vbasedev = opaque; > + VFIOMigration *migration = vbasedev->migration; > + int ret; > + > + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, > + VFIO_DEVICE_STATE_SAVING); > + if (ret) { > + error_report("%s: Failed to set state STOP and SAVING", > + vbasedev->name); > + return ret; > + } > + > + ret = vfio_save_device_config_state(f, opaque); > + if (ret) { > + return ret; > + } > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return ret; > + } > + > + while (migration->pending_bytes > 0) { > + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > + ret = vfio_save_buffer(f, vbasedev); > + if (ret < 0) { > + error_report("%s: Failed to save buffer", vbasedev->name); > + return ret; > + } else if (ret == 0) { > + break; > + } > + > + ret = vfio_update_pending(vbasedev); > + if (ret) { > + return ret; > + } > + } > + > + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > + } > + > + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); > + if (ret) { > + error_report("%s: Failed to set state STOPPED", vbasedev->name); > + return ret; > + } > + > + trace_vfio_save_complete_precopy(vbasedev->name); > + return ret; > +} > + > static SaveVMHandlers savevm_vfio_handlers = { > .save_setup = vfio_save_setup, > .save_cleanup = vfio_save_cleanup, > + .save_live_pending = vfio_save_pending, > + .save_live_iterate = vfio_save_iterate, > + .save_live_complete_precopy = vfio_save_complete_precopy, > }; > > /* ---------------------------------------------------------------------- */ > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index 4bb43f18f315..bdf40ba368c7 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -151,3 +151,9 @@ vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_st > vfio_migration_state_notifier(char *name, int state) " (%s) state %d" > vfio_save_setup(char *name) " (%s)" > vfio_save_cleanup(char *name) " (%s)" > +vfio_save_buffer(char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64 > +vfio_update_pending(char *name, uint64_t pending) " (%s) pending 0x%"PRIx64 > +vfio_save_device_config_state(char *name) " (%s)" > +vfio_save_pending(char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64 > +vfio_save_iterate(char *name, int data_size) " (%s) data_size %d" > +vfio_save_complete_precopy(char *name) " (%s)" > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 28f55f66d019..c78033e4149d 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -60,6 +60,7 @@ typedef struct VFIORegion { > > typedef struct VFIOMigration { > VFIORegion region; > + uint64_t pending_bytes; > } VFIOMigration; > > typedef struct VFIOAddressSpace { > -- > 2.7.0 >
On 5/5/2020 10:07 AM, Alex Williamson wrote: > On Tue, 5 May 2020 04:48:14 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > >> On 3/26/2020 3:33 AM, Alex Williamson wrote: >>> On Wed, 25 Mar 2020 02:39:07 +0530 >>> Kirti Wankhede <kwankhede@nvidia.com> wrote: >>> >>>> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy >>>> functions. These functions handles pre-copy and stop-and-copy phase. >>>> >>>> In _SAVING|_RUNNING device state or pre-copy phase: >>>> - read pending_bytes. If pending_bytes > 0, go through below steps. >>>> - read data_offset - indicates kernel driver to write data to staging >>>> buffer. >>>> - read data_size - amount of data in bytes written by vendor driver in >>>> migration region. >>>> - read data_size bytes of data from data_offset in the migration region. >>>> - Write data packet to file stream as below: >>>> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, >>>> VFIO_MIG_FLAG_END_OF_STATE } >>>> >>>> In _SAVING device state or stop-and-copy phase >>>> a. read config space of device and save to migration file stream. This >>>> doesn't need to be from vendor driver. Any other special config state >>>> from driver can be saved as data in following iteration. >>>> b. read pending_bytes. If pending_bytes > 0, go through below steps. >>>> c. read data_offset - indicates kernel driver to write data to staging >>>> buffer. >>>> d. read data_size - amount of data in bytes written by vendor driver in >>>> migration region. >>>> e. read data_size bytes of data from data_offset in the migration region. >>>> f. Write data packet as below: >>>> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} >>>> g. iterate through steps b to f while (pending_bytes > 0) >>>> h. Write {VFIO_MIG_FLAG_END_OF_STATE} >>>> >>>> When data region is mapped, its user's responsibility to read data from >>>> data_offset of data_size before moving to next steps. >>>> >>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >>>> Reviewed-by: Neo Jia <cjia@nvidia.com> >>>> --- >>>> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- >>>> hw/vfio/trace-events | 6 ++ >>>> include/hw/vfio/vfio-common.h | 1 + >>>> 3 files changed, 251 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >>>> index 033f76526e49..ecbeed5182c2 100644 >>>> --- a/hw/vfio/migration.c >>>> +++ b/hw/vfio/migration.c >>>> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, >>>> return 0; >>>> } >>>> >>>> +static void *find_data_region(VFIORegion *region, >>>> + uint64_t data_offset, >>>> + uint64_t data_size) >>>> +{ >>>> + void *ptr = NULL; >>>> + int i; >>>> + >>>> + for (i = 0; i < region->nr_mmaps; i++) { >>>> + if ((data_offset >= region->mmaps[i].offset) && >>>> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && >>>> + (data_size <= region->mmaps[i].size)) { >>> >>> (data_offset - region->mmaps[i].offset) can be non-zero, so this test >>> is invalid. Additionally the uapi does not require that a give data >>> chunk fits exclusively within an mmap'd area, it may overlap one or >>> more mmap'd sections of the region, possibly with non-mmap'd areas >>> included. >>> >> >> What's the advantage of having mmap and non-mmap overlapped regions? >> Isn't it better to have data section either mapped or trapped? > > The spec allows for it, therefore we need to support it. A vendor > driver might choose to include a header with sequence and checksum > information for each transaction, they might accomplish this by setting > data_offset to a trapped area backed by kernel memory followed by an > area supporting direct mmap to the device. The target end could then > fault on writing the header if the sequence information is incorrect. > A trapped area at the end of the transaction could allow the vendor > driver to validate a checksum. > If mmap and non-mmap regions overlapped is allowed then here read() should be used, which means buffer is allocated, then get data in buffer (first memcpy) and then call qemu_put_buffer(f, buf, data_size) (second memcpy) Advantage of using full mmaped region for data, qemu_put_buffer(f, buf, data_size) directly uses pointer to mmaped region and so we reduce one memcpy. >>>> + ptr = region->mmaps[i].mmap + (data_offset - >>>> + region->mmaps[i].offset); >>>> + break; >>>> + } >>>> + } >>>> + return ptr; >>>> +} >>>> + >>>> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) >>>> +{ >>>> + VFIOMigration *migration = vbasedev->migration; >>>> + VFIORegion *region = &migration->region; >>>> + uint64_t data_offset = 0, data_size = 0; >>>> + int ret; >>>> + >>>> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, >>>> + data_offset)); >>>> + if (ret != sizeof(data_offset)) { >>>> + error_report("%s: Failed to get migration buffer data offset %d", >>>> + vbasedev->name, ret); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, >>>> + data_size)); >>>> + if (ret != sizeof(data_size)) { >>>> + error_report("%s: Failed to get migration buffer data size %d", >>>> + vbasedev->name, ret); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (data_size > 0) { >>>> + void *buf = NULL; >>>> + bool buffer_mmaped; >>>> + >>>> + if (region->mmaps) { >>>> + buf = find_data_region(region, data_offset, data_size); >>>> + } >>>> + >>>> + buffer_mmaped = (buf != NULL) ? true : false; >>> >>> The ternary is unnecessary, "? true : false" is redundant. >>> >> >> Removing it. >> >>>> + >>>> + if (!buffer_mmaped) { >>>> + buf = g_try_malloc0(data_size); >>> >>> Why do we need zero'd memory? >>> >> >> Zeroed memory not required, removing 0 >> >>>> + if (!buf) { >>>> + error_report("%s: Error allocating buffer ", __func__); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + ret = pread(vbasedev->fd, buf, data_size, >>>> + region->fd_offset + data_offset); >>>> + if (ret != data_size) { >>>> + error_report("%s: Failed to get migration data %d", >>>> + vbasedev->name, ret); >>>> + g_free(buf); >>>> + return -EINVAL; >>>> + } >>>> + } >>>> + >>>> + qemu_put_be64(f, data_size); >>>> + qemu_put_buffer(f, buf, data_size); >>> >>> This can segfault when mmap'd given the above assumptions about size >>> and layout. >>> >>>> + >>>> + if (!buffer_mmaped) { >>>> + g_free(buf); >>>> + } >>>> + } else { >>>> + qemu_put_be64(f, data_size); >>> >>> We insert a zero? Couldn't we add the section header and end here and >>> skip it entirely? >>> >> >> This is used during resuming, data_size 0 indicates end of data. >> >>>> + } >>>> + >>>> + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, >>>> + migration->pending_bytes); >>>> + >>>> + ret = qemu_file_get_error(f); >>>> + if (ret) { >>>> + return ret; >>>> + } >>>> + >>>> + return data_size; >>>> +} >>>> + >>>> +static int vfio_update_pending(VFIODevice *vbasedev) >>>> +{ >>>> + VFIOMigration *migration = vbasedev->migration; >>>> + VFIORegion *region = &migration->region; >>>> + uint64_t pending_bytes = 0; >>>> + int ret; >>>> + >>>> + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, >>>> + pending_bytes)); >>>> + if ((ret < 0) || (ret != sizeof(pending_bytes))) { >>>> + error_report("%s: Failed to get pending bytes %d", >>>> + vbasedev->name, ret); >>>> + migration->pending_bytes = 0; >>>> + return (ret < 0) ? ret : -EINVAL; >>>> + } >>>> + >>>> + migration->pending_bytes = pending_bytes; >>>> + trace_vfio_update_pending(vbasedev->name, pending_bytes); >>>> + return 0; >>>> +} >>>> + >>>> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); >>>> + >>>> + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { >>>> + vbasedev->ops->vfio_save_config(vbasedev, f); >>>> + } >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >>>> + >>>> + trace_vfio_save_device_config_state(vbasedev->name); >>>> + >>>> + return qemu_file_get_error(f); >>>> +} >>>> + >>>> /* ---------------------------------------------------------------------- */ >>>> >>>> static int vfio_save_setup(QEMUFile *f, void *opaque) >>>> @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) >>>> qemu_mutex_unlock_iothread(); >>>> if (ret) { >>>> error_report("%s: Failed to mmap VFIO migration region %d: %s", >>>> - vbasedev->name, migration->region.index, >>>> + vbasedev->name, migration->region.nr, >>>> strerror(-ret)); >>>> return ret; >>>> } >>>> @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) >>>> trace_vfio_save_cleanup(vbasedev->name); >>>> } >>>> >>>> +static void vfio_save_pending(QEMUFile *f, void *opaque, >>>> + uint64_t threshold_size, >>>> + uint64_t *res_precopy_only, >>>> + uint64_t *res_compatible, >>>> + uint64_t *res_postcopy_only) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + VFIOMigration *migration = vbasedev->migration; >>>> + int ret; >>>> + >>>> + ret = vfio_update_pending(vbasedev); >>>> + if (ret) { >>>> + return; >>>> + } >>>> + >>>> + *res_precopy_only += migration->pending_bytes; >>>> + >>>> + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, >>>> + *res_postcopy_only, *res_compatible); >>>> +} >>>> + >>>> +static int vfio_save_iterate(QEMUFile *f, void *opaque) >>>> +{ >>>> + VFIODevice *vbasedev = opaque; >>>> + int ret, data_size; >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); >>>> + >>>> + data_size = vfio_save_buffer(f, vbasedev); >>>> + >>>> + if (data_size < 0) { >>>> + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, >>>> + strerror(errno)); >>>> + return data_size; >>>> + } >>>> + >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); >>>> + >>>> + ret = qemu_file_get_error(f); >>>> + if (ret) { >>>> + return ret; >>>> + } >>>> + >>>> + trace_vfio_save_iterate(vbasedev->name, data_size); >>>> + if (data_size == 0) { >>>> + /* indicates data finished, goto complete phase */ >>>> + return 1; >>> >>> But it's pending_bytes not data_size that indicates we're done. How do >>> we get away with ignoring pending_bytes for the save_live_iterate phase? >>> >> >> This is requirement mentioned above qemu_savevm_state_iterate() which >> calls .save_live_iterate. >> >> /* >> * this function has three return values: >> * negative: there was one error, and we have -errno. >> * 0 : We haven't finished, caller have to go again >> * 1 : We have finished, we can go to complete phase >> */ >> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy) >> >> This is to serialize savevm_state.handlers (or in other words devices). > > I've lost all context on this question in the interim, but I think this > highlights my question. We use pending_bytes to know how close we are > to the end of the stream and data_size to iterate each transaction > within that stream. So how does data_size == 0 indicate we've > completed the current phase? It seems like pending_bytes should > indicate that. Thanks, > Fixing this by adding a read on pending_bytes if its 0 and return accordingly. if (migration->pending_bytes == 0) { ret = vfio_update_pending(vbasedev); if (ret) { return ret; } if (migration->pending_bytes == 0) { /* indicates data finished, goto complete phase */ return 1; } } Thanks, Kirti
On 5/9/2020 11:01 AM, Yan Zhao wrote: > On Wed, Mar 25, 2020 at 05:09:07AM +0800, Kirti Wankhede wrote: >> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy >> functions. These functions handles pre-copy and stop-and-copy phase. >> >> In _SAVING|_RUNNING device state or pre-copy phase: >> - read pending_bytes. If pending_bytes > 0, go through below steps. >> - read data_offset - indicates kernel driver to write data to staging >> buffer. >> - read data_size - amount of data in bytes written by vendor driver in >> migration region. > I think we should change the sequence of reading data_size and > data_offset. see the next comment below. > >> - read data_size bytes of data from data_offset in the migration region. >> - Write data packet to file stream as below: >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, >> VFIO_MIG_FLAG_END_OF_STATE } >> >> In _SAVING device state or stop-and-copy phase >> a. read config space of device and save to migration file stream. This >> doesn't need to be from vendor driver. Any other special config state >> from driver can be saved as data in following iteration. >> b. read pending_bytes. If pending_bytes > 0, go through below steps. >> c. read data_offset - indicates kernel driver to write data to staging >> buffer. >> d. read data_size - amount of data in bytes written by vendor driver in >> migration region. >> e. read data_size bytes of data from data_offset in the migration region. >> f. Write data packet as below: >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} >> g. iterate through steps b to f while (pending_bytes > 0) >> h. Write {VFIO_MIG_FLAG_END_OF_STATE} >> >> When data region is mapped, its user's responsibility to read data from >> data_offset of data_size before moving to next steps. >> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >> Reviewed-by: Neo Jia <cjia@nvidia.com> >> --- >> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- >> hw/vfio/trace-events | 6 ++ >> include/hw/vfio/vfio-common.h | 1 + >> 3 files changed, 251 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index 033f76526e49..ecbeed5182c2 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, >> return 0; >> } >> >> +static void *find_data_region(VFIORegion *region, >> + uint64_t data_offset, >> + uint64_t data_size) >> +{ >> + void *ptr = NULL; >> + int i; >> + >> + for (i = 0; i < region->nr_mmaps; i++) { >> + if ((data_offset >= region->mmaps[i].offset) && >> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && >> + (data_size <= region->mmaps[i].size)) { >> + ptr = region->mmaps[i].mmap + (data_offset - >> + region->mmaps[i].offset); >> + break; >> + } >> + } >> + return ptr; >> +} >> + >> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + VFIORegion *region = &migration->region; >> + uint64_t data_offset = 0, data_size = 0; >> + int ret; >> + >> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), >> + region->fd_offset + offsetof(struct vfio_device_migration_info, >> + data_offset)); >> + if (ret != sizeof(data_offset)) { >> + error_report("%s: Failed to get migration buffer data offset %d", >> + vbasedev->name, ret); >> + return -EINVAL; >> + } >> + >> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), >> + region->fd_offset + offsetof(struct vfio_device_migration_info, >> + data_size)); >> + if (ret != sizeof(data_size)) { >> + error_report("%s: Failed to get migration buffer data size %d", >> + vbasedev->name, ret); >> + return -EINVAL; >> + } > data_size should be read first, and if it's 0, data_offset will not > be read further. > > the reasons are below: > 1. if there's no data region provided by vendor driver, there's no > reason to get a valid data_offset, so reading/writing of data_offset > should fail. And this should not be treated as a migration error. > > 2. even if pending_bytes is 0, vfio_save_iterate() is still possible to be > called and therefore vfio_save_buffer() is called. > As I mentioned in reply to Alex in: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02476.html With that, vfio_save_iterate() will read pending_bytes if its 0 and then if pending_bytes is not 0 then call vfio_save_buffer(). With that your above concerns should get resolved. Thanks, Kirti
On Mon, 11 May 2020 15:23:37 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 5/5/2020 10:07 AM, Alex Williamson wrote: > > On Tue, 5 May 2020 04:48:14 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > >> On 3/26/2020 3:33 AM, Alex Williamson wrote: > >>> On Wed, 25 Mar 2020 02:39:07 +0530 > >>> Kirti Wankhede <kwankhede@nvidia.com> wrote: > >>> > >>>> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy > >>>> functions. These functions handles pre-copy and stop-and-copy phase. > >>>> > >>>> In _SAVING|_RUNNING device state or pre-copy phase: > >>>> - read pending_bytes. If pending_bytes > 0, go through below steps. > >>>> - read data_offset - indicates kernel driver to write data to staging > >>>> buffer. > >>>> - read data_size - amount of data in bytes written by vendor driver in > >>>> migration region. > >>>> - read data_size bytes of data from data_offset in the migration region. > >>>> - Write data packet to file stream as below: > >>>> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, > >>>> VFIO_MIG_FLAG_END_OF_STATE } > >>>> > >>>> In _SAVING device state or stop-and-copy phase > >>>> a. read config space of device and save to migration file stream. This > >>>> doesn't need to be from vendor driver. Any other special config state > >>>> from driver can be saved as data in following iteration. > >>>> b. read pending_bytes. If pending_bytes > 0, go through below steps. > >>>> c. read data_offset - indicates kernel driver to write data to staging > >>>> buffer. > >>>> d. read data_size - amount of data in bytes written by vendor driver in > >>>> migration region. > >>>> e. read data_size bytes of data from data_offset in the migration region. > >>>> f. Write data packet as below: > >>>> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} > >>>> g. iterate through steps b to f while (pending_bytes > 0) > >>>> h. Write {VFIO_MIG_FLAG_END_OF_STATE} > >>>> > >>>> When data region is mapped, its user's responsibility to read data from > >>>> data_offset of data_size before moving to next steps. > >>>> > >>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > >>>> Reviewed-by: Neo Jia <cjia@nvidia.com> > >>>> --- > >>>> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- > >>>> hw/vfio/trace-events | 6 ++ > >>>> include/hw/vfio/vfio-common.h | 1 + > >>>> 3 files changed, 251 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > >>>> index 033f76526e49..ecbeed5182c2 100644 > >>>> --- a/hw/vfio/migration.c > >>>> +++ b/hw/vfio/migration.c > >>>> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, > >>>> return 0; > >>>> } > >>>> > >>>> +static void *find_data_region(VFIORegion *region, > >>>> + uint64_t data_offset, > >>>> + uint64_t data_size) > >>>> +{ > >>>> + void *ptr = NULL; > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < region->nr_mmaps; i++) { > >>>> + if ((data_offset >= region->mmaps[i].offset) && > >>>> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && > >>>> + (data_size <= region->mmaps[i].size)) { > >>> > >>> (data_offset - region->mmaps[i].offset) can be non-zero, so this test > >>> is invalid. Additionally the uapi does not require that a give data > >>> chunk fits exclusively within an mmap'd area, it may overlap one or > >>> more mmap'd sections of the region, possibly with non-mmap'd areas > >>> included. > >>> > >> > >> What's the advantage of having mmap and non-mmap overlapped regions? > >> Isn't it better to have data section either mapped or trapped? > > > > The spec allows for it, therefore we need to support it. A vendor > > driver might choose to include a header with sequence and checksum > > information for each transaction, they might accomplish this by setting > > data_offset to a trapped area backed by kernel memory followed by an > > area supporting direct mmap to the device. The target end could then > > fault on writing the header if the sequence information is incorrect. > > A trapped area at the end of the transaction could allow the vendor > > driver to validate a checksum. > > > > If mmap and non-mmap regions overlapped is allowed then here read() > should be used, which means buffer is allocated, then get data in buffer > (first memcpy) and then call qemu_put_buffer(f, buf, data_size) (second > memcpy) > > Advantage of using full mmaped region for data, qemu_put_buffer(f, buf, > data_size) directly uses pointer to mmaped region and so we reduce one > memcpy. If userspace wants to use read() for intermixed data ranges, they may. Using the mmap access for portions that allow it can improve efficiency. None of this changes the fact that the code I pointed out here has a bug and that bug still exists in v18: +static void *find_data_region(VFIORegion *region, + uint64_t data_offset, + uint64_t data_size) +{ + void *ptr = NULL; + int i; + + for (i = 0; i < region->nr_mmaps; i++) { + if ((data_offset >= region->mmaps[i].offset) && + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && These two tests verify that we have: region->mmaps[i].offset <= data_offset < (region->mmaps[i].offset + region->mmaps[i].size) ie. data_offset is somewhere within the start of the mmap capable range. + (data_size <= region->mmaps[i].size)) { This makes the wild assumption that data_offset == region->mmaps[i].offset + ptr = region->mmaps[i].mmap + (data_offset - + region->mmaps[i].offset); + break; + } + } + return ptr; +} Therefore, if the *start* of the data offset falls within an mmap, we pass back an mmap pointer to the caller who goes on to assume the entire data_size is available through that pointer. I think the latter tests needs to be something like: (data_size <= region->mmaps[i].size - (data_offset - region->mmaps[i].offset)) Or a more optimal solution might be to pass back a size and iterate over the data in chunks, using mmap for the extents available. Thanks, Alex > >>>> + ptr = region->mmaps[i].mmap + (data_offset - > >>>> + region->mmaps[i].offset); > >>>> + break; > >>>> + } > >>>> + } > >>>> + return ptr; > >>>> +} > >>>> + > >>>> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) > >>>> +{ > >>>> + VFIOMigration *migration = vbasedev->migration; > >>>> + VFIORegion *region = &migration->region; > >>>> + uint64_t data_offset = 0, data_size = 0; > >>>> + int ret; > >>>> + > >>>> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), > >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >>>> + data_offset)); > >>>> + if (ret != sizeof(data_offset)) { > >>>> + error_report("%s: Failed to get migration buffer data offset %d", > >>>> + vbasedev->name, ret); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), > >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >>>> + data_size)); > >>>> + if (ret != sizeof(data_size)) { > >>>> + error_report("%s: Failed to get migration buffer data size %d", > >>>> + vbasedev->name, ret); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + if (data_size > 0) { > >>>> + void *buf = NULL; > >>>> + bool buffer_mmaped; > >>>> + > >>>> + if (region->mmaps) { > >>>> + buf = find_data_region(region, data_offset, data_size); > >>>> + } > >>>> + > >>>> + buffer_mmaped = (buf != NULL) ? true : false; > >>> > >>> The ternary is unnecessary, "? true : false" is redundant. > >>> > >> > >> Removing it. > >> > >>>> + > >>>> + if (!buffer_mmaped) { > >>>> + buf = g_try_malloc0(data_size); > >>> > >>> Why do we need zero'd memory? > >>> > >> > >> Zeroed memory not required, removing 0 > >> > >>>> + if (!buf) { > >>>> + error_report("%s: Error allocating buffer ", __func__); > >>>> + return -ENOMEM; > >>>> + } > >>>> + > >>>> + ret = pread(vbasedev->fd, buf, data_size, > >>>> + region->fd_offset + data_offset); > >>>> + if (ret != data_size) { > >>>> + error_report("%s: Failed to get migration data %d", > >>>> + vbasedev->name, ret); > >>>> + g_free(buf); > >>>> + return -EINVAL; > >>>> + } > >>>> + } > >>>> + > >>>> + qemu_put_be64(f, data_size); > >>>> + qemu_put_buffer(f, buf, data_size); > >>> > >>> This can segfault when mmap'd given the above assumptions about size > >>> and layout. > >>> > >>>> + > >>>> + if (!buffer_mmaped) { > >>>> + g_free(buf); > >>>> + } > >>>> + } else { > >>>> + qemu_put_be64(f, data_size); > >>> > >>> We insert a zero? Couldn't we add the section header and end here and > >>> skip it entirely? > >>> > >> > >> This is used during resuming, data_size 0 indicates end of data. > >> > >>>> + } > >>>> + > >>>> + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, > >>>> + migration->pending_bytes); > >>>> + > >>>> + ret = qemu_file_get_error(f); > >>>> + if (ret) { > >>>> + return ret; > >>>> + } > >>>> + > >>>> + return data_size; > >>>> +} > >>>> + > >>>> +static int vfio_update_pending(VFIODevice *vbasedev) > >>>> +{ > >>>> + VFIOMigration *migration = vbasedev->migration; > >>>> + VFIORegion *region = &migration->region; > >>>> + uint64_t pending_bytes = 0; > >>>> + int ret; > >>>> + > >>>> + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), > >>>> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >>>> + pending_bytes)); > >>>> + if ((ret < 0) || (ret != sizeof(pending_bytes))) { > >>>> + error_report("%s: Failed to get pending bytes %d", > >>>> + vbasedev->name, ret); > >>>> + migration->pending_bytes = 0; > >>>> + return (ret < 0) ? ret : -EINVAL; > >>>> + } > >>>> + > >>>> + migration->pending_bytes = pending_bytes; > >>>> + trace_vfio_update_pending(vbasedev->name, pending_bytes); > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) > >>>> +{ > >>>> + VFIODevice *vbasedev = opaque; > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); > >>>> + > >>>> + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { > >>>> + vbasedev->ops->vfio_save_config(vbasedev, f); > >>>> + } > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > >>>> + > >>>> + trace_vfio_save_device_config_state(vbasedev->name); > >>>> + > >>>> + return qemu_file_get_error(f); > >>>> +} > >>>> + > >>>> /* ---------------------------------------------------------------------- */ > >>>> > >>>> static int vfio_save_setup(QEMUFile *f, void *opaque) > >>>> @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > >>>> qemu_mutex_unlock_iothread(); > >>>> if (ret) { > >>>> error_report("%s: Failed to mmap VFIO migration region %d: %s", > >>>> - vbasedev->name, migration->region.index, > >>>> + vbasedev->name, migration->region.nr, > >>>> strerror(-ret)); > >>>> return ret; > >>>> } > >>>> @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) > >>>> trace_vfio_save_cleanup(vbasedev->name); > >>>> } > >>>> > >>>> +static void vfio_save_pending(QEMUFile *f, void *opaque, > >>>> + uint64_t threshold_size, > >>>> + uint64_t *res_precopy_only, > >>>> + uint64_t *res_compatible, > >>>> + uint64_t *res_postcopy_only) > >>>> +{ > >>>> + VFIODevice *vbasedev = opaque; > >>>> + VFIOMigration *migration = vbasedev->migration; > >>>> + int ret; > >>>> + > >>>> + ret = vfio_update_pending(vbasedev); > >>>> + if (ret) { > >>>> + return; > >>>> + } > >>>> + > >>>> + *res_precopy_only += migration->pending_bytes; > >>>> + > >>>> + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, > >>>> + *res_postcopy_only, *res_compatible); > >>>> +} > >>>> + > >>>> +static int vfio_save_iterate(QEMUFile *f, void *opaque) > >>>> +{ > >>>> + VFIODevice *vbasedev = opaque; > >>>> + int ret, data_size; > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > >>>> + > >>>> + data_size = vfio_save_buffer(f, vbasedev); > >>>> + > >>>> + if (data_size < 0) { > >>>> + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, > >>>> + strerror(errno)); > >>>> + return data_size; > >>>> + } > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > >>>> + > >>>> + ret = qemu_file_get_error(f); > >>>> + if (ret) { > >>>> + return ret; > >>>> + } > >>>> + > >>>> + trace_vfio_save_iterate(vbasedev->name, data_size); > >>>> + if (data_size == 0) { > >>>> + /* indicates data finished, goto complete phase */ > >>>> + return 1; > >>> > >>> But it's pending_bytes not data_size that indicates we're done. How do > >>> we get away with ignoring pending_bytes for the save_live_iterate phase? > >>> > >> > >> This is requirement mentioned above qemu_savevm_state_iterate() which > >> calls .save_live_iterate. > >> > >> /* > >> * this function has three return values: > >> * negative: there was one error, and we have -errno. > >> * 0 : We haven't finished, caller have to go again > >> * 1 : We have finished, we can go to complete phase > >> */ > >> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy) > >> > >> This is to serialize savevm_state.handlers (or in other words devices). > > > > I've lost all context on this question in the interim, but I think this > > highlights my question. We use pending_bytes to know how close we are > > to the end of the stream and data_size to iterate each transaction > > within that stream. So how does data_size == 0 indicate we've > > completed the current phase? It seems like pending_bytes should > > indicate that. Thanks, > > > > Fixing this by adding a read on pending_bytes if its 0 and return > accordingly. > if (migration->pending_bytes == 0) { > ret = vfio_update_pending(vbasedev); > if (ret) { > return ret; > } > > if (migration->pending_bytes == 0) { > /* indicates data finished, goto complete phase */ > return 1; > } > } > > Thanks, > Kirti > >
On Mon, May 11, 2020 at 06:22:47PM +0800, Kirti Wankhede wrote: > > > On 5/9/2020 11:01 AM, Yan Zhao wrote: > > On Wed, Mar 25, 2020 at 05:09:07AM +0800, Kirti Wankhede wrote: > >> Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy > >> functions. These functions handles pre-copy and stop-and-copy phase. > >> > >> In _SAVING|_RUNNING device state or pre-copy phase: > >> - read pending_bytes. If pending_bytes > 0, go through below steps. > >> - read data_offset - indicates kernel driver to write data to staging > >> buffer. > >> - read data_size - amount of data in bytes written by vendor driver in > >> migration region. > > I think we should change the sequence of reading data_size and > > data_offset. see the next comment below. > > > >> - read data_size bytes of data from data_offset in the migration region. > >> - Write data packet to file stream as below: > >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, > >> VFIO_MIG_FLAG_END_OF_STATE } > >> > >> In _SAVING device state or stop-and-copy phase > >> a. read config space of device and save to migration file stream. This > >> doesn't need to be from vendor driver. Any other special config state > >> from driver can be saved as data in following iteration. > >> b. read pending_bytes. If pending_bytes > 0, go through below steps. > >> c. read data_offset - indicates kernel driver to write data to staging > >> buffer. > >> d. read data_size - amount of data in bytes written by vendor driver in > >> migration region. > >> e. read data_size bytes of data from data_offset in the migration region. > >> f. Write data packet as below: > >> {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} > >> g. iterate through steps b to f while (pending_bytes > 0) > >> h. Write {VFIO_MIG_FLAG_END_OF_STATE} > >> > >> When data region is mapped, its user's responsibility to read data from > >> data_offset of data_size before moving to next steps. > >> > >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > >> Reviewed-by: Neo Jia <cjia@nvidia.com> > >> --- > >> hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- > >> hw/vfio/trace-events | 6 ++ > >> include/hw/vfio/vfio-common.h | 1 + > >> 3 files changed, 251 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > >> index 033f76526e49..ecbeed5182c2 100644 > >> --- a/hw/vfio/migration.c > >> +++ b/hw/vfio/migration.c > >> @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, > >> return 0; > >> } > >> > >> +static void *find_data_region(VFIORegion *region, > >> + uint64_t data_offset, > >> + uint64_t data_size) > >> +{ > >> + void *ptr = NULL; > >> + int i; > >> + > >> + for (i = 0; i < region->nr_mmaps; i++) { > >> + if ((data_offset >= region->mmaps[i].offset) && > >> + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && > >> + (data_size <= region->mmaps[i].size)) { > >> + ptr = region->mmaps[i].mmap + (data_offset - > >> + region->mmaps[i].offset); > >> + break; > >> + } > >> + } > >> + return ptr; > >> +} > >> + > >> +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) > >> +{ > >> + VFIOMigration *migration = vbasedev->migration; > >> + VFIORegion *region = &migration->region; > >> + uint64_t data_offset = 0, data_size = 0; > >> + int ret; > >> + > >> + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), > >> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >> + data_offset)); > >> + if (ret != sizeof(data_offset)) { > >> + error_report("%s: Failed to get migration buffer data offset %d", > >> + vbasedev->name, ret); > >> + return -EINVAL; > >> + } > >> + > >> + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), > >> + region->fd_offset + offsetof(struct vfio_device_migration_info, > >> + data_size)); > >> + if (ret != sizeof(data_size)) { > >> + error_report("%s: Failed to get migration buffer data size %d", > >> + vbasedev->name, ret); > >> + return -EINVAL; > >> + } > > data_size should be read first, and if it's 0, data_offset will not > > be read further. > > > > the reasons are below: > > 1. if there's no data region provided by vendor driver, there's no > > reason to get a valid data_offset, so reading/writing of data_offset > > should fail. And this should not be treated as a migration error. > > > > 2. even if pending_bytes is 0, vfio_save_iterate() is still possible to be > > called and therefore vfio_save_buffer() is called. > > > > As I mentioned in reply to Alex in: > https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02476.html > > With that, vfio_save_iterate() will read pending_bytes if its 0 and then > if pending_bytes is not 0 then call vfio_save_buffer(). With that your > above concerns should get resolved. > what if pending_bytes is not 0, but vendor driver just does not want to send data in this iteration? isn't it right to get data_size first before getting data_offset? Thanks Yan
On Mon, May 11, 2020 at 05:53:37PM +0800, Kirti Wankhede wrote: > > > On 5/5/2020 10:07 AM, Alex Williamson wrote: > > On Tue, 5 May 2020 04:48:14 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > >> On 3/26/2020 3:33 AM, Alex Williamson wrote: > >>> On Wed, 25 Mar 2020 02:39:07 +0530 > >>> Kirti Wankhede <kwankhede@nvidia.com> wrote: > >>> <...> > >>>> +static int vfio_save_iterate(QEMUFile *f, void *opaque) > >>>> +{ > >>>> + VFIODevice *vbasedev = opaque; > >>>> + int ret, data_size; > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); > >>>> + > >>>> + data_size = vfio_save_buffer(f, vbasedev); > >>>> + > >>>> + if (data_size < 0) { > >>>> + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, > >>>> + strerror(errno)); > >>>> + return data_size; > >>>> + } > >>>> + > >>>> + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); > >>>> + > >>>> + ret = qemu_file_get_error(f); > >>>> + if (ret) { > >>>> + return ret; > >>>> + } > >>>> + > >>>> + trace_vfio_save_iterate(vbasedev->name, data_size); > >>>> + if (data_size == 0) { > >>>> + /* indicates data finished, goto complete phase */ > >>>> + return 1; > >>> > >>> But it's pending_bytes not data_size that indicates we're done. How do > >>> we get away with ignoring pending_bytes for the save_live_iterate phase? > >>> > >> > >> This is requirement mentioned above qemu_savevm_state_iterate() which > >> calls .save_live_iterate. > >> > >> /* > >> * this function has three return values: > >> * negative: there was one error, and we have -errno. > >> * 0 : We haven't finished, caller have to go again > >> * 1 : We have finished, we can go to complete phase > >> */ > >> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy) > >> > >> This is to serialize savevm_state.handlers (or in other words devices). > > > > I've lost all context on this question in the interim, but I think this > > highlights my question. We use pending_bytes to know how close we are > > to the end of the stream and data_size to iterate each transaction > > within that stream. So how does data_size == 0 indicate we've > > completed the current phase? It seems like pending_bytes should > > indicate that. Thanks, > > > > Fixing this by adding a read on pending_bytes if its 0 and return > accordingly. > if (migration->pending_bytes == 0) { > ret = vfio_update_pending(vbasedev); > if (ret) { > return ret; > } > > if (migration->pending_bytes == 0) { > /* indicates data finished, goto complete phase */ > return 1; > } > } > just a question. if 1 is only returned when migration->pending_bytes is 0, does that mean .save_live_iterate of vmstates after "vfio-pci" would never be called until migration->pending_bytes is 0 ? as in qemu_savevm_state_iterate(), qemu_savevm_state_iterate { ... QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { ... ret = se->ops->save_live_iterate(f, se->opaque); ... if (ret <= 0) { /* Do not proceed to the next vmstate before this one reported completion of the current stage. This serializes the migration and reduces the probability that a faster changing state is synchronized over and over again. */ break; } } return ret; } in ram's migration code, its pending_bytes(remaining_size) is only updated in ram_save_pending() when it's below threshold, which means in ram_save_iterate() the pending_bytes is possible to be 0, so other vmstates have their chance to be called. Thanks Yan
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 033f76526e49..ecbeed5182c2 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -138,6 +138,137 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask, return 0; } +static void *find_data_region(VFIORegion *region, + uint64_t data_offset, + uint64_t data_size) +{ + void *ptr = NULL; + int i; + + for (i = 0; i < region->nr_mmaps; i++) { + if ((data_offset >= region->mmaps[i].offset) && + (data_offset < region->mmaps[i].offset + region->mmaps[i].size) && + (data_size <= region->mmaps[i].size)) { + ptr = region->mmaps[i].mmap + (data_offset - + region->mmaps[i].offset); + break; + } + } + return ptr; +} + +static int vfio_save_buffer(QEMUFile *f, VFIODevice *vbasedev) +{ + VFIOMigration *migration = vbasedev->migration; + VFIORegion *region = &migration->region; + uint64_t data_offset = 0, data_size = 0; + int ret; + + ret = pread(vbasedev->fd, &data_offset, sizeof(data_offset), + region->fd_offset + offsetof(struct vfio_device_migration_info, + data_offset)); + if (ret != sizeof(data_offset)) { + error_report("%s: Failed to get migration buffer data offset %d", + vbasedev->name, ret); + return -EINVAL; + } + + ret = pread(vbasedev->fd, &data_size, sizeof(data_size), + region->fd_offset + offsetof(struct vfio_device_migration_info, + data_size)); + if (ret != sizeof(data_size)) { + error_report("%s: Failed to get migration buffer data size %d", + vbasedev->name, ret); + return -EINVAL; + } + + if (data_size > 0) { + void *buf = NULL; + bool buffer_mmaped; + + if (region->mmaps) { + buf = find_data_region(region, data_offset, data_size); + } + + buffer_mmaped = (buf != NULL) ? true : false; + + if (!buffer_mmaped) { + buf = g_try_malloc0(data_size); + if (!buf) { + error_report("%s: Error allocating buffer ", __func__); + return -ENOMEM; + } + + ret = pread(vbasedev->fd, buf, data_size, + region->fd_offset + data_offset); + if (ret != data_size) { + error_report("%s: Failed to get migration data %d", + vbasedev->name, ret); + g_free(buf); + return -EINVAL; + } + } + + qemu_put_be64(f, data_size); + qemu_put_buffer(f, buf, data_size); + + if (!buffer_mmaped) { + g_free(buf); + } + } else { + qemu_put_be64(f, data_size); + } + + trace_vfio_save_buffer(vbasedev->name, data_offset, data_size, + migration->pending_bytes); + + ret = qemu_file_get_error(f); + if (ret) { + return ret; + } + + return data_size; +} + +static int vfio_update_pending(VFIODevice *vbasedev) +{ + VFIOMigration *migration = vbasedev->migration; + VFIORegion *region = &migration->region; + uint64_t pending_bytes = 0; + int ret; + + ret = pread(vbasedev->fd, &pending_bytes, sizeof(pending_bytes), + region->fd_offset + offsetof(struct vfio_device_migration_info, + pending_bytes)); + if ((ret < 0) || (ret != sizeof(pending_bytes))) { + error_report("%s: Failed to get pending bytes %d", + vbasedev->name, ret); + migration->pending_bytes = 0; + return (ret < 0) ? ret : -EINVAL; + } + + migration->pending_bytes = pending_bytes; + trace_vfio_update_pending(vbasedev->name, pending_bytes); + return 0; +} + +static int vfio_save_device_config_state(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE); + + if (vbasedev->ops && vbasedev->ops->vfio_save_config) { + vbasedev->ops->vfio_save_config(vbasedev, f); + } + + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); + + trace_vfio_save_device_config_state(vbasedev->name); + + return qemu_file_get_error(f); +} + /* ---------------------------------------------------------------------- */ static int vfio_save_setup(QEMUFile *f, void *opaque) @@ -154,7 +285,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) qemu_mutex_unlock_iothread(); if (ret) { error_report("%s: Failed to mmap VFIO migration region %d: %s", - vbasedev->name, migration->region.index, + vbasedev->name, migration->region.nr, strerror(-ret)); return ret; } @@ -194,9 +325,121 @@ static void vfio_save_cleanup(void *opaque) trace_vfio_save_cleanup(vbasedev->name); } +static void vfio_save_pending(QEMUFile *f, void *opaque, + uint64_t threshold_size, + uint64_t *res_precopy_only, + uint64_t *res_compatible, + uint64_t *res_postcopy_only) +{ + VFIODevice *vbasedev = opaque; + VFIOMigration *migration = vbasedev->migration; + int ret; + + ret = vfio_update_pending(vbasedev); + if (ret) { + return; + } + + *res_precopy_only += migration->pending_bytes; + + trace_vfio_save_pending(vbasedev->name, *res_precopy_only, + *res_postcopy_only, *res_compatible); +} + +static int vfio_save_iterate(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + int ret, data_size; + + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); + + data_size = vfio_save_buffer(f, vbasedev); + + if (data_size < 0) { + error_report("%s: vfio_save_buffer failed %s", vbasedev->name, + strerror(errno)); + return data_size; + } + + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); + + ret = qemu_file_get_error(f); + if (ret) { + return ret; + } + + trace_vfio_save_iterate(vbasedev->name, data_size); + if (data_size == 0) { + /* indicates data finished, goto complete phase */ + return 1; + } + + return 0; +} + +static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) +{ + VFIODevice *vbasedev = opaque; + VFIOMigration *migration = vbasedev->migration; + int ret; + + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, + VFIO_DEVICE_STATE_SAVING); + if (ret) { + error_report("%s: Failed to set state STOP and SAVING", + vbasedev->name); + return ret; + } + + ret = vfio_save_device_config_state(f, opaque); + if (ret) { + return ret; + } + + ret = vfio_update_pending(vbasedev); + if (ret) { + return ret; + } + + while (migration->pending_bytes > 0) { + qemu_put_be64(f, VFIO_MIG_FLAG_DEV_DATA_STATE); + ret = vfio_save_buffer(f, vbasedev); + if (ret < 0) { + error_report("%s: Failed to save buffer", vbasedev->name); + return ret; + } else if (ret == 0) { + break; + } + + ret = vfio_update_pending(vbasedev); + if (ret) { + return ret; + } + } + + qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE); + + ret = qemu_file_get_error(f); + if (ret) { + return ret; + } + + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); + if (ret) { + error_report("%s: Failed to set state STOPPED", vbasedev->name); + return ret; + } + + trace_vfio_save_complete_precopy(vbasedev->name); + return ret; +} + static SaveVMHandlers savevm_vfio_handlers = { .save_setup = vfio_save_setup, .save_cleanup = vfio_save_cleanup, + .save_live_pending = vfio_save_pending, + .save_live_iterate = vfio_save_iterate, + .save_live_complete_precopy = vfio_save_complete_precopy, }; /* ---------------------------------------------------------------------- */ diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index 4bb43f18f315..bdf40ba368c7 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -151,3 +151,9 @@ vfio_vmstate_change(char *name, int running, const char *reason, uint32_t dev_st vfio_migration_state_notifier(char *name, int state) " (%s) state %d" vfio_save_setup(char *name) " (%s)" vfio_save_cleanup(char *name) " (%s)" +vfio_save_buffer(char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64 +vfio_update_pending(char *name, uint64_t pending) " (%s) pending 0x%"PRIx64 +vfio_save_device_config_state(char *name) " (%s)" +vfio_save_pending(char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64 +vfio_save_iterate(char *name, int data_size) " (%s) data_size %d" +vfio_save_complete_precopy(char *name) " (%s)" diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 28f55f66d019..c78033e4149d 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -60,6 +60,7 @@ typedef struct VFIORegion { typedef struct VFIOMigration { VFIORegion region; + uint64_t pending_bytes; } VFIOMigration; typedef struct VFIOAddressSpace {
Added .save_live_pending, .save_live_iterate and .save_live_complete_precopy functions. These functions handles pre-copy and stop-and-copy phase. In _SAVING|_RUNNING device state or pre-copy phase: - read pending_bytes. If pending_bytes > 0, go through below steps. - read data_offset - indicates kernel driver to write data to staging buffer. - read data_size - amount of data in bytes written by vendor driver in migration region. - read data_size bytes of data from data_offset in the migration region. - Write data packet to file stream as below: {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data, VFIO_MIG_FLAG_END_OF_STATE } In _SAVING device state or stop-and-copy phase a. read config space of device and save to migration file stream. This doesn't need to be from vendor driver. Any other special config state from driver can be saved as data in following iteration. b. read pending_bytes. If pending_bytes > 0, go through below steps. c. read data_offset - indicates kernel driver to write data to staging buffer. d. read data_size - amount of data in bytes written by vendor driver in migration region. e. read data_size bytes of data from data_offset in the migration region. f. Write data packet as below: {VFIO_MIG_FLAG_DEV_DATA_STATE, data_size, actual data} g. iterate through steps b to f while (pending_bytes > 0) h. Write {VFIO_MIG_FLAG_END_OF_STATE} When data region is mapped, its user's responsibility to read data from data_offset of data_size before moving to next steps. Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> Reviewed-by: Neo Jia <cjia@nvidia.com> --- hw/vfio/migration.c | 245 +++++++++++++++++++++++++++++++++++++++++- hw/vfio/trace-events | 6 ++ include/hw/vfio/vfio-common.h | 1 + 3 files changed, 251 insertions(+), 1 deletion(-)