[PATCH 0/5] Misc iommu_attach_handle enhancements in iommu core

[PATCH 0/5] Misc iommu_attach_handle enhancements in iommu core

[Yi Liu]

This patch series aims to enhance the iommu_attach_handle-related APIs
in the IOMMU core. The changes build upon Nic's previous series[1], which
ensured that users of the attach/replace handle APIs always provide a
valid handle. With this foundation, the series refactors the IOMMU core
to share code between the normal attach/replace APIs and the
handle-supporting APIs. Additionally, it seeks to avoid processing Page
Request Interrupts (PRIs) before the attach operation succeeds. Ultimately,
the group->domain becomes redundant and is retired.

This series also covers the prior series[2] which only takes care of the
iommu_attach_device_pasid().

[1] https://lore.kernel.org/linux-iommu/cover.1738645017.git.nicolinc@nvidia.com/
[2] https://lore.kernel.org/linux-iommu/20250120030840.4171-1-yi.l.liu@intel.com/

Regards,
	Yi Liu

Yi Liu (5):
  iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()
  iommu: Drop iommu_group_replace_domain()
  iommu: Store either domain or handle in group->pasid_array
  iommu: Swap the order of setting group->pasid_array and calling attach
    op of iommu drivers
  iommu: Retire group->domain

 drivers/iommu/dma-iommu.c  |   3 +-
 drivers/iommu/iommu-priv.h |   1 +
 drivers/iommu/iommu.c      | 254 +++++++++++++++++++++----------------
 3 files changed, 151 insertions(+), 107 deletions(-)

-- 
2.34.1

[PATCH 1/5] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Yi Liu]

Caller of the two APIs always provide a valid handle, make @handle as
mandatory parameter.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 870c3cdbd0f6..eef7e19c547d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3511,8 +3511,10 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (handle)
-		handle->domain = domain;
+	if (!handle)
+		return -EINVAL;
+
+	handle->domain = domain;
 
 	mutex_lock(&group->mutex);
 	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
@@ -3568,16 +3570,15 @@ int iommu_replace_group_handle(struct iommu_group *group,
 	void *curr;
 	int ret;
 
-	if (!new_domain)
+	if (!new_domain || !handle)
 		return -EINVAL;
 
+	handle->domain = new_domain;
+
 	mutex_lock(&group->mutex);
-	if (handle) {
-		ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
-		if (ret)
-			goto err_unlock;
-		handle->domain = new_domain;
-	}
+	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
+	if (ret)
+		goto err_unlock;
 
 	ret = __iommu_group_set_domain(group, new_domain);
 	if (ret)
-- 
2.34.1

Re: [PATCH 1/5] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:36PM -0800, Yi Liu wrote:
> Caller of the two APIs always provide a valid handle, make @handle as
> mandatory parameter.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 1/5] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Nicolin Chen]

On Tue, Feb 11, 2025 at 10:05:36PM -0800, Yi Liu wrote:
> Caller of the two APIs always provide a valid handle, make @handle as
> mandatory parameter.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

A side note here:

> @@ -3511,8 +3511,10 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>  {
>  	int ret;
>  
> -	if (handle)
> -		handle->domain = domain;
> +	if (!handle)
> +		return -EINVAL;
> +
> +	handle->domain = domain;
>  
>  	mutex_lock(&group->mutex);
>  	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> @@ -3568,16 +3570,15 @@ int iommu_replace_group_handle(struct iommu_group *group,
>  	void *curr;
>  	int ret;
>  
> -	if (!new_domain)
> +	if (!new_domain || !handle)
>  		return -EINVAL;
>  
> +	handle->domain = new_domain;
> +
>  	mutex_lock(&group->mutex);

So, we don't have the mutex fencing the handle->domain, since an
attach handle is always a newly allocated one in the caller, and
there won't be any potential race occurring to it.

Yet, this feels like an implication, which we should have stated
explicitly in the kdocs of these two functions?

Thanks
Nicolin

Re: [PATCH 1/5] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Yi Liu]

On 2025/2/20 00:48, Nicolin Chen wrote:
> On Tue, Feb 11, 2025 at 10:05:36PM -0800, Yi Liu wrote:
>> Caller of the two APIs always provide a valid handle, make @handle as
>> mandatory parameter.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> A side note here:
> 
>> @@ -3511,8 +3511,10 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   {
>>   	int ret;
>>   
>> -	if (handle)
>> -		handle->domain = domain;
>> +	if (!handle)
>> +		return -EINVAL;
>> +
>> +	handle->domain = domain;
>>   
>>   	mutex_lock(&group->mutex);
>>   	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
>> @@ -3568,16 +3570,15 @@ int iommu_replace_group_handle(struct iommu_group *group,
>>   	void *curr;
>>   	int ret;
>>   
>> -	if (!new_domain)
>> +	if (!new_domain || !handle)
>>   		return -EINVAL;
>>   
>> +	handle->domain = new_domain;
>> +
>>   	mutex_lock(&group->mutex);
> 
> So, we don't have the mutex fencing the handle->domain, since an
> attach handle is always a newly allocated one in the caller, and
> there won't be any potential race occurring to it.
> 
> Yet, this feels like an implication, which we should have stated
> explicitly in the kdocs of these two functions?
> 
hmmm. I can just move it under mutex. :)

-- 
Regards,
Yi Liu

RE: [PATCH 1/5] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 12, 2025 2:06 PM
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH 2/5] iommu: Drop iommu_group_replace_domain()

[Yi Liu]

iommufd does not use it now, so drop it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index eef7e19c547d..4ccb36b5aa30 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2187,32 +2187,6 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
-/**
- * iommu_group_replace_domain - replace the domain that a group is attached to
- * @group: IOMMU group that will be attached to the new domain
- * @new_domain: new IOMMU domain to replace with
- *
- * This API allows the group to switch domains without being forced to go to
- * the blocking domain in-between.
- *
- * If the currently attached domain is a core domain (e.g. a default_domain),
- * it will act just like the iommu_attach_group().
- */
-int iommu_group_replace_domain(struct iommu_group *group,
-			       struct iommu_domain *new_domain)
-{
-	int ret;
-
-	if (!new_domain)
-		return -EINVAL;
-
-	mutex_lock(&group->mutex);
-	ret = __iommu_group_set_domain(group, new_domain);
-	mutex_unlock(&group->mutex);
-	return ret;
-}
-EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, "IOMMUFD_INTERNAL");
-
 static int __iommu_device_set_domain(struct iommu_group *group,
 				     struct device *dev,
 				     struct iommu_domain *new_domain,
@@ -3559,9 +3533,12 @@ EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL");
  * @new_domain: new IOMMU domain to replace with
  * @handle: attach handle
  *
- * This is a variant of iommu_group_replace_domain(). It allows the caller to
- * provide an attach handle for the new domain and use it when the domain is
- * attached.
+ * This API allows the group to switch domains without being forced to go to
+ * the blocking domain in-between. It allows the caller to provide an attach
+ * handle for the new domain and use it when the domain is attached.
+ *
+ * If the currently attached domain is a core domain (e.g. a default_domain),
+ * it will act just like the iommu_attach_group_handle().
  */
 int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
-- 
2.34.1

Re: [PATCH 2/5] iommu: Drop iommu_group_replace_domain()

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:37PM -0800, Yi Liu wrote:
> iommufd does not use it now, so drop it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 35 ++++++-----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 2/5] iommu: Drop iommu_group_replace_domain()

[Nicolin Chen]

On Tue, Feb 11, 2025 at 10:05:37PM -0800, Yi Liu wrote:
> iommufd does not use it now, so drop it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 35 ++++++-----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)

Should we drop it in the following header too?
  drivers/iommu/iommu-priv.h

Otherwise,
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH 2/5] iommu: Drop iommu_group_replace_domain()

[Yi Liu]

