[PATCH v2 0/6] Make set_dev_pasid op supporting domain replacement

[PATCH v2 0/6] Make set_dev_pasid op supporting domain replacement

[Yi Liu]

This splits the preparation works of the iommu and the Intel iommu driver
out from the iommufd pasid attach/replace series. [1]

To support domain replacement, the definition of the set_dev_pasid op
needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
should be extended as well to suit the new definition.

This series first prepares the Intel iommu set_dev_pasid op for the new
definition, adds the missing set_dev_pasid support for nested domain, makes
ARM SMMUv3 set_dev_pasid op to suit the new definition, and in the end
enhances the definition of set_dev_pasid op. The AMD set_dev_pasid callback
is extended to fail if the caller tries to do domain replacement to meet the
new definition of set_dev_pasid op. AMD iommu driver would support it later
per Vasant [2].

[1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1-yi.l.liu@intel.com/
[2] https://lore.kernel.org/linux-iommu/fa9c4fc3-9365-465e-8926-b4d2d6361b9c@amd.com/

v2:
 - Make ARM SMMUv3 set_dev_pasid op support domain replacement (Jason)
 - Drop patch 03 of v1 (Kevin)
 - Multiple tweaks in VT-d driver (Kevin)

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

Regards,
	Yi Liu

Jason Gunthorpe (1):
  iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

Lu Baolu (1):
  iommu/vt-d: Add set_dev_pasid callback for nested domain

Yi Liu (4):
  iommu: Pass old domain to set_dev_pasid op
  iommu/vt-d: Move intel_drain_pasid_prq() into
    intel_pasid_tear_down_entry()
  iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
    replacement
  iommu: Make set_dev_pasid op support domain replacement

 drivers/iommu/amd/amd_iommu.h                 |   3 +-
 drivers/iommu/amd/pasid.c                     |   6 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   5 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |   8 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +-
 drivers/iommu/intel/iommu.c                   | 122 ++++++++++++------
 drivers/iommu/intel/iommu.h                   |   3 +
 drivers/iommu/intel/nested.c                  |   1 +
 drivers/iommu/intel/pasid.c                   |  13 +-
 drivers/iommu/intel/pasid.h                   |   8 +-
 drivers/iommu/intel/svm.c                     |   6 +-
 drivers/iommu/iommu.c                         |   3 +-
 include/linux/iommu.h                         |   5 +-
 13 files changed, 129 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] iommu: Pass old domain to set_dev_pasid op

[Yi Liu]

To support domain replacement for pasid, the underlying iommu driver needs
to know the old domain hence be able to clean up the existing attachment.
It would be much convenient for iommu layer to pass down the old domain.
Otherwise, iommu drivers would need to track domain for pasids by themselves,
this would duplicate code among the iommu drivers. Or iommu drivers would
rely group->pasid_array to get domain, which may not always the correct
one.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/amd/amd_iommu.h                   | 3 ++-
 drivers/iommu/amd/pasid.c                       | 3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++-
 drivers/iommu/intel/iommu.c                     | 6 ++++--
 drivers/iommu/intel/svm.c                       | 3 ++-
 drivers/iommu/iommu.c                           | 3 ++-
 include/linux/iommu.h                           | 2 +-
 7 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 2d5945c982bd..271406a45e33 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -51,7 +51,8 @@ struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 						struct mm_struct *mm);
 void amd_iommu_domain_free(struct iommu_domain *dom);
 int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid);
+			    struct device *dev, ioasid_t pasid,
+			    struct iommu_domain *old);
 void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				struct iommu_domain *domain);
 
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index a68215f2b3e1..77bf5f5f947a 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -100,7 +100,8 @@ static const struct mmu_notifier_ops sva_mn = {
 };
 
 int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid)
+			    struct device *dev, ioasid_t pasid,
+			    struct iommu_domain *old)
 {
 	struct pdom_dev_data *pdom_dev_data;
 	struct protection_domain *sva_pdom = to_pdomain(domain);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index a7c36654dee5..645da7b69bed 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -332,7 +332,8 @@ void arm_smmu_sva_notifier_synchronize(void)
 }
 
 static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t id)
+				      struct device *dev, ioasid_t id,
+				      struct iommu_domain *old)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 34006b6e89eb..1a2a5cf4ef60 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4283,7 +4283,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid)
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4569,7 +4570,8 @@ static int identity_domain_attach_dev(struct iommu_domain *domain, struct device
 }
 
 static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
-					 struct device *dev, ioasid_t pasid)
+					 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;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 0e3a9b38bef2..5ae1df7598b7 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -194,7 +194,8 @@ static const struct mmu_notifier_ops intel_mmuops = {
 };
 
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
-				   struct device *dev, ioasid_t pasid)
+				   struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 83c8e617a2c5..f3f81c04b8fb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3331,7 +3331,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	int ret;
 
 	for_each_group_device(group, device) {
-		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
+		ret = domain->ops->set_dev_pasid(domain, device->dev,
+						 pasid, NULL);
 		if (ret)
 			goto err_revert;
 	}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index bd722f473635..32dce80aa7fd 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -642,7 +642,7 @@ struct iommu_ops {
 struct iommu_domain_ops {
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
-			     ioasid_t pasid);
+			     ioasid_t pasid, struct iommu_domain *old);
 
 	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
-- 
2.34.1

Re: [PATCH v2 1/6] iommu: Pass old domain to set_dev_pasid op

[Jason Gunthorpe]

On Thu, Sep 12, 2024 at 06:04:22AM -0700, Yi Liu wrote:
> To support domain replacement for pasid, the underlying iommu driver needs
> to know the old domain hence be able to clean up the existing attachment.
> It would be much convenient for iommu layer to pass down the old domain.
> Otherwise, iommu drivers would need to track domain for pasids by themselves,
> this would duplicate code among the iommu drivers. Or iommu drivers would
> rely group->pasid_array to get domain, which may not always the correct
> one.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/amd/amd_iommu.h                   | 3 ++-
>  drivers/iommu/amd/pasid.c                       | 3 ++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++-
>  drivers/iommu/intel/iommu.c                     | 6 ++++--
>  drivers/iommu/intel/svm.c                       | 3 ++-
>  drivers/iommu/iommu.c                           | 3 ++-
>  include/linux/iommu.h                           | 2 +-
>  7 files changed, 15 insertions(+), 8 deletions(-)

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

Jason

