[PATCH v4 00/13] Make set_dev_pasid op supporting domain replacement

[PATCH v4 00/13] 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 passes the old domain to the set_dev_pasid op, and prepares
the Intel iommu set_dev_pasid callbacks (paging domain, identity domain, and
sva domain) for the new definition, add the missing set_dev_pasid callback
for the nested domain, makes ARM SMMUv3 set_dev_pasid op to suit the new
set_dev_pasid op definition, and in the end, claims the set_dev_pasid op support
domain replacement. The AMD set_dev_pasid callback is extended to fail if the
caller tries to do the 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/

v4:
 - Fix a missing input argument modification in patch 01 (Nicolin)
 - Per Baolu's suggestion, this series does not extend the pasid setup helpers to
   handle domain replacement, instead it adds pasid replace helpers, hence
   drop patch 02 of v3 as it is no longer required in this series.
 - Add a new helper to consolidate the cache flush for the modifications to a
   present pasid entry (the fields other than P and SSADE)   (Baolu)
 - Update the set_dev_pasid op of intel identity domain to handle replacement
 - Consolidate the dev_pasid_info code of intel_svm_set_dev_pasid()
 - Add a separate set_dev_pasid callback for nested domain instead of sharing
   intel_iommu_set_dev_pasid()  (Baolu)
 - Drop patch 05 of v3 as Baolu has another patch that touches it.

v3: https://lore.kernel.org/linux-iommu/20241018055402.23277-1-yi.l.liu@intel.com/
 - Add Kevin and Jason's r-b to patch 01, 02, 04, 05 and 06 of v2
 - Add back the patch 03 of v1 to make the pasid setup helpers do all the pasid entry
   modification, hence the set_dev_pasid path is really rollback-less, which is spotted
   by Baolu.
 - Rename prepare_domain_attach_device() (Baolu)
 - Use unsigned int instead of u32 for flags (Baolu)
 - Remove a stale comment in arm_smmu_set_pasid (Will)

v2: https://lore.kernel.org/linux-iommu/20240912130427.10119-1-yi.l.liu@intel.com/
 - 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 set_dev_pasid() op support replace

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

Yi Liu (11):
  iommu: Pass old domain to set_dev_pasid op
  iommu/vt-d: Add a helper to flush cache for updating present pasid
    entry
  iommu/vt-d: Refactor the pasid setup helpers
  iommu/vt-d: Add pasid replace helpers
  iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement
  iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
    replacement
  iommu/vt-d: Limit intel_iommu_set_dev_pasid() for paging domain
  iommu/vt-d: Make identity_domain_set_dev_pasid() to handle domain
    replacement
  iommu/vt-d: Consolidate the dev_pasid code in
    intel_svm_set_dev_pasid()
  iommu/vt-d: Fail SVA 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   |  12 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +-
 drivers/iommu/intel/iommu.c                   | 137 +++++--
 drivers/iommu/intel/iommu.h                   |  13 +
 drivers/iommu/intel/nested.c                  |  43 ++
 drivers/iommu/intel/pasid.c                   | 374 ++++++++++++++----
 drivers/iommu/intel/pasid.h                   |  43 ++
 drivers/iommu/intel/svm.c                     |  32 +-
 drivers/iommu/iommu.c                         |   3 +-
 include/linux/iommu.h                         |   5 +-
 13 files changed, 530 insertions(+), 148 deletions(-)

-- 
2.34.1

[PATCH v4 01/13] 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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Nicolin Chen <nicolinc@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/arm/arm-smmu-v3/arm-smmu-v3.c     | 3 ++-
 drivers/iommu/intel/iommu.c                     | 6 ++++--
 drivers/iommu/intel/svm.c                       | 3 ++-
 drivers/iommu/iommu.c                           | 3 ++-
 include/linux/iommu.h                           | 2 +-
 8 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 38509e1019e9..1bef5d55b2f9 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -53,7 +53,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 0657b9373be5..d1dfc745f55e 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/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 826db8894fb7..a40681f8c348 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2856,7 +2856,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);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e860bc9439a2..86000901de46 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4287,7 +4287,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);
@@ -4573,7 +4574,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 078d1e32a24e..3b5e3da24f19 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -197,7 +197,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 dfc2f9816086..ea6b4a96186d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3407,7 +3407,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 42243183e81d..e1c8e92170e9 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -640,7 +640,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 v4 01/13] iommu: Pass old domain to set_dev_pasid op

[Vasant Hegde]

On 11/4/2024 6:48 PM, 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>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

-Vasant

[PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Yi Liu]

There are more paths that need to flush cache for present pasid entry
after adding pasid replacement. Hence, add a helper for it. Per the
VT-d spec, the changes to the fields other than SSADE and P bit can
share the same code. So intel_pasid_setup_page_snoop_control() is the
first user of this helper.

No functional change is intended.

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

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 977c4ac00c4c..81d038222414 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -286,6 +286,41 @@ static void pasid_flush_caches(struct intel_iommu *iommu,
 	}
 }
 
+/*
+ * This function is supposed to be used after caller updates the fields
+ * except for the SSADE and P bit of a pasid table entry. It does the
+ * below:
+ * - Flush cacheline if needed
+ * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.
+ *   ”Guidance to Software for Invalidations“
+ *
+ * Caller of it should not modify the in-use pasid table entries.
+ */
+static void intel_pasid_flush_present(struct intel_iommu *iommu,
+				      struct device *dev,
+				      u32 pasid, u16 did,
+				      struct pasid_entry *pte)
+{
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(pte, sizeof(*pte));
+
+	/*
+	 * VT-d spec 5.0 table28 states guides for cache invalidation:
+	 *
+	 * - PASID-selective-within-Domain PASID-cache invalidation
+	 * - PASID-selective PASID-based IOTLB invalidation
+	 * - If (pasid is RID_PASID)
+	 *    - Global Device-TLB invalidation to affected functions
+	 *   Else
+	 *    - PASID-based Device-TLB invalidation (with S=1 and
+	 *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions
+	 */
+	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
+	qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
+
+	devtlb_invalidation_with_pasid(iommu, dev, pasid);
+}
+
 /*
  * Set up the scalable mode pasid table entry for first only
  * translation type.
@@ -551,24 +586,7 @@ void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 	did = pasid_get_domain_id(pte);
 	spin_unlock(&iommu->lock);
 
-	if (!ecap_coherent(iommu->ecap))
-		clflush_cache_range(pte, sizeof(*pte));
-
-	/*
-	 * VT-d spec 3.4 table23 states guides for cache invalidation:
-	 *
-	 * - PASID-selective-within-Domain PASID-cache invalidation
-	 * - PASID-selective PASID-based IOTLB invalidation
-	 * - If (pasid is RID_PASID)
-	 *    - Global Device-TLB invalidation to affected functions
-	 *   Else
-	 *    - PASID-based Device-TLB invalidation (with S=1 and
-	 *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions
-	 */
-	pasid_cache_invalidation_with_pasid(iommu, did, pasid);
-	qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
-
-	devtlb_invalidation_with_pasid(iommu, dev, pasid);
+	intel_pasid_flush_present(iommu, dev, pasid, did, pte);
 }
 
 /**
-- 
2.34.1

Re: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> There are more paths that need to flush cache for present pasid entry
> after adding pasid replacement. Hence, add a helper for it. Per the
> VT-d spec, the changes to the fields other than SSADE and P bit can
> share the same code. So intel_pasid_setup_page_snoop_control() is the
> first user of this helper.
> 
> No functional change is intended.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
>   1 file changed, 36 insertions(+), 18 deletions(-)

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

RE: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> There are more paths that need to flush cache for present pasid entry
> after adding pasid replacement. Hence, add a helper for it. Per the
> VT-d spec, the changes to the fields other than SSADE and P bit can
> share the same code. So intel_pasid_setup_page_snoop_control() is the
> first user of this helper.

No hw spec would ever talk about coding sharing in sw implementation.

and according to the following context the fact is just that two flows
between RID and PASID are similar so you decide to create a common
helper for both.

> 
> No functional change is intended.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 977c4ac00c4c..81d038222414 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -286,6 +286,41 @@ static void pasid_flush_caches(struct intel_iommu
> *iommu,
>  	}
>  }
> 
> +/*
> + * This function is supposed to be used after caller updates the fields
> + * except for the SSADE and P bit of a pasid table entry. It does the
> + * below:
> + * - Flush cacheline if needed
> + * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.

while at it please add the name for the table.

> + *   ”Guidance to Software for Invalidations“
> + *
> + * Caller of it should not modify the in-use pasid table entries.

I'm not sure about this statement. As long as the change doesn't
impact in-fly DMAs it's always the caller's right for whether to do it.

actually based on the main intention of supporting replace it's
quite possible.

otherwise this looks good,

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

Re: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Yi Liu]

On 2024/11/6 15:11, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> There are more paths that need to flush cache for present pasid entry
>> after adding pasid replacement. Hence, add a helper for it. Per the
>> VT-d spec, the changes to the fields other than SSADE and P bit can
>> share the same code. So intel_pasid_setup_page_snoop_control() is the
>> first user of this helper.
> 
> No hw spec would ever talk about coding sharing in sw implementation.

yes, it's just sw choice.

> and according to the following context the fact is just that two flows
> between RID and PASID are similar so you decide to create a common
> helper for both.

I'm not quite getting why RID is related. This is only about the cache
flush per pasid entry updating.

>>
>> No functional change is intended.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
>>   1 file changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 977c4ac00c4c..81d038222414 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -286,6 +286,41 @@ static void pasid_flush_caches(struct intel_iommu
>> *iommu,
>>   	}
>>   }
>>
>> +/*
>> + * This function is supposed to be used after caller updates the fields
>> + * except for the SSADE and P bit of a pasid table entry. It does the
>> + * below:
>> + * - Flush cacheline if needed
>> + * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.
> 
> while at it please add the name for the table.

sure.

> 
>> + *   ”Guidance to Software for Invalidations“
>> + *
>> + * Caller of it should not modify the in-use pasid table entries.
> 
> I'm not sure about this statement. As long as the change doesn't
> impact in-fly DMAs it's always the caller's right for whether to do it.
> 
> actually based on the main intention of supporting replace it's
> quite possible.

This comment is mainly due to the clflush_cache_range() within this
helper. If caller modifies the pte content, it will need to call
this again.

> otherwise this looks good,
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

-- 
Regards,
Yi Liu

RE: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 4:46 PM
> 
> On 2024/11/6 15:11, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, November 4, 2024 9:19 PM
> >>
> >> There are more paths that need to flush cache for present pasid entry
> >> after adding pasid replacement. Hence, add a helper for it. Per the
> >> VT-d spec, the changes to the fields other than SSADE and P bit can
> >> share the same code. So intel_pasid_setup_page_snoop_control() is the
> >> first user of this helper.
> >
> > No hw spec would ever talk about coding sharing in sw implementation.
> 
> yes, it's just sw choice.
> 
> > and according to the following context the fact is just that two flows
> > between RID and PASID are similar so you decide to create a common
> > helper for both.
> 
> I'm not quite getting why RID is related. This is only about the cache
> flush per pasid entry updating.

the comment says:

+	 * - If (pasid is RID_PASID)
+	 *    - Global Device-TLB invalidation to affected functions
+	 *   Else
+	 *    - PASID-based Device-TLB invalidation (with S=1 and
+	 *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions

so that is the only difference between two invalidation flows?

> 
> >>
> >> No functional change is intended.
> >>
> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >> ---
> >>   drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
> >>   1 file changed, 36 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> >> index 977c4ac00c4c..81d038222414 100644
> >> --- a/drivers/iommu/intel/pasid.c
> >> +++ b/drivers/iommu/intel/pasid.c
> >> @@ -286,6 +286,41 @@ static void pasid_flush_caches(struct
> intel_iommu
> >> *iommu,
> >>   	}
> >>   }
> >>
> >> +/*
> >> + * This function is supposed to be used after caller updates the fields
> >> + * except for the SSADE and P bit of a pasid table entry. It does the
> >> + * below:
> >> + * - Flush cacheline if needed
> >> + * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.
> >
> > while at it please add the name for the table.
> 
> sure.
> 
> >
> >> + *   ”Guidance to Software for Invalidations“
> >> + *
> >> + * Caller of it should not modify the in-use pasid table entries.
> >
> > I'm not sure about this statement. As long as the change doesn't
> > impact in-fly DMAs it's always the caller's right for whether to do it.
> >
> > actually based on the main intention of supporting replace it's
> > quite possible.
> 
> This comment is mainly due to the clflush_cache_range() within this
> helper. If caller modifies the pte content, it will need to call
> this again.
> 

Isn't it obvious about the latter part?

Re: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Yi Liu]

On 2024/11/6 17:40, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 6, 2024 4:46 PM
>>
>> On 2024/11/6 15:11, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>
>>>> There are more paths that need to flush cache for present pasid entry
>>>> after adding pasid replacement. Hence, add a helper for it. Per the
>>>> VT-d spec, the changes to the fields other than SSADE and P bit can
>>>> share the same code. So intel_pasid_setup_page_snoop_control() is the
>>>> first user of this helper.
>>>
>>> No hw spec would ever talk about coding sharing in sw implementation.
>>
>> yes, it's just sw choice.
>>
>>> and according to the following context the fact is just that two flows
>>> between RID and PASID are similar so you decide to create a common
>>> helper for both.
>>
>> I'm not quite getting why RID is related. This is only about the cache
>> flush per pasid entry updating.
> 
> the comment says:
> 
> +	 * - If (pasid is RID_PASID)
> +	 *    - Global Device-TLB invalidation to affected functions
> +	 *   Else
> +	 *    - PASID-based Device-TLB invalidation (with S=1 and
> +	 *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions
> 
> so that is the only difference between two invalidation flows?

oh, yes. But there are multiple PASID paths that need flush. e.g. the
pasid teardown. However, this path cannot use this helper introduced
here as it modifies the Present bit. Per table 28, the teardown path
should check pgtt to decide between p_iotlb_inv and iotlb_inv.

>>
>>>>
>>>> No functional change is intended.
>>>>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> ---
>>>>    drivers/iommu/intel/pasid.c | 54 ++++++++++++++++++++++++-------------
>>>>    1 file changed, 36 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>> index 977c4ac00c4c..81d038222414 100644
>>>> --- a/drivers/iommu/intel/pasid.c
>>>> +++ b/drivers/iommu/intel/pasid.c
>>>> @@ -286,6 +286,41 @@ static void pasid_flush_caches(struct
>> intel_iommu
>>>> *iommu,
>>>>    	}
>>>>    }
>>>>
>>>> +/*
>>>> + * This function is supposed to be used after caller updates the fields
>>>> + * except for the SSADE and P bit of a pasid table entry. It does the
>>>> + * below:
>>>> + * - Flush cacheline if needed
>>>> + * - Flush the caches per the guidance of VT-d spec 5.0 Table 28.
>>>
>>> while at it please add the name for the table.
>>
>> sure.
>>
>>>
>>>> + *   ”Guidance to Software for Invalidations“
>>>> + *
>>>> + * Caller of it should not modify the in-use pasid table entries.
>>>
>>> I'm not sure about this statement. As long as the change doesn't
>>> impact in-fly DMAs it's always the caller's right for whether to do it.
>>>
>>> actually based on the main intention of supporting replace it's
>>> quite possible.
>>
>> This comment is mainly due to the clflush_cache_range() within this
>> helper. If caller modifies the pte content, it will need to call
>> this again.
>>
> 
> Isn't it obvious about the latter part?

hmmm, I added it mainly by referring to the below helper. This helper
is for newly setup pasid entries, so in flight DMA is not relevant.

/*
  * This function flushes cache for a newly setup pasid table entry.
  * Caller of it should not modify the in-use pasid table entries.
  */
