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

[Baolu Lu]

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()

?

In cache_tag_flush_non_present_range(),

- if IOMMU is not in caching mode, flush the write buffer if necessary,
  or it's a no-op.
- if IOMMU is in caching mode, flush the IOTLB caches.

Best regards,
baolu