RE: [PATCH v2 1/6] iommu: Pass old domain to set_dev_pasid op

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:04 PM
> 
> To support domain replacement for pasid, the underlying iommu driver
> needs
> to know the old domain hence be able to clean up the existing attachment.
> It would be much convenient for iommu layer to pass down the old domain.
> Otherwise, iommu drivers would need to track domain for pasids by
> themselves,
> this would duplicate code among the iommu drivers. Or iommu drivers
> would
> rely group->pasid_array to get domain, which may not always the correct
> one.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Yi Liu]

Draining PRQ is mostly conjuncted with pasid teardown, and with more callers coming,
move it into it in the intel_pasid_tear_down_entry(). But there is scenario that only
teardown pasid entry but no PRQ drain, so passing a flag to mark it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c |  8 ++++----
 drivers/iommu/intel/pasid.c | 13 +++++++++++--
 drivers/iommu/intel/pasid.h |  8 +++++---
 drivers/iommu/intel/svm.c   |  3 ++-
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1a2a5cf4ef60..80b587de226d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3376,7 +3376,7 @@ void device_block_translation(struct device *dev)
 	if (!dev_is_real_dma_subdevice(dev)) {
 		if (sm_supported(iommu))
 			intel_pasid_tear_down_entry(iommu, dev,
-						    IOMMU_NO_PASID, false);
+						    IOMMU_NO_PASID, 0);
 		else
 			domain_context_clear(info);
 	}
@@ -4258,7 +4258,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	unsigned long flags;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
 		return;
 	}
 
@@ -4278,8 +4278,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_pasid);
-	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
-	intel_drain_pasid_prq(dev, pasid);
+	intel_pasid_tear_down_entry(iommu, dev, pasid,
+				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index b51fc268dc84..ceb9c5274a39 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 		qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT);
 }
 
+/*
+ * Not all PASID entry destroy requires PRQ drain as it can be handled in
+ * the remove_dev_pasid path. Caller should be clear about it and set the
+ * @flags properly.
+ */
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
-				 u32 pasid, bool fault_ignore)
+				 u32 pasid, u32 flags)
 {
 	struct pasid_entry *pte;
 	u16 did, pgtt;
@@ -251,7 +256,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 
 	did = pasid_get_domain_id(pte);
 	pgtt = pasid_pte_get_pgtt(pte);
-	intel_pasid_clear_entry(dev, pasid, fault_ignore);
+	intel_pasid_clear_entry(dev, pasid,
+				flags & INTEL_PASID_TEARDOWN_IGNORE_FAULT);
 	spin_unlock(&iommu->lock);
 
 	if (!ecap_coherent(iommu->ecap))
@@ -267,6 +273,9 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 	/* Device IOTLB doesn't need to be flushed in caching mode. */
 	if (!cap_caching_mode(iommu->cap))
 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
+
+	if (flags & INTEL_PASID_TEARDOWN_DRAIN_PRQ)
+		intel_drain_pasid_prq(dev, pasid);
 }
 
 /*
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index dde6d3ba5ae0..6eb849ec5fb8 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -303,9 +303,11 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 				   struct device *dev, u32 pasid);
 int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 			     u32 pasid, struct dmar_domain *domain);
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
-				 struct device *dev, u32 pasid,
-				 bool fault_ignore);
+
+#define INTEL_PASID_TEARDOWN_IGNORE_FAULT	(1U << 0)
+#define INTEL_PASID_TEARDOWN_DRAIN_PRQ		(1U << 1)
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
+				 u32 pasid, u32 flags);
 void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 					  struct device *dev, u32 pasid);
 int intel_pasid_setup_sm_context(struct device *dev);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 5ae1df7598b7..3c1e105b9da6 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -176,7 +176,8 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
 		info = dev_iommu_priv_get(dev_pasid->dev);
 		intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
-					    dev_pasid->pasid, true);
+					    dev_pasid->pasid,
+					    INTEL_PASID_TEARDOWN_IGNORE_FAULT);
 	}
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-- 
2.34.1

Re: [PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Baolu Lu]

On 2024/9/12 21:04, Yi Liu wrote:
> Draining PRQ is mostly conjuncted with pasid teardown, and with more callers coming,
> move it into it in the intel_pasid_tear_down_entry(). But there is scenario that only
> teardown pasid entry but no PRQ drain, so passing a flag to mark it.

Is it a reasonable case where PRI needs to be drained but the pasid
entry won't be torn down? For example, could this happen when a PRI is
disabled?

> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c |  8 ++++----
>   drivers/iommu/intel/pasid.c | 13 +++++++++++--
>   drivers/iommu/intel/pasid.h |  8 +++++---
>   drivers/iommu/intel/svm.c   |  3 ++-
>   4 files changed, 22 insertions(+), 10 deletions(-)

Thanks,
baolu

Re: [PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Yi Liu]

On 2024/9/12 21:22, Baolu Lu wrote:
> On 2024/9/12 21:04, Yi Liu wrote:
>> Draining PRQ is mostly conjuncted with pasid teardown, and with more 
>> callers coming,
>> move it into it in the intel_pasid_tear_down_entry(). But there is 
>> scenario that only
>> teardown pasid entry but no PRQ drain, so passing a flag to mark it.
> 
> Is it a reasonable case where PRI needs to be drained but the pasid
> entry won't be torn down? For example, could this happen when a PRI is
> disabled?

in concept, yes. But it seems no more than a debugging method in my
opinion. I cannot map it to a usage so far.

>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c |  8 ++++----
>>   drivers/iommu/intel/pasid.c | 13 +++++++++++--
>>   drivers/iommu/intel/pasid.h |  8 +++++---
>>   drivers/iommu/intel/svm.c   |  3 ++-
>>   4 files changed, 22 insertions(+), 10 deletions(-)
> 
> Thanks,
> baolu

-- 
Regards,
Yi Liu

Re: [PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Baolu Lu]

On 9/12/24 9:04 PM, Yi Liu wrote:
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index b51fc268dc84..ceb9c5274a39 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
>   		qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT);
>   }
>   
> +/*
> + * Not all PASID entry destroy requires PRQ drain as it can be handled in
> + * the remove_dev_pasid path. Caller should be clear about it and set the
> + * @flags properly.
> + */
>   void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
> -				 u32 pasid, bool fault_ignore)
> +				 u32 pasid, u32 flags)

As a generic opt-in bit flags, why not using 'unsigned int'?

Thanks,
baolu

Re: [PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Yi Liu]

