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

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

[Tina Zhang]

This series is to share sva(shared virtual addressing) domains with all
devices bound to one mm.

Problem
-------
In the current iommu core code, sva domain is allocated per IOMMU group,
when device driver is binding a process address space to a device (which is
handled in iommu_sva_bind_device()). If one than more device is bound to
the same process address space, there must be more than one sva domain
instance, with each device having one. In other words, the sva domain
doesn't share between those devices bound to the same process address
space, and that leads to two problems:
1) device driver has to duplicate sva domains with enqcmd, as those sva
domains have the same PASID and are relevant to one virtual address space.
This makes the sva domain handling complex in device drivers.
2) IOMMU driver cannot get sufficient info of the IOMMUs that have
devices behind them bound to the same virtual address space, when handling
mmu_notifier_ops callbacks. As a result, IOMMU IOTLB invalidation is
performed per device instead of per IOMMU, and that may lead to
superfluous IOTLB invalidation issue, especially in a virtualization
environment where all devices may be behind one virtual IOMMU.

Solution
--------
This patch-set tries to fix those two problems by allowing sharing sva
domains with all devices bound to a mm. To achieve this, a new structure
pointer is introduced to mm to replace the old PASID field, which can keep
the info of PASID as well as the corresponding shared sva domains.
Besides, function iommu_sva_bind_device() is updated to ensure a new sva
domain can only be allocated when the old ones cannot work for the IOMMU.
With these changes, a device driver can expect one sva domain could work
for per PASID instance(e.g., enqcmd PASID instance), and therefore may get
rid of handling sva domain duplication. Besides, IOMMU driver (e.g., intel
vt-d driver) can get sufficient info (e.g., the info of the IOMMUs having
their devices bound to one virtual address space) when handling
mmu_notifier_ops callbacks, to remove the redundant IOTLB invalidations.

Arguably there shouldn't be more than one sva_domain with the same PASID,
and in any sane configuration there should be only 1 type of IOMMU driver
that needs only 1 SVA domain. However, in reality, IOMMUs on one platform
may not be identical to each other. Thus, attaching a sva domain that has
been successfully bound to device A behind a IOMMU A, to device B behind
IOMMU B may get failed due to the difference between IOMMU A and IOMMU
B. In this case, a new sva domain with the same PASID needs to be
allocated to work with IOMMU B. That's why we need a list to keep sva
domains of one PASID. For the platform where IOMMUs are compatible to each
other, there should be one sva domain in the list.

v2:
 - Add mm_get_enqcmd_pasid().
 - Update commit message.

v1: https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/

Tina Zhang (5):
  iommu: Add mm_get_enqcmd_pasid() helper function
  iommu: Introduce mm_get_pasid() helper function
  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                         | 27 ++++++++--
 include/linux/mm_types.h                      |  3 +-
 kernel/fork.c                                 |  1 -
 mm/init-mm.c                                  |  3 --
 8 files changed, 66 insertions(+), 40 deletions(-)

-- 
2.34.1

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

[Tina Zhang]

mm_get_enqcmd_pasid() is for getting enqcmd pasid value.

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

v2: change mm_get_pasid() to mm_get_enqcmd_pasid()

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 4a817d20ce3bb..d9034b1bbfdd6 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_enqcmd_pasid(current->mm);
 
 	/*
 	 * Did this thread already have its PASID activated?
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d316425966759..ab9919746fd33 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_enqcmd_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_enqcmd_pasid(struct mm_struct *mm)
+{
+	return IOMMU_PASID_INVALID;
+}
 static inline void mm_pasid_drop(struct mm_struct *mm) {}
 #endif /* CONFIG_IOMMU_SVA */
 
-- 
2.34.1

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

[Baolu Lu]

On 2023/8/27 16:43, Tina Zhang wrote:
> mm_get_enqcmd_pasid() is for getting enqcmd pasid value.
> 
> The motivation is to replace mm->pasid with an iommu private data
> structure that is introduced in a later patch.
> 
> v2: change mm_get_pasid() to mm_get_enqcmd_pasid()

The change log should be moved under the tear line.

> 
> 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(-)

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

Best regards,
baolu

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

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, August 31, 2023 10:07 AM
> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> Tian, Kevin <kevin.tian@intel.com>; Michael Shavit <mshavit@google.com>
> Cc: baolu.lu@linux.intel.com; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/5] iommu: Add mm_get_enqcmd_pasid() helper
> function
> 
> On 2023/8/27 16:43, Tina Zhang wrote:
> > mm_get_enqcmd_pasid() is for getting enqcmd pasid value.
> >
> > The motivation is to replace mm->pasid with an iommu private data
> > structure that is introduced in a later patch.
> >
> > v2: change mm_get_pasid() to mm_get_enqcmd_pasid()
> 
> The change log should be moved under the tear line.
Right. Thanks.

