[PATCH v2 0/4] Misc iommu_attach_handle enhancements in iommu core

[PATCH v2 0/4] 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.

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/

Change log:

v2:
 - Move handl->domain under group->mutex protect (Nic)
 - Refine the patch 04 per the model of reserve xa first and then xa_store in
   the end which does not suppose to fail (Jason)
 - Drop the patch 05 of v1 as retire group->domain requires further effort
 - Add r-b tags

v1: https://lore.kernel.org/linux-iommu/20250212060540.261436-1-yi.l.liu@intel.com/

Regards,
	Yi Liu

Yi Liu (4):
  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

 drivers/iommu/iommu-priv.h |   3 -
 drivers/iommu/iommu.c      | 131 +++++++++++++++++++++----------------
 2 files changed, 73 insertions(+), 61 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] 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. Take this chance incoporate the handle->domain
set under the protection of group->mutex in iommu_attach_group_handle().

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 870c3cdbd0f6..715bdb8af31b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3511,10 +3511,11 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (handle)
-		handle->domain = domain;
+	if (!handle)
+		return -EINVAL;
 
 	mutex_lock(&group->mutex);
+	handle->domain = domain;
 	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
 	if (ret)
 		goto err_unlock;
@@ -3568,16 +3569,14 @@ int iommu_replace_group_handle(struct iommu_group *group,
 	void *curr;
 	int ret;
 
-	if (!new_domain)
+	if (!new_domain || !handle)
 		return -EINVAL;
 
 	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;
-	}
+	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 v2 1/4] iommu: Make @handle mandatory in iommu_{attach|replace}_group_handle()

[Baolu Lu]

On 2025/2/21 22:33, Yi Liu wrote:
> Caller of the two APIs always provide a valid handle, make @handle as
> mandatory parameter. Take this chance incoporate the handle->domain
> set under the protection of group->mutex in iommu_attach_group_handle().
> 
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Nicolin Chen<nicolinc@nvidia.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommu.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

[PATCH v2 2/4] iommu: Drop iommu_group_replace_domain()

[Yi Liu]

iommufd does not use it now, so drop it.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu-priv.h |  3 ---
 drivers/iommu/iommu.c      | 35 ++++++-----------------------------
 2 files changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index de5b54eaa8bf..b4508423e13b 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -24,9 +24,6 @@ static inline const struct iommu_ops *iommu_fwspec_ops(struct iommu_fwspec *fwsp
 	return iommu_ops_from_fwnode(fwspec ? fwspec->iommu_fwnode : NULL);
 }
 
-int iommu_group_replace_domain(struct iommu_group *group,
-			       struct iommu_domain *new_domain);
-
 int iommu_device_register_bus(struct iommu_device *iommu,
 			      const struct iommu_ops *ops,
 			      const struct bus_type *bus,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 715bdb8af31b..bee3ebc91ae4 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,
@@ -3558,9 +3532,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 v2 2/4] iommu: Drop iommu_group_replace_domain()

[Baolu Lu]

On 2025/2/21 22:33, Yi Liu wrote:
> iommufd does not use it now, so drop it.

iommufd still uses iommu_group_replace_domain() in Joerg's tree. This
could cause compiling issues if this series goes through the iommu tree.