On 2024/9/13 10:11, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index b51fc268dc84..ceb9c5274a39 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -236,8 +236,13 @@ devtlb_invalidation_with_pasid(struct intel_iommu 
>> *iommu,
>>           qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 
>> - VTD_PAGE_SHIFT);
>>   }
>> +/*
>> + * Not all PASID entry destroy requires PRQ drain as it can be handled in
>> + * the remove_dev_pasid path. Caller should be clear about it and set the
>> + * @flags properly.
>> + */
>>   void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>> device *dev,
>> -                 u32 pasid, bool fault_ignore)
>> +                 u32 pasid, u32 flags)
> 
> As a generic opt-in bit flags, why not using 'unsigned int'?

aha, I can use 'unsigned int'.

-- 
Regards,
Yi Liu

RE: [PATCH v2 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:04 PM
> 
> +/*
> + * Not all PASID entry destroy requires PRQ drain as it can be handled in
> + * the remove_dev_pasid path. Caller should be clear about it and set the
> + * @flags properly.
> + */

/*
 * Caller can request to drain PRQ in this helper if it hasn't done so,
 * e.g. in a path which doesn't follow remove_dev_pasid().
 */

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

[PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

set_dev_pasid op is going to support domain replacement and keep the old
hardware config if it fails. Make the Intel iommu driver be prepared for
it.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 80b587de226d..6f5a8e549f3f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4248,8 +4248,8 @@ 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,
-					 struct iommu_domain *domain)
+static void domain_remove_dev_pasid(struct iommu_domain *domain,
+				    struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dev_pasid_info *curr, *dev_pasid = NULL;
@@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	struct dmar_domain *dmar_domain;
 	unsigned long flags;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
-		return;
-	}
-
 	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
@@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_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 intel_iommu *iommu = info->iommu;
+
 	intel_pasid_tear_down_entry(iommu, dev, pasid,
 				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+	if (domain->type == IOMMU_DOMAIN_IDENTITY)
+		return;
+	domain_remove_dev_pasid(domain, dev, pasid);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+static struct dev_pasid_info *
+domain_prepare_dev_pasid(struct iommu_domain *domain,
+			 struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
-		return -EOPNOTSUPP;
-
-	if (domain->dirty_ops)
-		return -EINVAL;
-
-	if (context_copied(iommu, info->bus, info->devfn))
-		return -EBUSY;
-
 	ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
 	if (!dev_pasid)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	ret = domain_attach_iommu(dmar_domain, iommu);
 	if (ret)
@@ -4318,6 +4315,47 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (ret)
 		goto out_detach_iommu;
 
+	dev_pasid->dev = dev;
+	dev_pasid->pasid = pasid;
+	spin_lock_irqsave(&dmar_domain->lock, flags);
+	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+
+	return dev_pasid;
+out_detach_iommu:
+	domain_detach_iommu(dmar_domain, iommu);
+out_free:
+	kfree(dev_pasid);
+	return ERR_PTR(ret);
+}
+
+static int intel_iommu_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 dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	struct dev_pasid_info *dev_pasid;
+	int ret;
+
+	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;
+
+	if (domain->dirty_ops)
+		return -EINVAL;
+
+	if (context_copied(iommu, info->bus, info->devfn))
+		return -EBUSY;
+
+	dev_pasid = domain_prepare_dev_pasid(domain, dev, pasid);
+	if (IS_ERR(dev_pasid))
+		return PTR_ERR(dev_pasid);
+
+	/* Clear the old configuration if it already exists */
+	intel_pasid_tear_down_entry(iommu, dev, pasid,
+				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+
 	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
@@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
 						     dev, pasid);
 	if (ret)
-		goto out_unassign_tag;
+		goto out_undo_dev_pasid;
 
-	dev_pasid->dev = dev;
-	dev_pasid->pasid = pasid;
-	spin_lock_irqsave(&dmar_domain->lock, flags);
-	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
-	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+	if (old)
+		domain_remove_dev_pasid(old, dev, pasid);
 
 	if (domain->type & __IOMMU_DOMAIN_PAGING)
 		intel_iommu_debugfs_create_dev_pasid(dev_pasid);
 
 	return 0;
-out_unassign_tag:
-	cache_tag_unassign_domain(dmar_domain, dev, pasid);
-out_detach_iommu:
-	domain_detach_iommu(dmar_domain, iommu);
-out_free:
-	kfree(dev_pasid);
+
+out_undo_dev_pasid:
+	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 9/12/24 9:04 PM, Yi Liu wrote:
> set_dev_pasid op is going to support domain replacement and keep the old
> hardware config if it fails. Make the Intel iommu driver be prepared for
> it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++-------------
>   1 file changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 80b587de226d..6f5a8e549f3f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4248,8 +4248,8 @@ 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,
> -					 struct iommu_domain *domain)
> +static void domain_remove_dev_pasid(struct iommu_domain *domain,
> +				    struct device *dev, ioasid_t pasid)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct dev_pasid_info *curr, *dev_pasid = NULL;
> @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>   	struct dmar_domain *dmar_domain;
>   	unsigned long flags;
>   
> -	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> -		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
> -		return;
> -	}
> -
>   	dmar_domain = to_dmar_domain(domain);
>   	spin_lock_irqsave(&dmar_domain->lock, flags);
>   	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
> @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>   	domain_detach_iommu(dmar_domain, iommu);
>   	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>   	kfree(dev_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 intel_iommu *iommu = info->iommu;
> +
>   	intel_pasid_tear_down_entry(iommu, dev, pasid,
>   				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> +	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> +		return;

The static identity domain is not capable of handling page requests.
Therefore there is no need to drain PRQ for an identity domain removal.

So it probably should be something like this:

	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
		return;
	}

	intel_pasid_tear_down_entry(iommu, dev, pasid,
                                     INTEL_PASID_TEARDOWN_DRAIN_PRQ);

> +	domain_remove_dev_pasid(domain, dev, pasid);
>   }
>   
> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> -				     struct device *dev, ioasid_t pasid,
> -				     struct iommu_domain *old)
> +static struct dev_pasid_info *
> +domain_prepare_dev_pasid(struct iommu_domain *domain,
> +			 struct device *dev, ioasid_t pasid)

Why do you want to return a struct pointer instead of an integer? The
returned pointer is not used after it is returned.

Also, how about renaming this helper to domain_add_dev_pasid() to pair
it with domain_remove_dev_pasid()?

