Messages in this thread | | | Date | Tue, 23 Aug 2022 18:12:44 +0800 | Subject | Re: [PATCH v11 09/13] iommu/sva: Refactoring iommu_sva_bind/unbind_device() | From | Baolu Lu <> |
| |
On 2022/8/18 21:41, Jason Gunthorpe wrote: > On Wed, Aug 17, 2022 at 09:20:20AM +0800, Lu Baolu wrote: >> + >> +/** >> + * iommu_sva_bind_device() - Bind a process address space to a device >> + * @dev: the device >> + * @mm: the mm to bind, caller must hold a reference to mm_users >> + * >> + * Create a bond between device and address space, allowing the device to access >> + * the mm using the returned PASID. If a bond already exists between @device and >> + * @mm, it is returned and an additional reference is taken. Caller must call >> + * iommu_sva_unbind_device() to release each reference. >> + * >> + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to >> + * initialize the required SVA features. >> + * >> + * On error, returns an ERR_PTR value. >> + */ >> +struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm) >> +{ >> + struct iommu_domain *domain; >> + struct iommu_sva *bond; > > This is called handle below, pick one name please
Updated.
> >> + ioasid_t max_pasids; >> + int ret; >> + >> + max_pasids = dev->iommu->max_pasids; >> + if (!max_pasids) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + /* Allocate mm->pasid if necessary. */ >> + ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + bond = kzalloc(sizeof(*bond), GFP_KERNEL); >> + if (!bond) >> + return ERR_PTR(-ENOMEM); >> + >> + mutex_lock(&iommu_sva_lock); >> + /* Search for an existing domain. */ >> + domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid); >> + if (domain) { > > This isn't safe, or sane. A driver could have attached something to > this PASID that is not a SVA domain and thus not protected by the > iommu_sva_lock. > > At a minimum you should add a type match to > iommu_get_domain_for_dev_pasid(), eg to confirm it is a SVA domain and > do that check under the xa_lock of the pasid xarray. > > And then the general idea is that SVA domain attach/detach must hold > this janky global lock.
Make sense. I will add this logic.
> >> + refcount_inc(&domain->users); > > This atomic is always processed under the iommu_sva_lock, so it > doesn't need to be an atomic anymore.
Will change it to an integer.
> > Otherwise this design looks OK to me too
Thank you very much for your suggestions.
Best regards, baolu
| |