Regards,
-Tina
> 
> >
> > 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(-)
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> 
> Best regards,
> baolu

[PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Tina Zhang]

Use the helper function mm_get_pasid() to get a mm assigned pasid
value. The motivation is to replace mm->pasid with an iommu private
data structure that is introduced in a later patch.

v2:
- Update commit message
- Let mm_get_enqcmd_pasid() call mm_get_pasid() to get pasid

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 +++++++-------
 include/linux/iommu.h                           | 10 +++++++++-
 4 files changed, 26 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 05c0fb2acbc44..0a4a1ed40814c 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));
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ab9919746fd33..ab8784dfdbd98 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1180,10 +1180,14 @@ static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
 	return mm->pasid != IOMMU_PASID_INVALID;
 }
-static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
+static inline u32 mm_get_pasid(struct mm_struct *mm)
 {
 	return mm->pasid;
 }
+static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
+{
+	return mm_get_pasid(mm);
+}
 void mm_pasid_drop(struct mm_struct *mm);
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm);
@@ -1206,6 +1210,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 u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
 {
 	return IOMMU_PASID_INVALID;
-- 
2.34.1

Re: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Baolu Lu]

On 2023/8/27 16:43, Tina Zhang wrote:
> Use the helper function mm_get_pasid() to get a mm assigned pasid
> value. The motivation is to replace mm->pasid with an iommu private
> data structure that is introduced in a later patch.
> 
> v2:
> - Update commit message
> - Let mm_get_enqcmd_pasid() call mm_get_pasid() to get pasid

Ditto.

> 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 +++++++-------
>   include/linux/iommu.h                           | 10 +++++++++-
>   4 files changed, 26 insertions(+), 18 deletions(-)

Eventually perhaps we should have something like sva_domain_get_pasid().

But the subsystem still needs some evolution to achieve this. In the
individual iommu driver, the mm_notifier should be wrapped in the sva
domain, hence the domain could be retrieved in the mm notifier callback.
With this done, the iommu drivers call sva_domain_get_pasid() instead of
mm_get_pasid().

Finally the iommu drivers only need mm->pgd, nothing else.

Considering that this patch will make the subsequent patches simpler, I
do not object to it. Hence,

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

Best regards,
baolu

Re: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Jason Gunthorpe]

On Thu, Aug 31, 2023 at 10:24:31AM +0800, Baolu Lu wrote:
> On 2023/8/27 16:43, Tina Zhang wrote:
> > Use the helper function mm_get_pasid() to get a mm assigned pasid
> > value. The motivation is to replace mm->pasid with an iommu private
> > data structure that is introduced in a later patch.
> > 
> > v2:
> > - Update commit message
> > - Let mm_get_enqcmd_pasid() call mm_get_pasid() to get pasid
> 
> Ditto.
> 
> > 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 +++++++-------
> >   include/linux/iommu.h                           | 10 +++++++++-
> >   4 files changed, 26 insertions(+), 18 deletions(-)
> 
> Eventually perhaps we should have something like sva_domain_get_pasid().

There is never just a single PASID for a domain. That should not be
part of any of our APIs.

If we want to provide core code aide then the core code should have a
means to help the drvier maintain the attachment database for the
domain.

Eg maintain the list of RIDs, PASIDs, etc that the domain is linked
to.

But this is such trivial code I'm not sure it helps much

> Finally the iommu drivers only need mm->pgd, nothing else.

Yes, attaching the notifier and accessing the arch specific mm->pgd
should be the only driver touches of the mm_struct..

Jason

Re: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Baolu Lu]

On 2023/8/27 16:43, Tina Zhang wrote:
> 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);
>   	}

There is no need to use mm_get_pasid(mm) in the set_dev_pasid path. The
pasid has already passed as a parameter. Perhaps, pass domain and pasid
to intel_svm_bind_mm(), or simply merge intel_svm_bind_mm() to
intel_svm_set_dev_pasid()?

Something like below?

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 8f6d68006ab6..de490b3409cc 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -298,21 +298,22 @@ static int pasid_to_svm_sdev(struct device *dev, 
unsigned int pasid,
  }

  static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device 
