[PATCH 0/5] Share sva domains with all devices bound to a mm

[PATCH 0/5] Share sva domains with all devices bound to a mm

[Tina Zhang]

A sva domain's lifetime begins with binding a device to a mm and ends
by releasing all the bound devices from that sva domain. Technically,
there could be more than one sva domain identified by the mm PASID for
the use of bound devices issuing DMA transactions.

To support mm PASID 1:n with sva domains, each mm needs to keep both a
reference list of allocated sva domains and the 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:n mapping between PASID
and sva domains.

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 reference list of sva domains
and an assigned PASID.


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


The RFC version of this patch-set is here:
https://lore.kernel.org/linux-iommu/20230707013441.365583-1-tina.zhang@intel.com/

Tina Zhang (5):
  iommu: Add mm_get_pasid() helper function
  iommu: Call helper function to get assigned pasid value
  mm: Add structure to keep sva information
  iommu: Support mm PASID 1:n with sva domains
  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                     | 50 ++++++++++++-------
 include/linux/iommu.h                         | 19 +++++--
 include/linux/mm_types.h                      |  3 +-
 kernel/fork.c                                 |  1 -
 mm/init-mm.c                                  |  3 --
 8 files changed, 58 insertions(+), 40 deletions(-)

-- 
2.17.1

[PATCH 1/5] iommu: Add mm_get_pasid() helper function

[Tina Zhang]

mm_get_pasid() is for getting 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   | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4a817d20ce3b..6e259b11cd87 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 d31642596675..30e4d1ca39b5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1180,6 +1180,10 @@ 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;
+}
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm);
@@ -1202,6 +1206,10 @@ 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_pasid_drop(struct mm_struct *mm) {}
 #endif /* CONFIG_IOMMU_SVA */
 
-- 
2.17.1

[PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Tina Zhang]

Use the helper function mm_get_pasid() to get 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                       | 14 +++++++-------
 3 files changed, 17 insertions(+), 17 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 a5a63b1c947e..0b455654d365 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 e95b339e9cdc..e6377cff6a93 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 05c0fb2acbc4..0a4a1ed40814 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;
 	}
@@ -71,7 +71,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);
@@ -82,7 +82,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);
@@ -101,7 +101,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;
@@ -133,7 +133,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);
@@ -150,7 +150,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);
 
@@ -217,5 +217,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.17.1

[PATCH 3/5] mm: Add structure to keep sva information

[Tina Zhang]

Introduce iommu_mm_data structure to keep sva information (pasid and the
related sva domains). Add iommu_mm pointer, pointing to an instance of
iommu_mm_data structure, to mm.

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

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 30e4d1ca39b5..435d1c1afd23 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -670,6 +670,11 @@ struct iommu_sva {
 	struct iommu_domain		*domain;
 };
 
+struct iommu_mm_data {
+	u32			pasid;
+	struct list_head	sva_domains;
+};
+
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops);
 void iommu_fwspec_free(struct device *dev);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 5e74ce4a28cd..3fd65b7537f0 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -595,6 +595,7 @@ struct mm_cid {
 #endif
 
 struct kioctx_table;
+struct iommu_mm_data;
 struct mm_struct {
 	struct {
 		/*
@@ -808,6 +809,7 @@ struct mm_struct {
 
 #ifdef CONFIG_IOMMU_SVA
 		u32 pasid;
+		struct iommu_mm_data *iommu_mm;
 #endif
 #ifdef CONFIG_KSM
 		/*
-- 
2.17.1

[PATCH 4/5] iommu: Support mm PASID 1:n with sva domains

[Tina Zhang]

Each mm bound to devices gets a PASID and corresponding sva domains
allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
field of the mm. The PASID is released in __mmdrop(), while a sva domain
is released when on one is using it (the reference count is decremented
in iommu_sva_unbind_device()).

Since the required info of PASID and sva domains is kept in struct
iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
field in mm struct. The sva domain list is protected by iommu_sva_lock.

Besides, this patch removes mm_pasid_init(), as with the introduced
iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.

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

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 0a4a1ed40814..35366f51ad27 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,11 +34,22 @@ 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;
+	}
+
 	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
-	if (ret < 0)
+	if (ret < 0) {
+		kfree(iommu_mm);
 		goto out;
+	}
+
+	iommu_mm->pasid = ret;
+	mm->iommu_mm = iommu_mm;
+	INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
 
-	mm->pasid = ret;
 	ret = 0;
 out:
 	mutex_unlock(&iommu_sva_lock);
@@ -82,16 +94,12 @@ 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_get_pasid(mm),
-						IOMMU_DOMAIN_SVA);
-	if (IS_ERR(domain)) {
-		ret = PTR_ERR(domain);
-		goto out_unlock;
-	}
-
-	if (domain) {
-		domain->users++;
-		goto out;
+	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
+		ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
+		if (!ret) {
+			domain->users++;
+			goto out;
+		}
 	}
 
 	/* Allocate a new domain and set it on device pasid. */
@@ -105,6 +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	if (ret)
 		goto out_free_domain;
 	domain->users = 1;
+	list_add(&domain->next, &mm->iommu_mm->sva_domains);
+
 out:
 	mutex_unlock(&iommu_sva_lock);
 	handle->dev = dev;
@@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 	struct device *dev = handle->dev;
 
 	mutex_lock(&iommu_sva_lock);
+	iommu_detach_device_pasid(domain, dev, pasid);
 	if (--domain->users == 0) {
-		iommu_detach_device_pasid(domain, dev, pasid);
+		list_del(&domain->next);
 		iommu_domain_free(domain);
 	}
 	mutex_unlock(&iommu_sva_lock);
@@ -218,4 +229,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/include/linux/iommu.h b/include/linux/iommu.h
index 435d1c1afd23..6a87df6d8637 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -109,6 +109,7 @@ struct iommu_domain {
 		struct {	/* IOMMU_DOMAIN_SVA */
 			struct mm_struct *mm;
 			int users;
+			struct list_head next;
 		};
 	};
 };
@@ -1177,17 +1178,13 @@ 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;
-}
 static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
-	return mm->pasid != IOMMU_PASID_INVALID;
+	return mm->iommu_mm ? true : false;
 }
 static inline u32 mm_get_pasid(struct mm_struct *mm)
 {
-	return mm->pasid;
+	return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID;
 }
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
@@ -1209,7 +1206,6 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 {
 	return IOMMU_PASID_INVALID;
 }
