crypto: x86/sha256 - autoload if SHA-NI detected

PATCH Superseded Eric Biggers 3 participants 4 messages Oct 29, 2023 – Nov 1, 2023

Subsystems (2)

x86 Crypto API

lore

Share & tools

lore

Eric Biggers

author

2023-10-29 05:16 UTC #0 lore raw

PATCH 1/1 crypto: x86/sha256 - autoload if SHA-NI detected

From: Eric Biggers <[email protected]>

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 <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 arch/x86/crypto/sha256_ssse3_glue.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

arch/x86/crypto/sha256_ssse3_glue.c +3 -2

index 4c0383a90e11..a135cf9baca3 100644
@@ -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 <[email protected]>
  *
  * 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

Signature · 1 line

2.42.0

Roxana Nicolescu

Reviewed-by on v1 1/1

2023-10-31 13:19 UTC #1 lore raw

Quoted 11 lines · Eric Biggers

From: Eric Biggers <[email protected]>
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.

Quoted 2 lines

Cc Roxana Nicolescu <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Indeed, it was an oversight.

Reviewed-by Roxana Nicolescu <[email protected]>

Quoted 50 lines

---
  arch/x86/crypto/sha256_ssse3_glue.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

arch/x86/crypto/sha256_ssse3_glue.c +3 -2

index 4c0383a90e11..a135cf9baca3 100644
@@ -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 <[email protected]>
   *
   * 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

Elliott, Robert (Servers)

2023-10-31 14:53 UTC #2 lore raw

Quoted 32 lines

-----Original Message-----
From: Roxana Nicolescu <[email protected]>
Sent: Tuesday, October 31, 2023 8:19 AM
To: Eric Biggers <[email protected]>; [email protected]
Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected
Quoted 11 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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.
Quoted 2 lines
Cc Roxana Nicolescu <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
Indeed, it was an oversight.
Reviewed-by Roxana Nicolescu <[email protected]>
Review
Quoted 5 lines
---
  arch/x86/crypto/sha256_ssse3_glue.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/arch/x86/crypto/sha256_ssse3_glue.c
Quoted 2 lines
index 4c0383a90e11..a135cf9baca3 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c

...

Quoted 3 lines

arch/x86/crypto/sha256_ssse3_glue.c line 48
Quoted 3 lines
  static const struct x86_cpu_id module_cpu_ids[] = {
arch/x86/crypto/sha256_ssse3_glue.c +1 -0
+	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

Quoted 6 lines

  	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);

Eric Biggers

author

2023-11-01 03:22 UTC #3 lore raw

Quoted 46 lines · Robert (Servers)

Quoted 32 lines
-----Original Message-----
From: Roxana Nicolescu <[email protected]>
Sent: Tuesday, October 31, 2023 8:19 AM
To: Eric Biggers <[email protected]>; [email protected]
Subject: Re: [PATCH] crypto: x86/sha256 - autoload if SHA-NI detected
Quoted 11 lines · Eric Biggers
From: Eric Biggers <[email protected]>
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.
Quoted 2 lines
Cc Roxana Nicolescu <[email protected]>
Signed-off-by Eric Biggers <[email protected]>
Indeed, it was an oversight.
Reviewed-by Roxana Nicolescu <[email protected]>
Review
Quoted 5 lines
---
  arch/x86/crypto/sha256_ssse3_glue.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/arch/x86/crypto/sha256_ssse3_glue.c
Quoted 2 lines
index 4c0383a90e11..a135cf9baca3 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c
...
Quoted 3 lines
arch/x86/crypto/sha256_ssse3_glue.c line 48
Quoted 3 lines
  static const struct x86_cpu_id module_cpu_ids[] = {
arch/x86/crypto/sha256_ssse3_glue.c +1 -0
+	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:

Review

Quoted 3 lines

(quoted diff) +3 -0

+#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