static void pasid_flush_caches(struct intel_iommu *iommu,
				struct pasid_entry *pte,
			       u32 pasid, u16 did)
{

-- 
Regards,
Yi Liu

RE: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 5:57 PM
> 
> On 2024/11/6 17:40, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Wednesday, November 6, 2024 4:46 PM
> >>
> >> On 2024/11/6 15:11, Tian, Kevin wrote:
> >>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>> Sent: Monday, November 4, 2024 9:19 PM
> >>>>
> >>>> There are more paths that need to flush cache for present pasid entry
> >>>> after adding pasid replacement. Hence, add a helper for it. Per the
> >>>> VT-d spec, the changes to the fields other than SSADE and P bit can
> >>>> share the same code. So intel_pasid_setup_page_snoop_control() is the
> >>>> first user of this helper.
> >>>
> >>> No hw spec would ever talk about coding sharing in sw implementation.
> >>
> >> yes, it's just sw choice.
> >>
> >>> and according to the following context the fact is just that two flows
> >>> between RID and PASID are similar so you decide to create a common
> >>> helper for both.
> >>
> >> I'm not quite getting why RID is related. This is only about the cache
> >> flush per pasid entry updating.
> >
> > the comment says:
> >
> > +	 * - If (pasid is RID_PASID)
> > +	 *    - Global Device-TLB invalidation to affected functions
> > +	 *   Else
> > +	 *    - PASID-based Device-TLB invalidation (with S=1 and
> > +	 *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions
> >
> > so that is the only difference between two invalidation flows?
> 
> oh, yes. But there are multiple PASID paths that need flush. e.g. the
> pasid teardown. However, this path cannot use this helper introduced
> here as it modifies the Present bit. Per table 28, the teardown path
> should check pgtt to decide between p_iotlb_inv and iotlb_inv.
> 

Then just say that this patch generalizes the logic for flushing
pasid cache upon changes to bits other than SSADE and P which
requires a different flow according to VT-d spec

Re: [PATCH v4 02/13] iommu/vt-d: Add a helper to flush cache for updating present pasid entry

[Yi Liu]

On 2024/11/6 18:01, Tian, Kevin wrote:

> Then just say that this patch generalizes the logic for flushing
> pasid cache upon changes to bits other than SSADE and P which
> requires a different flow according to VT-d spec

sure.

-- 
Regards,
Yi Liu

[PATCH v4 03/13] iommu/vt-d: Refactor the pasid setup helpers

[Yi Liu]

As iommu driver is going to support pasid replacement, the new pasid replace
helpers need to config the pasid entry as well. Hence, there are quite a few
code to be shared with existing pasid setup helpers. This moves the pasid
config codes into helpers which can be used by existing pasid setup helpers
and the future new pasid replace helpers.

No functional change is intended.

Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/pasid.c | 169 ++++++++++++++++++++++--------------
 1 file changed, 105 insertions(+), 64 deletions(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 81d038222414..65fd2fee01b7 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -325,6 +325,32 @@ static void intel_pasid_flush_present(struct intel_iommu *iommu,
  * Set up the scalable mode pasid table entry for first only
  * translation type.
  */
+static void pasid_pte_config_first_level(struct intel_iommu *iommu,
+					 struct pasid_entry *pte,
+					 pgd_t *pgd, u16 did, int flags)
+{
+	lockdep_assert_held(&iommu->lock);
+
+	pasid_clear_entry(pte);
+
+	/* Setup the first level page table pointer: */
+	pasid_set_flptr(pte, (u64)__pa(pgd));
+
+	if (flags & PASID_FLAG_FL5LP)
+		pasid_set_flpm(pte, 1);
+
+	if (flags & PASID_FLAG_PAGE_SNOOP)
+		pasid_set_pgsnp(pte);
+
+	pasid_set_domain_id(pte, did);
+	pasid_set_address_width(pte, iommu->agaw);
+	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+
+	/* Setup Present and PASID Granular Transfer Type: */
+	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_FL_ONLY);
+	pasid_set_present(pte);
+}
+
 int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 				  struct device *dev, pgd_t *pgd,
 				  u32 pasid, u16 did, int flags)
@@ -355,24 +381,8 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		return -EBUSY;
 	}
 
-	pasid_clear_entry(pte);
-
-	/* Setup the first level page table pointer: */
-	pasid_set_flptr(pte, (u64)__pa(pgd));
-
-	if (flags & PASID_FLAG_FL5LP)
-		pasid_set_flpm(pte, 1);
-
-	if (flags & PASID_FLAG_PAGE_SNOOP)
-		pasid_set_pgsnp(pte);
-
-	pasid_set_domain_id(pte, did);
-	pasid_set_address_width(pte, iommu->agaw);
-	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+	pasid_pte_config_first_level(iommu, pte, pgd, did, flags);
 
-	/* Setup Present and PASID Granular Transfer Type: */
-	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_FL_ONLY);
-	pasid_set_present(pte);
 	spin_unlock(&iommu->lock);
 
 	pasid_flush_caches(iommu, pte, pasid, did);
@@ -402,6 +412,26 @@ static int iommu_skip_agaw(struct dmar_domain *domain,
 /*
  * Set up the scalable mode pasid entry for second only translation type.
  */
+static void pasid_pte_config_second_level(struct intel_iommu *iommu,
+					  struct pasid_entry *pte,
+					  u64 pgd_val, int agaw, u16 did,
+					  bool dirty_tracking)
+{
+	lockdep_assert_held(&iommu->lock);
+
+	pasid_clear_entry(pte);
+	pasid_set_domain_id(pte, did);
+	pasid_set_slptr(pte, pgd_val);
+	pasid_set_address_width(pte, agaw);
+	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_SL_ONLY);
+	pasid_set_fault_enable(pte);
+	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+	if (dirty_tracking)
+		pasid_set_ssade(pte);
+
+	pasid_set_present(pte);
+}
+
 int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 				   struct dmar_domain *domain,
 				   struct device *dev, u32 pasid)
@@ -444,17 +474,8 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 		return -EBUSY;
 	}
 
-	pasid_clear_entry(pte);
-	pasid_set_domain_id(pte, did);
-	pasid_set_slptr(pte, pgd_val);
-	pasid_set_address_width(pte, agaw);
-	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_SL_ONLY);
-	pasid_set_fault_enable(pte);
-	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
-	if (domain->dirty_tracking)
-		pasid_set_ssade(pte);
-
-	pasid_set_present(pte);
+	pasid_pte_config_second_level(iommu, pte, pgd_val, agaw,
+				      did, domain->dirty_tracking);
 	spin_unlock(&iommu->lock);
 
 	pasid_flush_caches(iommu, pte, pasid, did);
@@ -534,6 +555,20 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
 /*
  * Set up the scalable mode pasid entry for passthrough translation type.
  */
+static void pasid_pte_config_pass_through(struct intel_iommu *iommu,
+					  struct pasid_entry *pte, u16 did)
+{
+	lockdep_assert_held(&iommu->lock);
+
+	pasid_clear_entry(pte);
+	pasid_set_domain_id(pte, did);
+	pasid_set_address_width(pte, iommu->agaw);
+	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_PT);
+	pasid_set_fault_enable(pte);
+	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+	pasid_set_present(pte);
+}
+
 int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 				   struct device *dev, u32 pasid)
 {
@@ -552,13 +587,7 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 		return -EBUSY;
 	}
 
-	pasid_clear_entry(pte);
-	pasid_set_domain_id(pte, did);
-	pasid_set_address_width(pte, iommu->agaw);
-	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_PT);
-	pasid_set_fault_enable(pte);
-	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
-	pasid_set_present(pte);
+	pasid_pte_config_pass_through(iommu, pte, did);
 	spin_unlock(&iommu->lock);
 
 	pasid_flush_caches(iommu, pte, pasid, did);
@@ -589,6 +618,46 @@ void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 	intel_pasid_flush_present(iommu, dev, pasid, did, pte);
 }
 
+static void pasid_pte_config_nestd(struct intel_iommu *iommu,
+				   struct pasid_entry *pte,
+				   struct iommu_hwpt_vtd_s1 *s1_cfg,
+				   struct dmar_domain *s2_domain,
+				   u16 did)
+{
+	struct dma_pte *pgd = s2_domain->pgd;
+
+	lockdep_assert_held(&iommu->lock);
+
+	pasid_clear_entry(pte);
+
+	if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL)
+		pasid_set_flpm(pte, 1);
+
+	pasid_set_flptr(pte, s1_cfg->pgtbl_addr);
+
+	if (s1_cfg->flags & IOMMU_VTD_S1_SRE) {
+		pasid_set_sre(pte);
+		if (s1_cfg->flags & IOMMU_VTD_S1_WPE)
+			pasid_set_wpe(pte);
+	}
+
+	if (s1_cfg->flags & IOMMU_VTD_S1_EAFE)
+		pasid_set_eafe(pte);
+
+	if (s2_domain->force_snooping)
+		pasid_set_pgsnp(pte);
+
+	pasid_set_slptr(pte, virt_to_phys(pgd));
+	pasid_set_fault_enable(pte);
+	pasid_set_domain_id(pte, did);
+	pasid_set_address_width(pte, s2_domain->agaw);
+	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+	if (s2_domain->dirty_tracking)
+		pasid_set_ssade(pte);
+	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_NESTED);
+	pasid_set_present(pte);
+}
+
 /**
  * intel_pasid_setup_nested() - Set up PASID entry for nested translation.
  * @iommu:      IOMMU which the device belong to
@@ -606,7 +675,6 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 	struct iommu_hwpt_vtd_s1 *s1_cfg = &domain->s1_cfg;
 	struct dmar_domain *s2_domain = domain->s2_domain;
 	u16 did = domain_id_iommu(domain, iommu);
-	struct dma_pte *pgd = s2_domain->pgd;
 	struct pasid_entry *pte;
 
 	/* Address width should match the address width supported by hardware */
