[RFC PATCH 0/6] Share sva domain with all devices bound to a mm

[RFC PATCH 0/6] Share sva domain with all devices bound to a mm

[Tina Zhang]

During a sva domain's life time, which begins with binding a device to a
mm and ends by releasing all the bound devices from that mm, there is only
one PASID assigned for this sva domain. From platform perspective,
supporting 1:1 mapping between mm PASID and sva domain allows a sva domain
to be shared by mm's all bound devices issuing DMA transactions using the
assigned PASID.

To support mm PASID 1:1 with sva domain, each mm needs to keep both a
reference of an allocated sva domain and its corresponding PASID. However,
currently, mm struct only has one pasid field for sva usage, which is used
to keep the info of an assigned PASID. That pasid field cannot provide
sufficient info to build up the 1:1 mapping between sva domain and PASID.

This patch-set fills the gap by adding an mm_iommu field[1], whose type is
mm_iommu_data struct, to replace the old pasid field. The introduced
mm_iommu_data struct keeps info of both a sva domain reference and an
assigned PASID.


[1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/

Tina Zhang (6):
  iommu: Add two pasid helper functions
  iommu: Call helper functions to get/set assigned pasid value
  iommu: Introduce struct iommu_mm_data
  mm: Add iommu_mm field to mm_struct
  iommu: Support mm PASID 1:1 with sva domain
  mm: Deprecate pasid field

 arch/x86/kernel/traps.c                       |  2 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 12 ++--
 drivers/iommu/intel/svm.c                     |  8 +--
 drivers/iommu/iommu-sva.c                     | 68 ++++++++++++-------
 drivers/iommu/iommu.c                         |  3 +
 include/linux/iommu.h                         | 25 ++++++-
 include/linux/mm_types.h                      |  2 +
 mm/init-mm.c                                  |  2 +-
 8 files changed, 84 insertions(+), 38 deletions(-)

-- 
2.34.1

[RFC PATCH 1/6] iommu: Add two pasid helper functions

[Tina Zhang]

mm_get_pasid() is for getting mm pasid value and mm_set_pasid() is for
setting mm pasid value.

The motivation is to replace mm->pasid with an iommu private data
structure that is introduced in a later patch.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 arch/x86/kernel/traps.c |  2 +-
 include/linux/iommu.h   | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 58b1f208eff51..8587461c43664 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -678,7 +678,7 @@ static bool try_fixup_enqcmd_gp(void)
 	if (!mm_valid_pasid(current->mm))
 		return false;
 
-	pasid = current->mm->pasid;
+	pasid = mm_get_pasid(current->mm);
 
 	/*
 	 * Did this thread already have its PASID activated?
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e8c9a7da10609..d39e647219eb8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1174,6 +1174,15 @@ static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
 	return mm->pasid != IOMMU_PASID_INVALID;
 }
+static inline u32 mm_get_pasid(struct mm_struct *mm)
+{
+	return mm->pasid;
+}
+
+static inline void mm_set_pasid(struct mm_struct *mm, u32 pasid)
+{
+	mm->pasid = pasid;
+}
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm);
@@ -1196,6 +1205,11 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 }
 static inline void mm_pasid_init(struct mm_struct *mm) {}
 static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
+static inline u32 mm_get_pasid(struct mm_struct *mm)
+{
+	return IOMMU_PASID_INVALID;
+}
+static inline void mm_set_pasid(struct mm_struct *mm, u32 pasid) {}
 static inline void mm_pasid_drop(struct mm_struct *mm) {}
 #endif /* CONFIG_IOMMU_SVA */
 
-- 
2.34.1

[RFC PATCH 2/6] iommu: Call helper functions to get/set assigned pasid value

[Tina Zhang]

Use the helper function mm_get_pasid() to get the mm assigned pasid value
and mm_set_pasid() to set the mm assigned pasid value.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 ++++++------
 drivers/iommu/intel/svm.c                       |  8 ++++----
 drivers/iommu/iommu-sva.c                       | 16 ++++++++--------
 3 files changed, 18 insertions(+), 18 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 a5a63b1c947eb..0b455654d3650 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
@@ -204,7 +204,7 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn,
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
 		arm_smmu_tlb_inv_range_asid(start, size, smmu_mn->cd->asid,
 					    PAGE_SIZE, false, smmu_domain);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, start, size);
