>-----Original Message-----
>From: Alex Williamson <alex.william...@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.d...@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.d...@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
