| Message ID | 20231029051555.157720-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: x86/sha256 - autoload if SHA-NI detected | expand |
On 29/10/2023 06:15, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > The x86 SHA-256 module contains four implementations: SSSE3, AVX, AVX2, > and SHA-NI. Commit 1c43c0f1f84a ("crypto: x86/sha - load modules based > on CPU features") made the module be autoloaded when SSSE3, AVX, or AVX2 > is detected. The omission of SHA-NI appears to be an oversight, perhaps > because of the outdated file-level comment. This patch fixes this, > though in practice this makes no difference because SSSE3 is a subset of > the other three features anyway. Indeed, sha256_ni_transform() executes > SSSE3 instructions such as pshufb. > > Cc: Roxana Nicolescu <roxana.nicolescu@canonical.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> Indeed, it was an oversight. Reviewed-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> > --- > arch/x86/crypto/sha256_ssse3_glue.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c > index 4c0383a90e11..a135cf9baca3 100644 > --- a/arch/x86/crypto/sha256_ssse3_glue.c > +++ b/arch/x86/crypto/sha256_ssse3_glue.c > @@ -1,15 +1,15 @@ > /* > * Cryptographic API. > * > - * Glue code for the SHA256 Secure Hash Algorithm assembler > - * implementation using supplemental SSE3 / AVX / AVX2 instructions. > + * Glue code for the SHA256 Secure Hash Algorithm assembler implementations > + * using SSSE3, AVX, AVX2, and SHA-NI instructions. > * > * This file is based on sha256_generic.c > * > * Copyright (C) 2013 Intel Corporation. > * > * Author: > * Tim Chen <tim.c.chen@linux.intel.com> > * > * This program is free software; you can redistribute it and/or modify it > * under the terms of the GNU General Public License as published by the Free > @@ -38,20 +38,21 @@ > #include <crypto/sha2.h> > #include <crypto/sha256_base.h> > #include <linux/string.h> > #include <asm/cpu_device_id.h> > #include <asm/simd.h> > > asmlinkage void sha256_transform_ssse3(struct sha256_state *state, > const u8 *data, int blocks); > > static const struct x86_cpu_id module_cpu_ids[] = { > + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), > X86_MATCH_FEATURE(X86_FEATURE_AVX2, NULL), > X86_MATCH_FEATURE(X86_FEATURE_AVX, NULL), > X86_MATCH_FEATURE(X86_FEATURE_SSSE3, NULL), > {} > }; > MODULE_DEVICE_TABLE(x86cpu, module_cpu_ids); > > static int _sha256_update(struct shash_desc *desc, const u8 *data, > unsigned int len, sha256_block_fn *sha256_xform) > { > > base-commit: f2b88bab69c86d4dab2bfd25a0e741d7df411f7a
> -----Original Message----- > From: Roxana Nicolescu <roxana.nicolescu@canonical.com> > Sent: Tuesday, October 31, 2023 8:19 AM > To: Eric Biggers <ebiggers@kernel.org>; linux-crypto@vger.kernel.org > Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected > > On 29/10/2023 06:15, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > The x86 SHA-256 module contains four implementations: SSSE3, AVX, AVX2, > > and SHA-NI. Commit 1c43c0f1f84a ("crypto: x86/sha - load modules based > > on CPU features") made the module be autoloaded when SSSE3, AVX, or AVX2 > > is detected. The omission of SHA-NI appears to be an oversight, perhaps > > because of the outdated file-level comment. This patch fixes this, > > though in practice this makes no difference because SSSE3 is a subset of > > the other three features anyway. Indeed, sha256_ni_transform() executes > > SSSE3 instructions such as pshufb. > > > > Cc: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Indeed, it was an oversight. > > Reviewed-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > --- > > arch/x86/crypto/sha256_ssse3_glue.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/crypto/sha256_ssse3_glue.c > b/arch/x86/crypto/sha256_ssse3_glue.c > > index 4c0383a90e11..a135cf9baca3 100644 > > --- a/arch/x86/crypto/sha256_ssse3_glue.c ... > > > > static const struct x86_cpu_id module_cpu_ids[] = { > > + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), Unless something else has changed, this needs to be inside ifdefs, as discovered in the proposed patch series last year: for sha1_sse3_glue.c: #ifdef CONFIG_AS_SHA1_NI X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), #endif for sha256_sse3_glue.c: +#ifdef CONFIG_AS_SHA256_NI + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), +#endif > > X86_MATCH_FEATURE(X86_FEATURE_AVX2, NULL), > > X86_MATCH_FEATURE(X86_FEATURE_AVX, NULL), > > X86_MATCH_FEATURE(X86_FEATURE_SSSE3, NULL), > > {} > > }; > > MODULE_DEVICE_TABLE(x86cpu, module_cpu_ids);
On Tue, Oct 31, 2023 at 02:52:53PM +0000, Elliott, Robert (Servers) wrote: > > -----Original Message----- > > From: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > Sent: Tuesday, October 31, 2023 8:19 AM > > To: Eric Biggers <ebiggers@kernel.org>; linux-crypto@vger.kernel.org > > Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected > > > > On 29/10/2023 06:15, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > The x86 SHA-256 module contains four implementations: SSSE3, AVX, AVX2, > > > and SHA-NI. Commit 1c43c0f1f84a ("crypto: x86/sha - load modules based > > > on CPU features") made the module be autoloaded when SSSE3, AVX, or AVX2 > > > is detected. The omission of SHA-NI appears to be an oversight, perhaps > > > because of the outdated file-level comment. This patch fixes this, > > > though in practice this makes no difference because SSSE3 is a subset of > > > the other three features anyway. Indeed, sha256_ni_transform() executes > > > SSSE3 instructions such as pshufb. > > > > > > Cc: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > Indeed, it was an oversight. > > > > Reviewed-by: Roxana Nicolescu <roxana.nicolescu@canonical.com> > > > --- > > > arch/x86/crypto/sha256_ssse3_glue.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/crypto/sha256_ssse3_glue.c > > b/arch/x86/crypto/sha256_ssse3_glue.c > > > index 4c0383a90e11..a135cf9baca3 100644 > > > --- a/arch/x86/crypto/sha256_ssse3_glue.c > ... > > > > > > static const struct x86_cpu_id module_cpu_ids[] = { > > > + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), > > Unless something else has changed, this needs to be inside ifdefs, as discovered > in the proposed patch series last year: > > for sha1_sse3_glue.c: > #ifdef CONFIG_AS_SHA1_NI > X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), > #endif > > for sha256_sse3_glue.c: > +#ifdef CONFIG_AS_SHA256_NI > + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), > +#endif Right, thanks for pointing that out. It compiles either way, but we shouldn't autoload on SHA-NI when the code using SHA-NI isn't being built. Sent out v2 with this fixed. - Eric
diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c index 4c0383a90e11..a135cf9baca3 100644 --- a/arch/x86/crypto/sha256_ssse3_glue.c +++ b/arch/x86/crypto/sha256_ssse3_glue.c @@ -1,15 +1,15 @@ /* * Cryptographic API. * - * Glue code for the SHA256 Secure Hash Algorithm assembler - * implementation using supplemental SSE3 / AVX / AVX2 instructions. + * Glue code for the SHA256 Secure Hash Algorithm assembler implementations + * using SSSE3, AVX, AVX2, and SHA-NI instructions. * * This file is based on sha256_generic.c * * Copyright (C) 2013 Intel Corporation. * * Author: * Tim Chen <tim.c.chen@linux.intel.com> * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the Free @@ -38,20 +38,21 @@ #include <crypto/sha2.h> #include <crypto/sha256_base.h> #include <linux/string.h> #include <asm/cpu_device_id.h> #include <asm/simd.h> asmlinkage void sha256_transform_ssse3(struct sha256_state *state, const u8 *data, int blocks); static const struct x86_cpu_id module_cpu_ids[] = { + X86_MATCH_FEATURE(X86_FEATURE_SHA_NI, NULL), X86_MATCH_FEATURE(X86_FEATURE_AVX2, NULL), X86_MATCH_FEATURE(X86_FEATURE_AVX, NULL), X86_MATCH_FEATURE(X86_FEATURE_SSSE3, NULL), {} }; MODULE_DEVICE_TABLE(x86cpu, module_cpu_ids); static int _sha256_update(struct shash_desc *desc, const u8 *data, unsigned int len, sha256_block_fn *sha256_xform) {