$ git grep  iommu_group_replace_domain
drivers/iommu/iommu-priv.h:int iommu_group_replace_domain(struct 
iommu_group *group,
drivers/iommu/iommu.c: * iommu_group_replace_domain - replace the domain 
that a group is attached to
drivers/iommu/iommu.c:int iommu_group_replace_domain(struct iommu_group 
*group,
drivers/iommu/iommu.c:EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, 
"IOMMUFD_INTERNAL");
drivers/iommu/iommu.c: * This is a variant of 
iommu_group_replace_domain(). It allows the caller to
drivers/iommu/iommufd/iommufd_private.h:        return 
iommu_group_replace_domain(idev->igroup->group, hwpt->domain);

> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Nicolin Chen<nicolinc@nvidia.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommu-priv.h |  3 ---
>   drivers/iommu/iommu.c      | 35 ++++++-----------------------------
>   2 files changed, 6 insertions(+), 32 deletions(-)

Thanks,
baolu

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

[Yi Liu]

On 2025/2/23 21:18, Baolu Lu wrote:
> On 2025/2/21 22:33, Yi Liu wrote:
>> iommufd does not use it now, so drop it.
> 
> iommufd still uses iommu_group_replace_domain() in Joerg's tree. This
> could cause compiling issues if this series goes through the iommu tree.

you are right. This series is based on Jason's next tree which has included
Nic's patch that drops the iommu_group_replace_domain. Jason said he will
help to get a branch for Joerg.

https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/

> $ git grep  iommu_group_replace_domain
> drivers/iommu/iommu-priv.h:int iommu_group_replace_domain(struct 
> iommu_group *group,
> drivers/iommu/iommu.c: * iommu_group_replace_domain - replace the domain 
> that a group is attached to
> drivers/iommu/iommu.c:int iommu_group_replace_domain(struct iommu_group 
> *group,
> drivers/iommu/iommu.c:EXPORT_SYMBOL_NS_GPL(iommu_group_replace_domain, 
> "IOMMUFD_INTERNAL");
> drivers/iommu/iommu.c: * This is a variant of iommu_group_replace_domain(). 
> It allows the caller to
> drivers/iommu/iommufd/iommufd_private.h:        return 
> iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
> 
>> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
>> Reviewed-by: Nicolin Chen<nicolinc@nvidia.com>
>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu-priv.h |  3 ---
>>   drivers/iommu/iommu.c      | 35 ++++++-----------------------------
>>   2 files changed, 6 insertions(+), 32 deletions(-)
> 

-- 
Regards,
Yi Liu

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

[Jason Gunthorpe]

On Mon, Feb 24, 2025 at 12:40:44PM +0800, Yi Liu wrote:
> On 2025/2/23 21:18, Baolu Lu wrote:
> > On 2025/2/21 22:33, Yi Liu wrote:
> > > iommufd does not use it now, so drop it.
> > 
> > iommufd still uses iommu_group_replace_domain() in Joerg's tree. This
> > could cause compiling issues if this series goes through the iommu tree.
> 
> you are right. This series is based on Jason's next tree which has included
> Nic's patch that drops the iommu_group_replace_domain. Jason said he will
> help to get a branch for Joerg.
> 
> https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/

Yes

I need this series to take the other two pasid series anyhow..


Jason

[PATCH v2 3/4] 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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bee3ebc91ae4..eff5f678883b 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;
@@ -2147,6 +2150,17 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
 	return dev->iommu_group->default_domain;
 }
 
+static void *iommu_make_pasid_array_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);
+}
+
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group)
 {
@@ -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 *entry;
 	int ret;
 
 	if (!group)
@@ -3371,10 +3386,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	if (handle)
-		handle->domain = domain;
+	entry = iommu_make_pasid_array_entry(domain, handle);
 
-	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
+	ret = xa_insert(&group->pasid_array, 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 *entry;
 
 	xa_lock(&group->pasid_array);
-	handle = xa_load(&group->pasid_array, pasid);
-	if (!handle)
+	entry = xa_load(&group->pasid_array, pasid);
+	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
+		handle = xa_untag_pointer(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,14 +3501,15 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 			      struct iommu_group *group,
 			      struct iommu_attach_handle *handle)
 {
+	void *entry;
 	int ret;
 
 	if (!handle)
 		return -EINVAL;
 
 	mutex_lock(&group->mutex);
-	handle->domain = domain;
-	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+	entry = iommu_make_pasid_array_entry(domain, handle);
+	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
 	if (ret)
 		goto err_unlock;
 
@@ -3543,14 +3562,14 @@ int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle)
 {
-	void *curr;
+	void *curr, *entry;
 	int ret;
 
 	if (!new_domain || !handle)
 		return -EINVAL;
 
 	mutex_lock(&group->mutex);
-	handle->domain = new_domain;
+	entry = iommu_make_pasid_array_entry(new_domain, handle);
 	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
 	if (ret)
 		goto err_unlock;
@@ -3559,7 +3578,7 @@ 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, entry, GFP_KERNEL);
 	WARN_ON(xa_is_err(curr));
 
 	mutex_unlock(&group->mutex);
-- 
2.34.1

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

[Baolu Lu]

On 2025/2/21 22:33, Yi Liu wrote:
> @@ -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 *entry;
>   
>   	xa_lock(&group->pasid_array);
> -	handle = xa_load(&group->pasid_array, pasid);
> -	if (!handle)
> +	entry = xa_load(&group->pasid_array, pasid);

Could entry be NULL? If so, a null check might be needed here.

> +	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
> +		handle = xa_untag_pointer(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;

Thanks,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, February 23, 2025 9:23 PM
> 
> On 2025/2/21 22:33, Yi Liu wrote:
> > @@ -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 *entry;
> >
> >   	xa_lock(&group->pasid_array);
> > -	handle = xa_load(&group->pasid_array, pasid);
> > -	if (!handle)
> > +	entry = xa_load(&group->pasid_array, pasid);
> 
> Could entry be NULL? If so, a null check might be needed here.

a NULL entry will go to the else branch in following check hence
get an -ENOENT error.

> 
> > +	if (xa_pointer_tag(entry) == IOMMU_PASID_ARRAY_HANDLE) {
> > +		handle = xa_untag_pointer(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;
> 
> Thanks,
> baolu

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

[Baolu Lu]

On 2/24/25 10:38, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Sunday, February 23, 2025 9:23 PM
>>
>> On 2025/2/21 22:33, Yi Liu wrote:
>>> @@ -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 *entry;
>>>
>>>    	xa_lock(&group->pasid_array);
>>> -	handle = xa_load(&group->pasid_array, pasid);
>>> -	if (!handle)
>>> +	entry = xa_load(&group->pasid_array, pasid);
>> Could entry be NULL? If so, a null check might be needed here.
> a NULL entry will go to the else branch in following check hence
> get an -ENOENT error.

xa_pointer_tag() implicitly returns 0 when the entry is a NULL. I
believe this behavior is unintended. The comments of this helper says,

  * If you have stored a tagged pointer in the XArray, call this function
  * to get the tag of that pointer.

So I feel that it's a better code style if we check entry before calling
xa_pointer_tag() to make the logic explicit.

Thanks,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, February 24, 2025 10:58 AM
> 
> On 2/24/25 10:38, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@linux.intel.com>
> >> Sent: Sunday, February 23, 2025 9:23 PM
> >>
> >> On 2025/2/21 22:33, Yi Liu wrote:
> >>> @@ -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 *entry;
> >>>
> >>>    	xa_lock(&group->pasid_array);
> >>> -	handle = xa_load(&group->pasid_array, pasid);
> >>> -	if (!handle)
> >>> +	entry = xa_load(&group->pasid_array, pasid);
> >> Could entry be NULL? If so, a null check might be needed here.
> > a NULL entry will go to the else branch in following check hence
> > get an -ENOENT error.
> 
> xa_pointer_tag() implicitly returns 0 when the entry is a NULL. I
> believe this behavior is unintended. The comments of this helper says,
> 
>   * If you have stored a tagged pointer in the XArray, call this function
>   * to get the tag of that pointer.
> 
> So I feel that it's a better code style if we check entry before calling
> xa_pointer_tag() to make the logic explicit.
> 

Okay that makes sense.

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

[Yi Liu]

On 2025/2/24 11:04, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, February 24, 2025 10:58 AM
>>
>> On 2/24/25 10:38, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Sunday, February 23, 2025 9:23 PM
>>>>
>>>> On 2025/2/21 22:33, Yi Liu wrote:
>>>>> @@ -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 *entry;
>>>>>
>>>>>     	xa_lock(&group->pasid_array);
>>>>> -	handle = xa_load(&group->pasid_array, pasid);
>>>>> -	if (!handle)
>>>>> +	entry = xa_load(&group->pasid_array, pasid);
>>>> Could entry be NULL? If so, a null check might be needed here.
>>> a NULL entry will go to the else branch in following check hence
>>> get an -ENOENT error.
>>
>> xa_pointer_tag() implicitly returns 0 when the entry is a NULL. I
>> believe this behavior is unintended. The comments of this helper says,
>>
>>    * If you have stored a tagged pointer in the XArray, call this function
>>    * to get the tag of that pointer.
>>
>> So I feel that it's a better code style if we check entry before calling
>> xa_pointer_tag() to make the logic explicit.
>>
> 
> Okay that makes sense.

I see the point, will the code like below. thanks.

	xa_lock(&group->pasid_array);
	entry = xa_load(&group->pasid_array, pasid);
	if (!entry || (xa_pointer_tag(entry) != IOMMU_PASID_ARRAY_HANDLE))
		handle = ERR_PTR(-ENOENT);

	handle = xa_untag_pointer(entry);
	if (type && handle->domain->type != type)
		handle = ERR_PTR(-EBUSY);
	xa_unlock(&group->pasid_array);

	return handle;

-- 
Regards,
Yi Liu

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

[Yi Liu]

The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index eff5f678883b..73555e1cf016 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 
 	entry = iommu_make_pasid_array_entry(domain, handle);
 
-	ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
+	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_erase(&group->pasid_array, 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.
+	 */
+	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
+				   pasid, entry, GFP_KERNEL)));
+
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
@@ -3509,19 +3521,27 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
 
 	mutex_lock(&group->mutex);
 	entry = iommu_make_pasid_array_entry(domain, handle);
-	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
+	ret = xa_insert(&group->pasid_array,
+			IOMMU_NO_PASID, XA_ZERO_ENTRY, GFP_KERNEL);
 	if (ret)
-		goto err_unlock;
+		goto out_unlock;
 
 	ret = __iommu_attach_group(domain, group);
-	if (ret)
-		goto err_erase;
-	mutex_unlock(&group->mutex);
+	if (ret) {
+		xa_release(&group->pasid_array, IOMMU_NO_PASID);
+		goto out_unlock;
+	}
 
-	return 0;
-err_erase:
-	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
-err_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.
+	 */
+	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
+				   IOMMU_NO_PASID, entry, GFP_KERNEL)));
+
+out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
 }
