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

[PATCH v2 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 call into remove_dev_pasid() in error handling when there are devices
within the group that failed to set_dev_pasid. The remove_dev_pasid()
callbacks of the underlying iommu drivers get the 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 if the group is singleton, the existing
code logic would have unnecessary warnings in the error handling of the
set_dev_pasid() path. e.g. intel iommu driver.

The above issue can be fixed by improving the error handling in the
set_dev_pasid() path. Also, this reminds that it is not reliable for the
underlying iommu driver callback to get the domain from group->pasid_array.
So, the second patch of this series passes the domain to remove_dev_pasid
op.

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

Change log:

v2:
 - Make clear that the patch 1/2 of v1 does not fix the problem (Kevin)
 - Swap the order of patch 1/2 and 2/2 of v1. In this new series, patch 1/2
   fixes the real issue, patch 2/2 is to avoid potential issue in the future.

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

Regards,
	Yi Liu

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

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

-- 
2.34.1

[PATCH v2 1/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 drawbacks. It would result in unnecessary
remove_dev_pasid() calls on the devices that have not been attached to the
new domain. But the remove_dev_pasid() callback would get the new domain
from the group->pasid_array. So for such devices, the iommu driver won't
find the attachment under the domain, hence unable to do cleanup. This may
not be a real problem today. But it depends on the implementation of the
underlying iommu driver. e.g. the intel iommu driver would warn for such
devices. Such warnings are unnecessary.

To solve the above problem, it is necessary to handle the error within
__iommu_set_group_pasid(). It only loops the devices that have attached
to the new domain, and undo it.

Fixes: 16603704559c ("iommu: Add attach/detach_dev_pasid iommu interfaces")
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-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 | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 098869007c69..bb3244df525e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3317,15 +3317,26 @@ 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) {
+		const struct iommu_ops *ops = dev_iommu_ops(device->dev);
+
+		if (device == last_gdev)
+			break;
+		ops->remove_dev_pasid(device->dev, pasid);
+	}
 	return ret;
 }
 
@@ -3374,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);
+	if (ret)
 		xa_erase(&group->pasid_array, pasid);
-	}
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.34.1

[PATCH v2 2/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 domain
stored in group->pasid_array is not always correct in all scenarios.
A wrong domain may result in failure in remove_dev_pasid() callback.
To avoid such problems, it is more reliable to pass the domain to the
remove_dev_pasid() op.

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 bb3244df525e..c0b615f68a75 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3335,20 +3335,21 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 
 		if (device == last_gdev)
 			break;
-		ops->remove_dev_pasid(device->dev, pasid);
+		ops->remove_dev_pasid(device->dev, pasid, domain);
 	}
 	return ret;
 }
 
 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);
 	}
 }
 
@@ -3409,7 +3410,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 v2 0/2] Two enhancements to iommu_at[de]tach_device_pasid()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: [PATCH v2 0/2] Two enhancements to
>iommu_at[de]tach_device_pasid()
>
>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 call into remove_dev_pasid() in error handling when there are devices
>within the group that failed to set_dev_pasid.

Not related to this patch, just curious in which cases some of the devices
In same group failed to set_dev_pasid while others succeed?

Thanks
Zhenzhong

> The remove_dev_pasid()
>callbacks of the underlying iommu drivers get the 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 if the group is singleton, the existing
>code logic would have unnecessary warnings in the error handling of the
>set_dev_pasid() path. e.g. intel iommu driver.
>
>The above issue can be fixed by improving the error handling in the
>set_dev_pasid() path. Also, this reminds that it is not reliable for the
>underlying iommu driver callback to get the domain from group-
>>pasid_array.
>So, the second patch of this series passes the domain to remove_dev_pasid
>op.
>
>[1] https://lore.kernel.org/linux-
>iommu/20240320123803.GD159172@nvidia.com/
>
>Change log:
>
>v2:
> - Make clear that the patch 1/2 of v1 does not fix the problem (Kevin)
> - Swap the order of patch 1/2 and 2/2 of v1. In this new series, patch 1/2
>   fixes the real issue, patch 2/2 is to avoid potential issue in the future.
>
>v1: https://lore.kernel.org/linux-iommu/20240327125433.248946-1-
>yi.l.liu@intel.com/
>
>Regards,
>	Yi Liu
>
>Yi Liu (2):
>  iommu: Undo pasid attachment only for the devices that have succeeded
>  iommu: Pass domain to remove_dev_pasid() op
>
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++-----
> drivers/iommu/intel/iommu.c                 | 11 +++-----
> drivers/iommu/iommu.c                       | 28 ++++++++++++++-------
> include/linux/iommu.h                       |  3 ++-
> 4 files changed, 26 insertions(+), 25 deletions(-)
>
>--
>2.34.1

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

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, March 28, 2024 8:30 PM
> 
> Existing remove_dev_pasid() callbacks of the underlying iommu drivers
> get the attached domain from the group->pasid_array. However, the domain
> stored in group->pasid_array is not always correct in all scenarios.
> A wrong domain may result in failure in remove_dev_pasid() callback.
> To avoid such problems, it is more reliable to pass the domain to the
> remove_dev_pasid() op.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

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