-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)
 {
diff --git a/kernel/fork.c b/kernel/fork.c
index d2e12b6d2b18..f06392dd1ca8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1274,7 +1274,6 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
 	mm_init_cpumask(mm);
 	mm_init_aio(mm);
 	mm_init_owner(mm, p);
-	mm_pasid_init(mm);
 	RCU_INIT_POINTER(mm->exe_file, NULL);
 	mmu_notifier_subscriptions_init(mm);
 	init_tlb_flush_pending(mm);
-- 
2.17.1

[PATCH 5/5] 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>
---
 include/linux/mm_types.h | 1 -
 mm/init-mm.c             | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 3fd65b7537f0..6cb5cc53c480 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -808,7 +808,6 @@ struct mm_struct {
 		struct work_struct async_put_work;
 
 #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 efa97b57acfd..69719291463e 100644
--- a/mm/init-mm.c
+++ b/mm/init-mm.c
@@ -42,9 +42,6 @@ struct mm_struct init_mm = {
 #endif
 	.user_ns	= &init_user_ns,
 	.cpu_bitmap	= CPU_BITS_NONE,
-#ifdef CONFIG_IOMMU_SVA
-	.pasid		= IOMMU_PASID_INVALID,
-#endif
 	INIT_MM_CONTEXT(init_mm)
 };
 
-- 
2.17.1

Re: [PATCH 1/5] iommu: Add mm_get_pasid() helper function

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> mm_get_pasid() is for getting mm pasid value.
> 
> The motivation is to replace mm->pasid with an iommu private data
> structure that is introduced in a later patch.

Maybe we should start out by calling it what it actually is:

'mm_get_enqcmd_pasid()'

We can't actually have multiple SVA domains with different PASIDs
until the places wrongly calling this are removed :\

eg, I would expect this series to also come with removing
'pasid_private' from the Intel driver.

The mmu_notifier should be placed in the singular iommu_domain that is
the SVA domain for the mm. Drivers should not attempt to de-duplicate
this, the core code will do it like you are showing in this series.

Jason

Re: [PATCH 4/5] iommu: Support mm PASID 1:n with sva domains

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 03:49:43PM +0800, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when on one is using it (the reference count is decremented
> in iommu_sva_unbind_device()).
> 
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
> 
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
>  include/linux/iommu.h     | 10 +++-------
>  kernel/fork.c             |  1 -
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 0a4a1ed40814..35366f51ad27 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,11 +34,22 @@ 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;
> +	}
> +
>  	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		kfree(iommu_mm);
>  		goto out;
> +	}
> +
> +	iommu_mm->pasid = ret;
> +	mm->iommu_mm = iommu_mm;
> +	INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
>  
> -	mm->pasid = ret;
>  	ret = 0;
>  out:
>  	mutex_unlock(&iommu_sva_lock);
> @@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm

Lets please rework this function into two parts

The first should be 'iommu_sva_alloc_domain()'

It should do the list searching and user management. The usual
'iommu_domain_free()' should be modified to clean it up.

Then you have the 'alloc global/enqcmd pasid' function

Then bind is just callling alloc sva domain, alloc global pasid,
set_dev_pasid in a sequence.

It will make this much more understandable what all the parts are
supposed to be doing.

Jason

Re: [PATCH 4/5] iommu: Support mm PASID 1:n with sva domains

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 12:12:42PM -0300, Jason Gunthorpe wrote:

> Lets please rework this function into two parts
> 
> The first should be 'iommu_sva_alloc_domain()'
> 
> It should do the list searching and user management. The usual
> 'iommu_domain_free()' should be modified to clean it up.
 
I suppose we can't quite do this fully since the domain allocation
needs to use set_dev_pasid to detect compatability :( This is a
similar problem we had with iommufd too.

Still, the 'alloc and set_dev_pasid' should be in one function and it
should be undone with iommu_detach_device_pasid() and
iommu_domain_free().

The helper bind/unbind functions should wrapper that sequence.

Jason

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Baolu Lu]

On 2023/8/8 15:49, Tina Zhang wrote:
> A sva domain's lifetime begins with binding a device to a mm and ends
> by releasing all the bound devices from that sva domain. Technically,
> there could be more than one sva domain identified by the mm PASID for
> the use of bound devices issuing DMA transactions.
> 
> To support mm PASID 1:n with sva domains, each mm needs to keep both a
> reference list of allocated sva domains and the 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:n mapping between PASID
> and sva domains.