-- 
2.34.1

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

[Nicolin Chen]

On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

A nit:

> ---
>  drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index eff5f678883b..73555e1cf016 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>  
>  	entry = iommu_make_pasid_array_entry(domain, handle);
>  
> -	ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
> +	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);

Maybe xa_reserve() pairing xa_release()? Same thing though..

>  	if (ret)
>  		goto out_unlock;
>  
>  	ret = __iommu_set_group_pasid(domain, group, pasid);
> -	if (ret)
> -		xa_erase(&group->pasid_array, pasid);
> +	if (ret) {
> +		xa_release(&group->pasid_array, pasid);
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * The xa_insert() above reserved the memory, and the group->mutex is

Just in case we would like update the patch, reword "xa_insert".

> +	 * 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.
> +	 */
> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
> +				   pasid, entry, GFP_KERNEL)));
> +
>  out_unlock:
>  	mutex_unlock(&group->mutex);
>  	return ret;
> @@ -3509,19 +3521,27 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>  
>  	mutex_lock(&group->mutex);
>  	entry = iommu_make_pasid_array_entry(domain, handle);
> -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
> +	ret = xa_insert(&group->pasid_array,
> +			IOMMU_NO_PASID, XA_ZERO_ENTRY, GFP_KERNEL);

Ditto.

