Gcm pager #1959

jenswi-linaro · 2017-11-21T19:30:03Z

No description provided.

jforissier · 2017-11-22T10:04:33Z

core/arch/arm/plat-vexpress/conf.mk

@@ -69,6 +69,7 @@ $(call force,CFG_DT,y)
 CFG_SE_API ?= y
 CFG_SE_API_SELF_TEST ?= y
 CFG_PCSC_PASSTHRU_READER_DRV ?= y
+CFG_AES_GCM_TABLE_BASED ?= y


Why only for QEMU? Shouldn't that be always enabled in core/crypto.mk as long as CFG_CRYPTO_WITH_CE is not y?

Also it would be good to have performance numbers in the commit log. FWIW, here is the improvement I see on HiKey960 (with CFG_CRYPTO_WITH_CE=n of course, not a very realistic scenario since CE is much faster but for the sake of comparison it works):

root@HiKey960:/ xtest --aes-perf -m GCM (CFG_AES_GCM_TABLE_BASED=n) min=69.27us max=86.458us mean=70.5695us stddev=0.955826us (cv 1.35445%) (13.8383MiB/s) (CFG_AES_GCM_TABLE_BASED=y) min=41.666us max=53.646us mean=42.138us stddev=0.621345us (cv 1.47455%) (23.1753MiB/s)

Thanks, I'll update accordingly (using your numbers)

jforissier · 2017-11-22T10:17:14Z

core/crypto/sub.mk

@@ -1,4 +1,8 @@
 srcs-y += crypto.c
 srcs-y += aes-gcm.c
 srcs-y += aes-gcm-sw.c
+ifeq ($(CFG_AES_GCM_TABLE_BASED),y)


CFG_AES_GCM_TABLE_BASED is not compatible with CFG_CRYPTO_WITH_CE. I think this should be checked somewhere. In core/crypto.mk maybe?

You're right

jenswi-linaro · 2017-11-22T11:00:31Z

Update

jforissier

Nice update. In addition to consolidating the GCM code, this makes the pager significantly faster. For the record: on HiKey960, 64 bits, A73 cores only: "time xtest _60" takes about 7-8 seconds on master and only 1.5s with this PR.

Please see further comments below, a few minor things and one slightly bigger concern about "core: crypto: AES-GCM: add internal key expansion".

For "core: AES-GCM: import table based GF-mult", "core: crypto: AES-GCM: separate encryption key", "core: crypto: AES-GCM: internal_aes_gcm_{enc,dec}()"
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
For "core: LTC provide some AES primitives", "core: pager: use new aes-gcm implementation":
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
For "core: crypto: AES-GCM: rem tomcrypt.h dependency":
s/rem/remove/, with that:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2017-11-22T14:56:12Z

core/crypto.mk

@@ -41,6 +41,7 @@ CFG_CRYPTO_AES_GCM_FROM_CRYPTOLIB ?= n

 endif

+


Useless empty line

jforissier · 2017-11-22T15:42:06Z

core/crypto/aes-gcm-ghash-tbl.c

+ *  Copyright (C) 2006-2015, ARM Limited, All Rights Reserved
+ *  SPDX-License-Identifier: Apache-2.0
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License"); you may


Certainly redundant with the SPDX line but I suppose it was like this in the original implementation?

Yes, would you like me to trim it off?

jforissier · 2017-11-22T16:14:00Z

core/crypto/aes-gcm-sw.c

-void __weak internal_aes_gcm_encrypt_block(struct internal_aes_gcm_ctx *ctx,
-					   const void *src, void *dst)
+void __weak
+internal_aes_gcm_encrypt_block(const struct internal_aes_gcm_key *ek,


Minor: I'd rather have the line break after the opening parenthesis, because it is quite useful to be able to grep for the function name and see the __weak vs. non-__weak instances. The parameter list, OTOH, doesn't matter much (and it has to be split anyway).

If internal_aes_gcm_encrypt_block is folded up then "const struct internal_aes_gcm_key *ek" doesn't fit on that line and can't be aligned with the "(". Doing like this keeps checkpatch happy, even with -strict.

jforissier · 2017-11-22T16:14:55Z

core/crypto/aes-gcm-sw.c

-						  uint64_t *enc_key,
-						  unsigned int *rounds)
+TEE_Result __weak
+internal_aes_gcm_expand_enc_key(const void *key, size_t key_len,


Here also, splitting after key, would be better IMO.

jforissier · 2017-11-22T17:08:16Z

core/arch/arm/crypto/ghash-ce-core_a64.S

+	aese	v0.16b, v1.16b
+	umov	w0, v0.4s[0]
+	ret
+ENDPROC(pmull_gcm_load_round_keys)


s/pmull_gcm_load_round_keys/pmull_gcm_aes_sub/

jforissier · 2017-11-22T17:21:41Z

core/arch/arm/crypto/aes-gcm-ce.c

+	return (word >> shift) | (word << (32 - shift));
+}
+
+TEE_Result internal_aes_gcm_expand_enc_key(const void *key, size_t key_len,


Why ARM64 only?

I understand you're copying code from core/lib/libtomcrypt/src/ciphers/ (aes_armv8a_ce.c and aes_modes_armv8a_ce_a64.S) into core/arch/arm/crypto/ (aes-gcm-ce.c and ghash-ce-core_a64.S) to cut that dependency on LTC so that pager can use accelerated code. Two things:

You should clearly say where the code comes from in the commit log

Why not update similarly ghash-ce-core_a32.S using code from aes_modes_armv8a_ce_a32.S(function ce_aes_sub()) ?

Yes, I'll update the commit message

We don't have any AES routines for ARM32 yet so there's nothing to be done in this area for ARM32 yet.

core/lib/libtomcrypt/src/ciphers/aes_modes_armv8a_ce_a32.S?

Sorry I wasn't clear. In ARM64 we have an implementation with AES and GHASH fused, which gives a good speedup. In ARM32 we don't have such an implementation (yet, ever?) so instead we're using the AES API in the cryptolib.

What we could do, is to extract the CE AES implementation from LTC and supply it to LTC similarly to how GHASH is supplied. The advantage is that we can get rid of some redundancy and have easier access to the routines. This however is not in that scope of this PR.

I think I've got it now.

jenswi-linaro · 2017-11-23T07:33:15Z

Comments address, squashed and tags applied.
I haven't rebased on master yet to make it easier to diff against the prior state (7bb8053) if desired

jforissier

Perhaps arm64: would be welcome in the commit subject, but anyway:

For "core: crypto: AES-GCM: add internal key expansion":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2017-11-23T16:26:32Z

core/arch/arm/crypto/aes-gcm-ce.c

+	return (word >> shift) | (word << (32 - shift));
+}
+
+TEE_Result internal_aes_gcm_expand_enc_key(const void *key, size_t key_len,


I think I've got it now.

Imports table based GF multiplication from mbed TLS. Sets CFG_AES_GCM_TABLE_BASED to default y unless CFG_CRYPTO_WITH_CE is y, then CFG_AES_GCM_TABLE_BASED forced n. With tables performance is on HiKey960 (CFG_CRYPTO_WITH_CE=n): xtest --aes-perf -m GCM (CFG_AES_GCM_TABLE_BASED=n) min=69.27us max=86.458us mean=70.5695us stddev=0.955826us (cv 1.35445%) (13.8383MiB/s) (CFG_AES_GCM_TABLE_BASED=y) min=41.666us max=53.646us mean=42.138us stddev=0.621345us (cv 1.47455%) (23.1753MiB/s) Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Provides crypto_aes_expand_enc_key() and crypto_aes_enc_block(). These functions are needed to avoid exposing the type symmetric_key outside of LTC. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Removes tomcrypt.h dependency by replacing the "symmetric_key skey" field in struct internal_aes_gcm_ctx with a raw key. Replaces calls to the LTC functions aes_setup() and aes_ecb_encrypt() with calls to crypto_aes_expand_enc_key() and crypto_aes_enc_block() respectively. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds internal encryption key expansion when internal AES-GCM uses AES crypto extensions. This avoids a dependency on the crypto library to use the same endian on the expanded encryption key. Copies code from core/lib/libtomcrypt/src/ciphers/ aes_armv8a_ce.c and aes_modes_armv8a_ce_a64.S and makes some small changes to make it fit in the new place. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Separates the AES (CTR) encryption key from the rest of the context to allow more efficient key handling. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Adds internal_aes_gcm_enc() and internal_aes_gcm_dec() for encrypting/decrypting a complete message with an external expanded key. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Pager switches to use the new internal accelerated AES-GCM implementation instead of the old software only implementation. Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (QEMU, Hikey) Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-11-23T20:52:54Z

Rebased, tags applied

jforissier reviewed Nov 22, 2017

View reviewed changes

jenswi-linaro force-pushed the gcm-pager branch from 7bb8053 to 35c4cf8 Compare November 23, 2017 07:31

jforissier reviewed Nov 23, 2017

View reviewed changes

jenswi-linaro added 7 commits November 23, 2017 21:51

core: crypto: AES-GCM: separate encryption key

48ac11a

Separates the AES (CTR) encryption key from the rest of the context to allow more efficient key handling. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro force-pushed the gcm-pager branch from 35c4cf8 to 2164d0a Compare November 23, 2017 20:52

jforissier merged commit 947cfee into OP-TEE:master Nov 24, 2017

jenswi-linaro deleted the gcm-pager branch November 24, 2017 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gcm pager #1959

Gcm pager #1959

jenswi-linaro commented Nov 21, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 22, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 22, 2017

jenswi-linaro commented Nov 22, 2017

jforissier left a comment •

edited

Loading

jforissier Nov 22, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 23, 2017

jforissier Nov 23, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 22, 2017

jforissier Nov 23, 2017

jforissier Nov 22, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 23, 2017

jforissier Nov 22, 2017

jenswi-linaro Nov 23, 2017

jforissier Nov 23, 2017

jenswi-linaro Nov 23, 2017 •

edited

Loading

jforissier Nov 23, 2017

jenswi-linaro commented Nov 23, 2017

jforissier left a comment

jforissier Nov 23, 2017

jenswi-linaro commented Nov 23, 2017

		@@ -41,6 +41,7 @@ CFG_CRYPTO_AES_GCM_FROM_CRYPTOLIB ?= n

		endif

Gcm pager #1959

Gcm pager #1959

Conversation

jenswi-linaro commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 22, 2017

jforissier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 23, 2017

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 23, 2017

jforissier left a comment •

edited

Loading

jenswi-linaro Nov 23, 2017 •

edited

Loading