@@ -649,34 +717,7 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 		return -EBUSY;
 	}
 
-	pasid_clear_entry(pte);
-
-	if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL)
-		pasid_set_flpm(pte, 1);
-
-	pasid_set_flptr(pte, s1_cfg->pgtbl_addr);
-
-	if (s1_cfg->flags & IOMMU_VTD_S1_SRE) {
-		pasid_set_sre(pte);
-		if (s1_cfg->flags & IOMMU_VTD_S1_WPE)
-			pasid_set_wpe(pte);
-	}
-
-	if (s1_cfg->flags & IOMMU_VTD_S1_EAFE)
-		pasid_set_eafe(pte);
-
-	if (s2_domain->force_snooping)
-		pasid_set_pgsnp(pte);
-
-	pasid_set_slptr(pte, virt_to_phys(pgd));
-	pasid_set_fault_enable(pte);
-	pasid_set_domain_id(pte, did);
-	pasid_set_address_width(pte, s2_domain->agaw);
-	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
-	if (s2_domain->dirty_tracking)
-		pasid_set_ssade(pte);
-	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_NESTED);
-	pasid_set_present(pte);
+	pasid_pte_config_nestd(iommu, pte, s1_cfg, s2_domain, did);
 	spin_unlock(&iommu->lock);
 
 	pasid_flush_caches(iommu, pte, pasid, did);
-- 
2.34.1

Re: [PATCH v4 03/13] iommu/vt-d: Refactor the pasid setup helpers

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> As iommu driver is going to support pasid replacement, the new pasid replace
> helpers need to config the pasid entry as well. Hence, there are quite a few
> code to be shared with existing pasid setup helpers. This moves the pasid
> config codes into helpers which can be used by existing pasid setup helpers
> and the future new pasid replace helpers.
> 
> No functional change is intended.
> 
> Suggested-by: Lu Baolu<baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 169 ++++++++++++++++++++++--------------
>   1 file changed, 105 insertions(+), 64 deletions(-)

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

RE: [PATCH v4 03/13] iommu/vt-d: Refactor the pasid setup helpers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> As iommu driver is going to support pasid replacement, the new pasid
> replace
> helpers need to config the pasid entry as well. Hence, there are quite a few
> code to be shared with existing pasid setup helpers. This moves the pasid
> config codes into helpers which can be used by existing pasid setup helpers
> and the future new pasid replace helpers.

hmm probably you can add a link to the discussion which suggested
to have separate replace/setup helpers. It's not intuitive to require
this if just talking about "to support pasid replacement"

> 
> No functional change is intended.
> 
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

Re: [PATCH v4 03/13] iommu/vt-d: Refactor the pasid setup helpers

[Yi Liu]

On 2024/11/6 15:14, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> As iommu driver is going to support pasid replacement, the new pasid
>> replace
>> helpers need to config the pasid entry as well. Hence, there are quite a few
>> code to be shared with existing pasid setup helpers. This moves the pasid
>> config codes into helpers which can be used by existing pasid setup helpers
>> and the future new pasid replace helpers.
> 
> hmm probably you can add a link to the discussion which suggested
> to have separate replace/setup helpers. It's not intuitive to require
> this if just talking about "to support pasid replacement"

this came from the offline chat with Baolu. He shared this idea to me
and it makes sense that we use pasid replace helpers instead of extending
the setup helpers. How about:

This driver is going to support pasid replacement. A choice is to extend
the existing pasid setup helpers which is for creating a pasid entry.
However, it might change some assumption on the setup helpers. So adding
separate pasid replace helpers is chosen. Then we need to split the common
code that manipulate the pasid entry out from the setup helpers, hence it
can be used by the replace helpers as well.

>>
>> No functional change is intended.
>>
>> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

-- 
Regards,
Yi Liu

RE: [PATCH v4 03/13] iommu/vt-d: Refactor the pasid setup helpers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 5:23 PM
> 
> On 2024/11/6 15:14, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, November 4, 2024 9:19 PM
> >>
> >> As iommu driver is going to support pasid replacement, the new pasid
> >> replace
> >> helpers need to config the pasid entry as well. Hence, there are quite a
> few
> >> code to be shared with existing pasid setup helpers. This moves the pasid
> >> config codes into helpers which can be used by existing pasid setup
> helpers
> >> and the future new pasid replace helpers.
> >
> > hmm probably you can add a link to the discussion which suggested
> > to have separate replace/setup helpers. It's not intuitive to require
> > this if just talking about "to support pasid replacement"
> 
> this came from the offline chat with Baolu. He shared this idea to me
> and it makes sense that we use pasid replace helpers instead of extending
> the setup helpers. How about:
> 
> This driver is going to support pasid replacement. A choice is to extend
> the existing pasid setup helpers which is for creating a pasid entry.
> However, it might change some assumption on the setup helpers. So adding
> separate pasid replace helpers is chosen. Then we need to split the common
> code that manipulate the pasid entry out from the setup helpers, hence it
> can be used by the replace helpers as well.
> 

Just put it simple e.g. "It is clearer to have a new set of pasid
replacement helpers other than extending the existing ones
to cover both initial setup and replacement. Then abstract out 
common code for manipulating the pasid entry as preparation."

[PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu]

pasid replacement allows converting a present pasid entry to be FS, SS,
PT or nested, hence add helpers for such operations. This simplifies the
callers as well since the caller can switch the pasid to the new domain
by one-shot.

Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/pasid.c | 173 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/pasid.h |  12 +++
 2 files changed, 185 insertions(+)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 65fd2fee01b7..b7c2d65b8726 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 	return 0;
 }
 
+int intel_pasid_replace_first_level(struct intel_iommu *iommu,
+				    struct device *dev, pgd_t *pgd,
+				    u32 pasid, u16 did, int flags)
+{
+	struct pasid_entry *pte;
+	u16 old_did;
+
+	if (!ecap_flts(iommu->ecap) ||
+	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
+		return -EINVAL;
+
+	spin_lock(&iommu->lock);
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (!pte) {
+		spin_unlock(&iommu->lock);
+		return -ENODEV;
+	}
+
+	if (!pasid_pte_is_present(pte)) {
+		spin_unlock(&iommu->lock);
+		return -EINVAL;
+	}
+
+	old_did = pasid_get_domain_id(pte);
+
+	pasid_pte_config_first_level(iommu, pte, pgd, did, flags);
+	spin_unlock(&iommu->lock);
+
+	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
+	intel_drain_pasid_prq(dev, pasid);
+
+	return 0;
+}
+
 /*
  * Skip top levels of page tables for iommu which has less agaw
  * than default. Unnecessary for PT mode.
@@ -483,6 +517,55 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 	return 0;
 }
 
+int intel_pasid_replace_second_level(struct intel_iommu *iommu,
+				     struct dmar_domain *domain,
+				     struct device *dev, u32 pasid)
+{
+	struct pasid_entry *pte;
+	struct dma_pte *pgd;
+	u16 did, old_did;
+	u64 pgd_val;
+	int agaw;
+
+	/*
+	 * If hardware advertises no support for second level
+	 * translation, return directly.
+	 */
+	if (!ecap_slts(iommu->ecap))
+		return -EINVAL;
+
+	pgd = domain->pgd;
+	agaw = iommu_skip_agaw(domain, iommu, &pgd);
+	if (agaw < 0)
+		return -EINVAL;
+
+	pgd_val = virt_to_phys(pgd);
+	did = domain_id_iommu(domain, iommu);
+
+	spin_lock(&iommu->lock);
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (!pte) {
+		spin_unlock(&iommu->lock);
+		return -ENODEV;
+	}
+
+	if (!pasid_pte_is_present(pte)) {
+		spin_unlock(&iommu->lock);
+		return -EINVAL;
+	}
+
+	old_did = pasid_get_domain_id(pte);
+
+	pasid_pte_config_second_level(iommu, pte, pgd_val, agaw,
+				      did, domain->dirty_tracking);
+	spin_unlock(&iommu->lock);
+
+	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
+	intel_drain_pasid_prq(dev, pasid);
+
+	return 0;
+}
+
 /*
  * Set up dirty tracking on a second only or nested translation type.
  */
@@ -595,6 +678,35 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 	return 0;
 }
 
+int intel_pasid_replace_pass_through(struct intel_iommu *iommu,
+				     struct device *dev, u32 pasid)
+{
+	u16 did = FLPT_DEFAULT_DID, old_did;
+	struct pasid_entry *pte;
+
+	spin_lock(&iommu->lock);
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (!pte) {
+		spin_unlock(&iommu->lock);
+		return -ENODEV;
+	}
+
+	if (!pasid_pte_is_present(pte)) {
+		spin_unlock(&iommu->lock);
+		return -EINVAL;
+	}
+
+	old_did = pasid_get_domain_id(pte);
+
+	pasid_pte_config_pass_through(iommu, pte, did);
+	spin_unlock(&iommu->lock);
+
+	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
+	intel_drain_pasid_prq(dev, pasid);
+
+	return 0;
+}
+
 /*
  * Set the page snoop control for a pasid entry which has been set up.
  */
@@ -725,6 +837,67 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 	return 0;
 }
 
+int intel_pasid_replace_nested(struct intel_iommu *iommu,
+			       struct device *dev, u32 pasid,
+			       struct dmar_domain *domain)
+{
+	struct iommu_hwpt_vtd_s1 *s1_cfg = &domain->s1_cfg;
+	u16 did = domain_id_iommu(domain, iommu), old_did;
+	struct dmar_domain *s2_domain = domain->s2_domain;
+	struct pasid_entry *pte;
+
+	/* Address width should match the address width supported by hardware */
+	switch (s1_cfg->addr_width) {
+	case ADDR_WIDTH_4LEVEL:
+		break;
+	case ADDR_WIDTH_5LEVEL:
+		if (!cap_fl5lp_support(iommu->cap)) {
+			dev_err_ratelimited(dev,
+					    "5-level paging not supported\n");
+			return -EINVAL;
+		}
+		break;
+	default:
+		dev_err_ratelimited(dev, "Invalid stage-1 address width %d\n",
+				    s1_cfg->addr_width);
+		return -EINVAL;
+	}
+
+	if ((s1_cfg->flags & IOMMU_VTD_S1_SRE) && !ecap_srs(iommu->ecap)) {
+		pr_err_ratelimited("No supervisor request support on %s\n",
+				   iommu->name);
+		return -EINVAL;
+	}
+
+	if ((s1_cfg->flags & IOMMU_VTD_S1_EAFE) && !ecap_eafs(iommu->ecap)) {
+		pr_err_ratelimited("No extended access flag support on %s\n",
+				   iommu->name);
+		return -EINVAL;
+	}
+
+	spin_lock(&iommu->lock);
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (!pte) {
+		spin_unlock(&iommu->lock);
+		return -ENODEV;
+	}
+
+	if (!pasid_pte_is_present(pte)) {
+		spin_unlock(&iommu->lock);
+		return -EINVAL;
+	}
+
+	old_did = pasid_get_domain_id(pte);
+
+	pasid_pte_config_nestd(iommu, pte, s1_cfg, s2_domain, did);
+	spin_unlock(&iommu->lock);
+
+	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
+	intel_drain_pasid_prq(dev, pasid);
+
+	return 0;
+}
+
 /*
  * Interfaces to setup or teardown a pasid table to the scalable-mode
  * context table entry:
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index dde6d3ba5ae0..228938f3be51 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -303,6 +303,18 @@ 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);
+int intel_pasid_replace_first_level(struct intel_iommu *iommu,
+				    struct device *dev, pgd_t *pgd,
+				    u32 pasid, u16 did, int flags);
+int intel_pasid_replace_second_level(struct intel_iommu *iommu,
+				     struct dmar_domain *domain,
+				     struct device *dev, u32 pasid);
+int intel_pasid_replace_pass_through(struct intel_iommu *iommu,
+				     struct device *dev, u32 pasid);
+int intel_pasid_replace_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);
-- 
2.34.1

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> pasid replacement allows converting a present pasid entry to be FS, SS,
> PT or nested, hence add helpers for such operations. This simplifies the
> callers as well since the caller can switch the pasid to the new domain
> by one-shot.
> 
> Suggested-by: Lu Baolu<baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 173 ++++++++++++++++++++++++++++++++++++
>   drivers/iommu/intel/pasid.h |  12 +++
>   2 files changed, 185 insertions(+)

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

with a nit below

> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 65fd2fee01b7..b7c2d65b8726 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>   	return 0;
>   }
>   
> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
> +				    struct device *dev, pgd_t *pgd,
> +				    u32 pasid, u16 did, int flags)
> +{
> +	struct pasid_entry *pte;
> +	u16 old_did;
> +
> +	if (!ecap_flts(iommu->ecap) ||
> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
> +		return -EINVAL;
> +
> +	spin_lock(&iommu->lock);
> +	pte = intel_pasid_get_entry(dev, pasid);
> +	if (!pte) {
> +		spin_unlock(&iommu->lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!pasid_pte_is_present(pte)) {
> +		spin_unlock(&iommu->lock);
> +		return -EINVAL;
> +	}
> +
> +	old_did = pasid_get_domain_id(pte);
> +
> +	pasid_pte_config_first_level(iommu, pte, pgd, did, flags);
> +	spin_unlock(&iommu->lock);
> +
> +	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
> +	intel_drain_pasid_prq(dev, pasid);
> +
> +	return 0;
> +}
> +
>   /*
>    * Skip top levels of page tables for iommu which has less agaw
>    * than default. Unnecessary for PT mode.
> @@ -483,6 +517,55 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
>   	return 0;
>   }
>   
> +int intel_pasid_replace_second_level(struct intel_iommu *iommu,
> +				     struct dmar_domain *domain,
> +				     struct device *dev, u32 pasid)
> +{
> +	struct pasid_entry *pte;
> +	struct dma_pte *pgd;
> +	u16 did, old_did;
> +	u64 pgd_val;
> +	int agaw;
> +
> +	/*
> +	 * If hardware advertises no support for second level
> +	 * translation, return directly.
> +	 */
> +	if (!ecap_slts(iommu->ecap))
> +		return -EINVAL;
> +
> +	pgd = domain->pgd;
> +	agaw = iommu_skip_agaw(domain, iommu, &pgd);