Is it more appropriate to have the same life cycle for sva domain and mm
pasid? I feel that they represent the same thing, that is, the address
space shared by mm to a device.

Best regards,
baolu

> 
> 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 reference list of sva domains
> and an assigned PASID.
> 
> 
> [1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/
> 
> 
> The RFC version of this patch-set is here:
> https://lore.kernel.org/linux-iommu/20230707013441.365583-1-tina.zhang@intel.com/
> 
> Tina Zhang (5):
>    iommu: Add mm_get_pasid() helper function
>    iommu: Call helper function to get assigned pasid value
>    mm: Add structure to keep sva information
>    iommu: Support mm PASID 1:n with sva domains
>    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                     | 50 ++++++++++++-------
>   include/linux/iommu.h                         | 19 +++++--
>   include/linux/mm_types.h                      |  3 +-
>   kernel/fork.c                                 |  1 -
>   mm/init-mm.c                                  |  3 --
>   8 files changed, 58 insertions(+), 40 deletions(-)
> 

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Baolu Lu]

On 2023/8/8 15:49, Tina Zhang wrote:
> Use the helper function mm_get_pasid() to get the mm assigned pasid
> value.

For internal iommu drivers, perhaps we should use another helper.
Something like sva_domain_get_pasid()?

Suppose that the iommu drivers should have no idea about the "mm".

Best regards,
baolu

> 
> 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                       | 14 +++++++-------
>   3 files changed, 17 insertions(+), 17 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 a5a63b1c947e..0b455654d365 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 e95b339e9cdc..e6377cff6a93 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 05c0fb2acbc4..0a4a1ed40814 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;
>   	}
> @@ -71,7 +71,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);
> @@ -82,7 +82,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);
> @@ -101,7 +101,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;
> @@ -133,7 +133,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);
> @@ -150,7 +150,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);
>   
> @@ -217,5 +217,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));
>   }

Re: [PATCH 1/5] iommu: Add mm_get_pasid() helper function

[Baolu Lu]

On 2023/8/8 23:02, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
>> mm_get_pasid() is for getting mm pasid value.
>>
>> The motivation is to replace mm->pasid with an iommu private data
>> structure that is introduced in a later patch.
> Maybe we should start out by calling it what it actually is:
> 
> 'mm_get_enqcmd_pasid()'
> 
> We can't actually have multiple SVA domains with different PASIDs
> until the places wrongly calling this are removed :\
> 
> eg, I would expect this series to also come with removing
> 'pasid_private' from the Intel driver.
> 
> The mmu_notifier should be placed in the singular iommu_domain that is
> the SVA domain for the mm. Drivers should not attempt to de-duplicate
> this, the core code will do it like you are showing in this series.

The two tasks mentioned above are part of our plan. They will be
conducted in stages, which is more conducive to review and testing.
This series is just the beginning.

Best regards,
baolu

RE: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tian, Kevin]

> From: Zhang, Tina <tina.zhang@intel.com>
> Sent: Tuesday, August 8, 2023 3:50 PM
> 
> A sva domain's lifetime begins with binding a device to a mm and ends
> by releasing all the bound devices from that sva domain. Technically,
> there could be more than one sva domain identified by the mm PASID for
> the use of bound devices issuing DMA transactions.

Could you elaborate it with some concrete examples which motivate
this change?

> 
> To support mm PASID 1:n with sva domains, each mm needs to keep both a
> reference list of allocated sva domains and the 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:n mapping between PASID
> and sva domains.
> 
> 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 reference list of sva domains
> and an assigned PASID.
> 
> 
> [1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/
> 
> 
> The RFC version of this patch-set is here:
> https://lore.kernel.org/linux-iommu/20230707013441.365583-1-
> tina.zhang@intel.com/
> 
> Tina Zhang (5):
>   iommu: Add mm_get_pasid() helper function
>   iommu: Call helper function to get assigned pasid value
>   mm: Add structure to keep sva information
>   iommu: Support mm PASID 1:n with sva domains
>   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                     | 50 ++++++++++++-------
>  include/linux/iommu.h                         | 19 +++++--
>  include/linux/mm_types.h                      |  3 +-
>  kernel/fork.c                                 |  1 -
>  mm/init-mm.c                                  |  3 --
>  8 files changed, 58 insertions(+), 40 deletions(-)
> 
> --
> 2.17.1

RE: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, August 9, 2023 8:18 AM
> 
> On 2023/8/8 15:49, Tina Zhang wrote:
> > A sva domain's lifetime begins with binding a device to a mm and ends
> > by releasing all the bound devices from that sva domain. Technically,
> > there could be more than one sva domain identified by the mm PASID for
> > the use of bound devices issuing DMA transactions.
> >
> > To support mm PASID 1:n with sva domains, each mm needs to keep both
> a
> > reference list of allocated sva domains and the 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:n mapping between PASID
> > and sva domains.
> 
> Is it more appropriate to have the same life cycle for sva domain and mm
> pasid? I feel that they represent the same thing, that is, the address
> space shared by mm to a device.
> 

iirc it's a simplification to free mm pasid at __mmdrop() otherwise the
implementation is tricky, but I don't remember all the detail...

RE: [PATCH 1/5] iommu: Add mm_get_pasid() helper function

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, August 8, 2023 11:02 PM
> 
> On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> > mm_get_pasid() is for getting mm pasid value.
> >
> > The motivation is to replace mm->pasid with an iommu private data
> > structure that is introduced in a later patch.
> 
> Maybe we should start out by calling it what it actually is:
> 
> 'mm_get_enqcmd_pasid()'
> 
> We can't actually have multiple SVA domains with different PASIDs
> until the places wrongly calling this are removed :\
> 

it's kind of egg-chicken problem. mm_get_pasid() is used by all SVA
scenarios beyond enqcmd then calling it mm_get_enqcmd_pasid()
also sounds weird for non-enqcmd case.

unless you were suggesting to just create a new wrapper for this
specific enqcmd path (try_fixup_enqcmd_gp()) then I'm fine. 😊

RE: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, August 9, 2023 8:22 AM
> 
> On 2023/8/8 15:49, Tina Zhang wrote:
> > Use the helper function mm_get_pasid() to get the mm assigned pasid
> > value.
> 
> For internal iommu drivers, perhaps we should use another helper.
> Something like sva_domain_get_pasid()?
> 
> Suppose that the iommu drivers should have no idea about the "mm".
> 

Aren't all touched functions accept a struct mm_struct pointer?

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Baolu Lu]

