Re: [v2 PATCH 2/9] crypto: api - Add crypto_tfm_ctx_dma

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Herbert,

Thanks for putting this together. I'll try to go through the series but
my crypto knowledge is fairly limited.

On Fri, Nov 25, 2022 at 12:36:31PM +0800, Herbert Xu wrote:
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index f50c5d1725da..4c99eb66e654 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -7,6 +7,7 @@
>  #ifndef _CRYPTO_ALGAPI_H
>  #define _CRYPTO_ALGAPI_H
>  
> +#include <asm/cache.h>
>  #include <linux/align.h>
>  #include <linux/crypto.h>
>  #include <linux/kconfig.h>
> @@ -25,6 +26,14 @@
>  #define MAX_CIPHER_BLOCKSIZE		16
>  #define MAX_CIPHER_ALIGNMASK		15
>  
> +#ifdef ARCH_DMA_MINALIGN
> +#define CRYPTO_DMA_ALIGN ARCH_DMA_MINALIGN
> +#else
> +#define CRYPTO_DMA_ALIGN CRYPTO_MINALIGN
> +#endif
> +
> +#define CRYPTO_DMA_PADDING ((CRYPTO_DMA_ALIGN - 1) & ~(CRYPTO_MINALIGN - 1))

Is the CRYPTO_DMA_PADDING used anywhere? I couldn't find it in this
series and I'd rather drop it, together with CRYPTO_DMA_ALIGN (see
below).

> +
>  struct crypto_aead;
>  struct crypto_instance;
>  struct module;
> @@ -189,10 +198,38 @@ static inline void crypto_xor_cpy(u8 *dst, const u8 *src1, const u8 *src2,
>  	}
>  }
>  
> +static inline void *crypto_tfm_ctx(struct crypto_tfm *tfm)
> +{
> +	return tfm->__crt_ctx;
> +}
> +
> +static inline void *crypto_tfm_ctx_align(struct crypto_tfm *tfm,
> +					 unsigned int align)
> +{
> +	if (align <= crypto_tfm_ctx_alignment())
> +		align = 1;
> +
> +	return PTR_ALIGN(crypto_tfm_ctx(tfm), align);
> +}
> +
>  static inline void *crypto_tfm_ctx_aligned(struct crypto_tfm *tfm)
>  {
> -	return PTR_ALIGN(crypto_tfm_ctx(tfm),
> -			 crypto_tfm_alg_alignmask(tfm) + 1);
> +	return crypto_tfm_ctx_align(tfm, crypto_tfm_alg_alignmask(tfm) + 1);
>+}

I had an attempt to make crypto_tfm_alg_alignmask() the larger of the
cra_alignmask and ARCH_DMA_MINALIGN but for some reason the kernel
started to panic, so I gave up.

> +
> +static inline unsigned int crypto_dma_align(void)
> +{
> +	return CRYPTO_DMA_ALIGN;
> +}

We have a generic dma_get_cache_alignment() function which currently is
either 1 or ARCH_DMA_MINALIGN, if the latter is defined. My plan is to
make eventually make this dynamic based on the actual cache line size
(on most arm64 systems it would be 64 rather than 128). So could you use
this instead of defining a CRYPTO_DMA_ALIGN? The only difference would
be that dma_get_cache_alignment() returns 1 rather than
ARCH_KMALLOC_MINALIGN if ARCH_DMA_MINALIGN is not defined, but I don't
think that's an issue.

> +
> +static inline unsigned int crypto_dma_padding(void)
> +{
> +	return (crypto_dma_align() - 1) & ~(crypto_tfm_ctx_alignment() - 1);
> +}
> +
> +static inline void *crypto_tfm_ctx_dma(struct crypto_tfm *tfm)
> +{
> +	return crypto_tfm_ctx_align(tfm, crypto_dma_align());
>  }

These would need to cope with crypto_dma_align() < ARCH_KMALLOC_MINALIGN.
I think that's fine, the padding will be 0 if crypto_dma_align() is 1.

-- 
Catalin