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

[Lu Baolu]

Hi Jason,

On 2021/12/24 10:50, Jason Gunthorpe wrote:
> On Fri, Dec 24, 2021 at 09:30:17AM +0800, Lu Baolu wrote:
> > 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?
> It is just a mistake, I made this very fast. It should work as your
> patch had it with a ++. More like this:
>
> 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) {
> 		if (group->domain != group->default_domain ||
> 		    group->owner != NULL) {
> 			mutex_unlock(&group->mutex);
> 			return -EBUSY;
> 		}
> 	}
> 	group->owner_cnt++;
> 	mutex_unlock(&group->mutex);
> 	return 0;
> }
>
> > > 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.
> Well, I think when you type them out you'll find they don't work the
> same. Ie the iommu_group_set_dma_owner() does __iommu_detach_group()
> which iommu_device_use_dma_api() definately doesn't want to
> do. iommu_device_use_dma_api() checks the domain while
> iommu_group_set_dma_owner() must not.
>
> This is basically the issue, all the places touching ownercount are
> superficially the same but each use different predicates. Given the
> predicate is more than half the code I wouldn't try to share the rest
> of it. But maybe when it is all typed in something will become
> obvious?

Get you and agree with you. For the remaining comments, let me wait and
listen what Robin will comment.

Best regards,
baolu