iommu_skip_agaw() has been removed after domain_alloc_paging is
supported in this driver. Perhaps you need a rebase if you have a new
version.

> +	if (agaw < 0)
> +		return -EINVAL;
> +
> +	pgd_val = virt_to_phys(pgd);
> +	did = domain_id_iommu(domain, iommu);
> +
> +	spin_lock(&iommu->lock);
> +	pte = intel_pasid_get_entry(dev, pasid);
> +	if (!pte) {
> +		spin_unlock(&iommu->lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!pasid_pte_is_present(pte)) {
> +		spin_unlock(&iommu->lock);
> +		return -EINVAL;
> +	}
> +
> +	old_did = pasid_get_domain_id(pte);
> +
> +	pasid_pte_config_second_level(iommu, pte, pgd_val, agaw,
> +				      did, domain->dirty_tracking);
> +	spin_unlock(&iommu->lock);
> +
> +	intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
> +	intel_drain_pasid_prq(dev, pasid);
> +
> +	return 0;
> +}

--
baolu

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu]

On 2024/11/5 10:06, Baolu Lu wrote:
> On 11/4/24 21:18, Yi Liu wrote:
>> pasid replacement allows converting a present pasid entry to be FS, SS,
>> PT or nested, hence add helpers for such operations. This simplifies the
>> callers as well since the caller can switch the pasid to the new domain
>> by one-shot.
>>
>> Suggested-by: Lu Baolu<baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 173 ++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/intel/pasid.h |  12 +++
>>   2 files changed, 185 insertions(+)
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> with a nit below
> 
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 65fd2fee01b7..b7c2d65b8726 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu 
>> *iommu,
>>       return 0;
>>   }
>> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
>> +                    struct device *dev, pgd_t *pgd,
>> +                    u32 pasid, u16 did, int flags)
>> +{
>> +    struct pasid_entry *pte;
>> +    u16 old_did;
>> +
>> +    if (!ecap_flts(iommu->ecap) ||
>> +        ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
>> +        return -EINVAL;
>> +
>> +    spin_lock(&iommu->lock);
>> +    pte = intel_pasid_get_entry(dev, pasid);
>> +    if (!pte) {
>> +        spin_unlock(&iommu->lock);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!pasid_pte_is_present(pte)) {
>> +        spin_unlock(&iommu->lock);
>> +        return -EINVAL;
>> +    }
>> +
>> +    old_did = pasid_get_domain_id(pte);
>> +
>> +    pasid_pte_config_first_level(iommu, pte, pgd, did, flags);
>> +    spin_unlock(&iommu->lock);
>> +
>> +    intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
>> +    intel_drain_pasid_prq(dev, pasid);
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * Skip top levels of page tables for iommu which has less agaw
>>    * than default. Unnecessary for PT mode.
>> @@ -483,6 +517,55 @@ int intel_pasid_setup_second_level(struct 
>> intel_iommu *iommu,
>>       return 0;
>>   }
>> +int intel_pasid_replace_second_level(struct intel_iommu *iommu,
>> +                     struct dmar_domain *domain,
>> +                     struct device *dev, u32 pasid)
>> +{
>> +    struct pasid_entry *pte;
>> +    struct dma_pte *pgd;
>> +    u16 did, old_did;
>> +    u64 pgd_val;
>> +    int agaw;
>> +
>> +    /*
>> +     * If hardware advertises no support for second level
>> +     * translation, return directly.
>> +     */
>> +    if (!ecap_slts(iommu->ecap))
>> +        return -EINVAL;
>> +
>> +    pgd = domain->pgd;
>> +    agaw = iommu_skip_agaw(domain, iommu, &pgd);
> 
> iommu_skip_agaw() has been removed after domain_alloc_paging is
> supported in this driver. Perhaps you need a rebase if you have a new
> version.

yep.

> 
>> +    if (agaw < 0)
>> +        return -EINVAL;
>> +
>> +    pgd_val = virt_to_phys(pgd);
>> +    did = domain_id_iommu(domain, iommu);
>> +
>> +    spin_lock(&iommu->lock);
>> +    pte = intel_pasid_get_entry(dev, pasid);
>> +    if (!pte) {
>> +        spin_unlock(&iommu->lock);
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!pasid_pte_is_present(pte)) {
>> +        spin_unlock(&iommu->lock);
>> +        return -EINVAL;
>> +    }
>> +
>> +    old_did = pasid_get_domain_id(pte);
>> +
>> +    pasid_pte_config_second_level(iommu, pte, pgd_val, agaw,
>> +                      did, domain->dirty_tracking);
>> +    spin_unlock(&iommu->lock);
>> +
>> +    intel_pasid_flush_present(iommu, dev, pasid, old_did, pte);
>> +    intel_drain_pasid_prq(dev, pasid);
>> +
>> +    return 0;
>> +}
> 
> -- 
> baolu
> 

-- 
Regards,
Yi Liu

RE: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> pasid replacement allows converting a present pasid entry to be FS, SS,
> PT or nested, hence add helpers for such operations. This simplifies the
> callers as well since the caller can switch the pasid to the new domain
> by one-shot.

'simplify' compared to what? if it's an obvious result from creating
the helpers then no need to talk about it.

> 
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel/pasid.c | 173
> ++++++++++++++++++++++++++++++++++++
>  drivers/iommu/intel/pasid.h |  12 +++
>  2 files changed, 185 insertions(+)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 65fd2fee01b7..b7c2d65b8726 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu
> *iommu,
>  	return 0;
>  }
> 
> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
> +				    struct device *dev, pgd_t *pgd,
> +				    u32 pasid, u16 did, int flags)
> +{
> +	struct pasid_entry *pte;
> +	u16 old_did;
> +
> +	if (!ecap_flts(iommu->ecap) ||
> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
> +		return -EINVAL;

better copy the error messages from the setup part.

there may be further chance to consolidate them later but no clear
reason why different error warning schemes should be used
between them.

same for other helpers.

> +
> +	spin_lock(&iommu->lock);
> +	pte = intel_pasid_get_entry(dev, pasid);
> +	if (!pte) {
> +		spin_unlock(&iommu->lock);
> +		return -ENODEV;
> +	}
> +
> +	if (!pasid_pte_is_present(pte)) {
> +		spin_unlock(&iommu->lock);
> +		return -EINVAL;
> +	}
> +
> +	old_did = pasid_get_domain_id(pte);

probably should pass the old domain in and check whether the
domain->did is same as the one in the pasid entry and warn otherwise.

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

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu]

On 2024/11/6 15:31, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> pasid replacement allows converting a present pasid entry to be FS, SS,
>> PT or nested, hence add helpers for such operations. This simplifies the
>> callers as well since the caller can switch the pasid to the new domain
>> by one-shot.
> 
> 'simplify' compared to what? if it's an obvious result from creating
> the helpers then no need to talk about it.

agreed, no need to talk about it.

>>
>> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 173
>> ++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/intel/pasid.h |  12 +++
>>   2 files changed, 185 insertions(+)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 65fd2fee01b7..b7c2d65b8726 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -390,6 +390,40 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>   	return 0;
>>   }
>>
>> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
>> +				    struct device *dev, pgd_t *pgd,
>> +				    u32 pasid, u16 did, int flags)
>> +{
>> +	struct pasid_entry *pte;
>> +	u16 old_did;
>> +
>> +	if (!ecap_flts(iommu->ecap) ||
>> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
>> +		return -EINVAL;
> 
> better copy the error messages from the setup part.
> 
> there may be further chance to consolidate them later but no clear
> reason why different error warning schemes should be used
> between them.
> 
> same for other helpers.

sure. I think Baolu has a point that this may be trigger-able by userspace
hence drop the error message to avoid DOS.

>> +
>> +	spin_lock(&iommu->lock);
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	if (!pte) {
>> +		spin_unlock(&iommu->lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!pasid_pte_is_present(pte)) {
>> +		spin_unlock(&iommu->lock);
>> +		return -EINVAL;
>> +	}
>> +
>> +	old_did = pasid_get_domain_id(pte);
> 
> probably should pass the old domain in and check whether the
> domain->did is same as the one in the pasid entry and warn otherwise.

this would be a sw bug. :) Do we really want to catch every bug by warn? :)

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

-- 
Regards,
Yi Liu

RE: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 5:31 PM
> 
> On 2024/11/6 15:31, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, November 4, 2024 9:19 PM
> >>
> >>
> >> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
> >> +				    struct device *dev, pgd_t *pgd,
> >> +				    u32 pasid, u16 did, int flags)
> >> +{
> >> +	struct pasid_entry *pte;
> >> +	u16 old_did;
> >> +
> >> +	if (!ecap_flts(iommu->ecap) ||
> >> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
> >> +		return -EINVAL;
> >
> > better copy the error messages from the setup part.
> >
> > there may be further chance to consolidate them later but no clear
> > reason why different error warning schemes should be used
> > between them.
> >
> > same for other helpers.
> 
> sure. I think Baolu has a point that this may be trigger-able by userspace
> hence drop the error message to avoid DOS.
>

Isn't the existing path also trigger-able by userspace? It's better to
have a consistent policy cross all paths then you can clean it up
together later. 

 
> >> +
> >> +	spin_lock(&iommu->lock);
> >> +	pte = intel_pasid_get_entry(dev, pasid);
> >> +	if (!pte) {
> >> +		spin_unlock(&iommu->lock);
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	if (!pasid_pte_is_present(pte)) {
> >> +		spin_unlock(&iommu->lock);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	old_did = pasid_get_domain_id(pte);
> >
> > probably should pass the old domain in and check whether the
> > domain->did is same as the one in the pasid entry and warn otherwise.
> 
> this would be a sw bug. :) Do we really want to catch every bug by warn? :)
> 

this one should not happen. If it does, something severe jumps out...

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu]

On 2024/11/6 17:51, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, November 6, 2024 5:31 PM
>>
>> On 2024/11/6 15:31, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>
>>>>
>>>> +int intel_pasid_replace_first_level(struct intel_iommu *iommu,
>>>> +				    struct device *dev, pgd_t *pgd,
>>>> +				    u32 pasid, u16 did, int flags)
>>>> +{
>>>> +	struct pasid_entry *pte;
>>>> +	u16 old_did;
>>>> +
>>>> +	if (!ecap_flts(iommu->ecap) ||
>>>> +	    ((flags & PASID_FLAG_FL5LP) && !cap_fl5lp_support(iommu->cap)))
>>>> +		return -EINVAL;
>>>
>>> better copy the error messages from the setup part.
>>>
>>> there may be further chance to consolidate them later but no clear
>>> reason why different error warning schemes should be used
>>> between them.
>>>
>>> same for other helpers.
>>
>> sure. I think Baolu has a point that this may be trigger-able by userspace
>> hence drop the error message to avoid DOS.
>>
> 
> Isn't the existing path also trigger-able by userspace? It's better to
> have a consistent policy cross all paths then you can clean it up
> together later.

I see. May we add ratelimit to it.

>   
>>>> +
>>>> +	spin_lock(&iommu->lock);
>>>> +	pte = intel_pasid_get_entry(dev, pasid);
>>>> +	if (!pte) {
>>>> +		spin_unlock(&iommu->lock);
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	if (!pasid_pte_is_present(pte)) {
>>>> +		spin_unlock(&iommu->lock);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	old_did = pasid_get_domain_id(pte);
>>>
>>> probably should pass the old domain in and check whether the
>>> domain->did is same as the one in the pasid entry and warn otherwise.
>>
>> this would be a sw bug. :) Do we really want to catch every bug by warn? :)
>>
> 
> this one should not happen. If it does, something severe jumps out...

yes. that's why I doubt if it's valuable to do it. It should be a vital
bug that bring us this warn. or instead of passing id old domain, how
about just old_did? We use the passed in did instead of using the did
from pte.

-- 
Regards,
Yi Liu

RE: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, November 6, 2024 6:02 PM
> 
> On 2024/11/6 17:51, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Wednesday, November 6, 2024 5:31 PM
> >>
> >> On 2024/11/6 15:31, Tian, Kevin wrote:
> >>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>> Sent: Monday, November 4, 2024 9:19 PM
> >>>>
> >>>> +
> >>>> +	spin_lock(&iommu->lock);
> >>>> +	pte = intel_pasid_get_entry(dev, pasid);
> >>>> +	if (!pte) {
> >>>> +		spin_unlock(&iommu->lock);
> >>>> +		return -ENODEV;
> >>>> +	}
> >>>> +
> >>>> +	if (!pasid_pte_is_present(pte)) {
> >>>> +		spin_unlock(&iommu->lock);
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	old_did = pasid_get_domain_id(pte);
> >>>
> >>> probably should pass the old domain in and check whether the
> >>> domain->did is same as the one in the pasid entry and warn otherwise.
> >>
> >> this would be a sw bug. :) Do we really want to catch every bug by warn? :)
> >>
> >
> > this one should not happen. If it does, something severe jumps out...
> 
> yes. that's why I doubt if it's valuable to do it. It should be a vital
> bug that bring us this warn. or instead of passing id old domain, how
> about just old_did? We use the passed in did instead of using the did
> from pte.
> 

My personal feeling - it's worth as such rare bug once happening 
would be very difficult to debug. the warning provides useful hint.

passing did is OK.

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Yi Liu]