>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   	unsigned long flags;
>   	int ret;
>   
> -	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
> -		return -EOPNOTSUPP;
> -
> -	if (domain->dirty_ops)
> -		return -EINVAL;
> -
> -	if (context_copied(iommu, info->bus, info->devfn))
> -		return -EBUSY;
> -
>   	ret = prepare_domain_attach_device(domain, dev);
>   	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
>   
>   	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>   	if (!dev_pasid)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>   
>   	ret = domain_attach_iommu(dmar_domain, iommu);
>   	if (ret)

Thanks,
baolu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 9/13/24 9:35 AM, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> set_dev_pasid op is going to support domain replacement and keep the old
>> hardware config if it fails. Make the Intel iommu driver be prepared for
>> it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++-------------
>>   1 file changed, 65 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 80b587de226d..6f5a8e549f3f 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4248,8 +4248,8 @@ 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,
>> -                     struct iommu_domain *domain)
>> +static void domain_remove_dev_pasid(struct iommu_domain *domain,
>> +                    struct device *dev, ioasid_t pasid)
>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct dev_pasid_info *curr, *dev_pasid = NULL;
>> @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>       struct dmar_domain *dmar_domain;
>>       unsigned long flags;
>> -    if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>> -        intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>> -        return;
>> -    }
>> -
>>       dmar_domain = to_dmar_domain(domain);
>>       spin_lock_irqsave(&dmar_domain->lock, flags);
>>       list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>> @@ -4278,13 +4273,24 @@ static void 
>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>>       domain_detach_iommu(dmar_domain, iommu);
>>       intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>       kfree(dev_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 intel_iommu *iommu = info->iommu;
>> +
>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> +        return;
> 
> The static identity domain is not capable of handling page requests.
> Therefore there is no need to drain PRQ for an identity domain removal.
> 
> So it probably should be something like this:
> 
>      if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>          intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>          return;
>      }
> 
>      intel_pasid_tear_down_entry(iommu, dev, pasid,
>                                      INTEL_PASID_TEARDOWN_DRAIN_PRQ);

Just revisited this. It seems that we just need to drain PRQ if the
attached domain is iopf-capable. Therefore, how about making it like
this?

	unsigned int flags = 0;

	if (domain->iopf_handler)
		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;

	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);

	/* Identity domain has no meta data for pasid. */
	if (domain->type == IOMMU_DOMAIN_IDENTITY)
		return;

Thanks,
baolu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

