2022-11-25 04:37:39

by Herbert Xu

[permalink] [raw]
Subject: [v2 PATCH 2/9] crypto: api - Add crypto_tfm_ctx_dma

This patch adds the helpers crypto_tfm_ctx_aligned and
crypto_tfm_ctx_dma. The first aligns the tfm context to the
value cra_alignmask. The second sets the alignment according
to dma_cache_get_alignment();

This patch also moves crypto_tfm_ctx into algapi.h.

Signed-off-by: Herbert Xu <[email protected]>
---

include/crypto/algapi.h | 41 +++++++++++++++++++++++++++++++++++++++--
include/linux/crypto.h | 5 -----
2 files changed, 39 insertions(+), 7 deletions(-)

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))
+
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);
+}
+
+static inline unsigned int crypto_dma_align(void)
+{
+ return CRYPTO_DMA_ALIGN;
+}
+
+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());
}

static inline struct crypto_instance *crypto_tfm_alg_instance(
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 2324ab6f1846..5d1e961f810e 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -714,11 +714,6 @@ static inline void crypto_tfm_clear_flags(struct crypto_tfm *tfm, u32 flags)
tfm->crt_flags &= ~flags;
}

-static inline void *crypto_tfm_ctx(struct crypto_tfm *tfm)
-{
- return tfm->__crt_ctx;
-}
-
static inline unsigned int crypto_tfm_ctx_alignment(void)
{
struct crypto_tfm *tfm;


2022-11-25 11:34:48

by Catalin Marinas

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

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

2022-11-28 04:06:05

by Herbert Xu

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

On Fri, Nov 25, 2022 at 11:31:56AM +0000, Catalin Marinas wrote:
>
> 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).

Yes it's used by caam which needs it in a struct initialiser.

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

I'm trying to make the driver patches as robotic as possible.

We could always improve upon this with driver-specific patches
to change the struct initialiser to a run-time assignment to
improve things further.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt