[PATCH 0/2] Two enhancements to iommu_at[de]tach_device_pasid()

[PATCH 0/2] Two enhancements to iommu_at[de]tach_device_pasid()

[Yi Liu]

There are minor mistakes in the iommu set_dev_pasid() and remove_dev_pasid()
paths. The set_dev_pasid() path updates the group->pasid_array first, and
then may call into remove_dev_pasid() in error handling when there are devices
within the group failed to set_dev_pasid. The remove_dev_pasid() callbacks of
the underlying iommu drivers get domain for pasid from the group->pasid_array.
So the remove_dev_pasid() callback may get a wrong domain in the set_dev_pasid()
path. [1] Even the group is singleton, the existing code logic would have
unnecessary warnings in the error handling of the set_dev_pasid() path.

The fix is passing the domain to be detached to the remove_dev_pasid()
callback, and only undo the set_dev_pasid result on the devices that have
succeeded.

[1] https://lore.kernel.org/linux-iommu/20240320123803.GD159172@nvidia.com/

Regards,
	Yi Liu

Yi Liu (2):
  iommu: Pass domain to remove_dev_pasid() op
  iommu: Undo pasid attachment only for the devices that have succeeded

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++-----
 drivers/iommu/intel/iommu.c                 | 11 +++------
 drivers/iommu/iommu.c                       | 27 ++++++++++++++-------
 include/linux/iommu.h                       |  3 ++-
 4 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Yi Liu]

Existing remove_dev_pasid() callbacks of the underlying iommu drivers get
the attached domain from the group->pasid_array. However, the domains
stored in group->pasid_array are not always correct. For example, the
set_dev_pasid() path updates group->pasid_array first and then invoke
remove_dev_pasid() callback when error happened. The remove_dev_pasid()
callback would get the updated domain. This is not correct for the
devices that are still attached with an old domain or just no attached
domain.

To avoid the above problem, passing the attached domain to the
remove_dev_pasid() callback is more reliable.

Fixes: 386fa64fd52baadb ("arm-smmu-v3/sva: Add SVA domain support")
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++-------
 drivers/iommu/intel/iommu.c                 | 11 +++--------
 drivers/iommu/iommu.c                       |  9 +++++----
 include/linux/iommu.h                       |  3 ++-
 4 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 5ed036225e69..ced041777ec0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3044,14 +3044,9 @@ static int arm_smmu_def_domain_type(struct device *dev)
 	return 0;
 }
 
-static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
+static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *domain)
 {
-	struct iommu_domain *domain;
-
-	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
-	if (WARN_ON(IS_ERR(domain)) || !domain)
-		return;
-
 	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
 }
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 50eb9aed47cc..45c75a8a0ef5 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4587,19 +4587,15 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	return 0;
 }
 
-static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *domain)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 	struct dev_pasid_info *curr, *dev_pasid = NULL;
 	struct intel_iommu *iommu = info->iommu;
-	struct dmar_domain *dmar_domain;
-	struct iommu_domain *domain;
 	unsigned long flags;
 