On 2025/2/20 00:53, Nicolin Chen wrote:
> On Tue, Feb 11, 2025 at 10:05:37PM -0800, Yi Liu wrote:
>> iommufd does not use it now, so drop it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 35 ++++++-----------------------------
>>   1 file changed, 6 insertions(+), 29 deletions(-)
> 
> Should we drop it in the following header too?
>    drivers/iommu/iommu-priv.h

yes.

> Otherwise,
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

-- 
Regards,
Yi Liu

RE: [PATCH 2/5] iommu: Drop iommu_group_replace_domain()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 12, 2025 2:06 PM
> 
> iommufd does not use it now, so drop it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH 3/5] 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 previously.

To be complete, let the iommu_attach_device_pasid() store the domain
to group->pasid_array if no valid handle. The other users of the
group->pasid_array should be updated to be consistent. e.g. the
iommu_attach_group_handle() and iommu_replace_group_handle().

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4ccb36b5aa30..a7ebcbaafb10 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,9 @@ 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;
 
+/* Tags used with xa_tag_pointer() in group->pasid_array */
+enum { IOMMU_PASID_ARRAY_DOMAIN = 0, IOMMU_PASID_ARRAY_HANDLE = 1 };
+
 struct iommu_group {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -3331,6 +3334,17 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 		iommu_remove_dev_pasid(device->dev, pasid, domain);
 }
 
+static void *iommu_make_pasid_entry(struct iommu_domain *domain,
+				    struct iommu_attach_handle *handle)
+{
+	if (handle) {
+		handle->domain = domain;
+		return xa_tag_pointer(handle, IOMMU_PASID_ARRAY_HANDLE);
+	}
+
+	return xa_tag_pointer(domain, IOMMU_PASID_ARRAY_DOMAIN);
+}
+
 /*
  * iommu_attach_device_pasid() - Attach a domain to pasid of device
  * @domain: the iommu domain.
@@ -3348,6 +3362,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 *pasid_entry;
 	int ret;
 
 	if (!group)
@@ -3363,6 +3378,8 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	if (ops != domain->owner || pasid == IOMMU_NO_PASID)
 		return -EINVAL;
 
+	pasid_entry = iommu_make_pasid_entry(domain, handle);
+
 	mutex_lock(&group->mutex);
 	for_each_group_device(group, device) {
 		if (pasid >= device->dev->iommu->max_pasids) {
@@ -3371,10 +3388,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	if (handle)
-		handle->domain = domain;
-
-	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
+	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
 	if (ret)
 		goto out_unlock;
 
@@ -3454,13 +3468,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 *pasid_entry;
 
 	xa_lock(&group->pasid_array);
-	handle = xa_load(&group->pasid_array, pasid);
-	if (!handle)
+	pasid_entry = xa_load(&group->pasid_array, pasid);
+	if (xa_pointer_tag(pasid_entry) == IOMMU_PASID_ARRAY_HANDLE) {
+		handle = xa_untag_pointer(pasid_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;
@@ -3483,15 +3501,17 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 			      struct iommu_group *group,
 			      struct iommu_attach_handle *handle)
 {
+	void *pasid_entry;
 	int ret;
 
 	if (!handle)
 		return -EINVAL;
 
-	handle->domain = domain;
+	pasid_entry = iommu_make_pasid_entry(domain, handle);
 
 	mutex_lock(&group->mutex);
-	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+	ret = xa_insert(&group->pasid_array,
+			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
 	if (ret)
 		goto err_unlock;
 
@@ -3544,13 +3564,13 @@ int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle)
 {
-	void *curr;
+	void *curr, *pasid_entry;
 	int ret;
 
 	if (!new_domain || !handle)
 		return -EINVAL;
 
-	handle->domain = new_domain;
+	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
 
 	mutex_lock(&group->mutex);
 	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
@@ -3561,7 +3581,8 @@ int iommu_replace_group_handle(struct iommu_group *group,
 	if (ret)
 		goto err_release;
 
-	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+	curr = xa_store(&group->pasid_array,
+			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
 	WARN_ON(xa_is_err(curr));
 
 	mutex_unlock(&group->mutex);
-- 
2.34.1

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

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:38PM -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 previously.
> 
> To be complete, let the iommu_attach_device_pasid() store the domain
> to group->pasid_array if no valid handle. The other users of the
> group->pasid_array should be updated to be consistent. e.g. the
> iommu_attach_group_handle() and iommu_replace_group_handle().
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 47 +++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 13 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[Nicolin Chen]

On Tue, Feb 11, 2025 at 10:05:38PM -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 previously.
> 
> To be complete, let the iommu_attach_device_pasid() store the domain
> to group->pasid_array if no valid handle. The other users of the
> group->pasid_array should be updated to be consistent. e.g. the
> iommu_attach_group_handle() and iommu_replace_group_handle().
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

A nit:

> @@ -3483,15 +3501,17 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>  			      struct iommu_group *group,
>  			      struct iommu_attach_handle *handle)
>  {
> +	void *pasid_entry;
>  	int ret;
>  
>  	if (!handle)
>  		return -EINVAL;
>  
> -	handle->domain = domain;
> +	pasid_entry = iommu_make_pasid_entry(domain, handle);
>  
>  	mutex_lock(&group->mutex);
> -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> +	ret = xa_insert(&group->pasid_array,
> +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
[..]
> @@ -3561,7 +3581,8 @@ int iommu_replace_group_handle(struct iommu_group *group,
>  	if (ret)
>  		goto err_release;
>  
> -	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> +	curr = xa_store(&group->pasid_array,
> +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);

It's actually an entry for the group->pasid_array.

So, perhaps:
 - s/iommu_make_pasid_entry/iommu_make_pasid_array_entry/
 - s/pasid_entry/entry/

This feels a bit clearer and can avoid the two line breaks above.

Thanks
Nic

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

[Yi Liu]

On 2025/2/20 01:15, Nicolin Chen wrote:
> On Tue, Feb 11, 2025 at 10:05:38PM -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 previously.
>>
>> To be complete, let the iommu_attach_device_pasid() store the domain
>> to group->pasid_array if no valid handle. The other users of the
>> group->pasid_array should be updated to be consistent. e.g. the
>> iommu_attach_group_handle() and iommu_replace_group_handle().
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> A nit:
> 
>> @@ -3483,15 +3501,17 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   			      struct iommu_group *group,
>>   			      struct iommu_attach_handle *handle)
>>   {
>> +	void *pasid_entry;
>>   	int ret;
>>   
>>   	if (!handle)
>>   		return -EINVAL;
>>   
>> -	handle->domain = domain;
>> +	pasid_entry = iommu_make_pasid_entry(domain, handle);
>>   
>>   	mutex_lock(&group->mutex);
>> -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
>> +	ret = xa_insert(&group->pasid_array,
>> +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> [..]
>> @@ -3561,7 +3581,8 @@ int iommu_replace_group_handle(struct iommu_group *group,
>>   	if (ret)
>>   		goto err_release;
>>   
>> -	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
>> +	curr = xa_store(&group->pasid_array,
>> +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> 
> It's actually an entry for the group->pasid_array.
> 
> So, perhaps:
>   - s/iommu_make_pasid_entry/iommu_make_pasid_array_entry/
>   - s/pasid_entry/entry/
> 
> This feels a bit clearer and can avoid the two line breaks above.
> 

sure.

-- 
Regards,
Yi Liu

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, February 20, 2025 1:16 AM
> 
> On Tue, Feb 11, 2025 at 10:05:38PM -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 previously.
> >
> > To be complete, let the iommu_attach_device_pasid() store the domain
> > to group->pasid_array if no valid handle. The other users of the
> > group->pasid_array should be updated to be consistent. e.g. the
> > iommu_attach_group_handle() and iommu_replace_group_handle().
> >
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> A nit:
> 
> > @@ -3483,15 +3501,17 @@ int iommu_attach_group_handle(struct
> iommu_domain *domain,
> >  			      struct iommu_group *group,
> >  			      struct iommu_attach_handle *handle)
> >  {
> > +	void *pasid_entry;
> >  	int ret;
> >
> >  	if (!handle)
> >  		return -EINVAL;
> >
> > -	handle->domain = domain;
> > +	pasid_entry = iommu_make_pasid_entry(domain, handle);
> >
> >  	mutex_lock(&group->mutex);
> > -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle,
> GFP_KERNEL);
> > +	ret = xa_insert(&group->pasid_array,
> > +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> [..]
> > @@ -3561,7 +3581,8 @@ int iommu_replace_group_handle(struct
> iommu_group *group,
> >  	if (ret)
> >  		goto err_release;
> >
> > -	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, handle,
> GFP_KERNEL);
> > +	curr = xa_store(&group->pasid_array,
> > +			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> 
> It's actually an entry for the group->pasid_array.
> 
> So, perhaps:
>  - s/iommu_make_pasid_entry/iommu_make_pasid_array_entry/
>  - s/pasid_entry/entry/
> 
> This feels a bit clearer and can avoid the two line breaks above.
> 

or iommu_tag_pasid_pointer()

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

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, February 12, 2025 2:06 PM
> 
> 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 previously.
> 
> To be complete, let the iommu_attach_device_pasid() store the domain
> to group->pasid_array if no valid handle. The other users of the
> group->pasid_array should be updated to be consistent. e.g. the
> iommu_attach_group_handle() and iommu_replace_group_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>

[PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Yi Liu]

The current implementation stores group->pasid_array before the underlying
iommu driver has successfully set the new domain. This can lead to issues
where PRIs are received on the new domain before the attach operation is
completed. This patch swaps the order of operations to ensure that the
domain is set in the iommu driver before updating group->pasid_array.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a7ebcbaafb10..d673feab82aa 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3388,13 +3388,21 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
-	if (ret)
+	if (xa_load(&group->pasid_array, pasid)) {
+		ret = -EBUSY;
 		goto out_unlock;
+	}
 
 	ret = __iommu_set_group_pasid(domain, group, pasid);
 	if (ret)
-		xa_erase(&group->pasid_array, pasid);
+		goto out_unlock;
+
+	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
+	if (ret) {
+		WARN_ON(ret == -EBUSY);
+		__iommu_remove_group_pasid(group, pasid, domain);
+	}
+
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
@@ -3510,19 +3518,26 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 	pasid_entry = iommu_make_pasid_entry(domain, handle);
 
 	mutex_lock(&group->mutex);
-	ret = xa_insert(&group->pasid_array,
-			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
-	if (ret)
+	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
+		ret = -EBUSY;
 		goto err_unlock;
+	}
 
 	ret = __iommu_attach_group(domain, group);
 	if (ret)
-		goto err_erase;
+		goto err_unlock;
+
+	ret = xa_insert(&group->pasid_array,
+			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
+	if (ret) {
+		WARN_ON(ret == -EBUSY);
+		goto err_remove;
+	}
 	mutex_unlock(&group->mutex);
 
 	return 0;
-err_erase:
-	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
+err_remove:
+	__iommu_group_set_core_domain(group);
 err_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.34.1

Re: [PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:39PM -0800, Yi Liu wrote:
> The current implementation stores group->pasid_array before the underlying
> iommu driver has successfully set the new domain. This can lead to issues
> where PRIs are received on the new domain before the attach operation is
> completed. This patch swaps the order of operations to ensure that the
> domain is set in the iommu driver before updating group->pasid_array.

I think I would drop this note in a comment since it is subtle and
important..

> -	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
> -	if (ret)
> +	if (xa_load(&group->pasid_array, pasid)) {
> +		ret = -EBUSY;
>  		goto out_unlock;
> +	}

Let's write it like this:

	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
	if (ret)
		goto out_unlock;

	ret = __iommu_set_group_pasid(domain, group, pasid);
	if (ret) {
		xa_release(&group->pasid_array, pasid);
		goto out_unlock;
	}

	/*
	 * The xa_insert() above reserved the memory, and the group->mutex is
	 * held, this cannot fail. The new domain cannot be visible until the
	 * operation succeeds as we cannot tolerate PRIs becoming concurrently
	 * queued and then failing attach.
	 */
	ret = xa_err(
		xa_store(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL));
	WARN_ON(ret);
	ret = 0;