[Yi Liu]

On 2024/3/29 10:12, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Subject: [PATCH v2 0/2] Two enhancements to
>> iommu_at[de]tach_device_pasid()
>>
>> 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 call into remove_dev_pasid() in error handling when there are devices
>> within the group that failed to set_dev_pasid.
> 
> Not related to this patch, just curious in which cases some of the devices
> In same group failed to set_dev_pasid while others succeed?
There are multiple failure reasons. Given to the fact of some devices have 
already succeeded, the most typical error may be no memory. Not sure about
other reasons.

Regards,
Yi Liu

> Thanks
> Zhenzhong
> 
>> The remove_dev_pasid()
>> callbacks of the underlying iommu drivers get the 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 if the group is singleton, the existing
>> code logic would have unnecessary warnings in the error handling of the
>> set_dev_pasid() path. e.g. intel iommu driver.
>>
>> The above issue can be fixed by improving the error handling in the
>> set_dev_pasid() path. Also, this reminds that it is not reliable for the
>> underlying iommu driver callback to get the domain from group-
>>> pasid_array.
>> So, the second patch of this series passes the domain to remove_dev_pasid
>> op.
>>
>> [1] https://lore.kernel.org/linux-
>> iommu/20240320123803.GD159172@nvidia.com/
>>
>> Change log:
>>
>> v2:
>> - Make clear that the patch 1/2 of v1 does not fix the problem (Kevin)
>> - Swap the order of patch 1/2 and 2/2 of v1. In this new series, patch 1/2
>>    fixes the real issue, patch 2/2 is to avoid potential issue in the future.
>>
>> v1: https://lore.kernel.org/linux-iommu/20240327125433.248946-1-
>> yi.l.liu@intel.com/
>>
>> Regards,
>> 	Yi Liu
>>
>> Yi Liu (2):
>>   iommu: Undo pasid attachment only for the devices that have succeeded
>>   iommu: Pass domain to remove_dev_pasid() op
>>
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++-----
>> drivers/iommu/intel/iommu.c                 | 11 +++-----
>> drivers/iommu/iommu.c                       | 28 ++++++++++++++-------
>> include/linux/iommu.h                       |  3 ++-
>> 4 files changed, 26 insertions(+), 25 deletions(-)
>>
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu

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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v2 0/2] Two enhancements to
>iommu_at[de]tach_device_pasid()
>
>On 2024/3/29 10:12, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Subject: [PATCH v2 0/2] Two enhancements to
>>> iommu_at[de]tach_device_pasid()
>>>
>>> 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 call into remove_dev_pasid() in error handling when there are
>devices
>>> within the group that failed to set_dev_pasid.
>>
>> Not related to this patch, just curious in which cases some of the devices
>> In same group failed to set_dev_pasid while others succeed?
>There are multiple failure reasons. Given to the fact of some devices have
>already succeeded, the most typical error may be no memory. Not sure
>about
>other reasons.

Oh, the no memory case, clear, thanks Yi.

BRs.
Zhenzhong

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

[Baolu Lu]

On 3/29/24 10:12 AM, Duan, Zhenzhong wrote:
> 
>> -----Original Message-----
>> From: Liu, Yi L<yi.l.liu@intel.com>
>> Subject: [PATCH v2 0/2] Two enhancements to
>> iommu_at[de]tach_device_pasid()
>>
>> 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 call into remove_dev_pasid() in error handling when there are devices
>> within the group that failed to set_dev_pasid.
> Not related to this patch, just curious in which cases some of the devices
> In same group failed to set_dev_pasid while others succeed?

The failure cases could be checked in the set_dev_pasid implementation
of the individual iommu driver. For x86 platforms, which are PCI fabric-
based, there's no such case as PCI/PASID requires a singleton iommu
group.

Best regards,
baolu

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

[Baolu Lu]

On 3/28/24 8:29 PM, Yi Liu wrote:
> 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);

Previously, this callback said "Hey, remove any domain associated with
this pasid".

