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]

Date: Mon, 8 Apr 2024 03:14:37 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
 Gunthorpe" <jgg@...pe.ca>
CC: "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

> From: Baolu Lu <baolu.lu@...ux.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@...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()

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