[PATCH v3 0/9] Make set_dev_pasid op supporting domain replacement

[PATCH v3 0/9] Make set_dev_pasid op supporting domain replacement

[Yi Liu]

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

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

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

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

v3:
 - 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 (7):
  iommu: Pass old domain to set_dev_pasid op
  iommu/vt-d: Move intel_drain_pasid_prq() into
    intel_pasid_tear_down_entry()
  iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry
  iommu/vt-d: Make pasid setup helpers support modifying present pasid
    entry
  iommu/vt-d: Rename prepare_domain_attach_device()
  iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
    replacement
  iommu: Make set_dev_pasid op support domain replacement

 drivers/iommu/amd/amd_iommu.h                 |   3 +-
 drivers/iommu/amd/pasid.c                     |   6 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   5 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  12 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +-
 drivers/iommu/intel/iommu.c                   | 128 ++++++++++++------
 drivers/iommu/intel/iommu.h                   |   7 +-
 drivers/iommu/intel/nested.c                  |   3 +-
 drivers/iommu/intel/pasid.c                   |  82 ++++++-----
 drivers/iommu/intel/pasid.h                   |   9 +-
 drivers/iommu/intel/svm.c                     |   6 +-
 drivers/iommu/iommu.c                         |   3 +-
 include/linux/iommu.h                         |   5 +-
 13 files changed, 163 insertions(+), 108 deletions(-)

-- 
2.34.1

[PATCH v3 1/9] 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>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/amd/amd_iommu.h                   | 3 ++-
 drivers/iommu/amd/pasid.c                       | 3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++-
 drivers/iommu/intel/iommu.c                     | 6 ++++--
 drivers/iommu/intel/svm.c                       | 3 ++-
 drivers/iommu/iommu.c                           | 3 ++-
 include/linux/iommu.h                           | 2 +-
 7 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6386fa4556d9..b11b014fa82d 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -52,7 +52,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/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef..d5e3e0e79599 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4285,7 +4285,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);
@@ -4571,7 +4572,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 83c8e617a2c5..f3f81c04b8fb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3331,7 +3331,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	int ret;
 
 	for_each_group_device(group, device) {
-		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
+		ret = domain->ops->set_dev_pasid(domain, device->dev,
+						 pasid, NULL);
 		if (ret)
 			goto err_revert;
 	}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index bd722f473635..32dce80aa7fd 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -642,7 +642,7 @@ struct iommu_ops {
 struct iommu_domain_ops {
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
-			     ioasid_t pasid);
+			     ioasid_t pasid, struct iommu_domain *old);
 
 	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
-- 
2.34.1

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

[Yi Liu]

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

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d5e3e0e79599..ae3522a1e025 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3362,7 +3362,7 @@ void device_block_translation(struct device *dev)
 	if (!dev_is_real_dma_subdevice(dev)) {
 		if (sm_supported(iommu))
 			intel_pasid_tear_down_entry(iommu, dev,
-						    IOMMU_NO_PASID, false);
+						    IOMMU_NO_PASID, 0);
 		else
 			domain_context_clear(info);
 	}
@@ -4260,7 +4260,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	unsigned long flags;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
 		return;
 	}
 
@@ -4280,8 +4280,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_pasid);
-	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
-	intel_drain_pasid_prq(dev, pasid);
+	intel_pasid_tear_down_entry(iommu, dev, pasid,
+				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 2e5fa0a23299..2898e7af2cf4 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -236,8 +236,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 		qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT);
 }
 
+/*
+ * Caller can request to drain PRQ in this helper if it hasn't done so,
+ * e.g. in a path which doesn't follow remove_dev_pasid().
+ */
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
-				 u32 pasid, bool fault_ignore)
+				 u32 pasid, u32 flags)
 {
 	struct pasid_entry *pte;
 	u16 did, pgtt;
@@ -251,7 +255,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 
 	did = pasid_get_domain_id(pte);
 	pgtt = pasid_pte_get_pgtt(pte);
-	intel_pasid_clear_entry(dev, pasid, fault_ignore);
+	intel_pasid_clear_entry(dev, pasid,
+				flags & INTEL_PASID_TEARDOWN_IGNORE_FAULT);
 	spin_unlock(&iommu->lock);
 
 	if (!ecap_coherent(iommu->ecap))
@@ -265,6 +270,9 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
 
 	devtlb_invalidation_with_pasid(iommu, dev, pasid);
+
+	if (flags & INTEL_PASID_TEARDOWN_DRAIN_PRQ)
+		intel_drain_pasid_prq(dev, pasid);
 }
 
 /*
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index dde6d3ba5ae0..7dc9e4dfbd88 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -303,9 +303,11 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 				   struct device *dev, u32 pasid);
 int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 			     u32 pasid, struct dmar_domain *domain);
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
-				 struct device *dev, u32 pasid,
-				 bool fault_ignore);
+
+#define INTEL_PASID_TEARDOWN_IGNORE_FAULT	BIT(0)
+#define INTEL_PASID_TEARDOWN_DRAIN_PRQ		BIT(1)
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
+				 u32 pasid, u32 flags);
 void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 					  struct device *dev, u32 pasid);
 int intel_pasid_setup_sm_context(struct device *dev);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 3b5e3da24f19..f6cb35e9e6a8 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -176,7 +176,8 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
 		info = dev_iommu_priv_get(dev_pasid->dev);
 		intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
-					    dev_pasid->pasid, true);
+					    dev_pasid->pasid,
+					    INTEL_PASID_TEARDOWN_IGNORE_FAULT);
 	}
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-- 
2.34.1

[PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Yi Liu]

intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
There are paths that need to get the pasid entry, tear it down and
re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
entry can avoid duplicate codes to get the pasid entry. No functional
change is intended.

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

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 2898e7af2cf4..336f9425214c 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 /*
  * Caller can request to drain PRQ in this helper if it hasn't done so,
  * e.g. in a path which doesn't follow remove_dev_pasid().
+ * Return the pasid entry pointer if the entry is found or NULL if no
+ * entry found.
  */
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
-				 u32 pasid, u32 flags)
+struct pasid_entry *
+intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
+			    u32 pasid, u32 flags)
 {
 	struct pasid_entry *pte;
 	u16 did, pgtt;
@@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 	pte = intel_pasid_get_entry(dev, pasid);
 	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
 		spin_unlock(&iommu->lock);
-		return;
+		goto out;
 	}
 
 	did = pasid_get_domain_id(pte);
@@ -273,6 +276,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 
 	if (flags & INTEL_PASID_TEARDOWN_DRAIN_PRQ)
 		intel_drain_pasid_prq(dev, pasid);