On 2024/11/6 18:05, Tian, Kevin wrote:

> 
> My personal feeling - it's worth as such rare bug once happening
> would be very difficult to debug. the warning provides useful hint.
> 
> passing did is OK.

deal.

-- 
Regards,
Yi Liu

Re: [PATCH v4 04/13] iommu/vt-d: Add pasid replace helpers

[Baolu Lu]

On 2024/11/6 18:05, Tian, Kevin wrote:
>> From: Liu, Yi L<yi.l.liu@intel.com>
>> Sent: Wednesday, November 6, 2024 6:02 PM
>>
>> On 2024/11/6 17:51, Tian, Kevin wrote:
>>>> From: Liu, Yi L<yi.l.liu@intel.com>
>>>> Sent: Wednesday, November 6, 2024 5:31 PM
>>>>
>>>> On 2024/11/6 15:31, Tian, Kevin wrote:
>>>>>> From: Liu, Yi L<yi.l.liu@intel.com>
>>>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>>>
>>>>>> +
>>>>>> +	spin_lock(&iommu->lock);
>>>>>> +	pte = intel_pasid_get_entry(dev, pasid);
>>>>>> +	if (!pte) {
>>>>>> +		spin_unlock(&iommu->lock);
>>>>>> +		return -ENODEV;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!pasid_pte_is_present(pte)) {
>>>>>> +		spin_unlock(&iommu->lock);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	old_did = pasid_get_domain_id(pte);
>>>>> probably should pass the old domain in and check whether the
>>>>> domain->did is same as the one in the pasid entry and warn otherwise.
>>>> this would be a sw bug. 🙂 Do we really want to catch every bug by warn? 🙂
>>>>
>>> this one should not happen. If it does, something severe jumps out...
>> yes. that's why I doubt if it's valuable to do it. It should be a vital
>> bug that bring us this warn. or instead of passing id old domain, how
>> about just old_did? We use the passed in did instead of using the did
>> from pte.
>>
> My personal feeling - it's worth as such rare bug once happening
> would be very difficult to debug. the warning provides useful hint.

Agreed!

[PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Yi Liu]

To handle domain replacement, the intel_iommu_set_dev_pasid() needs to
keep the old configuration and the prepare for the new setup. This requires
a bit refactoring to prepare for it.

domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove
the dev_pasid_info which represents the association of the pasid/device and
domain. Till now, it's still not ready for replacement yet.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 86000901de46..6bc5ce03c6f5 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4252,8 +4252,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;
@@ -4261,10 +4261,12 @@ 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, false);
+	if (!domain)
+		return;
+
+	/* Identity domain has no meta data for pasid. */
+	if (domain->type == IOMMU_DOMAIN_IDENTITY)
 		return;
-	}
 
 	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
@@ -4282,8 +4284,54 @@ 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, false);
 	intel_drain_pasid_prq(dev, pasid);
+	domain_remove_dev_pasid(domain, dev, pasid);
+}
+
+static struct dev_pasid_info *
+domain_add_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);
+	struct intel_iommu *iommu = info->iommu;
+	struct dev_pasid_info *dev_pasid;
+	unsigned long flags;
+	int ret;
+
+	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+	if (!dev_pasid)
+		return ERR_PTR(-ENOMEM);
+
+	ret = domain_attach_iommu(dmar_domain, iommu);
+	if (ret)
+		goto out_free;
+
+	ret = cache_tag_assign_domain(dmar_domain, dev, pasid);
+	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,
@@ -4294,7 +4342,6 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 	struct intel_iommu *iommu = info->iommu;
 	struct dev_pasid_info *dev_pasid;
-	unsigned long flags;
 	int ret;
 
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
@@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (ret)
 		return ret;
 
-	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
-	if (!dev_pasid)
-		return -ENOMEM;
-
-	ret = domain_attach_iommu(dmar_domain, iommu);
-	if (ret)
-		goto out_free;
-
-	ret = cache_tag_assign_domain(dmar_domain, dev, pasid);
-	if (ret)
-		goto out_detach_iommu;
+	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
+	if (IS_ERR(dev_pasid))
+		return PTR_ERR(dev_pasid);
 
 	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
@@ -4329,24 +4368,17 @@ 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_remove_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);
+	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_remove_dev_pasid:
+	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> To handle domain replacement, the intel_iommu_set_dev_pasid() needs to
> keep the old configuration and the prepare for the new setup. This requires
> a bit refactoring to prepare for it.

Above description is a bit hard to understand, are you saying

... the intel_iommu_set_dev_pasid() needs to roll back to the old
configuration in the failure path, therefore refactor it to prepare for
the subsequent patches ...

?

> 
> domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove
> the dev_pasid_info which represents the association of the pasid/device and
> domain. Till now, it's still not ready for replacement yet.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 90 +++++++++++++++++++++++++------------
>   1 file changed, 61 insertions(+), 29 deletions(-)

The change itself looks good to me,

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

Re: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Yi Liu]

On 2024/11/5 10:49, Baolu Lu wrote:
> On 11/4/24 21:18, Yi Liu wrote:
>> To handle domain replacement, the intel_iommu_set_dev_pasid() needs to
>> keep the old configuration and the prepare for the new setup. This requires
>> a bit refactoring to prepare for it.
> 
> Above description is a bit hard to understand, are you saying
> 
> ... the intel_iommu_set_dev_pasid() needs to roll back to the old
> configuration in the failure path, therefore refactor it to prepare for
> the subsequent patches ...

This is the partial reason, but not the most related reason of this patch.
Say without this patch, the intel_iommu_set_dev_pasid() call avoid roll
back to the old configuration in the failure path as long as it calls the
pasid replace helpers. So I chose to describe like the above. Maybe another
choice is to name this patch as consolidate the dev_pasid_info adding and
removing to be a paired helpers. This can be used by other set_dev_pasid op
within intel iommu driver.

> ?
> 
>>
>> domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove
>> the dev_pasid_info which represents the association of the pasid/device and
>> domain. Till now, it's still not ready for replacement yet.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 90 +++++++++++++++++++++++++------------
>>   1 file changed, 61 insertions(+), 29 deletions(-)
> 
> The change itself looks good to me,
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 

-- 
Regards,
Yi Liu

RE: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, November 5, 2024 1:24 PM
> 
> On 2024/11/5 10:49, Baolu Lu wrote:
> > On 11/4/24 21:18, Yi Liu wrote:
> >> To handle domain replacement, the intel_iommu_set_dev_pasid() needs
> to
> >> keep the old configuration and the prepare for the new setup. This
> requires
> >> a bit refactoring to prepare for it.
> >
> > Above description is a bit hard to understand, are you saying
> >
> > ... the intel_iommu_set_dev_pasid() needs to roll back to the old
> > configuration in the failure path, therefore refactor it to prepare for
> > the subsequent patches ...
> 
> This is the partial reason, but not the most related reason of this patch.
> Say without this patch, the intel_iommu_set_dev_pasid() call avoid roll
> back to the old configuration in the failure path as long as it calls the
> pasid replace helpers. So I chose to describe like the above. Maybe another
> choice is to name this patch as consolidate the dev_pasid_info adding and
> removing to be a paired helpers. This can be used by other set_dev_pasid op
> within intel iommu driver.
> 

paired helpers is clearer.

RE: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> +
> +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, false);
>  	intel_drain_pasid_prq(dev, pasid);
> +	domain_remove_dev_pasid(domain, dev, pasid);

this changes the order between physical teardown and software teardown.

but looks harmless.

> @@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct
> iommu_domain *domain,
>  	if (ret)
>  		return ret;
> 
> -	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> -	if (!dev_pasid)
> -		return -ENOMEM;
> -
> -	ret = domain_attach_iommu(dmar_domain, iommu);
> -	if (ret)
> -		goto out_free;
> -
> -	ret = cache_tag_assign_domain(dmar_domain, dev, pasid);
> -	if (ret)
> -		goto out_detach_iommu;
> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
> +	if (IS_ERR(dev_pasid))
> +		return PTR_ERR(dev_pasid);
> 
>  	if (dmar_domain->use_first_level)
>  		ret = domain_setup_first_level(iommu, dmar_domain,

this also changes the order i.e. a dev_pasid might be valid in the list
before its pasid entry is configured. so other places walking the list
must not assume every node has a valid entry. what about adding
a note to the structure field?

> @@ -4329,24 +4368,17 @@ 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_remove_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);
> +	domain_remove_dev_pasid(old, dev, pasid);

My preference is moving the check of non-NULL old out here.

otherwise looks good,

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

Re: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Yi Liu]

On 2024/11/6 15:41, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> +
>> +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, false);
>>   	intel_drain_pasid_prq(dev, pasid);
>> +	domain_remove_dev_pasid(domain, dev, pasid);
> 
> this changes the order between physical teardown and software teardown.
> 
> but looks harmless.

yes.

>> @@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct
>> iommu_domain *domain,
>>   	if (ret)
>>   		return ret;
>>
>> -	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>> -	if (!dev_pasid)
>> -		return -ENOMEM;
>> -
>> -	ret = domain_attach_iommu(dmar_domain, iommu);
>> -	if (ret)
>> -		goto out_free;
>> -
>> -	ret = cache_tag_assign_domain(dmar_domain, dev, pasid);
>> -	if (ret)
>> -		goto out_detach_iommu;
>> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>> +	if (IS_ERR(dev_pasid))
>> +		return PTR_ERR(dev_pasid);
>>
>>   	if (dmar_domain->use_first_level)
>>   		ret = domain_setup_first_level(iommu, dmar_domain,
> 
> this also changes the order i.e. a dev_pasid might be valid in the list
> before its pasid entry is configured. so other places walking the list
> must not assume every node has a valid entry. what about adding
> a note to the structure field?

Do you mean a mark to say the entry is valid or not? Perhaps it's not
needed.  Even it is treated as a valid entry in the new domain or the
old domain, it looks to be fine. The major usage of this structure are
the cache invalidation (already dropped, but it is an example)and svm mm
release path. Either path looks to be fine as they just do more things
that are harmless.

>> @@ -4329,24 +4368,17 @@ 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_remove_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);
>> +	domain_remove_dev_pasid(old, dev, pasid);
> 
> My preference is moving the check of non-NULL old out here.

