diff mbox series

crypto: aegis128 - deal with missing simd.h header on some architecures

Message ID 20190729074434.21064-1-ard.biesheuvel@linaro.org
State New
Headers show
Series crypto: aegis128 - deal with missing simd.h header on some architecures | expand

Commit Message

Ard Biesheuvel July 29, 2019, 7:44 a.m. UTC 
The generic aegis128 driver has been updated to support using SIMD
intrinsics to implement the core AES based transform, and this has
been wired up for ARM and arm64, which both provide a simd.h header.

As it turns out, most architectures don't provide this header, even
though a version of it exists in include/asm-generic, and this is
not taken into account by the aegis128 driver, resulting in build
failures on those architectures.

So update the aegis128 code to only import simd.h (and the related
header in internal/crypto) if the SIMD functionality is enabled for
this driver.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 crypto/aegis128-core.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

-- 
2.17.1

Comments

Geert Uytterhoeven July 29, 2019, 7:55 a.m. UTC | #1 
Hi Ard,

On Mon, Jul 29, 2019 at 9:44 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> The generic aegis128 driver has been updated to support using SIMD

> intrinsics to implement the core AES based transform, and this has

> been wired up for ARM and arm64, which both provide a simd.h header.

>

> As it turns out, most architectures don't provide this header, even

> though a version of it exists in include/asm-generic, and this is

> not taken into account by the aegis128 driver, resulting in build

> failures on those architectures.

>

> So update the aegis128 code to only import simd.h (and the related

> header in internal/crypto) if the SIMD functionality is enabled for

> this driver.

>

> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Thanks for your patch!

> --- a/crypto/aegis128-core.c

> +++ b/crypto/aegis128-core.c

> @@ -8,7 +8,6 @@

>

>  #include <crypto/algapi.h>

>  #include <crypto/internal/aead.h>

> -#include <crypto/internal/simd.h>

>  #include <crypto/internal/skcipher.h>

>  #include <crypto/scatterwalk.h>

>  #include <linux/err.h>

> @@ -16,7 +15,11 @@

>  #include <linux/kernel.h>

>  #include <linux/module.h>

>  #include <linux/scatterlist.h>

> +

> +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD

> +#include <crypto/internal/simd.h>

>  #include <asm/simd.h>


Wouldn't including <crypto/internal/simd.h> unconditionally, and
adding just

    #else
    static inline bool may_use_simd(void)
    {
            return false;
    }

and be done with it, work too?

> +#endif

>

>  #include "aegis.h"

>

