[PATCH 0/2] Two enhancements to iommu_attach_device_pasid()

[PATCH 0/2] Two enhancements to iommu_attach_device_pasid()

[Yi Liu]

It's spotted during the review process of iommufd pasid series [1]. The
first one is about storing entry to group->pasid_array, and the second
one is to avoid the potential PRIs anchored on the failed domain. Details
can be found in the individial patch.

[1] https://lore.kernel.org/linux-iommu/20241219132746.16193-2-yi.l.liu@intel.com/

Yi Liu (2):
  iommu: Store either domain or handle in group->pasid_array
  iommu: Swap the order of setting group->pasid_array and
    __iommu_set_group_pasid()

 drivers/iommu/iommu.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

[Yi Liu]

iommu_attach_device_pasid() only stores handle to group->pasid_array
when there is a valid handle input. However, it makes the
iommu_attach_device_pasid() unable to detect if the pasid has been
attached or not. To be complete, let the iommu_attach_device_pasid()
store the domain to group->pasid_array if no valid handle.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 28ffd836592b..278c4eb8f225 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,8 @@ static unsigned int iommu_def_domain_type __read_mostly;
 static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
 static u32 iommu_cmd_line __read_mostly;
 
+enum { IOMMU_PASID_ARRAY_DOMAIN, IOMMU_PASID_ARRAY_HANDLE };
+
 struct iommu_group {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -3374,6 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	struct iommu_group *group = dev->iommu_group;
 	struct group_device *device;
 	const struct iommu_ops *ops;
+	void *xa_entry;
 	int ret;
 
 	if (!group)
@@ -3397,10 +3400,14 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	if (handle)
+	if (handle) {
 		handle->domain = domain;
+		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
+	} else {
+		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
+	}
 
-	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
+	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
 	if (ret)
 		goto out_unlock;
 
@@ -3480,13 +3487,17 @@ struct iommu_attach_handle *
 iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int type)
 {
 	struct iommu_attach_handle *handle;
+	void *xa_entry;
 
 	xa_lock(&group->pasid_array);
-	handle = xa_load(&group->pasid_array, pasid);
-	if (!handle)
+	xa_entry = xa_load(&group->pasid_array, pasid);
+	if (xa_pointer_tag(xa_entry) == IOMMU_PASID_ARRAY_HANDLE) {
+		handle = xa_untag_pointer(xa_entry);
+		if (type && handle->domain->type != type)
+			handle = ERR_PTR(-EBUSY);
+	} else {
 		handle = ERR_PTR(-ENOENT);
-	else if (type && handle->domain->type != type)
-		handle = ERR_PTR(-EBUSY);
+	}
 	xa_unlock(&group->pasid_array);
 
 	return handle;
-- 
2.34.1

RE: [PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

[Tian, Kevin]

> From: Yi Liu <yi.l.liu@intel.com>
> Sent: Monday, January 20, 2025 11:09 AM
>
> iommu_attach_device_pasid() only stores handle to group->pasid_array
> when there is a valid handle input. However, it makes the
> iommu_attach_device_pasid() unable to detect if the pasid has been
> attached or not. To be complete, let the iommu_attach_device_pasid()
> store the domain to group->pasid_array if no valid handle.
>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

[Jason Gunthorpe]

On Sun, Jan 19, 2025 at 07:08:39PM -0800, Yi Liu wrote:
> iommu_attach_device_pasid() only stores handle to group->pasid_array
> when there is a valid handle input. However, it makes the
> iommu_attach_device_pasid() unable to detect if the pasid has been
> attached or not. To be complete, let the iommu_attach_device_pasid()
> store the domain to group->pasid_array if no valid handle.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 28ffd836592b..278c4eb8f225 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -45,6 +45,8 @@ static unsigned int iommu_def_domain_type __read_mostly;
>  static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
>  static u32 iommu_cmd_line __read_mostly;
>  
> +enum { IOMMU_PASID_ARRAY_DOMAIN, IOMMU_PASID_ARRAY_HANDLE };

Since xarray says:

 * @tag: Tag value (0, 1 or 3).

I would initialize those enums and add a small comment

/* tags used with xa_tag_pointer() */
enum { IOMMU_PASID_ARRAY_DOMAIN = 0, IOMMU_PASID_ARRAY_HANDLE = 1 };

> @@ -3397,10 +3400,14 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  		}
>  	}
>  
> -	if (handle)
> +	if (handle) {
>  		handle->domain = domain;
> +		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
> +	} else {
> +		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
> +	}

What about iommu_attach_group_handle() ?

	mutex_lock(&group->mutex);
	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
	if (ret)

And iommu_replace_group_handle() ?

	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);

I think it should be consistent and it should call xa_tag_poiner() too
(even though it implicitly always is since the domain is stored in the
group)??