> @@ -3510,19 +3518,26 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>  	pasid_entry = iommu_make_pasid_entry(domain, handle);
>  
>  	mutex_lock(&group->mutex);
> -	ret = xa_insert(&group->pasid_array,
> -			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> -	if (ret)
> +	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
> +		ret = -EBUSY;
>  		goto err_unlock;
> +	}

Same here

Jason

Re: [PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Yi Liu]

On 2025/2/19 03:27, Jason Gunthorpe wrote:
> On Tue, Feb 11, 2025 at 10:05:39PM -0800, Yi Liu wrote:
>> The current implementation stores group->pasid_array before the underlying
>> iommu driver has successfully set the new domain. This can lead to issues
>> where PRIs are received on the new domain before the attach operation is
>> completed. This patch swaps the order of operations to ensure that the
>> domain is set in the iommu driver before updating group->pasid_array.
> 
> I think I would drop this note in a comment since it is subtle and
> important..
> 
>> -	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
>> -	if (ret)
>> +	if (xa_load(&group->pasid_array, pasid)) {
>> +		ret = -EBUSY;
>>   		goto out_unlock;
>> +	}
> 
> Let's write it like this:
> 
> 	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
> 	if (ret)
> 		goto out_unlock;
> 
> 	ret = __iommu_set_group_pasid(domain, group, pasid);
> 	if (ret) {
> 		xa_release(&group->pasid_array, pasid);
> 		goto out_unlock;
> 	}
> 
> 	/*
> 	 * The xa_insert() above reserved the memory, and the group->mutex is
> 	 * held, this cannot fail. The new domain cannot be visible until the
> 	 * operation succeeds as we cannot tolerate PRIs becoming concurrently
> 	 * queued and then failing attach.
> 	 */
> 	ret = xa_err(
> 		xa_store(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL));
> 	WARN_ON(ret);
> 	ret = 0;

perhaps we can save the above line by WARN_ON(xa_is_err(xa_store())); :)

> 
>> @@ -3510,19 +3518,26 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   	pasid_entry = iommu_make_pasid_entry(domain, handle);
>>   
>>   	mutex_lock(&group->mutex);
>> -	ret = xa_insert(&group->pasid_array,
>> -			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
>> -	if (ret)
>> +	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
>> +		ret = -EBUSY;
>>   		goto err_unlock;
>> +	}
> 
> Same here

yes.

-- 
Regards,
Yi Liu

RE: [PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, February 19, 2025 3:27 AM
> 
> On Tue, Feb 11, 2025 at 10:05:39PM -0800, Yi Liu wrote:
> > The current implementation stores group->pasid_array before the
> underlying
> > iommu driver has successfully set the new domain. This can lead to issues
> > where PRIs are received on the new domain before the attach operation is
> > completed. This patch swaps the order of operations to ensure that the
> > domain is set in the iommu driver before updating group->pasid_array.
> 
> I think I would drop this note in a comment since it is subtle and
> important..
> 
> > -	ret = xa_insert(&group->pasid_array, pasid, pasid_entry,
> GFP_KERNEL);
> > -	if (ret)
> > +	if (xa_load(&group->pasid_array, pasid)) {
> > +		ret = -EBUSY;
> >  		goto out_unlock;
> > +	}
> 
> Let's write it like this:
> 
> 	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY,
> GFP_KERNEL);
> 	if (ret)
> 		goto out_unlock;
> 
> 	ret = __iommu_set_group_pasid(domain, group, pasid);
> 	if (ret) {
> 		xa_release(&group->pasid_array, pasid);
> 		goto out_unlock;
> 	}
> 
> 	/*
> 	 * The xa_insert() above reserved the memory, and the group-
> >mutex is
> 	 * held, this cannot fail. The new domain cannot be visible until the
> 	 * operation succeeds as we cannot tolerate PRIs becoming
> concurrently
> 	 * queued and then failing attach.
> 	 */
> 	ret = xa_err(
> 		xa_store(&group->pasid_array, pasid, pasid_entry,
> GFP_KERNEL));
> 	WARN_ON(ret);
> 	ret = 0;
> 

