[PATCH v3 0/7] Support attaching PASID to the blocked_domain

[PATCH v3 0/7] Support attaching PASID to the blocked_domain

[Yi Liu]

During the review of iommufd pasid series, Kevin and Jason suggested
attaching PASID to the blocked domain hence replacing the usage of
remove_dev_pasid() op [1]. This makes sense as it makes the PASID path
aligned with the RID path which attaches the RID to the blocked_domain
when it is to be blocked. To do it, it requires passing the old domain
to the iommu driver. This has been done in [2].

This series makes the Intel iommu driver, ARM SMMUv3 driver and AMD iommu
driver support attaching PASID to the blocked domain. And in the end remove
the remove_dev_pasid op from iommu_ops.

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

v3:
 - Add a patch to check remove_dev_pasid() in iommu_attach_device_pasid()
 - Split patch 01 of v2 into two patches, drop the r-b of this patch due the
   split.
 - Add AMD iommu blocked domain pasid support (Jason)
 - Remove the remove_dev_pasid op as all the iommu drivers that support pasid
   attach have supported attaching pasid to blocked domain.

v2: https://lore.kernel.org/linux-iommu/20241018055824.24880-1-yi.l.liu@intel.com/#t
 - Add Kevin's r-b
 - Adjust the order of patch 03 of v1, it should be the first patch (Baolu)

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

Regards,
	Yi Liu

Jason Gunthorpe (1):
  iommu/arm-smmu-v3: Make the blocked domain support PASID

Yi Liu (6):
  iommu: Prevent pasid attach if no ops->remove_dev_pasid
  iommu: Consolidate the ops->remove_dev_pasid usage into a helper
  iommu: Detaching pasid by attaching to the blocked_domain
  iommu/vt-d: Make the blocked domain support PASID
  iommu/amd: Make the blocked domain support PASID
  iommu: Remove the remove_dev_pasid op

 drivers/iommu/amd/iommu.c                   | 10 ++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 +++++------
 drivers/iommu/intel/iommu.c                 | 15 ++++++++++----
 drivers/iommu/iommu.c                       | 23 ++++++++++++++-------
 include/linux/iommu.h                       |  5 -----
 5 files changed, 42 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH v3 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

[Yi Liu]

driver should implement both set_dev_pasid and remove_dev_pasid op, otherwise
it is a problem how to detach pasid. In reality, it is impossible that an
iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. However,
it is better to check it.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ea6b4a96186d..866559bbc4e4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3454,11 +3454,13 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 			      struct iommu_attach_handle *handle)
 {
 	/* Caller must be a probed driver on dev */
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_group *group = dev->iommu_group;
 	struct group_device *device;
 	int ret;
 
-	if (!domain->ops->set_dev_pasid)
+	if (!domain->ops->set_dev_pasid ||
+	    !ops->remove_dev_pasid)
 		return -EOPNOTSUPP;
 
 	if (!group)
-- 
2.34.1

[PATCH v3 2/7] iommu: Consolidate the ops->remove_dev_pasid usage into a helper

[Yi Liu]

Add a wrapper for the ops->remove_dev_pasid, this consolidates the iommu_ops
fetching and callback invoking. It is also a preparation for starting the
transition from using remove_dev_pasid op to detach pasid to the way using
blocked_domain to detach pasid.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 866559bbc4e4..21320578d801 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3400,6 +3400,14 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 
+static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *domain)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	ops->remove_dev_pasid(dev, pasid, domain);
+}
+
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
 				   struct iommu_group *group, ioasid_t pasid)
 {
@@ -3418,11 +3426,9 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 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, domain);
+		iommu_remove_dev_pasid(device->dev, pasid, domain);
 	}
 	return ret;
 }
@@ -3432,11 +3438,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 				       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, domain);
+		iommu_remove_dev_pasid(device->dev, pasid, domain);
 	}
 }
 
-- 
2.34.1

[PATCH v3 3/7] iommu: Detaching pasid by attaching to the blocked_domain

