[PATCH 00/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[PATCH 00/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Tina Zhang]

This series based on "Share sva" patch-set[1], aims to remove superfluous
IOMMU IOTLB invalidations in VT-d driver.

In the current VT-d driver, IOMMU IOTLB invalidation commands and
device-TLB commands are performed per device, which leads to superfluous
IOTLB invalidations. For example, if there are four devices behind a IOMMU
are attached to one sva domain (which could be a common case in
virtualization scenarios where one virtual IOMMU working for all the
virtual devices), four IOTLB invalidation commands and four device-IOTLB
invalidation commands will be issued. However, only one IOTLB invalidation
command and four device-IOTLB invalidation commands are necessary.
Superfluous IOMMU IOTLB invalidations impact run-time performance.

The first 11 patches of this patch-set are for refactoring:
1) Retire struct intel_svm_set_dev_pasid and struct intel_svm_dev to
avoid duplicating code for sva domain.
2) Reuse intel_iommu_set/remove_dev_pasid functions for both sva domain
and dmar_domain.

The last patch removes the superfluous IOMMU IOTLB invalidations.

[1]: https://lore.kernel.org/linux-iommu/ZXgj49cY2PEAwHsB@8bytes.org/


Change log:
RFC: https://lore.kernel.org/linux-iommu/20231017032045.114868-1-tina.zhang@intel.com/

Jason Gunthorpe (1):
  iommu: Add ops->domain_alloc_sva()

Tina Zhang (10):
  iommu/vt-d: Retire the treatment for revoking PASIDs with pending
    pgfaults
  iommu/vt-d: Remove initialization for dynamically heap-allocated
    rcu_head
  iommu/vt-d: Refactor intel_svm_set_dev_pasid function
  iommu/vt-d: Retire struct intel_svm_dev
  iommu/vt-d: Retire struct intel_svm
  iommu/vt-d: Use RCU for dev_pasids list updates in
    set/remove_dev_pasid()
  iommu/vt-d: Add domain_type_is_sva helper function
  iommu/vt-d: Reuse intel_iommu_set_dev_pasid function
  iommu/vt-d: Retire intel_svm_remove_dev_pasid function
  iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

 drivers/iommu/intel/iommu.c |  76 ++++++----
 drivers/iommu/intel/iommu.h |  31 ++--
 drivers/iommu/intel/svm.c   | 288 ++++++++----------------------------
 drivers/iommu/iommu-sva.c   |  16 +-
 include/linux/iommu.h       |   3 +
 5 files changed, 134 insertions(+), 280 deletions(-)

-- 
2.39.3

[PATCH 01/11] iommu/vt-d: Retire the treatment for revoking PASIDs with pending pgfaults

[Tina Zhang]

Revoking PASIDs with pending page faults has been achieved by clearing the
pasid entry and draining the pending page faults in both hardware and
software. The temporary treatment can be retired now.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/svm.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index e1cbcb9515f0..a88051b9d802 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -407,13 +407,6 @@ void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
 			if (svm->notifier.ops)
 				mmu_notifier_unregister(&svm->notifier, mm);
 			pasid_private_remove(svm->pasid);
-			/*
-			 * We mandate that no page faults may be outstanding
-			 * for the PASID when intel_svm_unbind_mm() is called.
-			 * If that is not obeyed, subtle errors will happen.
-			 * Let's make them less subtle...
-			 */
-			memset(svm, 0x6b, sizeof(*svm));
 			kfree(svm);
 		}
 	}
-- 
2.39.3

[PATCH 02/11] iommu/vt-d: Remove initialization for dynamically heap-allocated rcu_head

[Tina Zhang]

The rcu_head structures allocated dynamically in the heap don't need any
initialization. Therefore, remove the init_rcu_head().

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

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index a88051b9d802..1a72b684baea 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -359,7 +359,6 @@ static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
 	sdev->iommu = iommu;
 	sdev->did = FLPT_DEFAULT_DID;
 	sdev->sid = PCI_DEVID(info->bus, info->devfn);
-	init_rcu_head(&sdev->rcu);
 	if (info->ats_enabled) {
 		sdev->qdep = info->ats_qdep;
 		if (sdev->qdep >= QI_DEV_EIOTLB_MAX_INVS)
-- 
2.39.3

[PATCH 03/11] iommu/vt-d: Refactor intel_svm_set_dev_pasid function

[Tina Zhang]

Refactor intel_svm_set_dev_pasid() by moving the logic from
intel_svm_bind_mm() into intel_svm_set_dev_pasid() and retire
intel_svm_bind_mm(). No functional change intended.

The motivation is to enable intel_iommu_set_dev_pasid() to serve for both
sva domain's and default domain's set_dev_pasid operations, instead of
duplicating code for them.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/svm.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 1a72b684baea..5b1331b174df 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -314,10 +314,11 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
 	return 0;
 }
 
-static int intel_svm_bind_mm(struct intel_iommu *iommu, struct device *dev,
-			     struct iommu_domain *domain, ioasid_t pasid)
+static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
+				   struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
 	struct mm_struct *mm = domain->mm;
 	struct intel_svm_dev *sdev;
 	struct intel_svm *svm;
@@ -794,15 +795,6 @@ int intel_svm_page_response(struct device *dev,
 	return ret;
 }
 
-static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
-				   struct device *dev, ioasid_t pasid)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu = info->iommu;
-
-	return intel_svm_bind_mm(iommu, dev, domain, pasid);
-}
-
 static void intel_svm_domain_free(struct iommu_domain *domain)
 {
 	kfree(to_dmar_domain(domain));
-- 
2.39.3

[PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev

[Tina Zhang]

The struct dev_pasid_info is used by IOMMU domain to keep pasid info of
attached device. For sva domain, there is another structure which keeps
info of attached device, named intel_svm_dev. Instead of using two structs
for the same purpose, the struct dev_pasid_info can work for both sva
domain and IOMMU domain. As a result, struct intel_svm_dev gets retired.

The rcu/did/sid/qdep fields of struct intel_svm_dev are moved from struct
intel_svm_dev into struct dev_pasid_info. Besides, the names of functions
which are related to struct intel_svm_dev get changed to dev_pasid_info.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.h |  11 +---
 drivers/iommu/intel/svm.c   | 110 ++++++++++++++++++------------------
 2 files changed, 57 insertions(+), 64 deletions(-)

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 696d95293a69..4a2f6b80b837 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -761,6 +761,8 @@ struct dev_pasid_info {
 #ifdef CONFIG_INTEL_IOMMU_DEBUGFS
 	struct dentry *debugfs_dentry; /* pointer to pasid directory dentry */
 #endif
+	struct rcu_head rcu;
+	u16 did, sid, qdep;
 };
 
 static inline void __iommu_flush_cache(
@@ -1085,15 +1087,6 @@ struct iommu_domain *intel_svm_domain_alloc(void);
 void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
 void intel_drain_pasid_prq(struct device *dev, u32 pasid);
 
-struct intel_svm_dev {
-	struct list_head list;
-	struct rcu_head rcu;
-	struct device *dev;
-	struct intel_iommu *iommu;
-	u16 did;
-	u16 sid, qdep;
-};
-
 struct intel_svm {
 	struct mmu_notifier notifier;
 	struct mm_struct *mm;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 5b1331b174df..0a934866539f 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -43,21 +43,21 @@ static void *pasid_private_find(ioasid_t pasid)
 	return xa_load(&pasid_private_array, pasid);
 }
 
-static struct intel_svm_dev *
-svm_lookup_device_by_dev(struct intel_svm *svm, struct device *dev)
+static struct dev_pasid_info *
+svm_lookup_dev_pasid_info_by_dev(struct intel_svm *svm, struct device *dev)
 {
-	struct intel_svm_dev *sdev = NULL, *t;
+	struct dev_pasid_info *dev_pasid = NULL, *t;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(t, &svm->devs, list) {
+	list_for_each_entry_rcu(t, &svm->devs, link_domain) {
 		if (t->dev == dev) {
-			sdev = t;
+			dev_pasid = t;
 			break;
 		}
 	}
 	rcu_read_unlock();
 
-	return sdev;
+	return dev_pasid;
 }
 
 int intel_svm_enable_prq(struct intel_iommu *iommu)
@@ -169,27 +169,27 @@ void intel_svm_check(struct intel_iommu *iommu)
 }
 
 static void __flush_svm_range_dev(struct intel_svm *svm,
-				  struct intel_svm_dev *sdev,
+				  struct dev_pasid_info *dev_pasid,
 				  unsigned long address,
 				  unsigned long pages, int ih)
 {
-	struct device_domain_info *info = dev_iommu_priv_get(sdev->dev);
+	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid->dev);
 
 	if (WARN_ON(!pages))
 		return;
 
-	qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, pages, ih);
+	qi_flush_piotlb(info->iommu, dev_pasid->did, svm->pasid, address, pages, ih);
 	if (info->ats_enabled) {
-		qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid,
-					 svm->pasid, sdev->qdep, address,
+		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
+					 svm->pasid, dev_pasid->qdep, address,
 					 order_base_2(pages));
 		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
-					  svm->pasid, sdev->qdep);
+					  svm->pasid, dev_pasid->qdep);
 	}
 }
 
 static void intel_flush_svm_range_dev(struct intel_svm *svm,
-				      struct intel_svm_dev *sdev,
+				      struct dev_pasid_info *dev_pasid,
 				      unsigned long address,
 				      unsigned long pages, int ih)
 {
@@ -199,7 +199,7 @@ static void intel_flush_svm_range_dev(struct intel_svm *svm,
 	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
 
 	while (start < end) {
-		__flush_svm_range_dev(svm, sdev, start, align >> VTD_PAGE_SHIFT, ih);
+		__flush_svm_range_dev(svm, dev_pasid, start, align >> VTD_PAGE_SHIFT, ih);
 		start += align;
 	}
 }
@@ -207,30 +207,30 @@ static void intel_flush_svm_range_dev(struct intel_svm *svm,
 static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,
 				unsigned long pages, int ih)
 {
-	struct intel_svm_dev *sdev;
+	struct dev_pasid_info *dev_pasid;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(sdev, &svm->devs, list)
-		intel_flush_svm_range_dev(svm, sdev, address, pages, ih);
+	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain)
+		intel_flush_svm_range_dev(svm, dev_pasid, address, pages, ih);
 	rcu_read_unlock();
 }
 
 static void intel_flush_svm_all(struct intel_svm *svm)
 {
 	struct device_domain_info *info;
-	struct intel_svm_dev *sdev;
+	struct dev_pasid_info *dev_pasid = NULL;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(sdev, &svm->devs, list) {
-		info = dev_iommu_priv_get(sdev->dev);
+	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain) {
+		info = dev_iommu_priv_get(dev_pasid->dev);
 
-		qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, 0, -1UL, 0);
+		qi_flush_piotlb(info->iommu, dev_pasid->did, svm->pasid, 0, -1UL, 0);
 		if (info->ats_enabled) {
-			qi_flush_dev_iotlb_pasid(sdev->iommu, sdev->sid, info->pfsid,
-						 svm->pasid, sdev->qdep,
+			qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
+						 svm->pasid, dev_pasid->qdep,
 						 0, 64 - VTD_PAGE_SHIFT);
 			quirk_extra_dev_tlb_flush(info, 0, 64 - VTD_PAGE_SHIFT,
-						  svm->pasid, sdev->qdep);
+						  svm->pasid, dev_pasid->qdep);
 		}
 	}
 	rcu_read_unlock();