On 2024/9/13 10:17, Baolu Lu wrote:
> On 9/13/24 9:35 AM, Baolu Lu wrote:
>> On 9/12/24 9:04 PM, Yi Liu wrote:
>>> set_dev_pasid op is going to support domain replacement and keep the old
>>> hardware config if it fails. Make the Intel iommu driver be prepared for
>>> it.
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++-------------
>>>   1 file changed, 65 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 80b587de226d..6f5a8e549f3f 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -4248,8 +4248,8 @@ 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,
>>> -                     struct iommu_domain *domain)
>>> +static void domain_remove_dev_pasid(struct iommu_domain *domain,
>>> +                    struct device *dev, ioasid_t pasid)
>>>   {
>>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>       struct dev_pasid_info *curr, *dev_pasid = NULL;
>>> @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct 
>>> device *dev, ioasid_t pasid,
>>>       struct dmar_domain *dmar_domain;
>>>       unsigned long flags;
>>> -    if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>>> -        intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>>> -        return;
>>> -    }
>>> -
>>>       dmar_domain = to_dmar_domain(domain);
>>>       spin_lock_irqsave(&dmar_domain->lock, flags);
>>>       list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>>> @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct 
>>> device *dev, ioasid_t pasid,
>>>       domain_detach_iommu(dmar_domain, iommu);
>>>       intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>>       kfree(dev_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 intel_iommu *iommu = info->iommu;
>>> +
>>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>>                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>>> +        return;
>>
>> The static identity domain is not capable of handling page requests.
>> Therefore there is no need to drain PRQ for an identity domain removal.
>>
>> So it probably should be something like this:
>>
>>      if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>>          intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>>          return;
>>      }
>>
>>      intel_pasid_tear_down_entry(iommu, dev, pasid,
>>                                      INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> 
> Just revisited this. It seems that we just need to drain PRQ if the
> attached domain is iopf-capable. Therefore, how about making it like
> this?
> 
>      unsigned int flags = 0;
> 
>      if (domain->iopf_handler)
>          flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
> 
>      intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
> 
>      /* Identity domain has no meta data for pasid. */
>      if (domain->type == IOMMU_DOMAIN_IDENTITY)
>          return;
> 
got it.

-- 
Regards,
Yi Liu

RE: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, September 13, 2024 10:17 AM
> 
> On 9/13/24 9:35 AM, Baolu Lu wrote:
> > On 9/12/24 9:04 PM, Yi Liu wrote:
> >> +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 intel_iommu *iommu = info->iommu;
> >> +
> >>       intel_pasid_tear_down_entry(iommu, dev, pasid,
> >>                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> >> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
> >> +        return;
> >
> > The static identity domain is not capable of handling page requests.
> > Therefore there is no need to drain PRQ for an identity domain removal.
> >
> > So it probably should be something like this:
> >
> >      if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> >          intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
> >          return;
> >      }
> >
> >      intel_pasid_tear_down_entry(iommu, dev, pasid,
> >                                      INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> 
> Just revisited this. It seems that we just need to drain PRQ if the
> attached domain is iopf-capable. Therefore, how about making it like
> this?
> 
> 	unsigned int flags = 0;
> 
> 	if (domain->iopf_handler)
> 		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
> 
> 	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
> 
> 	/* Identity domain has no meta data for pasid. */
> 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> 		return;
> 

this is the right thing to do, but also suggesting a bug in existing
code. intel_pasid_tear_down_entry() is not just for PRQ drain.
It's also about iotlb/devtlb invalidation. From device p.o.v it
has no idea about the translation mode in the IOMMU side and
always caches the valid mappings in devtlb when ATS is enabled.

Existing code skips all those housekeeping for identify domain
by early return before intel_pasid_tear_down_entry(). We need
a separate fix for it before this series?

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 2024/9/30 15:19, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, September 13, 2024 10:17 AM
>>
>> On 9/13/24 9:35 AM, Baolu Lu wrote:
>>> On 9/12/24 9:04 PM, Yi Liu wrote:
>>>> +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 intel_iommu *iommu = info->iommu;
>>>> +
>>>>        intel_pasid_tear_down_entry(iommu, dev, pasid,
>>>>                        INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>>>> +        return;
>>>
>>> The static identity domain is not capable of handling page requests.
>>> Therefore there is no need to drain PRQ for an identity domain removal.
>>>
>>> So it probably should be something like this:
>>>
>>>       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>>>           intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>>>           return;
>>>       }
>>>
>>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>>                                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>
>> Just revisited this. It seems that we just need to drain PRQ if the
>> attached domain is iopf-capable. Therefore, how about making it like
>> this?
>>
>> 	unsigned int flags = 0;
>>
>> 	if (domain->iopf_handler)
>> 		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
>>
>> 	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
>>
>> 	/* Identity domain has no meta data for pasid. */
>> 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> 		return;
>>
> 
> this is the right thing to do, but also suggesting a bug in existing
> code. intel_pasid_tear_down_entry() is not just for PRQ drain.
> It's also about iotlb/devtlb invalidation. From device p.o.v it
> has no idea about the translation mode in the IOMMU side and
> always caches the valid mappings in devtlb when ATS is enabled.

Yes. You are right.

intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation.
So it's fine as long as intel_pasid_tear_down_entry() is called for the
IDENTITY domain path, right?

> Existing code skips all those housekeeping for identify domain
> by early return before intel_pasid_tear_down_entry(). We need
> a separate fix for it before this series?

Existing code doesn't skip intel_pasid_tear_down_entry().

         if (domain->type == IOMMU_DOMAIN_IDENTITY) {
                 intel_pasid_tear_down_entry(iommu, dev, pasid, false);
                 return;
         }

Or anything I overlooked?

Thanks,
baolu

RE: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, October 9, 2024 9:09 AM
> 
> On 2024/9/30 15:19, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Friday, September 13, 2024 10:17 AM
> >>
> >> On 9/13/24 9:35 AM, Baolu Lu wrote:
> >>> On 9/12/24 9:04 PM, Yi Liu wrote:
> >>>> +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 intel_iommu *iommu = info->iommu;
> >>>> +
> >>>>        intel_pasid_tear_down_entry(iommu, dev, pasid,
> >>>>                        INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> >>>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
> >>>> +        return;
> >>>
> >>> The static identity domain is not capable of handling page requests.
> >>> Therefore there is no need to drain PRQ for an identity domain removal.
> >>>
> >>> So it probably should be something like this:
> >>>
> >>>       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> >>>           intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
> >>>           return;
> >>>       }
> >>>
> >>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
> >>>                                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> >>
> >> Just revisited this. It seems that we just need to drain PRQ if the
> >> attached domain is iopf-capable. Therefore, how about making it like
> >> this?
> >>
> >> 	unsigned int flags = 0;
> >>
> >> 	if (domain->iopf_handler)
> >> 		flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ;
> >>
> >> 	intel_pasid_tear_down_entry(iommu, dev, pasid, flags);
> >>
> >> 	/* Identity domain has no meta data for pasid. */
> >> 	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> >> 		return;
> >>
> >
> > this is the right thing to do, but also suggesting a bug in existing
> > code. intel_pasid_tear_down_entry() is not just for PRQ drain.
> > It's also about iotlb/devtlb invalidation. From device p.o.v it
> > has no idea about the translation mode in the IOMMU side and
> > always caches the valid mappings in devtlb when ATS is enabled.
> 
> Yes. You are right.
> 
> intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation.
> So it's fine as long as intel_pasid_tear_down_entry() is called for the
> IDENTITY domain path, right?
> 
> > Existing code skips all those housekeeping for identify domain
> > by early return before intel_pasid_tear_down_entry(). We need
> > a separate fix for it before this series?
> 
> Existing code doesn't skip intel_pasid_tear_down_entry().
> 
>          if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>                  intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>                  return;
>          }
> 
> Or anything I overlooked?

No. Looks I was confused by this patch and misunderstood the
existing code.

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

On 2024/9/13 09:35, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> set_dev_pasid op is going to support domain replacement and keep the old
>> hardware config if it fails. Make the Intel iommu driver be prepared for
>> it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++-------------
>>   1 file changed, 65 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 80b587de226d..6f5a8e549f3f 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4248,8 +4248,8 @@ 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,
>> -                     struct iommu_domain *domain)
>> +static void domain_remove_dev_pasid(struct iommu_domain *domain,
>> +                    struct device *dev, ioasid_t pasid)
>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct dev_pasid_info *curr, *dev_pasid = NULL;
>> @@ -4257,11 +4257,6 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>       struct dmar_domain *dmar_domain;
>>       unsigned long flags;
>> -    if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>> -        intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>> -        return;
>> -    }
>> -
>>       dmar_domain = to_dmar_domain(domain);
>>       spin_lock_irqsave(&dmar_domain->lock, flags);
>>       list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>> @@ -4278,13 +4273,24 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid,
>>       domain_detach_iommu(dmar_domain, iommu);
>>       intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>       kfree(dev_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 intel_iommu *iommu = info->iommu;
>> +
>>       intel_pasid_tear_down_entry(iommu, dev, pasid,
>>                       INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>> +    if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> +        return;
> 
> The static identity domain is not capable of handling page requests.
> Therefore there is no need to drain PRQ for an identity domain removal.

oh, yes. so maybe for SI domain, there is no PRQ at all.

> So it probably should be something like this:
> 
>      if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>          intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
>          return;
>      }
> 
>      intel_pasid_tear_down_entry(iommu, dev, pasid,
>                                      INTEL_PASID_TEARDOWN_DRAIN_PRQ);
> 
>> +    domain_remove_dev_pasid(domain, dev, pasid);
>>   }
>> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>> -                     struct device *dev, ioasid_t pasid,
>> -                     struct iommu_domain *old)
>> +static struct dev_pasid_info *
>> +domain_prepare_dev_pasid(struct iommu_domain *domain,
>> +             struct device *dev, ioasid_t pasid)
> 
> Why do you want to return a struct pointer instead of an integer? The
> returned pointer is not used after it is returned.

it's for the intel_iommu_debugfs_create_dev_pasid(). it needs the dev_pasid.

> Also, how about renaming this helper to domain_add_dev_pasid() to pair
> it with domain_remove_dev_pasid()?

sure.