+	arm_smmu_atc_inv_domain(smmu_domain, mm_get_pasid(mm), start, size);
 }
 
 static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -222,10 +222,10 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events,
 	 * but disable translation.
 	 */
-	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, &quiet_cd);
+	arm_smmu_write_ctx_desc(smmu_domain, mm_get_pasid(mm), &quiet_cd);
 
 	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+	arm_smmu_atc_inv_domain(smmu_domain, mm_get_pasid(mm), 0, 0);
 
 	smmu_mn->cleared = true;
 	mutex_unlock(&sva_lock);
@@ -279,7 +279,7 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 		goto err_free_cd;
 	}
 
-	ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd);
+	ret = arm_smmu_write_ctx_desc(smmu_domain, mm_get_pasid(mm), cd);
 	if (ret)
 		goto err_put_notifier;
 
@@ -304,7 +304,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 		return;
 
 	list_del(&smmu_mn->list);
-	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL);
+	arm_smmu_write_ctx_desc(smmu_domain, mm_get_pasid(mm), NULL);
 
 	/*
 	 * If we went through clear(), we've already invalidated, and no
@@ -312,7 +312,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	 */
 	if (!smmu_mn->cleared) {
 		arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid);
-		arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+		arm_smmu_atc_inv_domain(smmu_domain, mm_get_pasid(mm), 0, 0);
 	}
 
 	/* Frees smmu_mn */
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index e95b339e9cdc0..e6377cff6a935 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -306,13 +306,13 @@ static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
 	unsigned long sflags;
 	int ret = 0;
 
-	svm = pasid_private_find(mm->pasid);
+	svm = pasid_private_find(mm_get_pasid(mm));
 	if (!svm) {
 		svm = kzalloc(sizeof(*svm), GFP_KERNEL);
 		if (!svm)
 			return -ENOMEM;
 
-		svm->pasid = mm->pasid;
+		svm->pasid = mm_get_pasid(mm);
 		svm->mm = mm;
 		INIT_LIST_HEAD_RCU(&svm->devs);
 
@@ -350,7 +350,7 @@ static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
 
 	/* 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, mm->pasid,
+	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm_get_pasid(mm),
 					    FLPT_DEFAULT_DID, sflags);
 	if (ret)
 		goto free_sdev;
@@ -364,7 +364,7 @@ static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
 free_svm:
 	if (list_empty(&svm->devs)) {
 		mmu_notifier_unregister(&svm->notifier, mm);
-		pasid_private_remove(mm->pasid);
+		pasid_private_remove(mm_get_pasid(mm));
 		kfree(svm);
 	}
 
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 9821bc44f5ac1..7a41b6510e385 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -28,7 +28,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
 	mutex_lock(&iommu_sva_lock);
 	/* Is a PASID already associated with this mm? */
 	if (mm_valid_pasid(mm)) {
-		if (mm->pasid < min || mm->pasid > max)
+		if (mm_get_pasid(mm) < min || mm_get_pasid(mm) > max)
 			ret = -EOVERFLOW;
 		goto out;
 	}
@@ -36,7 +36,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
 	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
 	if (ret < min)
 		goto out;
-	mm->pasid = ret;
+	mm_set_pasid(mm, ret);
 	ret = 0;
 out:
 	mutex_unlock(&iommu_sva_lock);
@@ -70,7 +70,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	if (!max_pasids)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	/* Allocate mm->pasid if necessary. */
+	/* Allocate pasid if necessary. */
 	ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
 	if (ret)
 		return ERR_PTR(ret);
@@ -81,7 +81,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 
 	mutex_lock(&iommu_sva_lock);
 	/* Search for an existing domain. */
-	domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
+	domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
 						IOMMU_DOMAIN_SVA);
 	if (IS_ERR(domain)) {
 		ret = PTR_ERR(domain);
@@ -100,7 +100,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 		goto out_unlock;
 	}
 
