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