[Yi Liu]

The iommu drivers are on the way to detach pasid by attaching to the blocked
domain. However, this cannot be done in one shot. During the transition, iommu
core would select between the remove_dev_pasid op and the blocked domain.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 21320578d801..e8b2850cc61f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3404,8 +3404,18 @@ static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				   struct iommu_domain *domain)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	struct iommu_domain *blocked_domain = ops->blocked_domain;
+	int ret = 1;
+
+	if (blocked_domain && blocked_domain->ops->set_dev_pasid) {
+		ret = blocked_domain->ops->set_dev_pasid(blocked_domain,
+							 dev, pasid, domain);
+	} else if (ops->remove_dev_pasid) {
+		ops->remove_dev_pasid(dev, pasid, domain);
+		ret = 0;
+	}
 
-	ops->remove_dev_pasid(dev, pasid, domain);
+	WARN_ON(ret);
 }
 
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
@@ -3464,7 +3474,9 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	int ret;
 
 	if (!domain->ops->set_dev_pasid ||
-	    !ops->remove_dev_pasid)
+	    (!ops->remove_dev_pasid &&
+	     (!ops->blocked_domain ||
+	      !ops->blocked_domain->ops->set_dev_pasid)))
 		return -EOPNOTSUPP;
 
 	if (!group)
-- 
2.34.1

[PATCH v3 4/7] iommu/arm-smmu-v3: Make the blocked domain support PASID

[Yi Liu]

From: Jason Gunthorpe <jgg@nvidia.com>

The blocked domain is used to park RID to be blocking DMA state. This
can be extended to PASID as well. By this, the remove_dev_pasid() op
of ARM SMMUv3 can be dropped.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 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 2d188d12f85c..57627ba44f6d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2961,13 +2961,12 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 	return ret;
 }
 
-static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				      struct iommu_domain *domain)
+static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
+					   struct device *dev, ioasid_t pasid,
+					   struct iommu_domain *old_domain)
 {
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(old_domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-	struct arm_smmu_domain *smmu_domain;
-
-	smmu_domain = to_smmu_domain(domain);
 
 	mutex_lock(&arm_smmu_asid_lock);
 	arm_smmu_clear_cd(master, pasid);
@@ -2988,6 +2987,7 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 		    sid_domain->type == IOMMU_DOMAIN_BLOCKED)
 			sid_domain->ops->attach_dev(sid_domain, dev);
 	}
+	return 0;
 }
 
 static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
@@ -3069,6 +3069,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops arm_smmu_blocked_ops = {
 	.attach_dev = arm_smmu_attach_dev_blocked,
+	.set_dev_pasid = arm_smmu_blocking_set_dev_pasid,
 };
 
 static struct iommu_domain arm_smmu_blocked_domain = {
@@ -3501,7 +3502,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= arm_smmu_device_group,
 	.of_xlate		= arm_smmu_of_xlate,
 	.get_resv_regions	= arm_smmu_get_resv_regions,
-	.remove_dev_pasid	= arm_smmu_remove_dev_pasid,
 	.dev_enable_feat	= arm_smmu_dev_enable_feature,
 	.dev_disable_feat	= arm_smmu_dev_disable_feature,
 	.page_response		= arm_smmu_page_response,
-- 
2.34.1

[PATCH v3 5/7] iommu/vt-d: Make the blocked domain support PASID

[Yi Liu]

The blocked domain can be extended to park PASID of a device to be the
DMA blocking state. By this the remove_dev_pasid() op is dropped.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7f1ca3c342a3..a1341078b962 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3414,10 +3414,15 @@ static int blocking_domain_attach_dev(struct iommu_domain *domain,
 	return 0;
 }
 
+static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old);
+
 static struct iommu_domain blocking_domain = {
 	.type = IOMMU_DOMAIN_BLOCKED,
 	.ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= blocking_domain_attach_dev,
+		.set_dev_pasid	= blocking_domain_set_dev_pasid,
 	}
 };
 