-	ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
+	ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
 	if (ret)
 		goto out_free_domain;
 	domain->users = 1;
@@ -132,7 +132,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
 void iommu_sva_unbind_device(struct iommu_sva *handle)
 {
 	struct iommu_domain *domain = handle->domain;
-	ioasid_t pasid = domain->mm->pasid;
+	ioasid_t pasid = mm_get_pasid(domain->mm);
 	struct device *dev = handle->dev;
 
 	mutex_lock(&iommu_sva_lock);
@@ -149,7 +149,7 @@ u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 {
 	struct iommu_domain *domain = handle->domain;
 
-	return domain->mm->pasid;
+	return mm_get_pasid(domain->mm);
 }
 EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
 
@@ -216,5 +216,5 @@ void mm_pasid_drop(struct mm_struct *mm)
 	if (likely(!mm_valid_pasid(mm)))
 		return;
 
-	ida_free(&iommu_global_pasid_ida, mm->pasid);
+	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
 }
-- 
2.34.1

[RFC PATCH 3/6] iommu: Introduce struct iommu_mm_data

[Tina Zhang]

To make sva domain 1:1 with mm pasid, mm needs to keep reference to the
sva domain as well as keeping the information of mm pasid. Introduce
struct iommu_mm_data to wrap the information up.

When a process is created, the mm pasid is initialized as IOMMU_PASID_
INVALID. The default_iommu_mms is introduced to initialize an mm pasid
with that default value.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/iommu.c | 2 ++
 include/linux/iommu.h | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f1dcfa3f1a1b4..35fa1c1b12826 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -83,6 +83,8 @@ static const char * const iommu_group_resv_type_string[] = {
 	[IOMMU_RESV_SW_MSI]			= "msi",
 };
 
+struct iommu_mm_data default_iommu_mm = { IOMMU_PASID_INVALID, NULL };
+
 #define IOMMU_CMD_LINE_DMA_API		BIT(0)
 #define IOMMU_CMD_LINE_STRICT		BIT(1)
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d39e647219eb8..20135912584ba 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,8 @@ struct iommu_sva;
 struct iommu_fault_event;
 struct iommu_dma_cookie;
 
+extern struct iommu_mm_data default_iommu_mm;
+
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
 #define IOMMU_FAULT_WRITE	0x1
@@ -664,6 +666,11 @@ struct iommu_sva {
 	struct iommu_domain		*domain;
 };
 
+struct iommu_mm_data {
+	u32                     pasid;
+	struct iommu_domain     *sva_domain;
+};
+
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops);
 void iommu_fwspec_free(struct device *dev);
-- 
2.34.1

[RFC PATCH 4/6] mm: Add iommu_mm field to mm_struct

[Tina Zhang]

