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