+out:
+	return pte;
 }
 
 /*
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 7dc9e4dfbd88..9b2351325b0e 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -306,8 +306,9 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 
 #define INTEL_PASID_TEARDOWN_IGNORE_FAULT	BIT(0)
 #define INTEL_PASID_TEARDOWN_DRAIN_PRQ		BIT(1)
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
-				 u32 pasid, u32 flags);
+struct pasid_entry *
+intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
+			    u32 pasid, u32 flags);
 void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 					  struct device *dev, u32 pasid);
 int intel_pasid_setup_sm_context(struct device *dev);
-- 
2.34.1

[PATCH v3 4/9] iommu/vt-d: Make pasid setup helpers support modifying present pasid entry

[Yi Liu]

To handle domain replacement, the set_dev_pasid() op needs to modify a
present pasid entry.

A natural way to implement the set_dev_pasid() op is to reuse the logic
of remove_dev_pasid() in the beginning to remove the old configuration.
Then set up the new pasid entry. Roll back to the old domain if it fails
to set up the new pasid entry. This needs to invoke the set_dev_pasid op
of the old domain. While this breaks the iommu layering a bit.

An alternative is implementing the set_dev_pasid() without rollback to the
old domain. This requires putting all the pasid entry modifications in
the pasid setup helpers. While the set_dev_pasid() op calls the helpers when
all the preparation work such as memory allocation, and sanity check has been
done.

To support modifying present pasid entry, the setup helpers needs to call
intel_pasid_tear_down_entry() to destroy the old configuration, which also
includes the necessary cache flushing and PRQ draining.

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

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 336f9425214c..ce0a3bf701df 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -321,18 +321,13 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		return -EINVAL;
 	}
 
-	spin_lock(&iommu->lock);
-	pte = intel_pasid_get_entry(dev, pasid);
-	if (!pte) {
-		spin_unlock(&iommu->lock);
+	/* Destroy the old configuration if it already exists */
+	pte = intel_pasid_tear_down_entry(iommu, dev, pasid,
+					  INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+	if (!pte)
 		return -ENODEV;
-	}
-
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
 
+	spin_lock(&iommu->lock);
 	pasid_clear_entry(pte);
 
 	/* Setup the first level page table pointer: */
@@ -407,21 +402,16 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 		return -EINVAL;
 	}
 
+	/* Destroy the old configuration if it already exists */
+	pte = intel_pasid_tear_down_entry(iommu, dev, pasid,
+					  INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+	if (!pte)
+		return -ENODEV;
+
 	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 -EBUSY;
-	}
-
 	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
 	pasid_set_slptr(pte, pgd_val);
@@ -518,18 +508,13 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 	u16 did = FLPT_DEFAULT_DID;
 	struct pasid_entry *pte;
 
-	spin_lock(&iommu->lock);
-	pte = intel_pasid_get_entry(dev, pasid);
-	if (!pte) {
-		spin_unlock(&iommu->lock);
+	/* Destroy the old configuration if it already exists */
+	pte = intel_pasid_tear_down_entry(iommu, dev, pasid,
+					  INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+	if (!pte)
 		return -ENODEV;
-	}
-
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
 
+	spin_lock(&iommu->lock);
 	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
 	pasid_set_address_width(pte, iommu->agaw);
@@ -634,17 +619,13 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 		return -EINVAL;
 	}
 
-	spin_lock(&iommu->lock);
-	pte = intel_pasid_get_entry(dev, pasid);
-	if (!pte) {
-		spin_unlock(&iommu->lock);
+	/* Destroy the old configuration if it already exists */
+	pte = intel_pasid_tear_down_entry(iommu, dev, pasid,
+					  INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+	if (!pte)
 		return -ENODEV;
-	}
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
 
+	spin_lock(&iommu->lock);
 	pasid_clear_entry(pte);
 
 	if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL)
-- 
2.34.1

[PATCH v3 5/9] iommu/vt-d: Rename prepare_domain_attach_device()

[Yi Liu]

This helper is to ensure the domain is compatible with the device's iommu,
so it is more sanity check than preparation. Hence, rename it.

Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 8 ++++----
 drivers/iommu/intel/iommu.h  | 4 ++--
 drivers/iommu/intel/nested.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ae3522a1e025..8d92a221d020 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3581,8 +3581,8 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)
 	domain_exit(dmar_domain);
 }
 
-int prepare_domain_attach_device(struct iommu_domain *domain,
-				 struct device *dev)
+int domain_attach_device_sanitize(struct iommu_domain *domain,
+				  struct device *dev)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -3632,7 +3632,7 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 
 	device_block_translation(dev);
 
-	ret = prepare_domain_attach_device(domain, dev);
+	ret = domain_attach_device_sanitize(domain, dev);
 	if (ret)
 		return ret;
 
@@ -4304,7 +4304,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (context_copied(iommu, info->bus, info->devfn))
 		return -EBUSY;
 
-	ret = prepare_domain_attach_device(domain, dev);
+	ret = domain_attach_device_sanitize(domain, dev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 1497f3112b12..b020ae90c47e 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1230,8 +1230,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
 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);
-int prepare_domain_attach_device(struct iommu_domain *domain,
-				 struct device *dev);
+int domain_attach_device_sanitize(struct iommu_domain *domain,
+				  struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
 
 int dmar_ir_support(void);
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 433c58944401..c1e97ad6be24 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -40,7 +40,7 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
 	 * The s2_domain will be used in nested translation, hence needs
 	 * to ensure the s2_domain is compatible with this IOMMU.
 	 */
-	ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev);
+	ret = domain_attach_device_sanitize(&dmar_domain->s2_domain->domain, dev);
 	if (ret) {
 		dev_err_ratelimited(dev, "s2 domain is not compatible\n");
 		return ret;
-- 
2.34.1

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

[Yi Liu]

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

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

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8d92a221d020..302260898c36 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4250,8 +4250,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;
@@ -4259,11 +4259,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	struct dmar_domain *dmar_domain;
 	unsigned long flags;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		intel_pasid_tear_down_entry(iommu, dev, pasid, 0);
-		return;
-	}
-
 	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
@@ -4280,13 +4275,27 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_pasid);
+}
+
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *domain)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
 	intel_pasid_tear_down_entry(iommu, dev, pasid,
 				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
+
+	/* Identity domain has no meta data for pasid. */
+	if (domain->type == IOMMU_DOMAIN_IDENTITY)
+		return;
+
+	domain_remove_dev_pasid(domain, dev, pasid);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+static struct dev_pasid_info *
+domain_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);
@@ -4295,22 +4304,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
-		return -EOPNOTSUPP;
-
-	if (domain->dirty_ops)
-		return -EINVAL;
-
-	if (context_copied(iommu, info->bus, info->devfn))
-		return -EBUSY;
-
 	ret = domain_attach_device_sanitize(domain, dev);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
 	if (!dev_pasid)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	ret = domain_attach_iommu(dmar_domain, iommu);
 	if (ret)