@@ -4291,15 +4296,18 @@ void domain_remove_dev_pasid(struct iommu_domain *domain,
 	kfree(dev_pasid);
 }
 
-static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-					 struct iommu_domain *domain)
+static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
 	intel_drain_pasid_prq(dev, pasid);
-	domain_remove_dev_pasid(domain, dev, pasid);
+	domain_remove_dev_pasid(old, dev, pasid);
+
+	return 0;
 }
 
 struct dev_pasid_info *
@@ -4664,7 +4672,6 @@ const struct iommu_ops intel_iommu_ops = {
 	.dev_disable_feat	= intel_iommu_dev_disable_feat,
 	.is_attach_deferred	= intel_iommu_is_attach_deferred,
 	.def_domain_type	= device_def_domain_type,
-	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,
 	.pgsize_bitmap		= SZ_4K,
 #ifdef CONFIG_INTEL_IOMMU_SVM
 	.page_response		= intel_svm_page_response,
-- 
2.34.1

[PATCH v3 6/7] iommu/amd: Make the blocked domain support PASID

[Yi Liu]

The blocked domain can be extended to park PASID of a device to be the
DMA blocking state. By this the remove_dev_pasid() op is dropped.

Remove PASID from old domain and device GCR3 table. No need to attach
PASID to the blocked domain as clearing PASID from GCR3 table will make
sure all DMAs for that PASID are blocked.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 477aaf76b7ad..e513ced1ab53 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2472,10 +2472,19 @@ static int blocked_domain_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
+static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid,
+					struct iommu_domain *old)
+{
+	amd_iommu_remove_dev_pasid(dev, pasid, old);
+	return 0;
+}
+
 static struct iommu_domain blocked_domain = {
 	.type = IOMMU_DOMAIN_BLOCKED,
 	.ops = &(const struct iommu_domain_ops) {
 		.attach_dev     = blocked_domain_attach_device,
+		.set_dev_pasid  = blocked_domain_set_dev_pasid,
 	}
 };
 
@@ -2908,7 +2917,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.def_domain_type = amd_iommu_def_domain_type,
 	.dev_enable_feat = amd_iommu_dev_enable_feature,
 	.dev_disable_feat = amd_iommu_dev_disable_feature,
-	.remove_dev_pasid = amd_iommu_remove_dev_pasid,
 	.page_response = amd_iommu_page_response,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
-- 
2.34.1

[PATCH v3 7/7] iommu: Remove the remove_dev_pasid op

[Yi Liu]

The iommu drivers that supports PASID have supported attaching pasid to the
blocked_domain, hence remove the remove_dev_pasid op from the iommu_ops.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 17 ++++-------------
 include/linux/iommu.h |  5 -----
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e8b2850cc61f..53f8e60acf30 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3405,17 +3405,9 @@ static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_domain *blocked_domain = ops->blocked_domain;
-	int ret = 1;
 
-	if (blocked_domain && blocked_domain->ops->set_dev_pasid) {
-		ret = blocked_domain->ops->set_dev_pasid(blocked_domain,
-							 dev, pasid, domain);
-	} else if (ops->remove_dev_pasid) {
-		ops->remove_dev_pasid(dev, pasid, domain);
-		ret = 0;
-	}
-
-	WARN_ON(ret);
+	WARN_ON(blocked_domain->ops->set_dev_pasid(blocked_domain,
+						   dev, pasid, domain));
 }
 
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
@@ -3474,9 +3466,8 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	int ret;
 
 	if (!domain->ops->set_dev_pasid ||
-	    (!ops->remove_dev_pasid &&
-	     (!ops->blocked_domain ||
-	      !ops->blocked_domain->ops->set_dev_pasid)))
+	    !ops->blocked_domain ||
+	    !ops->blocked_domain->ops->set_dev_pasid)
 		return -EOPNOTSUPP;
 
 	if (!group)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2ec0c9915aa5..257b1a53879a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -537,9 +537,6 @@ static inline int __iommu_copy_struct_from_user_array(
  *		- IOMMU_DOMAIN_DMA: must use a dma domain
  *		- 0: use the default setting
  * @default_domain_ops: the default ops for domains
- * @remove_dev_pasid: Remove any translation configurations of a specific
- *                    pasid, so that any DMA transactions with this pasid
- *                    will be blocked by the hardware.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  * @identity_domain: An always available, always attachable identity
@@ -586,8 +583,6 @@ 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,
-				 struct iommu_domain *domain);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
-- 
2.34.1