@Baolu, how about your thought?

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

-- 
Regards,
Yi Liu

Re: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Baolu Lu]

On 2024/11/6 16:02, Yi Liu wrote:
>>> @@ -4329,24 +4368,17 @@ 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_remove_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);
>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>
>> My preference is moving the check of non-NULL old out here.
> 
> @Baolu, how about your thought?

If we move the check out of this helper, there will be boilerplate code
in multiple places. Something like,

	if (old)
		domain_remove_dev_pasid(old, dev, pasid);

--
baolu

RE: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, November 6, 2024 4:39 PM
> 
> On 2024/11/6 16:02, Yi Liu wrote:
> >>> @@ -4329,24 +4368,17 @@ 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_remove_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);
> >>> +    domain_remove_dev_pasid(old, dev, pasid);
> >>
> >> My preference is moving the check of non-NULL old out here.
> >
> > @Baolu, how about your thought?
> 
> If we move the check out of this helper, there will be boilerplate code
> in multiple places. Something like,
> 
> 	if (old)
> 		domain_remove_dev_pasid(old, dev, pasid);
> 

yes, but the logic is slightly clearer to reflect a replace operation
in the caller side instead of pretending it to be a mandatory one.

[PATCH v4 06/13] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

Let intel_iommu_set_dev_pasid() call the pasid replace helpers hence be
able to do domain replacement.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 17 +++++++++++------
 drivers/iommu/intel/pasid.h | 11 +++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6bc5ce03c6f5..29a3d9de109c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1908,7 +1908,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 static int domain_setup_first_level(struct intel_iommu *iommu,
 				    struct dmar_domain *domain,
 				    struct device *dev,
-				    u32 pasid)
+				    u32 pasid, struct iommu_domain *old)
 {
 	struct dma_pte *pgd = domain->pgd;
 	int agaw, level;
@@ -1934,6 +1934,11 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
 	if (domain->force_snooping)
 		flags |= PASID_FLAG_PAGE_SNOOP;
 
+	if (old)
+		return intel_pasid_replace_first_level(iommu, dev,
+							  (pgd_t *)pgd, pasid,
+							  domain_id_iommu(domain, iommu),
+							  flags);
 	return intel_pasid_setup_first_level(iommu, dev, (pgd_t *)pgd, pasid,
 					     domain_id_iommu(domain, iommu),
 					     flags);
@@ -1968,9 +1973,9 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 	if (!sm_supported(iommu))
 		ret = domain_context_mapping(domain, dev);
 	else if (domain->use_first_level)
-		ret = domain_setup_first_level(iommu, domain, dev, IOMMU_NO_PASID);
+		ret = domain_setup_first_level(iommu, domain, dev, IOMMU_NO_PASID, NULL);
 	else
-		ret = intel_pasid_setup_second_level(iommu, domain, dev, IOMMU_NO_PASID);
+		ret = domain_setup_second_level(iommu, domain, dev, IOMMU_NO_PASID, NULL);
 
 	if (ret)
 		goto out_block_translation;
@@ -4363,10 +4368,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 
 	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
-					       dev, pasid);
+					       dev, pasid, old);
 	else
-		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
-						     dev, pasid);
+		ret = domain_setup_second_level(iommu, dmar_domain,
+						dev, pasid, old);
 	if (ret)
 		goto out_remove_dev_pasid;
 
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 228938f3be51..3f82f69a7bce 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -315,6 +315,17 @@ int intel_pasid_replace_nested(struct intel_iommu *iommu,
 			       struct device *dev, u32 pasid,
 			       struct dmar_domain *domain);
 
+static inline int domain_setup_second_level(struct intel_iommu *iommu,
+					    struct dmar_domain *domain,
+					    struct device *dev, ioasid_t pasid,
+					    struct iommu_domain *old)
+{
+	if (old)
+		return intel_pasid_replace_second_level(iommu, domain,
+							dev, pasid);
+	return intel_pasid_setup_second_level(iommu, domain, dev, pasid);
+}
+
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 				 struct device *dev, u32 pasid,
 				 bool fault_ignore);
-- 
2.34.1

Re: [PATCH v4 06/13] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> Let intel_iommu_set_dev_pasid() call the pasid replace helpers hence be
> able to do domain replacement.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 17 +++++++++++------
>   drivers/iommu/intel/pasid.h | 11 +++++++++++
>   2 files changed, 22 insertions(+), 6 deletions(-)

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

RE: [PATCH v4 06/13] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> 
> +static inline int domain_setup_second_level(struct intel_iommu *iommu,
> +					    struct dmar_domain *domain,
> +					    struct device *dev, ioasid_t pasid,
> +					    struct iommu_domain *old)
> +{
> +	if (old)
> +		return intel_pasid_replace_second_level(iommu, domain,
> +							dev, pasid);

As commented let's pass the old domain pointer into the replace
variants.

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

[PATCH v4 07/13] iommu/vt-d: Limit intel_iommu_set_dev_pasid() for paging domain

[Yi Liu]

intel_iommu_set_dev_pasid() is only supposed to be used by paging domain,
so limit it.

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 29a3d9de109c..58df1cbc1590 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4349,6 +4349,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	struct dev_pasid_info *dev_pasid;
 	int ret;
 
+	if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING)))
+		return -EINVAL;
+
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 		return -EOPNOTSUPP;
 
@@ -4377,8 +4380,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 
 	domain_remove_dev_pasid(old, dev, pasid);
 
-	if (domain->type & __IOMMU_DOMAIN_PAGING)
-		intel_iommu_debugfs_create_dev_pasid(dev_pasid);
+	intel_iommu_debugfs_create_dev_pasid(dev_pasid);
 
 	return 0;
 
-- 
2.34.1

Re: [PATCH v4 07/13] iommu/vt-d: Limit intel_iommu_set_dev_pasid() for paging domain

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> intel_iommu_set_dev_pasid() is only supposed to be used by paging domain,
> so limit it.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

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

RE: [PATCH v4 07/13] iommu/vt-d: Limit intel_iommu_set_dev_pasid() for paging domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> intel_iommu_set_dev_pasid() is only supposed to be used by paging domain,
> so limit it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH v4 08/13] iommu/vt-d: Make identity_domain_set_dev_pasid() to handle domain replacement

[Yi Liu]

Let identity_domain_set_dev_pasid() call the pasid replace helpers hence
be able to do domain replacement.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 8 +++++++-
 drivers/iommu/intel/pasid.h | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 58df1cbc1590..c5b07ccbe621 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4618,11 +4618,17 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
+	int ret;
 
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 		return -EOPNOTSUPP;
 
-	return intel_pasid_setup_pass_through(iommu, dev, pasid);
+	ret = domain_setup_passthrough(iommu, dev, pasid, old);
+	if (ret)
+		return ret;
+
+	domain_remove_dev_pasid(old, dev, pasid);
+	return 0;
 }
 
 static struct iommu_domain identity_domain = {
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 3f82f69a7bce..a3b5945a1812 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -326,6 +326,15 @@ static inline int domain_setup_second_level(struct intel_iommu *iommu,
 	return intel_pasid_setup_second_level(iommu, domain, dev, pasid);
 }
 
+static inline int domain_setup_passthrough(struct intel_iommu *iommu,
+					   struct device *dev, ioasid_t pasid,
+					   struct iommu_domain *old)
+{
+	if (old)
+		return intel_pasid_replace_pass_through(iommu, dev, pasid);
+	return intel_pasid_setup_pass_through(iommu, dev, pasid);
+}
+
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 				 struct device *dev, u32 pasid,
 				 bool fault_ignore);
-- 
2.34.1

Re: [PATCH v4 08/13] iommu/vt-d: Make identity_domain_set_dev_pasid() to handle domain replacement

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> Let identity_domain_set_dev_pasid() call the pasid replace helpers hence
> be able to do domain replacement.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 8 +++++++-
>   drivers/iommu/intel/pasid.h | 9 +++++++++
>   2 files changed, 16 insertions(+), 1 deletion(-)

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

RE: [PATCH v4 08/13] iommu/vt-d: Make identity_domain_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> Let identity_domain_set_dev_pasid() call the pasid replace helpers hence
> be able to do domain replacement.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

[PATCH v4 09/13] iommu/vt-d: Consolidate the dev_pasid code in intel_svm_set_dev_pasid()

[Yi Liu]

Use the domain_add_dev_pasid() and domain_remove_dev_pasid().

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c |  6 +++---
 drivers/iommu/intel/iommu.h |  6 ++++++
 drivers/iommu/intel/svm.c   | 28 +++++++---------------------
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c5b07ccbe621..0e0e9eb5188a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4257,8 +4257,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	return 0;
 }
 
-static void domain_remove_dev_pasid(struct iommu_domain *domain,
-				    struct device *dev, ioasid_t pasid)
+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;
@@ -4302,7 +4302,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_remove_dev_pasid(domain, dev, pasid);
 }
 
-static struct dev_pasid_info *
+struct dev_pasid_info *
 domain_add_dev_pasid(struct iommu_domain *domain,
 		     struct device *dev, ioasid_t pasid)
 {
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 009f518722e0..8e7ffb421ac4 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1232,6 +1232,12 @@ int prepare_domain_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
 
+struct dev_pasid_info *
+domain_add_dev_pasid(struct iommu_domain *domain,
+		     struct device *dev, ioasid_t pasid);
+void domain_remove_dev_pasid(struct iommu_domain *domain,
+			     struct device *dev, ioasid_t pasid);
+
 int dmar_ir_support(void);
 
 void iommu_flush_write_buffer(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 3b5e3da24f19..925afca7529e 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -201,43 +201,29 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 				   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 mm_struct *mm = domain->mm;
 	struct dev_pasid_info *dev_pasid;
 	unsigned long sflags;
-	unsigned long flags;
 	int ret = 0;
 
-	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
-	if (!dev_pasid)
-		return -ENOMEM;
-
-	dev_pasid->dev = dev;
-	dev_pasid->pasid = pasid;
-
-	ret = cache_tag_assign_domain(to_dmar_domain(domain), dev, pasid);
-	if (ret)
-		goto free_dev_pasid;
+	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
+	if (IS_ERR(dev_pasid))
+		return PTR_ERR(dev_pasid);
 
 	/* Setup the pasid table: */
 	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
 	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, pasid,
 					    FLPT_DEFAULT_DID, sflags);
 	if (ret)
-		goto unassign_tag;
+		goto out_remove_dev_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);
+	domain_remove_dev_pasid(old, dev, pasid);
 
 	return 0;
 
-unassign_tag:
-	cache_tag_unassign_domain(to_dmar_domain(domain), dev, pasid);
-free_dev_pasid:
-	kfree(dev_pasid);
-
+out_remove_dev_pasid:
+	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v4 09/13] iommu/vt-d: Consolidate the dev_pasid code in intel_svm_set_dev_pasid()

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> Use the domain_add_dev_pasid() and domain_remove_dev_pasid().
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c |  6 +++---
>   drivers/iommu/intel/iommu.h |  6 ++++++
>   drivers/iommu/intel/svm.c   | 28 +++++++---------------------
>   3 files changed, 16 insertions(+), 24 deletions(-)

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

RE: [PATCH v4 09/13] iommu/vt-d: Consolidate the dev_pasid code in intel_svm_set_dev_pasid()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> Use the domain_add_dev_pasid() and domain_remove_dev_pasid().
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

this could move to follow patch5 as it's an immediate user on the
two helpers before talking about replacement.