On 2023/8/9 17:44, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, August 9, 2023 8:18 AM
>>
>> On 2023/8/8 15:49, Tina Zhang wrote:
>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>> by releasing all the bound devices from that sva domain. Technically,
>>> there could be more than one sva domain identified by the mm PASID for
>>> the use of bound devices issuing DMA transactions.
>>>
>>> To support mm PASID 1:n with sva domains, each mm needs to keep both
>> a
>>> reference list of allocated sva domains and the 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:n mapping between PASID
>>> and sva domains.
>> Is it more appropriate to have the same life cycle for sva domain and mm
>> pasid? I feel that they represent the same thing, that is, the address
>> space shared by mm to a device.
>>
> iirc it's a simplification to free mm pasid at __mmdrop() otherwise the
> implementation is tricky, but I don't remember all the detail...

Yeah, probably we could also free the sva domains in __mmdrop()? Remove
the refcount for sva domain just like what we did for pasid (at the
beginning we had refcount for each pasid...).

Best regards,
baolu

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Baolu Lu]

On 2023/8/9 17:49, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, August 9, 2023 8:22 AM
>>
>> On 2023/8/8 15:49, Tina Zhang wrote:
>>> Use the helper function mm_get_pasid() to get the mm assigned pasid
>>> value.
>>
>> For internal iommu drivers, perhaps we should use another helper.
>> Something like sva_domain_get_pasid()?
>>
>> Suppose that the iommu drivers should have no idea about the "mm".
>>
> 
> Aren't all touched functions accept a struct mm_struct pointer?

In the end we should remove all mm reference in the individual drivers.
The drivers only need to know what they need to know. All mm-aware code
should eventually be moved to the core.

For now, at least we should avoid using mm in the set/remove_dev_pasid
code path. Later, once we complete consolidating mm notification in the
core, drivers will have no need to use "mm" anymore.

Best regards,
baolu

Re: [PATCH 1/5] iommu: Add mm_get_pasid() helper function

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 09:47:15AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Tuesday, August 8, 2023 11:02 PM
> > 
> > On Tue, Aug 08, 2023 at 03:49:40PM +0800, Tina Zhang wrote:
> > > mm_get_pasid() is for getting mm pasid value.
> > >
> > > The motivation is to replace mm->pasid with an iommu private data
> > > structure that is introduced in a later patch.
> > 
> > Maybe we should start out by calling it what it actually is:
> > 
> > 'mm_get_enqcmd_pasid()'
> > 
> > We can't actually have multiple SVA domains with different PASIDs
> > until the places wrongly calling this are removed :\
> > 
> 
> it's kind of egg-chicken problem. mm_get_pasid() is used by all SVA
> scenarios beyond enqcmd then calling it mm_get_enqcmd_pasid()
> also sounds weird for non-enqcmd case.

Well, those are wrong. We need to be fixing them not hide our eyes to
the wrongness.

Michael is cooking a fix for ARM, Tina should come with a fix for
Intel in this series.

Jason

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 09:49:16AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Wednesday, August 9, 2023 8:22 AM
> > 
> > On 2023/8/8 15:49, Tina Zhang wrote:
> > > Use the helper function mm_get_pasid() to get the mm assigned pasid
> > > value.
> > 
> > For internal iommu drivers, perhaps we should use another helper.
> > Something like sva_domain_get_pasid()?
> > 
> > Suppose that the iommu drivers should have no idea about the "mm".
> > 
> 
> Aren't all touched functions accept a struct mm_struct pointer?

It is wrong for the driver to even ask this question.

Domains, regardless of what they are, get attached to PASIDs. Maybe
many PASIDs, driver doesn't get to care. SVA isn't special. Stop
making it special.

The driver should rely on there being exactly one iommu_domain for SVA
per mm so it can hang the mm_notifier off the iommu_domain

But otherwise invalidation for a SVA domain should be *exactly the
same flow* as invalidation for a paging domain. It iterates over the
attachments and generates the correct list of PASIDs and ATCs.

Jason

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 08:18:18AM +0800, Baolu Lu wrote:
> On 2023/8/8 15:49, Tina Zhang wrote:
> > A sva domain's lifetime begins with binding a device to a mm and ends
> > by releasing all the bound devices from that sva domain. Technically,
> > there could be more than one sva domain identified by the mm PASID for
> > the use of bound devices issuing DMA transactions.
> > 
> > To support mm PASID 1:n with sva domains, each mm needs to keep both a
> > reference list of allocated sva domains and the 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:n mapping between PASID
> > and sva domains.
> 
> Is it more appropriate to have the same life cycle for sva domain and mm
> pasid? I feel that they represent the same thing, that is, the address
> space shared by mm to a device.

