| Message ID | 20190811225912.19412-4-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 198429631a85622da1d08d360ef02cfb84c95919 |
| Headers | show |
| Series | crypto: aegis128 followup | expand |
On Sun, Aug 11, 2019 at 3:59 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Provide a version of the core AES transform to the aegis128 SIMD > code that does not rely on the special AES instructions, but uses > plain NEON instructions instead. This allows the SIMD version of > the aegis128 driver to be used on arm64 systems that do not > implement those instructions (which are not mandatory in the > architecture), such as the Raspberry Pi 3. > > Since GCC makes a mess of this when using the tbl/tbx intrinsics > to perform the sbox substitution, preload the Sbox into v16..v31 > in this case and use inline asm to emit the tbl/tbx instructions. > Clang does not support this approach, nor does it require it, since > it does a much better job at code generation, so there we use the > intrinsics as usual. Oh, great job getting it working with Clang, too. I appreciate that. Certainly getting SIMD working exactly how you want across compilers can be tricky. > > Cc: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > crypto/Makefile | 9 ++- > crypto/aegis128-neon-inner.c | 65 ++++++++++++++++++++ > crypto/aegis128-neon.c | 8 ++- > 3 files changed, 80 insertions(+), 2 deletions(-) > > diff --git a/crypto/Makefile b/crypto/Makefile > index 99a9fa9087d1..0d2cdd523fd9 100644 > --- a/crypto/Makefile > +++ b/crypto/Makefile > @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8 > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > endif > ifeq ($(ARCH),arm64) > -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto > +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto > +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \ > + -ffixed-q19 -ffixed-q20 -ffixed-q21 \ > + -ffixed-q22 -ffixed-q23 -ffixed-q24 \ > + -ffixed-q25 -ffixed-q26 -ffixed-q27 \ > + -ffixed-q28 -ffixed-q29 -ffixed-q30 \ > + -ffixed-q31 I've filed https://bugs.llvm.org/show_bug.cgi?id=42974 for a feature request for this in Clang. > +CFLAGS_aegis128-neon-inner.o += $(aegis128-cflags-y) > CFLAGS_REMOVE_aegis128-neon-inner.o += -mgeneral-regs-only > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > endif > diff --git a/crypto/aegis128-neon-inner.c b/crypto/aegis128-neon-inner.c > index 3d8043c4832b..ed55568afd1b 100644 > --- a/crypto/aegis128-neon-inner.c > +++ b/crypto/aegis128-neon-inner.c > @@ -17,6 +17,8 @@ > > #include <stddef.h> > > +extern int aegis128_have_aes_insn; > + > void *memcpy(void *dest, const void *src, size_t n); > void *memset(void *s, int c, size_t n); > > @@ -24,6 +26,8 @@ struct aegis128_state { > uint8x16_t v[5]; > }; > > +extern const uint8x16x4_t crypto_aes_sbox[]; extern const uint8x16x4_t *crypto_aes_sbox; > + > static struct aegis128_state aegis128_load_state_neon(const void *state) > { > return (struct aegis128_state){ { > @@ -49,6 +53,46 @@ uint8x16_t aegis_aes_round(uint8x16_t w) > { > uint8x16_t z = {}; > > +#ifdef CONFIG_ARM64 > + if (!__builtin_expect(aegis128_have_aes_insn, 1)) { > + static const uint8x16_t shift_rows = { > + 0x0, 0x5, 0xa, 0xf, 0x4, 0x9, 0xe, 0x3, > + 0x8, 0xd, 0x2, 0x7, 0xc, 0x1, 0x6, 0xb, > + }; > + static const uint8x16_t ror32by8 = { > + 0x1, 0x2, 0x3, 0x0, 0x5, 0x6, 0x7, 0x4, > + 0x9, 0xa, 0xb, 0x8, 0xd, 0xe, 0xf, 0xc, > + }; > + uint8x16_t v; > + > + // shift rows > + w = vqtbl1q_u8(w, shift_rows); > + > + // sub bytes > + if (!IS_ENABLED(CONFIG_CC_IS_GCC)) { > + v = vqtbl4q_u8(crypto_aes_sbox[0], w); > + v = vqtbx4q_u8(v, crypto_aes_sbox[1], w - 0x40); > + v = vqtbx4q_u8(v, crypto_aes_sbox[2], w - 0x80); > + v = vqtbx4q_u8(v, crypto_aes_sbox[3], w - 0xc0); > + } else { > + asm("tbl %0.16b, {v16.16b-v19.16b}, %1.16b" : "=w"(v) : "w"(w)); > + w -= 0x40; > + asm("tbx %0.16b, {v20.16b-v23.16b}, %1.16b" : "+w"(v) : "w"(w)); > + w -= 0x40; > + asm("tbx %0.16b, {v24.16b-v27.16b}, %1.16b" : "+w"(v) : "w"(w)); > + w -= 0x40; > + asm("tbx %0.16b, {v28.16b-v31.16b}, %1.16b" : "+w"(v) : "w"(w)); > + } I find negation in a if condition that also has an else to be a code smell. Consider replacing: if !foo: bar() else: baz() with: if foo: baz() else: bar() (CONFIG_CC_IS_CLANG may be helpful here, too). With those 2 recommendations: Acked-by: Nick Desaulniers <ndesaulniers@google.com> in regards to compiling w/ Clang. Someone else should review the implementation of this crypto routine. > + > + // mix columns > + w = (v << 1) ^ (uint8x16_t)(((int8x16_t)v >> 7) & 0x1b); > + w ^= (uint8x16_t)vrev32q_u16((uint16x8_t)v); > + w ^= vqtbl1q_u8(v ^ w, ror32by8); > + > + return w; > + } > +#endif > + > /* > * We use inline asm here instead of the vaeseq_u8/vaesmcq_u8 intrinsics > * to force the compiler to issue the aese/aesmc instructions in pairs. > @@ -73,10 +117,27 @@ struct aegis128_state aegis128_update_neon(struct aegis128_state st, > return st; > } > > +static inline __attribute__((always_inline)) > +void preload_sbox(void) > +{ > + if (!IS_ENABLED(CONFIG_ARM64) || > + !IS_ENABLED(CONFIG_CC_IS_GCC) || > + __builtin_expect(aegis128_have_aes_insn, 1)) > + return; > + > + asm("ld1 {v16.16b-v19.16b}, [%0], #64 \n\t" > + "ld1 {v20.16b-v23.16b}, [%0], #64 \n\t" > + "ld1 {v24.16b-v27.16b}, [%0], #64 \n\t" > + "ld1 {v28.16b-v31.16b}, [%0] \n\t" > + :: "r"(crypto_aes_sbox)); > +} > + > void crypto_aegis128_update_neon(void *state, const void *msg) > { > struct aegis128_state st = aegis128_load_state_neon(state); > > + preload_sbox(); > + > st = aegis128_update_neon(st, vld1q_u8(msg)); > > aegis128_save_state_neon(st, state); > @@ -88,6 +149,8 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, > struct aegis128_state st = aegis128_load_state_neon(state); > uint8x16_t msg; > > + preload_sbox(); > + > while (size >= AEGIS_BLOCK_SIZE) { > uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; > > @@ -120,6 +183,8 @@ void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, > struct aegis128_state st = aegis128_load_state_neon(state); > uint8x16_t msg; > > + preload_sbox(); > + > while (size >= AEGIS_BLOCK_SIZE) { > msg = vld1q_u8(src) ^ st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; > st = aegis128_update_neon(st, msg); > diff --git a/crypto/aegis128-neon.c b/crypto/aegis128-neon.c > index c1c0a1686f67..751f9c195aa4 100644 > --- a/crypto/aegis128-neon.c > +++ b/crypto/aegis128-neon.c > @@ -14,9 +14,15 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, > void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, > unsigned int size); > > +int aegis128_have_aes_insn __ro_after_init; > + > bool crypto_aegis128_have_simd(void) > { > - return cpu_have_feature(cpu_feature(AES)); > + if (cpu_have_feature(cpu_feature(AES))) { > + aegis128_have_aes_insn = 1; If aegis128_have_aes_insn is __ro_after_init, is crypto_aegis128_have_simd() called exclusively from .init sectioned code? > + return true; > + } > + return IS_ENABLED(CONFIG_ARM64); > } > > void crypto_aegis128_update_simd(union aegis_block *state, const void *msg) > -- > 2.17.1 > -- Thanks, ~Nick Desaulniers
On Mon, 12 Aug 2019 at 19:50, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Sun, Aug 11, 2019 at 3:59 PM Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: > > > > Provide a version of the core AES transform to the aegis128 SIMD > > code that does not rely on the special AES instructions, but uses > > plain NEON instructions instead. This allows the SIMD version of > > the aegis128 driver to be used on arm64 systems that do not > > implement those instructions (which are not mandatory in the > > architecture), such as the Raspberry Pi 3. > > > > Since GCC makes a mess of this when using the tbl/tbx intrinsics > > to perform the sbox substitution, preload the Sbox into v16..v31 > > in this case and use inline asm to emit the tbl/tbx instructions. > > Clang does not support this approach, nor does it require it, since > > it does a much better job at code generation, so there we use the > > intrinsics as usual. > > Oh, great job getting it working with Clang, too. I appreciate that. > Certainly getting SIMD working exactly how you want across compilers > can be tricky. > Indeed. > > > > Cc: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > crypto/Makefile | 9 ++- > > crypto/aegis128-neon-inner.c | 65 ++++++++++++++++++++ > > crypto/aegis128-neon.c | 8 ++- > > 3 files changed, 80 insertions(+), 2 deletions(-) > > > > diff --git a/crypto/Makefile b/crypto/Makefile > > index 99a9fa9087d1..0d2cdd523fd9 100644 > > --- a/crypto/Makefile > > +++ b/crypto/Makefile > > @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8 > > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > > endif > > ifeq ($(ARCH),arm64) > > -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto > > +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto > > +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \ > > + -ffixed-q19 -ffixed-q20 -ffixed-q21 \ > > + -ffixed-q22 -ffixed-q23 -ffixed-q24 \ > > + -ffixed-q25 -ffixed-q26 -ffixed-q27 \ > > + -ffixed-q28 -ffixed-q29 -ffixed-q30 \ > > + -ffixed-q31 > > I've filed https://bugs.llvm.org/show_bug.cgi?id=42974 for a feature > request for this in Clang. > Good. But even GCC has issues here. Most notably, something like register uint8x16_t foo asm ("v16"); should permit a register that is excluded from general allocation to be used explicitly, but this throws a warning on GCC and an error with Clang. > > +CFLAGS_aegis128-neon-inner.o += $(aegis128-cflags-y) > > CFLAGS_REMOVE_aegis128-neon-inner.o += -mgeneral-regs-only > > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > > endif > > diff --git a/crypto/aegis128-neon-inner.c b/crypto/aegis128-neon-inner.c > > index 3d8043c4832b..ed55568afd1b 100644 > > --- a/crypto/aegis128-neon-inner.c > > +++ b/crypto/aegis128-neon-inner.c > > @@ -17,6 +17,8 @@ > > > > #include <stddef.h> > > > > +extern int aegis128_have_aes_insn; > > + > > void *memcpy(void *dest, const void *src, size_t n); > > void *memset(void *s, int c, size_t n); > > > > @@ -24,6 +26,8 @@ struct aegis128_state { > > uint8x16_t v[5]; > > }; > > > > +extern const uint8x16x4_t crypto_aes_sbox[]; > > extern const uint8x16x4_t *crypto_aes_sbox; > Ehm, nope. crypto_aes_sbox is an array of u8, not a pointer variable, so the former is the only correct way to declare it. > > + > > static struct aegis128_state aegis128_load_state_neon(const void *state) > > { > > return (struct aegis128_state){ { > > @@ -49,6 +53,46 @@ uint8x16_t aegis_aes_round(uint8x16_t w) > > { > > uint8x16_t z = {}; > > > > +#ifdef CONFIG_ARM64 > > + if (!__builtin_expect(aegis128_have_aes_insn, 1)) { > > + static const uint8x16_t shift_rows = { > > + 0x0, 0x5, 0xa, 0xf, 0x4, 0x9, 0xe, 0x3, > > + 0x8, 0xd, 0x2, 0x7, 0xc, 0x1, 0x6, 0xb, > > + }; > > + static const uint8x16_t ror32by8 = { > > + 0x1, 0x2, 0x3, 0x0, 0x5, 0x6, 0x7, 0x4, > > + 0x9, 0xa, 0xb, 0x8, 0xd, 0xe, 0xf, 0xc, > > + }; > > + uint8x16_t v; > > + > > + // shift rows > > + w = vqtbl1q_u8(w, shift_rows); > > + > > + // sub bytes > > + if (!IS_ENABLED(CONFIG_CC_IS_GCC)) { > > + v = vqtbl4q_u8(crypto_aes_sbox[0], w); > > + v = vqtbx4q_u8(v, crypto_aes_sbox[1], w - 0x40); > > + v = vqtbx4q_u8(v, crypto_aes_sbox[2], w - 0x80); > > + v = vqtbx4q_u8(v, crypto_aes_sbox[3], w - 0xc0); > > + } else { > > + asm("tbl %0.16b, {v16.16b-v19.16b}, %1.16b" : "=w"(v) : "w"(w)); > > + w -= 0x40; > > + asm("tbx %0.16b, {v20.16b-v23.16b}, %1.16b" : "+w"(v) : "w"(w)); > > + w -= 0x40; > > + asm("tbx %0.16b, {v24.16b-v27.16b}, %1.16b" : "+w"(v) : "w"(w)); > > + w -= 0x40; > > + asm("tbx %0.16b, {v28.16b-v31.16b}, %1.16b" : "+w"(v) : "w"(w)); > > + } > > I find negation in a if condition that also has an else to be a code > smell. Consider replacing: > > if !foo: > bar() > else: > baz() > > with: > > if foo: > baz() > else: > bar() > > (CONFIG_CC_IS_CLANG may be helpful here, too). > This was intentional. Since GCC is the compiler that needs the workaround, I test for GCC not Clang. Since the !GCC case is the default/correct case, I put it first. > With those 2 recommendations: > Acked-by: Nick Desaulniers <ndesaulniers@google.com> > in regards to compiling w/ Clang. Someone else should review the > implementation of this crypto routine. > > > + > > + // mix columns > > + w = (v << 1) ^ (uint8x16_t)(((int8x16_t)v >> 7) & 0x1b); > > + w ^= (uint8x16_t)vrev32q_u16((uint16x8_t)v); > > + w ^= vqtbl1q_u8(v ^ w, ror32by8); > > + > > + return w; > > + } > > +#endif > > + > > /* > > * We use inline asm here instead of the vaeseq_u8/vaesmcq_u8 intrinsics > > * to force the compiler to issue the aese/aesmc instructions in pairs. > > @@ -73,10 +117,27 @@ struct aegis128_state aegis128_update_neon(struct aegis128_state st, > > return st; > > } > > > > +static inline __attribute__((always_inline)) > > +void preload_sbox(void) > > +{ > > + if (!IS_ENABLED(CONFIG_ARM64) || > > + !IS_ENABLED(CONFIG_CC_IS_GCC) || > > + __builtin_expect(aegis128_have_aes_insn, 1)) > > + return; > > + > > + asm("ld1 {v16.16b-v19.16b}, [%0], #64 \n\t" > > + "ld1 {v20.16b-v23.16b}, [%0], #64 \n\t" > > + "ld1 {v24.16b-v27.16b}, [%0], #64 \n\t" > > + "ld1 {v28.16b-v31.16b}, [%0] \n\t" > > + :: "r"(crypto_aes_sbox)); > > +} > > + > > void crypto_aegis128_update_neon(void *state, const void *msg) > > { > > struct aegis128_state st = aegis128_load_state_neon(state); > > > > + preload_sbox(); > > + > > st = aegis128_update_neon(st, vld1q_u8(msg)); > > > > aegis128_save_state_neon(st, state); > > @@ -88,6 +149,8 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, > > struct aegis128_state st = aegis128_load_state_neon(state); > > uint8x16_t msg; > > > > + preload_sbox(); > > + > > while (size >= AEGIS_BLOCK_SIZE) { > > uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; > > > > @@ -120,6 +183,8 @@ void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, > > struct aegis128_state st = aegis128_load_state_neon(state); > > uint8x16_t msg; > > > > + preload_sbox(); > > + > > while (size >= AEGIS_BLOCK_SIZE) { > > msg = vld1q_u8(src) ^ st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; > > st = aegis128_update_neon(st, msg); > > diff --git a/crypto/aegis128-neon.c b/crypto/aegis128-neon.c > > index c1c0a1686f67..751f9c195aa4 100644 > > --- a/crypto/aegis128-neon.c > > +++ b/crypto/aegis128-neon.c > > @@ -14,9 +14,15 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, > > void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, > > unsigned int size); > > > > +int aegis128_have_aes_insn __ro_after_init; > > + > > bool crypto_aegis128_have_simd(void) > > { > > - return cpu_have_feature(cpu_feature(AES)); > > + if (cpu_have_feature(cpu_feature(AES))) { > > + aegis128_have_aes_insn = 1; > > If aegis128_have_aes_insn is __ro_after_init, is > crypto_aegis128_have_simd() called exclusively from .init sectioned > code? > Yes. the core aegis128 calls this only from the module init routine (which is turned into an initcall if the module is builtin). > > + return true; > > + } > > + return IS_ENABLED(CONFIG_ARM64); > > } > > > > void crypto_aegis128_update_simd(union aegis_block *state, const void *msg) > > -- > > 2.17.1 > > > > > -- > Thanks, > ~Nick Desaulniers
On Mon, Aug 12, 2019 at 10:22 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Mon, 12 Aug 2019 at 19:50, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > On Sun, Aug 11, 2019 at 3:59 PM Ard Biesheuvel > > <ard.biesheuvel@linaro.org> wrote: > > > diff --git a/crypto/Makefile b/crypto/Makefile > > > index 99a9fa9087d1..0d2cdd523fd9 100644 > > > --- a/crypto/Makefile > > > +++ b/crypto/Makefile > > > @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8 > > > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > > > endif > > > ifeq ($(ARCH),arm64) > > > -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto > > > +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto > > > +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \ > > > + -ffixed-q19 -ffixed-q20 -ffixed-q21 \ > > > + -ffixed-q22 -ffixed-q23 -ffixed-q24 \ > > > + -ffixed-q25 -ffixed-q26 -ffixed-q27 \ > > > + -ffixed-q28 -ffixed-q29 -ffixed-q30 \ > > > + -ffixed-q31 > > > > I've filed https://bugs.llvm.org/show_bug.cgi?id=42974 for a feature > > request for this in Clang. > > > > Good. But even GCC has issues here. Most notably, something like > > register uint8x16_t foo asm ("v16"); > > should permit a register that is excluded from general allocation to > be used explicitly, but this throws a warning on GCC and an error with > Clang. Consider filing bugs against GCC's issue tracker so that they're aware of the issue if you think there's more that can be improved on their end (for bugs in Clang, I'm always happy to help submit bug reports). What is the warning? for -ffixed-q* and `asm ("v16")`, on aarch64, what are the q registers and v registers? I assume they're related to NEON, but I'd only even worked with w* and x* GPRs. I *think* the explicit register syntax works for GPRs in Clang; maybe the v* and q* registers being broken is just oversight and can be fixed. > > With those 2 recommendations: > > Acked-by: Nick Desaulniers <ndesaulniers@google.com> > > in regards to compiling w/ Clang. Someone else should review the > > implementation of this crypto routine. -- Thanks, ~Nick Desaulniers
On Mon, 12 Aug 2019 at 20:31, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Aug 12, 2019 at 10:22 AM Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: > > > > On Mon, 12 Aug 2019 at 19:50, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > On Sun, Aug 11, 2019 at 3:59 PM Ard Biesheuvel > > > <ard.biesheuvel@linaro.org> wrote: > > > > diff --git a/crypto/Makefile b/crypto/Makefile > > > > index 99a9fa9087d1..0d2cdd523fd9 100644 > > > > --- a/crypto/Makefile > > > > +++ b/crypto/Makefile > > > > @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8 > > > > aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o > > > > endif > > > > ifeq ($(ARCH),arm64) > > > > -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto > > > > +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto > > > > +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \ > > > > + -ffixed-q19 -ffixed-q20 -ffixed-q21 \ > > > > + -ffixed-q22 -ffixed-q23 -ffixed-q24 \ > > > > + -ffixed-q25 -ffixed-q26 -ffixed-q27 \ > > > > + -ffixed-q28 -ffixed-q29 -ffixed-q30 \ > > > > + -ffixed-q31 > > > > > > I've filed https://bugs.llvm.org/show_bug.cgi?id=42974 for a feature > > > request for this in Clang. > > > > > > > Good. But even GCC has issues here. Most notably, something like > > > > register uint8x16_t foo asm ("v16"); > > > > should permit a register that is excluded from general allocation to > > be used explicitly, but this throws a warning on GCC and an error with > > Clang. > > Consider filing bugs against GCC's issue tracker so that they're aware > of the issue if you think there's more that can be improved on their > end (for bugs in Clang, I'm always happy to help submit bug reports). > What is the warning? > Cheers. With Clang, I get $ clang -target aarch64-linux-gnu -S -o - -O2 neon.c neon.c:4:1: error: bad type for named register variable register uint8x16_t foo asm ("v0"); ^ 1 error generated. but given that -fixed-vNN is not implemented either, this is not unexpected. But the latter is much more useful if the former is supported as well. I can't actually get the GCC warning to reproduce, so it might have been operator error :-) But if I define the sbox like this register uint8x16x4_t sbox0 asm ("v16"); register uint8x16x4_t sbox1 asm ("v20"); register uint8x16x4_t sbox2 asm ("v24"); register uint8x16x4_t sbox3 asm ("v28"); and use it in the sbox substitution, GCC emits lots of code like 2bc: 4eb01e08 mov v8.16b, v16.16b 2c0: 4eb11e29 mov v9.16b, v17.16b 2c4: 4eb21e4a mov v10.16b, v18.16b 2c8: 4eb31e6b mov v11.16b, v19.16b .. 2d4: 4e0e610e tbl v14.16b, {v8.16b-v11.16b}, v14.16b 2d8: 4e0d610d tbl v13.16b, {v8.16b-v11.16b}, v13.16b i.e., it moves the register contents around rather than use it in the tbl/tbx instructions directly, and the resulting code is absolutely dreadful. I will bring this up with the GCC developers, but it might be more useful to grab someone internally rather than go via the bugzilla. > for -ffixed-q* and `asm ("v16")`, on aarch64, what are the q registers > and v registers? I assume they're related to NEON, but I'd only even > worked with w* and x* GPRs. I *think* the explicit register syntax > works for GPRs in Clang; maybe the v* and q* registers being broken is > just oversight and can be fixed. > Yes. NEON/FP registers are referenced in different ways based on context: v0.16b, v0.8h, v0.4s, v0.2d for SIMD bytes, halfwords, single words double words q0/d0/s0 etc for scalar 128/64/32 bit FP > > > With those 2 recommendations: > > > Acked-by: Nick Desaulniers <ndesaulniers@google.com> > > > in regards to compiling w/ Clang. Someone else should review the > > > implementation of this crypto routine. > -- > Thanks, > ~Nick Desaulniers
diff --git a/crypto/Makefile b/crypto/Makefile index 99a9fa9087d1..0d2cdd523fd9 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -98,7 +98,14 @@ CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8 aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o endif ifeq ($(ARCH),arm64) -CFLAGS_aegis128-neon-inner.o += -ffreestanding -mcpu=generic+crypto +aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto +aegis128-cflags-$(CONFIG_CC_IS_GCC) += -ffixed-q16 -ffixed-q17 -ffixed-q18 \ + -ffixed-q19 -ffixed-q20 -ffixed-q21 \ + -ffixed-q22 -ffixed-q23 -ffixed-q24 \ + -ffixed-q25 -ffixed-q26 -ffixed-q27 \ + -ffixed-q28 -ffixed-q29 -ffixed-q30 \ + -ffixed-q31 +CFLAGS_aegis128-neon-inner.o += $(aegis128-cflags-y) CFLAGS_REMOVE_aegis128-neon-inner.o += -mgeneral-regs-only aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o endif diff --git a/crypto/aegis128-neon-inner.c b/crypto/aegis128-neon-inner.c index 3d8043c4832b..ed55568afd1b 100644 --- a/crypto/aegis128-neon-inner.c +++ b/crypto/aegis128-neon-inner.c @@ -17,6 +17,8 @@ #include <stddef.h> +extern int aegis128_have_aes_insn; + void *memcpy(void *dest, const void *src, size_t n); void *memset(void *s, int c, size_t n); @@ -24,6 +26,8 @@ struct aegis128_state { uint8x16_t v[5]; }; +extern const uint8x16x4_t crypto_aes_sbox[]; + static struct aegis128_state aegis128_load_state_neon(const void *state) { return (struct aegis128_state){ { @@ -49,6 +53,46 @@ uint8x16_t aegis_aes_round(uint8x16_t w) { uint8x16_t z = {}; +#ifdef CONFIG_ARM64 + if (!__builtin_expect(aegis128_have_aes_insn, 1)) { + static const uint8x16_t shift_rows = { + 0x0, 0x5, 0xa, 0xf, 0x4, 0x9, 0xe, 0x3, + 0x8, 0xd, 0x2, 0x7, 0xc, 0x1, 0x6, 0xb, + }; + static const uint8x16_t ror32by8 = { + 0x1, 0x2, 0x3, 0x0, 0x5, 0x6, 0x7, 0x4, + 0x9, 0xa, 0xb, 0x8, 0xd, 0xe, 0xf, 0xc, + }; + uint8x16_t v; + + // shift rows + w = vqtbl1q_u8(w, shift_rows); + + // sub bytes + if (!IS_ENABLED(CONFIG_CC_IS_GCC)) { + v = vqtbl4q_u8(crypto_aes_sbox[0], w); + v = vqtbx4q_u8(v, crypto_aes_sbox[1], w - 0x40); + v = vqtbx4q_u8(v, crypto_aes_sbox[2], w - 0x80); + v = vqtbx4q_u8(v, crypto_aes_sbox[3], w - 0xc0); + } else { + asm("tbl %0.16b, {v16.16b-v19.16b}, %1.16b" : "=w"(v) : "w"(w)); + w -= 0x40; + asm("tbx %0.16b, {v20.16b-v23.16b}, %1.16b" : "+w"(v) : "w"(w)); + w -= 0x40; + asm("tbx %0.16b, {v24.16b-v27.16b}, %1.16b" : "+w"(v) : "w"(w)); + w -= 0x40; + asm("tbx %0.16b, {v28.16b-v31.16b}, %1.16b" : "+w"(v) : "w"(w)); + } + + // mix columns + w = (v << 1) ^ (uint8x16_t)(((int8x16_t)v >> 7) & 0x1b); + w ^= (uint8x16_t)vrev32q_u16((uint16x8_t)v); + w ^= vqtbl1q_u8(v ^ w, ror32by8); + + return w; + } +#endif + /* * We use inline asm here instead of the vaeseq_u8/vaesmcq_u8 intrinsics * to force the compiler to issue the aese/aesmc instructions in pairs. @@ -73,10 +117,27 @@ struct aegis128_state aegis128_update_neon(struct aegis128_state st, return st; } +static inline __attribute__((always_inline)) +void preload_sbox(void) +{ + if (!IS_ENABLED(CONFIG_ARM64) || + !IS_ENABLED(CONFIG_CC_IS_GCC) || + __builtin_expect(aegis128_have_aes_insn, 1)) + return; + + asm("ld1 {v16.16b-v19.16b}, [%0], #64 \n\t" + "ld1 {v20.16b-v23.16b}, [%0], #64 \n\t" + "ld1 {v24.16b-v27.16b}, [%0], #64 \n\t" + "ld1 {v28.16b-v31.16b}, [%0] \n\t" + :: "r"(crypto_aes_sbox)); +} + void crypto_aegis128_update_neon(void *state, const void *msg) { struct aegis128_state st = aegis128_load_state_neon(state); + preload_sbox(); + st = aegis128_update_neon(st, vld1q_u8(msg)); aegis128_save_state_neon(st, state); @@ -88,6 +149,8 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, struct aegis128_state st = aegis128_load_state_neon(state); uint8x16_t msg; + preload_sbox(); + while (size >= AEGIS_BLOCK_SIZE) { uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; @@ -120,6 +183,8 @@ void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, struct aegis128_state st = aegis128_load_state_neon(state); uint8x16_t msg; + preload_sbox(); + while (size >= AEGIS_BLOCK_SIZE) { msg = vld1q_u8(src) ^ st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4]; st = aegis128_update_neon(st, msg); diff --git a/crypto/aegis128-neon.c b/crypto/aegis128-neon.c index c1c0a1686f67..751f9c195aa4 100644 --- a/crypto/aegis128-neon.c +++ b/crypto/aegis128-neon.c @@ -14,9 +14,15 @@ void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src, void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src, unsigned int size); +int aegis128_have_aes_insn __ro_after_init; + bool crypto_aegis128_have_simd(void) { - return cpu_have_feature(cpu_feature(AES)); + if (cpu_have_feature(cpu_feature(AES))) { + aegis128_have_aes_insn = 1; + return true; + } + return IS_ENABLED(CONFIG_ARM64); } void crypto_aegis128_update_simd(union aegis_block *state, const void *msg)
Provide a version of the core AES transform to the aegis128 SIMD code that does not rely on the special AES instructions, but uses plain NEON instructions instead. This allows the SIMD version of the aegis128 driver to be used on arm64 systems that do not implement those instructions (which are not mandatory in the architecture), such as the Raspberry Pi 3. Since GCC makes a mess of this when using the tbl/tbx intrinsics to perform the sbox substitution, preload the Sbox into v16..v31 in this case and use inline asm to emit the tbl/tbx instructions. Clang does not support this approach, nor does it require it, since it does a much better job at code generation, so there we use the intrinsics as usual. Cc: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- crypto/Makefile | 9 ++- crypto/aegis128-neon-inner.c | 65 ++++++++++++++++++++ crypto/aegis128-neon.c | 8 ++- 3 files changed, 80 insertions(+), 2 deletions(-) -- 2.17.1