The iommu_mm field has all the information needed for managing sva domain
1:1 with mm pasid. Add it to the mm_struct as the sva-related field and
use default_iommu_mm to initialize init_mm pasid.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/linux/mm_types.h | 2 ++
 mm/init-mm.c             | 1 +
 2 files changed, 3 insertions(+)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 306a3d1a0fa65..bb00caa078335 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -581,6 +581,7 @@ struct mm_cid {
 #endif
 
 struct kioctx_table;
+struct iommu_mm_data;
 struct mm_struct {
 	struct {
 		struct maple_tree mm_mt;
@@ -773,6 +774,7 @@ struct mm_struct {
 
 #ifdef CONFIG_IOMMU_SVA
 		u32 pasid;
+		struct iommu_mm_data *iommu_mm;
 #endif
 #ifdef CONFIG_KSM
 		/*
diff --git a/mm/init-mm.c b/mm/init-mm.c
index efa97b57acfd8..c43b7d0347312 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -44,6 +44,7 @@ struct mm_struct init_mm = {
 	.cpu_bitmap	= CPU_BITS_NONE,
 #ifdef CONFIG_IOMMU_SVA
 	.pasid		= IOMMU_PASID_INVALID,
+	.iommu_mm       = &default_iommu_mm,
 #endif
 	INIT_MM_CONTEXT(init_mm)
 };
-- 
2.34.1

[RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Tina Zhang]

Each mm bound to devices gets a PASID and a corresponding sva domain
allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
field of the mm. And that PASID and sva domain get released in iommu_sva_
unbind_device() when no devices are binding to that mm. As a result,
during the life cycle, sva domain has 1:1 with mm PASID.

Since the required info of PASID and sva domain are kept in struct
iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
field in mm struct.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/iommu-sva.c | 54 +++++++++++++++++++++++++++------------
 drivers/iommu/iommu.c     |  1 +
 include/linux/iommu.h     |  8 +++---
 3 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 7a41b6510e385..342d8ba9ab479 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
 /* Allocate a PASID for the mm within range (inclusive) */
 static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
 {
+	struct iommu_mm_data *iommu_mm = NULL;
 	int ret = 0;
 
 	if (min == IOMMU_PASID_INVALID ||
@@ -33,9 +34,18 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
 		goto out;
 	}
 
+	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
+	if (!iommu_mm) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	mm->iommu_mm = iommu_mm;
+
 	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
-	if (ret < min)
+	if (ret < min) {
+		kfree(iommu_mm);
 		goto out;
+	}
 	mm_set_pasid(mm, ret);
 	ret = 0;
 out:
@@ -61,7 +71,7 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
  */
 struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
-	struct iommu_domain *domain;
+	struct iommu_domain *domain, *sva_domain = mm->iommu_mm->sva_domain;
 	struct iommu_sva *handle;
 	ioasid_t max_pasids;
 	int ret;
@@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 		goto out_unlock;
 	}
 
-	if (domain) {
-		domain->users++;
-		goto out;
+	if (unlikely(domain)) {
+		/* Re-attach the device to the same domain? */
+		if (domain == sva_domain) {
+			goto out;
+		} else {
+			/* Didn't get detached from the previous domain? */
+			ret = -EBUSY;
+			goto out_unlock;
+		}
 	}
 
-	/* Allocate a new domain and set it on device pasid. */
-	domain = iommu_sva_domain_alloc(dev, mm);
-	if (!domain) {
-		ret = -ENOMEM;
-		goto out_unlock;
+	if (sva_domain) {
+		sva_domain->users++;
+	} else {
+		/* Allocate a new domain and set it on device pasid. */
+		sva_domain = iommu_sva_domain_alloc(dev, mm);
+		if (!sva_domain) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+		sva_domain->users = 1;
 	}
 
-	ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
+	ret = iommu_attach_device_pasid(sva_domain, dev, mm_get_pasid(mm));
 	if (ret)
 		goto out_free_domain;
-	domain->users = 1;
 out:
 	mutex_unlock(&iommu_sva_lock);
 	handle->dev = dev;
-	handle->domain = domain;
+	handle->domain = sva_domain;
 
 	return handle;
 
 out_free_domain:
-	iommu_domain_free(domain);
+	iommu_domain_free(sva_domain);
 out_unlock:
 	mutex_unlock(&iommu_sva_lock);
 	kfree(handle);
@@ -136,10 +156,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 	struct device *dev = handle->dev;
 
 	mutex_lock(&iommu_sva_lock);
-	if (--domain->users == 0) {
-		iommu_detach_device_pasid(domain, dev, pasid);
+	iommu_detach_device_pasid(domain, dev, pasid);
+	if (--domain->users == 0)
 		iommu_domain_free(domain);
-	}
 	mutex_unlock(&iommu_sva_lock);
 	kfree(handle);
 }
@@ -217,4 +236,5 @@ void mm_pasid_drop(struct mm_struct *mm)
 		return;
 
 	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
+	kfree(mm->iommu_mm);
 }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 35fa1c1b12826..2f55a157b1f15 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3393,5 +3393,6 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 	domain->iopf_handler = iommu_sva_handle_iopf;
 	domain->fault_data = mm;
 
+	mm->iommu_mm->sva_domain = domain;
 	return domain;
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20135912584ba..1511ded7bc910 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1175,20 +1175,20 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
 #ifdef CONFIG_IOMMU_SVA
 static inline void mm_pasid_init(struct mm_struct *mm)
 {
-	mm->pasid = IOMMU_PASID_INVALID;
+	mm->iommu_mm = &default_iommu_mm;
 }
 static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
-	return mm->pasid != IOMMU_PASID_INVALID;
+	return mm->iommu_mm->pasid != IOMMU_PASID_INVALID;
 }
 static inline u32 mm_get_pasid(struct mm_struct *mm)
 {
-	return mm->pasid;
+	return mm->iommu_mm->pasid;
 }
 
 static inline void mm_set_pasid(struct mm_struct *mm, u32 pasid)
 {
-	mm->pasid = pasid;
+	mm->iommu_mm->pasid = pasid;
 }
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
-- 
2.34.1

Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Jason Gunthorpe]

On Fri, Jul 07, 2023 at 09:34:40AM +0800, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and a corresponding sva domain
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. And that PASID and sva domain get released in iommu_sva_
> unbind_device() when no devices are binding to that mm. As a result,
> during the life cycle, sva domain has 1:1 with mm PASID.
> 
> Since the required info of PASID and sva domain are kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct.

This is not technically right, the domains need to be a list, and we
need to search the list. Almost always the list will have 0 or 1
entries in it.

> @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  		goto out_unlock;
>  	}
>  
> -	if (domain) {
> -		domain->users++;
> -		goto out;
> +	if (unlikely(domain)) {
> +		/* Re-attach the device to the same domain? */
> +		if (domain == sva_domain) {
> +			goto out;
> +		} else {
> +			/* Didn't get detached from the previous domain? */
> +			ret = -EBUSY;
> +			goto out_unlock;
> +		}
>  	}

And if we do all of this we should just get rid of the horrible
iommu_get_domain_for_dev_pasid() entirely.

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20135912584ba..1511ded7bc910 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1175,20 +1175,20 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
>  #ifdef CONFIG_IOMMU_SVA
>  static inline void mm_pasid_init(struct mm_struct *mm)
>  {
> -	mm->pasid = IOMMU_PASID_INVALID;
> +	mm->iommu_mm = &default_iommu_mm;
>  }
>  static inline bool mm_valid_pasid(struct mm_struct *mm)
>  {
> -	return mm->pasid != IOMMU_PASID_INVALID;
> +	return mm->iommu_mm->pasid != IOMMU_PASID_INVALID;
>  }

And these can now just test if the iommu_mmu->sva_domain is NULL

Jaon

RE: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Zhang, Tina]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, July 11, 2023 1:29 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; Joerg Roedel <joro@8bytes.org>; Will
> Deacon <will@kernel.org>; Lu Baolu <baolu.lu@linux.intel.com>; Michael
> Shavit <mshavit@google.com>; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain
> 
> On Fri, Jul 07, 2023 at 09:34:40AM +0800, Tina Zhang wrote:
> > Each mm bound to devices gets a PASID and a corresponding sva domain
> > allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> > field of the mm. And that PASID and sva domain get released in
> > iommu_sva_
> > unbind_device() when no devices are binding to that mm. As a result,
> > during the life cycle, sva domain has 1:1 with mm PASID.
> >
> > Since the required info of PASID and sva domain are kept in struct
> > iommu_mm_data of a mm, use mm->iommu_mm field instead of the old
> pasid
> > field in mm struct.
> 
> This is not technically right, the domains need to be a list, and we need to
> search the list. Almost always the list will have 0 or 1 entries in it.
OK. Then, the mapping between sva_domain and pasid is n:1. IIUC, the reason why we want this n:1 is because sva_domain is a type of iommu_domain and therefore may have some hardware IOMMU specific configurations that cannot be shared between different IOMMUs.

> 
> > @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
> >  		goto out_unlock;
> >  	}
> >
> > -	if (domain) {
> > -		domain->users++;
> > -		goto out;
> > +	if (unlikely(domain)) {
> > +		/* Re-attach the device to the same domain? */
> > +		if (domain == sva_domain) {
> > +			goto out;
> > +		} else {
> > +			/* Didn't get detached from the previous domain? */
> > +			ret = -EBUSY;
> > +			goto out_unlock;
> > +		}
> >  	}
> 
> And if we do all of this we should just get rid of the horrible
> iommu_get_domain_for_dev_pasid() entirely.
I think it depends on whether we could get rid of iommu_group->pasid_array, right? In the current implementation, it seems iommu_group->pasid_array is expected to identify the usages that need PASID-granular DMA address translation (i.e., not only for sva usage).

> 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > 20135912584ba..1511ded7bc910 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -1175,20 +1175,20 @@ static inline bool
> > tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream  #ifdef
> > CONFIG_IOMMU_SVA  static inline void mm_pasid_init(struct mm_struct
> > *mm)  {
> > -	mm->pasid = IOMMU_PASID_INVALID;
> > +	mm->iommu_mm = &default_iommu_mm;
> >  }
> >  static inline bool mm_valid_pasid(struct mm_struct *mm)  {
> > -	return mm->pasid != IOMMU_PASID_INVALID;
> > +	return mm->iommu_mm->pasid != IOMMU_PASID_INVALID;
> >  }
> 
> And these can now just test if the iommu_mmu->sva_domain is NULL
Right. Baolu also shared a similar suggestion. Thanks.

Regards,
-Tina

> 
> Jaon

Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Baolu Lu]

