Messages in this thread | | | Date | Fri, 24 Dec 2021 14:44:24 +0800 | From | Lu Baolu <> | Subject | Re: [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for multi-device groups |
| |
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
| |