Re: [PATCH v2 11/12] vfio/common: Introduce two kinds of VFIO device lists

[Eric Auger]

Hi Zhenzhong,
On 9/26/23 13:32, Zhenzhong Duan wrote:
> In VFIO subsystem, there are different VFIO device iteration requirements.
> One requirement is to iterate all VFIO devices, the other is to iterate
> VFIO device in same container.
>
> Currently VFIO device is iterated through VFIO group list which is group
> perceivable and less efficient.
>
> Introduce two kinds of VFIO device lists, one is a global list, the other
> is per container list. With the two lists added, we can make some migration
> and reset related functions group agnostic.
>
> For example, vfio_device_list is used in below functions:
> vfio_mig_active
> vfio_reset_handler
> vfio_multiple_devices_migration_is_supported
>
> Per container list is used in below functions:
> 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
>
> This is a prerequisite for future IOMMUFD backend support which
> has same kind of iteration requirement.
>
> vfio_group_list is preserved for some functions which honor group
> iteration, those functions are all related to legacy backend.
>
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

This may be split into 3 patches
1. creation of container->device_list
2. creation of global device list
3. addition of container field in vbasedev (which is not described in
the commit msg by the way) and looks somehow unrelated to me?

Eric
> ---
>  include/hw/vfio/vfio-common.h |   5 +
>  hw/vfio/common.c              | 194 ++++++++++++++++------------------
>  2 files changed, 94 insertions(+), 105 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index c486bdef2a..54905b9dd4 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -98,6 +98,7 @@ typedef struct VFIOContainer {
>      QLIST_HEAD(, VFIOGroup) group_list;
>      QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
>      QLIST_ENTRY(VFIOContainer) next;
> +    QLIST_HEAD(, VFIODevice) device_list;
>  } VFIOContainer;
>  
>  typedef struct VFIOGuestIOMMU {
> @@ -129,7 +130,10 @@ typedef struct VFIODeviceOps VFIODeviceOps;
>  
>  typedef struct VFIODevice {
>      QLIST_ENTRY(VFIODevice) next;
> +    QLIST_ENTRY(VFIODevice) container_next;
> +    QLIST_ENTRY(VFIODevice) global_next;
>      struct VFIOGroup *group;
> +    VFIOContainer *container;
>      char *sysfsdev;
>      char *name;
>      DeviceState *dev;
> @@ -229,6 +233,7 @@ int vfio_kvm_device_del_fd(int fd, Error **errp);
>  
>  extern const MemoryRegionOps vfio_region_ops;
>  typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
> +typedef QLIST_HEAD(VFIODeviceList, VFIODevice) VFIODeviceList;
>  extern VFIOGroupList vfio_group_list;
>  
>  bool vfio_mig_active(void);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 12ebf2f11d..645e2dc39a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -48,6 +48,8 @@
>  
>  VFIOGroupList vfio_group_list =
>      QLIST_HEAD_INITIALIZER(vfio_group_list);
> +static VFIODeviceList vfio_device_list =
> +    QLIST_HEAD_INITIALIZER(vfio_device_list);
>  static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
>      QLIST_HEAD_INITIALIZER(vfio_address_spaces);
>  
> @@ -94,18 +96,15 @@ static int vfio_get_dirty_bitmap(VFIOContainer 
> *container, uint64_t iova,
>  
>  bool vfio_mig_active(void)
>  {
> -    VFIOGroup *group;
>      VFIODevice *vbasedev;
>  
> -    if (QLIST_EMPTY(&vfio_group_list)) {
> +    if (QLIST_EMPTY(&vfio_device_list)) {
>          return false;
>      }
>  
> -    QLIST_FOREACH(group, &vfio_group_list, next) {
> -        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> -            if (vbasedev->migration_blocker) {
> -                return false;
> -            }
> +    QLIST_FOREACH(vbasedev, &vfio_device_list, next) {
> +        if (vbasedev->migration_blocker) {
> +            return false;
>          }
>      }
>      return true;
> @@ -120,19 +119,16 @@ static Error *multiple_devices_migration_blocker;
>   */
>  static bool vfio_multiple_devices_migration_is_supported(void)
>  {
> -    VFIOGroup *group;
>      VFIODevice *vbasedev;
>      unsigned int device_num = 0;
>      bool all_support_p2p = true;
>  
> -    QLIST_FOREACH(group, &vfio_group_list, next) {
> -        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> -            if (vbasedev->migration) {
> -                device_num++;
> +    QLIST_FOREACH(vbasedev, &vfio_device_list, next) {
> +        if (vbasedev->migration) {
> +            device_num++;
>  
> -                if (!(vbasedev->migration->mig_flags & VFIO_MIGRATION_P2P)) {
> -                    all_support_p2p = false;
> -                }
> +            if (!(vbasedev->migration->mig_flags & VFIO_MIGRATION_P2P)) {
> +                all_support_p2p = false;
>              }
>          }
>      }
> @@ -184,7 +180,7 @@ void vfio_unblock_multiple_devices_migration(void)
>  
>  bool vfio_viommu_preset(VFIODevice *vbasedev)
>  {
> -    return vbasedev->group->container->space->as != &address_space_memory;
> +    return vbasedev->container->space->as != &address_space_memory;
>  }
>  
>  static void vfio_set_migration_error(int err)
> @@ -218,7 +214,6 @@ bool vfio_device_state_is_precopy(VFIODevice *vbasedev)
>  
>  static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>  {
> -    VFIOGroup *group;
>      VFIODevice *vbasedev;
>      MigrationState *ms = migrate_get_current();
>  
> @@ -227,19 +222,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;
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        VFIOMigration *migration = vbasedev->migration;
>  
> -            if (!migration) {
> -                return false;
> -            }
> +        if (!migration) {
> +            return false;
> +        }
>  
> -            if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
> -                (vfio_device_state_is_running(vbasedev) ||
> -                 vfio_device_state_is_precopy(vbasedev))) {
> -                return false;
> -            }
> +        if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF &&
> +            (vfio_device_state_is_running(vbasedev) ||
> +             vfio_device_state_is_precopy(vbasedev))) {
> +            return false;
>          }
>      }
>      return true;
> @@ -247,14 +240,11 @@ static bool 
> vfio_devices_all_dirty_tracking(VFIOContainer *container)
>  
>  static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
>  {
> -    VFIOGroup *group;
>      VFIODevice *vbasedev;
>  
> -    QLIST_FOREACH(group, &container->group_list, container_next) {
> -        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> -            if (!vbasedev->dirty_pages_supported) {
> -                return false;
> -            }
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        if (!vbasedev->dirty_pages_supported) {
> +            return false;
>          }
>      }
>  
> @@ -267,27 +257,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;
>  
>      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;
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        VFIOMigration *migration = vbasedev->migration;
>  
> -            if (!migration) {
> -                return false;
> -            }
> +        if (!migration) {
> +            return false;
> +        }
>  
> -            if (vfio_device_state_is_running(vbasedev) ||
> -                vfio_device_state_is_precopy(vbasedev)) {
> -                continue;
> -            } else {
> -                return false;
> -            }
> +        if (vfio_device_state_is_running(vbasedev) ||
> +            vfio_device_state_is_precopy(vbasedev)) {
> +            continue;
> +        } else {
> +            return false;
>          }
>      }
>      return true;
> @@ -1187,20 +1174,17 @@ static bool 
> vfio_section_is_vfio_pci(MemoryRegionSection *section,
>  {
>      VFIOPCIDevice *pcidev;
>      VFIODevice *vbasedev;
> -    VFIOGroup *group;
>      Object *owner;
>  
>      owner = memory_region_owner(section->mr);
>  
> -    QLIST_FOREACH(group, &container->group_list, container_next) {
> -        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> -            if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> -                continue;
> -            }
> -            pcidev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> -            if (OBJECT(pcidev) == owner) {
> -                return true;
> -            }
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> +            continue;
> +        }
> +        pcidev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +        if (OBJECT(pcidev) == owner) {
> +            return true;
>          }
>      }
>  
> @@ -1296,24 +1280,21 @@ static void 
> vfio_devices_dma_logging_stop(VFIOContainer *container)
>                                sizeof(uint64_t))] = {};
>      struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
>      VFIODevice *vbasedev;
> -    VFIOGroup *group;
>  
>      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;
> -            }
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        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;
>      }
>  }
>  
> @@ -1396,7 +1377,6 @@ static int vfio_devices_dma_logging_start(VFIOContainer 
> *container)
>      struct vfio_device_feature *feature;
>      VFIODirtyRanges ranges;
>      VFIODevice *vbasedev;
> -    VFIOGroup *group;
>      int ret = 0;
>  
>      vfio_dirty_tracking_init(container, &ranges);
> @@ -1406,21 +1386,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;
> -            }
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_next) {
> +        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:
> @@ -1500,21 +1478,18 @@ static int 
> vfio_devices_query_dirty_bitmap(VFIOContainer *container,
>                                             hwaddr size)
>  {
>      VFIODevice *vbasedev;
> -    VFIOGroup *group;
>      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));
> +    QLIST_FOREACH(vbasedev, &container->device_list, container_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));
>  
> -                return ret;
> -            }
> +            return ret;
>          }
>      }
>  
> @@ -1798,22 +1773,17 @@ bool vfio_get_info_dma_avail(struct 
> vfio_iommu_type1_info *info,
>  
>  void vfio_reset_handler(void *opaque)
>  {
> -    VFIOGroup *group;
>      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(vbasedev, &vfio_device_list, next) {
> +        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(vbasedev, &vfio_device_list, next) {
> +        if (vbasedev->dev->realized && vbasedev->needs_reset) {
> +            vbasedev->ops->vfio_hot_reset_multi(vbasedev);
>          }
>      }
>  }
> @@ -2643,6 +2613,7 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev,
>      int groupid = vfio_device_groupid(vbasedev, errp);
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
> +    VFIOContainer *container;
>      int ret;
>  
>      if (groupid < 0) {
> @@ -2666,8 +2637,14 @@ int vfio_attach_device(char *name, VFIODevice 
> *vbasedev,
>      ret = vfio_get_device(group, name, vbasedev, errp);
>      if (ret) {
>          vfio_put_group(group);
> +        return ret;
>      }
>  
> +    container = group->container;
> +    vbasedev->container = container;
> +    QLIST_INSERT_HEAD(&container->device_list, vbasedev, container_next);
> +    QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
> +
>      return ret;
>  }
>  
> @@ -2675,6 +2652,13 @@ void vfio_detach_device(VFIODevice *vbasedev)
>  {
>      VFIOGroup *group = vbasedev->group;
>  
> +    if (!vbasedev->container) {
> +        return;
> +    }
> +
> +    QLIST_REMOVE(vbasedev, global_next);
> +    QLIST_REMOVE(vbasedev, container_next);
> +    vbasedev->container = NULL;
>      vfio_put_base_device(vbasedev);
>      vfio_put_group(group);
>  }