No! The iommu_domain and the PASID are totally seperate objects with
their own lifecycles.

The SVA domain should NEVER be tied to the mm enqcmd PASID.

We might decide to free all the domains and keep the PASID around (can
we even revoke the enqcmd pasid while the MM is alive?)

Jason

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 06:58:15PM +0800, Baolu Lu wrote:
> On 2023/8/9 17:49, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Wednesday, August 9, 2023 8:22 AM
> > > 
> > > On 2023/8/8 15:49, Tina Zhang wrote:
> > > > Use the helper function mm_get_pasid() to get the mm assigned pasid
> > > > value.
> > > 
> > > For internal iommu drivers, perhaps we should use another helper.
> > > Something like sva_domain_get_pasid()?
> > > 
> > > Suppose that the iommu drivers should have no idea about the "mm".
> > > 
> > 
> > Aren't all touched functions accept a struct mm_struct pointer?
> 
> In the end we should remove all mm reference in the individual drivers.
> The drivers only need to know what they need to know. All mm-aware code
> should eventually be moved to the core.
> 
> For now, at least we should avoid using mm in the set/remove_dev_pasid
> code path. Later, once we complete consolidating mm notification in the
> core, drivers will have no need to use "mm" anymore.

I'm not sure how this will play out...

We don't want extra function indirections here so ultimately the
driver needs to hook the arch_invalidate_range() in the mm_notifier
with its own function.

The core could put the mm_notifier in a common iommu_domain_sva struct
and it could stick in the driver's invalidate ops, that would be a
nice simplification (and discourage drivers from doing the crazy
things they are currently doing)

Jason

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Baolu Lu]

On 2023/8/9 22:46, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2023 at 08:18:18AM +0800, Baolu Lu wrote:
>> On 2023/8/8 15:49, Tina Zhang wrote:
>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>> by releasing all the bound devices from that sva domain. Technically,
>>> there could be more than one sva domain identified by the mm PASID for
>>> the use of bound devices issuing DMA transactions.
>>>
>>> To support mm PASID 1:n with sva domains, each mm needs to keep both a
>>> reference list of allocated sva domains and the 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:n mapping between PASID
>>> and sva domains.
>> Is it more appropriate to have the same life cycle for sva domain and mm
>> pasid? I feel that they represent the same thing, that is, the address
>> space shared by mm to a device.
> No! The iommu_domain and the PASID are totally seperate objects with
> their own lifecycles.
> 
> The SVA domain should NEVER be tied to the mm enqcmd PASID.

Okay. Fair enough.

> 
> We might decide to free all the domains and keep the PASID around (can
> we even revoke the enqcmd pasid while the MM is alive?)

We ever did this and was removed to make code simple.

Best regards,
baolu

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tina Zhang]

Hi,

On 8/9/23 17:41, Tian, Kevin wrote:
>> From: Zhang, Tina <tina.zhang@intel.com>
>> Sent: Tuesday, August 8, 2023 3:50 PM
>>
>> A sva domain's lifetime begins with binding a device to a mm and ends
>> by releasing all the bound devices from that sva domain. Technically,
>> there could be more than one sva domain identified by the mm PASID for
>> the use of bound devices issuing DMA transactions.
> 
> Could you elaborate it with some concrete examples which motivate
> this change?
The motivation is to remove the superfluous IOTLB invalidation in 
current VT-d driver.

Currently, in VT-d driver, due to lacking shared sva domain info, in 
intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations 
are performed per-device. However, difference devices could be behind 
one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb 
per-device gives us more iotlb invalidation than necessary (4 iotlb 
invalidation instead of 1). This issue may give more performance impact 
when in a virtual machine guest, as currently we have one virtual VT-d 
for in front of those virtual devices.


This patch fixes this issue by attaching shared sva domain information 
to mm, so that it can be utilized in the mm_notifier_ops callbacks.

Regards,
-Tina

> 
>>
>> To support mm PASID 1:n with sva domains, each mm needs to keep both a
>> reference list of allocated sva domains and the 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:n mapping between PASID
>> and sva domains.
>>
>> 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 reference list of sva domains
>> and an assigned PASID.
>>
>>
>> [1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/
>>
>>
>> The RFC version of this patch-set is here:
>> https://lore.kernel.org/linux-iommu/20230707013441.365583-1-
>> tina.zhang@intel.com/
>>
>> Tina Zhang (5):
>>    iommu: Add mm_get_pasid() helper function
>>    iommu: Call helper function to get assigned pasid value
>>    mm: Add structure to keep sva information
>>    iommu: Support mm PASID 1:n with sva domains
>>    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                     | 50 ++++++++++++-------
>>   include/linux/iommu.h                         | 19 +++++--
>>   include/linux/mm_types.h                      |  3 +-
>>   kernel/fork.c                                 |  1 -
>>   mm/init-mm.c                                  |  3 --
>>   8 files changed, 58 insertions(+), 40 deletions(-)
>>
>> --
>> 2.17.1
> 

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Baolu Lu]