Jason

Re: [PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

[Baolu Lu]

On 1/20/25 23:55, Jason Gunthorpe wrote:
>> @@ -3397,10 +3400,14 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		}
>>   	}
>>   
>> -	if (handle)
>> +	if (handle) {
>>   		handle->domain = domain;
>> +		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
>> +	} else {
>> +		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>> +	}
> What about iommu_attach_group_handle() ?
> 
> 	mutex_lock(&group->mutex);
> 	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 	if (ret)
> 
> And iommu_replace_group_handle() ?
> 
> 	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 
> I think it should be consistent and it should call xa_tag_poiner() too
> (even though it implicitly always is since the domain is stored in the
> group)??

We may eventually consider retiring group->domain.

---
baolu

Re: [PATCH 1/2] iommu: Store either domain or handle in group->pasid_array

[Yi Liu]

On 2025/1/20 23:55, Jason Gunthorpe wrote:
> On Sun, Jan 19, 2025 at 07:08:39PM -0800, Yi Liu wrote:
>> iommu_attach_device_pasid() only stores handle to group->pasid_array
>> when there is a valid handle input. However, it makes the
>> iommu_attach_device_pasid() unable to detect if the pasid has been
>> attached or not. To be complete, let the iommu_attach_device_pasid()
>> store the domain to group->pasid_array if no valid handle.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 23 +++++++++++++++++------
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 28ffd836592b..278c4eb8f225 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -45,6 +45,8 @@ static unsigned int iommu_def_domain_type __read_mostly;
>>   static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
>>   static u32 iommu_cmd_line __read_mostly;
>>   
>> +enum { IOMMU_PASID_ARRAY_DOMAIN, IOMMU_PASID_ARRAY_HANDLE };
> 
> Since xarray says:
> 
>   * @tag: Tag value (0, 1 or 3).
> 
> I would initialize those enums and add a small comment

yes.

> /* tags used with xa_tag_pointer() */
> enum { IOMMU_PASID_ARRAY_DOMAIN = 0, IOMMU_PASID_ARRAY_HANDLE = 1 };
> 
>> @@ -3397,10 +3400,14 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		}
>>   	}
>>   
>> -	if (handle)
>> +	if (handle) {
>>   		handle->domain = domain;
>> +		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
>> +	} else {
>> +		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>> +	}
> 
> What about iommu_attach_group_handle() ?
> 
> 	mutex_lock(&group->mutex);
> 	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 	if (ret)
> 
> And iommu_replace_group_handle() ?
> 
> 	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> 
> I think it should be consistent and it should call xa_tag_poiner() too
> (even though it implicitly always is since the domain is stored in the
> group)??

yes, it needs to be fixed as well. I asked a help from Baolu in prior
email, but I can enlarge this series to cover both RID and PASID path. :)

-- 
Regards,
Yi Liu

[PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

[Yi Liu]

Following the current order of setting group->pasid_array and
__iommu_set_group_pasid() in iommu_attach_device_pasid(), PRIs may be
forwarded to the domain before the attach succeeds. If the attach failed
in the end, the PRIs on the domain need to be flushed in the caller side.
Caller can do it, but it can be avoided by swapping the order. This is
more self-contained.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 278c4eb8f225..b69bcf559839 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3376,7 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	struct iommu_group *group = dev->iommu_group;
 	struct group_device *device;
 	const struct iommu_ops *ops;
-	void *xa_entry;
+	void *curr, *xa_entry;
 	int ret;
 
 	if (!group)
@@ -3400,6 +3400,16 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		}
 	}
 
+	curr = xa_load(&group->pasid_array, pasid);
+	if (curr) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	ret = __iommu_set_group_pasid(domain, group, pasid);
+	if (ret)
+		goto out_unlock;
+
 	if (handle) {
 		handle->domain = domain;
 		xa_entry = xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
@@ -3407,13 +3417,12 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
 	}
 
-	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
-	if (ret)
-		goto out_unlock;
+	curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
+	if (curr) {
+		__iommu_remove_group_pasid(group, pasid, domain);
+		ret = xa_err(curr);
+	}
 
-	ret = __iommu_set_group_pasid(domain, group, pasid);
-	if (ret)
-		xa_erase(&group->pasid_array, pasid);
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.34.1