>  	if (ret)
> -		goto err_unlock;
> +		goto out_unlock;
>  
>  	ret = __iommu_attach_group(domain, group);
> -	if (ret)
> -		goto err_erase;
> -	mutex_unlock(&group->mutex);
> +	if (ret) {
> +		xa_release(&group->pasid_array, IOMMU_NO_PASID);
> +		goto out_unlock;
> +	}
>  
> -	return 0;
> -err_erase:
> -	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
> -err_unlock:
> +	/*
> +	 * The xa_insert() above reserved the memory, and the group->mutex is

"xa_insert" here too.

Thanks
Nicolin

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

[Yi Liu]

On 2025/2/22 01:27, Nicolin Chen wrote:
> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> A nit:
> 
>> ---
>>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index eff5f678883b..73555e1cf016 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   
>>   	entry = iommu_make_pasid_array_entry(domain, handle);
>>   
>> -	ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>> +	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
> 
> Maybe xa_reserve() pairing xa_release()? Same thing though..

there is slight difference. xa_reserve() will return 0 if there is already
a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
failures here. You may notice that the replace function uses xa_reserve()
as it intend to replace existing entry.

>>   	if (ret)
>>   		goto out_unlock;
>>   
>>   	ret = __iommu_set_group_pasid(domain, group, pasid);
>> -	if (ret)
>> -		xa_erase(&group->pasid_array, pasid);
>> +	if (ret) {
>> +		xa_release(&group->pasid_array, pasid);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/*
>> +	 * The xa_insert() above reserved the memory, and the group->mutex is
> 
> Just in case we would like update the patch, reword "xa_insert".
> 
>> +	 * 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.
>> +	 */
>> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
>> +				   pasid, entry, GFP_KERNEL)));
>> +
>>   out_unlock:
>>   	mutex_unlock(&group->mutex);
>>   	return ret;
>> @@ -3509,19 +3521,27 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   
>>   	mutex_lock(&group->mutex);
>>   	entry = iommu_make_pasid_array_entry(domain, handle);
>> -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
>> +	ret = xa_insert(&group->pasid_array,
>> +			IOMMU_NO_PASID, XA_ZERO_ENTRY, GFP_KERNEL);
> 
> Ditto.
> 
>>   	if (ret)
>> -		goto err_unlock;
>> +		goto out_unlock;
>>   
>>   	ret = __iommu_attach_group(domain, group);
>> -	if (ret)
>> -		goto err_erase;
>> -	mutex_unlock(&group->mutex);
>> +	if (ret) {
>> +		xa_release(&group->pasid_array, IOMMU_NO_PASID);
>> +		goto out_unlock;
>> +	}
>>   
>> -	return 0;
>> -err_erase:
>> -	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
>> -err_unlock:
>> +	/*
>> +	 * The xa_insert() above reserved the memory, and the group->mutex is
> 
> "xa_insert" here too.
> 
> Thanks
> Nicolin
> 

-- 
Regards,
Yi Liu

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

[Nicolin Chen]

On Sat, Feb 22, 2025 at 09:59:05AM +0800, Yi Liu wrote:
> On 2025/2/22 01:27, Nicolin Chen wrote:
> > On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
> > > The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
> > > 
> > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > 
> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

> > > @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> > >   	entry = iommu_make_pasid_array_entry(domain, handle);
> > > -	ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
> > > +	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
> > 
> > Maybe xa_reserve() pairing xa_release()? Same thing though..
> 
> there is slight difference. xa_reserve() will return 0 if there is already
> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
> failures here. You may notice that the replace function uses xa_reserve()
> as it intend to replace existing entry.

I see. There is an xa_zero_to_null down in the xa_reserve path.
So, they are different. And there's no need to change anything.

Thanks
Nic

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

[Baolu Lu]

On 2025/2/22 9:59, Yi Liu wrote:
> On 2025/2/22 01:27, Nicolin Chen wrote:
>> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>>
>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>
>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>
>> A nit:
>>
>>> ---
>>>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index eff5f678883b..73555e1cf016 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct 
>>> iommu_domain *domain,
>>>       entry = iommu_make_pasid_array_entry(domain, handle);
>>> -    ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>>> +    ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, 
>>> GFP_KERNEL);
>>
>> Maybe xa_reserve() pairing xa_release()? Same thing though..
> 
> there is slight difference. xa_reserve() will return 0 if there is already
> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
> failures here. You may notice that the replace function uses xa_reserve()
> as it intend to replace existing entry.

Do you mind adding some comments around the code explaining why
xa_insert() is used instead of xa_reserve() here? This will make the
code easier to understand without needing to refer to this discussion.

Thanks,
baolu

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

[Yi Liu]

On 2025/2/23 21:32, Baolu Lu wrote:
> On 2025/2/22 9:59, Yi Liu wrote:
>> On 2025/2/22 01:27, Nicolin Chen wrote:
>>> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>>>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>>>
>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>
>>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>>
>>> A nit:
>>>
>>>> ---
>>>>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>>>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index eff5f678883b..73555e1cf016 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct 
>>>> iommu_domain *domain,
>>>>       entry = iommu_make_pasid_array_entry(domain, handle);
>>>> -    ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>>>> +    ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, 
>>>> GFP_KERNEL);
>>>
>>> Maybe xa_reserve() pairing xa_release()? Same thing though..
>>
>> there is slight difference. xa_reserve() will return 0 if there is already
>> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
>> failures here. You may notice that the replace function uses xa_reserve()
>> as it intend to replace existing entry.
> 
> Do you mind adding some comments around the code explaining why
> xa_insert() is used instead of xa_reserve() here? This will make the
> code easier to understand without needing to refer to this discussion.

hmmm. do you think it is enough when referring to the kdoc of xa_insert()?
It notes it acts just like xa_reserve() when no stored or reserved entry.
It fails if there is another present entry, and this suits the need here
since it does not intend to replace existing entry.


   * Inserting a NULL entry will store a reserved entry (like xa_reserve())
   * if no entry is present.  Inserting will fail if a reserved entry is
   * present, even though loading from this index will return NULL.

   * Return: 0 if the store succeeded.  -EBUSY if another entry was present.
   * -ENOMEM if memory could not be allocated.

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 2/24/25 12:38, Yi Liu wrote:
> On 2025/2/23 21:32, Baolu Lu wrote:
>> On 2025/2/22 9:59, Yi Liu wrote:
>>> On 2025/2/22 01:27, Nicolin Chen wrote:
>>>> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>>>>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>>>>
>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>
>>>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>>>
>>>> A nit:
>>>>
>>>>> ---
>>>>>   drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++ 
>>>>> +------------
>>>>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>> index eff5f678883b..73555e1cf016 100644
>>>>> --- a/drivers/iommu/iommu.c
>>>>> +++ b/drivers/iommu/iommu.c
>>>>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct 
>>>>> iommu_domain *domain,
>>>>>       entry = iommu_make_pasid_array_entry(domain, handle);
>>>>> -    ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>>>>> +    ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, 
>>>>> GFP_KERNEL);
>>>>
>>>> Maybe xa_reserve() pairing xa_release()? Same thing though..
>>>
>>> there is slight difference. xa_reserve() will return 0 if there is 
>>> already
>>> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
>>> failures here. You may notice that the replace function uses 
>>> xa_reserve()
>>> as it intend to replace existing entry.
>>
>> Do you mind adding some comments around the code explaining why
>> xa_insert() is used instead of xa_reserve() here? This will make the
>> code easier to understand without needing to refer to this discussion.
> 
> hmmm. do you think it is enough when referring to the kdoc of xa_insert()?
> It notes it acts just like xa_reserve() when no stored or reserved entry.
> It fails if there is another present entry, and this suits the need here
> since it does not intend to replace existing entry.
> 
> 
>    * Inserting a NULL entry will store a reserved entry (like xa_reserve())
>    * if no entry is present.  Inserting will fail if a reserved entry is
>    * present, even though loading from this index will return NULL.
> 
>    * Return: 0 if the store succeeded.  -EBUSY if another entry was 
> present.
>    * -ENOMEM if memory could not be allocated.

I feel that it's enough if we add something like:

	/*
	 * Entry present is a failure case. Use xa_insert() instead of
	 * xa_reserve().
	 */

