Re: [dm-devel] [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES implementation

From: Eric Biggers <ebiggers@kernel.org>
To: "Chang S. Bae" <chang.seok.bae@intel.com>
Cc: x86@kernel.org, herbert@gondor.apana.org.au,
	"David S. Miller" <davem@davemloft.net>,
	ardb@kernel.org, dave.hansen@linux.intel.com,
	dan.j.williams@intel.com, linux-kernel@vger.kernel.org,
	mingo@kernel.org, lalithambika.krishnakumar@intel.com,
	dm-devel@redhat.com, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	linux-crypto@vger.kernel.org, luto@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	bernie.keany@intel.com, tglx@linutronix.de, bp@suse.de,
	gmazyland@gmail.com, charishma1.gairuboyina@intel.com
Subject: Re: [dm-devel] [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES implementation
Date: Fri, 5 May 2023 23:27:30 +0000	[thread overview]
Message-ID: <ZFWQ4sZEVu/LHq+Q@gmail.com> (raw)
In-Reply-To: <20230410225936.8940-11-chang.seok.bae@intel.com>

On Mon, Apr 10, 2023 at 03:59:34PM -0700, Chang S. Bae wrote:
> Refactor the common C code to avoid code duplication. The AES-NI code uses
> it with a function pointer argument to call back the AES-NI assembly code.
> So will the AES-KL code.

Actually, the AES-NI XTS glue code currently makes direct calls to the assembly
code.  This patch changes it to make indirect calls.  Indirect calls are very
expensive these days, partly due to all the speculative execution mitigations.
So this patch likely causes a performance regression.  How about making
xts_crypt_common() and xts_setkey_common() be inline functions?

Another issue with having the above be exported symbols is that their names are
too generic, so they could easily collide with another symbols in the kernel.
To be exported symbols, they would need something x86-specific in their names.

>  arch/x86/crypto/Makefile           |   2 +-
>  arch/x86/crypto/aes-intel_asm.S    |  26 ++++
>  arch/x86/crypto/aes-intel_glue.c   | 127 ++++++++++++++++
>  arch/x86/crypto/aes-intel_glue.h   |  44 ++++++
>  arch/x86/crypto/aesni-intel_asm.S  |  58 +++----
>  arch/x86/crypto/aesni-intel_glue.c | 235 +++++++++--------------------
>  arch/x86/crypto/aesni-intel_glue.h |  17 +++

It's confusing having aes-intel, aesni-intel, *and* aeskl-intel.  Maybe call the
first one "aes-helpers" or "aes-common" instead?

> +struct aes_xts_ctx {
> +	u8 raw_tweak_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR;
> +	u8 raw_crypt_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR;
> +};

This struct does not make sense.  It should look like:

    struct aes_xts_ctx {
            struct crypto_aes_ctx tweak_ctx AES_ALIGN_ATTR;
            struct crypto_aes_ctx crypt_ctx AES_ALIGN_ATTR;
    };

The runtime alignment to a 16-byte boundary should happen when translating the
raw crypto_skcipher_ctx() into the pointer to the aes_xts_ctx.  It should not
happen when accessing each individual field in the aes_xts_ctx.

>  /*
> - * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> - *                   unsigned int key_len)
> + * int _aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
> + *                    unsigned int key_len)
>   */

It's conventional to use two leading underscores, not one.

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel