[PATCH v2 0/3] crypto: aegis128 followup

[PATCH v2 0/3] crypto: aegis128 followup

[Ard Biesheuvel]

This series resubmits the aegis128 SIMD patches that were reverted due to
the fact that the compiler's optimization behavior wrt variables with static
linkage does not turn out to guarantee that function calls that are
conditional on the value of such a variable are optimized away if the value
is a compile time constant and the condition evaluates to false at compile
time as well.

Changes since v1:
- minor tweaks to #2 to drop a memset() invocation from the decrypt path,
  and some temp vars in various places
- update the NEON code in #3 so it builds with Clang as well as GCC (and
  drop the RFC annotation)

Patch #1 reintroduces the changes to the generic code to permit SIMD
routines to be attached to the aegis128 driver. This time, the conditional
check is pulled into a helper function which collapses to 'return false'
if the CONFIG_CRYPTO_AEGIS128_SIMD Kconfig symbol is not set. (This has
been confirmed by one of the reporters of the original issue as sufficient
to address the problem).

Patch #2 is mostly unchanged wrt the version that got reverted, only some
inline annotations were added back.

Patch #3 is new and implements the SIMD routines for arm64 without using
the optional AES instructions, but using plain SIMD arithmetic instead.
This is much slower than AES instructions, but still substantially more
efficient than table based scalar AES on systems where memory accesses are
expensive, such as the Raspberry Pi 3 (which does not implement the AES
instructions)

Ard Biesheuvel (3):
  crypto: aegis128 - add support for SIMD acceleration
  crypto: aegis128 - provide a SIMD implementation based on NEON
    intrinsics
  crypto: arm64/aegis128 - implement plain NEON version

 crypto/Kconfig                         |   5 +
 crypto/Makefile                        |  20 ++
 crypto/{aegis128.c => aegis128-core.c} |  52 ++++-
 crypto/aegis128-neon-inner.c           | 212 ++++++++++++++++++++
 crypto/aegis128-neon.c                 |  49 +++++
 5 files changed, 334 insertions(+), 4 deletions(-)
 rename crypto/{aegis128.c => aegis128-core.c} (89%)
 create mode 100644 crypto/aegis128-neon-inner.c
 create mode 100644 crypto/aegis128-neon.c

-- 
2.17.1

[PATCH v2 1/3] crypto: aegis128 - add support for SIMD acceleration

[Ard Biesheuvel]

Add some plumbing to allow the AEGIS128 code to be built with SIMD
routines for acceleration.

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/Makefile                        |  1 +
 crypto/{aegis128.c => aegis128-core.c} | 52 ++++++++++++++++++--
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/crypto/Makefile b/crypto/Makefile
index cfcc954e59f9..92e985714ff6 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_CRYPTO_GCM) += gcm.o
 obj-$(CONFIG_CRYPTO_CCM) += ccm.o
 obj-$(CONFIG_CRYPTO_CHACHA20POLY1305) += chacha20poly1305.o
 obj-$(CONFIG_CRYPTO_AEGIS128) += aegis128.o
+aegis128-y := aegis128-core.o
 obj-$(CONFIG_CRYPTO_PCRYPT) += pcrypt.o
 obj-$(CONFIG_CRYPTO_CRYPTD) += cryptd.o
 obj-$(CONFIG_CRYPTO_DES) += des_generic.o
diff --git a/crypto/aegis128.c b/crypto/aegis128-core.c
similarity index 89%
rename from crypto/aegis128.c
rename to crypto/aegis128-core.c
index 32840d5e7f65..fa69e99968e2 100644
--- a/crypto/aegis128.c
+++ b/crypto/aegis128-core.c
@@ -8,6 +8,7 @@
 
 #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,6 +17,8 @@
 #include <linux/module.h>
 #include <linux/scatterlist.h>
 
+#include <asm/simd.h>
+
 #include "aegis.h"
 
 #define AEGIS128_NONCE_SIZE 16