*dev,
-			     struct mm_struct *mm)
+			     struct iommu_domain *domain, ioasid_t pasid)
  {
  	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct mm_struct *mm = domain->mm;
  	struct intel_svm_dev *sdev;
  	struct intel_svm *svm;
  	unsigned long sflags;
  	int ret = 0;

-	svm = pasid_private_find(mm->pasid);
+	svm = pasid_private_find(pasid);
  	if (!svm) {
  		svm = kzalloc(sizeof(*svm), GFP_KERNEL);
  		if (!svm)
  			return -ENOMEM;

-		svm->pasid = mm->pasid;
+		svm->pasid = pasid;
  		svm->mm = mm;
  		INIT_LIST_HEAD_RCU(&svm->devs);

@@ -350,7 +351,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, pasid,
  					    FLPT_DEFAULT_DID, sflags);
  	if (ret)
  		goto free_sdev;
@@ -364,7 +365,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(pasid);
  		kfree(svm);
  	}

@@ -839,11 +840,10 @@ static int intel_svm_set_dev_pasid(struct 
iommu_domain *domain,
  {
  	struct device_domain_info *info = dev_iommu_priv_get(dev);
  	struct intel_iommu *iommu = info->iommu;
-	struct mm_struct *mm = domain->mm;
  	int ret;

  	mutex_lock(&pasid_mutex);
-	ret = intel_svm_bind_mm(iommu, dev, mm);
+	ret = intel_svm_bind_mm(iommu, dev, domain, pasid);
  	mutex_unlock(&pasid_mutex);

  	return ret;

Best regards,
baolu

Re: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Jason Gunthorpe]

On Thu, Aug 31, 2023 at 01:14:20PM +0800, Baolu Lu wrote:
> On 2023/8/27 16:43, Tina Zhang wrote:
> > 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);
> >   	}
> 
> There is no need to use mm_get_pasid(mm) in the set_dev_pasid path. The
> pasid has already passed as a parameter. Perhaps, pass domain and pasid
> to intel_svm_bind_mm(), or simply merge intel_svm_bind_mm() to
> intel_svm_set_dev_pasid()?
> 
> Something like below?

Yes please! As a prep patch 'remove mm->pasid references from vt-d'

Jason

RE: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function

[Zhang, Tina]

Hi Baolu and Jason,

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, September 1, 2023 1:03 AM
> To: Baolu Lu <baolu.lu@linux.intel.com>
> Cc: Zhang, Tina <tina.zhang@intel.com>; Tian, Kevin <kevin.tian@intel.com>;
> Michael Shavit <mshavit@google.com>; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] iommu: Introduce mm_get_pasid() helper function
> 
> On Thu, Aug 31, 2023 at 01:14:20PM +0800, Baolu Lu wrote:
> > On 2023/8/27 16:43, Tina Zhang wrote:
> > > 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);
> > >   	}
> >
> > There is no need to use mm_get_pasid(mm) in the set_dev_pasid path.
> > The pasid has already passed as a parameter. Perhaps, pass domain and
> > pasid to intel_svm_bind_mm(), or simply merge intel_svm_bind_mm() to
> > intel_svm_set_dev_pasid()?
> >
> > Something like below?
> 
> Yes please! As a prep patch 'remove mm->pasid references from vt-d'

Reasonable, as we believe that eventually vt-d driver only needs mm->pgd, nothing else for mm.

Thanks,
-Tina
> 
> Jason

[PATCH v2 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 ab8784dfdbd98..937f3abc26f2e 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 5e74ce4a28cd6..3fd65b7537f0e 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.34.1

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

[Baolu Lu]

On 2023/8/27 16:43, Tina Zhang wrote:
> 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(+)

Nit:

iommu also has a per-device private pointer, it's defined as struct
dev_iommu and stored at dev->iommu. Is it valuable to align both?

Best regards,
baolu

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

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, August 31, 2023 10:45 AM
> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> Tian, Kevin <kevin.tian@intel.com>; Michael Shavit <mshavit@google.com>
> Cc: baolu.lu@linux.intel.com; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 3/5] mm: Add structure to keep sva information
> 
> On 2023/8/27 16:43, Tina Zhang wrote:
> > 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(+)
> 
> Nit:
> 
> iommu also has a per-device private pointer, it's defined as struct dev_iommu
> and stored at dev->iommu. Is it valuable to align both?
I'm not sure if I understand the idea correctly. This struct dev_iommu is used to describe a collection per-device IOMMU data. Is the idea about migrating some bits from this struct dev_iommu to iommu_mm_data structure?

