On Wed, 30 Aug 2023 18:37:38 +0800 Zhenzhong Duan <[email protected]> wrote:
> With a vfio device iterator added, we can make some migration and reset > related functions group agnostic. > E.x: > vfio_mig_active > vfio_migratable_device_num > vfio_devices_all_dirty_tracking > vfio_devices_all_device_dirty_tracking > vfio_devices_all_running_and_mig_active > vfio_devices_dma_logging_stop > vfio_devices_dma_logging_start > vfio_devices_query_dirty_bitmap > vfio_reset_handler > > Or else we need to add container specific callback variants for above > functions just because they iterate devices based on group. > > Move the reset handler registration/unregistration to a place that is not > group specific, saying first vfio address space created instead of the > first group. > > Signed-off-by: Zhenzhong Duan <[email protected]> > --- > hw/vfio/common.c | 224 ++++++++++++++++++++++++++--------------------- > 1 file changed, 122 insertions(+), 102 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 949ad6714a..51c6e7598e 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -84,6 +84,26 @@ static int vfio_ram_block_discard_disable(VFIOContainer > *container, bool state) > } > } > > +static VFIODevice *vfio_container_dev_iter_next(VFIOContainer *container, > + VFIODevice *curr) > +{ > + VFIOGroup *group; > + > + if (!curr) { > + group = QLIST_FIRST(&container->group_list); > + } else { > + if (curr->next.le_next) { > + return curr->next.le_next; > + } VFIODevice *device = QLIST_NEXT(curr, next); if (device) { return device; } > + group = curr->group->container_next.le_next; group = QLIST_NEXT(curr->group, container_next); > + } > + > + if (!group) { > + return NULL; > + } > + return QLIST_FIRST(&group->device_list); > +} > + > /* > * Device state interfaces > */ > @@ -112,17 +132,22 @@ static int vfio_get_dirty_bitmap(VFIOContainer > *container, uint64_t iova, > > bool vfio_mig_active(void) > { > - VFIOGroup *group; > + VFIOAddressSpace *space; > + VFIOContainer *container; > VFIODevice *vbasedev; > > - if (QLIST_EMPTY(&vfio_group_list)) { > + if (QLIST_EMPTY(&vfio_address_spaces)) { > return false; > } > > - QLIST_FOREACH(group, &vfio_group_list, next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (vbasedev->migration_blocker) { > - return false; > + QLIST_FOREACH(space, &vfio_address_spaces, list) { > + QLIST_FOREACH(container, &space->containers, next) { > + vbasedev = NULL; > + while ((vbasedev = vfio_container_dev_iter_next(container, > + vbasedev))) { > + if (vbasedev->migration_blocker) { > + return false; > + } Appears easy to avoid setting vbasedev in the loop iterator and improving the scope of vbasedev: VFIODevice *vbasedev = vfio_container_dev_iter_next(container, NULL); while (vbasedev) { if (vbasedev->migration_blocker) { return false; } vbasedev = vfio_container_dev_iter_next(container, vbasedev); } > } > } > } > @@ -133,14 +158,19 @@ static Error *multiple_devices_migration_blocker; > > static unsigned int vfio_migratable_device_num(void) > { > - VFIOGroup *group; > + VFIOAddressSpace *space; > + VFIOContainer *container; > VFIODevice *vbasedev; > unsigned int device_num = 0; > > - QLIST_FOREACH(group, &vfio_group_list, next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (vbasedev->migration) { > - device_num++; > + QLIST_FOREACH(space, &vfio_address_spaces, list) { > + QLIST_FOREACH(container, &space->containers, next) { > + vbasedev = NULL; > + while ((vbasedev = vfio_container_dev_iter_next(container, > + vbasedev))) { > + if (vbasedev->migration) { > + device_num++; > + } Same as above. > } > } > } > @@ -207,8 +237,7 @@ static void vfio_set_migration_error(int err) > > static bool vfio_devices_all_dirty_tracking(VFIOContainer *container) > { > - VFIOGroup *group; > - VFIODevice *vbasedev; > + VFIODevice *vbasedev = NULL; > MigrationState *ms = migrate_get_current(); > > if (ms->state != MIGRATION_STATUS_ACTIVE && > @@ -216,19 +245,17 @@ static bool > vfio_devices_all_dirty_tracking(VFIOContainer *container) > return false; > } > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - VFIOMigration *migration = vbasedev->migration; > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + VFIOMigration *migration = vbasedev->migration; Similar, and all the other loops below. > > - if (!migration) { > - return false; > - } > + if (!migration) { > + return false; > + } > > - if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF && > - (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > - migration->device_state == VFIO_DEVICE_STATE_PRE_COPY)) { > - return false; > - } > + if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF && > + (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > + migration->device_state == VFIO_DEVICE_STATE_PRE_COPY)) { > + return false; > } > } > return true; > @@ -236,14 +263,11 @@ static bool > vfio_devices_all_dirty_tracking(VFIOContainer *container) > > static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container) > { > - VFIOGroup *group; > - VFIODevice *vbasedev; > + VFIODevice *vbasedev = NULL; > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (!vbasedev->dirty_pages_supported) { > - return false; > - } > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + if (!vbasedev->dirty_pages_supported) { > + return false; > } > } > > @@ -256,27 +280,24 @@ static bool > vfio_devices_all_device_dirty_tracking(VFIOContainer *container) > */ > static bool vfio_devices_all_running_and_mig_active(VFIOContainer *container) > { > - VFIOGroup *group; > - VFIODevice *vbasedev; > + VFIODevice *vbasedev = NULL; > > if (!migration_is_active(migrate_get_current())) { > return false; > } > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - VFIOMigration *migration = vbasedev->migration; > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + VFIOMigration *migration = vbasedev->migration; > > - if (!migration) { > - return false; > - } > + if (!migration) { > + return false; > + } > > - if (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > - migration->device_state == VFIO_DEVICE_STATE_PRE_COPY) { > - continue; > - } else { > - return false; > - } > + if (migration->device_state == VFIO_DEVICE_STATE_RUNNING || > + migration->device_state == VFIO_DEVICE_STATE_PRE_COPY) { > + continue; > + } else { > + return false; > } > } > return true; > @@ -1243,25 +1264,22 @@ static void > vfio_devices_dma_logging_stop(VFIOContainer *container) > uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature), > sizeof(uint64_t))] = {}; > struct vfio_device_feature *feature = (struct vfio_device_feature *)buf; > - VFIODevice *vbasedev; > - VFIOGroup *group; > + VFIODevice *vbasedev = NULL; > > feature->argsz = sizeof(buf); > feature->flags = VFIO_DEVICE_FEATURE_SET | > VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP; > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (!vbasedev->dirty_tracking) { > - continue; > - } > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + if (!vbasedev->dirty_tracking) { > + continue; > + } > > - if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { > - warn_report("%s: Failed to stop DMA logging, err %d (%s)", > - vbasedev->name, -errno, strerror(errno)); > - } > - vbasedev->dirty_tracking = false; > + if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) { > + warn_report("%s: Failed to stop DMA logging, err %d (%s)", > + vbasedev->name, -errno, strerror(errno)); > } > + vbasedev->dirty_tracking = false; > } > } > > @@ -1336,8 +1354,7 @@ static int vfio_devices_dma_logging_start(VFIOContainer > *container) > { > struct vfio_device_feature *feature; > VFIODirtyRanges ranges; > - VFIODevice *vbasedev; > - VFIOGroup *group; > + VFIODevice *vbasedev = NULL; > int ret = 0; > > vfio_dirty_tracking_init(container, &ranges); > @@ -1347,21 +1364,19 @@ static int > vfio_devices_dma_logging_start(VFIOContainer *container) > return -errno; > } > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (vbasedev->dirty_tracking) { > - continue; > - } > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + if (vbasedev->dirty_tracking) { > + continue; > + } > > - ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature); > - if (ret) { > - ret = -errno; > - error_report("%s: Failed to start DMA logging, err %d (%s)", > - vbasedev->name, ret, strerror(errno)); > - goto out; > - } > - vbasedev->dirty_tracking = true; > + ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature); > + if (ret) { > + ret = -errno; > + error_report("%s: Failed to start DMA logging, err %d (%s)", > + vbasedev->name, ret, strerror(errno)); > + goto out; > } > + vbasedev->dirty_tracking = true; > } > > out: > @@ -1440,22 +1455,19 @@ static int > vfio_devices_query_dirty_bitmap(VFIOContainer *container, > VFIOBitmap *vbmap, hwaddr iova, > hwaddr size) > { > - VFIODevice *vbasedev; > - VFIOGroup *group; > + VFIODevice *vbasedev = NULL; > int ret; > > - QLIST_FOREACH(group, &container->group_list, container_next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - ret = vfio_device_dma_logging_report(vbasedev, iova, size, > - vbmap->bitmap); > - if (ret) { > - error_report("%s: Failed to get DMA logging report, iova: " > - "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx > - ", err: %d (%s)", > - vbasedev->name, iova, size, ret, > strerror(-ret)); > + while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev))) { > + ret = vfio_device_dma_logging_report(vbasedev, iova, size, > + vbmap->bitmap); > + if (ret) { > + error_report("%s: Failed to get DMA logging report, iova: " > + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx > + ", err: %d (%s)", > + vbasedev->name, iova, size, ret, strerror(-ret)); > > - return ret; > - } > + return ret; > } > } > > @@ -1739,21 +1751,30 @@ bool vfio_get_info_dma_avail(struct > vfio_iommu_type1_info *info, > > void vfio_reset_handler(void *opaque) > { > - VFIOGroup *group; > + VFIOAddressSpace *space; > + VFIOContainer *container; > VFIODevice *vbasedev; > > - QLIST_FOREACH(group, &vfio_group_list, next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (vbasedev->dev->realized) { > - vbasedev->ops->vfio_compute_needs_reset(vbasedev); > + QLIST_FOREACH(space, &vfio_address_spaces, list) { > + QLIST_FOREACH(container, &space->containers, next) { > + vbasedev = NULL; > + while ((vbasedev = vfio_container_dev_iter_next(container, > + vbasedev))) { > + if (vbasedev->dev->realized) { > + vbasedev->ops->vfio_compute_needs_reset(vbasedev); > + } > } > } > } > > - QLIST_FOREACH(group, &vfio_group_list, next) { > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (vbasedev->dev->realized && vbasedev->needs_reset) { > - vbasedev->ops->vfio_hot_reset_multi(vbasedev); > + QLIST_FOREACH(space, &vfio_address_spaces, list) { > + QLIST_FOREACH(container, &space->containers, next) { > + vbasedev = NULL; > + while ((vbasedev = vfio_container_dev_iter_next(container, > + vbasedev))) { > + if (vbasedev->dev->realized && vbasedev->needs_reset) { > + vbasedev->ops->vfio_hot_reset_multi(vbasedev); > + } > } > } > } > @@ -1841,6 +1862,10 @@ static VFIOAddressSpace > *vfio_get_address_space(AddressSpace *as) > space->as = as; > QLIST_INIT(&space->containers); > > + if (QLIST_EMPTY(&vfio_address_spaces)) { > + qemu_register_reset(vfio_reset_handler, NULL); > + } > + We could just have a vfio_device_list to avoid iterating either containers and group or address spaces. Thanks, Alex > QLIST_INSERT_HEAD(&vfio_address_spaces, space, list); > > return space; > @@ -1852,6 +1877,9 @@ static void vfio_put_address_space(VFIOAddressSpace > *space) > QLIST_REMOVE(space, list); > g_free(space); > } > + if (QLIST_EMPTY(&vfio_address_spaces)) { > + qemu_unregister_reset(vfio_reset_handler, NULL); > + } > } > > /* > @@ -2317,10 +2345,6 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace > *as, Error **errp) > goto close_fd_exit; > } > > - if (QLIST_EMPTY(&vfio_group_list)) { > - qemu_register_reset(vfio_reset_handler, NULL); > - } > - > QLIST_INSERT_HEAD(&vfio_group_list, group, next); > > return group; > @@ -2349,10 +2373,6 @@ void vfio_put_group(VFIOGroup *group) > trace_vfio_put_group(group->fd); > close(group->fd); > g_free(group); > - > - if (QLIST_EMPTY(&vfio_group_list)) { > - qemu_unregister_reset(vfio_reset_handler, NULL); > - } > } > > struct vfio_device_info *vfio_get_device_info(int fd)