> @@ -201,43 +201,29 @@ static int intel_svm_set_dev_pasid(struct
> iommu_domain *domain,
>  				   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 mm_struct *mm = domain->mm;
>  	struct dev_pasid_info *dev_pasid;
>  	unsigned long sflags;
> -	unsigned long flags;
>  	int ret = 0;
> 
> -	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> -	if (!dev_pasid)
> -		return -ENOMEM;
> -
> -	dev_pasid->dev = dev;
> -	dev_pasid->pasid = pasid;
> -
> -	ret = cache_tag_assign_domain(to_dmar_domain(domain), dev,
> pasid);
> -	if (ret)
> -		goto free_dev_pasid;
> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
> +	if (IS_ERR(dev_pasid))
> +		return PTR_ERR(dev_pasid);
> 
>  	/* Setup the pasid table: */
>  	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ?
> PASID_FLAG_FL5LP : 0;
>  	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, pasid,
>  					    FLPT_DEFAULT_DID, sflags);
>  	if (ret)
> -		goto unassign_tag;
> +		goto out_remove_dev_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);
> +	domain_remove_dev_pasid(old, dev, pasid);
> 

this also changes the order between pasid entry setup and the list
update. and intel_mm_release() walks the dev_pasids list and
call intel_pasid_tear_down_entry() which WARN_ON if an entry
is not valid.

but looks it's still OK here as intel_mm_release() is triggered in
the last step of iommu_domain_free() which cannot happen
when an attach is still in progress.

Just raise this point in case something is overlooked. otherwise,

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

[PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Yi Liu]

There is no known usage that will attach SVA domain or detach SVA domain
by replacing PASID to or from SVA domain. It is supposed to use the
iommu_sva_{un}bind_device() which invoke the  iommu_{at|de}tach_device_pasid().
So Intel iommu driver decides to fail the domain replacement if the old
domain or new domain is SVA type.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 8 ++++++++
 drivers/iommu/intel/svm.c   | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 0e0e9eb5188a..7e82b3a4bba7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4352,6 +4352,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING)))
 		return -EINVAL;
 
+	/* No SVA domain replacement usage so far */
+	if (old && old->type == IOMMU_DOMAIN_SVA)
+		return -EOPNOTSUPP;
+
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 		return -EOPNOTSUPP;
 
@@ -4620,6 +4624,10 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
 	struct intel_iommu *iommu = info->iommu;
 	int ret;
 
+	/* No SVA domain replacement usage so far */
+	if (old && old->type == IOMMU_DOMAIN_SVA)
+		return -EOPNOTSUPP;
+
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 		return -EOPNOTSUPP;
 
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 925afca7529e..06404716ad37 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -207,6 +207,9 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long sflags;
 	int ret = 0;
 
+	if (old)
+		return -EOPNOTSUPP;
+
 	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
-- 
2.34.1

Re: [PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> There is no known usage that will attach SVA domain or detach SVA domain
> by replacing PASID to or from SVA domain. It is supposed to use the
> iommu_sva_{un}bind_device() which invoke the  iommu_{at|de}tach_device_pasid().
> So Intel iommu driver decides to fail the domain replacement if the old
> domain or new domain is SVA type.

I would suggest dropping this patch.

The iommu driver now supports switching SVA domains to and from other
types of domains. The current limitation comes from the iommu core,
where the SVA interface doesn't use replace.

Looking forward, iommu_detach_device_pasid() will be converted to switch
an SVA domain to a blocking domain. Once this is implemented, perhaps
the check and failure scenario will no longer be relevant.

--
baolu

Re: [PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Yi Liu]

On 2024/11/5 11:30, Baolu Lu wrote:
> On 11/4/24 21:18, Yi Liu wrote:
>> There is no known usage that will attach SVA domain or detach SVA domain
>> by replacing PASID to or from SVA domain. It is supposed to use the
>> iommu_sva_{un}bind_device() which invoke the  
>> iommu_{at|de}tach_device_pasid().
>> So Intel iommu driver decides to fail the domain replacement if the old
>> domain or new domain is SVA type.
> 
> I would suggest dropping this patch.
> 
> The iommu driver now supports switching SVA domains to and from other
> types of domains. The current limitation comes from the iommu core,
> where the SVA interface doesn't use replace.

I also noticed other iommu drivers (like ARM SMMUv3 and AMD iommu) would
support SVA replacement. So may we also make intel_svm_set_dev_pasid() be
able to handle the case in which @old==non-null? At first, it looks to be
dead code when SVA interface does not use replace. But this may make all
drivers aligned.

> Looking forward, iommu_detach_device_pasid() will be converted to switch
> an SVA domain to a blocking domain. Once this is implemented, perhaps
> the check and failure scenario will no longer be relevant.

you are right.

-- 
Regards,
Yi Liu

Re: [PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Baolu Lu]

On 11/5/24 13:30, Yi Liu wrote:
> On 2024/11/5 11:30, Baolu Lu wrote:
>> On 11/4/24 21:18, Yi Liu wrote:
>>> There is no known usage that will attach SVA domain or detach SVA domain
>>> by replacing PASID to or from SVA domain. It is supposed to use the
>>> iommu_sva_{un}bind_device() which invoke the iommu_{at|de} 
>>> tach_device_pasid().
>>> So Intel iommu driver decides to fail the domain replacement if the old
>>> domain or new domain is SVA type.
>>
>> I would suggest dropping this patch.
>>
>> The iommu driver now supports switching SVA domains to and from other
>> types of domains. The current limitation comes from the iommu core,
>> where the SVA interface doesn't use replace.
> 
> I also noticed other iommu drivers (like ARM SMMUv3 and AMD iommu) would
> support SVA replacement. So may we also make intel_svm_set_dev_pasid() be
> able to handle the case in which @old==non-null? At first, it looks to be
> dead code when SVA interface does not use replace. But this may make all
> drivers aligned.

yeah, that works for me.

Re: [PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Jason Gunthorpe]

On Tue, Nov 05, 2024 at 11:30:25AM +0800, Baolu Lu wrote:
> On 11/4/24 21:18, Yi Liu wrote:
> > There is no known usage that will attach SVA domain or detach SVA domain
> > by replacing PASID to or from SVA domain. It is supposed to use the
> > iommu_sva_{un}bind_device() which invoke the  iommu_{at|de}tach_device_pasid().
> > So Intel iommu driver decides to fail the domain replacement if the old
> > domain or new domain is SVA type.
> 
> I would suggest dropping this patch.

Me too

Drivers should not make assumptions like this, the driver facing API
is clear, set_dev_pasid() is supposed to make the pasid domain the
translation for the pasid and replace whatever happened to be there.
Ideally hitlessly.

Good driver structure should not require caring what used to be
attached to the PASID.

Jason

RE: [PATCH v4 10/13] iommu/vt-d: Fail SVA domain replacement

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, November 5, 2024 10:44 PM
> 
> On Tue, Nov 05, 2024 at 11:30:25AM +0800, Baolu Lu wrote:
> > On 11/4/24 21:18, Yi Liu wrote:
> > > There is no known usage that will attach SVA domain or detach SVA
> domain
> > > by replacing PASID to or from SVA domain. It is supposed to use the
> > > iommu_sva_{un}bind_device() which invoke the
> iommu_{at|de}tach_device_pasid().
> > > So Intel iommu driver decides to fail the domain replacement if the old
> > > domain or new domain is SVA type.
> >
> > I would suggest dropping this patch.
> 
> Me too
> 
> Drivers should not make assumptions like this, the driver facing API
> is clear, set_dev_pasid() is supposed to make the pasid domain the
> translation for the pasid and replace whatever happened to be there.
> Ideally hitlessly.
> 
> Good driver structure should not require caring what used to be
> attached to the PASID.
> 

Agree

[PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

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

Add intel_nested_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  |  2 +-
 drivers/iommu/intel/iommu.h  |  7 ++++++
 drivers/iommu/intel/nested.c | 43 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/pasid.h  | 11 +++++++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7e82b3a4bba7..7f1ca3c342a3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
 					     flags);
 }
 
-static bool dev_is_real_dma_subdevice(struct device *dev)
+bool dev_is_real_dma_subdevice(struct device *dev)
 {
 	return dev && dev_is_pci(dev) &&
 	       pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 8e7ffb421ac4..55478d7b64cf 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
 	return info->did;
 }
 
+static inline int domain_type_is_nested(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_NESTED;
+}
+
 /*
  * 0: readable
  * 1: writable
@@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
  */
 #define QI_OPT_WAIT_DRAIN		BIT(0)
 
+bool dev_is_real_dma_subdevice(struct device *dev);
+
 int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
 void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
 void device_block_translation(struct device *dev);
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 3ce3c4fd210e..890087f3509f 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
 	return ret;
 }
 
+static int intel_nested_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;
+
+	/* No SVA domain replacement usage so far */
+	if (old && old->type == IOMMU_DOMAIN_SVA)
+		return -EOPNOTSUPP;
+
+	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;
+
+	if (context_copied(iommu, info->bus, info->devfn))
+		return -EBUSY;
+
+	ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain,
+					   dev);
+	if (ret)
+		return ret;
+
+	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
+	if (IS_ERR(dev_pasid))
+		return PTR_ERR(dev_pasid);
+
+	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
+	if (ret)
+		goto out_remove_dev_pasid;
+
+	domain_remove_dev_pasid(old, dev, pasid);
+
+	return 0;
+
+out_remove_dev_pasid:
+	domain_remove_dev_pasid(domain, dev, pasid);
+	return ret;
+}
+
 static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.attach_dev		= intel_nested_attach_dev,
+	.set_dev_pasid		= intel_nested_set_dev_pasid,
 	.free			= intel_nested_domain_free,
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index a3b5945a1812..31a4e7c01853 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct intel_iommu *iommu,
 	return intel_pasid_setup_pass_through(iommu, dev, pasid);
 }
 
+static inline int domain_setup_nested(struct intel_iommu *iommu,
+				      struct dmar_domain *domain,
+				      struct device *dev, ioasid_t pasid,
+				      struct iommu_domain *old)
+{
+	if (old)
+		return intel_pasid_replace_nested(iommu, dev,
+						  pasid, domain);
+	return intel_pasid_setup_nested(iommu, dev, pasid, domain);
+}
+
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 				 struct device *dev, u32 pasid,
 				 bool fault_ignore);
-- 
2.34.1

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 11/4/24 21:18, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Add intel_nested_set_dev_pasid() to set a nested type domain to a PASID
> of a device.
> 

Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 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>

And convert the patch author to you.

> ---
>   drivers/iommu/intel/iommu.c  |  2 +-
>   drivers/iommu/intel/iommu.h  |  7 ++++++
>   drivers/iommu/intel/nested.c | 43 ++++++++++++++++++++++++++++++++++++
>   drivers/iommu/intel/pasid.h  | 11 +++++++++
>   4 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 7e82b3a4bba7..7f1ca3c342a3 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct intel_iommu *iommu,
>   					     flags);
>   }
>   
> -static bool dev_is_real_dma_subdevice(struct device *dev)
> +bool dev_is_real_dma_subdevice(struct device *dev)

How about making this a static inline in the header?

>   {
>   	return dev && dev_is_pci(dev) &&
>   	       pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev);
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 8e7ffb421ac4..55478d7b64cf 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>   	return info->did;
>   }
>   
> +static inline int domain_type_is_nested(struct dmar_domain *domain)
> +{
> +	return domain->domain.type == IOMMU_DOMAIN_NESTED;
> +}

Why do you need this?

> +
>   /*
>    * 0: readable
>    * 1: writable
> @@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
>    */
>   #define QI_OPT_WAIT_DRAIN		BIT(0)
>   
> +bool dev_is_real_dma_subdevice(struct device *dev);
> +
>   int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
>   void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
>   void device_block_translation(struct device *dev);
> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index 3ce3c4fd210e..890087f3509f 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
>   	return ret;
>   }
>   
> +static int intel_nested_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;
> +
> +	/* No SVA domain replacement usage so far */
> +	if (old && old->type == IOMMU_DOMAIN_SVA)
> +		return -EOPNOTSUPP;

No need for this check from driver's point of view. If there is really a
need, it should go to the iommu core.

