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

[Baolu Lu <baolu.lu@linux.intel.com>]

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

?

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