Re: [PATCH v3 5/7] iommu/vt-d: Make the blocked domain support PASID

[Baolu Lu]

On 11/4/24 21:20, Yi Liu wrote:
> @@ -4291,15 +4296,18 @@ void domain_remove_dev_pasid(struct iommu_domain *domain,
>   	kfree(dev_pasid);
>   }
>   
> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
> -					 struct iommu_domain *domain)
> +static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
> +					 struct device *dev, ioasid_t pasid,
> +					 struct iommu_domain *old)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct intel_iommu *iommu = info->iommu;
>   
>   	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>   	intel_drain_pasid_prq(dev, pasid);
> -	domain_remove_dev_pasid(domain, dev, pasid);
> +	domain_remove_dev_pasid(old, dev, pasid);
> +
> +	return 0;
>   }
>   
>   struct dev_pasid_info *
> @@ -4664,7 +4672,6 @@ const struct iommu_ops intel_iommu_ops = {
>   	.dev_disable_feat	= intel_iommu_dev_disable_feat,
>   	.is_attach_deferred	= intel_iommu_is_attach_deferred,
>   	.def_domain_type	= device_def_domain_type,
> -	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,

This will cause iommu_attach_device_pasid() to fail due to the check and
failure condition introduced in patch 1/7.

--
baolu

Re: [PATCH v3 5/7] iommu/vt-d: Make the blocked domain support PASID

[Yi Liu]

On 2024/11/5 11:46, Baolu Lu wrote:
> On 11/4/24 21:20, Yi Liu wrote:
>> @@ -4291,15 +4296,18 @@ void domain_remove_dev_pasid(struct iommu_domain 
>> *domain,
>>       kfree(dev_pasid);
>>   }
>> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t 
>> pasid,
>> -                     struct iommu_domain *domain)
>> +static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
>> +                     struct device *dev, ioasid_t pasid,
>> +                     struct iommu_domain *old)
>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct intel_iommu *iommu = info->iommu;
>>       intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>       intel_drain_pasid_prq(dev, pasid);
>> -    domain_remove_dev_pasid(domain, dev, pasid);
>> +    domain_remove_dev_pasid(old, dev, pasid);
>> +
>> +    return 0;
>>   }
>>   struct dev_pasid_info *
>> @@ -4664,7 +4672,6 @@ const struct iommu_ops intel_iommu_ops = {
>>       .dev_disable_feat    = intel_iommu_dev_disable_feat,
>>       .is_attach_deferred    = intel_iommu_is_attach_deferred,
>>       .def_domain_type    = device_def_domain_type,
>> -    .remove_dev_pasid    = intel_iommu_remove_dev_pasid,
> 
> This will cause iommu_attach_device_pasid() to fail due to the check and
> failure condition introduced in patch 1/7.

the check introduced in patch 1 were enhanced in patch 3. So removing
remove_dev_pasid op does not fail as intel iommu driver provides
blocked domain which has the set_dev_pasid op.

-- 
Regards,
Yi Liu

Re: [PATCH v3 5/7] iommu/vt-d: Make the blocked domain support PASID

[Baolu Lu]

On 11/5/24 13:11, Yi Liu wrote:
> On 2024/11/5 11:46, Baolu Lu wrote:
>> On 11/4/24 21:20, Yi Liu wrote:
>>> @@ -4291,15 +4296,18 @@ void domain_remove_dev_pasid(struct 
>>> iommu_domain *domain,
>>>       kfree(dev_pasid);
>>>   }
>>> -static void intel_iommu_remove_dev_pasid(struct device *dev, 
>>> ioasid_t pasid,
>>> -                     struct iommu_domain *domain)
>>> +static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
>>> +                     struct device *dev, ioasid_t pasid,
>>> +                     struct iommu_domain *old)
>>>   {
>>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>       struct intel_iommu *iommu = info->iommu;
>>>       intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>>       intel_drain_pasid_prq(dev, pasid);
>>> -    domain_remove_dev_pasid(domain, dev, pasid);
>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>> +
>>> +    return 0;
>>>   }
>>>   struct dev_pasid_info *
>>> @@ -4664,7 +4672,6 @@ const struct iommu_ops intel_iommu_ops = {
>>>       .dev_disable_feat    = intel_iommu_dev_disable_feat,
>>>       .is_attach_deferred    = intel_iommu_is_attach_deferred,
>>>       .def_domain_type    = device_def_domain_type,
>>> -    .remove_dev_pasid    = intel_iommu_remove_dev_pasid,
>>
>> This will cause iommu_attach_device_pasid() to fail due to the check and
>> failure condition introduced in patch 1/7.
> 
> the check introduced in patch 1 were enhanced in patch 3. So removing
> remove_dev_pasid op does not fail as intel iommu driver provides
> blocked domain which has the set_dev_pasid op.
> 

Okay, I see. Thanks!

Re: [PATCH v3 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote:
> driver should implement both set_dev_pasid and remove_dev_pasid op, otherwise
> it is a problem how to detach pasid. In reality, it is impossible that an
> iommu driver implements set_dev_pasid() but no remove_dev_pasid() op. However,
> it is better to check it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I was wondering if we really needed this, but it does make the patches
a bit easier to understand

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

Jason

Re: [PATCH v3 2/7] iommu: Consolidate the ops->remove_dev_pasid usage into a helper

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:28AM -0800, Yi Liu wrote:
> Add a wrapper for the ops->remove_dev_pasid, this consolidates the iommu_ops
> fetching and callback invoking. It is also a preparation for starting the
> transition from using remove_dev_pasid op to detach pasid to the way using
> blocked_domain to detach pasid.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)

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

Jason

Re: [PATCH v3 3/7] iommu: Detaching pasid by attaching to the blocked_domain

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:29AM -0800, Yi Liu wrote:
> The iommu drivers are on the way to detach pasid by attaching to the blocked
> domain. However, this cannot be done in one shot. During the transition, iommu
> core would select between the remove_dev_pasid op and the blocked domain.
> 
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

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

Jason

Re: [PATCH v3 5/7] iommu/vt-d: Make the blocked domain support PASID

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:31AM -0800, Yi Liu wrote:
> The blocked domain can be extended to park PASID of a device to be the
> DMA blocking state. By this the remove_dev_pasid() op is dropped.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

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

Jason

Re: [PATCH v3 6/7] iommu/amd: Make the blocked domain support PASID

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:32AM -0800, Yi Liu wrote:
> The blocked domain can be extended to park PASID of a device to be the
> DMA blocking state. By this the remove_dev_pasid() op is dropped.
> 
> Remove PASID from old domain and device GCR3 table. No need to attach
> PASID to the blocked domain as clearing PASID from GCR3 table will make
> sure all DMAs for that PASID are blocked.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/amd/iommu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

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

Jason

Re: [PATCH v3 7/7] iommu: Remove the remove_dev_pasid op

[Jason Gunthorpe]

On Mon, Nov 04, 2024 at 05:20:33AM -0800, Yi Liu wrote:
> The iommu drivers that supports PASID have supported attaching pasid to the
> blocked_domain, hence remove the remove_dev_pasid op from the iommu_ops.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommu.c | 17 ++++-------------
>  include/linux/iommu.h |  5 -----
>  2 files changed, 4 insertions(+), 18 deletions(-)

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

Jason

Re: [PATCH v3 6/7] iommu/amd: Make the blocked domain support PASID

[Vasant Hegde]

On 11/4/2024 6:50 PM, Yi Liu wrote:
> The blocked domain can be extended to park PASID of a device to be the
> DMA blocking state. By this the remove_dev_pasid() op is dropped.
> 
> Remove PASID from old domain and device GCR3 table. No need to attach
> PASID to the blocked domain as clearing PASID from GCR3 table will make
> sure all DMAs for that PASID are blocked.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH v3 2/7] iommu: Consolidate the ops->remove_dev_pasid usage into a helper

