[PATCH] arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()

[Ard Biesheuvel]

On Wed, 24 Aug 2022 at 13:23, Will Deacon <will at kernel.org> wrote:
>
> Hi Ard,
>
> Cheers for having a look.
>
> On Wed, Aug 24, 2022 at 11:58:46AM +0200, Ard Biesheuvel wrote:
> > On Tue, 23 Aug 2022 at 14:21, Will Deacon <will at kernel.org> wrote:
> > >
> > > arch_dma_prep_coherent() is called when preparing a non-cacheable region
> > > for a consistent DMA buffer allocation. Since the buffer pages may
> > > previously have been written via a cacheable mapping and consequently
> > > allocated as dirty cachelines, the purpose of this function is to remove
> > > these dirty lines from the cache, writing them back so that the
> > > non-coherent device is able to see them.
> > >
> > > On arm64, this operation can be achieved with a clean to the point of
> > > coherency; a subsequent invalidation is not required and serves little
> > > purpose in the presence of a cacheable alias (e.g. the linear map),
> > > since clean lines can be speculatively fetched back into the cache after
> > > the invalidation operation has completed.
> > >
> > > Relax the cache maintenance in arch_dma_prep_coherent() so that only a
> > > clean, and not a clean-and-invalidate operation is performed.
> > >
> > > Cc: Catalin Marinas <catalin.marinas at arm.com>
> > > Cc: Mark Rutland <mark.rutland at arm.com>
> > > Cc: Robin Murphy <robin.murphy at arm.com>
> > > Cc: Christoph Hellwig <hch at lst.de>
> > > Cc: Ard Biesheuvel <ardb at kernel.org>
> > > Signed-off-by: Will Deacon <will at kernel.org>
> > > ---
> > >
> > > I'm slightly wary about this change as other architectures seem to do
> > > clean+invalidate here, but I'd like to hear what others think in any
> > > case.
> > >
> >
> > Assuming that the architecture never permits non-cacheable accesses to
> > hit in the caches [i.e., if the behavior the ARM ARM describes in
> > 'Behavior of caches at reset' is the only exception],
>
> As long as the allocated lines are clean, then the non-cacheable access
> won't hit.
>

If this is guaranteed to remain true even in the presence of
mismatched attribute mappings of the same physical region, then we
should be fine, I suppose.

But if not, shouldn't invalidating the lines substantially reduce the
likelihood of hitting this issue? And even if they /could/ be
speculated back in, how likely is that to happen if the linear alias
VA is not exposed anywhere?

> > then I agree that invalidation should make no difference. However, given
> > the fact that we are dealing with mismatched attribute mappings here,
> > retaining the invalidate seems like the safer bet. (IIRC, the
> > justification for permitting the linear map's mismatched attributes is
> > that we never access the region via its linear address, and so we are
> > unlikely to be affected by any conflicts. Purging cachelines that cover
> > this region seems like a sensible thing to do in that respect.)
>
> It's certainly harmless to keep the invalidation, but I also don't think
> it's serving any real purpose either given that the clean lines can be
> pulled straight back in by speculative accesses through the linear map.
>
> > Why are you proposing to change this?
>
> Some hardware folks were asking me why we had the invalidation here and
> then I realised that it's not necessary. An alternative to removing it
> would be to add a comment saying it's not needed, but it's a bit weird
> writing that!
>

I don't disagree with that. If we weren't violating the ARM ARM by
having two live mappings of the same physical region with mismatched
attributes, I wouldn't be as suspicious.