@@ -4320,6 +4320,43 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (ret)
 		goto out_detach_iommu;
 
+	dev_pasid->dev = dev;
+	dev_pasid->pasid = pasid;
+	spin_lock_irqsave(&dmar_domain->lock, flags);
+	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+
+	return dev_pasid;
+out_detach_iommu:
+	domain_detach_iommu(dmar_domain, iommu);
+out_free:
+	kfree(dev_pasid);
+	return ERR_PTR(ret);
+}
+
+static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	struct dev_pasid_info *dev_pasid;
+	int ret;
+
+	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;
+
+	if (domain->dirty_ops)
+		return -EINVAL;
+
+	if (context_copied(iommu, info->bus, info->devfn))
+		return -EBUSY;
+
+	dev_pasid = domain_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,
 					       dev, pasid);
@@ -4327,24 +4364,18 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
 						     dev, pasid);
 	if (ret)
-		goto out_unassign_tag;
+		goto out_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);
+	if (old)
+		domain_remove_dev_pasid(old, dev, pasid);
 
 	if (domain->type & __IOMMU_DOMAIN_PAGING)
 		intel_iommu_debugfs_create_dev_pasid(dev_pasid);
 
 	return 0;
-out_unassign_tag:
-	cache_tag_unassign_domain(dmar_domain, dev, pasid);
-out_detach_iommu:
-	domain_detach_iommu(dmar_domain, iommu);
-out_free:
-	kfree(dev_pasid);
+
+out_remove_dev_pasid:
+	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
 }
 
-- 
2.34.1

[PATCH v3 7/9] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

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

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

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@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  | 23 ++++++++++++++++++-----
 drivers/iommu/intel/iommu.h  |  3 +++
 drivers/iommu/intel/nested.c |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 302260898c36..d089ac148a7e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -284,6 +284,11 @@ static int __init intel_iommu_setup(char *str)
 }
 __setup("intel_iommu=", intel_iommu_setup);
 
+static int domain_type_is_nested(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_NESTED;
+}
+
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -4304,7 +4309,12 @@ domain_add_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	ret = domain_attach_device_sanitize(domain, dev);
+	/* Nested type domain should sanitize its parent domain */
+	if (domain_type_is_nested(dmar_domain))
+		ret = domain_attach_device_sanitize(
+				&dmar_domain->s2_domain->domain, dev);
+	else
+		ret = domain_attach_device_sanitize(domain, dev);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -4334,9 +4344,9 @@ domain_add_dev_pasid(struct iommu_domain *domain,
 	return ERR_PTR(ret);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4357,7 +4367,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
 
-	if (dmar_domain->use_first_level)
+	if (domain_type_is_nested(dmar_domain))
+		ret = intel_pasid_setup_nested(iommu, dev, pasid,
+					       dmar_domain);
+	else if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
 	else
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index b020ae90c47e..d045397b0a4c 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1233,6 +1233,9 @@ void device_block_translation(struct device *dev);
 int domain_attach_device_sanitize(struct iommu_domain *domain,
 				  struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old);
 
 int dmar_ir_support(void);
 
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index c1e97ad6be24..d57abca32810 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -132,6 +132,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.attach_dev		= intel_nested_attach_dev,
+	.set_dev_pasid		= intel_iommu_set_dev_pasid,
 	.free			= intel_nested_domain_free,
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
-- 
2.34.1

[PATCH v3 8/9] 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>
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     | 12 +++++-------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     |  2 +-
 3 files changed, 7 insertions(+), 9 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 737c5b882355..f70165f544df 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);
@@ -2882,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,
@@ -2912,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

[PATCH v3 9/9] 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>
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 32dce80aa7fd..27f923450a7c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -616,7 +616,8 @@ struct iommu_ops {
  * * EBUSY	- device is attached to a domain and cannot be changed
  * * ENODEV	- device specific errors, not able to be attached
  * * <others>	- treated as ENODEV by the caller. Use is discouraged
- * @set_dev_pasid: set an iommu domain to a pasid of device
+ * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The pasid of
+ *                 the device should be left in the old config in error case.
  * @map_pages: map a physically contiguous set of pages of the same size to
  *             an iommu domain.
  * @unmap_pages: unmap a number of pages of the same size from an iommu domain
-- 
2.34.1

Re: [PATCH v3 1/9] iommu: Pass old domain to set_dev_pasid op

[Baolu Lu]

On 2024/10/18 13:53, 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>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/amd/amd_iommu.h                   | 3 ++-
>   drivers/iommu/amd/pasid.c                       | 3 ++-
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++-
>   drivers/iommu/intel/iommu.c                     | 6 ++++--
>   drivers/iommu/intel/svm.c                       | 3 ++-
>   drivers/iommu/iommu.c                           | 3 ++-
>   include/linux/iommu.h                           | 2 +-
>   7 files changed, 15 insertions(+), 8 deletions(-)

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

Thanks,
baolu

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

[Baolu Lu]

On 2024/10/18 13:53, Yi Liu wrote:
> Draining PRQ is mostly conjuncted with pasid teardown, and with more
> callers coming, it makes sense to move it into the
> intel_pasid_tear_down_entry(). But there is scenario that only teardown
> pasid entry but no PRQ drain, so passing a flag to mark it.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c |  8 ++++----
>   drivers/iommu/intel/pasid.c | 12 ++++++++++--
>   drivers/iommu/intel/pasid.h |  8 +++++---
>   drivers/iommu/intel/svm.c   |  3 ++-
>   4 files changed, 21 insertions(+), 10 deletions(-)

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

Thanks,
baolu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu]