>>   {
>>       struct device_domain_info *info = dev_iommu_priv_get(dev);
>>       struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct 
>> iommu_domain *domain,
>>       unsigned long flags;
>>       int ret;
>> -    if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
>> -        return -EOPNOTSUPP;
>> -
>> -    if (domain->dirty_ops)
>> -        return -EINVAL;
>> -
>> -    if (context_copied(iommu, info->bus, info->devfn))
>> -        return -EBUSY;
>> -
>>       ret = prepare_domain_attach_device(domain, dev);
>>       if (ret)
>> -        return ret;
>> +        return ERR_PTR(ret);
>>       dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>>       if (!dev_pasid)
>> -        return -ENOMEM;
>> +        return ERR_PTR(-ENOMEM);
>>       ret = domain_attach_iommu(dmar_domain, iommu);
>>       if (ret)
> 
> Thanks,
> baolu

-- 
Regards,
Yi Liu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 9/12/24 9:04 PM, Yi Liu wrote:
> @@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>   						     dev, pasid);
>   	if (ret)
> -		goto out_unassign_tag;
> +		goto out_undo_dev_pasid;
>   
> -	dev_pasid->dev = dev;
> -	dev_pasid->pasid = pasid;
> -	spin_lock_irqsave(&dmar_domain->lock, flags);
> -	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> -	spin_unlock_irqrestore(&dmar_domain->lock, flags);
> +	if (old)
> +		domain_remove_dev_pasid(old, dev, pasid);
>   
>   	if (domain->type & __IOMMU_DOMAIN_PAGING)
>   		intel_iommu_debugfs_create_dev_pasid(dev_pasid);
>   
>   	return 0;
> -out_unassign_tag:
> -	cache_tag_unassign_domain(dmar_domain, dev, pasid);
> -out_detach_iommu:
> -	domain_detach_iommu(dmar_domain, iommu);
> -out_free:
> -	kfree(dev_pasid);
> +
> +out_undo_dev_pasid:
> +	domain_remove_dev_pasid(domain, dev, pasid);
>   	return ret;
>   }

Do you need to re-install the old domain to the pasid entry in the
failure path?

Thanks,
baolu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

On 2024/9/13 09:42, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> @@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct 
>> iommu_domain *domain,
>>           ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>>                                dev, pasid);
>>       if (ret)
>> -        goto out_unassign_tag;
>> +        goto out_undo_dev_pasid;
>> -    dev_pasid->dev = dev;
>> -    dev_pasid->pasid = pasid;
>> -    spin_lock_irqsave(&dmar_domain->lock, flags);
>> -    list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
>> -    spin_unlock_irqrestore(&dmar_domain->lock, flags);
>> +    if (old)
>> +        domain_remove_dev_pasid(old, dev, pasid);
>>       if (domain->type & __IOMMU_DOMAIN_PAGING)
>>           intel_iommu_debugfs_create_dev_pasid(dev_pasid);
>>       return 0;
>> -out_unassign_tag:
>> -    cache_tag_unassign_domain(dmar_domain, dev, pasid);
>> -out_detach_iommu:
>> -    domain_detach_iommu(dmar_domain, iommu);
>> -out_free:
>> -    kfree(dev_pasid);
>> +
>> +out_undo_dev_pasid:
>> +    domain_remove_dev_pasid(domain, dev, pasid);
>>       return ret;
>>   }
> 
> Do you need to re-install the old domain to the pasid entry in the
> failure path?

yes, but no. The old domain is still installed in the pasid entry
when the failure happened. :)

-- 
Regards,
Yi Liu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 9/13/24 8:21 PM, Yi Liu wrote:
> On 2024/9/13 09:42, Baolu Lu wrote:
>> On 9/12/24 9:04 PM, Yi Liu wrote:
>>> @@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct 
>>> iommu_domain *domain,
>>>           ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>>>                                dev, pasid);
>>>       if (ret)
>>> -        goto out_unassign_tag;
>>> +        goto out_undo_dev_pasid;
>>> -    dev_pasid->dev = dev;
>>> -    dev_pasid->pasid = pasid;
>>> -    spin_lock_irqsave(&dmar_domain->lock, flags);
>>> -    list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
>>> -    spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>> +    if (old)
>>> +        domain_remove_dev_pasid(old, dev, pasid);
>>>       if (domain->type & __IOMMU_DOMAIN_PAGING)
>>>           intel_iommu_debugfs_create_dev_pasid(dev_pasid);
>>>       return 0;
>>> -out_unassign_tag:
>>> -    cache_tag_unassign_domain(dmar_domain, dev, pasid);
>>> -out_detach_iommu:
>>> -    domain_detach_iommu(dmar_domain, iommu);
>>> -out_free:
>>> -    kfree(dev_pasid);
>>> +
>>> +out_undo_dev_pasid:
>>> +    domain_remove_dev_pasid(domain, dev, pasid);
>>>       return ret;
>>>   }
>>
>> Do you need to re-install the old domain to the pasid entry in the
>> failure path?
> 
> yes, but no. The old domain is still installed in the pasid entry
> when the failure happened. :)

I am afraid not. The old domain has already been cleaned up by
intel_pasid_tear_down_entry(). Or not?

Thanks,
baolu

Re: [PATCH v2 3/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Liu, Yi L]

> On Sep 14, 2024, at 09:08, Baolu Lu <baolu.lu@linux.intel.com> wrote:
> 
> On 9/13/24 8:21 PM, Yi Liu wrote:
>>> On 2024/9/13 09:42, Baolu Lu wrote:
>>> On 9/12/24 9:04 PM, Yi Liu wrote:
>>>> @@ -4325,24 +4363,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>>>>           ret = intel_pasid_setup_second_level(iommu, dmar_domain,
>>>>                                dev, pasid);
>>>>       if (ret)
>>>> -        goto out_unassign_tag;
>>>> +        goto out_undo_dev_pasid;
>>>> -    dev_pasid->dev = dev;
>>>> -    dev_pasid->pasid = pasid;
>>>> -    spin_lock_irqsave(&dmar_domain->lock, flags);
>>>> -    list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
>>>> -    spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>> +    if (old)
>>>> +        domain_remove_dev_pasid(old, dev, pasid);
>>>>       if (domain->type & __IOMMU_DOMAIN_PAGING)
>>>>           intel_iommu_debugfs_create_dev_pasid(dev_pasid);
>>>>       return 0;
>>>> -out_unassign_tag:
>>>> -    cache_tag_unassign_domain(dmar_domain, dev, pasid);
>>>> -out_detach_iommu:
>>>> -    domain_detach_iommu(dmar_domain, iommu);
>>>> -out_free:
>>>> -    kfree(dev_pasid);
>>>> +
>>>> +out_undo_dev_pasid:
>>>> +    domain_remove_dev_pasid(domain, dev, pasid);
>>>>       return ret;
>>>>   }
>>> 
>>> Do you need to re-install the old domain to the pasid entry in the
>>> failure path?
>> yes, but no. The old domain is still installed in the pasid entry
>> when the failure happened. :)
> 
> I am afraid not. The old domain has already been cleaned up by
> intel_pasid_tear_down_entry(). Or not?

