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

[Will Deacon]

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.

> 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!

Will