> +
> +	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 > +		return -EOPNOTSUPP;> +
> +	if (context_copied(iommu, info->bus, info->devfn))
> +		return -EBUSY;
> +
> +	ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain,
> +					   dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
> +	if (IS_ERR(dev_pasid))
> +		return PTR_ERR(dev_pasid);
> +
> +	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
> +	if (ret)
> +		goto out_remove_dev_pasid;
> +
> +	domain_remove_dev_pasid(old, dev, pasid);
> +
> +	return 0;
> +
> +out_remove_dev_pasid:
> +	domain_remove_dev_pasid(domain, dev, pasid);
> +	return ret;
> +}
> +
>   static const struct iommu_domain_ops intel_nested_domain_ops = {
>   	.attach_dev		= intel_nested_attach_dev,
> +	.set_dev_pasid		= intel_nested_set_dev_pasid,
>   	.free			= intel_nested_domain_free,
>   	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
>   };
> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
> index a3b5945a1812..31a4e7c01853 100644
> --- a/drivers/iommu/intel/pasid.h
> +++ b/drivers/iommu/intel/pasid.h
> @@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct intel_iommu *iommu,
>   	return intel_pasid_setup_pass_through(iommu, dev, pasid);
>   }
>   
> +static inline int domain_setup_nested(struct intel_iommu *iommu,
> +				      struct dmar_domain *domain,
> +				      struct device *dev, ioasid_t pasid,
> +				      struct iommu_domain *old)
> +{
> +	if (old)
> +		return intel_pasid_replace_nested(iommu, dev,
> +						  pasid, domain);
> +	return intel_pasid_setup_nested(iommu, dev, pasid, domain);
> +}
> +
>   void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
>   				 struct device *dev, u32 pasid,
>   				 bool fault_ignore);

Others look good to me.

--
baolu

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/11/5 11:38, Baolu Lu wrote:
> On 11/4/24 21:18, Yi Liu wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>
>> Add intel_nested_set_dev_pasid() to set a nested type domain to a PASID
>> of a device.
>>
> 
> Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> 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>
> 
> And convert the patch author to you.

ok.

> 
>> ---
>>   drivers/iommu/intel/iommu.c  |  2 +-
>>   drivers/iommu/intel/iommu.h  |  7 ++++++
>>   drivers/iommu/intel/nested.c | 43 ++++++++++++++++++++++++++++++++++++
>>   drivers/iommu/intel/pasid.h  | 11 +++++++++
>>   4 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 7e82b3a4bba7..7f1ca3c342a3 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1944,7 +1944,7 @@ static int domain_setup_first_level(struct 
>> intel_iommu *iommu,
>>                            flags);
>>   }
>> -static bool dev_is_real_dma_subdevice(struct device *dev)
>> +bool dev_is_real_dma_subdevice(struct device *dev)
> 
> How about making this a static inline in the header?

got it.

> 
>>   {
>>       return dev && dev_is_pci(dev) &&
>>              pci_real_dma_dev(to_pci_dev(dev)) != to_pci_dev(dev);
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 8e7ffb421ac4..55478d7b64cf 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -818,6 +818,11 @@ domain_id_iommu(struct dmar_domain *domain, struct 
>> intel_iommu *iommu)
>>       return info->did;
>>   }
>> +static inline int domain_type_is_nested(struct dmar_domain *domain)
>> +{
>> +    return domain->domain.type == IOMMU_DOMAIN_NESTED;
>> +}
> 
> Why do you need this?

sigh, it should be dropped as we non longer share set_dev_pasid callback
with paging domain.

>> +
>>   /*
>>    * 0: readable
>>    * 1: writable
>> @@ -1225,6 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, 
>> u16 did, u64 addr,
>>    */
>>   #define QI_OPT_WAIT_DRAIN        BIT(0)
>> +bool dev_is_real_dma_subdevice(struct device *dev);
>> +
>>   int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu 
>> *iommu);
>>   void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu 
>> *iommu);
>>   void device_block_translation(struct device *dev);
>> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
>> index 3ce3c4fd210e..890087f3509f 100644
>> --- a/drivers/iommu/intel/nested.c
>> +++ b/drivers/iommu/intel/nested.c
>> @@ -130,8 +130,51 @@ static int intel_nested_cache_invalidate_user(struct 
>> iommu_domain *domain,
>>       return ret;
>>   }
>> +static int intel_nested_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;
>> +
>> +    /* No SVA domain replacement usage so far */
>> +    if (old && old->type == IOMMU_DOMAIN_SVA)
>> +        return -EOPNOTSUPP;
> 
> No need for this check from driver's point of view. If there is really a
> need, it should go to the iommu core.

I'm going to drop it per the discussion in patch 10.

> 
>> +
>> +    if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
>  > +        return -EOPNOTSUPP;> +
>> +    if (context_copied(iommu, info->bus, info->devfn))
>> +        return -EBUSY;
>> +
>> +    ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain,
>> +                       dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>> +    if (IS_ERR(dev_pasid))
>> +        return PTR_ERR(dev_pasid);
>> +
>> +    ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>> +    if (ret)
>> +        goto out_remove_dev_pasid;
>> +
>> +    domain_remove_dev_pasid(old, dev, pasid);
>> +
>> +    return 0;
>> +
>> +out_remove_dev_pasid:
>> +    domain_remove_dev_pasid(domain, dev, pasid);
>> +    return ret;
>> +}
>> +
>>   static const struct iommu_domain_ops intel_nested_domain_ops = {
>>       .attach_dev        = intel_nested_attach_dev,
>> +    .set_dev_pasid        = intel_nested_set_dev_pasid,
>>       .free            = intel_nested_domain_free,
>>       .cache_invalidate_user    = intel_nested_cache_invalidate_user,
>>   };
>> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
>> index a3b5945a1812..31a4e7c01853 100644
>> --- a/drivers/iommu/intel/pasid.h
>> +++ b/drivers/iommu/intel/pasid.h
>> @@ -335,6 +335,17 @@ static inline int domain_setup_passthrough(struct 
>> intel_iommu *iommu,
>>       return intel_pasid_setup_pass_through(iommu, dev, pasid);
>>   }
>> +static inline int domain_setup_nested(struct intel_iommu *iommu,
>> +                      struct dmar_domain *domain,
>> +                      struct device *dev, ioasid_t pasid,
>> +                      struct iommu_domain *old)
>> +{
>> +    if (old)
>> +        return intel_pasid_replace_nested(iommu, dev,
>> +                          pasid, domain);
>> +    return intel_pasid_setup_nested(iommu, dev, pasid, domain);
>> +}
>> +
>>   void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
>>                    struct device *dev, u32 pasid,
>>                    bool fault_ignore);
> 
> Others look good to me.
> 
> -- 
> baolu

-- 
Regards,
Yi Liu

RE: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Add intel_nested_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>

with Baolo's comment addressed:

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

RE: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, November 4, 2024 9:19 PM
> 
> +
> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
> +	if (IS_ERR(dev_pasid))
> +		return PTR_ERR(dev_pasid);
> +
> +	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
> +	if (ret)
> +		goto out_remove_dev_pasid;
> +
> +	domain_remove_dev_pasid(old, dev, pasid);
> +

forgot one thing. Why not required to create a debugfs entry for
nested dev_pasid?

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 2024/11/6 16:17, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 9:19 PM
>>
>> +
>> +	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>> +	if (IS_ERR(dev_pasid))
>> +		return PTR_ERR(dev_pasid);
>> +
>> +	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>> +	if (ret)
>> +		goto out_remove_dev_pasid;
>> +
>> +	domain_remove_dev_pasid(old, dev, pasid);
>> +
> 
> forgot one thing. Why not required to create a debugfs entry for
> nested dev_pasid?

This debugfs node is only created for paging domain.

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/11/6 16:41, Baolu Lu wrote:
> On 2024/11/6 16:17, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Monday, November 4, 2024 9:19 PM
>>>
>>> +
>>> +    dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>>> +    if (IS_ERR(dev_pasid))
>>> +        return PTR_ERR(dev_pasid);
>>> +
>>> +    ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>>> +    if (ret)
>>> +        goto out_remove_dev_pasid;
>>> +
>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>> +
>>
>> forgot one thing. Why not required to create a debugfs entry for
>> nested dev_pasid?
> 
> This debugfs node is only created for paging domain.

I think Kevin got one point. The debugfs is added when paging domain
is attached. How about the paging domains that is only used as nested
parent domain. We seem to lack a debugfs node for such paging domains.

-- 
Regards,
Yi Liu

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 2024/11/6 17:14, Yi Liu wrote:
> On 2024/11/6 16:41, Baolu Lu wrote:
>> On 2024/11/6 16:17, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>
>>>> +
>>>> +    dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>>>> +    if (IS_ERR(dev_pasid))
>>>> +        return PTR_ERR(dev_pasid);
>>>> +
>>>> +    ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>>>> +    if (ret)
>>>> +        goto out_remove_dev_pasid;
>>>> +
>>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>>> +
>>>
>>> forgot one thing. Why not required to create a debugfs entry for
>>> nested dev_pasid?
>>
>> This debugfs node is only created for paging domain.
> 
> I think Kevin got one point. The debugfs is added when paging domain
> is attached. How about the paging domains that is only used as nested
> parent domain. We seem to lack a debugfs node for such paging domains.

Are you talking about the nested parent domain? It's also a paging
domain, hence a debugfs node will be created.

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

On 2024/11/6 18:45, Baolu Lu wrote:
> On 2024/11/6 17:14, Yi Liu wrote:
>> On 2024/11/6 16:41, Baolu Lu wrote:
>>> On 2024/11/6 16:17, Tian, Kevin wrote:
>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>>
>>>>> +
>>>>> +    dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>>>>> +    if (IS_ERR(dev_pasid))
>>>>> +        return PTR_ERR(dev_pasid);
>>>>> +
>>>>> +    ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>>>>> +    if (ret)
>>>>> +        goto out_remove_dev_pasid;
>>>>> +
>>>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>>>> +
>>>>
>>>> forgot one thing. Why not required to create a debugfs entry for
>>>> nested dev_pasid?
>>>
>>> This debugfs node is only created for paging domain.
>>
>> I think Kevin got one point. The debugfs is added when paging domain
>> is attached. How about the paging domains that is only used as nested
>> parent domain. We seem to lack a debugfs node for such paging domains.
> 
> Are you talking about the nested parent domain? It's also a paging
> domain, hence a debugfs node will be created.

yes, nested parent domains. But as I mentioned, the debugfs node is created
only in the attach point so far. While the nested attach does not attach
the nested parent, it is subjected to the paging_domain_compatible()
check and contributes its pgd to act as SS page table in the pasid entry.
So it's missed though it should be in another patch to add it.

-- 
Regards,
Yi Liu

Re: [PATCH v4 11/13] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Baolu Lu]

On 2024/11/6 19:00, Yi Liu wrote:
> On 2024/11/6 18:45, Baolu Lu wrote:
>> On 2024/11/6 17:14, Yi Liu wrote:
>>> On 2024/11/6 16:41, Baolu Lu wrote:
>>>> On 2024/11/6 16:17, Tian, Kevin wrote:
>>>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>>>> Sent: Monday, November 4, 2024 9:19 PM
>>>>>>
>>>>>> +
>>>>>> +    dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>>>>>> +    if (IS_ERR(dev_pasid))
>>>>>> +        return PTR_ERR(dev_pasid);
>>>>>> +
>>>>>> +    ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
>>>>>> +    if (ret)
>>>>>> +        goto out_remove_dev_pasid;
>>>>>> +
>>>>>> +    domain_remove_dev_pasid(old, dev, pasid);
>>>>>> +
>>>>>
>>>>> forgot one thing. Why not required to create a debugfs entry for
>>>>> nested dev_pasid?
>>>>
>>>> This debugfs node is only created for paging domain.
>>>
>>> I think Kevin got one point. The debugfs is added when paging domain
>>> is attached. How about the paging domains that is only used as nested
>>> parent domain. We seem to lack a debugfs node for such paging domains.
>>
>> Are you talking about the nested parent domain? It's also a paging
>> domain, hence a debugfs node will be created.
> 
> yes, nested parent domains. But as I mentioned, the debugfs node is created
> only in the attach point so far. While the nested attach does not attach
> the nested parent, it is subjected to the paging_domain_compatible()
> check and contributes its pgd to act as SS page table in the pasid entry.
> So it's missed though it should be in another patch to add it.

I see. Thanks!

[PATCH v4 12/13] iommu/arm-smmu-v3: Make 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>
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-sva.c | 2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 9 +++------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 +-
 3 files changed, 5 insertions(+), 8 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 a40681f8c348..2d188d12f85c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2883,7 +2883,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,
@@ -2913,16 +2913,13 @@ 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 = {
 		.master = master,
-		/*
-		 * For now the core code prevents calling this when a domain is
-		 * 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 1e9952ca989f..52eaa0bedee1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -875,7 +875,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 v4 12/13] iommu/arm-smmu-v3: Make set_dev_pasid() op support replace

[Will Deacon]

On Mon, Nov 04, 2024 at 05:18:41AM -0800, 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>
> 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-sva.c | 2 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 9 +++------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 +-
>  3 files changed, 5 insertions(+), 8 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

[PATCH v4 13/13] 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>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.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 d1dfc745f55e..8c73a30c2800 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 e1c8e92170e9..2ec0c9915aa5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -614,7 +614,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