oops, yes. The tear down was lifted to this function now instead of in the setup helpers. I may move them back to the setup helpers just like v1 does.

Good catch!

Regards,
Yi Liu

[PATCH v2 4/6] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

From: Lu Baolu <baolu.lu@linux.intel.com>

Extend intel_iommu_set_dev_pasid() to set a nested type domain to a PASID
of a device.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Co-developed-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 22 +++++++++++++++++-----
 drivers/iommu/intel/iommu.h  |  3 +++
 drivers/iommu/intel/nested.c |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6f5a8e549f3f..749ee7741ec4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -284,6 +284,11 @@ static int __init intel_iommu_setup(char *str)
 }
 __setup("intel_iommu=", intel_iommu_setup);
 
+static int domain_type_is_nested(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_NESTED;
+}
+
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	ret = prepare_domain_attach_device(domain, dev);
+	/* Nested type domain should prepare its parent domain */
+	if (domain_type_is_nested(dmar_domain))
+		ret = prepare_domain_attach_device(
+				&dmar_domain->s2_domain->domain, dev);
+	else
+		ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -4329,9 +4339,9 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
 	return ERR_PTR(ret);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+int intel_iommu_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 dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4356,7 +4366,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	intel_pasid_tear_down_entry(iommu, dev, pasid,
 				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
 
-	if (dmar_domain->use_first_level)
+	if (domain_type_is_nested(dmar_domain))
+		ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain);
+	else if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
 	else
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index b3b295e60626..36cf59bfe4ea 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1109,6 +1109,9 @@ void device_block_translation(struct device *dev);
 int prepare_domain_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old);
 
 int dmar_ir_support(void);
 
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 36a91b1b52be..c5a94c00b396 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -131,6 +131,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.attach_dev		= intel_nested_attach_dev,
+	.set_dev_pasid		= intel_iommu_set_dev_pasid,
 	.free			= intel_nested_domain_free,
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
-- 
2.34.1

Re: [PATCH v2 4/6] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 9/12/24 9:04 PM, Yi Liu wrote:
> @@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
>   	unsigned long flags;
>   	int ret;
>   
> -	ret = prepare_domain_attach_device(domain, dev);
> +	/* Nested type domain should prepare its parent domain */
> +	if (domain_type_is_nested(dmar_domain))
> +		ret = prepare_domain_attach_device(
> +				&dmar_domain->s2_domain->domain, dev);
> +	else
> +		ret = prepare_domain_attach_device(domain, dev);
>   	if (ret)
>   		return ERR_PTR(ret);
>   

'prepare' is a bit confusing in this context. It actually means checking
whether a domain is compatible with the hardware capabilities of RID or
PASID. Or not?

I know this confusion comes from the naming of
prepare_domain_attach_device(). Hence, do you mind renaming this helper
in a small patch?

Thanks,
baolu

Re: [PATCH v2 4/6] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/9/13 09:52, Baolu Lu wrote:
> On 9/12/24 9:04 PM, Yi Liu wrote:
>> @@ -4299,7 +4304,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
>>       unsigned long flags;
>>       int ret;
>> -    ret = prepare_domain_attach_device(domain, dev);
>> +    /* Nested type domain should prepare its parent domain */
>> +    if (domain_type_is_nested(dmar_domain))
>> +        ret = prepare_domain_attach_device(
>> +                &dmar_domain->s2_domain->domain, dev);
>> +    else
>> +        ret = prepare_domain_attach_device(domain, dev);
>>       if (ret)
>>           return ERR_PTR(ret);
> 
> 'prepare' is a bit confusing in this context. It actually means checking
> whether a domain is compatible with the hardware capabilities of RID or
> PASID. Or not?
> 
> I know this confusion comes from the naming of
> prepare_domain_attach_device(). Hence, do you mind renaming this helper
> in a small patch?

good suggestion.

-- 
Regards,
Yi Liu

RE: [PATCH v2 4/6] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:04 PM
> 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Extend intel_iommu_set_dev_pasid() to set a nested type domain to a PASID
> of a device.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH v2 5/6] iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

[Yi Liu]

From: Jason Gunthorpe <jgg@nvidia.com>

set_dev_pasid() op is going to be enhanced to support domain replacement
of a pasid. This prepares for this op definition.

Signed-off-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-sva.c | 2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 8 +++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 645da7b69bed..1d3e71569775 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	 * get reassigned
 	 */
 	arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid);
-	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
+	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
 
 	mmput(domain->mm);
 	return ret;
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 ed2b106e02dd..f7a73b854151 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 }
 
 static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t id)
+				     struct device *dev, ioasid_t id,
+				     struct iommu_domain *old)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
@@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
 	 */
 	arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
 	return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
-				  &target_cd);
+				  &target_cd, old);
 }
 
 static void arm_smmu_update_ste(struct arm_smmu_master *master,
@@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
 
 int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
-		       struct arm_smmu_cd *cd)
+		       struct arm_smmu_cd *cd, struct iommu_domain *old)
 {
 	struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev);
 	struct arm_smmu_attach_state state = {
@@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		 * already attached, no need to set old_domain.
 		 */
 		.ssid = pasid,
+		.old_domain = old,
 	};
 	struct arm_smmu_cd *cdptr;
 	int ret;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 14bca41a981b..a942f5d40051 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -806,7 +806,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
 
 int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
-		       struct arm_smmu_cd *cd);
+		       struct arm_smmu_cd *cd, struct iommu_domain *old);
 
 void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
 void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
-- 
2.34.1

RE: [PATCH v2 5/6] iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:04 PM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> set_dev_pasid() op is going to be enhanced to support domain replacement
> of a pasid. This prepares for this op definition.
> 
> Signed-off-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 5/6] iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

[Will Deacon]

