Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Fri, Aug 02, 2019 at 09:44:43AM +0300, Ard Biesheuvel wrote: > > OK. I will adopt this mechanism [0] after all and resubmit, once I get > confirmation from either Voldis or Heiko that this makes the issue go > away (given that my local GCC does not reproduce the issue) > > [0] > https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/ Please drop the ifdefs around the header files since we've already fixed that. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Fri, Aug 02, 2019 at 02:48:44PM +1000, Stephen Rothwell wrote: > Hi Herbert, > > On Fri, 2 Aug 2019 13:14:14 +1000 Herbert Xu > wrote: > > > > For now I'm going to back out those two specific patches as the > > rest seem to be valid by themselves. > > I have applied the top commit from your tree to linux-next today just > to help with building and testing over the weekend (I had already > merged your tree before you added the revert). Thanks Stephen! We don't have any automatic testing coverage on s390 for any linux-next release this week due to the build breakage(s).
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Fri, 2 Aug 2019 at 09:46, Heiko Carstens wrote: > > On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote: > > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens > > wrote: > > > Still not... with linux-next as of today I get this (s390 defconfig): > > > > > > 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! > > > scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed > > > > > > > Hello Heiko, > > > > Apologies for the breakage. The first two fixes addressed obvious > > shortcomings in my code, but with this issue, I'm a bit puzzled tbh. > > The calls to these missing functions should be optimized away, since > > have_simd never gets assigned if CONFIG_CRYPTO_AEGIS128_SIMD is not > > defined, but for some reason, this isn't working. Which version of GCC > > are you using? > > Plain vanilla gcc 9.1.0. > > > Also, could you please try whether the patch below fixes the problem? Thanks > > https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/ > > Yes, with that patch applied the code compiles. > Thanks for confirming. Since Voldis is reporting GCC 9.1.x as well, this might be a compiler regression (and it explains why I did not see the issue locally) In any case, the patches have been reverted now, so I will resubmit them with the above change folded in. Thanks, Ard.
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Fri, 2 Aug 2019 at 03:20, Stephen Rothwell wrote: > > Hi Herbert, > > On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel > wrote: > > > > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens > > wrote: > > > > > > On Wed, Jul 31, 2019 at 01:44:54PM +0200, Heiko Carstens wrote: > > > > On Wed, Jul 31, 2019 at 09:32:16PM +1000, Herbert Xu wrote: > > > > > On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > > > > > > > > > > > However that doesn't fix the simd.h header file breakage with the > > > > > > second patch :) > > > > > > > > > > That fix should be there now too. > > > > > > > > Yes, works now. Thank you! > > > > > > Still not... with linux-next as of today I get this (s390 defconfig): > > > > > > 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! > > > scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed > > > > > > > Hello Heiko, > > > > Apologies for the breakage. The first two fixes addressed obvious > > shortcomings in my code, but with this issue, I'm a bit puzzled tbh. > > The calls to these missing functions should be optimized away, since > > have_simd never gets assigned if CONFIG_CRYPTO_AEGIS128_SIMD is not > > defined, but for some reason, this isn't working. Which version of GCC > > are you using? > > > > Also, could you please try whether the patch below fixes the problem? Thanks > > > > https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/ > > It might be time to revert all this series and try again. The > implementation seems to have not been well thought through from a kernel > building point of view. For a start the two commits > > 7cdc0ddbf74a ("crypto: aegis128 - add support for SIMD acceleration") > ecc8bc81f2fb ("crypto: aegis128 - provide a SIMD implementation based on > NEON intrinsics") > > seem to be in the wrong order (function used in the first before being > defined in the second). There are a series of declarations of external > functions in crypto/aegis128-core.c that should be in a header file. > And there was the assumption that asm/simd.h was available everywhere. > > Also crypto_aegis128_decrypt_chunk_simd() is referenced in a structure > initialisation (unprotected by any CONFIG_ variable - and so will be > referenced even if it does not exist). The compiler will have a hard > time knowing that "have_simd" is effectively a constant zero (and > crypto_simd_usable() is not constant). The only assignment to have_simd is guarded by a if (IS_ENABLED(CONFIG_xxx)) conditional, which is optimized away if the Kconfig symbol is not set. Usually, the compiler uses this information to infer that have_simd is a compile time constant '0', and optimizes away all the code that depends on have_simd being true. I haven't figured out yet why this doesn't work as expected on some versions of GCC, since it is a very common pattern throughout the kernel.
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote: > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens wrote: > > Still not... with linux-next as of today I get this (s390 defconfig): > > > > 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! > > scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed > > > > Hello Heiko, > > Apologies for the breakage. The first two fixes addressed obvious > shortcomings in my code, but with this issue, I'm a bit puzzled tbh. > The calls to these missing functions should be optimized away, since > have_simd never gets assigned if CONFIG_CRYPTO_AEGIS128_SIMD is not > defined, but for some reason, this isn't working. Which version of GCC > are you using? Plain vanilla gcc 9.1.0. > Also, could you please try whether the patch below fixes the problem? Thanks > https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/ Yes, with that patch applied the code compiles.
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Fri, 2 Aug 2019 at 06:14, Herbert Xu wrote: > > Hi Stephen: > > On Fri, Aug 02, 2019 at 10:20:19AM +1000, Stephen Rothwell wrote: > > > > It might be time to revert all this series and try again. The > > implementation seems to have not been well thought through from a kernel > > building point of view. For a start the two commits > > > > 7cdc0ddbf74a ("crypto: aegis128 - add support for SIMD acceleration") > > ecc8bc81f2fb ("crypto: aegis128 - provide a SIMD implementation based on > > NEON intrinsics") > > I think the idea was that it would get optimised out if the > implementation is absent which is why it was meant to work in > this order. But oviously as we have found out this didn't work. > > Ard, I think relying on the compiler to optimise something out based > on an assignment within an if statement is just too error-prone. > We'll need a different mechanism for this. > Indeed. This is definitely something I tested, and it appears to be dependent on the GCC version. > For now I'm going to back out those two specific patches as the > rest seem to be valid by themselves. > OK. I will adopt this mechanism [0] after all and resubmit, once I get confirmation from either Voldis or Heiko that this makes the issue go away (given that my local GCC does not reproduce the issue) [0] https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
Hi Herbert, On Fri, 2 Aug 2019 13:14:14 +1000 Herbert Xu wrote: > > For now I'm going to back out those two specific patches as the > rest seem to be valid by themselves. I have applied the top commit from your tree to linux-next today just to help with building and testing over the weekend (I had already merged your tree before you added the revert). Thanks -- Cheers, Stephen Rothwell pgpMA74jciiJk.pgp Description: OpenPGP digital signature
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
Hi Stephen: On Fri, Aug 02, 2019 at 10:20:19AM +1000, Stephen Rothwell wrote: > > It might be time to revert all this series and try again. The > implementation seems to have not been well thought through from a kernel > building point of view. For a start the two commits > > 7cdc0ddbf74a ("crypto: aegis128 - add support for SIMD acceleration") > ecc8bc81f2fb ("crypto: aegis128 - provide a SIMD implementation based on > NEON intrinsics") I think the idea was that it would get optimised out if the implementation is absent which is why it was meant to work in this order. But oviously as we have found out this didn't work. Ard, I think relying on the compiler to optimise something out based on an assignment within an if statement is just too error-prone. We'll need a different mechanism for this. For now I'm going to back out those two specific patches as the rest seem to be valid by themselves. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
Hi Herbert, On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel wrote: > > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens wrote: > > > > On Wed, Jul 31, 2019 at 01:44:54PM +0200, Heiko Carstens wrote: > > > On Wed, Jul 31, 2019 at 09:32:16PM +1000, Herbert Xu wrote: > > > > On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > > > > > > > > > However that doesn't fix the simd.h header file breakage with the > > > > > second patch :) > > > > > > > > That fix should be there now too. > > > > > > Yes, works now. Thank you! > > > > Still not... with linux-next as of today I get this (s390 defconfig): > > > > 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! > > scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed > > > > Hello Heiko, > > Apologies for the breakage. The first two fixes addressed obvious > shortcomings in my code, but with this issue, I'm a bit puzzled tbh. > The calls to these missing functions should be optimized away, since > have_simd never gets assigned if CONFIG_CRYPTO_AEGIS128_SIMD is not > defined, but for some reason, this isn't working. Which version of GCC > are you using? > > Also, could you please try whether the patch below fixes the problem? Thanks > > https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/ It might be time to revert all this series and try again. The implementation seems to have not been well thought through from a kernel building point of view. For a start the two commits 7cdc0ddbf74a ("crypto: aegis128 - add support for SIMD acceleration") ecc8bc81f2fb ("crypto: aegis128 - provide a SIMD implementation based on NEON intrinsics") seem to be in the wrong order (function used in the first before being defined in the second). There are a series of declarations of external functions in crypto/aegis128-core.c that should be in a header file. And there was the assumption that asm/simd.h was available everywhere. Also crypto_aegis128_decrypt_chunk_simd() is referenced in a structure initialisation (unprotected by any CONFIG_ variable - and so will be referenced even if it does not exist). The compiler will have a hard time knowing that "have_simd" is effectively a constant zero (and crypto_simd_usable() is not constant). -- Cheers, Stephen Rothwell pgpr1dUdiAVwe.pgp Description: OpenPGP digital signature
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Thu, 1 Aug 2019 at 15:28, Heiko Carstens wrote: > > On Wed, Jul 31, 2019 at 01:44:54PM +0200, Heiko Carstens wrote: > > On Wed, Jul 31, 2019 at 09:32:16PM +1000, Herbert Xu wrote: > > > On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > > > > > > > However that doesn't fix the simd.h header file breakage with the > > > > second patch :) > > > > > > That fix should be there now too. > > > > Yes, works now. Thank you! > > Still not... with linux-next as of today I get this (s390 defconfig): > > 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! > scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed > Hello Heiko, Apologies for the breakage. The first two fixes addressed obvious shortcomings in my code, but with this issue, I'm a bit puzzled tbh. The calls to these missing functions should be optimized away, since have_simd never gets assigned if CONFIG_CRYPTO_AEGIS128_SIMD is not defined, but for some reason, this isn't working. Which version of GCC are you using? Also, could you please try whether the patch below fixes the problem? Thanks https://lore.kernel.org/linux-crypto/20190729074434.21064-1-ard.biesheu...@linaro.org/
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 01:44:54PM +0200, Heiko Carstens wrote: > On Wed, Jul 31, 2019 at 09:32:16PM +1000, Herbert Xu wrote: > > On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > > > > > However that doesn't fix the simd.h header file breakage with the > > > second patch :) > > > > That fix should be there now too. > > Yes, works now. Thank you! Still not... with linux-next as of today I get this (s390 defconfig): 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! scripts/Makefile.modpost:105: recipe for target 'modules-modpost' failed
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 09:32:16PM +1000, Herbert Xu wrote: > On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > > > However that doesn't fix the simd.h header file breakage with the > > second patch :) > > That fix should be there now too. Yes, works now. Thank you!
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 01:15:20PM +0200, Heiko Carstens wrote: > > However that doesn't fix the simd.h header file breakage with the > second patch :) That fix should be there now too. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 09:08:17PM +1000, Herbert Xu wrote: > On Wed, Jul 31, 2019 at 10:58:20AM +0200, Heiko Carstens wrote: > > On Wed, Jul 31, 2019 at 04:39:15PM +1000, Stephen Rothwell wrote: > > > Hi all, > > > > > > Changes since 20190730: > > > > Hello Ard, > > > > two of your patches in the crypto tree cause build breakage on s390: > > > > The patch ("crypto: aes - create AES library based on the fixed time AES > > code") > > causes this: > > Ard already sent a patch for this which I've just pushed out. Ok, thanks! However that doesn't fix the simd.h header file breakage with the second patch :)
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 10:58:20AM +0200, Heiko Carstens wrote: > On Wed, Jul 31, 2019 at 04:39:15PM +1000, Stephen Rothwell wrote: > > Hi all, > > > > Changes since 20190730: > > Hello Ard, > > two of your patches in the crypto tree cause build breakage on s390: > > The patch ("crypto: aes - create AES library based on the fixed time AES > code") > causes this: Ard already sent a patch for this which I've just pushed out. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: linux-next: Tree for Jul 31 - s390 crypto build breakage
On Wed, Jul 31, 2019 at 04:39:15PM +1000, Stephen Rothwell wrote: > Hi all, > > Changes since 20190730: Hello Ard, two of your patches in the crypto tree cause build breakage on s390: The patch ("crypto: aes - create AES library based on the fixed time AES code") causes this: arch/s390/crypto/aes_s390.c:111:13: error: conflicting types for 'aes_encrypt' 111 | static void aes_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in) | ^~~ And the commit ("crypto: aegis128 - add support for SIMD acceleration") causes another build breakage: crypto/aegis128-core.c:19:10: fatal error: asm/simd.h: No such file or directory 19 | #include | ^~~~