| Message ID | 13353.1564635114@turing-police (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | linux-next 20190731 - aegis128-core.c fails to build | expand |
(+ Arnd) On Thu, 1 Aug 2019 at 07:52, Valdis Klētnieks <valdis.kletnieks@vt.edu> wrote: > > The recent NEON SIMD patches break the build if CONFIG_CRYPTO_AEGIS128_SIMD isn't set: > > MODPOST 558 modules > ERROR: "crypto_aegis128_decrypt_chunk_simd" [crypto/aegis128.ko] undefined! > ERROR: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > ERROR: "crypto_aegis128_encrypt_chunk_simd" [crypto/aegis128.ko] undefined! > make[1]: *** [scripts/Makefile.modpost:105: modules-modpost] Error 1 > make: *** [Makefile:1299: modules] Error 2 > > Add proper definitions and stubs to aegis.h so it builds both ways. This > necessitated moving other stuff from aegis128-core.c to aegis.h so things were > defined in the proper order. > > Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> Which compiler version are you using? All references to the crypt_aegis128_xx_simd() routines should disappear if CONFIG_CRYPTO_AEGIS128_SIMD is not set (in which case have_simd will always be false and so the compiler should optimize away those calls). > --- > diff --git a/crypto/aegis.h b/crypto/aegis.h > index 4d56a85aea49..50a7496ca4ae 100644 > --- a/crypto/aegis.h > +++ b/crypto/aegis.h > @@ -13,6 +13,11 @@ > #include <linux/bitops.h> > #include <linux/types.h> > > +#define AEGIS128_NONCE_SIZE 16 > +#define AEGIS128_STATE_BLOCKS 5 > +#define AEGIS128_KEY_SIZE 16 > +#define AEGIS128_MIN_AUTH_SIZE 8 > +#define AEGIS128_MAX_AUTH_SIZE 16 > #define AEGIS_BLOCK_SIZE 16 > > union aegis_block { > @@ -21,6 +26,39 @@ union aegis_block { > u8 bytes[AEGIS_BLOCK_SIZE]; > }; > > +struct aegis_state { > + union aegis_block blocks[AEGIS128_STATE_BLOCKS]; > +}; > + > +struct aegis_ctx { > + union aegis_block key; > +}; > + > +struct aegis128_ops { > + int (*skcipher_walk_init)(struct skcipher_walk *walk, > + struct aead_request *req, bool atomic); > + > + void (*crypt_chunk)(struct aegis_state *state, u8 *dst, > + const u8 *src, unsigned int size); > +}; > + > + > +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD > +bool crypto_aegis128_have_simd(void); > +void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg); > +void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, > + const u8 *src, unsigned int size); > +void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, > + const u8 *src, unsigned int size); > +#else > +static inline bool crypto_aegis128_have_simd(void) { return false; } > +static inline void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg) { } > +static inline void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, > + const u8 *src, unsigned int size) { } > +static inline void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, > + const u8 *src, unsigned int size) { } > +#endif > + > #define AEGIS_BLOCK_ALIGN (__alignof__(union aegis_block)) > #define AEGIS_ALIGNED(p) IS_ALIGNED((uintptr_t)p, AEGIS_BLOCK_ALIGN) > > diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c > index f815b4685156..8b738128a921 100644 > --- a/crypto/aegis128-core.c > +++ b/crypto/aegis128-core.c > @@ -20,37 +20,8 @@ > > #include "aegis.h" > > -#define AEGIS128_NONCE_SIZE 16 > -#define AEGIS128_STATE_BLOCKS 5 > -#define AEGIS128_KEY_SIZE 16 > -#define AEGIS128_MIN_AUTH_SIZE 8 > -#define AEGIS128_MAX_AUTH_SIZE 16 > - > -struct aegis_state { > - union aegis_block blocks[AEGIS128_STATE_BLOCKS]; > -}; > - > -struct aegis_ctx { > - union aegis_block key; > -}; > - > -struct aegis128_ops { > - int (*skcipher_walk_init)(struct skcipher_walk *walk, > - struct aead_request *req, bool atomic); > - > - void (*crypt_chunk)(struct aegis_state *state, u8 *dst, > - const u8 *src, unsigned int size); > -}; > - > static bool have_simd; > > -bool crypto_aegis128_have_simd(void); > -void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg); > -void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, > - const u8 *src, unsigned int size); > -void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, > - const u8 *src, unsigned int size); > - > static void crypto_aegis128_update(struct aegis_state *state) > { > union aegis_block tmp; >
On Thu, 01 Aug 2019 08:01:54 +0300, Ard Biesheuvel said: > > ERROR: "crypto_aegis128_decrypt_chunk_simd" [crypto/aegis128.ko] undefined! > > ERROR: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > > ERROR: "crypto_aegis128_encrypt_chunk_simd" [crypto/aegis128.ko] undefined! > > make[1]: *** [scripts/Makefile.modpost:105: modules-modpost] Error 1 > > make: *** [Makefile:1299: modules] Error 2 > Which compiler version are you using? All references to the > crypt_aegis128_xx_simd() routines should disappear if > CONFIG_CRYPTO_AEGIS128_SIMD is not set (in which case have_simd will > always be false and so the compiler should optimize away those calls). gcc 9.1.1 obviously doesn't think it can be optimized away. Apparently, it's not smart enough to realize that nothing sets have_simd in any of the functions and therefor it's guaranteed to be zero, and it can do dead code optimization based on that. Now, if we had something like: #ifdef CONFIG_CRYPTO_AEGIS_128_SIMD static bool have_simd; #else #define have_simd (0) #endif then that should be enough to tell the compiler it can optimize it away, except that then runs into problems here: if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD)) have_simd = crypto_aegis128_have_simd(); because it will whine about the lack of an lvalue before it optimizes the assignment away...
On Thu, 1 Aug 2019 at 08:47, Valdis Klētnieks <valdis.kletnieks@vt.edu> wrote: > > On Thu, 01 Aug 2019 08:01:54 +0300, Ard Biesheuvel said: > > > > ERROR: "crypto_aegis128_decrypt_chunk_simd" [crypto/aegis128.ko] undefined! > > > ERROR: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > > > ERROR: "crypto_aegis128_encrypt_chunk_simd" [crypto/aegis128.ko] undefined! > > > make[1]: *** [scripts/Makefile.modpost:105: modules-modpost] Error 1 > > > make: *** [Makefile:1299: modules] Error 2 > > > Which compiler version are you using? All references to the > > crypt_aegis128_xx_simd() routines should disappear if > > CONFIG_CRYPTO_AEGIS128_SIMD is not set (in which case have_simd will > > always be false and so the compiler should optimize away those calls). > > gcc 9.1.1 obviously doesn't think it can be optimized away. Apparently, it's > not smart enough to realize that nothing sets have_simd in any of the functions > and therefor it's guaranteed to be zero, and it can do dead code optimization > based on that. > > Now, if we had something like: > > #ifdef CONFIG_CRYPTO_AEGIS_128_SIMD > static bool have_simd; > #else > #define have_simd (0) > #endif > > then that should be enough to tell the compiler it can optimize it away, except > that then runs into problems here: > > if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD)) > have_simd = crypto_aegis128_have_simd(); > > because it will whine about the lack of an lvalue before it optimizes the assignment away... The fact that crypto_aegis128_have_simd() does get optimized away, but crypto_aegis128_update_simd() doesn't (which is only called directly and not via a function pointer like the other two routines) makes me suspicious that this is some pathology in the compiler. Is this a distro build of gcc? Also, which architecture are you compiling for?
On Thu, 01 Aug 2019 09:04:11 +0300, Ard Biesheuvel said: > The fact that crypto_aegis128_have_simd() does get optimized away, but > crypto_aegis128_update_simd() doesn't (which is only called directly > and not via a function pointer like the other two routines) makes me > suspicious that this is some pathology in the compiler. Is this a > distro build of gcc? Also, which architecture are you compiling for? It's the Fedora Rawhide build on x86_64. [/usr/src/linux-next] gcc --version gcc (GCC) 9.1.1 20190605 (Red Hat 9.1.1-2) Copyright (C) 2019 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
On Thu, 1 Aug 2019 at 09:08, Valdis Klētnieks <valdis.kletnieks@vt.edu> wrote: > > On Thu, 01 Aug 2019 09:04:11 +0300, Ard Biesheuvel said: > > > The fact that crypto_aegis128_have_simd() does get optimized away, but > > crypto_aegis128_update_simd() doesn't (which is only called directly > > and not via a function pointer like the other two routines) makes me > > suspicious that this is some pathology in the compiler. Is this a > > distro build of gcc? Also, which architecture are you compiling for? > > It's the Fedora Rawhide build on x86_64. > > [/usr/src/linux-next] gcc --version > gcc (GCC) 9.1.1 20190605 (Red Hat 9.1.1-2) > Copyright (C) 2019 Free Software Foundation, Inc. > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > Strange. Does the following patch make any difference? https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheuvel@linaro.org/
diff --git a/crypto/aegis.h b/crypto/aegis.h index 4d56a85aea49..50a7496ca4ae 100644 --- a/crypto/aegis.h +++ b/crypto/aegis.h @@ -13,6 +13,11 @@ #include <linux/bitops.h> #include <linux/types.h> +#define AEGIS128_NONCE_SIZE 16 +#define AEGIS128_STATE_BLOCKS 5 +#define AEGIS128_KEY_SIZE 16 +#define AEGIS128_MIN_AUTH_SIZE 8 +#define AEGIS128_MAX_AUTH_SIZE 16 #define AEGIS_BLOCK_SIZE 16 union aegis_block { @@ -21,6 +26,39 @@ union aegis_block { u8 bytes[AEGIS_BLOCK_SIZE]; }; +struct aegis_state { + union aegis_block blocks[AEGIS128_STATE_BLOCKS]; +}; + +struct aegis_ctx { + union aegis_block key; +}; + +struct aegis128_ops { + int (*skcipher_walk_init)(struct skcipher_walk *walk, + struct aead_request *req, bool atomic); + + void (*crypt_chunk)(struct aegis_state *state, u8 *dst, + const u8 *src, unsigned int size); +}; + + +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD +bool crypto_aegis128_have_simd(void); +void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg); +void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, + const u8 *src, unsigned int size); +void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, + const u8 *src, unsigned int size); +#else +static inline bool crypto_aegis128_have_simd(void) { return false; } +static inline void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg) { } +static inline void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, + const u8 *src, unsigned int size) { } +static inline void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, + const u8 *src, unsigned int size) { } +#endif + #define AEGIS_BLOCK_ALIGN (__alignof__(union aegis_block)) #define AEGIS_ALIGNED(p) IS_ALIGNED((uintptr_t)p, AEGIS_BLOCK_ALIGN) diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c index f815b4685156..8b738128a921 100644 --- a/crypto/aegis128-core.c +++ b/crypto/aegis128-core.c @@ -20,37 +20,8 @@ #include "aegis.h" -#define AEGIS128_NONCE_SIZE 16 -#define AEGIS128_STATE_BLOCKS 5 -#define AEGIS128_KEY_SIZE 16 -#define AEGIS128_MIN_AUTH_SIZE 8 -#define AEGIS128_MAX_AUTH_SIZE 16 - -struct aegis_state { - union aegis_block blocks[AEGIS128_STATE_BLOCKS]; -}; - -struct aegis_ctx { - union aegis_block key; -}; - -struct aegis128_ops { - int (*skcipher_walk_init)(struct skcipher_walk *walk, - struct aead_request *req, bool atomic); - - void (*crypt_chunk)(struct aegis_state *state, u8 *dst, - const u8 *src, unsigned int size); -}; - static bool have_simd; -bool crypto_aegis128_have_simd(void); -void crypto_aegis128_update_simd(struct aegis_state *state, const void *msg); -void crypto_aegis128_encrypt_chunk_simd(struct aegis_state *state, u8 *dst, - const u8 *src, unsigned int size); -void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst, - const u8 *src, unsigned int size); - static void crypto_aegis128_update(struct aegis_state *state) { union aegis_block tmp;
The recent NEON SIMD patches break the build if CONFIG_CRYPTO_AEGIS128_SIMD isn't set: MODPOST 558 modules ERROR: "crypto_aegis128_decrypt_chunk_simd" [crypto/aegis128.ko] undefined! ERROR: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! ERROR: "crypto_aegis128_encrypt_chunk_simd" [crypto/aegis128.ko] undefined! make[1]: *** [scripts/Makefile.modpost:105: modules-modpost] Error 1 make: *** [Makefile:1299: modules] Error 2 Add proper definitions and stubs to aegis.h so it builds both ways. This necessitated moving other stuff from aegis128-core.c to aegis.h so things were defined in the proper order. Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> ---