On Thu, Sep 12, 2024 at 06:04:26AM -0700, Yi Liu wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> set_dev_pasid() op is going to be enhanced to support domain replacement
> of a pasid. This prepares for this op definition.
> 
> Signed-off-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-sva.c | 2 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 8 +++++---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 +-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 645da7b69bed..1d3e71569775 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>  	 * get reassigned
>  	 */
>  	arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid);
> -	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
> +	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
>  
>  	mmput(domain->mm);
>  	return ret;
> 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 ed2b106e02dd..f7a73b854151 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  }
>  
>  static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
> -				      struct device *dev, ioasid_t id)
> +				     struct device *dev, ioasid_t id,
> +				     struct iommu_domain *old)
>  {
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> @@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
>  	 */
>  	arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
>  	return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
> -				  &target_cd);
> +				  &target_cd, old);
>  }
>  
>  static void arm_smmu_update_ste(struct arm_smmu_master *master,
> @@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
>  
>  int arm_smmu_set_pasid(struct arm_smmu_master *master,
>  		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
> -		       struct arm_smmu_cd *cd)
> +		       struct arm_smmu_cd *cd, struct iommu_domain *old)
>  {
>  	struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev);
>  	struct arm_smmu_attach_state state = {
> @@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>  		 * already attached, no need to set old_domain.
>  		 */
>  		.ssid = pasid,
> +		.old_domain = old,

nit: The existing comment says "no need to set old_domain" and now you're
doing exactly that! Please can you update the commentary (probably just
remove the comment entirely)?

Cheers,

Will

Re: [PATCH v2 5/6] iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

[Yi Liu]

On 2024/10/15 16:43, Will Deacon wrote:
> On Thu, Sep 12, 2024 at 06:04:26AM -0700, Yi Liu wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>>
>> set_dev_pasid() op is going to be enhanced to support domain replacement
>> of a pasid. This prepares for this op definition.
>>
>> Signed-off-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-sva.c | 2 +-
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 8 +++++---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 +-
>>   3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> index 645da7b69bed..1d3e71569775 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> @@ -349,7 +349,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>>   	 * get reassigned
>>   	 */
>>   	arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->cd.asid);
>> -	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
>> +	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old);
>>   
>>   	mmput(domain->mm);
>>   	return ret;
>> 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 ed2b106e02dd..f7a73b854151 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2824,7 +2824,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>   }
>>   
>>   static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
>> -				      struct device *dev, ioasid_t id)
>> +				     struct device *dev, ioasid_t id,
>> +				     struct iommu_domain *old)
>>   {
>>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>   	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>> @@ -2850,7 +2851,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
>>   	 */
>>   	arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
>>   	return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
>> -				  &target_cd);
>> +				  &target_cd, old);
>>   }
>>   
>>   static void arm_smmu_update_ste(struct arm_smmu_master *master,
>> @@ -2880,7 +2881,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
>>   
>>   int arm_smmu_set_pasid(struct arm_smmu_master *master,
>>   		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
>> -		       struct arm_smmu_cd *cd)
>> +		       struct arm_smmu_cd *cd, struct iommu_domain *old)
>>   {
>>   	struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev);
>>   	struct arm_smmu_attach_state state = {
>> @@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>>   		 * already attached, no need to set old_domain.
>>   		 */
>>   		.ssid = pasid,
>> +		.old_domain = old,
> 
> nit: The existing comment says "no need to set old_domain" and now you're
> doing exactly that! Please can you update the commentary (probably just
> remove the comment entirely)?

oops, indeed, should remove this comment entirely. thanks for spotting it.

-- 
Regards,
Yi Liu

Re: [PATCH v2 5/6] iommu/arm-smmu-v3: Make smmuv3 set_dev_pasid() op support replace

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 09:43:24AM +0100, Will Deacon wrote:
> > @@ -2890,6 +2891,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> >  		 * already attached, no need to set old_domain.
> >  		 */
> >  		.ssid = pasid,
> > +		.old_domain = old,
> 
> nit: The existing comment says "no need to set old_domain" and now you're
> doing exactly that! Please can you update the commentary (probably just
> remove the comment entirely)?

+1

Jason

[PATCH v2 6/6] iommu: Make set_dev_pasid op support domain replacement

[Yi Liu]

The iommu core is going to support domain replacement for pasid, it needs
to make the set_dev_pasid op support replacing domain and keep the old
domain config in the failure case.

AMD iommu driver does not support domain replacement for pasid yet, so it
would fail the set_dev_pasid op to keep the old config if the input @old
is non-NULL.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/amd/pasid.c | 3 +++
 include/linux/iommu.h     | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 77bf5f5f947a..30e27bda3fac 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -109,6 +109,9 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret = -EINVAL;
 
+	if (old)
+		return -EOPNOTSUPP;
+
 	/* PASID zero is used for requests from the I/O device without PASID */
 	if (!is_pasid_valid(dev_data, pasid))
 		return ret;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32dce80aa7fd..27f923450a7c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -616,7 +616,8 @@ struct iommu_ops {
  * * EBUSY	- device is attached to a domain and cannot be changed
  * * ENODEV	- device specific errors, not able to be attached
  * * <others>	- treated as ENODEV by the caller. Use is discouraged
- * @set_dev_pasid: set an iommu domain to a pasid of device
+ * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The pasid of
+ *                 the device should be left in the old config in error case.
  * @map_pages: map a physically contiguous set of pages of the same size to
  *             an iommu domain.
  * @unmap_pages: unmap a number of pages of the same size from an iommu domain
-- 
2.34.1

Re: [PATCH v2 6/6] iommu: Make set_dev_pasid op support domain replacement

[Jason Gunthorpe]

On Thu, Sep 12, 2024 at 06:04:27AM -0700, Yi Liu wrote:
> The iommu core is going to support domain replacement for pasid, it needs
> to make the set_dev_pasid op support replacing domain and keep the old
> domain config in the failure case.
> 
> AMD iommu driver does not support domain replacement for pasid yet, so it
> would fail the set_dev_pasid op to keep the old config if the input @old
> is non-NULL.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/amd/pasid.c | 3 +++
>  include/linux/iommu.h     | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)

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

Jason

RE: [PATCH v2 6/6] iommu: Make set_dev_pasid op support domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:04 PM
> 
> The iommu core is going to support domain replacement for pasid, it needs
> to make the set_dev_pasid op support replacing domain and keep the old
> domain config in the failure case.
> 
> AMD iommu driver does not support domain replacement for pasid yet, so it
> would fail the set_dev_pasid op to keep the old config if the input @old
> is non-NULL.
> 
> 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>