[Vasant Hegde]

On 11/4/2024 6:50 PM, Yi Liu wrote:
> Add a wrapper for the ops->remove_dev_pasid, this consolidates the iommu_ops
> fetching and callback invoking. It is also a preparation for starting the
> transition from using remove_dev_pasid op to detach pasid to the way using
> blocked_domain to detach pasid.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>



> ---
>  drivers/iommu/iommu.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 866559bbc4e4..21320578d801 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3400,6 +3400,14 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
>  
> +static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
> +				   struct iommu_domain *domain)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +
> +	ops->remove_dev_pasid(dev, pasid, domain);
> +}
> +
>  static int __iommu_set_group_pasid(struct iommu_domain *domain,
>  				   struct iommu_group *group, ioasid_t pasid)
>  {
> @@ -3418,11 +3426,9 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
>  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, domain);
> +		iommu_remove_dev_pasid(device->dev, pasid, domain);
>  	}
>  	return ret;
>  }
> @@ -3432,11 +3438,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
>  				       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, domain);
> +		iommu_remove_dev_pasid(device->dev, pasid, domain);
>  	}

Now its single statement inside loop. We don't need braces.

-Vasant

Re: [PATCH v3 3/7] iommu: Detaching pasid by attaching to the blocked_domain

[Vasant Hegde]