> @@ -44,6 +47,15 @@ struct aegis128_ops {

>

>  static bool have_simd;

>

> +static bool aegis128_do_simd(void)

> +{

> +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD

> +       if (have_simd)

> +               return crypto_simd_usable();

> +#endif

> +       return false;

> +}

> +

>  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,

> @@ -66,7 +78,7 @@ static void crypto_aegis128_update(struct aegis_state *state)

>  static void crypto_aegis128_update_a(struct aegis_state *state,

>                                      const union aegis_block *msg)

>  {

> -       if (have_simd && crypto_simd_usable()) {

> +       if (aegis128_do_simd()) {

>                 crypto_aegis128_update_simd(state, msg);

>                 return;

>         }


[...]

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Ard Biesheuvel July 29, 2019, 8:02 a.m. UTC | #2 
On Mon, 29 Jul 2019 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Ard,

>

> On Mon, Jul 29, 2019 at 9:44 AM Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

> > The generic aegis128 driver has been updated to support using SIMD

> > intrinsics to implement the core AES based transform, and this has

> > been wired up for ARM and arm64, which both provide a simd.h header.

> >

> > As it turns out, most architectures don't provide this header, even

> > though a version of it exists in include/asm-generic, and this is

> > not taken into account by the aegis128 driver, resulting in build

> > failures on those architectures.

> >

> > So update the aegis128 code to only import simd.h (and the related

> > header in internal/crypto) if the SIMD functionality is enabled for

> > this driver.

> >

> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> Thanks for your patch!

>

> > --- a/crypto/aegis128-core.c

> > +++ b/crypto/aegis128-core.c

> > @@ -8,7 +8,6 @@

> >

> >  #include <crypto/algapi.h>

> >  #include <crypto/internal/aead.h>

> > -#include <crypto/internal/simd.h>

> >  #include <crypto/internal/skcipher.h>

> >  #include <crypto/scatterwalk.h>

> >  #include <linux/err.h>

> > @@ -16,7 +15,11 @@

> >  #include <linux/kernel.h>

> >  #include <linux/module.h>

> >  #include <linux/scatterlist.h>

> > +

> > +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD

> > +#include <crypto/internal/simd.h>

> >  #include <asm/simd.h>

>

> Wouldn't including <crypto/internal/simd.h> unconditionally, and

> adding just

>

>     #else

>     static inline bool may_use_simd(void)

>     {

>             return false;

>     }

>

> and be done with it, work too?

>


I guess, but I felt it was more appropriate to include as little of
the SIMD infrastructure as possible when building this driver without
SIMD support. Also, I think it is slightly ugly to have a alternative
local implementation of something that is provided by a header file.



> > +#endif

> >

> >  #include "aegis.h"

> >

> > @@ -44,6 +47,15 @@ struct aegis128_ops {

> >

> >  static bool have_simd;

> >

> > +static bool aegis128_do_simd(void)

> > +{

> > +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD

> > +       if (have_simd)

> > +               return crypto_simd_usable();

> > +#endif

> > +       return false;

> > +}

> > +

> >  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,

> > @@ -66,7 +78,7 @@ static void crypto_aegis128_update(struct aegis_state *state)

> >  static void crypto_aegis128_update_a(struct aegis_state *state,

> >                                      const union aegis_block *msg)

> >  {

> > -       if (have_simd && crypto_simd_usable()) {

> > +       if (aegis128_do_simd()) {

> >                 crypto_aegis128_update_simd(state, msg);

> >                 return;

> >         }

>

> [...]

>

> Gr{oetje,eeting}s,

>

>                         Geert

>

> --

> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

>

> In personal conversations with technical people, I call myself a hacker. But

> when I'm talking to journalists I just say "programmer" or something like that.

>                                 -- Linus Torvalds
Geert Uytterhoeven July 29, 2019, 8:26 a.m. UTC | #3 
Hi Ard,

On Mon, Jul 29, 2019 at 10:02 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On Mon, 29 Jul 2019 at 10:55, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Mon, Jul 29, 2019 at 9:44 AM Ard Biesheuvel

> > <ard.biesheuvel@linaro.org> wrote:

> > > The generic aegis128 driver has been updated to support using SIMD

> > > intrinsics to implement the core AES based transform, and this has

> > > been wired up for ARM and arm64, which both provide a simd.h header.

> > >

> > > As it turns out, most architectures don't provide this header, even

> > > though a version of it exists in include/asm-generic, and this is

> > > not taken into account by the aegis128 driver, resulting in build

> > > failures on those architectures.

> > >

> > > So update the aegis128 code to only import simd.h (and the related

> > > header in internal/crypto) if the SIMD functionality is enabled for

> > > this driver.

> > >

> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >

> > Thanks for your patch!

> >

> > > --- a/crypto/aegis128-core.c

> > > +++ b/crypto/aegis128-core.c

> > > @@ -8,7 +8,6 @@

> > >

> > >  #include <crypto/algapi.h>

> > >  #include <crypto/internal/aead.h>

> > > -#include <crypto/internal/simd.h>

> > >  #include <crypto/internal/skcipher.h>

> > >  #include <crypto/scatterwalk.h>

> > >  #include <linux/err.h>

> > > @@ -16,7 +15,11 @@

> > >  #include <linux/kernel.h>

> > >  #include <linux/module.h>

> > >  #include <linux/scatterlist.h>

> > > +

> > > +#ifdef CONFIG_CRYPTO_AEGIS128_SIMD

> > > +#include <crypto/internal/simd.h>

> > >  #include <asm/simd.h>

> >

> > Wouldn't including <crypto/internal/simd.h> unconditionally, and

> > adding just

> >

> >     #else

> >     static inline bool may_use_simd(void)

> >     {

> >             return false;

> >     }

> >

> > and be done with it, work too?

> >

>

> I guess, but I felt it was more appropriate to include as little of

> the SIMD infrastructure as possible when building this driver without

> SIMD support. Also, I think it is slightly ugly to have a alternative

> local implementation of something that is provided by a header file.


Alternatively, generic-y += simd.h on all architectures lacking asm/simd.h,
which is probably the best solution in the long run.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Herbert Xu July 29, 2019, 9:12 a.m. UTC | #4 
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The generic aegis128 driver has been updated to support using SIMD

> intrinsics to implement the core AES based transform, and this has

> been wired up for ARM and arm64, which both provide a simd.h header.

> 

> As it turns out, most architectures don't provide this header, even

> though a version of it exists in include/asm-generic, and this is

> not taken into account by the aegis128 driver, resulting in build

> failures on those architectures.

> 

> So update the aegis128 code to only import simd.h (and the related

> header in internal/crypto) if the SIMD functionality is enabled for

> this driver.

> 

> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> crypto/aegis128-core.c | 22 +++++++++++++++++-----

> 1 file changed, 17 insertions(+), 5 deletions(-)


I think we should dig a little deeper into why asm-generic isn't
working for this case.  AFAICS we rely on the same mechanism for
errno.h on m68k and that obviously works.

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
Ard Biesheuvel July 29, 2019, 9:15 a.m. UTC | #5 
On Mon, 29 Jul 2019 at 12:12, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>

> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > The generic aegis128 driver has been updated to support using SIMD

> > intrinsics to implement the core AES based transform, and this has

> > been wired up for ARM and arm64, which both provide a simd.h header.

> >

> > As it turns out, most architectures don't provide this header, even

> > though a version of it exists in include/asm-generic, and this is

> > not taken into account by the aegis128 driver, resulting in build

> > failures on those architectures.

> >

> > So update the aegis128 code to only import simd.h (and the related

> > header in internal/crypto) if the SIMD functionality is enabled for

> > this driver.

> >

> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> > crypto/aegis128-core.c | 22 +++++++++++++++++-----

> > 1 file changed, 17 insertions(+), 5 deletions(-)

>

> I think we should dig a little deeper into why asm-generic isn't

> working for this case.  AFAICS we rely on the same mechanism for

> errno.h on m68k and that obviously works.

>


It is simply a matter of adding simd.h to the various
arch/<...>/include/asm/Kbuild files, but we'd have to do that for all
architectures.
Herbert Xu July 29, 2019, 9:21 a.m. UTC | #6 
On Mon, Jul 29, 2019 at 12:15:20PM +0300, Ard Biesheuvel wrote:
>

> It is simply a matter of adding simd.h to the various

> arch/<...>/include/asm/Kbuild files, but we'd have to do that for all

> architectures.


How does errno.h get added then? It doesn't appear to be in the
m68k (or arm) Kbuild file either.

Cheers,
-- 
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
Ard Biesheuvel July 29, 2019, 9:32 a.m. UTC | #7 
On Mon, 29 Jul 2019 at 12:21, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>

> On Mon, Jul 29, 2019 at 12:15:20PM +0300, Ard Biesheuvel wrote:

> >

> > It is simply a matter of adding simd.h to the various

> > arch/<...>/include/asm/Kbuild files, but we'd have to do that for all

> > architectures.

>

> How does errno.h get added then? It doesn't appear to be in the

> m68k (or arm) Kbuild file either.

>


This seems to work

diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index 6f4536d70b8e..adff14fcb8e4 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -3,3 +3,5 @@
 # asm headers that all architectures except um should have
 # (This file is not included when SRCARCH=um since UML borrows several
 # asm headers from the host architecutre.)
+
+mandatory-y += simd.h
Herbert Xu July 29, 2019, 9:36 a.m. UTC | #8 
On Mon, Jul 29, 2019 at 12:32:33PM +0300, Ard Biesheuvel wrote:
>

> This seems to work


Looks good to me.

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
diff mbox series

Patch

diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c
index f815b4685156..d46a12872d35 100644
--- a/crypto/aegis128-core.c
+++ b/crypto/aegis128-core.c
@@ -8,7 +8,6 @@ 
 
 #include <crypto/algapi.h>
 #include <crypto/internal/aead.h>
-#include <crypto/internal/simd.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/scatterwalk.h>
 #include <linux/err.h>
@@ -16,7 +15,11 @@ 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+
+#ifdef CONFIG_CRYPTO_AEGIS128_SIMD
+#include <crypto/internal/simd.h>
 #include <asm/simd.h>
+#endif
 
 #include "aegis.h"
 
@@ -44,6 +47,15 @@  struct aegis128_ops {
 
 static bool have_simd;
 
+static bool aegis128_do_simd(void)
+{
+#ifdef CONFIG_CRYPTO_AEGIS128_SIMD
+	if (have_simd)
+		return crypto_simd_usable();
+#endif
+	return false;
+}
+
 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,
@@ -66,7 +78,7 @@  static void crypto_aegis128_update(struct aegis_state *state)
 static void crypto_aegis128_update_a(struct aegis_state *state,
 				     const union aegis_block *msg)
 {
-	if (have_simd && crypto_simd_usable()) {
+	if (aegis128_do_simd()) {
 		crypto_aegis128_update_simd(state, msg);
 		return;
 	}
@@ -77,7 +89,7 @@  static void crypto_aegis128_update_a(struct aegis_state *state,
 
 static void crypto_aegis128_update_u(struct aegis_state *state, const void *msg)
 {
-	if (have_simd && crypto_simd_usable()) {
+	if (aegis128_do_simd()) {
 		crypto_aegis128_update_simd(state, msg);
 		return;
 	}
@@ -396,7 +408,7 @@  static int crypto_aegis128_encrypt(struct aead_request *req)
 	unsigned int authsize = crypto_aead_authsize(tfm);
 	unsigned int cryptlen = req->cryptlen;
 
-	if (have_simd && crypto_simd_usable())
+	if (aegis128_do_simd())
 		ops = &(struct aegis128_ops){
 			.skcipher_walk_init = skcipher_walk_aead_encrypt,
 			.crypt_chunk = crypto_aegis128_encrypt_chunk_simd };
@@ -424,7 +436,7 @@  static int crypto_aegis128_decrypt(struct aead_request *req)
 	scatterwalk_map_and_copy(tag.bytes, req->src, req->assoclen + cryptlen,
 				 authsize, 0);
 
-	if (have_simd && crypto_simd_usable())
+	if (aegis128_do_simd())
 		ops = &(struct aegis128_ops){
 			.skcipher_walk_init = skcipher_walk_aead_decrypt,
 			.crypt_chunk = crypto_aegis128_decrypt_chunk_simd };