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 :)
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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!
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
On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <[email protected]> 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/[email protected]/
Hi Herbert,
On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel <[email protected]> wrote:
>
> On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <[email protected]> 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/[email protected]/
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
On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote:
> On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <[email protected]> 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/[email protected]/
Yes, with that patch applied the code compiles.
On Fri, 2 Aug 2019 at 03:20, Stephen Rothwell <[email protected]> wrote:
>
> Hi Herbert,
>
> On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel <[email protected]> wrote:
> >
> > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <[email protected]> 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/[email protected]/
>
> 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.
On Fri, 2 Aug 2019 at 09:46, Heiko Carstens <[email protected]> wrote:
>
> On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote:
> > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <[email protected]> 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/[email protected]/
>
> 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.
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi Herbert,
On Fri, 2 Aug 2019 13:14:14 +1000 Herbert Xu <[email protected]> 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
On Fri, 2 Aug 2019 at 06:14, Herbert Xu <[email protected]> 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/[email protected]/