Thanks,
-Tina
> 
> Best regards,
> baolu

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

[Baolu Lu]

On 2023/9/1 11:36, Zhang, Tina wrote:
>> -----Original Message-----
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Thursday, August 31, 2023 10:45 AM
>> To: Zhang, Tina<tina.zhang@intel.com>; Jason Gunthorpe<jgg@ziepe.ca>;
>> Tian, Kevin<kevin.tian@intel.com>; Michael Shavit<mshavit@google.com>
>> Cc:baolu.lu@linux.intel.com;iommu@lists.linux.dev; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 3/5] mm: Add structure to keep sva information
>>
>> On 2023/8/27 16:43, Tina Zhang wrote:
>>> 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(+)
>> Nit:
>>
>> iommu also has a per-device private pointer, it's defined as struct dev_iommu
>> and stored at dev->iommu. Is it valuable to align both?
> I'm not sure if I understand the idea correctly. This struct dev_iommu is used to describe a collection per-device IOMMU data. Is the idea about migrating some bits from this struct dev_iommu to iommu_mm_data structure?

Never mind. I just thought about this when I was reading the patch. This
does not constitute any suggestion.

Best regards,
baolu

[PATCH v2 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 no 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 0a4a1ed40814c..35366f51ad27d 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 937f3abc26f2e..cfbd35ceb375f 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;
 }
 static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
 {
@@ -1213,7 +1210,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 d2e12b6d2b180..f06392dd1ca8a 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.34.1

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

[Vasant Hegde]

Hi Tina,

On 8/27/2023 2:14 PM, 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 no 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(-)
> 


.../...

>  
>  	/* 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);


I am not sure whether I understood the flow completely. Just wondering why you
are not freeing pasid in iommu_sva_unbind_device().
I mean once iommu_mm->sva_domains becomes free shouldn't we free the
iommu_mm->pasid?

Also in this function (mm_pasid_drop()), should we check/free sva domains?

-Vasant

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

[Zhang, Tina]

Hi Vasant,

> -----Original Message-----
> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Monday, August 28, 2023 4:33 PM
> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> Tian, Kevin <kevin.tian@intel.com>; Lu Baolu <baolu.lu@linux.intel.com>;
> Michael Shavit <mshavit@google.com>
> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains
> 
> Hi Tina,
> 
> On 8/27/2023 2:14 PM, 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 no 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(-)
> >
> 
> 
> .../...
> 
> >
> >  	/* 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);
> 
> 
> I am not sure whether I understood the flow completely. Just wondering why
> you are not freeing pasid in iommu_sva_unbind_device().
> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
> iommu_mm->pasid?
No, the sva domain and the PASID are separate objects with their own lifecycles.
The iommu_mm->pasid is released when the mm is being released, meanwhile the sva_domain is released when no one is using it.

Regards,
-Tina
> 
> Also in this function (mm_pasid_drop()), should we check/free sva domains?
> 
> -Vasant

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

[Vasant Hegde]

Hi Tina,

On 8/28/2023 2:40 PM, Zhang, Tina wrote:
> Hi Vasant,
> 
>> -----Original Message-----
>> From: Vasant Hegde <vasant.hegde@amd.com>
>> Sent: Monday, August 28, 2023 4:33 PM
>> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
>> Tian, Kevin <kevin.tian@intel.com>; Lu Baolu <baolu.lu@linux.intel.com>;
>> Michael Shavit <mshavit@google.com>
>> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains
>>
>> Hi Tina,
>>
>> On 8/27/2023 2:14 PM, 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 no 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(-)
>>>
>>
>>
>> .../...
>>
>>>
>>>  	/* 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);
>>
>>
>> I am not sure whether I understood the flow completely. Just wondering why
>> you are not freeing pasid in iommu_sva_unbind_device().
>> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
>> iommu_mm->pasid?
> No, the sva domain and the PASID are separate objects with their own lifecycles.
> The iommu_mm->pasid is released when the mm is being released, meanwhile the sva_domain is released when no one is using it.

Thanks for the explanation.

-Vasant

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

[Jason Gunthorpe]

On Mon, Aug 28, 2023 at 02:02:52PM +0530, Vasant Hegde wrote:

> I am not sure whether I understood the flow completely. Just wondering why you
> are not freeing pasid in iommu_sva_unbind_device().
> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
> iommu_mm->pasid?

No, for Intel use cases that PASID permanently becomes part of the
ENQCMD MSR and cannot be revoked once it has been set
 
> Also in this function (mm_pasid_drop()), should we check/free sva domains?

No, the driver must support a SVA domain with an empty mm_struct, eg
by hooking release.

Jason

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

[Vasant Hegde]

On 8/31/2023 2:06 AM, Jason Gunthorpe wrote:
> On Mon, Aug 28, 2023 at 02:02:52PM +0530, Vasant Hegde wrote:
> 
>> I am not sure whether I understood the flow completely. Just wondering why you
>> are not freeing pasid in iommu_sva_unbind_device().
>> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
>> iommu_mm->pasid?
> 
> No, for Intel use cases that PASID permanently becomes part of the
> ENQCMD MSR and cannot be revoked once it has been set
>  

Fair enough.

Patch description did explain it to some extent. ("The PASID is released in
__mmdrop()"). May be this needs to be expanded to cover why pasid is not
released in iommu_sva_unbind_device().


>> Also in this function (mm_pasid_drop()), should we check/free sva domains?
> 
> No, the driver must support a SVA domain with an empty mm_struct, eg
> by hooking release.

Sorry. Looks like confused you. Looking into code I got this.

My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
getting called before freeing all SVA domains?

-Vasant

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

[Baolu Lu]

On 2023/8/31 14:42, Vasant Hegde wrote:
>>> Also in this function (mm_pasid_drop()), should we check/free sva domains?
>> No, the driver must support a SVA domain with an empty mm_struct, eg
>> by hooking release.
> Sorry. Looks like confused you. Looking into code I got this.
> 
> My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
> getting called before freeing all SVA domains?

I remember we have discussed this during sva development. If I remember
it correctly, in any case, mm_pasid_drop() will be called after the
device is closed. The device driver will detach the sva domains in the
close path.

Best regards,
baolu

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

[Jason Gunthorpe]

On Thu, Aug 31, 2023 at 03:35:36PM +0800, Baolu Lu wrote:
> On 2023/8/31 14:42, Vasant Hegde wrote:
> > > > Also in this function (mm_pasid_drop()), should we check/free sva domains?
> > > No, the driver must support a SVA domain with an empty mm_struct, eg
> > > by hooking release.
> > Sorry. Looks like confused you. Looking into code I got this.
> > 
> > My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
> > getting called before freeing all SVA domains?
> 
> I remember we have discussed this during sva development. If I remember
> it correctly, in any case, mm_pasid_drop() will be called after the
> device is closed. 

Yes, we guarentee this because the SVA domain should be holding a
mmgrab on the mm_struct while it exists. The mmdrop cannot happen
until the SVA domain is freed which puts back that ref.

But drivers must not make any assumptions about this, the lifecycle of
the PASID is a core concern, so long as the driver is asked to attach
a domain to a PASID it should assume the PASID is valid.

A driver should *never* look at the mm_struct PASID, all the examples
of this in-tree today are wrong.

Jason

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

[Baolu Lu]

On 2023/8/27 16:44, 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 no 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 0a4a1ed40814c..35366f51ad27d 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;

No need to initialize this variable.

>   	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);

If you change the order of the two lines above, it will look more
comfortable.

>   
> -	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 937f3abc26f2e..cfbd35ceb375f 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;

It looks better if you add a comment to describe how this list node is
used and how the list is protected.

>   		};
>   	};
>   };
> @@ -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;
>   }
>   static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
>   {
> @@ -1213,7 +1210,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 d2e12b6d2b180..f06392dd1ca8a 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);

This patch overall looks good to me. With above nits addressed,

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

Best regards,
baolu

[PATCH v2 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 3fd65b7537f0e..6cb5cc53c4803 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 efa97b57acfd8..69719291463ed 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.34.1

Re: [PATCH v2 5/5] mm: Deprecate pasid field

[Niklas Schnelle]

On Sun, 2023-08-27 at 16:44 +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.

I think it should say "Drop" instead of "Deprecate" in the subject line
as well since this is field is completely removed.

> 
> 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 3fd65b7537f0e..6cb5cc53c4803 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 efa97b57acfd8..69719291463ed 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)
>  };
>  

Re: [PATCH v2 5/5] mm: Deprecate pasid field

[Baolu Lu]

On 2023/8/27 16:44, 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>
> ---
>   include/linux/mm_types.h | 1 -
>   mm/init-mm.c             | 3 ---
>   2 files changed, 4 deletions(-)

mm->pasid is dead code now. So remove it.

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

Best regards,
baolu