RE: [PATCH 02/12] iommu/vt-d: Add cache tag invalidation helpers

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Monday, April 8, 2024 10:54 AM
> 
> On 4/8/24 10:33 AM, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Sunday, April 7, 2024 1:33 PM
> >>
> >> On 3/28/24 3:39 PM, Tian, Kevin wrote:
> >>>> From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> >>>> 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