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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d90bb2f-ea04-4492-b1d4-1bebe54b8ca2@linux.intel.com>
Date: Mon, 8 Apr 2024 10:53:54 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: "Tian, Kevin" <kevin.tian@...el.com>, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 Jason Gunthorpe <jgg@...pe.ca>
Cc: baolu.lu@...ux.intel.com, "Zhang, Tina" <tina.zhang@...el.com>,
 "Liu, Yi L" <yi.l.liu@...el.com>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/12] iommu/vt-d: Add cache tag invalidation helpers

On 4/8/24 10:33 AM, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@...ux.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@...ux.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