@@ -255,7 +255,8 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
 static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
 	struct intel_svm *svm = container_of(mn, struct intel_svm, notifier);
-	struct intel_svm_dev *sdev;
+	struct device_domain_info *info;
+	struct dev_pasid_info *dev_pasid;
 
 	/* This might end up being called from exit_mmap(), *before* the page
 	 * tables are cleared. And __mmu_notifier_release() will delete us from
@@ -270,9 +271,11 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * *has* to handle gracefully without affecting other processes.
 	 */
 	rcu_read_lock();
-	list_for_each_entry_rcu(sdev, &svm->devs, list)
-		intel_pasid_tear_down_entry(sdev->iommu, sdev->dev,
+	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain) {
+		info = dev_iommu_priv_get(dev_pasid->dev);
+		intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
 					    svm->pasid, true);
+	}
 	rcu_read_unlock();
 
 }
@@ -282,11 +285,11 @@ static const struct mmu_notifier_ops intel_mmuops = {
 	.arch_invalidate_secondary_tlbs = intel_arch_invalidate_secondary_tlbs,
 };
 
-static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
+static int pasid_to_dev_pasid_info(struct device *dev, unsigned int pasid,
 			     struct intel_svm **rsvm,
-			     struct intel_svm_dev **rsdev)
+			     struct dev_pasid_info **rsdev_pasid_info)
 {
-	struct intel_svm_dev *sdev = NULL;
+	struct dev_pasid_info *dev_pasid = NULL;
 	struct intel_svm *svm;
 
 	if (pasid == IOMMU_PASID_INVALID || pasid >= PASID_MAX)
@@ -305,11 +308,11 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
 	 */
 	if (WARN_ON(list_empty(&svm->devs)))
 		return -EINVAL;
-	sdev = svm_lookup_device_by_dev(svm, dev);
+	dev_pasid = svm_lookup_dev_pasid_info_by_dev(svm, dev);
 
 out:
 	*rsvm = svm;
-	*rsdev = sdev;
+	*rsdev_pasid_info = dev_pasid;
 
 	return 0;
 }
@@ -320,7 +323,7 @@ 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;
-	struct intel_svm_dev *sdev;
+	struct dev_pasid_info *dev_pasid;
 	struct intel_svm *svm;
 	unsigned long sflags;
 	int ret = 0;
@@ -350,35 +353,34 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
-	if (!sdev) {
+	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+	if (!dev_pasid) {
 		ret = -ENOMEM;
 		goto free_svm;
 	}
 
-	sdev->dev = dev;
-	sdev->iommu = iommu;
-	sdev->did = FLPT_DEFAULT_DID;
-	sdev->sid = PCI_DEVID(info->bus, info->devfn);
+	dev_pasid->dev = dev;
+	dev_pasid->did = FLPT_DEFAULT_DID;
+	dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
 	if (info->ats_enabled) {
-		sdev->qdep = info->ats_qdep;
-		if (sdev->qdep >= QI_DEV_EIOTLB_MAX_INVS)
-			sdev->qdep = 0;
+		dev_pasid->qdep = info->ats_qdep;
+		if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
+			dev_pasid->qdep = 0;
 	}
 
 	/* Setup the pasid table: */
 	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
 	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, pasid,
-					    FLPT_DEFAULT_DID, sflags);
+					    dev_pasid->did, sflags);
 	if (ret)
-		goto free_sdev;
+		goto free_dev_pasid;
 
-	list_add_rcu(&sdev->list, &svm->devs);
+	list_add_rcu(&dev_pasid->link_domain, &svm->devs);
 
 	return 0;
 
