On 6/5/24 4:15 PM, Tian, Kevin wrote:
From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> Sent: Monday, May 27, 2024 12:05 PM @@ -69,11 +68,16 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de */ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm) { + struct iommu_group *group = dev->iommu_group; + struct iommu_attach_handle *attach_handle; struct iommu_mm_data *iommu_mm; struct iommu_domain *domain; struct iommu_sva *handle;it's confusing to have both 'handle' and 'attach_handle' in one function. Clearer to rename 'handle' as 'sva'.
Yes. Could be cleaned up in a separated patch. All sva handle in iommu-sva.c should be converted if we decide to do that.
int ret; + if (!group) + return ERR_PTR(-ENODEV); + mutex_lock(&iommu_sva_lock); /* Allocate mm->pasid if necessary. */ @@ -83,12 +87,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm goto out_unlock; } - list_for_each_entry(handle, &mm->iommu_mm->sva_handles, handle_item) { - if (handle->dev == dev) { - refcount_inc(&handle->users); - mutex_unlock(&iommu_sva_lock); - return handle; - } + /* A bond already exists, just take a reference`. */ + attach_handle = iommu_attach_handle_get(group, iommu_mm-pasid, IOMMU_DOMAIN_SVA);+ if (!IS_ERR(attach_handle)) { + handle = container_of(attach_handle, struct iommu_sva, handle); + refcount_inc(&handle->users); + mutex_unlock(&iommu_sva_lock); + return handle; }It's counter-intuitive to move forward when an error is returned. e.g. if it's -EBUSY indicating the pasid already used for another type then following attempts shouldn't been tried. probably we should have iommu_attach_handle_get() return NULL instead of -ENOENT when the entry is free? then: attach_handle = iommu_attach_handle_get(); if (IS_ERR(attach_handle)) { ret = PTR_ERR(attach_handle); goto out_unlock; } else if (attach_handle) { /* matched and increase handle->users */ } /* free entry falls through */ But then there is one potential issue with the design that 'handle' can be optional in iommu_attach_device_pasid(). In that case xa_load returns NULL then we cannot differentiate a real unused PASID vs. one which has been attached w/o an handle.
The PASID should be allocated exclusively. This means that once a PASID is assigned to A, it shouldn't be assigned to B at the same time. If a single PASID is used for multiple purposes, it's likely a bug in the system. So the logic of iommu_attach_handle_get() here is: has an SVA domain already been installed for this PASID? If so, just reuse it. Otherwise, try to install a new SVA domain.
Does it suggest that having the caller to always provide a handle makes more sense?
I'm neutral on this since only sva bind and iopf path delivery currently require an attach handle. Best regards, baolu