-	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
-	if (WARN_ON_ONCE(!domain))
-		goto out_tear_down;
-
 	/*
 	 * The SVA implementation needs to handle its own stuffs like the mm
 	 * notification. Before consolidating that code into iommu core, let
@@ -4610,7 +4606,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 		goto out_tear_down;
 	}
 
-	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
 		if (curr->dev == dev && curr->pasid == pasid) {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 098869007c69..681e916d285b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3330,14 +3330,15 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 }
 
 static void __iommu_remove_group_pasid(struct iommu_group *group,
-				       ioasid_t pasid)
+				       ioasid_t pasid,
+				       struct iommu_domain *domain)
 {
 	struct group_device *device;
 	const struct iommu_ops *ops;
 
 	for_each_group_device(group, device) {
 		ops = dev_iommu_ops(device->dev);
-		ops->remove_dev_pasid(device->dev, pasid);
+		ops->remove_dev_pasid(device->dev, pasid, domain);
 	}
 }
 
@@ -3375,7 +3376,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 
 	ret = __iommu_set_group_pasid(domain, group, pasid);
 	if (ret) {
-		__iommu_remove_group_pasid(group, pasid);
+		__iommu_remove_group_pasid(group, pasid, domain);
 		xa_erase(&group->pasid_array, pasid);
 	}
 out_unlock:
@@ -3400,7 +3401,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
 	struct iommu_group *group = dev->iommu_group;
 
 	mutex_lock(&group->mutex);
-	__iommu_remove_group_pasid(group, pasid);
+	__iommu_remove_group_pasid(group, pasid, domain);
 	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
 	mutex_unlock(&group->mutex);
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2e925b5eba53..40dd439307e8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -578,7 +578,8 @@ struct iommu_ops {
 			      struct iommu_page_response *msg);
 
 	int (*def_domain_type)(struct device *dev);
-	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
+	void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid,
+				 struct iommu_domain *domain);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
-- 
2.34.1

Re: [PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Jason Gunthorpe]

On Wed, Mar 27, 2024 at 05:54:32AM -0700, Yi Liu wrote:
> Existing remove_dev_pasid() callbacks of the underlying iommu drivers get
> the attached domain from the group->pasid_array. However, the domains
> stored in group->pasid_array are not always correct. For example, the
> set_dev_pasid() path updates group->pasid_array first and then invoke
> remove_dev_pasid() callback when error happened. The remove_dev_pasid()
> callback would get the updated domain. This is not correct for the
> devices that are still attached with an old domain or just no attached
> domain.
> 
> To avoid the above problem, passing the attached domain to the
> remove_dev_pasid() callback is more reliable.

I've relaized we have the same issue with set_dev_pasid, there is no
way for the driver to get the old domain since the xarray was updated
before calling set_dev_pasid. This is unlike the RID path. Meaning
drivers can't implement PASID replace.

So we need another patch to pass the old domain into set_dev_pasid
too...

This looks fine

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

Jason

Re: [PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Yi Liu]

On 2024/3/27 21:02, Jason Gunthorpe wrote:
> On Wed, Mar 27, 2024 at 05:54:32AM -0700, Yi Liu wrote:
>> Existing remove_dev_pasid() callbacks of the underlying iommu drivers get
>> the attached domain from the group->pasid_array. However, the domains
>> stored in group->pasid_array are not always correct. For example, the
>> set_dev_pasid() path updates group->pasid_array first and then invoke
>> remove_dev_pasid() callback when error happened. The remove_dev_pasid()
>> callback would get the updated domain. This is not correct for the
>> devices that are still attached with an old domain or just no attached
>> domain.
>>
>> To avoid the above problem, passing the attached domain to the
>> remove_dev_pasid() callback is more reliable.
> 
> I've relaized we have the same issue with set_dev_pasid, there is no
> way for the driver to get the old domain since the xarray was updated
> before calling set_dev_pasid. This is unlike the RID path. Meaning
> drivers can't implement PASID replace.
> 
> So we need another patch to pass the old domain into set_dev_pasid
> too...

yes. you've given this suggestion in below. :) I've already in this way.
Will send it out together with other iommufd pasid attach/replace/detach
patches.

https://lore.kernel.org/linux-iommu/20240318165247.GD5825@nvidia.com/

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

thanks.

-- 
Regards,
Yi Liu

Re: [PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Yi Liu]

On 2024/3/27 21:02, Jason Gunthorpe wrote:
> On Wed, Mar 27, 2024 at 05:54:32AM -0700, Yi Liu wrote:
>> Existing remove_dev_pasid() callbacks of the underlying iommu drivers get
>> the attached domain from the group->pasid_array. However, the domains
>> stored in group->pasid_array are not always correct. For example, the
>> set_dev_pasid() path updates group->pasid_array first and then invoke
>> remove_dev_pasid() callback when error happened. The remove_dev_pasid()
>> callback would get the updated domain. This is not correct for the
>> devices that are still attached with an old domain or just no attached
>> domain.
>>
>> To avoid the above problem, passing the attached domain to the
>> remove_dev_pasid() callback is more reliable.
> 
> I've relaized we have the same issue with set_dev_pasid, there is no
> way for the driver to get the old domain since the xarray was updated
> before calling set_dev_pasid. This is unlike the RID path. Meaning
> drivers can't implement PASID replace.
> 
> So we need another patch to pass the old domain into set_dev_pasid
> too...
> 
> This looks fine
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

I dropped your r-b as the description changed. Please have another look
at the v2 of this patch. :)

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

-- 
Regards,
Yi Liu

RE: [PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 27, 2024 8:55 PM
> 
> @@ -3375,7 +3376,7 @@ int iommu_attach_device_pasid(struct
> iommu_domain *domain,
> 
>  	ret = __iommu_set_group_pasid(domain, group, pasid);
>  	if (ret) {
> -		__iommu_remove_group_pasid(group, pasid);
> +		__iommu_remove_group_pasid(group, pasid, domain);
>  		xa_erase(&group->pasid_array, pasid);

I didn't get why this patch alone fixes anything. You are passing the
new domain which is same as original code which gets it from
xarray.

so it is at most a non-functional refactoring with the next patch
doing real fix?

Re: [PATCH 1/2] iommu: Pass domain to remove_dev_pasid() op

[Yi Liu]

On 2024/3/28 11:12, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, March 27, 2024 8:55 PM
>>
>> @@ -3375,7 +3376,7 @@ int iommu_attach_device_pasid(struct
>> iommu_domain *domain,
>>
>>   	ret = __iommu_set_group_pasid(domain, group, pasid);
>>   	if (ret) {
>> -		__iommu_remove_group_pasid(group, pasid);
>> +		__iommu_remove_group_pasid(group, pasid, domain);
>>   		xa_erase(&group->pasid_array, pasid);
> 
> I didn't get why this patch alone fixes anything. You are passing the
> new domain which is same as original code which gets it from
> xarray.

you are right. This is to avoid getting domain from xarray in
remove_dev_pasid(). But this part is still the same with the
original code.

> so it is at most a non-functional refactoring with the next patch
> doing real fix?

yes. let me make it clearer.

-- 
Regards,
Yi Liu

[PATCH 2/2] iommu: Undo pasid attachment only for the devices that have succeeded

[Yi Liu]

There is no error handling now in __iommu_set_group_pasid(), it relies on
its caller to loop all the devices to undo the pasid attachment. This is
not self-contained and has unnecessary remove_dev_pasid() calls on the
devices that have not changed in the __iommu_set_group_pasid() call. this
results in unnecessary warnings by the underlying iommu drivers. Like the
Intel iommu driver, it would warn when there is no pasid attachment to
destroy in the remove_dev_pasid() callback.

The ideal way is to handle the error within __iommu_set_group_pasid(). This
not only makes __iommu_set_group_pasid() self-contained, but also avoids
unnecessary warnings.

Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 681e916d285b..2a12c9c9e045 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3317,15 +3317,25 @@ EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
 				   struct iommu_group *group, ioasid_t pasid)
 {
-	struct group_device *device;
-	int ret = 0;
+	struct group_device *device, *last_gdev;
+	int ret;
 
 	for_each_group_device(group, device) {
 		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
 		if (ret)
-			break;
+			goto err_revert;
 	}
 
+	return 0;
+
+err_revert:
+	last_gdev = device;
+	for_each_group_device(group, device) {
+		if (device == last_gdev)
+			break;
+		dev_iommu_ops(device->dev)->remove_dev_pasid(device->dev,
+							     pasid, domain);
+	}
 	return ret;
 }
 
@@ -3375,10 +3385,8 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	}
 
 	ret = __iommu_set_group_pasid(domain, group, pasid);
-	if (ret) {
-		__iommu_remove_group_pasid(group, pasid, domain);
+	if (ret)
 		xa_erase(&group->pasid_array, pasid);
-	}
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.34.1

Re: [PATCH 2/2] iommu: Undo pasid attachment only for the devices that have succeeded

[Jason Gunthorpe]

On Wed, Mar 27, 2024 at 05:54:33AM -0700, Yi Liu wrote:
> There is no error handling now in __iommu_set_group_pasid(), it relies on
> its caller to loop all the devices to undo the pasid attachment. This is
> not self-contained and has unnecessary remove_dev_pasid() calls on the
> devices that have not changed in the __iommu_set_group_pasid() call. this
> results in unnecessary warnings by the underlying iommu drivers. Like the
> Intel iommu driver, it would warn when there is no pasid attachment to
> destroy in the remove_dev_pasid() callback.
> 
> The ideal way is to handle the error within __iommu_set_group_pasid(). This
> not only makes __iommu_set_group_pasid() self-contained, but also avoids
> unnecessary warnings.
> 
> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

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

This will need revising when we get to PASID replace, but good enough
for now

Thanks,
Jason

Re: [PATCH 2/2] iommu: Undo pasid attachment only for the devices that have succeeded

[Yi Liu]

On 2024/3/27 21:05, Jason Gunthorpe wrote:
> On Wed, Mar 27, 2024 at 05:54:33AM -0700, Yi Liu wrote:
>> There is no error handling now in __iommu_set_group_pasid(), it relies on
>> its caller to loop all the devices to undo the pasid attachment. This is
>> not self-contained and has unnecessary remove_dev_pasid() calls on the
>> devices that have not changed in the __iommu_set_group_pasid() call. this
>> results in unnecessary warnings by the underlying iommu drivers. Like the
>> Intel iommu driver, it would warn when there is no pasid attachment to
>> destroy in the remove_dev_pasid() callback.
>>
>> The ideal way is to handle the error within __iommu_set_group_pasid(). This
>> not only makes __iommu_set_group_pasid() self-contained, but also avoids
>> unnecessary warnings.
>>
>> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> This will need revising when we get to PASID replace, but good enough
> for now

yes, the rollback part would be revised a bit when coming to support
PASID domain replacement. Already in the pending list, will send it
out soon. :)

-- 
Regards,
Yi Liu

RE: [PATCH 2/2] iommu: Undo pasid attachment only for the devices that have succeeded

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 27, 2024 8:55 PM
> +
> +err_revert:
> +	last_gdev = device;
> +	for_each_group_device(group, device) {
> +		if (device == last_gdev)
> +			break;
> +		dev_iommu_ops(device->dev)->remove_dev_pasid(device-
> >dev,
> +							     pasid, domain);
> +	}

break the long line into:
	ops = dev_iommu_ops(device->dev);
	ops->remove_dev_pasid();

otherwise it looks good to me:

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

Re: [PATCH 2/2] iommu: Undo pasid attachment only for the devices that have succeeded

[Yi Liu]

On 2024/3/28 11:15, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, March 27, 2024 8:55 PM
>> +
>> +err_revert:
>> +	last_gdev = device;
>> +	for_each_group_device(group, device) {
>> +		if (device == last_gdev)
>> +			break;
>> +		dev_iommu_ops(device->dev)->remove_dev_pasid(device-
>>> dev,
>> +							     pasid, domain);
>> +	}
> 
> break the long line into:
> 	ops = dev_iommu_ops(device->dev);
> 	ops->remove_dev_pasid();

got it.

> otherwise it looks good to me:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

-- 
Regards,
Yi Liu