On 2023/8/9 22:48, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2023 at 06:58:15PM +0800, Baolu Lu wrote:
>> On 2023/8/9 17:49, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Wednesday, August 9, 2023 8:22 AM
>>>>
>>>> On 2023/8/8 15:49, Tina Zhang wrote:
>>>>> Use the helper function mm_get_pasid() to get the mm assigned pasid
>>>>> value.
>>>>
>>>> For internal iommu drivers, perhaps we should use another helper.
>>>> Something like sva_domain_get_pasid()?
>>>>
>>>> Suppose that the iommu drivers should have no idea about the "mm".
>>>>
>>>
>>> Aren't all touched functions accept a struct mm_struct pointer?
>>
>> In the end we should remove all mm reference in the individual drivers.
>> The drivers only need to know what they need to know. All mm-aware code
>> should eventually be moved to the core.
>>
>> For now, at least we should avoid using mm in the set/remove_dev_pasid
>> code path. Later, once we complete consolidating mm notification in the
>> core, drivers will have no need to use "mm" anymore.
> 
> I'm not sure how this will play out...
> 
> We don't want extra function indirections here so ultimately the
> driver needs to hook the arch_invalidate_range() in the mm_notifier
> with its own function.

Agreed. Given the mm notification callback is a performance path, an
extra indirection call here is not good.

> 
> The core could put the mm_notifier in a common iommu_domain_sva struct
> and it could stick in the driver's invalidate ops, that would be a
> nice simplification (and discourage drivers from doing the crazy
> things they are currently doing)

Yes. So the iommu driver can retrieve the sva domain from struct
mmu_notifier. The callback implementation will still be domain centric.
Hence, there will be no need to use mm.

Best regards,
baolu

Re: [PATCH 4/5] iommu: Support mm PASID 1:n with sva domains

[Tina Zhang]

On 8/8/23 23:19, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 12:12:42PM -0300, Jason Gunthorpe wrote:
> 
>> Lets please rework this function into two parts
>>
>> The first should be 'iommu_sva_alloc_domain()'
>>
>> It should do the list searching and user management. The usual
>> 'iommu_domain_free()' should be modified to clean it up.
>   
> I suppose we can't quite do this fully since the domain allocation
> needs to use set_dev_pasid to detect compatability :( This is a
> similar problem we had with iommufd too.
> 
> Still, the 'alloc and set_dev_pasid' should be in one function and it
> should be undone with iommu_detach_device_pasid() and
> iommu_domain_free().
> 
> The helper bind/unbind functions should wrapper that sequence.
Make sense. I'll refine the error handling paths in bind/unbind 
functions in the next version.

Thanks,
-Tina
> 
> Jason

RE: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, August 10, 2023 9:23 AM
> On 2023/8/9 22:46, Jason Gunthorpe wrote:
> >
> > We might decide to free all the domains and keep the PASID around (can
> > we even revoke the enqcmd pasid while the MM is alive?)
> 
> We ever did this and was removed to make code simple.
> 

https://lore.kernel.org/lkml/87mto0ckpd.ffs@tglx/

RE: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tian, Kevin]

> From: Zhang, Tina <tina.zhang@intel.com>
> Sent: Thursday, August 10, 2023 9:32 AM
> 
> Hi,
> 
> On 8/9/23 17:41, Tian, Kevin wrote:
> >> From: Zhang, Tina <tina.zhang@intel.com>
> >> Sent: Tuesday, August 8, 2023 3:50 PM
> >>
> >> A sva domain's lifetime begins with binding a device to a mm and ends
> >> by releasing all the bound devices from that sva domain. Technically,
> >> there could be more than one sva domain identified by the mm PASID for
> >> the use of bound devices issuing DMA transactions.
> >
> > Could you elaborate it with some concrete examples which motivate
> > this change?
> The motivation is to remove the superfluous IOTLB invalidation in
> current VT-d driver.
> 
> Currently, in VT-d driver, due to lacking shared sva domain info, in
> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
> are performed per-device. However, difference devices could be behind
> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
> per-device gives us more iotlb invalidation than necessary (4 iotlb
> invalidation instead of 1). This issue may give more performance impact
> when in a virtual machine guest, as currently we have one virtual VT-d
> for in front of those virtual devices.
> 
> 
> This patch fixes this issue by attaching shared sva domain information
> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
> 

that is one of the motivations. e.g. another one as Jason suggested
is to cleanup to decouple the common sva logic from enqcmd. Both
should be mentioned in next version cover letter.

RE: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 9, 2023 8:35 PM
> 
> On Wed, Aug 09, 2023 at 09:49:16AM +0000, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Wednesday, August 9, 2023 8:22 AM
> > >
> > > On 2023/8/8 15:49, Tina Zhang wrote:
> > > > Use the helper function mm_get_pasid() to get the mm assigned pasid
> > > > value.
> > >
> > > For internal iommu drivers, perhaps we should use another helper.
> > > Something like sva_domain_get_pasid()?
> > >
> > > Suppose that the iommu drivers should have no idea about the "mm".
> > >
> >
> > Aren't all touched functions accept a struct mm_struct pointer?
> 
> It is wrong for the driver to even ask this question.
> 
> Domains, regardless of what they are, get attached to PASIDs. Maybe
> many PASIDs, driver doesn't get to care. SVA isn't special. Stop
> making it special.

Agree. I didn't think that far for what this series intends to achieve.

> 
> The driver should rely on there being exactly one iommu_domain for SVA
> per mm so it can hang the mm_notifier off the iommu_domain

I'm confused. Isn't this series trying to allow multiple domains per mm?

> 
> But otherwise invalidation for a SVA domain should be *exactly the
> same flow* as invalidation for a paging domain. It iterates over the
> attachments and generates the correct list of PASIDs and ATCs.
> 
> Jason

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Jason Gunthorpe]