Now this callback changes to "Hey, please remove *this* domain from the
pasid". What the driver should do if it doesn't match the previously
attached domain, or the pasid hasn't been attached to any domain?

Best regards,
baolu

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

[Baolu Lu]

On 3/28/24 8:29 PM, 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 drawbacks. It would result in unnecessary
> remove_dev_pasid() calls on the devices that have not been attached to the
> new domain. But the remove_dev_pasid() callback would get the new domain
> from the group->pasid_array. So for such devices, the iommu driver won't
> find the attachment under the domain, hence unable to do cleanup. This may
> not be a real problem today. But it depends on the implementation of the
> underlying iommu driver. e.g. the intel iommu driver would warn for such
> devices. Such warnings are unnecessary.
> 
> To solve the above problem, it is necessary to handle the error within
> __iommu_set_group_pasid(). It only loops the devices that have attached
> to the new domain, and undo it.
> 
> Fixes: 16603704559c ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-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: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

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

[Yi Liu]

On 2024/4/3 11:04, Baolu Lu wrote:
> On 3/28/24 8:29 PM, Yi Liu wrote:
>> 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);
> 
> Previously, this callback said "Hey, remove any domain associated with
> this pasid".
> 
> Now this callback changes to "Hey, please remove *this* domain from the
> pasid". What the driver should do if it doesn't match the previously
> attached domain, or the pasid hasn't been attached to any domain?

I think the caller of this callback should know very well whether
a pasid has been attached to this domain or not. So the problem
you described should not happen at all. Otherwise, it is a bug in
the iommu layer.

Actually, there is similar concern in the iommu_detach_device_pasid().
The input domain may be different with what iommu layer tracks. If
so there is a warn. This means the external callers of this API are
buggy. While, I have more faith on iommu layer. :)

-- 
Regards,
Yi Liu

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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Baolu Lu <baolu.lu@linux.intel.com>
>Subject: Re: [PATCH v2 0/2] Two enhancements to
>iommu_at[de]tach_device_pasid()
>
>On 3/29/24 10:12 AM, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Liu, Yi L<yi.l.liu@intel.com>
>>> Subject: [PATCH v2 0/2] Two enhancements to
>>> iommu_at[de]tach_device_pasid()
>>>
>>> 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 call into remove_dev_pasid() in error handling when there are
>devices
>>> within the group that failed to set_dev_pasid.
>> Not related to this patch, just curious in which cases some of the devices
>> In same group failed to set_dev_pasid while others succeed?
>
>The failure cases could be checked in the set_dev_pasid implementation
>of the individual iommu driver. For x86 platforms, which are PCI fabric-
>based, there's no such case as PCI/PASID requires a singleton iommu
>group.

Clear, thanks Baolu.

BRs.
Zhenzhong

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

[Jason Gunthorpe]

On Wed, Apr 03, 2024 at 11:25:35AM +0800, Yi Liu wrote:
> On 2024/4/3 11:04, Baolu Lu wrote:
> > On 3/28/24 8:29 PM, Yi Liu wrote:
> > > 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);
> > 
> > Previously, this callback said "Hey, remove any domain associated with
> > this pasid".
> > 
> > Now this callback changes to "Hey, please remove *this* domain from the
> > pasid". What the driver should do if it doesn't match the previously
> > attached domain, or the pasid hasn't been attached to any domain?
> 
> I think the caller of this callback should know very well whether
> a pasid has been attached to this domain or not. So the problem
> you described should not happen at all. Otherwise, it is a bug in
> the iommu layer.
> 
> Actually, there is similar concern in the iommu_detach_device_pasid().
> The input domain may be different with what iommu layer tracks. If
> so there is a warn. This means the external callers of this API are
> buggy. While, I have more faith on iommu layer. :)

Yeah, the iommu layer should obtain the domain from the pasid xarray
and every driver today also obtains the domain from the pasid
xarray. It is not something different, it is just moving code around.

The core code should guarentee the invariant.

Jason

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

[Joerg Roedel]

On Thu, Mar 28, 2024 at 05:29:56AM -0700, Yi Liu wrote:
> Yi Liu (2):
>   iommu: Undo pasid attachment only for the devices that have succeeded
>   iommu: Pass domain to remove_dev_pasid() op
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++-----
>  drivers/iommu/intel/iommu.c                 | 11 +++-----
>  drivers/iommu/iommu.c                       | 28 ++++++++++++++-------
>  include/linux/iommu.h                       |  3 ++-
>  4 files changed, 26 insertions(+), 25 deletions(-)

Applied, thanks.