diff mbox series

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

Message ID 20230830103754.36461-7-zhenzhong.duan@intel.com
State New
Headers show
Series vfio: Adopt iommufd | expand

Commit Message

Zhenzhong Duan Aug. 30, 2023, 10:37 a.m. UTC 
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 <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c | 224 ++++++++++++++++++++++++++---------------------
 1 file changed, 122 insertions(+), 102 deletions(-)

Comments

Eric Auger Sept. 20, 2023, 12:25 p.m. UTC | #1 
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
Alex Williamson Sept. 20, 2023, 10:16 p.m. UTC | #2 
On Wed, 30 Aug 2023 18:37:38 +0800
Zhenzhong Duan <zhenzhong.duan@intel.com> 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 <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)
>      }
>  }
>  
> +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)
Zhenzhong Duan Sept. 21, 2023, 2:16 a.m. UTC | #3 
>-----Original Message-----
>From: Alex Williamson <alex.williamson@redhat.com>
>Subject: Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator
>
>On Wed, 30 Aug 2023 18:37:38 +0800
>Zhenzhong Duan <zhenzhong.duan@intel.com> 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 <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)
>>      }
>>  }
>>
>> +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,

Good idea! Will do.
A vfio_device_list can be used by both BEs and I can have this
patch dropped.

Thanks
Zhenzhong
Zhenzhong Duan Sept. 21, 2023, 2:27 a.m. UTC | #4 
Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Wednesday, September 20, 2023 8:26 PM
>Subject: Re: [PATCH v1 06/22] vfio/common: Add a vfio device iterator
>
>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.

Got it.

>besides,  I don't catch what you mean by
>"saying first vfio address space created instead of the first group."

Before patch, reset hander is registered in first group creation,
after patch, it's registered in first address space creation.
The main purpose is to make this code group agnostic.

For the device iteration part of this patch, I plan to follow Alex's
suggestion to use vfio_device_list for both BEs. Thanks for your
time on this patch.

BRs.
Zhenzhong
diff mbox series

Patch

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;
+        }
+        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))) {
+                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)