On 11/4/2024 6:50 PM, Yi Liu wrote:
> The iommu drivers are on the way to detach pasid by attaching to the blocked
> domain. However, this cannot be done in one shot. During the transition, iommu
> core would select between the remove_dev_pasid op and the blocked domain.
> 
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH v3 7/7] iommu: Remove the remove_dev_pasid op

[Vasant Hegde]

On 11/4/2024 6:50 PM, Yi Liu wrote:
> The iommu drivers that supports PASID have supported attaching pasid to the
> blocked_domain, hence remove the remove_dev_pasid op from the iommu_ops.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


-Vasant

RE: [PATCH v3 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, November 5, 2024 11:42 PM
> 
> On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote:
> > driver should implement both set_dev_pasid and remove_dev_pasid op,
> otherwise
> > it is a problem how to detach pasid. In reality, it is impossible that an
> > iommu driver implements set_dev_pasid() but no remove_dev_pasid() op.
> However,
> > it is better to check it.
> >
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/iommu/iommu.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I was wondering if we really needed this, but it does make the patches
> a bit easier to understand
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 

me too. btw when Vasant's series introduces the PASID flag to
domain above checks can be moved to the domain alloc time
then here just needs to check whether the domain allows 
pasid attach.

for now,

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

and a nit - there is another reference to dev_iommu_ops in this
function:

	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||

could be replaced with ops too.

RE: [PATCH v3 2/7] iommu: Consolidate the ops->remove_dev_pasid usage into a helper

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:20 PM
> 
> Add a wrapper for the ops->remove_dev_pasid, this consolidates the
> iommu_ops
> fetching and callback invoking. It is also a preparation for starting the
> transition from using remove_dev_pasid op to detach pasid to the way using
> blocked_domain to detach pasid.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

RE: [PATCH v3 3/7] iommu: Detaching pasid by attaching to the blocked_domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:20 PM
>
> @@ -3404,8 +3404,18 @@ static void iommu_remove_dev_pasid(struct
> device *dev, ioasid_t pasid,
>  				   struct iommu_domain *domain)
>  {
>  	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	struct iommu_domain *blocked_domain = ops->blocked_domain;
> +	int ret = 1;
> +
> +	if (blocked_domain && blocked_domain->ops->set_dev_pasid) {
> +		ret = blocked_domain->ops->set_dev_pasid(blocked_domain,
> +							 dev, pasid, domain);
> +	} else if (ops->remove_dev_pasid) {
> +		ops->remove_dev_pasid(dev, pasid, domain);
> +		ret = 0;
> +	}
> 

given ops->remove_dev_pasid is already checked at attach time
here could just call it w/o further check.

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

RE: [PATCH v3 6/7] iommu/amd: Make the blocked domain support PASID

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:21 PM
> 
> The blocked domain can be extended to park PASID of a device to be the
> DMA blocking state. By this the remove_dev_pasid() op is dropped.
> 
> Remove PASID from old domain and device GCR3 table. No need to attach
> PASID to the blocked domain as clearing PASID from GCR3 table will make
> sure all DMAs for that PASID are blocked.
> 
> 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 v3 7/7] iommu: Remove the remove_dev_pasid op

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:21 PM
> 
> The iommu drivers that supports PASID have supported attaching pasid to
> the
> blocked_domain, hence remove the remove_dev_pasid op from the
> iommu_ops.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

Re: [PATCH v3 1/7] iommu: Prevent pasid attach if no ops->remove_dev_pasid

[Yi Liu]

On 2024/11/7 17:33, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Tuesday, November 5, 2024 11:42 PM
>>
>> On Mon, Nov 04, 2024 at 05:20:27AM -0800, Yi Liu wrote:
>>> driver should implement both set_dev_pasid and remove_dev_pasid op,
>> otherwise
>>> it is a problem how to detach pasid. In reality, it is impossible that an
>>> iommu driver implements set_dev_pasid() but no remove_dev_pasid() op.
>> However,
>>> it is better to check it.
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/iommu.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> I was wondering if we really needed this, but it does make the patches
>> a bit easier to understand
>>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>
> 
> me too. btw when Vasant's series introduces the PASID flag to
> domain above checks can be moved to the domain alloc time
> then here just needs to check whether the domain allows
> pasid attach.

hmmm. Are we sure all the caller of iommu_attach_device_pasid() will
allocate domain with this flag? Besides iommufd, idxd driver would
also call iommu_attach_device_pasid(), and it uses the DMA domain of
the device. I suppose this domain is allocated with the pasid flag.

Given that Vasant's series has been queued and this series is based
on that. It might make sense to drop this patch. If the pasid capable
domain can be allocated successfully, the iommu driver already indicates
it has the ability to support set_dev_pasid and the undo op.

> for now,
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> and a nit - there is another reference to dev_iommu_ops in this
> function:
> 
> 	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> 
> could be replaced with ops too.

If we decide to keep this patch, I'll add it.

-- 
Regards,
Yi Liu

Re: [PATCH v3 3/7] iommu: Detaching pasid by attaching to the blocked_domain

[Yi Liu]

On 2024/11/7 17:35, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:20 PM
>>
>> @@ -3404,8 +3404,18 @@ static void iommu_remove_dev_pasid(struct
>> device *dev, ioasid_t pasid,
>>   				   struct iommu_domain *domain)
>>   {
>>   	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +	struct iommu_domain *blocked_domain = ops->blocked_domain;
>> +	int ret = 1;
>> +
>> +	if (blocked_domain && blocked_domain->ops->set_dev_pasid) {
>> +		ret = blocked_domain->ops->set_dev_pasid(blocked_domain,
>> +							 dev, pasid, domain);
>> +	} else if (ops->remove_dev_pasid) {
>> +		ops->remove_dev_pasid(dev, pasid, domain);
>> +		ret = 0;
>> +	}
>>
> 
> given ops->remove_dev_pasid is already checked at attach time
> here could just call it w/o further check.

yep. A driver does not pass that check should not get here at all. So
either should be valid.

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

-- 
Regards,
Yi Liu