Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC_MINALIGN

[Catalin Marinas]

On Tue, Apr 12, 2022 at 05:40:58PM +0800, Herbert Xu wrote:
> On Tue, Apr 12, 2022 at 10:32:58AM +0100, Catalin Marinas wrote:
> > At a quick grep, most cra_alignmask values are currently 15 or smaller.
> > I'm not convinced the driver needs to know about the CPU cache
> > alignment. We could set cra_alignmask to CRYPTO_MINALIGN but that would
> > incur unnecessary overhead via function like setkey_unaligned() when the
> > arch_kmalloc_minalign() was already sufficient for DMA safety.
> > 
> > Maybe I miss some use-cases or I focus too much on DMA safety.
> 
> The alignment was never designed for DMA.  It's mainly for CPUs
> that provide accelerated instructions that require a certain
> amount of alignment, most commonly 16 bytes.
> 
> Therefore CRYPTO_MINALIGN was never meant to be a guarantee for
> DMA alignment.  Any drivers relying on this is simply broken.

Thanks for the clarification (the comment in crypto.h implies that it
covers this aspect as well).

> I understand that on ARM for historical reasons you have had a
> situation that CRYPTO_MINALIGN happened to be large enough to
> guarantee DMA alignment.  I totally agree with your patch to
> try to fix this but it should not penalise other architectures
> and raise the CRYPTO_MINALIGN unnecessarily.

This series does not penalise any architecture. It doesn't even make
arm64 any worse than it currently is.

For arm64, before the series:

  ARCH_DMA_MINALIGN == 128
  ARCH_KMALLOC_MINALIGN == 128
  CRYPTO_MINALIGN == 128

After the series, on arm64:

  ARCH_DMA_MINALIGN == 128
  ARCH_KMALLOC_MINALIGN == 64
  CRYPTO_MINALIGN == 128

For x86, before the series:

  ARCH_DMA_MINALIGN undefined
  ARCH_KMALLOC_MINALIGN == 8
  CRYPTO_MINALIGN == 8

After the series, on x86 (or any architecture that doesn't define
ARCH_DMA_MINALIGN):

  ARCH_DMA_MINALIGN == 8
  ARCH_KMALLOC_MINALIGN == 8
  CRYPTO_MINALIGN == 8

For other architectures that define ARCH_DMA_MINALIGN to 'N' but do not
define ARCH_KMALLOC_MINALIGN, before and after this series:

  ARCH_DMA_MINALIGN == N
  ARCH_KMALLOC_MINALIGN == N
  CRYPTO_MINALIGN == N

> I think if CRYPTO_MINALIGN makes those drivers work then so
> should cra_alignmask.  And that would be a relatively easy
> patch to do.

Yes, the patch would be simple, subject to figuring out which drivers
and what alignment they actually need (any help appreciated).

There are already arm64 vendor kernels that change ARCH_DMA_MINALIGN to
64, hence ARCH_KMALLOC_MINALIGN and CRYPTO_MINALIGN become 64. We also
discussed a Kconfig option for this in the past. Would that have broken
any crypto drivers that rely on a strict 128 byte alignment?

Thanks.

-- 
Catalin