-free_sdev:
-	kfree(sdev);
+free_dev_pasid:
+	kfree(dev_pasid);
 free_svm:
 	if (list_empty(&svm->devs)) {
 		mmu_notifier_unregister(&svm->notifier, mm);
@@ -391,21 +393,19 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 
 void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
 {
-	struct intel_svm_dev *sdev;
+	struct dev_pasid_info *dev_pasid;
 	struct intel_svm *svm;
-	struct mm_struct *mm;
 
-	if (pasid_to_svm_sdev(dev, pasid, &svm, &sdev))
+	if (pasid_to_dev_pasid_info(dev, pasid, &svm, &dev_pasid))
 		return;
-	mm = svm->mm;
 
-	if (sdev) {
-		list_del_rcu(&sdev->list);
-		kfree_rcu(sdev, rcu);
+	if (dev_pasid) {
+		list_del_rcu(&dev_pasid->link_domain);
+		kfree_rcu(dev_pasid, rcu);
 
 		if (list_empty(&svm->devs)) {
 			if (svm->notifier.ops)
-				mmu_notifier_unregister(&svm->notifier, mm);
+				mmu_notifier_unregister(&svm->notifier, svm->mm);
 			pasid_private_remove(svm->pasid);
 			kfree(svm);
 		}
-- 
2.39.3

Re: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev

[Baolu Lu]

On 2024/1/16 9:11, Tina Zhang wrote:
> The struct dev_pasid_info is used by IOMMU domain to keep pasid info of
> attached device. For sva domain, there is another structure which keeps
> info of attached device, named intel_svm_dev. Instead of using two structs
> for the same purpose, the struct dev_pasid_info can work for both sva
> domain and IOMMU domain. As a result, struct intel_svm_dev gets retired.
> 
> The rcu/did/sid/qdep fields of struct intel_svm_dev are moved from struct
> intel_svm_dev into struct dev_pasid_info. Besides, the names of functions
> which are related to struct intel_svm_dev get changed to dev_pasid_info.
> 
> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.h |  11 +---
>   drivers/iommu/intel/svm.c   | 110 ++++++++++++++++++------------------
>   2 files changed, 57 insertions(+), 64 deletions(-)

I like the direction that this patch is taking. I agree that
intel_svm_dev should be replaced with dev_pasid_info. However, since
dev_pasid_info is protected by domain->lock and intel_svm_dev is
protected by RCU, I recommend splitting this change into two patches:

- The first patch would modify the code to use domain->lock to protect
   intel_svm_dev.
- The second patch would then perform the actual replacement of
   intel_svm_dev with dev_pasid_info.

Best regards,
baolu

RE: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, January 17, 2024 3:21 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev
> 
> On 2024/1/16 9:11, Tina Zhang wrote:
> > The struct dev_pasid_info is used by IOMMU domain to keep pasid info
> > of attached device. For sva domain, there is another structure which
> > keeps info of attached device, named intel_svm_dev. Instead of using
> > two structs for the same purpose, the struct dev_pasid_info can work
> > for both sva domain and IOMMU domain. As a result, struct intel_svm_dev
> gets retired.
> >
> > The rcu/did/sid/qdep fields of struct intel_svm_dev are moved from
> > struct intel_svm_dev into struct dev_pasid_info. Besides, the names of
> > functions which are related to struct intel_svm_dev get changed to
> dev_pasid_info.
> >
> > Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.h |  11 +---
> >   drivers/iommu/intel/svm.c   | 110 ++++++++++++++++++------------------
> >   2 files changed, 57 insertions(+), 64 deletions(-)
> 
> I like the direction that this patch is taking. I agree that intel_svm_dev should be
> replaced with dev_pasid_info. However, since dev_pasid_info is protected by
> domain->lock and intel_svm_dev is protected by RCU, I recommend splitting this
> change into two patches:
> 
> - The first patch would modify the code to use domain->lock to protect
>    intel_svm_dev.
> - The second patch would then perform the actual replacement of
>    intel_svm_dev with dev_pasid_info.
Thanks for the suggestion. Sounds like replacing RCU with domain->lock is a good idea. I'll include it in the next version of this patch.

Regards,
-Tina

> 
> Best regards,
> baolu

RE: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Zhang, Tina <tina.zhang@intel.com>
> Sent: Tuesday, January 30, 2024 11:05 AM
> To: Baolu Lu <baolu.lu@linux.intel.com>; iommu@lists.linux.dev
> Cc: Tian, Kevin <kevin.tian@intel.com>
> Subject: RE: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev
> 
> Hi Baolu,
> 
> > -----Original Message-----
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Wednesday, January 17, 2024 3:21 PM
> > To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> > Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> > Subject: Re: [PATCH 04/11] iommu/vt-d: Retire struct intel_svm_dev
> >
> > On 2024/1/16 9:11, Tina Zhang wrote:
> > > The struct dev_pasid_info is used by IOMMU domain to keep pasid info
> > > of attached device. For sva domain, there is another structure which
> > > keeps info of attached device, named intel_svm_dev. Instead of using
> > > two structs for the same purpose, the struct dev_pasid_info can work
> > > for both sva domain and IOMMU domain. As a result, struct
> > > intel_svm_dev
> > gets retired.
> > >
> > > The rcu/did/sid/qdep fields of struct intel_svm_dev are moved from
> > > struct intel_svm_dev into struct dev_pasid_info. Besides, the names
> > > of functions which are related to struct intel_svm_dev get changed
> > > to
> > dev_pasid_info.
> > >
> > > Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> > > ---
> > >   drivers/iommu/intel/iommu.h |  11 +---
> > >   drivers/iommu/intel/svm.c   | 110 ++++++++++++++++++------------------
> > >   2 files changed, 57 insertions(+), 64 deletions(-)
> >
> > I like the direction that this patch is taking. I agree that
> > intel_svm_dev should be replaced with dev_pasid_info. However, since
> > dev_pasid_info is protected by
> > domain->lock and intel_svm_dev is protected by RCU, I recommend
> > domain->splitting this
> > change into two patches:
> >
> > - The first patch would modify the code to use domain->lock to protect
> >    intel_svm_dev.
> > - The second patch would then perform the actual replacement of
> >    intel_svm_dev with dev_pasid_info.
> Thanks for the suggestion. Sounds like replacing RCU with domain->lock is a
> good idea. I'll include it in the next version of this patch.
After rethinking about the idea of using domain->lock for sva domain instead of RCU, I have a concern that we may end up with sacrificing performance, as spin_lock()/spin_unlock() is heavier than rcu_read_lock()/rcu_read_unlock().

The arch_invalidate_secondary_tlbs() callback, which is the reader of dev_pasid_info list, could be invoked on a hot path.  If this is the case, we may expect the dev_pasid_info list itself as well as the data element listed in the list could be accessed concurrently with an updater for better performance. That's the place RCU can help. Besides, we also need to use spin lock between the updaters to ensure there is only one updater each time.

Regards,
-Tina

> 
> Regards,
> -Tina
> 
> >
> > Best regards,
> > baolu

[PATCH 05/11] iommu: Add ops->domain_alloc_sva()

[Tina Zhang]

From: Jason Gunthorpe <jgg@nvidia.com>

Make a new op that receives the device and the mm_struct that the SVA
domain should be created for. Unlike domain_alloc_paging() the dev
argument is never NULL here.

This allows drivers to fully initialize the SVA domain and allocate the
mmu_notifier during allocation. It allows the notifier lifetime to follow
the lifetime of the iommu_domain.

Since we have only one call site, upgrade the new op to return ERR_PTR
instead of NULL.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
[Updated domain owner and removed smmu3 related changes - Vasant]
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/iommu-sva.c | 16 +++++++++++-----
 include/linux/iommu.h     |  3 +++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index b51995b4fe90..c2e3a083ea43 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -99,8 +99,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 
 	/* Allocate a new domain and set it on device pasid. */
 	domain = iommu_sva_domain_alloc(dev, mm);
-	if (!domain) {
-		ret = -ENOMEM;
+	if (IS_ERR(domain)) {
+		ret = PTR_ERR(domain);
 		goto out_free_handle;
 	}
 
@@ -266,9 +266,15 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_domain *domain;
 
-	domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
-	if (!domain)
-		return NULL;
+	if (ops->domain_alloc_sva) {
+		domain = ops->domain_alloc_sva(dev, mm);
+		if (IS_ERR(domain))
+			return domain;
+	} else {
+		domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
+		if (!domain)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	domain->type = IOMMU_DOMAIN_SVA;
 	mmgrab(mm);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2f765ae06021..4cee5b22524c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -454,6 +454,7 @@ static inline int __iommu_copy_struct_from_user(
  *                     Upon failure, ERR_PTR must be returned.
  * @domain_alloc_paging: Allocate an iommu_domain that can be used for
  *                       UNMANAGED, DMA, and DMA_FQ domain types.
+ * @domain_alloc_sva: Allocate an iommu_domain for Shared Virtual Addressing.
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
@@ -494,6 +495,8 @@ struct iommu_ops {
 		struct device *dev, u32 flags, struct iommu_domain *parent,
 		const struct iommu_user_data *user_data);
 	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
+	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
+						 struct mm_struct *mm);
 
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
-- 
2.39.3

[PATCH 06/11] iommu/vt-d: Retire struct intel_svm

[Tina Zhang]

The sva domain is a kind of iommu_domain which allows the processor and
device to use the same virtual addresses that are managed by system
memory management. As a result, use dmar_domain to stand for a sva domain
instead of using struct intel_svm to avoid duplicating code.

Besides, allocate sva domain in domain_alloc_sva() callback which allows
the memory management notifier lifetime to follow the lifetime of the
iommu_domain.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c |   3 +-
 drivers/iommu/intel/iommu.h |  14 +--
 drivers/iommu/intel/svm.c   | 179 +++++++++++-------------------------
 3 files changed, 61 insertions(+), 135 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a81a2be9b870..847e21117e7a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3864,8 +3864,6 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
 		return domain;
 	case IOMMU_DOMAIN_IDENTITY:
 		return &si_domain->domain;
-	case IOMMU_DOMAIN_SVA:
-		return intel_svm_domain_alloc();
 	default:
 		return NULL;
 	}
@@ -4730,6 +4728,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
 	.hw_info		= intel_iommu_hw_info,
 	.domain_alloc		= intel_iommu_domain_alloc,
+	.domain_alloc_sva	= intel_svm_domain_alloc,
 	.domain_alloc_user	= intel_iommu_domain_alloc_user,
 	.probe_device		= intel_iommu_probe_device,
 	.probe_finalize		= intel_iommu_probe_finalize,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 4a2f6b80b837..f0d4a685a105 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -642,6 +642,7 @@ struct dmar_domain {
 
 	struct iommu_domain domain;	/* generic domain data structure for
 					   iommu core */
+	struct mmu_notifier notifier;
 };
 
 /*
@@ -1083,20 +1084,15 @@ int intel_svm_enable_prq(struct intel_iommu *iommu);
 int intel_svm_finish_prq(struct intel_iommu *iommu);
 int intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
 			    struct iommu_page_response *msg);
-struct iommu_domain *intel_svm_domain_alloc(void);
+struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
+					    struct mm_struct *mm);
 void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
 void intel_drain_pasid_prq(struct device *dev, u32 pasid);
-
-struct intel_svm {
-	struct mmu_notifier notifier;
-	struct mm_struct *mm;
-	u32 pasid;
-	struct list_head devs;
-};
 #else
 static inline void intel_svm_check(struct intel_iommu *iommu) {}
 static inline void intel_drain_pasid_prq(struct device *dev, u32 pasid) {}
-static inline struct iommu_domain *intel_svm_domain_alloc(void)
+static inline struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
+							  struct mm_struct *mm)
 {
 	return NULL;
 }
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 0a934866539f..8ba2872bd92f 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -26,30 +26,13 @@
 
 static irqreturn_t prq_event_thread(int irq, void *d);
 
-static DEFINE_XARRAY_ALLOC(pasid_private_array);
-static int pasid_private_add(ioasid_t pasid, void *priv)
-{
-	return xa_alloc(&pasid_private_array, &pasid, priv,
-			XA_LIMIT(pasid, pasid), GFP_ATOMIC);
-}
-
-static void pasid_private_remove(ioasid_t pasid)
-{
-	xa_erase(&pasid_private_array, pasid);
-}
-
-static void *pasid_private_find(ioasid_t pasid)
-{
-	return xa_load(&pasid_private_array, pasid);
-}
-
 static struct dev_pasid_info *
-svm_lookup_dev_pasid_info_by_dev(struct intel_svm *svm, struct device *dev)
+domain_lookup_dev_pasid_info_by_dev(struct dmar_domain *domain, struct device *dev)
 {
 	struct dev_pasid_info *dev_pasid = NULL, *t;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(t, &svm->devs, link_domain) {
+	list_for_each_entry_rcu(t, &domain->dev_pasids, link_domain) {
 		if (t->dev == dev) {
 			dev_pasid = t;
 			break;
@@ -168,27 +151,28 @@ void intel_svm_check(struct intel_iommu *iommu)
 	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
 }
 
-static void __flush_svm_range_dev(struct intel_svm *svm,
+static void __flush_svm_range_dev(struct dmar_domain *domain,
 				  struct dev_pasid_info *dev_pasid,
 				  unsigned long address,
 				  unsigned long pages, int ih)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid->dev);
+	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
 
 	if (WARN_ON(!pages))
 		return;
 
-	qi_flush_piotlb(info->iommu, dev_pasid->did, svm->pasid, address, pages, ih);
+	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
 	if (info->ats_enabled) {
 		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
-					 svm->pasid, dev_pasid->qdep, address,
+					 pasid, dev_pasid->qdep, address,
 					 order_base_2(pages));
 		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
-					  svm->pasid, dev_pasid->qdep);
+					  pasid, dev_pasid->qdep);
 	}
 }
 
-static void intel_flush_svm_range_dev(struct intel_svm *svm,
+static void intel_flush_svm_range_dev(struct dmar_domain *domain,
 				      struct dev_pasid_info *dev_pasid,
 				      unsigned long address,
 				      unsigned long pages, int ih)
@@ -199,38 +183,39 @@ static void intel_flush_svm_range_dev(struct intel_svm *svm,
 	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
 
 	while (start < end) {
-		__flush_svm_range_dev(svm, dev_pasid, start, align >> VTD_PAGE_SHIFT, ih);
+		__flush_svm_range_dev(domain, dev_pasid, start, align >> VTD_PAGE_SHIFT, ih);
 		start += align;
 	}
 }
 
-static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,
+static void intel_flush_svm_range(struct dmar_domain *domain, unsigned long address,
 				unsigned long pages, int ih)
 {
 	struct dev_pasid_info *dev_pasid;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain)
-		intel_flush_svm_range_dev(svm, dev_pasid, address, pages, ih);
+	list_for_each_entry_rcu(dev_pasid, &domain->dev_pasids, link_domain)
+		intel_flush_svm_range_dev(domain, dev_pasid, address, pages, ih);
 	rcu_read_unlock();
 }
 
-static void intel_flush_svm_all(struct intel_svm *svm)
+static void intel_flush_svm_all(struct dmar_domain *domain)
 {
 	struct device_domain_info *info;
 	struct dev_pasid_info *dev_pasid = NULL;
+	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain) {
+	list_for_each_entry_rcu(dev_pasid, &domain->dev_pasids, link_domain) {
 		info = dev_iommu_priv_get(dev_pasid->dev);
 
-		qi_flush_piotlb(info->iommu, dev_pasid->did, svm->pasid, 0, -1UL, 0);
+		qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, 0, -1UL, 0);
 		if (info->ats_enabled) {
 			qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
-						 svm->pasid, dev_pasid->qdep,
+						 pasid, dev_pasid->qdep,
 						 0, 64 - VTD_PAGE_SHIFT);
 			quirk_extra_dev_tlb_flush(info, 0, 64 - VTD_PAGE_SHIFT,
-						  svm->pasid, dev_pasid->qdep);
+						  pasid, dev_pasid->qdep);
 		}
 	}
 	rcu_read_unlock();
@@ -241,20 +226,20 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
 					struct mm_struct *mm,
 					unsigned long start, unsigned long end)
 {
-	struct intel_svm *svm = container_of(mn, struct intel_svm, notifier);
+	struct dmar_domain *domain = container_of(mn, struct dmar_domain, notifier);
 
 	if (start == 0 && end == -1UL) {
-		intel_flush_svm_all(svm);
+		intel_flush_svm_all(domain);
 		return;
 	}
 
-	intel_flush_svm_range(svm, start,
+	intel_flush_svm_range(domain, start,
 			      (end - start + PAGE_SIZE - 1) >> VTD_PAGE_SHIFT, 0);
 }
 
 static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
-	struct intel_svm *svm = container_of(mn, struct intel_svm, notifier);
+	struct dmar_domain *domain = container_of(mn, struct dmar_domain, notifier);
 	struct device_domain_info *info;
 	struct dev_pasid_info *dev_pasid;
 
@@ -271,10 +256,10 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * *has* to handle gracefully without affecting other processes.
 	 */
 	rcu_read_lock();
-	list_for_each_entry_rcu(dev_pasid, &svm->devs, link_domain) {
+	list_for_each_entry_rcu(dev_pasid, &domain->dev_pasids, link_domain) {
 		info = dev_iommu_priv_get(dev_pasid->dev);
 		intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
-					    svm->pasid, true);
+					    mm_get_enqcmd_pasid(mm), true);
 	}
 	rcu_read_unlock();
 
@@ -285,78 +270,21 @@ static const struct mmu_notifier_ops intel_mmuops = {
 	.arch_invalidate_secondary_tlbs = intel_arch_invalidate_secondary_tlbs,
 };
 
-static int pasid_to_dev_pasid_info(struct device *dev, unsigned int pasid,
-			     struct intel_svm **rsvm,
-			     struct dev_pasid_info **rsdev_pasid_info)
-{
-	struct dev_pasid_info *dev_pasid = NULL;
-	struct intel_svm *svm;
-
-	if (pasid == IOMMU_PASID_INVALID || pasid >= PASID_MAX)
-		return -EINVAL;
-
-	svm = pasid_private_find(pasid);
-	if (IS_ERR(svm))
-		return PTR_ERR(svm);
-
-	if (!svm)
-		goto out;
-
-	/*
-	 * If we found svm for the PASID, there must be at least one device
-	 * bond.
-	 */
-	if (WARN_ON(list_empty(&svm->devs)))
-		return -EINVAL;
-	dev_pasid = svm_lookup_dev_pasid_info_by_dev(svm, dev);
-
-out:
-	*rsvm = svm;
-	*rsdev_pasid_info = dev_pasid;
-
-	return 0;
-}
-
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t pasid)
 {
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 	struct mm_struct *mm = domain->mm;
 	struct dev_pasid_info *dev_pasid;
-	struct intel_svm *svm;
 	unsigned long sflags;
 	int ret = 0;
 
-	svm = pasid_private_find(pasid);
-	if (!svm) {
-		svm = kzalloc(sizeof(*svm), GFP_KERNEL);
-		if (!svm)
-			return -ENOMEM;
-
-		svm->pasid = pasid;
-		svm->mm = mm;
-		INIT_LIST_HEAD_RCU(&svm->devs);
-
-		svm->notifier.ops = &intel_mmuops;
-		ret = mmu_notifier_register(&svm->notifier, mm);
-		if (ret) {
-			kfree(svm);
-			return ret;
-		}
-
-		ret = pasid_private_add(svm->pasid, svm);
-		if (ret) {
-			mmu_notifier_unregister(&svm->notifier, mm);
-			kfree(svm);
-			return ret;
-		}
-	}
-
 	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
 	if (!dev_pasid) {
 		ret = -ENOMEM;
-		goto free_svm;
+		goto out;
 	}
 
 	dev_pasid->dev = dev;
@@ -372,43 +300,30 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
 	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, pasid,
 					    dev_pasid->did, sflags);
-	if (ret)
-		goto free_dev_pasid;
-
-	list_add_rcu(&dev_pasid->link_domain, &svm->devs);
-
-	return 0;
-
-free_dev_pasid:
-	kfree(dev_pasid);
-free_svm:
-	if (list_empty(&svm->devs)) {
-		mmu_notifier_unregister(&svm->notifier, mm);
-		pasid_private_remove(pasid);
-		kfree(svm);
+	if (ret) {
+		kfree(dev_pasid);
+		goto out;
 	}
 
+	list_add_rcu(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+out:
 	return ret;
 }
 
 void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
 {
+	struct iommu_domain *domain;
 	struct dev_pasid_info *dev_pasid;
-	struct intel_svm *svm;
 
-	if (pasid_to_dev_pasid_info(dev, pasid, &svm, &dev_pasid))
+	domain = iommu_get_domain_for_dev_pasid(dev, pasid,
+						IOMMU_DOMAIN_SVA);
+	if (WARN_ON_ONCE(IS_ERR_OR_NULL(domain)))
 		return;
 
+	dev_pasid = domain_lookup_dev_pasid_info_by_dev(to_dmar_domain(domain), dev);
 	if (dev_pasid) {
 		list_del_rcu(&dev_pasid->link_domain);
 		kfree_rcu(dev_pasid, rcu);
-
-		if (list_empty(&svm->devs)) {
-			if (svm->notifier.ops)
-				mmu_notifier_unregister(&svm->notifier, svm->mm);
-			pasid_private_remove(svm->pasid);
-			kfree(svm);
-		}
 	}
 }
 
@@ -797,7 +712,12 @@ int intel_svm_page_response(struct device *dev,
 
 static void intel_svm_domain_free(struct iommu_domain *domain)
 {
-	kfree(to_dmar_domain(domain));
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+
+	if (dmar_domain->notifier.ops)
+		mmu_notifier_unregister(&dmar_domain->notifier, domain->mm);
+
+	kfree(dmar_domain);
 }
 
 static const struct iommu_domain_ops intel_svm_domain_ops = {
@@ -805,14 +725,25 @@ static const struct iommu_domain_ops intel_svm_domain_ops = {
 	.free			= intel_svm_domain_free
 };
 
-struct iommu_domain *intel_svm_domain_alloc(void)
+struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
+						  struct mm_struct *mm)
 {
 	struct dmar_domain *domain;
+	int ret;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
+
 	domain->domain.ops = &intel_svm_domain_ops;
+	INIT_LIST_HEAD(&domain->dev_pasids);
+
+	domain->notifier.ops = &intel_mmuops;
+	ret = mmu_notifier_register(&domain->notifier, mm);
+	if (ret) {
+		kfree(domain);
+		return ERR_PTR(ret);
+	}
 
 	return &domain->domain;
 }
-- 
2.39.3

[PATCH 07/11] iommu/vt-d: Use RCU for dev_pasids list updates in set/remove_dev_pasid()

[Tina Zhang]

Extend intel_iommu_remove_dev_pasid() and intel_iommu_set_dev_pasid() to
support updating dev_pasids list concurrently with readers.

For default domain operations, the dev_pasids list accesses are protected
by domain->lock and therefore all read/write accesses of default domain
operations to dev_pasids list are performed sequentially. However, for sva
domain, the dev_pasids list accesses could be performed concurrently.
For example, the callbacks invoked by memory management notifier may run
in a process which runs concurrently with another process wherein the
intel_iommu_set/remove_dev_pasid operations are performed.

To extend intel_iommu_set/remove_dev_pasid() to have the ability to update
the dev_pasids list concurrently with multiple readers (which is required
by sva domain), RCU mechanism is being used here.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 847e21117e7a..0eec5d971315 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4553,7 +4553,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
 		if (curr->dev == dev && curr->pasid == pasid) {
-			list_del(&curr->link_domain);
+			list_del_rcu(&curr->link_domain);
 			dev_pasid = curr;
 			break;
 		}
@@ -4563,7 +4563,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
-	kfree(dev_pasid);
+	kfree_rcu(dev_pasid, rcu);
 out_tear_down:
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
 	intel_drain_pasid_prq(dev, pasid);
@@ -4613,8 +4613,14 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 
 	dev_pasid->dev = dev;
 	dev_pasid->pasid = pasid;
+
+	/*
+	 * Spin lock protects dev_pasids list from being updated concurrently with
+	 * multiple updaters, while rcu ensures concurrency between one updater
+	 * and multiple readers
+	 */
 	spin_lock_irqsave(&dmar_domain->lock, flags);
-	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+	list_add_rcu(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
 	spin_unlock_irqrestore(&dmar_domain->lock, flags);
 
 	if (domain->type & __IOMMU_DOMAIN_PAGING)
-- 
2.39.3

Re: [PATCH 07/11] iommu/vt-d: Use RCU for dev_pasids list updates in set/remove_dev_pasid()

[Baolu Lu]

On 2024/1/16 9:11, Tina Zhang wrote:
> Extend intel_iommu_remove_dev_pasid() and intel_iommu_set_dev_pasid() to
> support updating dev_pasids list concurrently with readers.
> 
> For default domain operations, the dev_pasids list accesses are protected
> by domain->lock and therefore all read/write accesses of default domain
> operations to dev_pasids list are performed sequentially. However, for sva
> domain, the dev_pasids list accesses could be performed concurrently.
> For example, the callbacks invoked by memory management notifier may run
> in a process which runs concurrently with another process wherein the
> intel_iommu_set/remove_dev_pasid operations are performed.
> 
> To extend intel_iommu_set/remove_dev_pasid() to have the ability to update
> the dev_pasids list concurrently with multiple readers (which is required
> by sva domain), RCU mechanism is being used here.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 847e21117e7a..0eec5d971315 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4553,7 +4553,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	spin_lock_irqsave(&dmar_domain->lock, flags);
>   	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>   		if (curr->dev == dev && curr->pasid == pasid) {
> -			list_del(&curr->link_domain);
> +			list_del_rcu(&curr->link_domain);
>   			dev_pasid = curr;
>   			break;
>   		}
> @@ -4563,7 +4563,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   
>   	domain_detach_iommu(dmar_domain, iommu);
>   	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> -	kfree(dev_pasid);
> +	kfree_rcu(dev_pasid, rcu);
>   out_tear_down:
>   	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>   	intel_drain_pasid_prq(dev, pasid);
> @@ -4613,8 +4613,14 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   
>   	dev_pasid->dev = dev;
>   	dev_pasid->pasid = pasid;
> +
> +	/*
> +	 * Spin lock protects dev_pasids list from being updated concurrently with
> +	 * multiple updaters, while rcu ensures concurrency between one updater
> +	 * and multiple readers
> +	 */
>   	spin_lock_irqsave(&dmar_domain->lock, flags);
> -	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> +	list_add_rcu(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
>   	spin_unlock_irqrestore(&dmar_domain->lock, flags);
>   
>   	if (domain->type & __IOMMU_DOMAIN_PAGING)

It appears you're using an RCU lock within another spinlock critical
region. Wouldn't that be redundant?

Best regards,
baolu

[PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper function

[Tina Zhang]

Introduce a helper function to tell if the type of a dmar_domain is sva
or not. In general, comparing with normal iommu_domain, a sva domain
shouldn't be special other than having a different source for the page
table. Thus, this helper function should be used as little as possible
(i.e. only in the cases related to handling page table source).

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 0eec5d971315..af466ba301b7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -244,6 +244,11 @@ static int domain_type_is_si(struct dmar_domain *domain)
 	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
 }
 
+static inline bool domain_type_is_sva(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_SVA;
+}
+
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -4544,7 +4549,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	 * notification. Before consolidating that code into iommu core, let
 	 * the intel sva code handle it.
 	 */
-	if (domain->type == IOMMU_DOMAIN_SVA) {
+	if (domain_type_is_sva(dmar_domain)) {
 		intel_svm_remove_dev_pasid(dev, pasid);
 		goto out_tear_down;
 	}
-- 
2.39.3

Re: [PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper function

[Baolu Lu]

On 2024/1/16 9:11, Tina Zhang wrote:
> Introduce a helper function to tell if the type of a dmar_domain is sva
> or not. In general, comparing with normal iommu_domain, a sva domain
> shouldn't be special other than having a different source for the page
> table. Thus, this helper function should be used as little as possible
> (i.e. only in the cases related to handling page table source).
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 0eec5d971315..af466ba301b7 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -244,6 +244,11 @@ static int domain_type_is_si(struct dmar_domain *domain)
>   	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
>   }
>   
> +static inline bool domain_type_is_sva(struct dmar_domain *domain)
> +{
> +	return domain->domain.type == IOMMU_DOMAIN_SVA;
> +}

Move the helper into the header file and put appropriate comments around
it.

Best regards,
baolu

RE: [PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper function

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, January 17, 2024 3:33 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper
> function
> 
> On 2024/1/16 9:11, Tina Zhang wrote:
> > Introduce a helper function to tell if the type of a dmar_domain is
> > sva or not. In general, comparing with normal iommu_domain, a sva
> > domain shouldn't be special other than having a different source for
> > the page table. Thus, this helper function should be used as little as
> > possible (i.e. only in the cases related to handling page table source).
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 0eec5d971315..af466ba301b7 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -244,6 +244,11 @@ static int domain_type_is_si(struct dmar_domain
> *domain)
> >   	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
> >   }
> >
> > +static inline bool domain_type_is_sva(struct dmar_domain *domain) {
> > +	return domain->domain.type == IOMMU_DOMAIN_SVA; }
> 
> Move the helper into the header file and put appropriate comments around it.
Is this domain_type_is_sva() helper expected to be reused by others?

Regards,
-Tina
> 
> Best regards,
> baolu

Re: [PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper function

[Baolu Lu]

On 2/19/24 8:13 AM, Zhang, Tina wrote:
> Hi Baolu,
> 
>> -----Original Message-----
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, January 17, 2024 3:33 PM
>> To: Zhang, Tina<tina.zhang@intel.com>;iommu@lists.linux.dev
>> Cc:baolu.lu@linux.intel.com; Tian, Kevin<kevin.tian@intel.com>
>> Subject: Re: [PATCH 08/11] iommu/vt-d: Add domain_type_is_sva helper
>> function
>>
>> On 2024/1/16 9:11, Tina Zhang wrote:
>>> Introduce a helper function to tell if the type of a dmar_domain is
>>> sva or not. In general, comparing with normal iommu_domain, a sva
>>> domain shouldn't be special other than having a different source for
>>> the page table. Thus, this helper function should be used as little as
>>> possible (i.e. only in the cases related to handling page table source).
>>>
>>> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
>>> ---
>>>    drivers/iommu/intel/iommu.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 0eec5d971315..af466ba301b7 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -244,6 +244,11 @@ static int domain_type_is_si(struct dmar_domain
>> *domain)
>>>    	return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
>>>    }
>>>
>>> +static inline bool domain_type_is_sva(struct dmar_domain *domain) {
>>> +	return domain->domain.type == IOMMU_DOMAIN_SVA; }
>> Move the helper into the header file and put appropriate comments around it.
> Is this domain_type_is_sva() helper expected to be reused by others?

I am not sure. But let's put inline helpers in the header instead of C
file.

Best regards,
baolu

[PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid function

[Tina Zhang]

Refactor intel_iommu_set_dev_pasid() and make it be reused by sva domain
as the set_dev_pasid operation.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 43 ++++++++++++++++++++++++++-----------
 drivers/iommu/intel/iommu.h |  2 ++
 drivers/iommu/intel/svm.c   | 42 +-----------------------------------
 3 files changed, 33 insertions(+), 54 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index af466ba301b7..8d1b9de78a43 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4574,7 +4574,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	intel_drain_pasid_prq(dev, pasid);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 				     struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
@@ -4593,31 +4593,48 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (context_copied(iommu, info->bus, info->devfn))
 		return -EBUSY;
 
-	ret = prepare_domain_attach_device(domain, dev);
-	if (ret)
-		return ret;
-
 	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
 	if (!dev_pasid)
 		return -ENOMEM;
 
-	ret = domain_attach_iommu(dmar_domain, iommu);
-	if (ret)
-		goto out_free;
+	dev_pasid->dev = dev;
+	dev_pasid->pasid = pasid;
+	if (domain_type_is_sva(dmar_domain)) {
+		dev_pasid->did = FLPT_DEFAULT_DID;
+		dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
+		if (info->ats_enabled) {
+			dev_pasid->qdep = info->ats_qdep;
+			if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
+				dev_pasid->qdep = 0;
+		}
+	} else {
+		ret = prepare_domain_attach_device(domain, dev);
+		if (ret)
+			goto out_free;
+
+		ret = domain_attach_iommu(dmar_domain, iommu);
+		if (ret)
+			goto out_free;
+	}
 
 	if (domain_type_is_si(dmar_domain))
 		ret = intel_pasid_setup_pass_through(iommu, dev, pasid);
 	else if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
+	else if (domain_type_is_sva(dmar_domain))
+		ret = intel_pasid_setup_first_level(iommu, dev,
+			domain->mm->pgd, pasid, dev_pasid->did,
+			cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0);
 	else
 		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
 						     dev, pasid);
-	if (ret)
-		goto out_detach_iommu;
-
-	dev_pasid->dev = dev;
-	dev_pasid->pasid = pasid;
+	if (ret) {
+		if (domain_type_is_sva(dmar_domain))
+			goto out_free;
+		else
+			goto out_detach_iommu;
+	}
 
 	/*
 	 * Spin lock protects dev_pasids list from being updated concurrently with
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index f0d4a685a105..ebc8edc7ea8a 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1077,6 +1077,8 @@ void free_pgtable_page(void *vaddr);
 void iommu_flush_write_buffer(struct intel_iommu *iommu);
 struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
 					       const struct iommu_user_data *user_data);
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid);
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 void intel_svm_check(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 8ba2872bd92f..1648c31d6259 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -270,46 +270,6 @@ static const struct mmu_notifier_ops intel_mmuops = {
 	.arch_invalidate_secondary_tlbs = intel_arch_invalidate_secondary_tlbs,
 };
 
-static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
-				   struct device *dev, ioasid_t pasid)
-{
-	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu = info->iommu;
-	struct mm_struct *mm = domain->mm;
-	struct dev_pasid_info *dev_pasid;
-	unsigned long sflags;
-	int ret = 0;
-
-	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
-	if (!dev_pasid) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	dev_pasid->dev = dev;
-	dev_pasid->did = FLPT_DEFAULT_DID;
-	dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
-	if (info->ats_enabled) {
-		dev_pasid->qdep = info->ats_qdep;
-		if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
-			dev_pasid->qdep = 0;
-	}
-
-	/* Setup the pasid table: */
-	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
-	ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, pasid,
-					    dev_pasid->did, sflags);
-	if (ret) {
-		kfree(dev_pasid);
-		goto out;
-	}
-
-	list_add_rcu(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
-out:
-	return ret;
-}
-
 void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
 {
 	struct iommu_domain *domain;
@@ -721,7 +681,7 @@ static void intel_svm_domain_free(struct iommu_domain *domain)
 }
 
 static const struct iommu_domain_ops intel_svm_domain_ops = {
-	.set_dev_pasid		= intel_svm_set_dev_pasid,
+	.set_dev_pasid		= intel_iommu_set_dev_pasid,
 	.free			= intel_svm_domain_free
 };
 
-- 
2.39.3

Re: [PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid function

[Baolu Lu]

On 2024/1/16 9:11, Tina Zhang wrote:
> Refactor intel_iommu_set_dev_pasid() and make it be reused by sva domain
> as the set_dev_pasid operation.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 43 ++++++++++++++++++++++++++-----------
>   drivers/iommu/intel/iommu.h |  2 ++
>   drivers/iommu/intel/svm.c   | 42 +-----------------------------------
>   3 files changed, 33 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index af466ba301b7..8d1b9de78a43 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4574,7 +4574,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	intel_drain_pasid_prq(dev, pasid);
>   }
>   
> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> +int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   				     struct device *dev, ioasid_t pasid)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
> @@ -4593,31 +4593,48 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   	if (context_copied(iommu, info->bus, info->devfn))
>   		return -EBUSY;
>   
> -	ret = prepare_domain_attach_device(domain, dev);
> -	if (ret)
> -		return ret;
> -
>   	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>   	if (!dev_pasid)
>   		return -ENOMEM;
>   
> -	ret = domain_attach_iommu(dmar_domain, iommu);
> -	if (ret)
> -		goto out_free;
> +	dev_pasid->dev = dev;
> +	dev_pasid->pasid = pasid;
> +	if (domain_type_is_sva(dmar_domain)) {
> +		dev_pasid->did = FLPT_DEFAULT_DID;
> +		dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
> +		if (info->ats_enabled) {
> +			dev_pasid->qdep = info->ats_qdep;
> +			if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
> +				dev_pasid->qdep = 0;
> +		}

This is generic device information. There's no need to do it only for
the SVA domain; otherwise, you'll have to check the domain type every
time you use these fields.

> +	} else {
> +		ret = prepare_domain_attach_device(domain, dev);

The prepare_domain_attach_device() function is called to check whether
the device is compatible with the domain to be attached. You could make
it work for the SVA domain as well, rather than ignoring it.

> +		if (ret)
> +			goto out_free;
> +
> +		ret = domain_attach_iommu(dmar_domain, iommu);

The same here.

The domain_attach_iommu() is called to setup the relationship between a
domain and an iommu. You should make it compatible with sva domain as
well.

Best regards,
baolu

RE: [PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid function

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, January 17, 2024 3:40 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid
> function
> 
> On 2024/1/16 9:11, Tina Zhang wrote:
> > Refactor intel_iommu_set_dev_pasid() and make it be reused by sva
> > domain as the set_dev_pasid operation.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 43 ++++++++++++++++++++++++++--------
> ---
> >   drivers/iommu/intel/iommu.h |  2 ++
> >   drivers/iommu/intel/svm.c   | 42 +-----------------------------------
> >   3 files changed, 33 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index af466ba301b7..8d1b9de78a43 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4574,7 +4574,7 @@ static void
> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >   	intel_drain_pasid_prq(dev, pasid);
> >   }
> >
> > -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> > +int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> >   				     struct device *dev, ioasid_t pasid)
> >   {
> >   	struct device_domain_info *info = dev_iommu_priv_get(dev); @@
> > -4593,31 +4593,48 @@ static int intel_iommu_set_dev_pasid(struct
> iommu_domain *domain,
> >   	if (context_copied(iommu, info->bus, info->devfn))
> >   		return -EBUSY;
> >
> > -	ret = prepare_domain_attach_device(domain, dev);
> > -	if (ret)
> > -		return ret;
> > -
> >   	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> >   	if (!dev_pasid)
> >   		return -ENOMEM;
> >
> > -	ret = domain_attach_iommu(dmar_domain, iommu);
> > -	if (ret)
> > -		goto out_free;
> > +	dev_pasid->dev = dev;
> > +	dev_pasid->pasid = pasid;
> > +	if (domain_type_is_sva(dmar_domain)) {
> > +		dev_pasid->did = FLPT_DEFAULT_DID;
> > +		dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
> > +		if (info->ats_enabled) {
> > +			dev_pasid->qdep = info->ats_qdep;
> > +			if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
> > +				dev_pasid->qdep = 0;
> > +		}
> 
> This is generic device information. There's no need to do it only for the SVA
> domain; otherwise, you'll have to check the domain type every time you use
> these fields.
Right.

> 
> > +	} else {
> > +		ret = prepare_domain_attach_device(domain, dev);
> 
> The prepare_domain_attach_device() function is called to check whether the
> device is compatible with the domain to be attached. You could make it work
> for the SVA domain as well, rather than ignoring it.
OK.

> 
> > +		if (ret)
> > +			goto out_free;
> > +
> > +		ret = domain_attach_iommu(dmar_domain, iommu);
> 
> The same here.
> 
> The domain_attach_iommu() is called to setup the relationship between a
> domain and an iommu. You should make it compatible with sva domain as
> well.
The problem is whether we need struct iommu_domain_info for sva domain. I didn't see any necessary to allocate struct iommu_domain_info instance for sva domain, as all the fields of struct iommu_domain_info can be gotten from dev_pasids.

Since we want to make the domain_attach_iommu() generic (i.e., all domains, no matter what type they are, need to call this function), I think we can check the domain type within the function and add comments there to tell that we didn't see any necessary to let sva domain have a struct iommu_domain_info and therefore the function returns immediately if it meets a sva domain. What do you think?

Regards,
-Tina

> 
> Best regards,
> baolu

Re: [PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid function

[Baolu Lu]

On 2/19/24 9:01 AM, Zhang, Tina wrote:
>>> +		if (ret)
>>> +			goto out_free;
>>> +
>>> +		ret = domain_attach_iommu(dmar_domain, iommu);
>> The same here.
>>
>> The domain_attach_iommu() is called to setup the relationship between a
>> domain and an iommu. You should make it compatible with sva domain as
>> well.
> The problem is whether we need struct iommu_domain_info for sva domain. I didn't see any necessary to allocate struct iommu_domain_info instance for sva domain, as all the fields of struct iommu_domain_info can be gotten from dev_pasids.
> 
> Since we want to make the domain_attach_iommu() generic (i.e., all domains, no matter what type they are, need to call this function), I think we can check the domain type within the function and add comments there to tell that we didn't see any necessary to let sva domain have a struct iommu_domain_info and therefore the function returns immediately if it meets a sva domain. What do you think?

The SVA domain is nothing special compared to other types of domains.
The ultimate goal is that we should have a common attach-detach
implementation for all.

The iommu_domain_info structure stores the relationship between a domain
and IOMMU, mainly for the cache tag information, also known as the
domain ID. It's a bit outdated if we consider SVA and nested domains. As
I told in another thread, I am working on a solution to address
this. Let's sync our efforts to avoid duplication of efforts.

Best regards,
baolu

[PATCH 10/11] iommu/vt-d: Retire intel_svm_remove_dev_pasid function

[Tina Zhang]

IOMMU core has added a dedicated callback for allocating sva domain.
So intel driver doesn't need intel_svm_remove_dev_pasid function to handle
mm notification any more.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 15 ++++-----------
 drivers/iommu/intel/iommu.h |  4 ----
 drivers/iommu/intel/svm.c   | 34 ----------------------------------
 3 files changed, 4 insertions(+), 49 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8d1b9de78a43..92af368c37a7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4541,19 +4541,9 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	unsigned long flags;
 
 	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
-	if (WARN_ON_ONCE(!domain))
+	if (WARN_ON_ONCE(IS_ERR_OR_NULL(domain)))
 		goto out_tear_down;
 
-	/*
-	 * The SVA implementation needs to handle its own stuffs like the mm
-	 * notification. Before consolidating that code into iommu core, let
-	 * the intel sva code handle it.
-	 */
-	if (domain_type_is_sva(dmar_domain)) {
-		intel_svm_remove_dev_pasid(dev, pasid);
-		goto out_tear_down;
-	}
-
 	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
@@ -4566,6 +4556,9 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	WARN_ON_ONCE(!dev_pasid);
 	spin_unlock_irqrestore(&dmar_domain->lock, flags);
 
+	if (domain_type_is_sva(dmar_domain))
+		goto out_tear_down;
+
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree_rcu(dev_pasid, rcu);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index ebc8edc7ea8a..243474dfaf1d 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1088,7 +1088,6 @@ int intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
 			    struct iommu_page_response *msg);
 struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
 					    struct mm_struct *mm);
-void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
 void intel_drain_pasid_prq(struct device *dev, u32 pasid);
 #else
 static inline void intel_svm_check(struct intel_iommu *iommu) {}
@@ -1099,9 +1098,6 @@ static inline struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
 	return NULL;
 }
 
-static inline void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid)
-{
-}
 #endif
 
 #ifdef CONFIG_INTEL_IOMMU_DEBUGFS
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 1648c31d6259..79d1f3107847 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -26,23 +26,6 @@
 
 static irqreturn_t prq_event_thread(int irq, void *d);
 
-static struct dev_pasid_info *
-domain_lookup_dev_pasid_info_by_dev(struct dmar_domain *domain, struct device *dev)
-{
-	struct dev_pasid_info *dev_pasid = NULL, *t;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(t, &domain->dev_pasids, link_domain) {
-		if (t->dev == dev) {
-			dev_pasid = t;
-			break;
-		}
-	}
-	rcu_read_unlock();
-
-	return dev_pasid;
-}
-
 int intel_svm_enable_prq(struct intel_iommu *iommu)
 {
 	struct iopf_queue *iopfq;
@@ -270,23 +253,6 @@ static const struct mmu_notifier_ops intel_mmuops = {
 	.arch_invalidate_secondary_tlbs = intel_arch_invalidate_secondary_tlbs,
 };
 
-void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
-{
-	struct iommu_domain *domain;
-	struct dev_pasid_info *dev_pasid;
-
-	domain = iommu_get_domain_for_dev_pasid(dev, pasid,
-						IOMMU_DOMAIN_SVA);
-	if (WARN_ON_ONCE(IS_ERR_OR_NULL(domain)))
-		return;
-
-	dev_pasid = domain_lookup_dev_pasid_info_by_dev(to_dmar_domain(domain), dev);
-	if (dev_pasid) {
-		list_del_rcu(&dev_pasid->link_domain);
-		kfree_rcu(dev_pasid, rcu);
-	}
-}
-
 /* Page request queue descriptor */
 struct page_req_dsc {
 	union {
-- 
2.39.3

[PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Tina Zhang]

Devices behind different IOMMUs can be bound to one sva domain. When a
range of a sva domain address is being invalidated, VT-d driver needs to
issue IOMMU IOTLB and Dev-IOTLB invalidation commands to ask IOMMU
hardware and related devices to invalidate their caches.

The current logic issues both IOTLB invalidation command and device-TLB
command per device, which leads to superfluous IOTLB invalidation (e.g.,
if there are four devices behind a IOMMU are attached to one sva domain.
In the current logic, during handing intel_invalidate_range(), four IOTLB
invalidation commands and four Dev-IOTLB invalidation commands will be
issued. However, only one IOTLB invalidation command and four Dev-IOTLB
invalidation command are necessary.), and therefore impacts run-time
performance.

The patch removes the redundant IOMMU IOTLB invalidations by allowing
issuing IOMMU IOTLB invalidation command per iommu instead of per device.

Suggested-by: Sanjay Kumar <sanjay.k.kumar@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/svm.c | 53 ++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 79d1f3107847..bf4962eb229c 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu *iommu)
 	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
 }
 
-static void __flush_svm_range_dev(struct dmar_domain *domain,
-				  struct dev_pasid_info *dev_pasid,
+static void __flush_svm_range(struct iommu_domain *domain,
 				  unsigned long address,
 				  unsigned long pages, int ih)
 {
-	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid->dev);
-	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
+	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
+	struct device_domain_info *dev_info;
+	struct iommu_domain_info *iommu_info;
+	struct dev_pasid_info *dev_pasid;
+	unsigned long idx;
 
 	if (WARN_ON(!pages))
 		return;
 
-	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
-	if (info->ats_enabled) {
-		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
-					 pasid, dev_pasid->qdep, address,
-					 order_base_2(pages));
-		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
-					  pasid, dev_pasid->qdep);
+	rcu_read_lock();
+	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx, iommu_info)
+		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
+				pasid, address, pages, ih);
+
+	list_for_each_entry_rcu(dev_pasid, &to_dmar_domain(domain)->dev_pasids, link_domain) {
+		dev_info = dev_iommu_priv_get(dev_pasid->dev);
+		if (dev_info->ats_enabled) {
+			qi_flush_dev_iotlb_pasid(dev_info->iommu, dev_pasid->sid, dev_info->pfsid,
+						 pasid, dev_pasid->qdep, address,
+						 order_base_2(pages));
+			quirk_extra_dev_tlb_flush(dev_info, address, order_base_2(pages),
+						  pasid, dev_pasid->qdep);
+		}
 	}
+	rcu_read_unlock();
 }
 
-static void intel_flush_svm_range_dev(struct dmar_domain *domain,
-				      struct dev_pasid_info *dev_pasid,
-				      unsigned long address,
-				      unsigned long pages, int ih)
+static void intel_flush_svm_range(struct iommu_domain *domain, unsigned long address,
+				unsigned long pages, int ih)
 {
 	unsigned long shift = ilog2(__roundup_pow_of_two(pages));
 	unsigned long align = (1ULL << (VTD_PAGE_SHIFT + shift));
@@ -166,22 +174,11 @@ static void intel_flush_svm_range_dev(struct dmar_domain *domain,
 	unsigned long end = ALIGN(address + (pages << VTD_PAGE_SHIFT), align);
 
 	while (start < end) {
-		__flush_svm_range_dev(domain, dev_pasid, start, align >> VTD_PAGE_SHIFT, ih);
+		__flush_svm_range(domain, start, align >> VTD_PAGE_SHIFT, ih);
 		start += align;
 	}
 }
 
-static void intel_flush_svm_range(struct dmar_domain *domain, unsigned long address,
-				unsigned long pages, int ih)
-{
-	struct dev_pasid_info *dev_pasid;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(dev_pasid, &domain->dev_pasids, link_domain)
-		intel_flush_svm_range_dev(domain, dev_pasid, address, pages, ih);
-	rcu_read_unlock();
-}
-
 static void intel_flush_svm_all(struct dmar_domain *domain)
 {
 	struct device_domain_info *info;
@@ -216,7 +213,7 @@ static void intel_arch_invalidate_secondary_tlbs(struct mmu_notifier *mn,
 		return;
 	}
 
-	intel_flush_svm_range(domain, start,
+	intel_flush_svm_range(&domain->domain, start,
 			      (end - start + PAGE_SIZE - 1) >> VTD_PAGE_SHIFT, 0);
 }
 
-- 
2.39.3

Re: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Baolu Lu]

On 2024/1/16 9:11, Tina Zhang wrote:
> Devices behind different IOMMUs can be bound to one sva domain. When a
> range of a sva domain address is being invalidated, VT-d driver needs to
> issue IOMMU IOTLB and Dev-IOTLB invalidation commands to ask IOMMU
> hardware and related devices to invalidate their caches.
> 
> The current logic issues both IOTLB invalidation command and device-TLB
> command per device, which leads to superfluous IOTLB invalidation (e.g.,
> if there are four devices behind a IOMMU are attached to one sva domain.
> In the current logic, during handing intel_invalidate_range(), four IOTLB
> invalidation commands and four Dev-IOTLB invalidation commands will be
> issued. However, only one IOTLB invalidation command and four Dev-IOTLB
> invalidation command are necessary.), and therefore impacts run-time
> performance.
> 
> The patch removes the redundant IOMMU IOTLB invalidations by allowing
> issuing IOMMU IOTLB invalidation command per iommu instead of per device.
> 
> Suggested-by: Sanjay Kumar<sanjay.k.kumar@intel.com>
> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/svm.c | 53 ++++++++++++++++++---------------------
>   1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 79d1f3107847..bf4962eb229c 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu *iommu)
>   	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>   }
>   
> -static void __flush_svm_range_dev(struct dmar_domain *domain,
> -				  struct dev_pasid_info *dev_pasid,
> +static void __flush_svm_range(struct iommu_domain *domain,
>   				  unsigned long address,
>   				  unsigned long pages, int ih)
>   {
> -	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid->dev);
> -	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
> +	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
> +	struct device_domain_info *dev_info;
> +	struct iommu_domain_info *iommu_info;
> +	struct dev_pasid_info *dev_pasid;
> +	unsigned long idx;
>   
>   	if (WARN_ON(!pages))
>   		return;
>   
> -	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
> -	if (info->ats_enabled) {
> -		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info->pfsid,
> -					 pasid, dev_pasid->qdep, address,
> -					 order_base_2(pages));
> -		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
> -					  pasid, dev_pasid->qdep);
> +	rcu_read_lock();
> +	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx, iommu_info)
> +		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
> +				pasid, address, pages, ih);

The xa_array for iommu is already broken. We should fix it before
further use.

https://lore.kernel.org/linux-iommu/20240103124403.GM50406@nvidia.com/

Best regards,
baolu

RE: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, January 17, 2024 4:00 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB
> invalidations
> 
> On 2024/1/16 9:11, Tina Zhang wrote:
> > Devices behind different IOMMUs can be bound to one sva domain. When a
> > range of a sva domain address is being invalidated, VT-d driver needs
> > to issue IOMMU IOTLB and Dev-IOTLB invalidation commands to ask IOMMU
> > hardware and related devices to invalidate their caches.
> >
> > The current logic issues both IOTLB invalidation command and
> > device-TLB command per device, which leads to superfluous IOTLB
> > invalidation (e.g., if there are four devices behind a IOMMU are attached to
> one sva domain.
> > In the current logic, during handing intel_invalidate_range(), four
> > IOTLB invalidation commands and four Dev-IOTLB invalidation commands
> > will be issued. However, only one IOTLB invalidation command and four
> > Dev-IOTLB invalidation command are necessary.), and therefore impacts
> > run-time performance.
> >
> > The patch removes the redundant IOMMU IOTLB invalidations by allowing
> > issuing IOMMU IOTLB invalidation command per iommu instead of per device.
> >
> > Suggested-by: Sanjay Kumar<sanjay.k.kumar@intel.com>
> > Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/svm.c | 53 ++++++++++++++++++---------------------
> >   1 file changed, 25 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> > index 79d1f3107847..bf4962eb229c 100644
> > --- a/drivers/iommu/intel/svm.c
> > +++ b/drivers/iommu/intel/svm.c
> > @@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu *iommu)
> >   	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
> >   }
> >
> > -static void __flush_svm_range_dev(struct dmar_domain *domain,
> > -				  struct dev_pasid_info *dev_pasid,
> > +static void __flush_svm_range(struct iommu_domain *domain,
> >   				  unsigned long address,
> >   				  unsigned long pages, int ih)
> >   {
> > -	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid-
> >dev);
> > -	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
> > +	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
> > +	struct device_domain_info *dev_info;
> > +	struct iommu_domain_info *iommu_info;
> > +	struct dev_pasid_info *dev_pasid;
> > +	unsigned long idx;
> >
> >   	if (WARN_ON(!pages))
> >   		return;
> >
> > -	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
> > -	if (info->ats_enabled) {
> > -		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info-
> >pfsid,
> > -					 pasid, dev_pasid->qdep, address,
> > -					 order_base_2(pages));
> > -		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
> > -					  pasid, dev_pasid->qdep);
> > +	rcu_read_lock();
> > +	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx,
> iommu_info)
> > +		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
> > +				pasid, address, pages, ih);
> 
> The xa_array for iommu is already broken. We should fix it before further use.
> 
> https://lore.kernel.org/linux-iommu/20240103124403.GM50406@nvidia.com/
Agree that domain->iommu_array needs to protect the element (struct iommu_domain_info) from UAF as well. This could be another topic.

