On 6/12/24 9:05 PM, Jason Gunthorpe wrote:
On Fri, Jun 07, 2024 at 09:35:23AM +0000, Tian, Kevin wrote:
From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Thursday, June 6, 2024 2:07 PM
On 6/5/24 4:15 PM, Tian, Kevin wrote:
From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Monday, May 27, 2024 12:05 PM
- 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.
Yes, it looks like iommu_sva_bind_device() should fail with EBUSY if
the PASID is already in use and is not exactly the same SVA as being
asked for here.
It will eventually do this naturally when iommu_attach_device_pasid()
is called with an in-use PASID, but may as well do it here for
clarity.
Also, is there a missing test for the same mm too?
I'd maybe change iommu_attach_handle() to return NULL if there is no
handle and then write it like:
if (IS_ERR(attach_handle) && PTR_ERR(attach_handle) != -ENOENT) {
ret = PTR_ERR(attach_handle);
goto out_unlock;
}
if (!IS_ERR(attach_handle) && attach_handle->domain->mm == mm) {
/* Can re-use the existing SVA attachment */
}
Okay, I will change it like below:
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -91,11 +91,20 @@ struct iommu_sva *iommu_sva_bind_device(struct
device *dev, struct mm_struct *mm
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);
+ if (attach_handle->domain->mm != mm) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
refcount_inc(&handle->users);
mutex_unlock(&iommu_sva_lock);
return handle;
}
+ if (PTR_ERR(attach_handle) != -ENOENT) {
+ ret = PTR_ERR(attach_handle);
+ goto out_unlock;
+ }
+
handle = kzalloc(sizeof(*handle), GFP_KERNEL);
if (!handle) {
ret = -ENOMEM;
Does it suggest that having the caller to always provide a handle
makes more sense?
I was thinking no just to avoid memory allocation.. But how does the
caller not provide a handle? My original draft of this concept used an
XA_MARK to indicate if the xarray pointed to a handle or a domain
This seems to require the handle:
- curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
- if (curr) {
- ret = xa_err(curr) ? : -EBUSY;
+ ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
Confused.
XA_MARK was used to indicate whether the stored pointer was an iommu
domain or an attach handle. Since this series removes
iommu_get_domain_for_dev_pasid(), there is no longer any need to store
the domain pointer at all.
I'm neutral on this since only sva bind and iopf path delivery currently
require an attach handle.
let's hear Jason's opinion.
At least iommu_attach_handle_get() should not return NULL if there is
no handle, it should return EBUSY as though it couldn't match the
type.
Agreed.
Jason
Best regards,
baolu