On Thu, Aug 10, 2023 at 07:49:11AM +0000, Tian, Kevin wrote:
> > From: Zhang, Tina <tina.zhang@intel.com>
> > Sent: Thursday, August 10, 2023 9:32 AM
> > 
> > Hi,
> > 
> > On 8/9/23 17:41, Tian, Kevin wrote:
> > >> From: Zhang, Tina <tina.zhang@intel.com>
> > >> Sent: Tuesday, August 8, 2023 3:50 PM
> > >>
> > >> A sva domain's lifetime begins with binding a device to a mm and ends
> > >> by releasing all the bound devices from that sva domain. Technically,
> > >> there could be more than one sva domain identified by the mm PASID for
> > >> the use of bound devices issuing DMA transactions.
> > >
> > > Could you elaborate it with some concrete examples which motivate
> > > this change?
> > The motivation is to remove the superfluous IOTLB invalidation in
> > current VT-d driver.
> > 
> > Currently, in VT-d driver, due to lacking shared sva domain info, in
> > intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
> > are performed per-device. However, difference devices could be behind
> > one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
> > per-device gives us more iotlb invalidation than necessary (4 iotlb
> > invalidation instead of 1). This issue may give more performance impact
> > when in a virtual machine guest, as currently we have one virtual VT-d
> > for in front of those virtual devices.
> > 
> > 
> > This patch fixes this issue by attaching shared sva domain information
> > to mm, so that it can be utilized in the mm_notifier_ops callbacks.
> > 
> 
> that is one of the motivations. e.g. another one as Jason suggested
> is to cleanup to decouple the common sva logic from enqcmd. Both
> should be mentioned in next version cover letter.

I also want to purge all the de-duplication and refcounting code
around mm's and sva_binds from the drivers. Eg see the mess this makes
of SMMUv3.

Core code provides a single iommu_domain per-mm for SVA. Driver can
rely on this optimization and does not need to de-duplicate.

Single domain tracks all attachments. Driver can optimize using that
information by de-duplicating (eg ASID invalidation vs ATC
invalidation)

After this we need to fix the domain allocation op to add a
'alloc_domain_sva(dev, mm_struct)' op so that the drivers can setup
their SVA domains fully in a nice lock-safe environment.

Jason

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Jason Gunthorpe]

On Thu, Aug 10, 2023 at 09:37:09AM +0800, Baolu Lu wrote:

> > The core could put the mm_notifier in a common iommu_domain_sva struct
> > and it could stick in the driver's invalidate ops, that would be a
> > nice simplification (and discourage drivers from doing the crazy
> > things they are currently doing)
> 
> Yes. So the iommu driver can retrieve the sva domain from struct
> mmu_notifier. The callback implementation will still be domain centric.
> Hence, there will be no need to use mm.

Remember the driver always needs the mm as it has to extract the page
table address from it.

At the end of the day installing the notifier is a single call in the
SVA alloc op, so it may not be worth optimizing beyond allowing
drivers to do it in their SVA alloc op.

Jason

Re: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Jason Gunthorpe]

On Thu, Aug 10, 2023 at 07:52:52AM +0000, Tian, Kevin wrote:

> > The driver should rely on there being exactly one iommu_domain for SVA
> > per mm so it can hang the mm_notifier off the iommu_domain
> 
> I'm confused. Isn't this series trying to allow multiple domains per mm?

It is doing both.

The main objective is to allow de-duplicating the SVA domains in the
core code. The driver should be able to assume one SVA domain per
instance, or even one SVA domain per compatible instance. The driver
should not do any de-duplication.

But we can't just store a single iommu_domain in the mm_struct - we
have the same problem as iommufd and we need to create more domains if
the domains we already have are incompatible with the device.

Arguably this should not happen, and in any sane configuration we
should have only 1 type of IOMMU driver that needs only 1 SVA domain.

But right now things like SMMUv3 have problems crossing domains across
instances, so we could have one SVA domain per IOMMU instance until
that is fixed.

Jason

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tina Zhang]

Hi,

On 8/9/23 18:51, Baolu Lu wrote:
> On 2023/8/9 17:44, Tian, Kevin wrote:
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Wednesday, August 9, 2023 8:18 AM
>>>
>>> On 2023/8/8 15:49, Tina Zhang wrote:
>>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>>> by releasing all the bound devices from that sva domain. Technically,
>>>> there could be more than one sva domain identified by the mm PASID for
>>>> the use of bound devices issuing DMA transactions.
>>>>
>>>> To support mm PASID 1:n with sva domains, each mm needs to keep both
>>> a
>>>> reference list of allocated sva domains and the 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:n mapping between 
>>>> PASID
>>>> and sva domains.
>>> Is it more appropriate to have the same life cycle for sva domain and mm
>>> pasid? I feel that they represent the same thing, that is, the address
>>> space shared by mm to a device.
>>>
>> iirc it's a simplification to free mm pasid at __mmdrop() otherwise the
>> implementation is tricky, but I don't remember all the detail...
> 
> Yeah, probably we could also free the sva domains in __mmdrop()? Remove
> the refcount for sva domain just like what we did for pasid (at the
> beginning we had refcount for each pasid...).

For sva usage, mm->mm_count is increased in iommu_sva_domain_alloc(), 
and gets decreased when the domain has no users (which is checked in 
iommu_sva_unbind_device()).