RE: [PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

[Tian, Kevin]

> From: Yi Liu <yi.l.liu@intel.com>
> Sent: Monday, January 20, 2025 11:09 AM
>
> -     ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
> -     if (ret)
> -             goto out_unlock;
> +     curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
> +     if (curr) {
> +             __iommu_remove_group_pasid(group, pasid, domain);
> +             ret = xa_err(curr);
> +     }
>

s/handle/xa_entry/

btw you may continue to use xa_insert() to save one line for xa_err()

Re: [PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

[Yi Liu]

On 2025/1/20 13:58, Tian, Kevin wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>> Sent: Monday, January 20, 2025 11:09 AM
>>
>> -	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
>> -	if (ret)
>> -		goto out_unlock;
>> +	curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
>> +	if (curr) {
>> +		__iommu_remove_group_pasid(group, pasid, domain);
>> +		ret = xa_err(curr);
>> +	}
>>
> 
> s/handle/xa_entry/

oops. will fix it.

> btw you may continue to use xa_insert() to save one line for xa_err()

got it.

-- 
Regards,
Yi Liu

Re: [PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

[Jason Gunthorpe]

On Sun, Jan 19, 2025 at 07:08:40PM -0800, Yi Liu wrote:
> Following the current order of setting group->pasid_array and
> __iommu_set_group_pasid() in iommu_attach_device_pasid(), PRIs may be
> forwarded to the domain before the attach succeeds. If the attach failed
> in the end, the PRIs on the domain need to be flushed in the caller side.
> Caller can do it, but it can be avoided by swapping the order. This is
> more self-contained.

Can the caller do it? I thought only the driver could manage it?

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 278c4eb8f225..b69bcf559839 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3376,7 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  	struct iommu_group *group = dev->iommu_group;
>  	struct group_device *device;
>  	const struct iommu_ops *ops;
> -	void *xa_entry;
> +	void *curr, *xa_entry;
>  	int ret;
>  
>  	if (!group)
> @@ -3400,6 +3400,16 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  		}
>  	}
>  
> +	curr = xa_load(&group->pasid_array, pasid);
> +	if (curr) {

No need for curr

if (xa_load()) {

> @@ -3407,13 +3417,12 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>  	}
>  
> -	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
> -	if (ret)
> -		goto out_unlock;
> +	curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
> +	if (curr) {
> +		__iommu_remove_group_pasid(group, pasid, domain);
> +		ret = xa_err(curr);

curr could be a valid non-error entry, but that
would be a WARN_ON condition due to the above check. Like this:

      ret = xa_insert(&group->pasid_array, pasid, xa_entry,GFP_KERNEL);
      if (ret) {
		WARN_ON(ret == -EBUSY);
		goto out_remove;
      }

Jason

Re: [PATCH 2/2] iommu: Swap the order of setting group->pasid_array and __iommu_set_group_pasid()

[Yi Liu]

On 2025/1/21 00:13, Jason Gunthorpe wrote:
> On Sun, Jan 19, 2025 at 07:08:40PM -0800, Yi Liu wrote:
>> Following the current order of setting group->pasid_array and
>> __iommu_set_group_pasid() in iommu_attach_device_pasid(), PRIs may be
>> forwarded to the domain before the attach succeeds. If the attach failed
>> in the end, the PRIs on the domain need to be flushed in the caller side.
>> Caller can do it, but it can be avoided by swapping the order. This is
>> more self-contained.
> 
> Can the caller do it? I thought only the driver could manage it?

I think yes. Take iommufd as example, if the hwpt is fault-able, the PRIs
would be forwarded to iommufd, and recorded in hwpt->fault->deliver list.
This cannot be done in driver alone, iommufd needs to flush it. Otherwise,
the PRIs would be kept until the hwpt is destroyed.

>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 278c4eb8f225..b69bcf559839 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3376,7 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   	struct iommu_group *group = dev->iommu_group;
>>   	struct group_device *device;
>>   	const struct iommu_ops *ops;
>> -	void *xa_entry;
>> +	void *curr, *xa_entry;
>>   	int ret;
>>   
>>   	if (!group)
>> @@ -3400,6 +3400,16 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		}
>>   	}
>>   
>> +	curr = xa_load(&group->pasid_array, pasid);
>> +	if (curr) {
> 
> No need for curr
> 
> if (xa_load()) {

got it.

>> @@ -3407,13 +3417,12 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   		xa_entry = xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
>>   	}
>>   
>> -	ret = xa_insert(&group->pasid_array, pasid, xa_entry, GFP_KERNEL);
>> -	if (ret)
>> -		goto out_unlock;
>> +	curr = xa_store(&group->pasid_array, pasid, handle, GFP_KERNEL);
>> +	if (curr) {
>> +		__iommu_remove_group_pasid(group, pasid, domain);
>> +		ret = xa_err(curr);
> 
> curr could be a valid non-error entry, but that
> would be a WARN_ON condition due to the above check. Like this:
> 
>        ret = xa_insert(&group->pasid_array, pasid, xa_entry,GFP_KERNEL);
>        if (ret) {
> 		WARN_ON(ret == -EBUSY);
> 		goto out_remove;
>        }

yes. it should be a warn_on after the above check.

-- 
Regards,
Yi Liu