I realize that we don't need to use domain->iommu_array here. What we want here is a pointer pointing to a struct intel_iommu. We can get the info from device using dev_iommu_priv_get(), instead of getting the info from iommu_array.

Besides, I'm thinking about sorting the domain->dev_pasids in intel_iommu_set_dev_pasid() to make struct dev_pasid_info of the devices behind the same IOMMU stay together. It can be helpful to reduce the redundant IOMMU IOTLB invalidations.

Regards,
-Tina

> 
> Best regards,
> baolu

Re: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Baolu Lu]

On 2024/2/16 13:38, Zhang, Tina wrote:
>>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>>> index 79d1f3107847..bf4962eb229c 100644
>>> --- a/drivers/iommu/intel/svm.c
>>> +++ b/drivers/iommu/intel/svm.c
>>> @@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu *iommu)
>>>    	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
>>>    }
>>>
>>> -static void __flush_svm_range_dev(struct dmar_domain *domain,
>>> -				  struct dev_pasid_info *dev_pasid,
>>> +static void __flush_svm_range(struct iommu_domain *domain,
>>>    				  unsigned long address,
>>>    				  unsigned long pages, int ih)
>>>    {
>>> -	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid-
>>> dev);
>>> -	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
>>> +	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
>>> +	struct device_domain_info *dev_info;
>>> +	struct iommu_domain_info *iommu_info;
>>> +	struct dev_pasid_info *dev_pasid;
>>> +	unsigned long idx;
>>>
>>>    	if (WARN_ON(!pages))
>>>    		return;
>>>
>>> -	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages, ih);
>>> -	if (info->ats_enabled) {
>>> -		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info-
>>> pfsid,
>>> -					 pasid, dev_pasid->qdep, address,
>>> -					 order_base_2(pages));
>>> -		quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
>>> -					  pasid, dev_pasid->qdep);
>>> +	rcu_read_lock();
>>> +	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx,
>> iommu_info)
>>> +		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
>>> +				pasid, address, pages, ih);
>> The xa_array for iommu is already broken. We should fix it before further use.
>>
>> https://lore.kernel.org/linux-iommu/20240103124403.GM50406@nvidia.com/
> Agree that domain->iommu_array needs to protect the element (struct iommu_domain_info) from UAF as well. This could be another topic.
> 
> I realize that we don't need to use domain->iommu_array here. What we want here is a pointer pointing to a struct intel_iommu. We can get the info from device using dev_iommu_priv_get(), instead of getting the info from iommu_array.
> 
> Besides, I'm thinking about sorting the domain->dev_pasids in intel_iommu_set_dev_pasid() to make struct dev_pasid_info of the devices behind the same IOMMU stay together. It can be helpful to reduce the redundant IOMMU IOTLB invalidations.