On 2024/10/18 13:53, Yi Liu wrote:
> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
> There are paths that need to get the pasid entry, tear it down and
> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
> entry can avoid duplicate codes to get the pasid entry. No functional
> change is intended.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>   drivers/iommu/intel/pasid.h |  5 +++--
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 2898e7af2cf4..336f9425214c 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
>   /*
>    * Caller can request to drain PRQ in this helper if it hasn't done so,
>    * e.g. in a path which doesn't follow remove_dev_pasid().
> + * Return the pasid entry pointer if the entry is found or NULL if no
> + * entry found.
>    */
> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
> -				 u32 pasid, u32 flags)
> +struct pasid_entry *
> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
> +			    u32 pasid, u32 flags)
>   {
>   	struct pasid_entry *pte;
>   	u16 did, pgtt;
> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
>   	pte = intel_pasid_get_entry(dev, pasid);
>   	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>   		spin_unlock(&iommu->lock);
> -		return;
> +		goto out;

The pasid table entry is protected by iommu->lock. It's  not reasonable
to return the pte pointer which is beyond the lock protected range.

Thanks,
baolu

Re: [PATCH v3 5/9] iommu/vt-d: Rename prepare_domain_attach_device()

[Baolu Lu]

On 2024/10/18 13:53, Yi Liu wrote:
> This helper is to ensure the domain is compatible with the device's iommu,
> so it is more sanity check than preparation. Hence, rename it.
> 
> Suggested-by: Lu Baolu<baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/iommu.c  | 8 ++++----
>   drivers/iommu/intel/iommu.h  | 4 ++--
>   drivers/iommu/intel/nested.c | 2 +-
>   3 files changed, 7 insertions(+), 7 deletions(-)

Yes, this helper calls for a name change. I have already a patch in the
coming v2 of below series:

https://lore.kernel.org/linux-iommu/20241011042722.73930-8-baolu.lu@linux.intel.com/

Just for your information to avoid duplication.

Thanks,
baolu

Re: [PATCH v3 9/9] iommu: Make set_dev_pasid op support domain replacement

[Baolu Lu]

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

I would suggest merging this patch with patch 1/9.

Thanks,
baolu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Yi Liu]

On 2024/10/21 14:13, Baolu Lu wrote:
> On 2024/10/18 13:53, Yi Liu wrote:
>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>> There are paths that need to get the pasid entry, tear it down and
>> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
>> entry can avoid duplicate codes to get the pasid entry. No functional
>> change is intended.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>   drivers/iommu/intel/pasid.h |  5 +++--
>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 2898e7af2cf4..336f9425214c 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu 
>> *iommu,
>>   /*
>>    * Caller can request to drain PRQ in this helper if it hasn't done so,
>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>> + * Return the pasid entry pointer if the entry is found or NULL if no
>> + * entry found.
>>    */
>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>> device *dev,
>> -                 u32 pasid, u32 flags)
>> +struct pasid_entry *
>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
>> +                u32 pasid, u32 flags)
>>   {
>>       struct pasid_entry *pte;
>>       u16 did, pgtt;
>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu 
>> *iommu, struct device *dev,
>>       pte = intel_pasid_get_entry(dev, pasid);
>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>           spin_unlock(&iommu->lock);
>> -        return;
>> +        goto out;
> 
> The pasid table entry is protected by iommu->lock. It's  not reasonable
> to return the pte pointer which is beyond the lock protected range.

Per my understanding, the iommu->lock protects the content of the entry,
so the modifications to the entry need to hold it. While, it looks not
necessary to protect the pasid entry pointer itself. The pasid table should
exist during device probe and release. is it?

-- 
Regards,
Yi Liu

Re: [PATCH v3 5/9] iommu/vt-d: Rename prepare_domain_attach_device()

[Yi Liu]

On 2024/10/21 14:18, Baolu Lu wrote:
> On 2024/10/18 13:53, Yi Liu wrote:
>> This helper is to ensure the domain is compatible with the device's iommu,
>> so it is more sanity check than preparation. Hence, rename it.
>>
>> Suggested-by: Lu Baolu<baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c  | 8 ++++----
>>   drivers/iommu/intel/iommu.h  | 4 ++--
>>   drivers/iommu/intel/nested.c | 2 +-
>>   3 files changed, 7 insertions(+), 7 deletions(-)
> 
> Yes, this helper calls for a name change. I have already a patch in the
> coming v2 of below series:
> 
> https://lore.kernel.org/linux-iommu/20241011042722.73930-8-baolu.lu@linux.intel.com/
> 
> Just for your information to avoid duplication.

then I can drop it. It's not a critical patch in this series. :)

-- 
Regards,
Yi Liu

Re: [PATCH v3 9/9] iommu: Make set_dev_pasid op support domain replacement

[Yi Liu]

On 2024/10/21 14:27, Baolu Lu wrote:
> On 2024/10/18 13:54, Yi Liu wrote:
>> The iommu core is going to support domain replacement for pasid, it needs
>> to make the set_dev_pasid op support replacing domain and keep the old
>> domain config in the failure case.
>>
>> AMD iommu driver does not support domain replacement for pasid yet, so it
>> would fail the set_dev_pasid op to keep the old config if the input @old
>> is non-NULL.
>>
>> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
>> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/amd/pasid.c | 3 +++
>>   include/linux/iommu.h     | 3 ++-
>>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> I would suggest merging this patch with patch 1/9.

If merging it with patch 01, none of the drivers is ready for the new
definition. With the current order, the new definition are claimed after
all the drivers are ready. So it seems more reasonable. Also good from
bisect p.o.v. is it?:)

-- 
Regards,
Yi Liu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu]

On 2024/10/21 14:35, Yi Liu wrote:
> On 2024/10/21 14:13, Baolu Lu wrote:
>> On 2024/10/18 13:53, Yi Liu wrote:
>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>> There are paths that need to get the pasid entry, tear it down and
>>> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>> change is intended.
>>>
>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>> index 2898e7af2cf4..336f9425214c 100644
>>> --- a/drivers/iommu/intel/pasid.c
>>> +++ b/drivers/iommu/intel/pasid.c
>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>> intel_iommu *iommu,
>>>   /*
>>>    * Caller can request to drain PRQ in this helper if it hasn't done 
>>> so,
>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>> + * entry found.
>>>    */
>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>> device *dev,
>>> -                 u32 pasid, u32 flags)
>>> +struct pasid_entry *
>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device 
>>> *dev,
>>> +                u32 pasid, u32 flags)
>>>   {
>>>       struct pasid_entry *pte;
>>>       u16 did, pgtt;
>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>> intel_iommu *iommu, struct device *dev,
>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>           spin_unlock(&iommu->lock);
>>> -        return;
>>> +        goto out;
>>
>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>> to return the pte pointer which is beyond the lock protected range.
> 
> Per my understanding, the iommu->lock protects the content of the entry,
> so the modifications to the entry need to hold it. While, it looks not
> necessary to protect the pasid entry pointer itself. The pasid table should
> exist during device probe and release. is it?

The pattern of the code that modifies a pasid table entry is,

	spin_lock(&iommu->lock);
	pte = intel_pasid_get_entry(dev, pasid);
	... modify the pasid table entry ...
	spin_unlock(&iommu->lock);

Returning the pte pointer to the caller introduces a potential race
condition. If the caller subsequently modifies the pte without re-
acquiring the spin lock, there's a risk of data corruption or
inconsistencies.

Thanks,
baolu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Yi Liu]

