Add AES with armv8 crypto extension #6895

yuhaoth · 2023-01-10T07:27:38Z

Description

fix #5387

Add aes with Armv8 crypto extension

Gatekeeper checklist

changelog provided in Changelog and terminology tidy-up for AESCE #7313
backport not required
tests provided

yuhaoth · 2023-01-10T10:23:24Z

local tests pass at d0f7759. with gcc 11.3 on ubuntu 22.04

yanesca · 2023-01-10T11:55:40Z

library/aesce.h

+ * \note            This function is only for internal use by other library
+ *                  functions; you must not call it directly.


This is true for every function in library/, only functions declared in include/ are part of the public API.

yanesca · 2023-01-10T15:55:31Z

The general direction looks good to me.

Do you think we could split this into two consecutive PRs? (Eg. the first just dealing with AES and the second adding AES-GCM.) I mean, it would make review slightly easier, but if it makes development more inconvenient for you then it doesn't worth it.

daverodgman · 2023-01-10T17:27:53Z

library/aesce.c

+    if( mode == MBEDTLS_AES_ENCRYPT )
+        vst1q_u8( &output[0], aesce_encrypt_block( in, keys, ctx->nr ) );
+    else
+        vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );;


Suggested change

vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );;

vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );

Oops, will fix it in exists commit.

yuhaoth · 2023-01-11T01:32:39Z

The general direction looks good to me.

Do you think we could split this into two consecutive PRs? (Eg. the first just dealing with AES and the second adding AES-GCM.) I mean, it would make review slightly easier, but if it makes development more inconvenient for you then it doesn't worth it.

Yes. I will split it if I think it is ready for review.

Beside GCM and AES, I plan to finish some works before review.

Clang and GCC versions compatible tests.
Add tests for aarch64. Base on qemu or Arm64 node.
Improve compile time architecture detection.
Improve runtime CPU feature sets detection.

tom-cosgrove-arm · 2023-01-12T08:12:33Z

library/aesce.c

+#if defined(__clang__)
+#error "Clang not supported yet"
+#elif defined(__GNUC__)
+#pragma GCC target ("arch=armv8-a+crypto")


We did this the other way round with the SHA-2 acceleration: when the option is enabled, we check that we've been given the correct compiler flags, rather than silently updating the target from what is specified on the command line

With arch=armv8.2-a+sha3+crypto and -O3, eor3 instruction is generated in assembly code of aesce.c . That cause illegal instruction error. So I disable SHA512 in .travis.yml . But that is not expected.

For time being, I think

We should set arch flags per file or per function

We should not force user enable the compiler flags. Those flags is only used for building library. If user only want to user library, why he must set the flags?

That is not finalized, I am still thinking and testing the issues. I might change above points :)

I have posted issues at #5387 (comment)

I have removed it.

And I think we should re-consider the rule due to the illegal instruction error when runtime detection enabled

As I've also mentioned on the architecture document, we can't just decide to compile for a different target. If the user wants to compile for a particular processor, we can't decide to compile for a different processor!

And I think we should re-consider the rule due to the illegal instruction error when runtime detection enabled

I'm sorry, I have no idea 1. what rule you're referring to and 2. what this has to do with an error when compiling some code (?) using a particular compiler option (?).

I believe the problem is with runtime detection:

You compile in both SHA acceleration (sha3 extension) and AES acceleration (crypto extension) with the intent of shipping it and trusting runtime detection to decide whether it is safe to run the optimised code.

The compiler with -O3 option uses an instruction in the AES code that is only present in the sha3 extension but not in the generic crypto extension.

On a platform where crypto extension is present but sha3 is not, the runtime detection will pass and run the accelerated AES code which uses an instruction that is not present.

(@yuhaoth please correct me if I am wrong.)

@yanesca , Yes, it is the problem.

And if sha3 extension is present, with sha3 compiler options, we can got best optimization. That's one another point I am thinking about in #7004 .

I'm sorry, I have no idea 1.
GCC and clang support two method to define target CPU modifier.

Option 1 with pragma

/* GCC */ #pragma GCC target ("arch=armv8-a+crypto") /* Clang */ #pragma clang attribute push (__attribute__((target("crypto"))), apply_to=function)

Option2 with attribute