So, in a mm's life time, there could be multiple sva domains, though 
they are using the same PASID. I think it makes sense to mm. Because it 
makes no sense to keep a sva domain alive when no users are using it, 
even though the mm is alive.

Regards,
-Tina

> 
> Best regards,
> baolu

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tina Zhang]

Hi,

On 8/10/23 15:49, Tian, Kevin wrote:
>> From: Zhang, Tina <tina.zhang@intel.com>
>> Sent: Thursday, August 10, 2023 9:32 AM
>>
>> Hi,
>>
>> On 8/9/23 17:41, Tian, Kevin wrote:
>>>> From: Zhang, Tina <tina.zhang@intel.com>
>>>> Sent: Tuesday, August 8, 2023 3:50 PM
>>>>
>>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>>> by releasing all the bound devices from that sva domain. Technically,
>>>> there could be more than one sva domain identified by the mm PASID for
>>>> the use of bound devices issuing DMA transactions.
>>>
>>> Could you elaborate it with some concrete examples which motivate
>>> this change?
>> The motivation is to remove the superfluous IOTLB invalidation in
>> current VT-d driver.
>>
>> Currently, in VT-d driver, due to lacking shared sva domain info, in
>> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
>> are performed per-device. However, difference devices could be behind
>> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
>> per-device gives us more iotlb invalidation than necessary (4 iotlb
>> invalidation instead of 1). This issue may give more performance impact
>> when in a virtual machine guest, as currently we have one virtual VT-d
>> for in front of those virtual devices.
>>
>>
>> This patch fixes this issue by attaching shared sva domain information
>> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
>>
> 
> that is one of the motivations. e.g. another one as Jason suggested
> is to cleanup to decouple the common sva logic from enqcmd. Both
> should be mentioned in next version cover letter.
Right.

Regards,
-Tina

Re: [PATCH 0/5] Share sva domains with all devices bound to a mm

[Tina Zhang]

Hi,

On 8/11/23 00:27, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 07:49:11AM +0000, Tian, Kevin wrote:
>>> From: Zhang, Tina <tina.zhang@intel.com>
>>> Sent: Thursday, August 10, 2023 9:32 AM
>>>
>>> Hi,
>>>
>>> On 8/9/23 17:41, Tian, Kevin wrote:
>>>>> From: Zhang, Tina <tina.zhang@intel.com>
>>>>> Sent: Tuesday, August 8, 2023 3:50 PM
>>>>>
>>>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>>>> by releasing all the bound devices from that sva domain. Technically,
>>>>> there could be more than one sva domain identified by the mm PASID for
>>>>> the use of bound devices issuing DMA transactions.
>>>>
>>>> Could you elaborate it with some concrete examples which motivate
>>>> this change?
>>> The motivation is to remove the superfluous IOTLB invalidation in
>>> current VT-d driver.
>>>
>>> Currently, in VT-d driver, due to lacking shared sva domain info, in
>>> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
>>> are performed per-device. However, difference devices could be behind
>>> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
>>> per-device gives us more iotlb invalidation than necessary (4 iotlb
>>> invalidation instead of 1). This issue may give more performance impact
>>> when in a virtual machine guest, as currently we have one virtual VT-d
>>> for in front of those virtual devices.
>>>
>>>
>>> This patch fixes this issue by attaching shared sva domain information
>>> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
>>>
>>
>> that is one of the motivations. e.g. another one as Jason suggested
>> is to cleanup to decouple the common sva logic from enqcmd. Both
>> should be mentioned in next version cover letter.
> 
> I also want to purge all the de-duplication and refcounting code
> around mm's and sva_binds from the drivers. Eg see the mess this makes
> of SMMUv3.
> 
> Core code provides a single iommu_domain per-mm for SVA. Driver can
> rely on this optimization and does not need to de-duplicate.
> 
> Single domain tracks all attachments. Driver can optimize using that
> information by de-duplicating (eg ASID invalidation vs ATC
> invalidation)
> 
> After this we need to fix the domain allocation op to add a
> 'alloc_domain_sva(dev, mm_struct)' op so that the drivers can setup
> their SVA domains fully in a nice lock-safe environment.
Agree. These can be added to the to-do list.

Regards,
-Tina
> 
> Jason

RE: [PATCH 2/5] iommu: Call helper function to get assigned pasid value

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, August 11, 2023 12:37 AM
> 
> On Thu, Aug 10, 2023 at 07:52:52AM +0000, Tian, Kevin wrote:
> 
> > > The driver should rely on there being exactly one iommu_domain for SVA
> > > per mm so it can hang the mm_notifier off the iommu_domain
> >
> > I'm confused. Isn't this series trying to allow multiple domains per mm?
> 
> It is doing both.
> 
> The main objective is to allow de-duplicating the SVA domains in the
> core code. The driver should be able to assume one SVA domain per
> instance, or even one SVA domain per compatible instance. The driver
> should not do any de-duplication.
> 
> But we can't just store a single iommu_domain in the mm_struct - we
> have the same problem as iommufd and we need to create more domains if
> the domains we already have are incompatible with the device.
> 
> Arguably this should not happen, and in any sane configuration we
> should have only 1 type of IOMMU driver that needs only 1 SVA domain.
> 
> But right now things like SMMUv3 have problems crossing domains across
> instances, so we could have one SVA domain per IOMMU instance until
> that is fixed.
> 

Thanks for the explanation. Tina, please include this information in your
next version so others can easily understand the motivation of introducing
multiple sva domains per mm.