On 2024/10/21 14:59, Baolu Lu wrote:
> On 2024/10/21 14:35, Yi Liu wrote:
>> On 2024/10/21 14:13, Baolu Lu wrote:
>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>>> There are paths that need to get the pasid entry, tear it down and
>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
>>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>>> change is intended.
>>>>
>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>> ---
>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>> index 2898e7af2cf4..336f9425214c 100644
>>>> --- a/drivers/iommu/intel/pasid.c
>>>> +++ b/drivers/iommu/intel/pasid.c
>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu 
>>>> *iommu,
>>>>   /*
>>>>    * Caller can request to drain PRQ in this helper if it hasn't done so,
>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>>> + * entry found.
>>>>    */
>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>> device *dev,
>>>> -                 u32 pasid, u32 flags)
>>>> +struct pasid_entry *
>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device 
>>>> *dev,
>>>> +                u32 pasid, u32 flags)
>>>>   {
>>>>       struct pasid_entry *pte;
>>>>       u16 did, pgtt;
>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu 
>>>> *iommu, struct device *dev,
>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>           spin_unlock(&iommu->lock);
>>>> -        return;
>>>> +        goto out;
>>>
>>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>>> to return the pte pointer which is beyond the lock protected range.
>>
>> Per my understanding, the iommu->lock protects the content of the entry,
>> so the modifications to the entry need to hold it. While, it looks not
>> necessary to protect the pasid entry pointer itself. The pasid table should
>> exist during device probe and release. is it?
> 
> The pattern of the code that modifies a pasid table entry is,
> 
>      spin_lock(&iommu->lock);
>      pte = intel_pasid_get_entry(dev, pasid);
>      ... modify the pasid table entry ...
>      spin_unlock(&iommu->lock);
> 
> Returning the pte pointer to the caller introduces a potential race
> condition. If the caller subsequently modifies the pte without re-
> acquiring the spin lock, there's a risk of data corruption or
> inconsistencies.

it appears that we are on the same page about if pte pointer needs to be
protected or not. And I agree the modifications to the pte should be
protected by iommu->lock. If so, will documenting that the caller must hold
iommu->lock if is tries to modify the content of pte work? Also, it might
be helpful to add lockdep to make sure all the modifications of pte entry
are under protection.

Or any suggestion from you given a path that needs to get pte first, check
if it exists and then call intel_pasid_tear_down_entry(). For example the
intel_pasid_setup_first_level() [1], in my series, I need to call the
unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
iommu->lock and do more modifications on the pasid entry. It would invoke
the intel_pasid_get_entry() twice if no change to
intel_pasid_tear_down_entry().

[1] 
https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/pasid.c#L317

-- 
Regards,
Yi Liu

Re: [PATCH v3 9/9] iommu: Make set_dev_pasid op support domain replacement

[Vasant Hegde]

On 10/18/2024 11:24 AM, Yi Liu wrote:
> The iommu core is going to support domain replacement for pasid, it needs
> to make the set_dev_pasid op support replacing domain and keep the old
> domain config in the failure case.
> 
> AMD iommu driver does not support domain replacement for pasid yet, so it
> would fail the set_dev_pasid op to keep the old config if the input @old
> is non-NULL.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

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

-Vasant

Re: [PATCH v3 1/9] iommu: Pass old domain to set_dev_pasid op

[Nicolin Chen]

On Thu, Oct 17, 2024 at 10:53:54PM -0700, Yi Liu wrote:
> To support domain replacement for pasid, the underlying iommu driver needs
> to know the old domain hence be able to clean up the existing attachment.
> It would be much convenient for iommu layer to pass down the old domain.
> Otherwise, iommu drivers would need to track domain for pasids by themselves,
> this would duplicate code among the iommu drivers. Or iommu drivers would
> rely group->pasid_array to get domain, which may not always the correct
> one.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Re: [PATCH v3 8/9] iommu/arm-smmu-v3: Make set_dev_pasid() op support replace

[Nicolin Chen]

On Thu, Oct 17, 2024 at 10:54:01PM -0700, Yi Liu wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> set_dev_pasid() op is going to be enhanced to support domain replacement
> of a pasid. This prepares for this op definition.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

With one thing:

> 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 737c5b882355..f70165f544df 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)

Seems that this should be squashed to PATCH-1. This function is
another set_dev_pasid op for the default domain, while the one
in the PATCH-1 is for sva domains.

Thanks
Nicolin

Re: [PATCH v3 8/9] iommu/arm-smmu-v3: Make set_dev_pasid() op support replace

[Yi Liu]

On 2024/10/22 13:25, Nicolin Chen wrote:
> On Thu, Oct 17, 2024 at 10:54:01PM -0700, Yi Liu wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>>
>> set_dev_pasid() op is going to be enhanced to support domain replacement
>> of a pasid. This prepares for this op definition.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With one thing:
> 
>> 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 737c5b882355..f70165f544df 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)
> 
> Seems that this should be squashed to PATCH-1. This function is
> another set_dev_pasid op for the default domain, while the one
> in the PATCH-1 is for sva domains.

you are right, let me fix it. :)

-- 
Regards,
Yi Liu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu]

