Re: [PATCH 1/2] vfio/type1: Simplify bus_type determination

[Robin Murphy <robin.murphy@arm.com>]

On 2022-06-10 01:03, Jason Gunthorpe via iommu wrote:
> On Wed, Jun 08, 2022 at 03:25:49PM +0100, Robin Murphy wrote:
>> Since IOMMU groups are mandatory for drivers to support, it stands to
>> reason that any device which has been successfully be added to a group
>> must be on a bus supported by that IOMMU driver, and therefore a domain
>> viable for any device in the group must be viable for all devices in
>> the group. This already has to be the case for the IOMMU API's internal
>> default domain, for instance. Thus even if the group contains devices
>> on different buses, that can only mean that the IOMMU driver actually
>> supports such an odd topology, and so without loss of generality we can
>> expect the bus type of any arbitrary device in a group to be suitable
>> for IOMMU API calls.
>>
>> Replace vfio_bus_type() with a trivial callback that simply returns any
>> device from which to then derive a usable bus type. This is also a step
>> towards removing the vague bus-based interfaces from the IOMMU API.
>>
>> Furthermore, scrutiny reveals a lack of protection for the bus and/or
>> device being removed while .attach_group is inspecting them; the
>> reference we hold on the iommu_group ensures that data remains valid,
>> but does not prevent the group's membership changing underfoot. Holding
>> the vfio_goup's device_lock should be sufficient to block any relevant
>> device's VFIO driver from unregistering, and thus block unbinding and
>> any further stages of removal for the duration of the attach operation.
> 
> The device_lock only protects devices that are on the device_list from
> concurrent unregistration, the device returned by
> iommu_group_for_each_dev() is not guarented to be the on the device
> list.

Sigh, you're quite right, and now I have a vague feeling that you called 
that out in the previous discussion too, so apologies for forgetting.

>> @@ -760,8 +760,11 @@ static int __vfio_container_attach_groups(struct vfio_container *container,
>>   	int ret = -ENODEV;
>>   
>>   	list_for_each_entry(group, &container->group_list, container_next) {
>> +		/* Prevent devices unregistering during attach */
>> +		mutex_lock(&group->device_lock);
>>   		ret = driver->ops->attach_group(data, group->iommu_group,
>>   						group->type);
>> +		mutex_unlock(&group->device_lock);
> 
> I still prefer the version where we pass in an arbitrary vfio_device
> from the list the group maintains:
> 
>     list_first_entry(group->device_list)
> 
> And don't call iommu_group_for_each_dev(), it is much simpler to
> reason about how it works.

Agreed, trying to figure out which are the VFIO devices from within the 
iommu_group iterator seems beyond the threshold of practicality.

Quick consensus then: does anyone have a particular preference between 
changing the .attach_group signature vs. adding a helper based on 
vfio_group_get_from_iommu() for type1 to call from within its callback? 
They seem about equal (but opposite) in terms of the simplicity vs. 
impact tradeoff to me, so I can't quite decide conclusively...

Thanks,
Robin.