Re: [PATCH v3 14/15] vfio/common: Introduce a global VFIODevice list

[Eric Auger]

Hi Cédric,

On 10/3/23 17:56, Cédric Le Goater wrote:
> On 10/3/23 12:14, Eric Auger wrote:
>> From: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>
>> Some functions iterate over all the VFIODevices. This is currently
>> achieved by iterating over all groups/devices. Let's
>> introduce a global list of VFIODevices simplifying that scan.
>
> Maybe we should move the qemu_register_reset() when the first device
> is added to the list, in vfio_attach_device() ?

Well at the moment this is done on the first address space addition to
vfio_address_spaces. I think it is quite similar and I would be tempted
to leave it there atm except if you or anybody has a strong opinion here.

Eric
>
> Thanks,
>
> C.
>
>>
>> This will also be useful while migrating to IOMMUFD by hiding the
>> group specificity.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>   include/hw/vfio/vfio-common.h |  2 ++
>>   hw/vfio/common.c              | 45 +++++++++++++++--------------------
>>   2 files changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h
>> b/include/hw/vfio/vfio-common.h
>> index bf12e40667..54905b9dd4 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -131,6 +131,7 @@ 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;
>> @@ -232,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 55f8a113ea..95bc50bcda 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;
>>               }
>>           }
>>       }
>> @@ -1777,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);
>>           }
>>       }
>>   }
>> @@ -2657,6 +2648,7 @@ int vfio_attach_device(char *name, VFIODevice
>> *vbasedev,
>>       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;
>>   }
>> @@ -2669,6 +2661,7 @@ void vfio_detach_device(VFIODevice *vbasedev)
>>           return;
>>       }
>>   +    QLIST_REMOVE(vbasedev, global_next);
>>       QLIST_REMOVE(vbasedev, container_next);
>>       vbasedev->container = NULL;
>>       trace_vfio_detach_device(vbasedev->name, group->groupid);
>