On 2024/10/21 15:24, Yi Liu wrote:
> On 2024/10/21 14:59, Baolu Lu wrote:
>> On 2024/10/21 14:35, Yi Liu wrote:
>>> On 2024/10/21 14:13, Baolu Lu wrote:
>>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>>>> There are paths that need to get the pasid entry, tear it down and
>>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the 
>>>>> pasid
>>>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>>>> change is intended.
>>>>>
>>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>>> ---
>>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>>> index 2898e7af2cf4..336f9425214c 100644
>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>>>> intel_iommu *iommu,
>>>>>   /*
>>>>>    * Caller can request to drain PRQ in this helper if it hasn't 
>>>>> done so,
>>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>>>> + * entry found.
>>>>>    */
>>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>> device *dev,
>>>>> -                 u32 pasid, u32 flags)
>>>>> +struct pasid_entry *
>>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>> device *dev,
>>>>> +                u32 pasid, u32 flags)
>>>>>   {
>>>>>       struct pasid_entry *pte;
>>>>>       u16 did, pgtt;
>>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>>>> intel_iommu *iommu, struct device *dev,
>>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>>           spin_unlock(&iommu->lock);
>>>>> -        return;
>>>>> +        goto out;
>>>>
>>>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>>>> to return the pte pointer which is beyond the lock protected range.
>>>
>>> Per my understanding, the iommu->lock protects the content of the entry,
>>> so the modifications to the entry need to hold it. While, it looks not
>>> necessary to protect the pasid entry pointer itself. The pasid table 
>>> should
>>> exist during device probe and release. is it?
>>
>> The pattern of the code that modifies a pasid table entry is,
>>
>>      spin_lock(&iommu->lock);
>>      pte = intel_pasid_get_entry(dev, pasid);
>>      ... modify the pasid table entry ...
>>      spin_unlock(&iommu->lock);
>>
>> Returning the pte pointer to the caller introduces a potential race
>> condition. If the caller subsequently modifies the pte without re-
>> acquiring the spin lock, there's a risk of data corruption or
>> inconsistencies.
> 
> it appears that we are on the same page about if pte pointer needs to be
> protected or not. And I agree the modifications to the pte should be
> protected by iommu->lock. If so, will documenting that the caller must hold
> iommu->lock if is tries to modify the content of pte work? Also, it might
> be helpful to add lockdep to make sure all the modifications of pte entry
> are under protection.

People will soon forget about this lock and may modify the returned pte
pointer without locking, introducing a race condition silently.

> Or any suggestion from you given a path that needs to get pte first, check
> if it exists and then call intel_pasid_tear_down_entry(). For example the
> intel_pasid_setup_first_level() [1], in my series, I need to call the
> unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
> iommu->lock and do more modifications on the pasid entry. It would invoke
> the intel_pasid_get_entry() twice if no change to
> intel_pasid_tear_down_entry().

There is no need to check the present of a pte entry before calling into
intel_pasid_tear_down_entry(). The helper will return directly if the
pte is not present:

         spin_lock(&iommu->lock);
         pte = intel_pasid_get_entry(dev, pasid);
         if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
                 spin_unlock(&iommu->lock);
                 return;
         }

Does it work for you?

Thanks,
baolu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Yi Liu]

On 2024/10/22 17:23, Baolu Lu wrote:
> On 2024/10/21 15:24, Yi Liu wrote:
>> On 2024/10/21 14:59, Baolu Lu wrote:
>>> On 2024/10/21 14:35, Yi Liu wrote:
>>>> On 2024/10/21 14:13, Baolu Lu wrote:
>>>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>>>>> There are paths that need to get the pasid entry, tear it down and
>>>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
>>>>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>>>>> change is intended.
>>>>>>
>>>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>>>> ---
>>>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>>>> index 2898e7af2cf4..336f9425214c 100644
>>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>>>>> intel_iommu *iommu,
>>>>>>   /*
>>>>>>    * Caller can request to drain PRQ in this helper if it hasn't done 
>>>>>> so,
>>>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>>>>> + * entry found.
>>>>>>    */
>>>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>>> device *dev,
>>>>>> -                 u32 pasid, u32 flags)
>>>>>> +struct pasid_entry *
>>>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device 
>>>>>> *dev,
>>>>>> +                u32 pasid, u32 flags)
>>>>>>   {
>>>>>>       struct pasid_entry *pte;
>>>>>>       u16 did, pgtt;
>>>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>>>>> intel_iommu *iommu, struct device *dev,
>>>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>>>           spin_unlock(&iommu->lock);
>>>>>> -        return;
>>>>>> +        goto out;
>>>>>
>>>>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>>>>> to return the pte pointer which is beyond the lock protected range.
>>>>
>>>> Per my understanding, the iommu->lock protects the content of the entry,
>>>> so the modifications to the entry need to hold it. While, it looks not
>>>> necessary to protect the pasid entry pointer itself. The pasid table 
>>>> should
>>>> exist during device probe and release. is it?
>>>
>>> The pattern of the code that modifies a pasid table entry is,
>>>
>>>      spin_lock(&iommu->lock);
>>>      pte = intel_pasid_get_entry(dev, pasid);
>>>      ... modify the pasid table entry ...
>>>      spin_unlock(&iommu->lock);
>>>
>>> Returning the pte pointer to the caller introduces a potential race
>>> condition. If the caller subsequently modifies the pte without re-
>>> acquiring the spin lock, there's a risk of data corruption or
>>> inconsistencies.
>>
>> it appears that we are on the same page about if pte pointer needs to be
>> protected or not. And I agree the modifications to the pte should be
>> protected by iommu->lock. If so, will documenting that the caller must hold
>> iommu->lock if is tries to modify the content of pte work? Also, it might
>> be helpful to add lockdep to make sure all the modifications of pte entry
>> are under protection.
> 
> People will soon forget about this lock and may modify the returned pte
> pointer without locking, introducing a race condition silently.
> 
>> Or any suggestion from you given a path that needs to get pte first, check
>> if it exists and then call intel_pasid_tear_down_entry(). For example the
>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
>> iommu->lock and do more modifications on the pasid entry. It would invoke
>> the intel_pasid_get_entry() twice if no change to
>> intel_pasid_tear_down_entry().
> 
> There is no need to check the present of a pte entry before calling into
> intel_pasid_tear_down_entry(). The helper will return directly if the
> pte is not present:
> 
>          spin_lock(&iommu->lock);
>          pte = intel_pasid_get_entry(dev, pasid);
>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>                  spin_unlock(&iommu->lock);
>                  return;
>          }
> 
> Does it work for you?

This is not I'm talking about. My intention is to avoid duplicated
intel_pasid_get_entry() call when calling intel_pasid_tear_down_entry() in
intel_pasid_setup_first_level(). Both the two functions call the
intel_pasid_get_entry() to get pte pointer. So I think it might be good to
save one of them.

-- 
Regards,
Yi Liu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu]