/* GCC */ __attribute__((target("arch=armv8-a+crypto")) void function(void); /* Clang */ __attribute__((target("crypto")) void function(void);

what rule you're referring to and 2. what this has to do with an error when compiling some code (?) using a particular compiler option (?).

mbedtls/include/mbedtls/check_config.h

Line 734 in daa6595

# error "Must use minimum -march=armv8.2-a+sha3 for MBEDTLS_SHA512_USE_A64_CRYPTO_*"

and

mbedtls/include/mbedtls/check_config.h

Line 767 in daa6595

#error "Must use minimum -march=armv8-a+crypto for MBEDTLS_SHA256_USE_A64_CRYPTO_*"

. Compiler options are forced . If command line does not include the cpu modifier flags, the place will report error.

I prefer does not force them. And if users need best code size optimization, they should compile application and library with same cpu modifier flags.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

yanesca

LGTM

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

yanesca

LGTM

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2023-02-28T11:29:34Z

CI is fully green

tom-cosgrove-arm · 2023-02-28T11:30:01Z

@xkqian Have the changes you requested been addressed?

All requested changes have been performed.

gilles-peskine-arm · 2023-02-28T17:16:33Z

@xkqian Given that this is very-high-priority and all the changes you requested have been addressed, I'm going ahead and merging this pull request. If you want more changes, we can do them in a follow-up PR.

xkqian · 2023-03-01T02:22:17Z

@xkqian Have the changes you requested been addressed?

Yes, all of them have been addressed, thanks for the reminder.

xkqian · 2023-03-01T02:23:18Z

@xkqian Given that this is very-high-priority and all the changes you requested have been addressed, I'm going ahead and merging this pull request. If you want more changes, we can do them in a follow-up PR.

Sure, all of my requests have been addressed. go ahead!

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from aa4a0b0 to ff9386d Compare January 10, 2023 09:55

yuhaoth changed the title ~~Add AES and GCM with armv8 crypto extension~~ WIP: Add AES and GCM with armv8 crypto extension Jan 10, 2023

yanesca reviewed Jan 10, 2023

View reviewed changes

daverodgman reviewed Jan 10, 2023

View reviewed changes

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from d0f7759 to e1956c3 Compare January 11, 2023 02:27

yuhaoth mentioned this pull request Jan 11, 2023

Support armv8 crypto extensions for AES and GCM #5387

Closed

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch 6 times, most recently from 2022f3c to c737076 Compare January 12, 2023 04:52

tom-cosgrove-arm reviewed Jan 12, 2023

View reviewed changes

yuhaoth changed the title ~~WIP: Add AES and GCM with armv8 crypto extension~~ [WIP] Add AES and GCM with armv8 crypto extension Jan 12, 2023

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from 7cebf0e to 2876234 Compare January 13, 2023 05:36

yuhaoth changed the title ~~[WIP] Add AES and GCM with armv8 crypto extension~~ Add AES with armv8 crypto extension Jan 13, 2023

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from 2876234 to 93c9e24 Compare January 13, 2023 06:15

yuhaoth mentioned this pull request Jan 13, 2023

Improve runtime cpu feature detection #6921

Closed

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch 3 times, most recently from 7b25375 to 18500f6 Compare January 17, 2023 10:47

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from 3a100de to f7c1dda Compare January 30, 2023 07:22

yuhaoth added priority-high High priority - will be reviewed soon component-crypto Crypto primitives and low-level interfaces size-m Estimated task size: medium (~1w) labels Jan 30, 2023

yuhaoth force-pushed the pr/add-aes-with-armv8-crypto-extension branch from f7c1dda to 330a0e0 Compare January 30, 2023 07:47

yuhaoth mentioned this pull request Jan 30, 2023

Add GCM with armv8 crypto extension #6918

Merged

3 tasks

yuhaoth added 3 commits February 24, 2023 11:18

fix code style and comment issues

ba1e78f

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

Add comments about travis test.

aa18c4b

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

Add explanation for aesce limitation

c66deda

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

yuhaoth dismissed yanesca’s stale review via c66deda February 24, 2023 03:42

yuhaoth requested review from yanesca, tom-cosgrove-arm and xkqian February 24, 2023 05:01

yanesca previously approved these changes Feb 24, 2023

View reviewed changes

Improve comment about conflicts between aesce and sha512-crypto

608e109

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>

yuhaoth dismissed yanesca’s stale review via 608e109 February 28, 2023 04:50

yuhaoth requested a review from yanesca February 28, 2023 04:50

yanesca approved these changes Feb 28, 2023

View reviewed changes

tom-cosgrove-arm approved these changes Feb 28, 2023

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 28, 2023

gilles-peskine-arm merged commit b52b788 into Mbed-TLS:development Feb 28, 2023

yuhaoth deleted the pr/add-aes-with-armv8-crypto-extension branch March 1, 2023 02:40

yuhaoth mentioned this pull request Mar 3, 2023

Add CPU modifier for AESCE #7203

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AES with armv8 crypto extension #6895

Add AES with armv8 crypto extension #6895

yuhaoth commented Jan 10, 2023 •

edited by daverodgman

Loading

yuhaoth commented Jan 10, 2023

yanesca Jan 10, 2023

yanesca commented Jan 10, 2023

daverodgman Jan 10, 2023

yuhaoth Jan 11, 2023

yuhaoth commented Jan 11, 2023 •

edited

Loading

tom-cosgrove-arm Jan 12, 2023

yuhaoth Jan 12, 2023

yuhaoth Jan 12, 2023

yuhaoth Feb 1, 2023 •

edited

Loading

gilles-peskine-arm Feb 9, 2023

yanesca Feb 10, 2023

yuhaoth Feb 13, 2023

yuhaoth Feb 13, 2023

yanesca left a comment

yanesca left a comment

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Feb 28, 2023

tom-cosgrove-arm commented Feb 28, 2023

gilles-peskine-arm commented Feb 28, 2023

xkqian commented Mar 1, 2023

xkqian commented Mar 1, 2023

		* \note This function is only for internal use by other library
		* functions; you must not call it directly.

	vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );;
	vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );

Add AES with armv8 crypto extension #6895

Add AES with armv8 crypto extension #6895

Conversation

yuhaoth commented Jan 10, 2023 • edited by daverodgman Loading

Description

Gatekeeper checklist

yuhaoth commented Jan 10, 2023

Choose a reason for hiding this comment

yanesca commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth commented Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth Feb 1, 2023 • 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

yanesca left a comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Feb 28, 2023

tom-cosgrove-arm commented Feb 28, 2023

gilles-peskine-arm commented Feb 28, 2023

xkqian commented Mar 1, 2023

xkqian commented Mar 1, 2023

yuhaoth commented Jan 10, 2023 •

edited by daverodgman

Loading

yuhaoth commented Jan 11, 2023 •

edited

Loading

yuhaoth Feb 1, 2023 •

edited

Loading