Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator

[Eric Auger]

Hi Zhenzhong,

On 8/30/23 12:37, Zhenzhong Duan 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.
I would move the reset handler registration/unregistration changes in a
separate patch.
besides,  I don't catch what you mean by
"saying first vfio address space created instead of the first group."
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  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)
>      }
>  }
>  
I would add a comment:
iterate on all devices from all groups attached to a container
> +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;
> +        }
> +        group = curr->group->container_next.le_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))) {

Couldn't you use an extra define such as:
#define CONTAINER_FOREACH_DEV(container, vbasedev) \
vbasedev = NULL
while ((vbasedev = vfio_container_dev_iter_next(container, vbasedev)))

> +                if (vbasedev->migration_blocker) {
> +                    return false;
> +                }
>              }
>          }
>      }
> @@ -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++;
> +                }
>              }
>          }
>      }
> @@ -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;
>  
> -            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);
> +    }
> +
>      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)
Thanks

Eric