with above change:

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

[PATCH 5/5] iommu: Retire group->domain

[Yi Liu]

group->domain stores the current attached domain of the group. Now, it is
also stored in group->pasid_array[0], hence retire the group->domain.

Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/dma-iommu.c  |   3 +-
 drivers/iommu/iommu-priv.h |   1 +
 drivers/iommu/iommu.c      | 178 +++++++++++++++++++++----------------
 3 files changed, 106 insertions(+), 76 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2a9fa0c8cc00..5da9aca0c021 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -34,6 +34,7 @@
 
 #include "dma-iommu.h"
 #include "iommu-pages.h"
+#include "iommu-priv.h"
 
 struct iommu_dma_msi_page {
 	struct list_head	list;
@@ -1746,7 +1747,7 @@ size_t iommu_dma_max_mapping_size(struct device *dev)
 
 void iommu_setup_dma_ops(struct device *dev)
 {
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iommu_domain *domain = iommu_group_domain(dev->iommu_group);
 
 	if (dev_is_pci(dev))
 		dev->iommu->pci_32bit_workaround = !iommu_dma_forcedac;
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index de5b54eaa8bf..fedc57754a48 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -46,4 +46,5 @@ void iommu_detach_group_handle(struct iommu_domain *domain,
 int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle);
+struct iommu_domain *iommu_group_domain(struct iommu_group *group);
 #endif /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d673feab82aa..c3bf8e131a8e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -60,7 +60,6 @@ struct iommu_group {
 	int id;
 	struct iommu_domain *default_domain;
 	struct iommu_domain *blocking_domain;
-	struct iommu_domain *domain;
 	struct list_head entry;
 	unsigned int owner_cnt;
 	void *owner;
@@ -101,7 +100,8 @@ static void iommu_release_device(struct device *dev);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
-				struct iommu_group *group);
+				struct iommu_group *group,
+				struct iommu_attach_handle *handle);
 static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
 						       unsigned int type,
 						       unsigned int flags);
@@ -116,19 +116,26 @@ static int __iommu_device_set_domain(struct iommu_group *group,
 				     unsigned int flags);
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
 					     struct iommu_domain *new_domain,
+					     struct iommu_attach_handle *handle,
 					     unsigned int flags);
 static int __iommu_group_set_domain(struct iommu_group *group,
 				    struct iommu_domain *new_domain)
 {
-	return __iommu_group_set_domain_internal(group, new_domain, 0);
+	return __iommu_group_set_domain_internal(group, new_domain, NULL, 0);
 }
 static void __iommu_group_set_domain_nofail(struct iommu_group *group,
 					    struct iommu_domain *new_domain)
 {
 	WARN_ON(__iommu_group_set_domain_internal(
-		group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED));
+		group, new_domain, NULL, IOMMU_SET_DOMAIN_MUST_SUCCEED));
 }
 
+static int __iommu_group_set_domain_handle(struct iommu_group *group,
+					   struct iommu_domain *new_domain,
+					   struct iommu_attach_handle *handle)
+{
+	return __iommu_group_set_domain_internal(group, new_domain, handle, 0);
+}
 static int iommu_setup_default_domain(struct iommu_group *group,
 				      int target_type);
 static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
@@ -504,7 +511,7 @@ static void iommu_deinit_device(struct device *dev)
 			iommu_domain_free(group->blocking_domain);
 			group->blocking_domain = NULL;
 		}
-		group->domain = NULL;
+		xa_erase(&group->pasid_array, IOMMU_NO_PASID);
 	}
 
 	/* Caller must put iommu_group */
@@ -515,8 +522,29 @@ static void iommu_deinit_device(struct device *dev)
 
 DEFINE_MUTEX(iommu_probe_device_lock);
 
+struct iommu_domain *iommu_group_domain(struct iommu_group *group)
+{
+	struct iommu_domain *domain;
+	void *pasid_entry;
+
+	lockdep_assert_held(&group->mutex);
+
+	pasid_entry = xa_load(&group->pasid_array, IOMMU_NO_PASID);
+	if (xa_pointer_tag(pasid_entry) == IOMMU_PASID_ARRAY_HANDLE) {
+		struct iommu_attach_handle *handle;
+
+		handle = xa_untag_pointer(pasid_entry);
+		domain = handle->domain;
+	} else {
+		domain = xa_untag_pointer(pasid_entry);
+	}
+
+	return domain;
+}
+
 static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
 {
+	struct iommu_domain *gdomain;
 	const struct iommu_ops *ops;
 	struct iommu_group *group;
 	struct group_device *gdev;
@@ -563,11 +591,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 * iommu_setup_default_domain()
 	 */
 	list_add_tail(&gdev->list, &group->devices);
-	WARN_ON(group->default_domain && !group->domain);
+	gdomain = iommu_group_domain(group);
+	WARN_ON(group->default_domain && !gdomain);
 	if (group->default_domain)
 		iommu_create_device_direct_mappings(group->default_domain, dev);
-	if (group->domain) {
-		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
+	if (gdomain) {
+		ret = __iommu_device_set_domain(group, dev, gdomain, 0);
 		if (ret)
 			goto err_remove_gdev;
 	} else if (!group->default_domain && !group_list) {
@@ -625,6 +654,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
 {
 	struct device *dev = grp_dev->dev;
 
+	lockdep_assert_held(&group->mutex);
+
 	sysfs_remove_link(group->devices_kobj, grp_dev->name);
 	sysfs_remove_link(&dev->kobj, "iommu_group");
 
@@ -637,7 +668,7 @@ static void __iommu_group_free_device(struct iommu_group *group,
 	 */
 	if (list_empty(&group->devices))
 		WARN_ON(group->owner_cnt ||
-			group->domain != group->default_domain);
+			iommu_group_domain(group) != group->default_domain);
 
 	kfree(grp_dev->name);
 	kfree(grp_dev);
@@ -2094,7 +2125,7 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 	if (list_count_nodes(&group->devices) != 1)
 		goto out_unlock;
 
-	ret = __iommu_attach_group(domain, group);
+	ret = __iommu_attach_group(domain, group, NULL);
 
 out_unlock:
 	mutex_unlock(&group->mutex);
@@ -2119,7 +2150,7 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 		return;
 
 	mutex_lock(&group->mutex);
-	if (WARN_ON(domain != group->domain) ||
+	if (WARN_ON(domain != iommu_group_domain(group)) ||
 	    WARN_ON(list_count_nodes(&group->devices) != 1))
 		goto out_unlock;
 	__iommu_group_set_core_domain(group);
@@ -2133,11 +2164,16 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
 {
 	/* Caller must be a probed driver on dev */
 	struct iommu_group *group = dev->iommu_group;
+	struct iommu_domain *gdomain;
 
 	if (!group)
 		return NULL;
 
-	return group->domain;
+	mutex_lock(&group->mutex);
+	gdomain = iommu_group_domain(group);
+	mutex_unlock(&group->mutex);
+
+	return gdomain;
 }
 EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
 
@@ -2151,19 +2187,25 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
 }
 
 static int __iommu_attach_group(struct iommu_domain *domain,
-				struct iommu_group *group)
+				struct iommu_group *group,
+				struct iommu_attach_handle *handle)
 {
+	struct iommu_domain *gdomain;
 	struct device *dev;
 
-	if (group->domain && group->domain != group->default_domain &&
-	    group->domain != group->blocking_domain)
+	lockdep_assert_held(&group->mutex);
+
+	gdomain = iommu_group_domain(group);
+
+	if (gdomain && gdomain != group->default_domain &&
+	    gdomain != group->blocking_domain)
 		return -EBUSY;
 
 	dev = iommu_group_first_dev(group);
 	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
 		return -EINVAL;
 
-	return __iommu_group_set_domain(group, domain);
+	return __iommu_group_set_domain_handle(group, domain, handle);
 }
 
 /**
@@ -2183,7 +2225,7 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 	int ret;
 
 	mutex_lock(&group->mutex);
-	ret = __iommu_attach_group(domain, group);
+	ret = __iommu_attach_group(domain, group, NULL);
 	mutex_unlock(&group->mutex);
 
 	return ret;
@@ -2234,6 +2276,9 @@ static int __iommu_device_set_domain(struct iommu_group *group,
 	return 0;
 }
 
+static void *iommu_make_pasid_entry(struct iommu_domain *domain,
+				    struct iommu_attach_handle *handle);
+
 /*
  * If 0 is returned the group's domain is new_domain. If an error is returned
  * then the group's domain will be set back to the existing domain unless
@@ -2251,26 +2296,34 @@ static int __iommu_device_set_domain(struct iommu_group *group,
  */
 static int __iommu_group_set_domain_internal(struct iommu_group *group,
 					     struct iommu_domain *new_domain,
+					     struct iommu_attach_handle *handle,
 					     unsigned int flags)
 {
 	struct group_device *last_gdev;
+	struct iommu_domain *gdomain;
 	struct group_device *gdev;
+	void *pasid_entry;
 	int result;
 	int ret;
 
 	lockdep_assert_held(&group->mutex);
 
-	if (group->domain == new_domain)
+	gdomain = iommu_group_domain(group);
+	if (gdomain == new_domain)
 		return 0;
 
 	if (WARN_ON(!new_domain))
 		return -EINVAL;
 
+	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
+	if (ret)
+		return ret;
+
 	/*
 	 * Changing the domain is done by calling attach_dev() on the new
 	 * domain. This switch does not have to be atomic and DMA can be
 	 * discarded during the transition. DMA must only be able to access
-	 * either new_domain or group->domain, never something else.
+	 * either new_domain or the old domain, never something else.
 	 */
 	result = 0;
 	for_each_group_device(group, gdev) {
@@ -2290,7 +2343,10 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
 			goto err_revert;
 		}
 	}
-	group->domain = new_domain;
+
+	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
+	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
+				   IOMMU_NO_PASID, pasid_entry, GFP_KERNEL)));
 	return result;
 
 err_revert:
