The struct sva_iommu represents an association between an SVA domain and
a PASID of a device. It's stored in the iommu group's pasid array and also
tracked by a list in the per-mm data structure. Removes duplicate tracking
of sva_iommu by eliminating the list.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  2 --
 drivers/iommu/iommu-priv.h |  3 +++
 drivers/iommu/iommu-sva.c  | 21 ++++++++++++---------
 drivers/iommu/iommu.c      | 31 +++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b89404acacd6..823fa3bcc2c6 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1005,14 +1005,12 @@ struct iommu_attach_handle {
 struct iommu_sva {
 	struct iommu_attach_handle	handle;
 	struct device			*dev;
-	struct list_head		handle_item;
 	refcount_t			users;
 };
 
 struct iommu_mm_data {
 	u32			pasid;
 	struct list_head	sva_domains;
-	struct list_head	sva_handles;
 };
 
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 5f731d994803..f1536a5ebb0d 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -28,4 +28,7 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
 				 const struct bus_type *bus,
 				 struct notifier_block *nb);
 
+struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
+						    ioasid_t pasid,
+						    unsigned int type);
 #endif /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 0fb923254062..e85f4ccc9dcc 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -41,7 +41,6 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
 	}
 	iommu_mm->pasid = pasid;
 	INIT_LIST_HEAD(&iommu_mm->sva_domains);
-	INIT_LIST_HEAD(&iommu_mm->sva_handles);
 	/*
 	 * Make sure the write to mm->iommu_mm is not reordered in front of
 	 * initialization to iommu_mm fields. If it does, readers may see a
@@ -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;
 	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;
 	}
 
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
@@ -125,7 +130,6 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 
 out:
 	refcount_set(&handle->users, 1);
-	list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
 	mutex_unlock(&iommu_sva_lock);
 	handle->dev = dev;
 	return handle;
@@ -159,7 +163,6 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 		mutex_unlock(&iommu_sva_lock);
 		return;
 	}
-	list_del(&handle->handle_item);
 
 	iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
 	if (--domain->users == 0) {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index efd281ddfa63..0263814cba6b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3483,3 +3483,34 @@ void iommu_free_global_pasid(ioasid_t pasid)
 	ida_free(&iommu_global_pasid_ida, pasid);
 }
 EXPORT_SYMBOL_GPL(iommu_free_global_pasid);
+
+/**
+ * iommu_attach_handle_get - Return the attach handle
+ * @group: the iommu group that domain was attached to
+ * @pasid: the pasid within the group
+ * @type: matched domain type, 0 for any match
+ *
+ * Return handle or ERR_PTR(-ENOENT) on none, ERR_PTR(-EBUSY) on mismatch.
+ *
+ * Return the attach handle to the caller. The life cycle of an iommu attach
+ * handle is from the time when the domain is attached to the time when the
+ * domain is detached. Callers are required to synchronize the call of
+ * iommu_attach_handle_get() with domain attachment and detachment. The attach
+ * handle can only be used during its life cycle.
+ */
+struct iommu_attach_handle *
+iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int type)
+{
+	struct iommu_attach_handle *handle;
+
+	xa_lock(&group->pasid_array);
+	handle = xa_load(&group->pasid_array, pasid);
+	if (!handle)
+		handle = ERR_PTR(-ENOENT);
+	else if (type && handle->domain->type != type)
+		handle = ERR_PTR(-EBUSY);
+	xa_unlock(&group->pasid_array);
+
+	return handle;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_attach_handle_get, IOMMUFD_INTERNAL);
-- 
2.34.1

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 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'.

>  	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.

Does it suggest that having the caller to always provide a handle
makes more sense?

On 6/5/24 4:15 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> 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

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, June 6, 2024 2:07 PM
> 
> On 6/5/24 4:15 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> 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.
> >
> > 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.

yes there is a bug but catching it here would make diagnostic easier.

> 
> 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.
> 

let's hear Jason's opinion.

On Fri, Jun 07, 2024 at 09:35:23AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Thursday, June 6, 2024 2:07 PM
> > 
> > On 6/5/24 4:15 PM, Tian, Kevin wrote:
> > >> From: Lu Baolu <baolu.lu@linux.intel.com>
> > >> 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 */
}

> > > 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.

> > 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.

Jason

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@linux.intel.com>
>>> Sent: Thursday, June 6, 2024 2:07 PM
>>>
>>> On 6/5/24 4:15 PM, Tian, Kevin wrote:
>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>> 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