On 2024/10/22 17:38, Yi Liu wrote:
> On 2024/10/22 17:23, Baolu Lu wrote:
>> On 2024/10/21 15:24, Yi Liu wrote:
>>> On 2024/10/21 14:59, Baolu Lu wrote:
>>>> On 2024/10/21 14:35, Yi Liu wrote:
>>>>> On 2024/10/21 14:13, Baolu Lu wrote:
>>>>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it 
>>>>>>> down.
>>>>>>> There are paths that need to get the pasid entry, tear it down and
>>>>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the 
>>>>>>> pasid
>>>>>>> entry can avoid duplicate codes to get the pasid entry. No 
>>>>>>> functional
>>>>>>> change is intended.
>>>>>>>
>>>>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>>>>> ---
>>>>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/ 
>>>>>>> pasid.c
>>>>>>> index 2898e7af2cf4..336f9425214c 100644
>>>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>>>>>> intel_iommu *iommu,
>>>>>>>   /*
>>>>>>>    * Caller can request to drain PRQ in this helper if it hasn't 
>>>>>>> done so,
>>>>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>>>>> + * Return the pasid entry pointer if the entry is found or NULL 
>>>>>>> if no
>>>>>>> + * entry found.
>>>>>>>    */
>>>>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, 
>>>>>>> struct device *dev,
>>>>>>> -                 u32 pasid, u32 flags)
>>>>>>> +struct pasid_entry *
>>>>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>>>> device *dev,
>>>>>>> +                u32 pasid, u32 flags)
>>>>>>>   {
>>>>>>>       struct pasid_entry *pte;
>>>>>>>       u16 did, pgtt;
>>>>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>>>>>> intel_iommu *iommu, struct device *dev,
>>>>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>>>>           spin_unlock(&iommu->lock);
>>>>>>> -        return;
>>>>>>> +        goto out;
>>>>>>
>>>>>> The pasid table entry is protected by iommu->lock. It's  not 
>>>>>> reasonable
>>>>>> to return the pte pointer which is beyond the lock protected range.
>>>>>
>>>>> Per my understanding, the iommu->lock protects the content of the 
>>>>> entry,
>>>>> so the modifications to the entry need to hold it. While, it looks not
>>>>> necessary to protect the pasid entry pointer itself. The pasid 
>>>>> table should
>>>>> exist during device probe and release. is it?
>>>>
>>>> The pattern of the code that modifies a pasid table entry is,
>>>>
>>>>      spin_lock(&iommu->lock);
>>>>      pte = intel_pasid_get_entry(dev, pasid);
>>>>      ... modify the pasid table entry ...
>>>>      spin_unlock(&iommu->lock);
>>>>
>>>> Returning the pte pointer to the caller introduces a potential race
>>>> condition. If the caller subsequently modifies the pte without re-
>>>> acquiring the spin lock, there's a risk of data corruption or
>>>> inconsistencies.
>>>
>>> it appears that we are on the same page about if pte pointer needs to be
>>> protected or not. And I agree the modifications to the pte should be
>>> protected by iommu->lock. If so, will documenting that the caller 
>>> must hold
>>> iommu->lock if is tries to modify the content of pte work? Also, it 
>>> might
>>> be helpful to add lockdep to make sure all the modifications of pte 
>>> entry
>>> are under protection.
>>
>> People will soon forget about this lock and may modify the returned pte
>> pointer without locking, introducing a race condition silently.
>>
>>> Or any suggestion from you given a path that needs to get pte first, 
>>> check
>>> if it exists and then call intel_pasid_tear_down_entry(). For example 
>>> the
>>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
>>> iommu->lock and do more modifications on the pasid entry. It would 
>>> invoke
>>> the intel_pasid_get_entry() twice if no change to
>>> intel_pasid_tear_down_entry().
>>
>> There is no need to check the present of a pte entry before calling into
>> intel_pasid_tear_down_entry(). The helper will return directly if the
>> pte is not present:
>>
>>          spin_lock(&iommu->lock);
>>          pte = intel_pasid_get_entry(dev, pasid);
>>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>                  spin_unlock(&iommu->lock);
>>                  return;
>>          }
>>
>> Does it work for you?
> 
> This is not I'm talking about. My intention is to avoid duplicated
> intel_pasid_get_entry() call when calling intel_pasid_tear_down_entry() in
> intel_pasid_setup_first_level(). Both the two functions call the
> intel_pasid_get_entry() to get pte pointer. So I think it might be good to
> save one of them.

Then, perhaps you can add a pasid_entry_tear_down() helper which asserts
iommu->lock and call it in both intel_pasid_tear_down_entry() and
intel_pasid_setup_first_level()?

Thanks,
baolu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Yi Liu]

On 2024/10/22 19:23, Baolu Lu wrote:
> On 2024/10/22 17:38, Yi Liu wrote:
>> On 2024/10/22 17:23, Baolu Lu wrote:
>>> On 2024/10/21 15:24, Yi Liu wrote:
>>>> On 2024/10/21 14:59, Baolu Lu wrote:
>>>>> On 2024/10/21 14:35, Yi Liu wrote:
>>>>>> On 2024/10/21 14:13, Baolu Lu wrote:
>>>>>>> On 2024/10/18 13:53, Yi Liu wrote:
>>>>>>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>>>>>>> There are paths that need to get the pasid entry, tear it down and
>>>>>>>> re-configure it. Letting intel_pasid_tear_down_entry() return the 
>>>>>>>> pasid
>>>>>>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>>>>>>> change is intended.
>>>>>>>>
>>>>>>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>>>>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>>>>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/ 
>>>>>>>> pasid.c
>>>>>>>> index 2898e7af2cf4..336f9425214c 100644
>>>>>>>> --- a/drivers/iommu/intel/pasid.c
>>>>>>>> +++ b/drivers/iommu/intel/pasid.c
>>>>>>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>>>>>>> intel_iommu *iommu,
>>>>>>>>   /*
>>>>>>>>    * Caller can request to drain PRQ in this helper if it hasn't 
>>>>>>>> done so,
>>>>>>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>>>>>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>>>>>>> + * entry found.
>>>>>>>>    */
>>>>>>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>>>>> device *dev,
>>>>>>>> -                 u32 pasid, u32 flags)
>>>>>>>> +struct pasid_entry *
>>>>>>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>>>>>>> device *dev,
>>>>>>>> +                u32 pasid, u32 flags)
>>>>>>>>   {
>>>>>>>>       struct pasid_entry *pte;
>>>>>>>>       u16 did, pgtt;
>>>>>>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>>>>>>> intel_iommu *iommu, struct device *dev,
>>>>>>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>>>>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>>>>>           spin_unlock(&iommu->lock);
>>>>>>>> -        return;
>>>>>>>> +        goto out;
>>>>>>>
>>>>>>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>>>>>>> to return the pte pointer which is beyond the lock protected range.
>>>>>>
>>>>>> Per my understanding, the iommu->lock protects the content of the entry,
>>>>>> so the modifications to the entry need to hold it. While, it looks not
>>>>>> necessary to protect the pasid entry pointer itself. The pasid table 
>>>>>> should
>>>>>> exist during device probe and release. is it?
>>>>>
>>>>> The pattern of the code that modifies a pasid table entry is,
>>>>>
>>>>>      spin_lock(&iommu->lock);
>>>>>      pte = intel_pasid_get_entry(dev, pasid);
>>>>>      ... modify the pasid table entry ...
>>>>>      spin_unlock(&iommu->lock);
>>>>>
>>>>> Returning the pte pointer to the caller introduces a potential race
>>>>> condition. If the caller subsequently modifies the pte without re-
>>>>> acquiring the spin lock, there's a risk of data corruption or
>>>>> inconsistencies.
>>>>
>>>> it appears that we are on the same page about if pte pointer needs to be
>>>> protected or not. And I agree the modifications to the pte should be
>>>> protected by iommu->lock. If so, will documenting that the caller must 
>>>> hold
>>>> iommu->lock if is tries to modify the content of pte work? Also, it might
>>>> be helpful to add lockdep to make sure all the modifications of pte entry
>>>> are under protection.
>>>
>>> People will soon forget about this lock and may modify the returned pte
>>> pointer without locking, introducing a race condition silently.
>>>
>>>> Or any suggestion from you given a path that needs to get pte first, check
>>>> if it exists and then call intel_pasid_tear_down_entry(). For example the
>>>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>>>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then lock
>>>> iommu->lock and do more modifications on the pasid entry. It would invoke
>>>> the intel_pasid_get_entry() twice if no change to
>>>> intel_pasid_tear_down_entry().
>>>
>>> There is no need to check the present of a pte entry before calling into
>>> intel_pasid_tear_down_entry(). The helper will return directly if the
>>> pte is not present:
>>>
>>>          spin_lock(&iommu->lock);
>>>          pte = intel_pasid_get_entry(dev, pasid);
>>>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>                  spin_unlock(&iommu->lock);
>>>                  return;
>>>          }
>>>
>>> Does it work for you?
>>
>> This is not I'm talking about. My intention is to avoid duplicated
>> intel_pasid_get_entry() call when calling intel_pasid_tear_down_entry() in
>> intel_pasid_setup_first_level(). Both the two functions call the
>> intel_pasid_get_entry() to get pte pointer. So I think it might be good to
>> save one of them.
> 
> Then, perhaps you can add a pasid_entry_tear_down() helper which asserts
> iommu->lock and call it in both intel_pasid_tear_down_entry() and
> intel_pasid_setup_first_level()?