Thanks,
baolu

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

[Yi Liu]

On 2025/2/24 13:07, Baolu Lu wrote:
> On 2/24/25 12:38, Yi Liu wrote:
>> On 2025/2/23 21:32, Baolu Lu wrote:
>>> On 2025/2/22 9:59, Yi Liu wrote:
>>>> On 2025/2/22 01:27, Nicolin Chen wrote:
>>>>> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>>>>>> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
>>>>>>
>>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>
>>>>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>>>>
>>>>> A nit:
>>>>>
>>>>>> ---
>>>>>>   drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++++++++ 
>>>>>> +------------
>>>>>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>>> index eff5f678883b..73555e1cf016 100644
>>>>>> --- a/drivers/iommu/iommu.c
>>>>>> +++ b/drivers/iommu/iommu.c
>>>>>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct 
>>>>>> iommu_domain *domain,
>>>>>>       entry = iommu_make_pasid_array_entry(domain, handle);
>>>>>> -    ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>>>>>> +    ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, 
>>>>>> GFP_KERNEL);
>>>>>
>>>>> Maybe xa_reserve() pairing xa_release()? Same thing though..
>>>>
>>>> there is slight difference. xa_reserve() will return 0 if there is already
>>>> a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
>>>> failures here. You may notice that the replace function uses xa_reserve()
>>>> as it intend to replace existing entry.
>>>
>>> Do you mind adding some comments around the code explaining why
>>> xa_insert() is used instead of xa_reserve() here? This will make the
>>> code easier to understand without needing to refer to this discussion.
>>
>> hmmm. do you think it is enough when referring to the kdoc of xa_insert()?
>> It notes it acts just like xa_reserve() when no stored or reserved entry.
>> It fails if there is another present entry, and this suits the need here
>> since it does not intend to replace existing entry.
>>
>>
>>    * Inserting a NULL entry will store a reserved entry (like xa_reserve())
>>    * if no entry is present.  Inserting will fail if a reserved entry is
>>    * present, even though loading from this index will return NULL.
>>
>>    * Return: 0 if the store succeeded.  -EBUSY if another entry was present.
>>    * -ENOMEM if memory could not be allocated.
> 
> I feel that it's enough if we add something like:
> 
>      /*
>       * Entry present is a failure case. Use xa_insert() instead of
>       * xa_reserve().
>       */
got it.

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 2025/2/21 22:33, Yi Liu wrote:
> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
> 
> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>   1 file changed, 32 insertions(+), 12 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

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