@@ -2302,16 +2358,17 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
 	for_each_group_device(group, gdev) {
 		/*
 		 * A NULL domain can happen only for first probe, in which case
-		 * we leave group->domain as NULL and let release clean
+		 * we leave domain in pasid_array as NULL and let release clean
 		 * everything up.
 		 */
-		if (group->domain)
+		if (gdomain)
 			WARN_ON(__iommu_device_set_domain(
-				group, gdev->dev, group->domain,
+				group, gdev->dev, gdomain,
 				IOMMU_SET_DOMAIN_MUST_SUCCEED));
 		if (gdev == last_gdev)
 			break;
 	}
+	xa_release(&group->pasid_array, IOMMU_NO_PASID);
 	return ret;
 }
 
@@ -2944,7 +3001,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 
 	/* We must set default_domain early for __iommu_device_set_domain */
 	group->default_domain = dom;
-	if (!group->domain) {
+	if (!iommu_group_domain(group)) {
 		/*
 		 * Drivers are not allowed to fail the first domain attach.
 		 * The only way to recover from this is to fail attaching the
@@ -2952,7 +3009,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 		 * in group->default_domain so it is freed after.
 		 */
 		ret = __iommu_group_set_domain_internal(
-			group, dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
+			group, dom, NULL, IOMMU_SET_DOMAIN_MUST_SUCCEED);
 		if (WARN_ON(ret))
 			goto out_free_old;
 	} else {
@@ -2983,7 +3040,7 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 err_restore_domain:
 	if (old_dom)
 		__iommu_group_set_domain_internal(
-			group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
+			group, old_dom, NULL, IOMMU_SET_DOMAIN_MUST_SUCCEED);
 err_restore_def_domain:
 	if (old_dom) {
 		iommu_domain_free(dom);
@@ -3056,6 +3113,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 	return ret ?: count;
 }
 
+static bool iommu_group_pasid_used(struct iommu_group *group)
+{
+	unsigned long start = IOMMU_NO_PASID;
+
+	lockdep_assert_held(&group->mutex);
+	return xa_find_after(&group->pasid_array, &start, UINT_MAX, XA_PRESENT);
+}
+
 /**
  * iommu_device_use_default_domain() - Device driver wants to handle device
  *                                     DMA through the kernel DMA API.
@@ -3075,8 +3140,8 @@ int iommu_device_use_default_domain(struct device *dev)
 
 	mutex_lock(&group->mutex);
 	if (group->owner_cnt) {
-		if (group->domain != group->default_domain || group->owner ||
-		    !xa_empty(&group->pasid_array)) {
+		if (iommu_group_domain(group) != group->default_domain ||
+		    group->owner || iommu_group_pasid_used(group)) {
 			ret = -EBUSY;
 			goto unlock_out;
 		}
@@ -3106,7 +3171,7 @@ void iommu_device_unuse_default_domain(struct device *dev)
 		return;
 
 	mutex_lock(&group->mutex);
-	if (!WARN_ON(!group->owner_cnt || !xa_empty(&group->pasid_array)))
+	if (!WARN_ON(!group->owner_cnt || iommu_group_pasid_used(group)))
 		group->owner_cnt--;
 
 	mutex_unlock(&group->mutex);
@@ -3139,10 +3204,14 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 
 static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
 {
+	struct iommu_domain *gdomain;
 	int ret;
 
-	if ((group->domain && group->domain != group->default_domain) ||
-	    !xa_empty(&group->pasid_array))
+	lockdep_assert_held(&group->mutex);
+
+	gdomain = iommu_group_domain(group);
+	if ((gdomain && gdomain != group->default_domain) ||
+	    iommu_group_pasid_used(group))
 		return -EBUSY;
 
 	ret = __iommu_group_alloc_blocking_domain(group);
@@ -3228,7 +3297,7 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
 static void __iommu_release_dma_ownership(struct iommu_group *group)
 {
 	if (WARN_ON(!group->owner_cnt || !group->owner ||
-		    !xa_empty(&group->pasid_array)))
+		    iommu_group_pasid_used(group)))
 		return;
 
 	group->owner_cnt = 0;
@@ -3509,37 +3578,15 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 			      struct iommu_group *group,
 			      struct iommu_attach_handle *handle)
 {
-	void *pasid_entry;
 	int ret;
 
 	if (!handle)
 		return -EINVAL;
 
-	pasid_entry = iommu_make_pasid_entry(domain, handle);
-
 	mutex_lock(&group->mutex);
-	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
-		ret = -EBUSY;
-		goto err_unlock;
-	}
-
-	ret = __iommu_attach_group(domain, group);
-	if (ret)
-		goto err_unlock;
-
-	ret = xa_insert(&group->pasid_array,
-			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
-	if (ret) {
-		WARN_ON(ret == -EBUSY);
-		goto err_remove;
-	}
+	ret = __iommu_attach_group(domain, group, handle);
 	mutex_unlock(&group->mutex);
 
-	return 0;
-err_remove:
-	__iommu_group_set_core_domain(group);
-err_unlock:
-	mutex_unlock(&group->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(iommu_attach_group_handle, "IOMMUFD_INTERNAL");
@@ -3579,34 +3626,15 @@ int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle)
 {
-	void *curr, *pasid_entry;
 	int ret;
 
 	if (!new_domain || !handle)
 		return -EINVAL;
 
-	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
-
 	mutex_lock(&group->mutex);
-	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
-	if (ret)
-		goto err_unlock;
-
-	ret = __iommu_group_set_domain(group, new_domain);
-	if (ret)
-		goto err_release;
-
-	curr = xa_store(&group->pasid_array,
-			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
-	WARN_ON(xa_is_err(curr));
-
+	ret = __iommu_group_set_domain_handle(group, new_domain, handle);
 	mutex_unlock(&group->mutex);
 
-	return 0;
-err_release:
-	xa_release(&group->pasid_array, IOMMU_NO_PASID);
-err_unlock:
-	mutex_unlock(&group->mutex);
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");
-- 
2.34.1

Re: [PATCH 5/5] iommu: Retire group->domain

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
> @@ -2133,11 +2164,16 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
>  {
>  	/* Caller must be a probed driver on dev */
>  	struct iommu_group *group = dev->iommu_group;
> +	struct iommu_domain *gdomain;
>  
>  	if (!group)
>  		return NULL;
>  
> -	return group->domain;
> +	mutex_lock(&group->mutex);
> +	gdomain = iommu_group_domain(group);
> +	mutex_unlock(&group->mutex);

That will deadlock

Eg:

static int mtk_iommu_identity_attach(struct iommu_domain *identity_domain,
				     struct device *dev)
{
	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

And attach is already called under the group mutex.

IMHO I would have two operations, iommu_group_domain() which lockdep
asserts that group->mutex is held by the caller. We should try to call
this function when possible to get clear locking rules

And then iommu_get_domain_for_dev() which explains it is an older
function that depends on the caller somehow ensuring that the attached
domain cannot change. Ideally we would someday try to remove calls to
iommu_get_domain_for_dev()..

Since the xarray cannot change for non-lockdepable reasons, it can
just call xa_load(). Probably re-organize iommu_group_domain() to have
a __iommu_group_domain() which has the body but no lockdep assertion.

Jason

Re: [PATCH 5/5] iommu: Retire group->domain

[Yi Liu]

On 2025/2/19 03:39, Jason Gunthorpe wrote:
> On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
>> @@ -2133,11 +2164,16 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
>>   {
>>   	/* Caller must be a probed driver on dev */
>>   	struct iommu_group *group = dev->iommu_group;
>> +	struct iommu_domain *gdomain;
>>   
>>   	if (!group)
>>   		return NULL;
>>   
>> -	return group->domain;
>> +	mutex_lock(&group->mutex);
>> +	gdomain = iommu_group_domain(group);
>> +	mutex_unlock(&group->mutex);
> 
> That will deadlock
> 
> Eg:
> 
> static int mtk_iommu_identity_attach(struct iommu_domain *identity_domain,
> 				     struct device *dev)
> {
> 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> 
> And attach is already called under the group mutex.

oops. yes, it is!

> IMHO I would have two operations, iommu_group_domain() which lockdep
> asserts that group->mutex is held by the caller. We should try to call
> this function when possible to get clear locking rules
> 
> And then iommu_get_domain_for_dev() which explains it is an older
> function that depends on the caller somehow ensuring that the attached
> domain cannot change. Ideally we would someday try to remove calls to
> iommu_get_domain_for_dev()..

The existing iommu_get_domain_for_dev() seems to be abused in some
places. There are some device drivers that call it without group->mutex.

> Since the xarray cannot change for non-lockdepable reasons, it can
> just call xa_load(). Probably re-organize iommu_group_domain() to have
> a __iommu_group_domain() which has the body but no lockdep assertion.

makes sense.

-- 
Regards,
Yi Liu

Re: [PATCH 5/5] iommu: Retire group->domain

[Yi Liu]

On 2025/2/19 14:52, Yi Liu wrote:
> On 2025/2/19 03:39, Jason Gunthorpe wrote:
>> On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
>>> @@ -2133,11 +2164,16 @@ struct iommu_domain 
>>> *iommu_get_domain_for_dev(struct device *dev)
>>>   {
>>>       /* Caller must be a probed driver on dev */
>>>       struct iommu_group *group = dev->iommu_group;
>>> +    struct iommu_domain *gdomain;
>>>       if (!group)
>>>           return NULL;
>>> -    return group->domain;
>>> +    mutex_lock(&group->mutex);
>>> +    gdomain = iommu_group_domain(group);
>>> +    mutex_unlock(&group->mutex);
>>
>> That will deadlock
>>
>> Eg:
>>
>> static int mtk_iommu_identity_attach(struct iommu_domain *identity_domain,
>>                      struct device *dev)
>> {
>>     struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>
>> And attach is already called under the group mutex.
> 
> oops. yes, it is!
> 
>> IMHO I would have two operations, iommu_group_domain() which lockdep
>> asserts that group->mutex is held by the caller. We should try to call
>> this function when possible to get clear locking rules
>>
>> And then iommu_get_domain_for_dev() which explains it is an older
>> function that depends on the caller somehow ensuring that the attached
>> domain cannot change. Ideally we would someday try to remove calls to
>> iommu_get_domain_for_dev()..

currently, the paths hold group->mutex are mostly in the iommu drivers.
So what we may do is updating the iommu drivers to use a version that
has lockdep. While other callers keep using the iommu_get_domain_for_dev().

>> Since the xarray cannot change for non-lockdepable reasons, it can
>> just call xa_load(). Probably re-organize iommu_group_domain() to have
>> a __iommu_group_domain() which has the body but no lockdep assertion.

another approach may be holding xa_lock when retrieving domain from xarray
in the iommu_get_domain_for_dev() helper. This approach is consistent with
the usage pattern of iommu_attach_handle_get(), where callers also do not
acquire the group->mutex. Thoughts?

-- 
Regards,
Yi Liu

Re: [PATCH 5/5] iommu: Retire group->domain

[Jason Gunthorpe]

On Wed, Feb 19, 2025 at 08:31:52PM +0800, Yi Liu wrote:
> > > And then iommu_get_domain_for_dev() which explains it is an older
> > > function that depends on the caller somehow ensuring that the attached
> > > domain cannot change. Ideally we would someday try to remove calls to
> > > iommu_get_domain_for_dev()..
> 
> currently, the paths hold group->mutex are mostly in the iommu drivers.
> So what we may do is updating the iommu drivers to use a version that
> has lockdep. While other callers keep using the iommu_get_domain_for_dev().

Honestly I would prefer to pass the old domain in as a function argument to
attach and remove the calls entirely.

> > > Since the xarray cannot change for non-lockdepable reasons, it can
> > > just call xa_load(). Probably re-organize iommu_group_domain() to have
> > > a __iommu_group_domain() which has the body but no lockdep assertion.
> 
> another approach may be holding xa_lock when retrieving domain from xarray
> in the iommu_get_domain_for_dev() helper. This approach is consistent with
> the usage pattern of iommu_attach_handle_get(), where callers also do not
> acquire the group->mutex. Thoughts?

It doesn't matter, the pointer is unsafe the instant the xa_lock is
released.

The locking works in most places that don't hold the group mutex
because they rely on a driver being probed and the dma api keeping the
domain constant during that period.

Jason

Re: [PATCH 5/5] iommu: Retire group->domain

[Yi Liu]

On 2025/2/19 21:10, Jason Gunthorpe wrote:
> On Wed, Feb 19, 2025 at 08:31:52PM +0800, Yi Liu wrote:
>>>> And then iommu_get_domain_for_dev() which explains it is an older
>>>> function that depends on the caller somehow ensuring that the attached
>>>> domain cannot change. Ideally we would someday try to remove calls to
>>>> iommu_get_domain_for_dev()..
>>
>> currently, the paths hold group->mutex are mostly in the iommu drivers.
>> So what we may do is updating the iommu drivers to use a version that
>> has lockdep. While other callers keep using the iommu_get_domain_for_dev().
> 
> Honestly I would prefer to pass the old domain in as a function argument to
> attach and remove the calls entirely.

hmmm. I may drop the retire group->domain from this series to avoid
blocking the iommufd pasid series.

>>>> Since the xarray cannot change for non-lockdepable reasons, it can
>>>> just call xa_load(). Probably re-organize iommu_group_domain() to have
>>>> a __iommu_group_domain() which has the body but no lockdep assertion.
>>
>> another approach may be holding xa_lock when retrieving domain from xarray
>> in the iommu_get_domain_for_dev() helper. This approach is consistent with
>> the usage pattern of iommu_attach_handle_get(), where callers also do not
>> acquire the group->mutex. Thoughts?
> 
> It doesn't matter, the pointer is unsafe the instant the xa_lock is
> released.
> 
> The locking works in most places that don't hold the group mutex
> because they rely on a driver being probed and the dma api keeping the
> domain constant during that period.

yes. Since group->domain is set/clear under group->mutex as well. So I'm
wondering if it is ok to add kdoc to iommu_get_domain_for_dev() to note
it, and just add the xa_load && entry_decode stuffs without lockdep?

-- 
Regards,
Yi Liu

RE: [PATCH 5/5] iommu: Retire group->domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, February 20, 2025 12:02 PM
> 
> On 2025/2/19 21:10, Jason Gunthorpe wrote:
> > On Wed, Feb 19, 2025 at 08:31:52PM +0800, Yi Liu wrote:
> >>>> And then iommu_get_domain_for_dev() which explains it is an older
> >>>> function that depends on the caller somehow ensuring that the
> attached
> >>>> domain cannot change. Ideally we would someday try to remove calls
> to
> >>>> iommu_get_domain_for_dev()..
> >>
> >> currently, the paths hold group->mutex are mostly in the iommu drivers.
> >> So what we may do is updating the iommu drivers to use a version that
> >> has lockdep. While other callers keep using the
> iommu_get_domain_for_dev().
> >
> > Honestly I would prefer to pass the old domain in as a function argument
> to
> > attach and remove the calls entirely.
> 
> hmmm. I may drop the retire group->domain from this series to avoid
> blocking the iommufd pasid series.
> 

This kind of makes sense to me.

Re: [PATCH 5/5] iommu: Retire group->domain

[Jason Gunthorpe]

On Thu, Feb 20, 2025 at 12:01:56PM +0800, Yi Liu wrote:
> On 2025/2/19 21:10, Jason Gunthorpe wrote:
> > On Wed, Feb 19, 2025 at 08:31:52PM +0800, Yi Liu wrote:
> > > > > And then iommu_get_domain_for_dev() which explains it is an older
> > > > > function that depends on the caller somehow ensuring that the attached
> > > > > domain cannot change. Ideally we would someday try to remove calls to
> > > > > iommu_get_domain_for_dev()..
> > > 
> > > currently, the paths hold group->mutex are mostly in the iommu drivers.
> > > So what we may do is updating the iommu drivers to use a version that
> > > has lockdep. While other callers keep using the iommu_get_domain_for_dev().
> > 
> > Honestly I would prefer to pass the old domain in as a function argument to
> > attach and remove the calls entirely.
> 
> hmmm. I may drop the retire group->domain from this series to avoid
> blocking the iommufd pasid series.

Ok

> yes. Since group->domain is set/clear under group->mutex as well. So I'm
> wondering if it is ok to add kdoc to iommu_get_domain_for_dev() to note
> it, and just add the xa_load && entry_decode stuffs without lockdep?

Yes, kdoc is the way to go here

Jason 

Re: [PATCH 5/5] iommu: Retire group->domain

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
>  static int __iommu_attach_device(struct iommu_domain *domain,
>  				 struct device *dev);
>  static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group);
> +				struct iommu_group *group,
> +				struct iommu_attach_handle *handle);

I think I would split this particular transformation and it's related
into its own patch, it would shrink this alot. It is principally about
sharing the handle and non-handle paths..

> @@ -563,11 +591,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  	 * iommu_setup_default_domain()
>  	 */
>  	list_add_tail(&gdev->list, &group->devices);
> -	WARN_ON(group->default_domain && !group->domain);
> +	gdomain = iommu_group_domain(group);
> +	WARN_ON(group->default_domain && !gdomain);
>  	if (group->default_domain)
>  		iommu_create_device_direct_mappings(group->default_domain, dev);
> -	if (group->domain) {
> -		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
> +	if (gdomain) {
> +		ret = __iommu_device_set_domain(group, dev, gdomain, 0);
>  		if (ret)
>  			goto err_remove_gdev;
>  	} else if (!group->default_domain && !group_list) {
> @@ -625,6 +654,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
>  {
>  	struct device *dev = grp_dev->dev;
>  
> +	lockdep_assert_held(&group->mutex);
> +
>  	sysfs_remove_link(group->devices_kobj, grp_dev->name);
>  	sysfs_remove_link(&dev->kobj, "iommu_group");
>  
> @@ -637,7 +668,7 @@ static void __iommu_group_free_device(struct iommu_group *group,
>  	 */
>  	if (list_empty(&group->devices))
>  		WARN_ON(group->owner_cnt ||
> -			group->domain != group->default_domain);
> +			iommu_group_domain(group) != group->default_domain);

You could also probably put the conversion of locked group->domain
into iommu_group_domain() into its own patch (though not using the
xarray to start), that conversion looks like 9 hunks.

> +static void *iommu_make_pasid_entry(struct iommu_domain *domain,
> +				    struct iommu_attach_handle *handle);

Should move the helper to the top of the file in the patch that
introduces it

>  static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  					     struct iommu_domain *new_domain,
> +					     struct iommu_attach_handle *handle,
>  					     unsigned int flags)
>  {
>  	struct group_device *last_gdev;
> +	struct iommu_domain *gdomain;
>  	struct group_device *gdev;
> +	void *pasid_entry;
>  	int result;
>  	int ret;
>  
>  	lockdep_assert_held(&group->mutex);
>  
> -	if (group->domain == new_domain)
> +	gdomain = iommu_group_domain(group);
> +	if (gdomain == new_domain)
>  		return 0;

This is not quite right anymore, we could replace a domain with a
handle or viceversa.

Probably like this:

	if (WARN_ON(!new_domain))
		return -EINVAL;

	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
	curr = xa_cmpxchg(&group->pasid_array, IOMMU_NO_PASID, NULL,
			  XA_ZERO_ENTRY, GFP_KERNEL);
	if (xa_is_err(curr))
		return xa_err(curr);
	if (curr == pasid_entry)
		return 0;
	gdomain = decode_pasid_entry(curr);

> @@ -2290,7 +2343,10 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  			goto err_revert;
>  		}
>  	}
> -	group->domain = new_domain;
> +
> +	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
> +				   IOMMU_NO_PASID, pasid_entry, GFP_KERNEL)));

Ah, this flow uses the same as I suggested a few messages ago already!

> @@ -2302,16 +2358,17 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  	for_each_group_device(group, gdev) {
>  		/*
>  		 * A NULL domain can happen only for first probe, in which case
> -		 * we leave group->domain as NULL and let release clean
> +		 * we leave domain in pasid_array as NULL and let release clean
>  		 * everything up.
>  		 */
> -		if (group->domain)
> +		if (gdomain)
>  			WARN_ON(__iommu_device_set_domain(
> -				group, gdev->dev, group->domain,
> +				group, gdev->dev, gdomain,
>  				IOMMU_SET_DOMAIN_MUST_SUCCEED));
>  		if (gdev == last_gdev)
>  			break;
>  	}
> +	xa_release(&group->pasid_array, IOMMU_NO_PASID);

This does not seem right, the prior flow left group->domain unchanged
here. IMHO just drop it, the xarray is always populated and almost
never has NULL.

Jason

Re: [PATCH 5/5] iommu: Retire group->domain

[Yi Liu]

On 2025/2/19 03:57, Jason Gunthorpe wrote:
> On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
>>   static int __iommu_attach_device(struct iommu_domain *domain,
>>   				 struct device *dev);
>>   static int __iommu_attach_group(struct iommu_domain *domain,
>> -				struct iommu_group *group);
>> +				struct iommu_group *group,
>> +				struct iommu_attach_handle *handle);
> 
> I think I would split this particular transformation and it's related
> into its own patch, it would shrink this alot. It is principally about
> sharing the handle and non-handle paths..

got it.

> 
>> @@ -563,11 +591,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>>   	 * iommu_setup_default_domain()
>>   	 */
>>   	list_add_tail(&gdev->list, &group->devices);
>> -	WARN_ON(group->default_domain && !group->domain);
>> +	gdomain = iommu_group_domain(group);
>> +	WARN_ON(group->default_domain && !gdomain);
>>   	if (group->default_domain)
>>   		iommu_create_device_direct_mappings(group->default_domain, dev);
>> -	if (group->domain) {
>> -		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
>> +	if (gdomain) {
>> +		ret = __iommu_device_set_domain(group, dev, gdomain, 0);
>>   		if (ret)
>>   			goto err_remove_gdev;
>>   	} else if (!group->default_domain && !group_list) {
>> @@ -625,6 +654,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
>>   {
>>   	struct device *dev = grp_dev->dev;
>>   
>> +	lockdep_assert_held(&group->mutex);
>> +
>>   	sysfs_remove_link(group->devices_kobj, grp_dev->name);
>>   	sysfs_remove_link(&dev->kobj, "iommu_group");
>>   
>> @@ -637,7 +668,7 @@ static void __iommu_group_free_device(struct iommu_group *group,
>>   	 */
>>   	if (list_empty(&group->devices))
>>   		WARN_ON(group->owner_cnt ||
>> -			group->domain != group->default_domain);
>> +			iommu_group_domain(group) != group->default_domain);
> 
> You could also probably put the conversion of locked group->domain
> into iommu_group_domain() into its own patch (though not using the
> xarray to start), that conversion looks like 9 hunks.

got it.

>> +static void *iommu_make_pasid_entry(struct iommu_domain *domain,
>> +				    struct iommu_attach_handle *handle);
> 
> Should move the helper to the top of the file in the patch that
> introduces it
> 
>>   static int __iommu_group_set_domain_internal(struct iommu_group *group,
>>   					     struct iommu_domain *new_domain,
>> +					     struct iommu_attach_handle *handle,
>>   					     unsigned int flags)
>>   {
>>   	struct group_device *last_gdev;
>> +	struct iommu_domain *gdomain;
>>   	struct group_device *gdev;
>> +	void *pasid_entry;
>>   	int result;
>>   	int ret;
>>   
>>   	lockdep_assert_held(&group->mutex);
>>   
>> -	if (group->domain == new_domain)
>> +	gdomain = iommu_group_domain(group);
>> +	if (gdomain == new_domain)
>>   		return 0;
> 
> This is not quite right anymore, we could replace a domain with a
> handle or viceversa.

I see. yeah, even domain is the same, it may be on a replace_handle path
which needs to update the pasid_entry for sure.

> 
> Probably like this:
> 
> 	if (WARN_ON(!new_domain))
> 		return -EINVAL;
> 
> 	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
> 	curr = xa_cmpxchg(&group->pasid_array, IOMMU_NO_PASID, NULL,
> 			  XA_ZERO_ENTRY, GFP_KERNEL);
> 	if (xa_is_err(curr))
> 		return xa_err(curr);
> 	if (curr == pasid_entry)
> 		return 0;
> 	gdomain = decode_pasid_entry(curr);
> 
>> @@ -2290,7 +2343,10 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>>   			goto err_revert;
>>   		}
>>   	}
>> -	group->domain = new_domain;
>> +
>> +	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
>> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
>> +				   IOMMU_NO_PASID, pasid_entry, GFP_KERNEL)));
> 
> Ah, this flow uses the same as I suggested a few messages ago already!

yeah, I used xa_reserve() though.

>> @@ -2302,16 +2358,17 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>>   	for_each_group_device(group, gdev) {
>>   		/*
>>   		 * A NULL domain can happen only for first probe, in which case
>> -		 * we leave group->domain as NULL and let release clean
>> +		 * we leave domain in pasid_array as NULL and let release clean
>>   		 * everything up.
>>   		 */
>> -		if (group->domain)
>> +		if (gdomain)
>>   			WARN_ON(__iommu_device_set_domain(
>> -				group, gdev->dev, group->domain,
>> +				group, gdev->dev, gdomain,
>>   				IOMMU_SET_DOMAIN_MUST_SUCCEED));
>>   		if (gdev == last_gdev)
>>   			break;
>>   	}
>> +	xa_release(&group->pasid_array, IOMMU_NO_PASID);
> 
> This does not seem right, the prior flow left group->domain unchanged
> here. IMHO just drop it, the xarray is always populated and almost
> never has NULL.

yes.

-- 
Regards,
Yi Liu

Re: [PATCH 0/5] Misc iommu_attach_handle enhancements in iommu core

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 10:05:35PM -0800, Yi Liu wrote:
> This patch series aims to enhance the iommu_attach_handle-related APIs
> in the IOMMU core. The changes build upon Nic's previous series[1], 

So it needs some branching help to go into Joerg's tree ?

> This series also covers the prior series[2] which only takes care of the
> iommu_attach_device_pasid().

Is this a pre-condition for your iommufd PASID series?

Jason

Re: [PATCH 0/5] Misc iommu_attach_handle enhancements in iommu core

[Yi Liu]

On 2025/2/12 23:25, Jason Gunthorpe wrote:
> On Tue, Feb 11, 2025 at 10:05:35PM -0800, Yi Liu wrote:
>> This patch series aims to enhance the iommu_attach_handle-related APIs
>> in the IOMMU core. The changes build upon Nic's previous series[1],
> 
> So it needs some branching help to go into Joerg's tree ?

yes since all the changes are in the core. Current series is based on your
for-next.

https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next 
   dc10ba25d43f433ad5d9e8e6be4f4d2bb3cd9ddb

> 
>> This series also covers the prior series[2] which only takes care of the
>> iommu_attach_device_pasid().
> 
> Is this a pre-condition for your iommufd PASID series?

yes. The last patch is nice to have. I included it as it is just one minor
step.

-- 
Regards,
Yi Liu

Re: [PATCH 0/5] Misc iommu_attach_handle enhancements in iommu core

[Jason Gunthorpe]

On Thu, Feb 13, 2025 at 11:16:23AM +0800, Yi Liu wrote:
> On 2025/2/12 23:25, Jason Gunthorpe wrote:
> > On Tue, Feb 11, 2025 at 10:05:35PM -0800, Yi Liu wrote:
> > > This patch series aims to enhance the iommu_attach_handle-related APIs
> > > in the IOMMU core. The changes build upon Nic's previous series[1],
> > 
> > So it needs some branching help to go into Joerg's tree ?
> 
> yes since all the changes are in the core. Current series is based on your
> for-next.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next
> dc10ba25d43f433ad5d9e8e6be4f4d2bb3cd9ddb

OK, Joerg I will make a branch for you to pick up

> > > This series also covers the prior series[2] which only takes care of the
> > > iommu_attach_device_pasid().
> > 
> > Is this a pre-condition for your iommufd PASID series?
> 
> yes. The last patch is nice to have. I included it as it is just one minor
> step.

Ok

Jason