hmmm. I still have a doubt. Only part of the intel_pasid_tear_down_entry()
holds the iommu->lock. I'm afraid it's uneasy to split the
intel_pasid_tear_down_entry() without letting the cache flush code under
the iommu->lock. But it seems unnecessary to do cache flush under the
iommu->lock. What about your thought? or am I getting you correctly?
Also, I suppose this split allows the caller of the new 
pasid_entry_tear_down() helper to pass in the pte pointer. is it?

void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
				 u32 pasid, bool fault_ignore)
{
	struct pasid_entry *pte;
	u16 did, pgtt;

	spin_lock(&iommu->lock);
	pte = intel_pasid_get_entry(dev, pasid);
	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
		spin_unlock(&iommu->lock);
		return;
	}

	did = pasid_get_domain_id(pte);
	pgtt = pasid_pte_get_pgtt(pte);
	intel_pasid_clear_entry(dev, pasid, fault_ignore);
	spin_unlock(&iommu->lock);

	if (!ecap_coherent(iommu->ecap))
		clflush_cache_range(pte, sizeof(*pte));

	pasid_cache_invalidation_with_pasid(iommu, did, pasid);

	if (pgtt == PASID_ENTRY_PGTT_PT || pgtt == PASID_ENTRY_PGTT_FL_ONLY)
		qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
	else
		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);

	devtlb_invalidation_with_pasid(iommu, dev, pasid);
}

-- 
Regards,
Yi Liu

Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu]

On 2024/10/22 21:25, Yi Liu wrote:
>>>>> Or any suggestion from you given a path that needs to get pte 
>>>>> first, check
>>>>> if it exists and then call intel_pasid_tear_down_entry(). For 
>>>>> example the
>>>>> intel_pasid_setup_first_level() [1], in my series, I need to call the
>>>>> unlock iommu->lock and call intel_pasid_tear_down_entry() and then 
>>>>> lock
>>>>> iommu->lock and do more modifications on the pasid entry. It would 
>>>>> invoke
>>>>> the intel_pasid_get_entry() twice if no change to
>>>>> intel_pasid_tear_down_entry().
>>>>
>>>> There is no need to check the present of a pte entry before calling 
>>>> into
>>>> intel_pasid_tear_down_entry(). The helper will return directly if the
>>>> pte is not present:
>>>>
>>>>          spin_lock(&iommu->lock);
>>>>          pte = intel_pasid_get_entry(dev, pasid);
>>>>          if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>>                  spin_unlock(&iommu->lock);
>>>>                  return;
>>>>          }
>>>>
>>>> Does it work for you?
>>>
>>> This is not I'm talking about. My intention is to avoid duplicated
>>> intel_pasid_get_entry() call when calling 
>>> intel_pasid_tear_down_entry() in
>>> intel_pasid_setup_first_level(). Both the two functions call the
>>> intel_pasid_get_entry() to get pte pointer. So I think it might be 
>>> good to
>>> save one of them.
>>
>> Then, perhaps you can add a pasid_entry_tear_down() helper which asserts
>> iommu->lock and call it in both intel_pasid_tear_down_entry() and
>> intel_pasid_setup_first_level()?
> 
> hmmm. I still have a doubt. Only part of the intel_pasid_tear_down_entry()
> holds the iommu->lock. I'm afraid it's uneasy to split the
> intel_pasid_tear_down_entry() without letting the cache flush code under
> the iommu->lock. But it seems unnecessary to do cache flush under the
> iommu->lock. What about your thought? or am I getting you correctly?
> Also, I suppose this split allows the caller of the new
 > pasid_entry_tear_down() helper to pass in the pte pointer. is it?

Okay, so you want to implement a "replace" on a PASID. I think there are
two ways to achieve this. First, we can transition the PASID to the
blocking state and then replace it with a new translation. Second, we
can implement a native replacement by directly modifying the present
PASID entry.

For the first solution, we could do something like this:

	/* blocking the translation on the PASID */
	intel_pasid_tear_down_entry(dev, pasid);
	... ...
	/* setup the new domain on the PASID */
	ret = intel_pasid_setup_first_level(domain, dev, pasid);
	if (ret)
		intel_pasid_setup_first_level(old_domain, dev, pasid);

For the second solution, we need to implement a new helper function,
intel_pasid_replace_first_level(), and use it like this:

	ret = intel_pasid_replace_first_level(domain, dev, pasid);

The PASID entry remains unchanged if an error occurs.

I don't see a need of refactoring current PASID tear_down and setup
helpers.

Thanks,
baolu