I think what you want here are the {domain_id, pasid} pairs to flush the
IOTLB cache in the IOMMU, right? Perhaps we could refine this code
further by introducing a structure of cache tag. The cache tags can be
stored in the domain's IOMMU array. I'm working on a solution and I can
post it for discussion later.

Best regards,
baolu

RE: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB invalidations

[Zhang, Tina]

Hi Baolu,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, February 18, 2024 5:19 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>
> Subject: Re: [PATCH 11/11] iommu/vt-d: Remove superfluous IOMMU IOTLB
> invalidations
> 
> On 2024/2/16 13:38, Zhang, Tina wrote:
> >>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> >>> index 79d1f3107847..bf4962eb229c 100644
> >>> --- a/drivers/iommu/intel/svm.c
> >>> +++ b/drivers/iommu/intel/svm.c
> >>> @@ -134,31 +134,39 @@ void intel_svm_check(struct intel_iommu
> *iommu)
> >>>    	iommu->flags |= VTD_FLAG_SVM_CAPABLE;
> >>>    }
> >>>
> >>> -static void __flush_svm_range_dev(struct dmar_domain *domain,
> >>> -				  struct dev_pasid_info *dev_pasid,
> >>> +static void __flush_svm_range(struct iommu_domain *domain,
> >>>    				  unsigned long address,
> >>>    				  unsigned long pages, int ih)
> >>>    {
> >>> -	struct device_domain_info *info = dev_iommu_priv_get(dev_pasid-
> >>> dev);
> >>> -	u32 pasid = mm_get_enqcmd_pasid(domain->domain.mm);
> >>> +	u32 pasid = mm_get_enqcmd_pasid(domain->mm);
> >>> +	struct device_domain_info *dev_info;
> >>> +	struct iommu_domain_info *iommu_info;
> >>> +	struct dev_pasid_info *dev_pasid;
> >>> +	unsigned long idx;
> >>>
> >>>    	if (WARN_ON(!pages))
> >>>    		return;
> >>>
> >>> -	qi_flush_piotlb(info->iommu, dev_pasid->did, pasid, address, pages,
> ih);
> >>> -	if (info->ats_enabled) {
> >>> -		qi_flush_dev_iotlb_pasid(info->iommu, dev_pasid->sid, info-
> >>> pfsid,
> >>> -					 pasid, dev_pasid->qdep, address,
> >>> -					 order_base_2(pages));
> >>> -		quirk_extra_dev_tlb_flush(info, address,
> order_base_2(pages),
> >>> -					  pasid, dev_pasid->qdep);
> >>> +	rcu_read_lock();
> >>> +	xa_for_each(&to_dmar_domain(domain)->iommu_array, idx,
> >> iommu_info)
> >>> +		qi_flush_piotlb(iommu_info->iommu, dev_pasid->did,
> >>> +				pasid, address, pages, ih);
> >> The xa_array for iommu is already broken. We should fix it before further
> use.
> >>
> >> https://lore.kernel.org/linux-
> iommu/20240103124403.GM50406@nvidia.com
> >> /
> > Agree that domain->iommu_array needs to protect the element (struct
> iommu_domain_info) from UAF as well. This could be another topic.
> >
> > I realize that we don't need to use domain->iommu_array here. What we
> want here is a pointer pointing to a struct intel_iommu. We can get the info
> from device using dev_iommu_priv_get(), instead of getting the info from
> iommu_array.
> >
> > Besides, I'm thinking about sorting the domain->dev_pasids in
> intel_iommu_set_dev_pasid() to make struct dev_pasid_info of the devices
> behind the same IOMMU stay together. It can be helpful to reduce the
> redundant IOMMU IOTLB invalidations.
> 
> I think what you want here are the {domain_id, pasid} pairs to flush the IOTLB
Well, for this sva case we need to know the pointers of struct intel_iommu instances.

For example, there are four devices (device1, device2, device3, device4) and two IOMMUs (IOMMU1 and IOMMU2). Device1 and device2 are behind IOMMU1, while device3 and device4 are behind IOMMU2. If all the four devices are bound to a sva domain, the domain->dev_pasids will list those four devices. We expect when arch_invalidate_secondary_tlbs() callback is invoked, there will be 2 qi_flush_piotlb() + 4 qi_flush_dev_iotlb_pasid(). However, currently, without this path-set, there will be 4 qi_flush_piotlb() + 4 qi_flush_dev_iotlb_pasid(). The reason why there are 2 more redundant qi_flush_piotlb() is because domain->dev_pasids list only has the attached device info (no IOMMU info). But IOMMU info can be got from device.

So, if we can sort the domain->dev_pasids and make the devices behind one IOMMU stay together, then when iterating the domain->dev_pasids, we can skip calling redundant qi_flush_piotlb() by checking if the device's IOMMU is the same as the pervious one (if it's same, then no need to call qi_flush_piotlb() again).

I plan to submit the new version of the patch-set soon. Also, I'm looking forward to seeing your proposal. Let's discuss later. Thanks.

Regards,
-Tina

> cache in the IOMMU, right? Perhaps we could refine this code further by
> introducing a structure of cache tag. The cache tags can be stored in the
> domain's IOMMU array. I'm working on a solution and I can post it for
> discussion later.
> 
> Best regards,
> baolu