Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Heiko Carstens]

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 <asm/simd.h>
      |          ^~~~~~~~~~~~

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
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

[Heiko Carstens]

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

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
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

[Heiko Carstens]

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

[Heiko Carstens]

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

[Ard Biesheuvel]

On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <heiko.carstens@de.ibm.com> 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.biesheuvel@linaro.org/

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

Hi Herbert,

On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <heiko.carstens@de.ibm.com> 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.biesheuvel@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Hi Herbert,

On Fri, 2 Aug 2019 13:14:14 +1000 Herbert Xu <herbert@gondor.apana.org.au> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Heiko Carstens]

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 <herbert@gondor.apana.org.au> 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

[Ard Biesheuvel]

On Fri, 2 Aug 2019 at 06:14, Herbert Xu <herbert@gondor.apana.org.au> 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.biesheuvel@linaro.org/

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Herbert Xu]

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.biesheuvel@linaro.org/

Please drop the ifdefs around the header files since we've already
fixed that.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
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

[Ard Biesheuvel]

On Fri, 2 Aug 2019 at 03:20, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Herbert,
>
> On Thu, 1 Aug 2019 20:28:56 +0300 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <heiko.carstens@de.ibm.com> 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.biesheuvel@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

[Heiko Carstens]

On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote:
> On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <heiko.carstens@de.ibm.com> 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.biesheuvel@linaro.org/

Yes, with that patch applied the code compiles.

Re: linux-next: Tree for Jul 31 - s390 crypto build breakage

[Ard Biesheuvel]

On Fri, 2 Aug 2019 at 09:46, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
>
> On Thu, Aug 01, 2019 at 08:28:56PM +0300, Ard Biesheuvel wrote:
> > On Thu, 1 Aug 2019 at 15:28, Heiko Carstens <heiko.carstens@de.ibm.com> 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.biesheuvel@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.