@@ -40,6 +43,24 @@ struct aegis128_ops {
 			    const u8 *src, unsigned int size);
 };
 
+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,
+					const u8 *src, unsigned int size);
+void crypto_aegis128_decrypt_chunk_simd(struct aegis_state *state, u8 *dst,
+					const u8 *src, unsigned int size);
+
 static void crypto_aegis128_update(struct aegis_state *state)
 {
 	union aegis_block tmp;
@@ -55,12 +76,22 @@ 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 (aegis128_do_simd()) {
+		crypto_aegis128_update_simd(state, msg);
+		return;
+	}
+
 	crypto_aegis128_update(state);
 	crypto_aegis_block_xor(&state->blocks[0], msg);
 }
 
 static void crypto_aegis128_update_u(struct aegis_state *state, const void *msg)
 {
+	if (aegis128_do_simd()) {
+		crypto_aegis128_update_simd(state, msg);
+		return;
+	}
+
 	crypto_aegis128_update(state);
 	crypto_xor(state->blocks[0].bytes, msg, AEGIS_BLOCK_SIZE);
 }
@@ -365,7 +396,7 @@ static void crypto_aegis128_crypt(struct aead_request *req,
 
 static int crypto_aegis128_encrypt(struct aead_request *req)
 {
-	static const struct aegis128_ops ops = {
+	const struct aegis128_ops *ops = &(struct aegis128_ops){
 		.skcipher_walk_init = skcipher_walk_aead_encrypt,
 		.crypt_chunk = crypto_aegis128_encrypt_chunk,
 	};
@@ -375,7 +406,12 @@ static int crypto_aegis128_encrypt(struct aead_request *req)
 	unsigned int authsize = crypto_aead_authsize(tfm);
 	unsigned int cryptlen = req->cryptlen;
 
-	crypto_aegis128_crypt(req, &tag, cryptlen, &ops);
+	if (aegis128_do_simd())
+		ops = &(struct aegis128_ops){
+			.skcipher_walk_init = skcipher_walk_aead_encrypt,
+			.crypt_chunk = crypto_aegis128_encrypt_chunk_simd };
+
+	crypto_aegis128_crypt(req, &tag, cryptlen, ops);
 
 	scatterwalk_map_and_copy(tag.bytes, req->dst, req->assoclen + cryptlen,
 				 authsize, 1);
@@ -384,7 +420,7 @@ static int crypto_aegis128_encrypt(struct aead_request *req)
 
 static int crypto_aegis128_decrypt(struct aead_request *req)
 {
-	static const struct aegis128_ops ops = {
+	const struct aegis128_ops *ops = &(struct aegis128_ops){
 		.skcipher_walk_init = skcipher_walk_aead_decrypt,
 		.crypt_chunk = crypto_aegis128_decrypt_chunk,
 	};
@@ -398,7 +434,12 @@ static int crypto_aegis128_decrypt(struct aead_request *req)
 	scatterwalk_map_and_copy(tag.bytes, req->src, req->assoclen + cryptlen,
 				 authsize, 0);
 
-	crypto_aegis128_crypt(req, &tag, cryptlen, &ops);
+	if (aegis128_do_simd())
+		ops = &(struct aegis128_ops){
+			.skcipher_walk_init = skcipher_walk_aead_decrypt,
+			.crypt_chunk = crypto_aegis128_decrypt_chunk_simd };
+
+	crypto_aegis128_crypt(req, &tag, cryptlen, ops);
 
 	return crypto_memneq(tag.bytes, zeros, authsize) ? -EBADMSG : 0;
 }
@@ -429,6 +470,9 @@ static struct aead_alg crypto_aegis128_alg = {
 
 static int __init crypto_aegis128_module_init(void)
 {
+	if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD))
+		have_simd = crypto_aegis128_have_simd();
+
 	return crypto_register_aead(&crypto_aegis128_alg);
 }
 
-- 
2.17.1

[PATCH v2 2/3] crypto: aegis128 - provide a SIMD implementation based on NEON intrinsics

[Ard Biesheuvel]

Provide an accelerated implementation of aegis128 by wiring up the
SIMD hooks in the generic driver to an implementation based on NEON
intrinsics, which can be compiled to both ARM and arm64 code.

This results in a performance of 2.2 cycles per byte on Cortex-A53,
which is a performance increase of ~11x compared to the generic
code.

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/Kconfig               |   5 +
 crypto/Makefile              |  12 ++
 crypto/aegis128-neon-inner.c | 147 ++++++++++++++++++++
 crypto/aegis128-neon.c       |  43 ++++++
 4 files changed, 207 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 8880c1fc51d8..455a3354e291 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -306,6 +306,11 @@ config CRYPTO_AEGIS128
 	help
 	 Support for the AEGIS-128 dedicated AEAD algorithm.
 
+config CRYPTO_AEGIS128_SIMD
+	bool "Support SIMD acceleration for AEGIS-128"
+	depends on CRYPTO_AEGIS128 && ((ARM || ARM64) && KERNEL_MODE_NEON)
+	default y
+
 config CRYPTO_AEGIS128_AESNI_SSE2
 	tristate "AEGIS-128 AEAD algorithm (x86_64 AESNI+SSE2 implementation)"
 	depends on X86 && 64BIT
diff --git a/crypto/Makefile b/crypto/Makefile
index 92e985714ff6..99a9fa9087d1 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -91,6 +91,18 @@ obj-$(CONFIG_CRYPTO_CCM) += ccm.o
 obj-$(CONFIG_CRYPTO_CHACHA20POLY1305) += chacha20poly1305.o
 obj-$(CONFIG_CRYPTO_AEGIS128) += aegis128.o
 aegis128-y := aegis128-core.o
+
+ifeq ($(ARCH),arm)
+CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv7-a -mfloat-abi=softfp
+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
+CFLAGS_REMOVE_aegis128-neon-inner.o += -mgeneral-regs-only
+aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o
+endif
+
 obj-$(CONFIG_CRYPTO_PCRYPT) += pcrypt.o
 obj-$(CONFIG_CRYPTO_CRYPTD) += cryptd.o
 obj-$(CONFIG_CRYPTO_DES) += des_generic.o
diff --git a/crypto/aegis128-neon-inner.c b/crypto/aegis128-neon-inner.c
new file mode 100644
index 000000000000..3d8043c4832b
--- /dev/null
+++ b/crypto/aegis128-neon-inner.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 Linaro, Ltd. <ard.biesheuvel@linaro.org>
+ */
+
+#ifdef CONFIG_ARM64
+#include <asm/neon-intrinsics.h>
+
+#define AES_ROUND	"aese %0.16b, %1.16b \n\t aesmc %0.16b, %0.16b"
+#else
+#include <arm_neon.h>
+
+#define AES_ROUND	"aese.8 %q0, %q1 \n\t aesmc.8 %q0, %q0"
+#endif
+
+#define AEGIS_BLOCK_SIZE	16
+
+#include <stddef.h>
+
+void *memcpy(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+
+struct aegis128_state {
+	uint8x16_t v[5];
+};
+
+static struct aegis128_state aegis128_load_state_neon(const void *state)
+{
+	return (struct aegis128_state){ {
+		vld1q_u8(state),
+		vld1q_u8(state + 16),
+		vld1q_u8(state + 32),
+		vld1q_u8(state + 48),
+		vld1q_u8(state + 64)
+	} };
+}
+
+static void aegis128_save_state_neon(struct aegis128_state st, void *state)
+{
+	vst1q_u8(state, st.v[0]);
+	vst1q_u8(state + 16, st.v[1]);
+	vst1q_u8(state + 32, st.v[2]);
+	vst1q_u8(state + 48, st.v[3]);
+	vst1q_u8(state + 64, st.v[4]);
+}
+
+static inline __attribute__((always_inline))
+uint8x16_t aegis_aes_round(uint8x16_t w)
+{
+	uint8x16_t z = {};
+
+	/*
+	 * 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.
+	 * This is much faster on many cores, where the instruction pair can
+	 * execute in a single cycle.
+	 */
+	asm(AES_ROUND : "+w"(w) : "w"(z));
+	return w;
+}
+
+static inline __attribute__((always_inline))
+struct aegis128_state aegis128_update_neon(struct aegis128_state st,
+					   uint8x16_t m)
+{
+	m       ^= aegis_aes_round(st.v[4]);
+	st.v[4] ^= aegis_aes_round(st.v[3]);
+	st.v[3] ^= aegis_aes_round(st.v[2]);
+	st.v[2] ^= aegis_aes_round(st.v[1]);
+	st.v[1] ^= aegis_aes_round(st.v[0]);
+	st.v[0] ^= m;
+
+	return st;
+}
+
+void crypto_aegis128_update_neon(void *state, const void *msg)
+{
+	struct aegis128_state st = aegis128_load_state_neon(state);
+
+	st = aegis128_update_neon(st, vld1q_u8(msg));
+
+	aegis128_save_state_neon(st, state);
+}
+
+void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src,
+					unsigned int size)
+{
+	struct aegis128_state st = aegis128_load_state_neon(state);
+	uint8x16_t msg;
+
+	while (size >= AEGIS_BLOCK_SIZE) {
+		uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4];
+
+		msg = vld1q_u8(src);
+		st = aegis128_update_neon(st, msg);
+		vst1q_u8(dst, msg ^ s);
+
+		size -= AEGIS_BLOCK_SIZE;
+		src += AEGIS_BLOCK_SIZE;
+		dst += AEGIS_BLOCK_SIZE;
+	}
+
+	if (size > 0) {
+		uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4];
+		uint8_t buf[AEGIS_BLOCK_SIZE] = {};
+
+		memcpy(buf, src, size);
+		msg = vld1q_u8(buf);
+		st = aegis128_update_neon(st, msg);
+		vst1q_u8(buf, msg ^ s);
+		memcpy(dst, buf, size);
+	}
+
+	aegis128_save_state_neon(st, state);
+}
+
+void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src,
+					unsigned int size)
+{
+	struct aegis128_state st = aegis128_load_state_neon(state);
+	uint8x16_t msg;
+
+	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);
+		vst1q_u8(dst, msg);
+
+		size -= AEGIS_BLOCK_SIZE;
+		src += AEGIS_BLOCK_SIZE;
+		dst += AEGIS_BLOCK_SIZE;
+	}
+
+	if (size > 0) {
+		uint8x16_t s = st.v[1] ^ (st.v[2] & st.v[3]) ^ st.v[4];
+		uint8_t buf[AEGIS_BLOCK_SIZE];
+
+		vst1q_u8(buf, s);
+		memcpy(buf, src, size);
+		msg = vld1q_u8(buf) ^ s;
+		vst1q_u8(buf, msg);
+		memcpy(dst, buf, size);
+
+		st = aegis128_update_neon(st, msg);
+	}
+
+	aegis128_save_state_neon(st, state);
+}
diff --git a/crypto/aegis128-neon.c b/crypto/aegis128-neon.c
new file mode 100644
index 000000000000..c1c0a1686f67
--- /dev/null
+++ b/crypto/aegis128-neon.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 Linaro Ltd <ard.biesheuvel@linaro.org>
+ */
+
+#include <asm/cpufeature.h>
+#include <asm/neon.h>
+
+#include "aegis.h"
+
+void crypto_aegis128_update_neon(void *state, const void *msg);
+void crypto_aegis128_encrypt_chunk_neon(void *state, void *dst, const void *src,
+					unsigned int size);
+void crypto_aegis128_decrypt_chunk_neon(void *state, void *dst, const void *src,
+					unsigned int size);
+
+bool crypto_aegis128_have_simd(void)
+{
+	return cpu_have_feature(cpu_feature(AES));
+}
+
+void crypto_aegis128_update_simd(union aegis_block *state, const void *msg)
+{
+	kernel_neon_begin();
+	crypto_aegis128_update_neon(state, msg);
+	kernel_neon_end();
+}
+
+void crypto_aegis128_encrypt_chunk_simd(union aegis_block *state, u8 *dst,
+					const u8 *src, unsigned int size)
+{
+	kernel_neon_begin();
+	crypto_aegis128_encrypt_chunk_neon(state, dst, src, size);
+	kernel_neon_end();
+}
+
+void crypto_aegis128_decrypt_chunk_simd(union aegis_block *state, u8 *dst,
+					const u8 *src, unsigned int size)
+{
+	kernel_neon_begin();
+	crypto_aegis128_decrypt_chunk_neon(state, dst, src, size);
+	kernel_neon_end();
+}
-- 
2.17.1

[PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

[Ard Biesheuvel]

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(-)

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)
-- 
2.17.1

Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

[Nick Desaulniers]

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

Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

[Ard Biesheuvel]

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

Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

[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

Re: [PATCH v2 3/3] crypto: arm64/aegis128 - implement plain NEON version

[Ard Biesheuvel]

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

Re: [PATCH v2 0/3] crypto: aegis128 followup

[Herbert Xu]

On Mon, Aug 12, 2019 at 01:59:09AM +0300, Ard Biesheuvel wrote:
> This series resubmits the aegis128 SIMD patches that were reverted due to
> the fact that the compiler's optimization behavior wrt variables with static
> linkage does not turn out to guarantee that function calls that are
> conditional on the value of such a variable are optimized away if the value
> is a compile time constant and the condition evaluates to false at compile
> time as well.
> 
> Changes since v1:
> - minor tweaks to #2 to drop a memset() invocation from the decrypt path,
>   and some temp vars in various places
> - update the NEON code in #3 so it builds with Clang as well as GCC (and
>   drop the RFC annotation)
> 
> Patch #1 reintroduces the changes to the generic code to permit SIMD
> routines to be attached to the aegis128 driver. This time, the conditional
> check is pulled into a helper function which collapses to 'return false'
> if the CONFIG_CRYPTO_AEGIS128_SIMD Kconfig symbol is not set. (This has
> been confirmed by one of the reporters of the original issue as sufficient
> to address the problem).
> 
> Patch #2 is mostly unchanged wrt the version that got reverted, only some
> inline annotations were added back.
> 
> Patch #3 is new and implements the SIMD routines for arm64 without using
> the optional AES instructions, but using plain SIMD arithmetic instead.
> This is much slower than AES instructions, but still substantially more
> efficient than table based scalar AES on systems where memory accesses are
> expensive, such as the Raspberry Pi 3 (which does not implement the AES
> instructions)
> 
> Ard Biesheuvel (3):
>   crypto: aegis128 - add support for SIMD acceleration
>   crypto: aegis128 - provide a SIMD implementation based on NEON
>     intrinsics
>   crypto: arm64/aegis128 - implement plain NEON version
> 
>  crypto/Kconfig                         |   5 +
>  crypto/Makefile                        |  20 ++
>  crypto/{aegis128.c => aegis128-core.c} |  52 ++++-
>  crypto/aegis128-neon-inner.c           | 212 ++++++++++++++++++++
>  crypto/aegis128-neon.c                 |  49 +++++
>  5 files changed, 334 insertions(+), 4 deletions(-)
>  rename crypto/{aegis128.c => aegis128-core.c} (89%)
>  create mode 100644 crypto/aegis128-neon-inner.c
>  create mode 100644 crypto/aegis128-neon.c

All applied.  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