Re: [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for multi-device groups

[Lu Baolu]

Hi Jason,

On 12/23/21 10:03 PM, Jason Gunthorpe wrote:
> > > I think it would be clear why iommu_group_set_dma_owner(), which
> > > actually does detatch, is not the same thing as iommu_attach_device().
> > iommu_device_set_dma_owner() will eventually call
> > iommu_group_set_dma_owner(). I didn't get why
> > iommu_group_set_dma_owner() is special and need to keep.
> Not quite, they would not call each other, they have different
> implementations:
>
> int iommu_device_use_dma_api(struct device *device)
> {
> 	struct iommu_group *group = device->iommu_group;
>
> 	if (!group)
> 		return 0;
>
> 	mutex_lock(&group->mutex);
> 	if (group->owner_cnt != 0 ||
> 	    group->domain != group->default_domain) {
> 		mutex_unlock(&group->mutex);
> 		return -EBUSY;
> 	}
> 	group->owner_cnt = 1;
> 	group->owner = NULL;
> 	mutex_unlock(&group->mutex);
> 	return 0;
> }

It seems that this function doesn't work for multi-device groups. When
the user unbinds all native drivers from devices in the group and start
to bind them with vfio-pci and assign them to user, how could iommu know
whether the group is viable for user?

> int iommu_group_set_dma_owner(struct iommu_group *group, struct file *owner)
> {
> 	mutex_lock(&group->mutex);
> 	if (group->owner_cnt != 0) {
> 		if (group->owner != owner)
> 			goto err_unlock;
> 		group->owner_cnt++;
> 		mutex_unlock(&group->mutex);
> 		return 0;
> 	}
> 	if (group->domain && group->domain != group->default_domain)
> 		goto err_unlock;
>
> 	__iommu_detach_group(group->domain, group);
> 	group->owner_cnt = 1;
> 	group->owner = owner;
> 	mutex_unlock(&group->mutex);
> 	return 0;
>
> err_unlock;
> 	mutex_unlock(&group->mutex);
> 	return -EBUSY;
> }
>
> It is the same as how we ended up putting the refcounting logic
> directly into the iommu_attach_device().
>
> See, we get rid of the enum as a multiplexor parameter, each API does
> only wnat it needs, they don't call each other.

I like the idea of removing enum parameter and make the API name
specific. But I didn't get why they can't call each other even the
data in group is the same.

> We don't need _USER anymore because iommu_group_set_dma_owner() always
> does detatch, and iommu_replace_group_domain() avoids ever reassigning
> default_domain. The sepecial USER behavior falls out automatically.

This means we will grow more group-centric interfaces. My understanding
is the opposite that we should hide the concept of group in IOMMU
subsystem, and the device drivers only faces device specific interfaces.

The iommu groups are created by the iommu subsystem. The device drivers
don't play any role in determining which device belongs to which group.
So the iommu interfaces for device driver shouldn't rely on the group.

Best regards,
baolu