On 2023/7/11 1:28, Jason Gunthorpe wrote:
>> @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>>   		goto out_unlock;
>>   	}
>>   
>> -	if (domain) {
>> -		domain->users++;
>> -		goto out;
>> +	if (unlikely(domain)) {
>> +		/* Re-attach the device to the same domain? */
>> +		if (domain == sva_domain) {
>> +			goto out;
>> +		} else {
>> +			/* Didn't get detached from the previous domain? */
>> +			ret = -EBUSY;
>> +			goto out_unlock;
>> +		}
>>   	}
> And if we do all of this we should just get rid of the horrible
> iommu_get_domain_for_dev_pasid() entirely.

At the core level, we have no idea about whether an sva domain allocated
for one device is compatible with another device. Hence, we should loop
the sva domains in the list and if the attach interface feeds back
-EINVAL's (not compatible), we should allocate a new domain for the
attached device and put it in the list if the new attachment is
successful.

Perhaps I'm too worried?

Best regards,
baolu

Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Jason Gunthorpe]

On Tue, Jul 11, 2023 at 10:43:43AM +0800, Baolu Lu wrote:
> On 2023/7/11 1:28, Jason Gunthorpe wrote:
> > > @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> > >   		goto out_unlock;
> > >   	}
> > > -	if (domain) {
> > > -		domain->users++;
> > > -		goto out;
> > > +	if (unlikely(domain)) {
> > > +		/* Re-attach the device to the same domain? */
> > > +		if (domain == sva_domain) {
> > > +			goto out;
> > > +		} else {
> > > +			/* Didn't get detached from the previous domain? */
> > > +			ret = -EBUSY;
> > > +			goto out_unlock;
> > > +		}
> > >   	}
> > And if we do all of this we should just get rid of the horrible
> > iommu_get_domain_for_dev_pasid() entirely.
> 
> At the core level, we have no idea about whether an sva domain allocated
> for one device is compatible with another device. Hence, we should loop
> the sva domains in the list and if the attach interface feeds back
> -EINVAL's (not compatible), we should allocate a new domain for the
> attached device and put it in the list if the new attachment is
> successful.

Yes, generally.

It would be good to undertand if the drivers desire one sva domain per
instance or one sva domain per driver - but with this approach it
becomes a driver choice which to use. I would guess that one sva
domain per instance is slightly simpler in the drivers..

iommu_get_domain_for_dev_pasid() turns into a check if the group
already has a SVA domain bound from a list, ie we turn it upside down
and have it search the list under the xa_lock instead of trying to
return a domain pointer back out.

Jason

Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain

[Yanfei Xu]

Hi Tina,

On 7/7/2023 9:34 AM, Tina Zhang wrote:
> @@ -33,9 +34,18 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
>   		goto out;
>   	}
>   
> +	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> +	if (!iommu_mm) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	mm->iommu_mm = iommu_mm;
> +
>   	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> -	if (ret < min)
> +	if (ret < min) {
> +		kfree(iommu_mm);
>   		goto out;

Once pasid allocation fails at this part, we should reassign the 
mm->iommu_mm back to default value, or it will keep a unavailable 
pointer, right?

Thanks,
Yanfei
> +	}
>   	mm_set_pasid(mm, ret);
>   	ret = 0;
>   out:

