The IOMMU hardware cache needs to be invalidated whenever the mappings
in the domain are changed. Currently, domain cache invalidation is
scattered across different places, causing several issues:
- IOMMU IOTLB Invalidation: This is done by iterating through the domain
IDs of each domain using the following code:
xa_for_each(&dmar_domain->iommu_array, i, info)
iommu_flush_iotlb_psi(info->iommu, dmar_domain,
start_pfn, nrpages,
list_empty(&gather->freelist), 0);
This code could theoretically cause a use-after-free problem because
there's no lock to protect the "info" pointer within the loop.
- Inconsistent Invalidation Methods: Different domain types implement
their own cache invalidation methods, making the code difficult to
maintain. For example, the DMA domain, SVA domain, and nested domain
have similar cache invalidation code scattered across different files.
- SVA Domain Inconsistency: The SVA domain implementation uses a
completely different data structure to track attached devices compared
to other domains. This creates unnecessary differences and, even
worse, leads to duplicate IOTLB invalidation when an SVA domain is
attached to devices belonging to different IOMMU domains.
- Nested Domain Dependency: The special overlap between a nested domain
and its parent domain requires a dedicated parent_domain_flush()
helper function to be called everywhere the parent domain's mapping
changes.
- Limited Debugging Support: There are currently no debugging aids
available for domain cache invalidation.
By consolidating domain cache invalidation into a common location, we
can address the issues mentioned above and improve the code's
maintainability and debuggability.
Jason Gunthorpe (1):
iommu: Add ops->domain_alloc_sva()
Lu Baolu (11):
iommu/vt-d: Add cache tag assignment interface
iommu/vt-d: Add cache tag invalidation helpers
iommu/vt-d: Add trace events for cache tag interface
iommu/vt-d: Use cache_tag_flush_all() in flush_iotlb_all
iommu/vt-d: Use cache_tag_flush_range() in tlb_sync
iommu/vt-d: Use cache_tag_flush_cm_range() in iotlb_sync_map
iommu/vt-d: Cleanup use of iommu_flush_iotlb_psi()
iommu/vt-d: Use cache_tag_flush_range() in cache_invalidate_user
iommu/vt-d: Use cache helpers in arch_invalidate_secondary_tlbs
iommu/vt-d: Retire intel_svm_dev
iommu/vt-d: Retire struct intel_svm
include/linux/iommu.h | 3 +
drivers/iommu/intel/iommu.h | 66 +++---
drivers/iommu/intel/trace.h | 97 +++++++++
drivers/iommu/intel/cache.c | 389 +++++++++++++++++++++++++++++++++++
drivers/iommu/intel/iommu.c | 294 ++++----------------------
drivers/iommu/intel/nested.c | 71 ++-----
drivers/iommu/intel/svm.c | 279 ++++++-------------------
drivers/iommu/iommu-sva.c | 16 +-
drivers/iommu/intel/Makefile | 2 +-
9 files changed, 660 insertions(+), 557 deletions(-)
create mode 100644 drivers/iommu/intel/cache.c
--
2.34.1
The cache_invalidate_user callback is called to invalidate a range
of caches for the affected user domain. Use cache_tag_flush_range()
in this callback.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/nested.c | 50 +++---------------------------------
1 file changed, 3 insertions(+), 47 deletions(-)
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 85c744099558..e251507cfcc0 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -90,50 +90,6 @@ static void intel_nested_domain_free(struct iommu_domain *domain)
kfree(dmar_domain);
}
-static void nested_flush_dev_iotlb(struct dmar_domain *domain, u64 addr,
- unsigned int mask)
-{
- struct device_domain_info *info;
- unsigned long flags;
- u16 sid, qdep;
-
- spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(info, &domain->devices, link) {
- if (!info->ats_enabled)
- continue;
- sid = info->bus << 8 | info->devfn;
- qdep = info->ats_qdep;
- qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
- qdep, addr, mask);
- quirk_extra_dev_tlb_flush(info, addr, mask,
- IOMMU_NO_PASID, qdep);
- }
- spin_unlock_irqrestore(&domain->lock, flags);
-}
-
-static void intel_nested_flush_cache(struct dmar_domain *domain, u64 addr,
- u64 npages, bool ih)
-{
- struct iommu_domain_info *info;
- unsigned int mask;
- unsigned long i;
-
- xa_for_each(&domain->iommu_array, i, info)
- qi_flush_piotlb(info->iommu,
- domain_id_iommu(domain, info->iommu),
- IOMMU_NO_PASID, addr, npages, ih);
-
- if (!domain->has_iotlb_device)
- return;
-
- if (npages == U64_MAX)
- mask = 64 - VTD_PAGE_SHIFT;
- else
- mask = ilog2(__roundup_pow_of_two(npages));
-
- nested_flush_dev_iotlb(domain, addr, mask);
-}
-
static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
struct iommu_user_data_array *array)
{
@@ -166,9 +122,9 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
break;
}
- intel_nested_flush_cache(dmar_domain, inv_entry.addr,
- inv_entry.npages,
- inv_entry.flags & IOMMU_VTD_INV_FLAGS_LEAF);
+ cache_tag_flush_range(dmar_domain, inv_entry.addr,
+ inv_entry.npages,
+ inv_entry.flags & IOMMU_VTD_INV_FLAGS_LEAF);
processed++;
}
--
2.34.1
The iotlb_sync_map callback is called by the iommu core after non-present
to present mappings are created. The iommu driver uses this callback to
invalidate caches if IOMMU is working in caching mode and second-only
translation is used for the domain. Use cache_tag_flush_cm_range() in this
callback.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1c03d2dafb9d..2dcab1e5cd4d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
iommu_flush_dev_iotlb(domain, addr, mask);
}
-/* Notification for newly created mappings */
-static void __mapping_notify_one(struct intel_iommu *iommu, struct dmar_domain *domain,
- unsigned long pfn, unsigned int pages)
-{
- /*
- * It's a non-present to present mapping. Only flush if caching mode
- * and second level.
- */
- if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
- iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
- else
- iommu_flush_write_buffer(iommu);
-}
-
/*
* Flush the relevant caches in nested translation if the domain
* also serves as a parent
@@ -4549,14 +4535,8 @@ static bool risky_device(struct pci_dev *pdev)
static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
unsigned long iova, size_t size)
{
- struct dmar_domain *dmar_domain = to_dmar_domain(domain);
- unsigned long pages = aligned_nrpages(iova, size);
- unsigned long pfn = iova >> VTD_PAGE_SHIFT;
- struct iommu_domain_info *info;
- unsigned long i;
+ cache_tag_flush_cm_range(to_dmar_domain(domain), iova, iova + size - 1);
- xa_for_each(&dmar_domain->iommu_array, i, info)
- __mapping_notify_one(info->iommu, dmar_domain, pfn, pages);
return 0;
}
--
2.34.1
The intel_svm_dev data structure used in the sva implementation for the
Intel IOMMU driver stores information about a device attached to an SVA
domain. It is a duplicate of dev_pasid_info that serves the same purpose.
Replace intel_svm_dev with dev_pasid_info and clean up the use of
intel_svm_dev.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.h | 15 +----
drivers/iommu/intel/iommu.c | 30 ++++-----
drivers/iommu/intel/svm.c | 131 +++++++++++-------------------------
3 files changed, 55 insertions(+), 121 deletions(-)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 2575ded9f354..5dcced6f3634 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -650,6 +650,7 @@ struct dmar_domain {
struct list_head s2_link;
};
};
+ struct intel_svm *svm;
struct iommu_domain domain; /* generic domain data structure for
iommu core */
@@ -1131,23 +1132,13 @@ int intel_svm_finish_prq(struct intel_iommu *iommu);
void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
struct iommu_page_response *msg);
struct iommu_domain *intel_svm_domain_alloc(void);
-void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
+void intel_svm_remove_dev_pasid(struct iommu_domain *domain);
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;
u32 pasid;
- struct list_head devs;
struct dmar_domain *domain;
};
#else
@@ -1158,7 +1149,7 @@ static inline struct iommu_domain *intel_svm_domain_alloc(void)
return NULL;
}
-static inline void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid)
+static inline void intel_svm_remove_dev_pasid(struct iommu_domain *domain)
{
}
#endif
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6e019297843b..2ac1fc041333 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4384,18 +4384,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
goto out_tear_down;
dmar_domain = to_dmar_domain(domain);
- /*
- * 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 == IOMMU_DOMAIN_SVA) {
- intel_svm_remove_dev_pasid(dev, pasid);
- cache_tag_unassign_domain(dmar_domain,
- FLPT_DEFAULT_DID, dev, pasid);
- goto out_tear_down;
- }
-
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) {
@@ -4407,10 +4395,20 @@ 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);
- did = domain_id_iommu(dmar_domain, iommu);
- cache_tag_unassign_domain(dmar_domain, did, dev, pasid);
- domain_detach_iommu(dmar_domain, iommu);
- intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
+ /*
+ * 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 == IOMMU_DOMAIN_SVA) {
+ cache_tag_unassign_domain(dmar_domain, FLPT_DEFAULT_DID, dev, pasid);
+ intel_svm_remove_dev_pasid(domain);
+ } else {
+ did = domain_id_iommu(dmar_domain, iommu);
+ cache_tag_unassign_domain(dmar_domain, did, dev, pasid);
+ domain_detach_iommu(dmar_domain, iommu);
+ intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
+ }
kfree(dev_pasid);
out_tear_down:
intel_pasid_tear_down_entry(iommu, dev, pasid, false);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 751fab476fa2..0b767d16fb71 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -43,23 +43,6 @@ 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)
-{
- struct intel_svm_dev *sdev = NULL, *t;
-
- rcu_read_lock();
- list_for_each_entry_rcu(t, &svm->devs, list) {
- if (t->dev == dev) {
- sdev = t;
- break;
- }
- }
- rcu_read_unlock();
-
- return sdev;
-}
-
int intel_svm_enable_prq(struct intel_iommu *iommu)
{
struct iopf_queue *iopfq;
@@ -187,7 +170,10 @@ 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 dmar_domain *domain = svm->domain;
+ struct dev_pasid_info *dev_pasid;
+ struct device_domain_info *info;
+ unsigned long flags;
/* This might end up being called from exit_mmap(), *before* the page
* tables are cleared. And __mmu_notifier_release() will delete us from
@@ -201,11 +187,13 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
* page) so that we end up taking a fault that the hardware really
* *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,
- svm->pasid, true);
- rcu_read_unlock();
+ spin_lock_irqsave(&domain->lock, flags);
+ list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
+ info = dev_iommu_priv_get(dev_pasid->dev);
+ intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
+ dev_pasid->pasid, true);
+ }
+ spin_unlock_irqrestore(&domain->lock, flags);
}
@@ -214,47 +202,17 @@ 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,
- struct intel_svm **rsvm,
- struct intel_svm_dev **rsdev)
-{
- struct intel_svm_dev *sdev = 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;
- sdev = svm_lookup_device_by_dev(svm, dev);
-
-out:
- *rsvm = svm;
- *rsdev = sdev;
-
- return 0;
-}
-
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 dmar_domain *dmar_domain = to_dmar_domain(domain);
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;
+ unsigned long flags;
int ret = 0;
svm = pasid_private_find(pasid);
@@ -265,7 +223,6 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
svm->pasid = pasid;
svm->mm = mm;
- INIT_LIST_HEAD_RCU(&svm->devs);
svm->notifier.ops = &intel_mmuops;
svm->domain = to_dmar_domain(domain);
@@ -283,26 +240,19 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
}
}
- sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
- if (!sdev) {
- ret = -ENOMEM;
+ dmar_domain->svm = svm;
+
+ dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+ if (!dev_pasid)
goto free_svm;
- }
- sdev->dev = dev;
- sdev->iommu = iommu;
- sdev->did = FLPT_DEFAULT_DID;
- sdev->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->dev = dev;
+ dev_pasid->pasid = pasid;
ret = cache_tag_assign_domain(to_dmar_domain(domain),
FLPT_DEFAULT_DID, dev, pasid);
if (ret)
- goto free_sdev;
+ goto free_dev_pasid;
/* Setup the pasid table: */
sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
@@ -311,17 +261,19 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
if (ret)
goto unassign_tag;
- list_add_rcu(&sdev->list, &svm->devs);
+ spin_lock_irqsave(&dmar_domain->lock, flags);
+ list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+ spin_unlock_irqrestore(&dmar_domain->lock, flags);
return 0;
unassign_tag:
cache_tag_unassign_domain(to_dmar_domain(domain),
FLPT_DEFAULT_DID, dev, pasid);
-free_sdev:
- kfree(sdev);
+free_dev_pasid:
+ kfree(dev_pasid);
free_svm:
- if (list_empty(&svm->devs)) {
+ if (list_empty(&dmar_domain->dev_pasids)) {
mmu_notifier_unregister(&svm->notifier, mm);
pasid_private_remove(pasid);
kfree(svm);
@@ -330,26 +282,17 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
return ret;
}
-void intel_svm_remove_dev_pasid(struct device *dev, u32 pasid)
+void intel_svm_remove_dev_pasid(struct iommu_domain *domain)
{
- struct intel_svm_dev *sdev;
- struct intel_svm *svm;
- struct mm_struct *mm;
+ struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+ struct intel_svm *svm = dmar_domain->svm;
+ struct mm_struct *mm = domain->mm;
- if (pasid_to_svm_sdev(dev, pasid, &svm, &sdev))
- return;
- mm = svm->mm;
-
- if (sdev) {
- list_del_rcu(&sdev->list);
- kfree_rcu(sdev, rcu);
-
- if (list_empty(&svm->devs)) {
- if (svm->notifier.ops)
- mmu_notifier_unregister(&svm->notifier, mm);
- pasid_private_remove(svm->pasid);
- kfree(svm);
- }
+ if (list_empty(&dmar_domain->dev_pasids)) {
+ if (svm->notifier.ops)
+ mmu_notifier_unregister(&svm->notifier, mm);
+ pasid_private_remove(svm->pasid);
+ kfree(svm);
}
}
@@ -737,8 +680,10 @@ struct iommu_domain *intel_svm_domain_alloc(void)
return NULL;
domain->domain.ops = &intel_svm_domain_ops;
domain->use_first_level = true;
+ INIT_LIST_HEAD(&domain->dev_pasids);
INIT_LIST_HEAD(&domain->cache_tags);
spin_lock_init(&domain->cache_lock);
+ spin_lock_init(&domain->lock);
return &domain->domain;
}
--
2.34.1
The tlb_sync callback is called by the iommu core to flush a range of
caches for the affected domain. Use cache_tag_flush_range() in this
callback.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 21 ++-------------------
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 4ce98f23917c..1c03d2dafb9d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4112,25 +4112,8 @@ static size_t intel_iommu_unmap_pages(struct iommu_domain *domain,
static void intel_iommu_tlb_sync(struct iommu_domain *domain,
struct iommu_iotlb_gather *gather)
{
- struct dmar_domain *dmar_domain = to_dmar_domain(domain);
- unsigned long iova_pfn = IOVA_PFN(gather->start);
- size_t size = gather->end - gather->start;
- struct iommu_domain_info *info;
- unsigned long start_pfn;
- unsigned long nrpages;
- unsigned long i;
-
- nrpages = aligned_nrpages(gather->start, size);
- start_pfn = mm_to_dma_pfn_start(iova_pfn);
-
- xa_for_each(&dmar_domain->iommu_array, i, info)
- iommu_flush_iotlb_psi(info->iommu, dmar_domain,
- start_pfn, nrpages,
- list_empty(&gather->freelist), 0);
-
- if (dmar_domain->nested_parent)
- parent_domain_flush(dmar_domain, start_pfn, nrpages,
- list_empty(&gather->freelist));
+ cache_tag_flush_range(to_dmar_domain(domain), gather->start,
+ gather->end, list_empty(&gather->freelist));
put_pages_list(&gather->freelist);
}
--
2.34.1
The arch_invalidate_secondary_tlbs callback is called in the SVA mm
notification path. It invalidates all or a range of caches after the
CPU page table is modified. Use the cache tag helps in this path.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.h | 1 +
drivers/iommu/intel/svm.c | 76 +++----------------------------------
2 files changed, 6 insertions(+), 71 deletions(-)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index d05fa0122d65..2575ded9f354 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1148,6 +1148,7 @@ struct intel_svm {
struct mm_struct *mm;
u32 pasid;
struct list_head devs;
+ struct dmar_domain *domain;
};
#else
static inline void intel_svm_check(struct intel_iommu *iommu) {}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index d706226e84ee..751fab476fa2 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -168,88 +168,20 @@ void intel_svm_check(struct intel_iommu *iommu)
iommu->flags |= VTD_FLAG_SVM_CAPABLE;
}
-static void __flush_svm_range_dev(struct intel_svm *svm,
- struct intel_svm_dev *sdev,
- unsigned long address,
- unsigned long pages, int ih)
-{
- struct device_domain_info *info = dev_iommu_priv_get(sdev->dev);
-
- if (WARN_ON(!pages))
- return;
-
- qi_flush_piotlb(sdev->iommu, sdev->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,
- order_base_2(pages));
- quirk_extra_dev_tlb_flush(info, address, order_base_2(pages),
- svm->pasid, sdev->qdep);
- }
-}
-
-static void intel_flush_svm_range_dev(struct intel_svm *svm,
- struct intel_svm_dev *sdev,
- 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));
- unsigned long start = ALIGN_DOWN(address, align);
- 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);
- start += align;
- }
-}
-
-static void intel_flush_svm_range(struct intel_svm *svm, unsigned long address,
- unsigned long pages, int ih)
-{
- struct intel_svm_dev *sdev;
-
- rcu_read_lock();
- list_for_each_entry_rcu(sdev, &svm->devs, list)
- intel_flush_svm_range_dev(svm, sdev, 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;
-
- rcu_read_lock();
- list_for_each_entry_rcu(sdev, &svm->devs, list) {
- info = dev_iommu_priv_get(sdev->dev);
-
- qi_flush_piotlb(sdev->iommu, sdev->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,
- 0, 64 - VTD_PAGE_SHIFT);
- quirk_extra_dev_tlb_flush(info, 0, 64 - VTD_PAGE_SHIFT,
- svm->pasid, sdev->qdep);
- }
- }
- rcu_read_unlock();
-}
-
/* Pages have been freed at this point */
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 = svm->domain;
if (start == 0 && end == -1UL) {
- intel_flush_svm_all(svm);
+ cache_tag_flush_all(domain);
return;
}
- intel_flush_svm_range(svm, start,
- (end - start + PAGE_SIZE - 1) >> VTD_PAGE_SHIFT, 0);
+ cache_tag_flush_range(domain, start, end, 0);
}
static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -336,6 +268,7 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
INIT_LIST_HEAD_RCU(&svm->devs);
svm->notifier.ops = &intel_mmuops;
+ svm->domain = to_dmar_domain(domain);
ret = mmu_notifier_register(&svm->notifier, mm);
if (ret) {
kfree(svm);
@@ -803,6 +736,7 @@ struct iommu_domain *intel_svm_domain_alloc(void)
if (!domain)
return NULL;
domain->domain.ops = &intel_svm_domain_ops;
+ domain->use_first_level = true;
INIT_LIST_HEAD(&domain->cache_tags);
spin_lock_init(&domain->cache_lock);
--
2.34.1
The flush_iotlb_all callback is called by the iommu core to flush
all caches for the affected domain. Use cache_tag_flush_all() in
this callback.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 93e4422c9b10..4ce98f23917c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1560,26 +1560,7 @@ static void parent_domain_flush(struct dmar_domain *domain,
static void intel_flush_iotlb_all(struct iommu_domain *domain)
{
- struct dmar_domain *dmar_domain = to_dmar_domain(domain);
- struct iommu_domain_info *info;
- unsigned long idx;
-
- xa_for_each(&dmar_domain->iommu_array, idx, info) {
- struct intel_iommu *iommu = info->iommu;
- u16 did = domain_id_iommu(dmar_domain, iommu);
-
- if (dmar_domain->use_first_level)
- domain_flush_pasid_iotlb(iommu, dmar_domain, 0, -1, 0);
- else
- iommu->flush.flush_iotlb(iommu, did, 0, 0,
- DMA_TLB_DSI_FLUSH);
-
- if (!cap_caching_mode(iommu->cap))
- iommu_flush_dev_iotlb(dmar_domain, 0, MAX_AGAW_PFN_WIDTH);
- }
-
- if (dmar_domain->nested_parent)
- parent_domain_flush(dmar_domain, 0, -1, 0);
+ cache_tag_flush_all(to_dmar_domain(domain));
}
static void iommu_disable_protect_mem_regions(struct intel_iommu *iommu)
--
2.34.1
Caching tag is a combination of tags used by the hardware to cache various
translations. Whenever a mapping in a domain is changed, the IOMMU driver
should invalidate the caches with the caching tags. The VT-d specification
describes caching tags in section 6.2.1, Tagging of Cached Translations.
Add interface to assign caching tags to an IOMMU domain when attached to a
RID or PASID, and unassign caching tags when a domain is detached from a
RID or PASID. All caching tags are listed in the per-domain tag list and
are protected by a dedicated lock.
In addition to the basic IOTLB and devTLB caching tag types, PARENT_IOTLB
and PARENT_DEVTLB tag types are also introduced. These tags are used for
caches that store translations for DMA accesses through a nested user
domain. They are affected by changes to mappings in the parent domain.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.h | 25 +++++
drivers/iommu/intel/cache.c | 192 +++++++++++++++++++++++++++++++++++
drivers/iommu/intel/iommu.c | 31 +++++-
drivers/iommu/intel/nested.c | 21 +++-
drivers/iommu/intel/svm.c | 12 ++-
drivers/iommu/intel/Makefile | 2 +-
6 files changed, 274 insertions(+), 9 deletions(-)
create mode 100644 drivers/iommu/intel/cache.c
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 404d2476a877..e3723b7a0b31 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -607,6 +607,9 @@ struct dmar_domain {
struct list_head devices; /* all devices' list */
struct list_head dev_pasids; /* all attached pasids */
+ spinlock_t cache_lock; /* Protect the cache tag list */
+ struct list_head cache_tags; /* Cache tag list */
+
int iommu_superpage;/* Level of superpages supported:
0 == 4KiB (no superpages), 1 == 2MiB,
2 == 1GiB, 3 == 512GiB, 4 == 1TiB */
@@ -1092,6 +1095,28 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
const struct iommu_user_data *user_data);
struct device *device_rbtree_find(struct intel_iommu *iommu, u16 rid);
+enum cache_tag_type {
+ CACHE_TAG_TYPE_IOTLB,
+ CACHE_TAG_TYPE_DEVTLB,
+ CACHE_TAG_TYPE_PARENT_IOTLB,
+ CACHE_TAG_TYPE_PARENT_DEVTLB,
+};
+
+struct cache_tag {
+ struct list_head node;
+ enum cache_tag_type type;
+ struct intel_iommu *iommu;
+ struct device *dev;
+ u16 domain_id;
+ ioasid_t pasid;
+ int users;
+};
+
+int cache_tag_assign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid);
+void cache_tag_unassign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid);
+
#ifdef CONFIG_INTEL_IOMMU_SVM
void intel_svm_check(struct intel_iommu *iommu);
int intel_svm_enable_prq(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
new file mode 100644
index 000000000000..5a4e12e494b6
--- /dev/null
+++ b/drivers/iommu/intel/cache.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cache.c - Intel VT-d cache invalidation
+ *
+ * Copyright (C) 2024 Intel Corporation
+ *
+ * Author: Lu Baolu <[email protected]>
+ */
+
+#define pr_fmt(fmt) "DMAR: " fmt
+
+#include <linux/dmar.h>
+#include <linux/iommu.h>
+#include <linux/memory.h>
+#include <linux/spinlock.h>
+
+#include "iommu.h"
+#include "pasid.h"
+
+/* Checks if an existing cache tag can be reused for a new association. */
+static bool cache_tag_reusable(struct cache_tag *tag, u16 domain_id,
+ struct intel_iommu *iommu, struct device *dev,
+ ioasid_t pasid, enum cache_tag_type type)
+{
+ if (tag->type != type)
+ return false;
+
+ if (tag->domain_id != domain_id || tag->pasid != pasid)
+ return false;
+
+ if (type == CACHE_TAG_TYPE_IOTLB)
+ return tag->iommu == iommu;
+
+ if (type == CACHE_TAG_TYPE_DEVTLB)
+ return tag->dev == dev;
+
+ return false;
+}
+
+/* Assign a cache tag with specified type to domain. */
+static int cache_tag_assign(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid,
+ enum cache_tag_type type)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+ struct cache_tag *tag, *temp;
+ unsigned long flags;
+
+ tag = kzalloc(sizeof(*tag), GFP_KERNEL);
+ if (!tag)
+ return -ENOMEM;
+
+ tag->type = type;
+ tag->iommu = iommu;
+ tag->dev = dev;
+ tag->domain_id = did;
+ tag->pasid = pasid;
+ tag->users = 1;
+
+ spin_lock_irqsave(&domain->cache_lock, flags);
+ list_for_each_entry(temp, &domain->cache_tags, node) {
+ if (cache_tag_reusable(temp, did, iommu, dev, pasid, type)) {
+ temp->users++;
+ spin_unlock_irqrestore(&domain->cache_lock, flags);
+ kfree(tag);
+ return 0;
+ }
+ }
+ list_add_tail(&tag->node, &domain->cache_tags);
+ spin_unlock_irqrestore(&domain->cache_lock, flags);
+
+ return 0;
+}
+
+/* Unassign a cache tag with specified type from domain. */
+static void cache_tag_unassign(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid,
+ enum cache_tag_type type)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+ struct cache_tag *tag;
+ unsigned long flags;
+
+ spin_lock_irqsave(&domain->cache_lock, flags);
+ list_for_each_entry(tag, &domain->cache_tags, node) {
+ if (cache_tag_reusable(tag, did, iommu, dev, pasid, type)) {
+ if (--tag->users == 0) {
+ list_del(&tag->node);
+ kfree(tag);
+ }
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&domain->cache_lock, flags);
+}
+
+static int __cache_tag_assign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ int ret;
+
+ ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_IOTLB);
+ if (ret || !info->ats_enabled)
+ return ret;
+
+ ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_DEVTLB);
+ if (ret)
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_IOTLB);
+
+ return ret;
+}
+
+static void __cache_tag_unassign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_IOTLB);
+
+ if (info->ats_enabled)
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_DEVTLB);
+}
+
+static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ int ret;
+
+ ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
+ if (ret || !info->ats_enabled)
+ return ret;
+
+ ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_DEVTLB);
+ if (ret)
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
+
+ return ret;
+}
+
+static void __cache_tag_unassign_parent_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
+
+ if (info->ats_enabled)
+ cache_tag_unassign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_DEVTLB);
+}
+
+/*
+ * Assigns cache tags to a domain when it's associated with a device's
+ * PASID using a specific domain ID.
+ *
+ * On success (return value of 0), cache tags are created and added to the
+ * domain's cache tag list. On failure (negative return value), an error
+ * code is returned indicating the reason for the failure.
+ */
+int cache_tag_assign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ int ret;
+
+ ret = __cache_tag_assign_domain(domain, did, dev, pasid);
+ if (ret || domain->domain.type != IOMMU_DOMAIN_NESTED)
+ return ret;
+
+ ret = __cache_tag_assign_parent_domain(domain->s2_domain, did, dev, pasid);
+ if (ret)
+ __cache_tag_unassign_domain(domain, did, dev, pasid);
+
+ return ret;
+}
+
+/*
+ * Removes the cache tags associated with a device's PASID when the domain is
+ * detached from the device.
+ *
+ * The cache tags must be previously assigned to the domain by calling the
+ * assign interface.
+ */
+void cache_tag_unassign_domain(struct dmar_domain *domain, u16 did,
+ struct device *dev, ioasid_t pasid)
+{
+ __cache_tag_unassign_domain(domain, did, dev, pasid);
+ if (domain->domain.type == IOMMU_DOMAIN_NESTED)
+ __cache_tag_unassign_parent_domain(domain->s2_domain, did, dev, pasid);
+}
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 50eb9aed47cc..b4efbdedccce 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1750,7 +1750,9 @@ static struct dmar_domain *alloc_domain(unsigned int type)
domain->has_iotlb_device = false;
INIT_LIST_HEAD(&domain->devices);
INIT_LIST_HEAD(&domain->dev_pasids);
+ INIT_LIST_HEAD(&domain->cache_tags);
spin_lock_init(&domain->lock);
+ spin_lock_init(&domain->cache_lock);
xa_init(&domain->iommu_array);
return domain;
@@ -2322,11 +2324,20 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
struct device_domain_info *info = dev_iommu_priv_get(dev);
struct intel_iommu *iommu = info->iommu;
unsigned long flags;
+ u16 did;
int ret;
ret = domain_attach_iommu(domain, iommu);
if (ret)
return ret;
+
+ did = domain_id_iommu(domain, iommu);
+ ret = cache_tag_assign_domain(domain, did, dev, IOMMU_NO_PASID);
+ if (ret) {
+ domain_detach_iommu(domain, iommu);
+ return ret;
+ }
+
info->domain = domain;
spin_lock_irqsave(&domain->lock, flags);
list_add(&info->link, &domain->devices);
@@ -3798,6 +3809,7 @@ void device_block_translation(struct device *dev)
struct device_domain_info *info = dev_iommu_priv_get(dev);
struct intel_iommu *iommu = info->iommu;
unsigned long flags;
+ u16 did;
iommu_disable_pci_caps(info);
if (!dev_is_real_dma_subdevice(dev)) {
@@ -3815,6 +3827,8 @@ void device_block_translation(struct device *dev)
list_del(&info->link);
spin_unlock_irqrestore(&info->domain->lock, flags);
+ did = domain_id_iommu(info->domain, iommu);
+ cache_tag_unassign_domain(info->domain, did, dev, IOMMU_NO_PASID);
domain_detach_iommu(info->domain, iommu);
info->domain = NULL;
}
@@ -4595,10 +4609,12 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
struct dmar_domain *dmar_domain;
struct iommu_domain *domain;
unsigned long flags;
+ u16 did;
domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
if (WARN_ON_ONCE(!domain))
goto out_tear_down;
+ dmar_domain = to_dmar_domain(domain);
/*
* The SVA implementation needs to handle its own stuffs like the mm
@@ -4607,10 +4623,11 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
*/
if (domain->type == IOMMU_DOMAIN_SVA) {
intel_svm_remove_dev_pasid(dev, pasid);
+ cache_tag_unassign_domain(dmar_domain,
+ FLPT_DEFAULT_DID, 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) {
if (curr->dev == dev && curr->pasid == pasid) {
@@ -4622,6 +4639,8 @@ 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);
+ did = domain_id_iommu(dmar_domain, iommu);
+ cache_tag_unassign_domain(dmar_domain, did, dev, pasid);
domain_detach_iommu(dmar_domain, iommu);
intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
kfree(dev_pasid);
@@ -4638,6 +4657,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
struct intel_iommu *iommu = info->iommu;
struct dev_pasid_info *dev_pasid;
unsigned long flags;
+ u16 did;
int ret;
if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
@@ -4661,6 +4681,11 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
if (ret)
goto out_free;
+ did = domain_id_iommu(dmar_domain, iommu);
+ ret = cache_tag_assign_domain(dmar_domain, did, dev, pasid);
+ if (ret)
+ goto out_detach_iommu;
+
if (domain_type_is_si(dmar_domain))
ret = intel_pasid_setup_pass_through(iommu, dev, pasid);
else if (dmar_domain->use_first_level)
@@ -4670,7 +4695,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
ret = intel_pasid_setup_second_level(iommu, dmar_domain,
dev, pasid);
if (ret)
- goto out_detach_iommu;
+ goto out_unassign_tag;
dev_pasid->dev = dev;
dev_pasid->pasid = pasid;
@@ -4682,6 +4707,8 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
intel_iommu_debugfs_create_dev_pasid(dev_pasid);
return 0;
+out_unassign_tag:
+ cache_tag_unassign_domain(dmar_domain, did, dev, pasid);
out_detach_iommu:
domain_detach_iommu(dmar_domain, iommu);
out_free:
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index a7d68f3d518a..85c744099558 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -26,6 +26,7 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
struct intel_iommu *iommu = info->iommu;
unsigned long flags;
int ret = 0;
+ u16 did;
if (info->domain)
device_block_translation(dev);
@@ -52,13 +53,15 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
return ret;
}
+ did = domain_id_iommu(dmar_domain, iommu);
+ ret = cache_tag_assign_domain(dmar_domain, did, dev, IOMMU_NO_PASID);
+ if (ret)
+ goto detach_iommu;
+
ret = intel_pasid_setup_nested(iommu, dev,
IOMMU_NO_PASID, dmar_domain);
- if (ret) {
- domain_detach_iommu(dmar_domain, iommu);
- dev_err_ratelimited(dev, "Failed to setup pasid entry\n");
- return ret;
- }
+ if (ret)
+ goto unassign_tag;
info->domain = dmar_domain;
spin_lock_irqsave(&dmar_domain->lock, flags);
@@ -68,6 +71,12 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
domain_update_iotlb(dmar_domain);
return 0;
+unassign_tag:
+ cache_tag_unassign_domain(dmar_domain, did, dev, IOMMU_NO_PASID);
+detach_iommu:
+ domain_detach_iommu(dmar_domain, iommu);
+
+ return ret;
}
static void intel_nested_domain_free(struct iommu_domain *domain)
@@ -206,7 +215,9 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
domain->domain.type = IOMMU_DOMAIN_NESTED;
INIT_LIST_HEAD(&domain->devices);
INIT_LIST_HEAD(&domain->dev_pasids);
+ INIT_LIST_HEAD(&domain->cache_tags);
spin_lock_init(&domain->lock);
+ spin_lock_init(&domain->cache_lock);
xa_init(&domain->iommu_array);
spin_lock(&s2_domain->s1_lock);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index c1bed89b1026..d706226e84ee 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -366,17 +366,25 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
sdev->qdep = 0;
}
+ ret = cache_tag_assign_domain(to_dmar_domain(domain),
+ FLPT_DEFAULT_DID, dev, pasid);
+ if (ret)
+ goto free_sdev;
+
/* 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);
if (ret)
- goto free_sdev;
+ goto unassign_tag;
list_add_rcu(&sdev->list, &svm->devs);
return 0;
+unassign_tag:
+ cache_tag_unassign_domain(to_dmar_domain(domain),
+ FLPT_DEFAULT_DID, dev, pasid);
free_sdev:
kfree(sdev);
free_svm:
@@ -795,6 +803,8 @@ struct iommu_domain *intel_svm_domain_alloc(void)
if (!domain)
return NULL;
domain->domain.ops = &intel_svm_domain_ops;
+ INIT_LIST_HEAD(&domain->cache_tags);
+ spin_lock_init(&domain->cache_lock);
return &domain->domain;
}
diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
index 5402b699a122..c8beb0281559 100644
--- a/drivers/iommu/intel/Makefile
+++ b/drivers/iommu/intel/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_DMAR_TABLE) += dmar.o
-obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o nested.o
+obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o nested.o cache.o
obj-$(CONFIG_DMAR_TABLE) += trace.o cap_audit.o
obj-$(CONFIG_DMAR_PERF) += perf.o
obj-$(CONFIG_INTEL_IOMMU_DEBUGFS) += debugfs.o
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Monday, March 25, 2024 10:17 AM
>
> +enum cache_tag_type {
> + CACHE_TAG_TYPE_IOTLB,
> + CACHE_TAG_TYPE_DEVTLB,
> + CACHE_TAG_TYPE_PARENT_IOTLB,
> + CACHE_TAG_TYPE_PARENT_DEVTLB,
> +};
'_TYPE_' can be removed to make it shorter
> +
> +/* Checks if an existing cache tag can be reused for a new association. */
> +static bool cache_tag_reusable(struct cache_tag *tag, u16 domain_id,
> + struct intel_iommu *iommu, struct device *dev,
> + ioasid_t pasid, enum cache_tag_type type)
cache_tage_match()
> +{
> + if (tag->type != type)
> + return false;
> +
> + if (tag->domain_id != domain_id || tag->pasid != pasid)
> + return false;
> +
> + if (type == CACHE_TAG_TYPE_IOTLB)
> + return tag->iommu == iommu;
> +
> + if (type == CACHE_TAG_TYPE_DEVTLB)
> + return tag->dev == dev;
> +
> + return false;
why do you disallow PARENT_TYPE from reusing? It's not uncommon
to have two devices attached to a same nested domain hence with
the same parent domain. Disallowing tag reuse implies unnecessarily
duplicated cache flushes...
> +}
> +
> +/* Assign a cache tag with specified type to domain. */
> +static int cache_tag_assign(struct dmar_domain *domain, u16 did,
> + struct device *dev, ioasid_t pasid,
> + enum cache_tag_type type)
> +{
> + struct device_domain_info *info = dev_iommu_priv_get(dev);
> + struct intel_iommu *iommu = info->iommu;
> + struct cache_tag *tag, *temp;
> + unsigned long flags;
> +
> + tag = kzalloc(sizeof(*tag), GFP_KERNEL);
> + if (!tag)
> + return -ENOMEM;
> +
> + tag->type = type;
> + tag->iommu = iommu;
> + tag->dev = dev;
should we set tag->dev only for DEVTLB type? It's a bit confusing to set
it for IOTLB type which doesn't care about device. Actually doing so
is instead misleading as the 1st device creating the tag may have been
detached but then it will still show up in the trace when the last device
detach destroying the tag.
> +static int __cache_tag_assign_parent_domain(struct dmar_domain
> *domain, u16 did,
> + struct device *dev, ioasid_t pasid)
this pair is similar to the earlier one except the difference on type.
what about keeping just one pair which accepts a 'parent' argument to
decide the type internally?
> +/*
> + * Assigns cache tags to a domain when it's associated with a device's
> + * PASID using a specific domain ID.
s/Assigns/Assign/
> +
> + did = domain_id_iommu(domain, iommu);
> + ret = cache_tag_assign_domain(domain, did, dev,
> IOMMU_NO_PASID);
there are many occurrences of this pattern. What about passing in
a 'iommu' parameter and getting 'did' inside the helper? for svm
it can be specialized internally too.
> @@ -4607,10 +4623,11 @@ static void
> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> */
> if (domain->type == IOMMU_DOMAIN_SVA) {
> intel_svm_remove_dev_pasid(dev, pasid);
> + cache_tag_unassign_domain(dmar_domain,
> + FLPT_DEFAULT_DID, dev, pasid);
is it correct to destroy the tag before teardown completes, e.g. iotlb still
needs to be flushed in intel_pasid_tear_down_entry()?
> From: Lu Baolu <[email protected]>
> Sent: Monday, March 25, 2024 10:17 AM
>
> The flush_iotlb_all callback is called by the iommu core to flush
> all caches for the affected domain. Use cache_tag_flush_all() in
> this callback.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 21 +--------------------
> 1 file changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 93e4422c9b10..4ce98f23917c 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1560,26 +1560,7 @@ static void parent_domain_flush(struct
> dmar_domain *domain,
>
> static void intel_flush_iotlb_all(struct iommu_domain *domain)
> {
> - struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> - struct iommu_domain_info *info;
> - unsigned long idx;
> -
> - xa_for_each(&dmar_domain->iommu_array, idx, info) {
> - struct intel_iommu *iommu = info->iommu;
> - u16 did = domain_id_iommu(dmar_domain, iommu);
> -
> - if (dmar_domain->use_first_level)
> - domain_flush_pasid_iotlb(iommu, dmar_domain, 0,
> -1, 0);
> - else
> - iommu->flush.flush_iotlb(iommu, did, 0, 0,
> - DMA_TLB_DSI_FLUSH);
> -
> - if (!cap_caching_mode(iommu->cap))
> - iommu_flush_dev_iotlb(dmar_domain, 0,
> MAX_AGAW_PFN_WIDTH);
> - }
> -
> - if (dmar_domain->nested_parent)
> - parent_domain_flush(dmar_domain, 0, -1, 0);
> + cache_tag_flush_all(to_dmar_domain(domain));
> }
>
this replacement causes a functional change. Now devtlb is always
invalidated while old code doesn't do so for caching mode.
Probably you may want to first clean up all inconsistent devtlb
invalidation policies for caching mode before going to this series...
> From: Lu Baolu <[email protected]>
> Sent: Monday, March 25, 2024 10:17 AM
>
> The iotlb_sync_map callback is called by the iommu core after non-present
> to present mappings are created. The iommu driver uses this callback to
> invalidate caches if IOMMU is working in caching mode and second-only
> translation is used for the domain. Use cache_tag_flush_cm_range() in this
> callback.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 1c03d2dafb9d..2dcab1e5cd4d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct
> intel_iommu *iommu,
> iommu_flush_dev_iotlb(domain, addr, mask);
> }
>
> -/* Notification for newly created mappings */
> -static void __mapping_notify_one(struct intel_iommu *iommu, struct
> dmar_domain *domain,
> - unsigned long pfn, unsigned int pages)
> -{
> - /*
> - * It's a non-present to present mapping. Only flush if caching mode
> - * and second level.
> - */
> - if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
> - iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
> - else
> - iommu_flush_write_buffer(iommu);
> -}
iommu_flush_write_buffer is for a quite different issue. it's clearer to
keep it separated from the iotlb helpers.
> From: Lu Baolu <[email protected]>
> Sent: Monday, March 25, 2024 10:17 AM
>
> @@ -166,9 +122,9 @@ static int intel_nested_cache_invalidate_user(struct
> iommu_domain *domain,
> break;
> }
>
> - intel_nested_flush_cache(dmar_domain, inv_entry.addr,
> - inv_entry.npages,
> - inv_entry.flags &
> IOMMU_VTD_INV_FLAGS_LEAF);
> + cache_tag_flush_range(dmar_domain, inv_entry.addr,
> + inv_entry.npages,
> + inv_entry.flags &
> IOMMU_VTD_INV_FLAGS_LEAF);
the helper requires an 'end' address but 'npages' is incorrectly used here.
> From: Lu Baolu <[email protected]>
> Sent: Monday, March 25, 2024 10:17 AM
>
> The IOMMU hardware cache needs to be invalidated whenever the
> mappings
> in the domain are changed. Currently, domain cache invalidation is
> scattered across different places, causing several issues:
>
> - IOMMU IOTLB Invalidation: This is done by iterating through the domain
> IDs of each domain using the following code:
>
> xa_for_each(&dmar_domain->iommu_array, i, info)
> iommu_flush_iotlb_psi(info->iommu, dmar_domain,
> start_pfn, nrpages,
> list_empty(&gather->freelist), 0);
>
> This code could theoretically cause a use-after-free problem because
> there's no lock to protect the "info" pointer within the loop.
>
> - Inconsistent Invalidation Methods: Different domain types implement
> their own cache invalidation methods, making the code difficult to
> maintain. For example, the DMA domain, SVA domain, and nested domain
> have similar cache invalidation code scattered across different files.
>
> - SVA Domain Inconsistency: The SVA domain implementation uses a
> completely different data structure to track attached devices compared
> to other domains. This creates unnecessary differences and, even
> worse, leads to duplicate IOTLB invalidation when an SVA domain is
> attached to devices belonging to different IOMMU domains.
can you elaborate how duplicated invalidations are caused?
>
> - Nested Domain Dependency: The special overlap between a nested domain
> and its parent domain requires a dedicated parent_domain_flush()
> helper function to be called everywhere the parent domain's mapping
> changes.
>
> - Limited Debugging Support: There are currently no debugging aids
> available for domain cache invalidation.
>
> By consolidating domain cache invalidation into a common location, we
> can address the issues mentioned above and improve the code's
> maintainability and debuggability.
>
overall this is a nice work!
Hi Kevin,
Thanks for your review comments.
On 3/28/24 3:12 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> +enum cache_tag_type {
>> + CACHE_TAG_TYPE_IOTLB,
>> + CACHE_TAG_TYPE_DEVTLB,
>> + CACHE_TAG_TYPE_PARENT_IOTLB,
>> + CACHE_TAG_TYPE_PARENT_DEVTLB,
>> +};
>
> '_TYPE_' can be removed to make it shorter
Okay.
>
>> +
>> +/* Checks if an existing cache tag can be reused for a new association. */
>> +static bool cache_tag_reusable(struct cache_tag *tag, u16 domain_id,
>> + struct intel_iommu *iommu, struct device *dev,
>> + ioasid_t pasid, enum cache_tag_type type)
>
> cache_tage_match()
Okay.
>
>> +{
>> + if (tag->type != type)
>> + return false;
>> +
>> + if (tag->domain_id != domain_id || tag->pasid != pasid)
>> + return false;
>> +
>> + if (type == CACHE_TAG_TYPE_IOTLB)
>> + return tag->iommu == iommu;
>> +
>> + if (type == CACHE_TAG_TYPE_DEVTLB)
>> + return tag->dev == dev;
>> +
>> + return false;
>
> why do you disallow PARENT_TYPE from reusing? It's not uncommon
> to have two devices attached to a same nested domain hence with
> the same parent domain. Disallowing tag reuse implies unnecessarily
> duplicated cache flushes...
PARENT_TYPE could be reused. The new helper looks like the following:
/* Checks if an existing cache tag can be reused for a new association. */
static bool cache_tage_match(struct cache_tag *tag, u16 domain_id,
struct intel_iommu *iommu, struct device
*dev,
ioasid_t pasid, enum cache_tag_type type)
{
if (tag->type != type)
return false;
if (tag->domain_id != domain_id || tag->pasid != pasid)
return false;
if (type == CACHE_TAG_IOTLB || type == CACHE_TAG_PARENT_IOTLB)
return tag->iommu == iommu;
if (type == CACHE_TAG_DEVTLB || type == CACHE_TAG_PARENT_DEVTLB)
return tag->dev == dev;
return false;
}
>> +}
>> +
>> +/* Assign a cache tag with specified type to domain. */
>> +static int cache_tag_assign(struct dmar_domain *domain, u16 did,
>> + struct device *dev, ioasid_t pasid,
>> + enum cache_tag_type type)
>> +{
>> + struct device_domain_info *info = dev_iommu_priv_get(dev);
>> + struct intel_iommu *iommu = info->iommu;
>> + struct cache_tag *tag, *temp;
>> + unsigned long flags;
>> +
>> + tag = kzalloc(sizeof(*tag), GFP_KERNEL);
>> + if (!tag)
>> + return -ENOMEM;
>> +
>> + tag->type = type;
>> + tag->iommu = iommu;
>> + tag->dev = dev;
>
> should we set tag->dev only for DEVTLB type? It's a bit confusing to set
> it for IOTLB type which doesn't care about device. Actually doing so
> is instead misleading as the 1st device creating the tag may have been
> detached but then it will still show up in the trace when the last device
> detach destroying the tag.
For IOTLB types, perhaps we could add a struct device pointer for the
iommu. This way, the tag->dev could more directly indicate the device
implementing the cache.
>
>> +static int __cache_tag_assign_parent_domain(struct dmar_domain
>> *domain, u16 did,
>> + struct device *dev, ioasid_t pasid)
>
> this pair is similar to the earlier one except the difference on type.
>
> what about keeping just one pair which accepts a 'parent' argument to
> decide the type internally?
Okay, let try to refine it.
>
>
>> +/*
>> + * Assigns cache tags to a domain when it's associated with a device's
>> + * PASID using a specific domain ID.
>
> s/Assigns/Assign/
Done.
>
>> +
>> + did = domain_id_iommu(domain, iommu);
>> + ret = cache_tag_assign_domain(domain, did, dev,
>> IOMMU_NO_PASID);
>
> there are many occurrences of this pattern. What about passing in
> a 'iommu' parameter and getting 'did' inside the helper? for svm
> it can be specialized internally too.
Perhaps, let me try it later and see what the code looks like.
>
>> @@ -4607,10 +4623,11 @@ static void
>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>> */
>> if (domain->type == IOMMU_DOMAIN_SVA) {
>> intel_svm_remove_dev_pasid(dev, pasid);
>> + cache_tag_unassign_domain(dmar_domain,
>> + FLPT_DEFAULT_DID, dev, pasid);
>
> is it correct to destroy the tag before teardown completes, e.g. iotlb still
> needs to be flushed in intel_pasid_tear_down_entry()?
You are right. iotlb still needs to be there until the teardown
completes. I will investigate this more later.
Beset regards,
baolu
On 4/6/24 8:55 PM, Baolu Lu wrote:
>>
>>> @@ -4607,10 +4623,11 @@ static void
>>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>> */
>>> if (domain->type == IOMMU_DOMAIN_SVA) {
>>> intel_svm_remove_dev_pasid(dev, pasid);
>>> + cache_tag_unassign_domain(dmar_domain,
>>> + FLPT_DEFAULT_DID, dev, pasid);
>>
>> is it correct to destroy the tag before teardown completes, e.g. iotlb
>> still
>> needs to be flushed in intel_pasid_tear_down_entry()?
>
> You are right. iotlb still needs to be there until the teardown
> completes. I will investigate this more later.
I reviewed this again. Cache tags are designed specifically for mapping
and unmapping paths. Therefore, there is no required order for attaching
and detaching paths.
Best regards,
baolu
On 3/28/24 3:59 PM, Tian, Kevin wrote:
>> From: Lu Baolu<[email protected]>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> The IOMMU hardware cache needs to be invalidated whenever the
>> mappings
>> in the domain are changed. Currently, domain cache invalidation is
>> scattered across different places, causing several issues:
>>
>> - IOMMU IOTLB Invalidation: This is done by iterating through the domain
>> IDs of each domain using the following code:
>>
>> xa_for_each(&dmar_domain->iommu_array, i, info)
>> iommu_flush_iotlb_psi(info->iommu, dmar_domain,
>> start_pfn, nrpages,
>> list_empty(&gather->freelist), 0);
>>
>> This code could theoretically cause a use-after-free problem because
>> there's no lock to protect the "info" pointer within the loop.
>>
>> - Inconsistent Invalidation Methods: Different domain types implement
>> their own cache invalidation methods, making the code difficult to
>> maintain. For example, the DMA domain, SVA domain, and nested domain
>> have similar cache invalidation code scattered across different files.
>>
>> - SVA Domain Inconsistency: The SVA domain implementation uses a
>> completely different data structure to track attached devices compared
>> to other domains. This creates unnecessary differences and, even
>> worse, leads to duplicate IOTLB invalidation when an SVA domain is
>> attached to devices belonging to different IOMMU domains.
> can you elaborate how duplicated invalidations are caused?
Yes, sure.
Current Intel SVA implementation keeps the bond between mm and a PASID
of a device in a list of intel_svm_dev. In the mm notifier callback, it
iterates all intel_svam_dev in the list and invalidates the IOTLB and
device TLB sequentially.
If multiple devices belong to a single IOMMU, the IOTLB will be flushed
multiple times. However, since these devices share the same domain ID
and PASID, a single IOTLB cache invalidation is sufficient. The
additional flushes are redundant and negatively impact performance.
Best regards,
baolu
On 3/28/24 3:47 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> The flush_iotlb_all callback is called by the iommu core to flush
>> all caches for the affected domain. Use cache_tag_flush_all() in
>> this callback.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/intel/iommu.c | 21 +--------------------
>> 1 file changed, 1 insertion(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 93e4422c9b10..4ce98f23917c 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1560,26 +1560,7 @@ static void parent_domain_flush(struct
>> dmar_domain *domain,
>>
>> static void intel_flush_iotlb_all(struct iommu_domain *domain)
>> {
>> - struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> - struct iommu_domain_info *info;
>> - unsigned long idx;
>> -
>> - xa_for_each(&dmar_domain->iommu_array, idx, info) {
>> - struct intel_iommu *iommu = info->iommu;
>> - u16 did = domain_id_iommu(dmar_domain, iommu);
>> -
>> - if (dmar_domain->use_first_level)
>> - domain_flush_pasid_iotlb(iommu, dmar_domain, 0,
>> -1, 0);
>> - else
>> - iommu->flush.flush_iotlb(iommu, did, 0, 0,
>> - DMA_TLB_DSI_FLUSH);
>> -
>> - if (!cap_caching_mode(iommu->cap))
>> - iommu_flush_dev_iotlb(dmar_domain, 0,
>> MAX_AGAW_PFN_WIDTH);
>> - }
>> -
>> - if (dmar_domain->nested_parent)
>> - parent_domain_flush(dmar_domain, 0, -1, 0);
>> + cache_tag_flush_all(to_dmar_domain(domain));
>> }
>>
>
> this replacement causes a functional change. Now devtlb is always
> invalidated while old code doesn't do so for caching mode.
You are right.
As my understanding of the VT-d spec, caching mode has nothing to do
with the device TLB, hence we should remove all checks on caching mode
before a device TLB invalidation.
I understand that people are leveraging the cache mode to avoid the
unnecessary VMEXIT from the guest OS since the host iommu unmap() path
ensures that device TLB is also invalidated.
Our direction should be to use the batched queue invalidation requests.
As we are consolidating code to a common place, that becomes much
easier. :-)
> Probably you may want to first clean up all inconsistent devtlb
> invalidation policies for caching mode before going to this series...
Sure thing!
Best regards,
baolu
On 3/28/24 3:48 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> The iotlb_sync_map callback is called by the iommu core after non-present
>> to present mappings are created. The iommu driver uses this callback to
>> invalidate caches if IOMMU is working in caching mode and second-only
>> translation is used for the domain. Use cache_tag_flush_cm_range() in this
>> callback.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/intel/iommu.c | 22 +---------------------
>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 1c03d2dafb9d..2dcab1e5cd4d 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct
>> intel_iommu *iommu,
>> iommu_flush_dev_iotlb(domain, addr, mask);
>> }
>>
>> -/* Notification for newly created mappings */
>> -static void __mapping_notify_one(struct intel_iommu *iommu, struct
>> dmar_domain *domain,
>> - unsigned long pfn, unsigned int pages)
>> -{
>> - /*
>> - * It's a non-present to present mapping. Only flush if caching mode
>> - * and second level.
>> - */
>> - if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
>> - iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
>> - else
>> - iommu_flush_write_buffer(iommu);
>> -}
>
> iommu_flush_write_buffer is for a quite different issue. it's clearer to
> keep it separated from the iotlb helpers.
The VT-d spec describes the write buffer flushing in section 6.8. It
states,
1. Write buffer flushing is required only for earlier VT-d hardware
implementations. Newer hardware implementations are expected to NOT
require this.
2. Write buffer flushing is related to cache invalidation. The hardware
performs an implicit write-buffer-flushing as a pre-condition to
the invalidation operations.
According to this, write buffer flushing is only required in the map
path when IOMMU is not in caching mode and the RWBF iommu capability is
set, which perfectly matches the case of cache_tag_flush_cm_range().
By doing it in cache_tag_flush_cm_range(), the code will be simpler and
easier for maintenance.
Best regards,
baolu
On 3/28/24 3:54 PM, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Monday, March 25, 2024 10:17 AM
>>
>> @@ -166,9 +122,9 @@ static int intel_nested_cache_invalidate_user(struct
>> iommu_domain *domain,
>> break;
>> }
>>
>> - intel_nested_flush_cache(dmar_domain, inv_entry.addr,
>> - inv_entry.npages,
>> - inv_entry.flags &
>> IOMMU_VTD_INV_FLAGS_LEAF);
>> + cache_tag_flush_range(dmar_domain, inv_entry.addr,
>> + inv_entry.npages,
>> + inv_entry.flags &
>> IOMMU_VTD_INV_FLAGS_LEAF);
>
> the helper requires an 'end' address but 'npages' is incorrectly used here.
Oh yes! Thanks for pointing this out. I will fix it like below.
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index c139524274b9..cef997c07158 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1050,6 +1050,12 @@ static inline unsigned long
aligned_nrpages(unsigned long host_addr, size_t size
return PAGE_ALIGN(host_addr + size) >> VTD_PAGE_SHIFT;
}
+/* Return a size from number of VTD pages. */
+static inline unsigned long nrpages_to_size(unsigned long npages)
+{
+ return npages << VTD_PAGE_SHIFT;
+}
+
/* Convert value to context PASID directory size field coding. */
#define context_pdts(pds) (((pds) & 0x7) << 9)
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index e251507cfcc0..ffbd8d98a3b8 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -123,7 +123,7 @@ static int intel_nested_cache_invalidate_user(struct
iommu_domain *domain,
}
cache_tag_flush_range(dmar_domain, inv_entry.addr,
- inv_entry.npages,
+ inv_entry.addr +
nrpages_to_size(inv_entry.npages) - 1,
inv_entry.flags &
IOMMU_VTD_INV_FLAGS_LEAF);
processed++;
}
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Sunday, April 7, 2024 12:35 PM
>
> On 4/6/24 8:55 PM, Baolu Lu wrote:
> >>
> >>> @@ -4607,10 +4623,11 @@ static void
> >>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >>> */
> >>> if (domain->type == IOMMU_DOMAIN_SVA) {
> >>> intel_svm_remove_dev_pasid(dev, pasid);
> >>> + cache_tag_unassign_domain(dmar_domain,
> >>> + FLPT_DEFAULT_DID, dev, pasid);
> >>
> >> is it correct to destroy the tag before teardown completes, e.g. iotlb
> >> still
> >> needs to be flushed in intel_pasid_tear_down_entry()?
> >
> > You are right. iotlb still needs to be there until the teardown
> > completes. I will investigate this more later.
>
> I reviewed this again. Cache tags are designed specifically for mapping
> and unmapping paths. Therefore, there is no required order for attaching
> and detaching paths.
>
Okay. intel_pasid_tear_down_entry() directly retrieves the information
from the pasid entry instead of relying on the domain cache tag info.
so yes destroying the tag at this point is fine.
> From: Baolu Lu <[email protected]>
> Sent: Sunday, April 7, 2024 2:41 PM
>
> On 3/28/24 3:48 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <[email protected]>
> >> Sent: Monday, March 25, 2024 10:17 AM
> >>
> >> The iotlb_sync_map callback is called by the iommu core after non-
> present
> >> to present mappings are created. The iommu driver uses this callback to
> >> invalidate caches if IOMMU is working in caching mode and second-only
> >> translation is used for the domain. Use cache_tag_flush_cm_range() in
> this
> >> callback.
> >>
> >> Signed-off-by: Lu Baolu <[email protected]>
> >> ---
> >> drivers/iommu/intel/iommu.c | 22 +---------------------
> >> 1 file changed, 1 insertion(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >> index 1c03d2dafb9d..2dcab1e5cd4d 100644
> >> --- a/drivers/iommu/intel/iommu.c
> >> +++ b/drivers/iommu/intel/iommu.c
> >> @@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct
> >> intel_iommu *iommu,
> >> iommu_flush_dev_iotlb(domain, addr, mask);
> >> }
> >>
> >> -/* Notification for newly created mappings */
> >> -static void __mapping_notify_one(struct intel_iommu *iommu, struct
> >> dmar_domain *domain,
> >> - unsigned long pfn, unsigned int pages)
> >> -{
> >> - /*
> >> - * It's a non-present to present mapping. Only flush if caching mode
> >> - * and second level.
> >> - */
> >> - if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
> >> - iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
> >> - else
> >> - iommu_flush_write_buffer(iommu);
> >> -}
> >
> > iommu_flush_write_buffer is for a quite different issue. it's clearer to
> > keep it separated from the iotlb helpers.
>
> The VT-d spec describes the write buffer flushing in section 6.8. It
> states,
>
> 1. Write buffer flushing is required only for earlier VT-d hardware
> implementations. Newer hardware implementations are expected to NOT
> require this.
> 2. Write buffer flushing is related to cache invalidation. The hardware
> performs an implicit write-buffer-flushing as a pre-condition to
> the invalidation operations.
>
> According to this, write buffer flushing is only required in the map
> path when IOMMU is not in caching mode and the RWBF iommu capability is
> set, which perfectly matches the case of cache_tag_flush_cm_range().
>
> By doing it in cache_tag_flush_cm_range(), the code will be simpler and
> easier for maintenance.
>
but above applies actually when caching mode is false or when 1st
stage is being used. this is the opposite of what flush_cm_range()
implies.
if we do it through a general flush_range() helper which accepts
a parameter to indicate the map path, then it's reasonable to move
this special handling to that helper as that is another trick which
the caller doesn't need to know.
On 4/8/24 10:51 AM, Tian, Kevin wrote:
>> From: Baolu Lu<[email protected]>
>> Sent: Sunday, April 7, 2024 2:41 PM
>>
>> On 3/28/24 3:48 PM, Tian, Kevin wrote:
>>>> From: Lu Baolu<[email protected]>
>>>> Sent: Monday, March 25, 2024 10:17 AM
>>>>
>>>> The iotlb_sync_map callback is called by the iommu core after non-
>> present
>>>> to present mappings are created. The iommu driver uses this callback to
>>>> invalidate caches if IOMMU is working in caching mode and second-only
>>>> translation is used for the domain. Use cache_tag_flush_cm_range() in
>> this
>>>> callback.
>>>>
>>>> Signed-off-by: Lu Baolu<[email protected]>
>>>> ---
>>>> drivers/iommu/intel/iommu.c | 22 +---------------------
>>>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> index 1c03d2dafb9d..2dcab1e5cd4d 100644
>>>> --- a/drivers/iommu/intel/iommu.c
>>>> +++ b/drivers/iommu/intel/iommu.c
>>>> @@ -1504,20 +1504,6 @@ static void iommu_flush_iotlb_psi(struct
>>>> intel_iommu *iommu,
>>>> iommu_flush_dev_iotlb(domain, addr, mask);
>>>> }
>>>>
>>>> -/* Notification for newly created mappings */
>>>> -static void __mapping_notify_one(struct intel_iommu *iommu, struct
>>>> dmar_domain *domain,
>>>> - unsigned long pfn, unsigned int pages)
>>>> -{
>>>> - /*
>>>> - * It's a non-present to present mapping. Only flush if caching mode
>>>> - * and second level.
>>>> - */
>>>> - if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
>>>> - iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
>>>> - else
>>>> - iommu_flush_write_buffer(iommu);
>>>> -}
>>> iommu_flush_write_buffer is for a quite different issue. it's clearer to
>>> keep it separated from the iotlb helpers.
>> The VT-d spec describes the write buffer flushing in section 6.8. It
>> states,
>>
>> 1. Write buffer flushing is required only for earlier VT-d hardware
>> implementations. Newer hardware implementations are expected to NOT
>> require this.
>> 2. Write buffer flushing is related to cache invalidation. The hardware
>> performs an implicit write-buffer-flushing as a pre-condition to
>> the invalidation operations.
>>
>> According to this, write buffer flushing is only required in the map
>> path when IOMMU is not in caching mode and the RWBF iommu capability is
>> set, which perfectly matches the case of cache_tag_flush_cm_range().
>>
>> By doing it in cache_tag_flush_cm_range(), the code will be simpler and
>> easier for maintenance.
>>
> but above applies actually when caching mode is false or when 1st
> stage is being used. this is the opposite of what flush_cm_range()
> implies.
>
> if we do it through a general flush_range() helper which accepts
> a parameter to indicate the map path, then it's reasonable to move
> this special handling to that helper as that is another trick which
> the caller doesn't need to know.
This could be an option. How about my proposal in another reply?
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Sunday, April 7, 2024 3:28 PM
>
> On 3/28/24 3:59 PM, Tian, Kevin wrote:
> >> From: Lu Baolu<[email protected]>
> >> Sent: Monday, March 25, 2024 10:17 AM
> >>
> >> The IOMMU hardware cache needs to be invalidated whenever the
> >> mappings
> >> in the domain are changed. Currently, domain cache invalidation is
> >> scattered across different places, causing several issues:
> >>
> >> - IOMMU IOTLB Invalidation: This is done by iterating through the domain
> >> IDs of each domain using the following code:
> >>
> >> xa_for_each(&dmar_domain->iommu_array, i, info)
> >> iommu_flush_iotlb_psi(info->iommu, dmar_domain,
> >> start_pfn, nrpages,
> >> list_empty(&gather->freelist), 0);
> >>
> >> This code could theoretically cause a use-after-free problem because
> >> there's no lock to protect the "info" pointer within the loop.
> >>
> >> - Inconsistent Invalidation Methods: Different domain types implement
> >> their own cache invalidation methods, making the code difficult to
> >> maintain. For example, the DMA domain, SVA domain, and nested
> domain
> >> have similar cache invalidation code scattered across different files.
> >>
> >> - SVA Domain Inconsistency: The SVA domain implementation uses a
> >> completely different data structure to track attached devices compared
> >> to other domains. This creates unnecessary differences and, even
> >> worse, leads to duplicate IOTLB invalidation when an SVA domain is
> >> attached to devices belonging to different IOMMU domains.
> > can you elaborate how duplicated invalidations are caused?
>
> Yes, sure.
>
> Current Intel SVA implementation keeps the bond between mm and a PASID
> of a device in a list of intel_svm_dev. In the mm notifier callback, it
> iterates all intel_svam_dev in the list and invalidates the IOTLB and
> device TLB sequentially.
>
> If multiple devices belong to a single IOMMU, the IOTLB will be flushed
> multiple times. However, since these devices share the same domain ID
> and PASID, a single IOTLB cache invalidation is sufficient. The
> additional flushes are redundant and negatively impact performance.
>
yes it's redundant. But what does "devices belonging to different
IOMMU domains" in the original context try to convey? From above
explanation it sounds irrelevant...
On 4/8/24 11:03 AM, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Sunday, April 7, 2024 3:28 PM
>>
>> On 3/28/24 3:59 PM, Tian, Kevin wrote:
>>>> From: Lu Baolu<[email protected]>
>>>> Sent: Monday, March 25, 2024 10:17 AM
>>>>
>>>> The IOMMU hardware cache needs to be invalidated whenever the
>>>> mappings
>>>> in the domain are changed. Currently, domain cache invalidation is
>>>> scattered across different places, causing several issues:
>>>>
>>>> - IOMMU IOTLB Invalidation: This is done by iterating through the domain
>>>> IDs of each domain using the following code:
>>>>
>>>> xa_for_each(&dmar_domain->iommu_array, i, info)
>>>> iommu_flush_iotlb_psi(info->iommu, dmar_domain,
>>>> start_pfn, nrpages,
>>>> list_empty(&gather->freelist), 0);
>>>>
>>>> This code could theoretically cause a use-after-free problem because
>>>> there's no lock to protect the "info" pointer within the loop.
>>>>
>>>> - Inconsistent Invalidation Methods: Different domain types implement
>>>> their own cache invalidation methods, making the code difficult to
>>>> maintain. For example, the DMA domain, SVA domain, and nested
>> domain
>>>> have similar cache invalidation code scattered across different files.
>>>>
>>>> - SVA Domain Inconsistency: The SVA domain implementation uses a
>>>> completely different data structure to track attached devices compared
>>>> to other domains. This creates unnecessary differences and, even
>>>> worse, leads to duplicate IOTLB invalidation when an SVA domain is
>>>> attached to devices belonging to different IOMMU domains.
>>> can you elaborate how duplicated invalidations are caused?
>>
>> Yes, sure.
>>
>> Current Intel SVA implementation keeps the bond between mm and a PASID
>> of a device in a list of intel_svm_dev. In the mm notifier callback, it
>> iterates all intel_svam_dev in the list and invalidates the IOTLB and
>> device TLB sequentially.
>>
>> If multiple devices belong to a single IOMMU, the IOTLB will be flushed
>> multiple times. However, since these devices share the same domain ID
>> and PASID, a single IOTLB cache invalidation is sufficient. The
>> additional flushes are redundant and negatively impact performance.
>>
>
> yes it's redundant. But what does "devices belonging to different
> IOMMU domains" in the original context try to convey? From above
> explanation it sounds irrelevant...
My typo. :-) Sorry for the confusion.
I should say,
"... attached to devices belonging to a same IOMMU ..."
Best regards,
baolu
On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
> Caching tag is a combination of tags used by the hardware to cache various
> translations. Whenever a mapping in a domain is changed, the IOMMU driver
> should invalidate the caches with the caching tags. The VT-d specification
> describes caching tags in section 6.2.1, Tagging of Cached Translations.
>
> Add interface to assign caching tags to an IOMMU domain when attached to a
> RID or PASID, and unassign caching tags when a domain is detached from a
> RID or PASID. All caching tags are listed in the per-domain tag list and
> are protected by a dedicated lock.
>
> In addition to the basic IOTLB and devTLB caching tag types, PARENT_IOTLB
> and PARENT_DEVTLB tag types are also introduced. These tags are used for
> caches that store translations for DMA accesses through a nested user
> domain. They are affected by changes to mappings in the parent domain.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/iommu.h | 25 +++++
> drivers/iommu/intel/cache.c | 192 +++++++++++++++++++++++++++++++++++
> drivers/iommu/intel/iommu.c | 31 +++++-
> drivers/iommu/intel/nested.c | 21 +++-
> drivers/iommu/intel/svm.c | 12 ++-
> drivers/iommu/intel/Makefile | 2 +-
> 6 files changed, 274 insertions(+), 9 deletions(-)
> create mode 100644 drivers/iommu/intel/cache.c
>
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 404d2476a877..e3723b7a0b31 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -607,6 +607,9 @@ struct dmar_domain {
> struct list_head devices; /* all devices' list */
> struct list_head dev_pasids; /* all attached pasids */
>
> + spinlock_t cache_lock; /* Protect the cache tag list */
> + struct list_head cache_tags; /* Cache tag list */
That is quite a neat trick - though building a dedicated invalidation
list duplicates data stored in the attached devices list?
You didn't try to make it RCU safe for invalidation?
> +struct cache_tag {
> + struct list_head node;
> + enum cache_tag_type type;
> + struct intel_iommu *iommu;
> + struct device *dev;
iommu and dev probably don't both need to be stored together. We have
iommu_get_iommu_dev() now.. I suppose this is probably a union of the
two pointers depending on tag. DEVTLB needs the dev and IOTLB needs
the iommu.
> + u16 domain_id;
> + ioasid_t pasid;
> + int users;
unsigned int
> +static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct device_domain_info *info = dev_iommu_priv_get(dev);
> + int ret;
> +
> + ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
> + if (ret || !info->ats_enabled)
> + return ret;
I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
different implementation?
Isn't this backwards though? Each domain should have a list of things
to invalidate if the domain itself changes.
So the nesting parent should have a list of CHILD_DEVTLB's that need
cleaning. That list is changed when the nesting domains are attached
to something.
And a list of CHILD_IOTLBs, but the HW doesn't seem to need that?
Jason
> From: Jason Gunthorpe <[email protected]>
> Sent: Wednesday, April 10, 2024 11:42 PM
>
> On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
> > +static int __cache_tag_assign_parent_domain(struct dmar_domain
> *domain, u16 did,
> > + struct device *dev, ioasid_t pasid)
> > +{
> > + struct device_domain_info *info = dev_iommu_priv_get(dev);
> > + int ret;
> > +
> > + ret = cache_tag_assign(domain, did, dev, pasid,
> CACHE_TAG_TYPE_PARENT_IOTLB);
> > + if (ret || !info->ats_enabled)
> > + return ret;
>
> I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
> different implementation?
>
> Isn't this backwards though? Each domain should have a list of things
> to invalidate if the domain itself changes.
>
> So the nesting parent should have a list of CHILD_DEVTLB's that need
> cleaning. That list is changed when the nesting domains are attached
> to something.
>
probably just a naming confusion. it's called PARENT_IOTLB from the
angle that this domain is used as a parent domain but actually it
tracks the child tags in nested attach.
On 2024/4/10 23:41, Jason Gunthorpe wrote:
> On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
>> Caching tag is a combination of tags used by the hardware to cache various
>> translations. Whenever a mapping in a domain is changed, the IOMMU driver
>> should invalidate the caches with the caching tags. The VT-d specification
>> describes caching tags in section 6.2.1, Tagging of Cached Translations.
>>
>> Add interface to assign caching tags to an IOMMU domain when attached to a
>> RID or PASID, and unassign caching tags when a domain is detached from a
>> RID or PASID. All caching tags are listed in the per-domain tag list and
>> are protected by a dedicated lock.
>>
>> In addition to the basic IOTLB and devTLB caching tag types, PARENT_IOTLB
>> and PARENT_DEVTLB tag types are also introduced. These tags are used for
>> caches that store translations for DMA accesses through a nested user
>> domain. They are affected by changes to mappings in the parent domain.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/intel/iommu.h | 25 +++++
>> drivers/iommu/intel/cache.c | 192 +++++++++++++++++++++++++++++++++++
>> drivers/iommu/intel/iommu.c | 31 +++++-
>> drivers/iommu/intel/nested.c | 21 +++-
>> drivers/iommu/intel/svm.c | 12 ++-
>> drivers/iommu/intel/Makefile | 2 +-
>> 6 files changed, 274 insertions(+), 9 deletions(-)
>> create mode 100644 drivers/iommu/intel/cache.c
>>
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 404d2476a877..e3723b7a0b31 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -607,6 +607,9 @@ struct dmar_domain {
>> struct list_head devices; /* all devices' list */
>> struct list_head dev_pasids; /* all attached pasids */
>>
>> + spinlock_t cache_lock; /* Protect the cache tag list */
>> + struct list_head cache_tags; /* Cache tag list */
>
> That is quite a neat trick - though building a dedicated invalidation
> list duplicates data stored in the attached devices list?
Yes. The device and dev_pasid lists appear to be duplicate. I am about
to remove these two lists later.
> You didn't try to make it RCU safe for invalidation?
The queued invalidation interface is a bit complicated, especially when
it comes to device TLB invalidation. Device TLB invalidation might
result in a timeout, which requires special treatment.
>> +struct cache_tag {
>> + struct list_head node;
>> + enum cache_tag_type type;
>> + struct intel_iommu *iommu;
>> + struct device *dev;
>
> iommu and dev probably don't both need to be stored together. We have
> iommu_get_iommu_dev() now.. I suppose this is probably a union of the
> two pointers depending on tag. DEVTLB needs the dev and IOTLB needs
> the iommu.
>
>> + u16 domain_id;
>> + ioasid_t pasid;
>> + int users;
>
> unsigned int
Sure.
>
>> +static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
>> + struct device *dev, ioasid_t pasid)
>> +{
>> + struct device_domain_info *info = dev_iommu_priv_get(dev);
>> + int ret;
>> +
>> + ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_TYPE_PARENT_IOTLB);
>> + if (ret || !info->ats_enabled)
>> + return ret;
>
> I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
> different implementation?
>
> Isn't this backwards though? Each domain should have a list of things
> to invalidate if the domain itself changes.
>
> So the nesting parent should have a list of CHILD_DEVTLB's that need
> cleaning. That list is changed when the nesting domains are attached
> to something.
>
> And a list of CHILD_IOTLBs, but the HW doesn't seem to need that?
This is a partial replacement of below series.
https://lore.kernel.org/all/[email protected]/
Best regards,
baolu
On 2024/4/10 23:41, Jason Gunthorpe wrote:
>> +struct cache_tag {
>> + struct list_head node;
>> + enum cache_tag_type type;
>> + struct intel_iommu *iommu;
>> + struct device *dev;
> iommu and dev probably don't both need to be stored together. We have
> iommu_get_iommu_dev() now.. I suppose this is probably a union of the
> two pointers depending on tag. DEVTLB needs the dev and IOTLB needs
> the iommu.
I forgot to reply this comment in previous reply. Sorry about it.
struct cache_tag {
[ ... ]
struct intel_iommu *iommu;
struct device *dev;
[ ... ]
};
I treat @iommu as the queued invalidation interface. All cache
invalidation raises to hardware through the invalidation queue.
The @dev field represents the location of the cache. For IOTLB cache, it
resides on the IOMMU hardware. In this case, the field stores the device
pointer to the IOMMU hardware. For DevTLB cache, it locates in the PCIe
endpoint. Here, the field stores the device pointer to that endpoint.
A correctly set @dev pointer allows users to see more accurate trace
message.
Best regards,
baolu
On 2024/4/11 7:14, Tian, Kevin wrote:
>> From: Jason Gunthorpe <[email protected]>
>> Sent: Wednesday, April 10, 2024 11:42 PM
>>
>> On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
>>> +static int __cache_tag_assign_parent_domain(struct dmar_domain
>> *domain, u16 did,
>>> + struct device *dev, ioasid_t pasid)
>>> +{
>>> + struct device_domain_info *info = dev_iommu_priv_get(dev);
>>> + int ret;
>>> +
>>> + ret = cache_tag_assign(domain, did, dev, pasid,
>> CACHE_TAG_TYPE_PARENT_IOTLB);
>>> + if (ret || !info->ats_enabled)
>>> + return ret;
>>
>> I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
>> different implementation?
>>
>> Isn't this backwards though? Each domain should have a list of things
>> to invalidate if the domain itself changes.
>>
>> So the nesting parent should have a list of CHILD_DEVTLB's that need
>> cleaning. That list is changed when the nesting domains are attached
>> to something.
>>
>
> probably just a naming confusion. it's called PARENT_IOTLB from the
> angle that this domain is used as a parent domain but actually it
> tracks the child tags in nested attach.
Is NESTING_IOTLB more readable?
Best regards,
baolu
On Thu, Apr 11, 2024 at 09:17:41PM +0800, Baolu Lu wrote:
> On 2024/4/11 7:14, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <[email protected]>
> > > Sent: Wednesday, April 10, 2024 11:42 PM
> > >
> > > On Mon, Mar 25, 2024 at 10:16:54AM +0800, Lu Baolu wrote:
> > > > +static int __cache_tag_assign_parent_domain(struct dmar_domain
> > > *domain, u16 did,
> > > > + struct device *dev, ioasid_t pasid)
> > > > +{
> > > > + struct device_domain_info *info = dev_iommu_priv_get(dev);
> > > > + int ret;
> > > > +
> > > > + ret = cache_tag_assign(domain, did, dev, pasid,
> > > CACHE_TAG_TYPE_PARENT_IOTLB);
> > > > + if (ret || !info->ats_enabled)
> > > > + return ret;
> > >
> > > I'm not sure I understood the point of PARENT_IOTLB? I didn't see any
> > > different implementation?
> > >
> > > Isn't this backwards though? Each domain should have a list of things
> > > to invalidate if the domain itself changes.
> > >
> > > So the nesting parent should have a list of CHILD_DEVTLB's that need
> > > cleaning. That list is changed when the nesting domains are attached
> > > to something.
> > >
> >
> > probably just a naming confusion. it's called PARENT_IOTLB from the
> > angle that this domain is used as a parent domain but actually it
> > tracks the child tags in nested attach.
>
> Is NESTING_IOTLB more readable?
Yes
Jason
> From: Baolu Lu <[email protected]>
> Sent: Thursday, April 11, 2024 8:39 PM
>
> On 2024/4/10 23:41, Jason Gunthorpe wrote:
> >> +struct cache_tag {
> >> + struct list_head node;
> >> + enum cache_tag_type type;
> >> + struct intel_iommu *iommu;
> >> + struct device *dev;
> > iommu and dev probably don't both need to be stored together. We have
> > iommu_get_iommu_dev() now.. I suppose this is probably a union of the
> > two pointers depending on tag. DEVTLB needs the dev and IOTLB needs
> > the iommu.
>
> I forgot to reply this comment in previous reply. Sorry about it.
>
> struct cache_tag {
> [ ... ]
> struct intel_iommu *iommu;
> struct device *dev;
> [ ... ]
> };
>
> I treat @iommu as the queued invalidation interface. All cache
> invalidation raises to hardware through the invalidation queue.
>
> The @dev field represents the location of the cache. For IOTLB cache, it
> resides on the IOMMU hardware. In this case, the field stores the device
> pointer to the IOMMU hardware. For DevTLB cache, it locates in the PCIe
> endpoint. Here, the field stores the device pointer to that endpoint.
>
> A correctly set @dev pointer allows users to see more accurate trace
> message.
>
it's not a bad to add a comment for @dev here.