Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH 02/12] iommu/vt-d: Add cache tag invalidation helpers | Date | Mon, 8 Apr 2024 03:14:37 +0000 |
| |
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Monday, April 8, 2024 10:54 AM > > On 4/8/24 10:33 AM, Tian, Kevin wrote: > >> From: Baolu Lu<baolu.lu@linux.intel.com> > >> Sent: Sunday, April 7, 2024 1:33 PM > >> > >> On 3/28/24 3:39 PM, Tian, Kevin wrote: > >>>> From: Lu Baolu<baolu.lu@linux.intel.com> > >>>> Sent: Monday, March 25, 2024 10:17 AM > >>>> > >>>> +/* > >>>> + * Invalidate a range of IOVA when IOMMU is in caching mode and > new > >>>> mappings > >>>> + * are added to the target domain. > >>>> + */ > >>>> +void cache_tag_flush_cm_range(struct dmar_domain *domain, > unsigned > >>>> long start, > >>>> + unsigned long end) > >>>> +{ > >>> I'm also not sure why this is worth a separate helper. why couldn't it > >>> be managed by previous flush_range()? > >> This is only my preference. I'd like to separate things belonging to > >> different paths, so that it's easier for maintenance. For example, if, > >> in the future, we need to add or enhance something for a specific case, > >> we don't need to care about other cases. > > IMHO caching mode is an attribute in low level iommu which can be > > handled perfectly well within the helper by checking that attribute. > > > > it sounds a bit weird for the caller to know that detail and call different > > helpers when all paths just want to request to flush a specific range. > > I see. The helper name is a bit confusing. > > Generally speaking, cache_tag_flush_range() and cache_tag_flush_all() > are designed to flush caches for mapping change from present to non- > present. While cache_tag_flush_cm_range() is designed to flush caches > for mapping change from non-present to present. > > How about renaming these helpers to > > cache_tag_flush_present_range() > cache_tag_flush_present_all() > cache_tag_flush_non_present_range()
I'll probably keep cache_tag_flush_range/all() as it is because the naming matches the general interpretation of the tlb semantics.
then call the 3rd one as cache_tag_flush_range_np() for various additional requirements on transitions from non-present entry.
> > ? > > In cache_tag_flush_non_present_range(), > > - if IOMMU is not in caching mode, flush the write buffer if necessary,
or if using stage-1
> or it's a no-op. > - if IOMMU is in caching mode, flush the IOTLB caches. > > Best regards, > baolu
| |