[Jason Gunthorpe]

On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
> The current implementation stores entry to the 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 underlying iommu driver before updating the group->pasid_array.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)

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

Jason

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

[Jason Gunthorpe]

On Fri, Feb 21, 2025 at 06:33:31AM -0800, Yi Liu wrote:
> Yi Liu (4):
>   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

Joerg, this also needs some of the iommufd patches on my tree, I can
pick it up and send you a PR with this and the MSI stuff. OK?

Thanks,
Jason

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

[Yi Liu]

On 2025/2/25 08:13, Jason Gunthorpe wrote:
> On Fri, Feb 21, 2025 at 06:33:31AM -0800, Yi Liu wrote:
>> Yi Liu (4):
>>    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
> 
> Joerg, this also needs some of the iommufd patches on my tree, I can
> pick it up and send you a PR with this and the MSI stuff. OK?

I'll send a v3 given Baolu's comment.

-- 
Regards,
Yi Liu

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

[Joerg Roedel]

On Mon, Feb 24, 2025 at 08:13:17PM -0400, Jason Gunthorpe wrote:
> Joerg, this also needs some of the iommufd patches on my tree, I can
> pick it up and send you a PR with this and the MSI stuff. OK?

Sounds good. V3 seems to be posted now.

Regards,

	Joerg