| Message ID | 20210610134459.28541-1-tianjia.zhang@linux.alibaba.com |
|---|---|
| Headers | show |
| Series | Introduce x86 assembler accelerated implementation for SM4 algorithm | expand |
On Thu, Jun 10, 2021 at 09:44:57PM +0800, Tianjia Zhang wrote: > Take the existing small footprint and mostly time invariant C code It is using an S-box without any prefetching. That doesn't look very "time invariant" to me. > diff --git a/lib/crypto/sm4.c b/lib/crypto/sm4.c > new file mode 100644 > index 000000000000..cbdd14a254d0 [..] > +/** > + * crypto_sm4_expand_key - Expands the SM4 key as described in GB/T 32907-2016 > + * @ctx: The location where the computed key will be stored. > + * @in_key: The supplied key. > + * @key_len: The length of the supplied key. > + * > + * Returns 0 on success. The function fails only if an invalid key size (or > + * pointer) is supplied. > + */ > +int crypto_sm4_expand_key(struct crypto_sm4_ctx *ctx, const u8 *in_key, > + unsigned int key_len) [...] > +/** > + * crypto_sm4_do_crypt - Encrypt or decrypt a single SM4 block > + * @rk: The rkey_enc for encrypt or rkey_dec for decrypt > + * @out: Buffer to store output data > + * @in: Buffer containing the input data > + */ > +void crypto_sm4_do_crypt(const u32 *rk, u8 *out, const u8 *in) Calling these "sm4_expandkey()" and "sm4_crypt_block()" would be more consistent with the other lib/crypto/ functions such as the AES ones. The other lib/crypto/ functions don't have a "crypto_" prefix, as that is used for functions related to the traditional crypto API rather than the library API. - Eric
On Thu, Jun 10, 2021 at 09:44:59PM +0800, Tianjia Zhang wrote: > This patch adds AES-NI/AVX/x86_64 assembler implementation of SM4 > block cipher. Through two affine transforms, we can use the AES > S-Box to simulate the SM4 S-Box to achieve the effect of instruction > acceleration. > Benchmark results, please. Also, is this passing the self-tests, including the fuzz tests? > +/* > + * void sm4_aesni_avx_expand_key(const u8 *key, u32 *rk_enc, > + * u32 *rk_dec, const u32 *fk, const u32 *ck); > + */ > +SYM_FUNC_START(sm4_aesni_avx_expand_key) > + /* input: > + * %rdi: 128-bit key > + * %rsi: rkey_enc > + * %rdx: rkey_dec > + * %rcx: fk array > + * %r8: ck array > + */ > + FRAME_BEGIN Key expansion isn't performance-critical. Can the C library version be used, or does the key need to be expanded in a way specific to this x86 implementation? > +/* > + * void sm4_aesni_avx_crypt4(const u32 *rk, u8 *dst, > + * const u8 *src, int nblocks) > + */ > +SYM_FUNC_START(sm4_aesni_avx_crypt4) > + /* input: > + * %rdi: round key array, CTX > + * %rsi: dst (1..4 blocks) > + * %rdx: src (1..4 blocks) > + * %rcx: num blocks (1..4) > + */ > + FRAME_BEGIN [...] > +static void sm4_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in) > +{ > + const struct crypto_sm4_ctx *ctx = crypto_tfm_ctx(tfm); > + > + if (crypto_simd_usable()) { > + kernel_fpu_begin(); > + sm4_aesni_avx_crypt4(ctx->rkey_enc, out, in, 1); > + kernel_fpu_end(); > + } else > + crypto_sm4_do_crypt(ctx->rkey_enc, out, in); > +} > + > +static void sm4_decrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in) > +{ > + const struct crypto_sm4_ctx *ctx = crypto_tfm_ctx(tfm); > + > + if (crypto_simd_usable()) { > + kernel_fpu_begin(); > + sm4_aesni_avx_crypt4(ctx->rkey_dec, out, in, 1); > + kernel_fpu_end(); > + } else > + crypto_sm4_do_crypt(ctx->rkey_dec, out, in); > +} Your assembly code appears to handle encrypting up to 4 blocks at a time. However you have only wired this up to the "cipher" API which does 1 block at a time. Is this intentional? What are your performance results with real-world chaining modes like XTS, and do you plan to implement any of these modes directly? > + > +static struct crypto_alg sm4_asm_alg = { > + .cra_name = "sm4", > + .cra_driver_name = "sm4-asm", In arch/x86/crypto/, "-asm" usually means a vanilla x86 assembly implementation without any AES-NI, SSE, AVX, etc. instructions. Calling this something like "sm4-aesni-avx" would make more sense. (Or is it actually avx2, not avx?) > +config CRYPTO_SM4_AESNI_AVX_X86_64 > + tristate "SM4 cipher algorithm (x86_64/AES-NI/AVX)" > + depends on X86 && 64BIT > + select CRYPTO_SKCIPHER > + select CRYPTO_SIMD > + select CRYPTO_ALGAPI > + select CRYPTO_LIB_SM4 As-is, neither CRYPTO_SKCIPHER nor CRYPTO_SIMD needs to be selected here. > + help > + SM4 cipher algorithms (OSCCA GB/T 32907-2016) (x86_64/AES-NI/AVX). > + > + SM4 (GBT.32907-2016) is a cryptographic standard issued by the > + Organization of State Commercial Administration of China (OSCCA) > + as an authorized cryptographic algorithms for the use within China. > + > + SMS4 was originally created for use in protecting wireless > + networks, and is mandated in the Chinese National Standard for > + Wireless LAN WAPI (Wired Authentication and Privacy Infrastructure) > + (GB.15629.11-2003). > + > + The latest SM4 standard (GBT.32907-2016) was proposed by OSCCA and > + standardized through TC 260 of the Standardization Administration > + of the People's Republic of China (SAC). > + > + The input, output, and key of SMS4 are each 128 bits. > + > + See also: <https://eprint.iacr.org/2008/329.pdf> > + > + If unsure, say N. This is the help text for the x86 implementation specifically. Please don't have boilerplate text about the algorithm here; that already exists for the generic implementation. The text should explain about the x86 implementation. - Eric
Hi Eric, On 6/11/21 7:19 AM, Eric Biggers wrote: > On Thu, Jun 10, 2021 at 09:44:57PM +0800, Tianjia Zhang wrote: >> Take the existing small footprint and mostly time invariant C code > > It is using an S-box without any prefetching. That doesn't look very > "time invariant" to me. > Thanks for your suggestion, will do in the next version patch. >> diff --git a/lib/crypto/sm4.c b/lib/crypto/sm4.c >> new file mode 100644 >> index 000000000000..cbdd14a254d0 > [..] >> +/** >> + * crypto_sm4_expand_key - Expands the SM4 key as described in GB/T 32907-2016 >> + * @ctx: The location where the computed key will be stored. >> + * @in_key: The supplied key. >> + * @key_len: The length of the supplied key. >> + * >> + * Returns 0 on success. The function fails only if an invalid key size (or >> + * pointer) is supplied. >> + */ >> +int crypto_sm4_expand_key(struct crypto_sm4_ctx *ctx, const u8 *in_key, >> + unsigned int key_len) > [...] >> +/** >> + * crypto_sm4_do_crypt - Encrypt or decrypt a single SM4 block >> + * @rk: The rkey_enc for encrypt or rkey_dec for decrypt >> + * @out: Buffer to store output data >> + * @in: Buffer containing the input data >> + */ >> +void crypto_sm4_do_crypt(const u32 *rk, u8 *out, const u8 *in) > > Calling these "sm4_expandkey()" and "sm4_crypt_block()" would be more consistent > with the other lib/crypto/ functions such as the AES ones. The other > lib/crypto/ functions don't have a "crypto_" prefix, as that is used for > functions related to the traditional crypto API rather than the library API. Ditto. thanks for pointing it out. > > - Eric > Best regards, Tianjia
Hi Eric, On 6/11/21 7:27 AM, Eric Biggers wrote: > On Thu, Jun 10, 2021 at 09:44:59PM +0800, Tianjia Zhang wrote: >> This patch adds AES-NI/AVX/x86_64 assembler implementation of SM4 >> block cipher. Through two affine transforms, we can use the AES >> S-Box to simulate the SM4 S-Box to achieve the effect of instruction >> acceleration. >> > > Benchmark results, please. > > Also, is this passing the self-tests, including the fuzz tests? > I will provide this information in the next version. >> +/* >> + * void sm4_aesni_avx_expand_key(const u8 *key, u32 *rk_enc, >> + * u32 *rk_dec, const u32 *fk, const u32 *ck); >> + */ >> +SYM_FUNC_START(sm4_aesni_avx_expand_key) >> + /* input: >> + * %rdi: 128-bit key >> + * %rsi: rkey_enc >> + * %rdx: rkey_dec >> + * %rcx: fk array >> + * %r8: ck array >> + */ >> + FRAME_BEGIN > > Key expansion isn't performance-critical. Can the C library version be used, or > does the key need to be expanded in a way specific to this x86 implementation? > It can be replaced by a common implementation of C library. For expand key that are not called frequently, the optimization of a specific instruction set does not bring much benefit. Of course, it is possible to delete this implementation. >> +/* >> + * void sm4_aesni_avx_crypt4(const u32 *rk, u8 *dst, >> + * const u8 *src, int nblocks) >> + */ >> +SYM_FUNC_START(sm4_aesni_avx_crypt4) >> + /* input: >> + * %rdi: round key array, CTX >> + * %rsi: dst (1..4 blocks) >> + * %rdx: src (1..4 blocks) >> + * %rcx: num blocks (1..4) >> + */ >> + FRAME_BEGIN > [...] > >> +static void sm4_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in) >> +{ >> + const struct crypto_sm4_ctx *ctx = crypto_tfm_ctx(tfm); >> + >> + if (crypto_simd_usable()) { >> + kernel_fpu_begin(); >> + sm4_aesni_avx_crypt4(ctx->rkey_enc, out, in, 1); >> + kernel_fpu_end(); >> + } else >> + crypto_sm4_do_crypt(ctx->rkey_enc, out, in); >> +} >> + >> +static void sm4_decrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in) >> +{ >> + const struct crypto_sm4_ctx *ctx = crypto_tfm_ctx(tfm); >> + >> + if (crypto_simd_usable()) { >> + kernel_fpu_begin(); >> + sm4_aesni_avx_crypt4(ctx->rkey_dec, out, in, 1); >> + kernel_fpu_end(); >> + } else >> + crypto_sm4_do_crypt(ctx->rkey_dec, out, in); >> +} > > Your assembly code appears to handle encrypting up to 4 blocks at a time. > However you have only wired this up to the "cipher" API which does 1 block at a > time. Is this intentional? > > What are your performance results with real-world chaining modes like XTS, and > do you plan to implement any of these modes directly? > This implementation is intentional. First, a general block encryption is provided. There is no obvious performance improvement in this implementation. The key to optimization is to make full use of parallel four blocks encryption at a time. This is still under development, and I will continue to implement things like XTS in the future. Optimization of such specific modes. >> + >> +static struct crypto_alg sm4_asm_alg = { >> + .cra_name = "sm4", >> + .cra_driver_name = "sm4-asm", > > In arch/x86/crypto/, "-asm" usually means a vanilla x86 assembly implementation > without any AES-NI, SSE, AVX, etc. instructions. Calling this something like > "sm4-aesni-avx" would make more sense. (Or is it actually avx2, not avx?) > will do in next version patch. >> +config CRYPTO_SM4_AESNI_AVX_X86_64 >> + tristate "SM4 cipher algorithm (x86_64/AES-NI/AVX)" >> + depends on X86 && 64BIT >> + select CRYPTO_SKCIPHER >> + select CRYPTO_SIMD >> + select CRYPTO_ALGAPI >> + select CRYPTO_LIB_SM4 > > As-is, neither CRYPTO_SKCIPHER nor CRYPTO_SIMD needs to be selected here. > ditto. >> + help >> + SM4 cipher algorithms (OSCCA GB/T 32907-2016) (x86_64/AES-NI/AVX). >> + >> + SM4 (GBT.32907-2016) is a cryptographic standard issued by the >> + Organization of State Commercial Administration of China (OSCCA) >> + as an authorized cryptographic algorithms for the use within China. >> + >> + SMS4 was originally created for use in protecting wireless >> + networks, and is mandated in the Chinese National Standard for >> + Wireless LAN WAPI (Wired Authentication and Privacy Infrastructure) >> + (GB.15629.11-2003). >> + >> + The latest SM4 standard (GBT.32907-2016) was proposed by OSCCA and >> + standardized through TC 260 of the Standardization Administration >> + of the People's Republic of China (SAC). >> + >> + The input, output, and key of SMS4 are each 128 bits. >> + >> + See also: <https://eprint.iacr.org/2008/329.pdf> >> + >> + If unsure, say N. > > This is the help text for the x86 implementation specifically. Please don't > have boilerplate text about the algorithm here; that already exists for the > generic implementation. The text should explain about the x86 implementation. > ditto. > - Eric > Thanks for your suggestion. Cheers, Tianjia
> lib/crypto/Kconfig | 3 + > lib/crypto/Makefile | 3 + > lib/crypto/sm4.c | 184 +++++++++++++++++++++++++++++++++++++++++++ If this is only used by the crypto API, it does not belong in lib/crypto. I understand you want fallback generic code for the SIMD implementation, but we've generally done that in crypto/ when the use case is only the crypto API. Can you move this to the right place?
Hi Jason, On 3/1/22 6:34 PM, Jason A. Donenfeld wrote: >> lib/crypto/Kconfig | 3 + >> lib/crypto/Makefile | 3 + >> lib/crypto/sm4.c | 184 +++++++++++++++++++++++++++++++++++++++++++ > > If this is only used by the crypto API, it does not belong in > lib/crypto. I understand you want fallback generic code for the SIMD > implementation, but we've generally done that in crypto/ when the use > case is only the crypto API. Can you move this to the right place? This is not only used by the crypto API, but also used for SIMD acceleration under the x86 and arm architectures, mainly for processing the remaining blocks after SIMD acceleration. In general, the performance of SIMD processing a single block is not as good as that of general software implementations. Kind regards, Tianjia
On Tue, Mar 1, 2022 at 2:22 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > You additional export symbols of those SIMD implementations in > arch/crypto/, which is not correct either, since nothing in the tree > uses those symbols. Please remove those EXPORT_SYMBOL directives as > well. Those functions can be static, and do not need to be declared in > the .h file. Actually, this part isn't quite so, because you share the avx implementation in the avx2 implementation. However, > Yes, and those accelerated implementations are part of the crypto API, > and are not used by anything except the crypto API. Hence this should > be in crypto/, just like everything else that is /only/ used for the > cryto API. lib/crypto/ is for in-kernel users of crypto via normal > code paths. sm4.c does not belong in lib/crypto/ and should be moved. This still holds. Jason
Hi Tianjia, On Wed, Mar 2, 2022 at 3:23 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Mar 02, 2022 at 01:26:13AM +0100, Jason A. Donenfeld wrote: > > On Wed, Mar 2, 2022 at 1:24 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > On Tue, Mar 01, 2022 at 11:34:28AM +0100, Jason A. Donenfeld wrote: > > > > > lib/crypto/Kconfig | 3 + > > > > > lib/crypto/Makefile | 3 + > > > > > lib/crypto/sm4.c | 184 +++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > If this is only used by the crypto API, it does not belong in > > > > lib/crypto. > > > > > > Nope there is no such rule. lib/crypto is fine if you're adding > > > code that is shared between crypto and arch/*/crypto. > > > > The sprawling madness continues then... Noted. > > I think it would make more sense for this code to be in crypto/, for the reason > that Jason gave. Were you planning on submitting a patch for this? Thanks, Jason
Hi Herbert, Are you willing to consider the views of Eric and me? Or is this a hard nack from you? Thanks, Jason
On Sun, Mar 13, 2022 at 08:40:00PM -0600, Jason A. Donenfeld wrote: > Hi Herbert, > > Are you willing to consider the views of Eric and me? Or is this a > hard nack from you? Please present your patch to move the code with the reasoning for the move and then I can consider it. Cheers,
This patchset extracts the public SM4 algorithm as a separate library, and introduces an accelerated implementation of the instruction set on x86, and will also introduce more mode-specific accelerated implementations later. Tianjia Zhang (3): crypto: sm4 - create SM4 library based on sm4 generic code crypto: arm64/sm4-ce - Make dependent on sm4 library instead of sm4-generic crypto: x86/sm4 - add AES-NI/AVX/x86_64 assembler implementation arch/arm64/crypto/Kconfig | 2 +- arch/arm64/crypto/sm4-ce-glue.c | 14 +- arch/x86/crypto/Makefile | 3 + arch/x86/crypto/sm4-aesni-avx-asm_64.S | 339 +++++++++++++++++++++++++ arch/x86/crypto/sm4_aesni_avx_glue.c | 115 +++++++++ crypto/Kconfig | 30 +++ crypto/sm4_generic.c | 164 +----------- include/crypto/sm4.h | 25 +- lib/crypto/Kconfig | 3 + lib/crypto/Makefile | 3 + lib/crypto/sm4.c | 184 ++++++++++++++ 11 files changed, 718 insertions(+), 164 deletions(-) create mode 100644 arch/x86/crypto/sm4-aesni-avx-asm_64.S create mode 100644 arch/x86/crypto/sm4_aesni_avx_glue.c create mode 100644 lib/crypto/sm4.c