Messages in this thread | | | Date | Thu, 11 Apr 2024 20:10:52 +0800 | Subject | Re: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface | From | Baolu Lu <> |
| |
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 <baolu.lu@linux.intel.com> >> --- >> 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/20240208082307.15759-1-yi.l.liu@intel.com/
Best regards, baolu
| |