[RFC PATCH 6/6] mm: Deprecate pasid field

[Tina Zhang]

Drop the pasid field, as all the information needed for sva domain
management has been moved to the newly added iommu_mm field.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 mm/init-mm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/init-mm.c b/mm/init-mm.c
index c43b7d0347312..50824dd5a949a 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -43,7 +43,6 @@ struct mm_struct init_mm = {
 	.user_ns	= &init_user_ns,
 	.cpu_bitmap	= CPU_BITS_NONE,
 #ifdef CONFIG_IOMMU_SVA
-	.pasid		= IOMMU_PASID_INVALID,
 	.iommu_mm       = &default_iommu_mm,
 #endif
 	INIT_MM_CONTEXT(init_mm)
-- 
2.34.1

Re: [RFC PATCH 6/6] mm: Deprecate pasid field

[Jason Gunthorpe]

On Fri, Jul 07, 2023 at 09:34:41AM +0800, Tina Zhang wrote:
> Drop the pasid field, as all the information needed for sva domain
> management has been moved to the newly added iommu_mm field.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  mm/init-mm.c | 1 -
>  1 file changed, 1 deletion(-)

Shouldn't this remove the field from the mmu_struct as well??

Jason

RE: [RFC PATCH 6/6] mm: Deprecate pasid field

[Zhang, Tina]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, July 11, 2023 1:29 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; Joerg Roedel <joro@8bytes.org>; Will
> Deacon <will@kernel.org>; Lu Baolu <baolu.lu@linux.intel.com>; Michael
> Shavit <mshavit@google.com>; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 6/6] mm: Deprecate pasid field
> 
> On Fri, Jul 07, 2023 at 09:34:41AM +0800, Tina Zhang wrote:
> > Drop the pasid field, as all the information needed for sva domain
> > management has been moved to the newly added iommu_mm field.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  mm/init-mm.c | 1 -
> >  1 file changed, 1 deletion(-)
> 
> Shouldn't this remove the field from the mmu_struct as well??
Oops! You're right 😊

Thanks,
-Tina
> 
> Jason