-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AES with armv8 crypto extension #6895
Add AES with armv8 crypto extension #6895
Conversation
aa4a0b0
to
ff9386d
Compare
local tests pass at d0f7759. with gcc 11.3 on ubuntu 22.04 |
library/aesce.h
Outdated
* \note This function is only for internal use by other library | ||
* functions; you must not call it directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true for every function in library/
, only functions declared in include/
are part of the public API.
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. |
library/aesce.c
Outdated
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 ) );; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) );; | |
vst1q_u8( &output[0], aesce_decrypt_block( in, keys, ctx->nr ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, will fix it in exists commit.
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.
|
d0f7759
to
e1956c3
Compare
2022f3c
to
c737076
Compare
library/aesce.c
Outdated
#if defined(__clang__) | ||
#error "Clang not supported yet" | ||
#elif defined(__GNUC__) | ||
#pragma GCC target ("arch=armv8-a+crypto") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have posted issues at #5387 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed it.
And I think we should re-consider the rule due to the illegal instruction
error when runtime detection
enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_*" |
mbedtls/include/mbedtls/check_config.h
Line 767 in daa6595
#error "Must use minimum -march=armv8-a+crypto for MBEDTLS_SHA256_USE_A64_CRYPTO_*" |
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.
7cebf0e
to
2876234
Compare
2876234
to
93c9e24
Compare
7b25375
to
18500f6
Compare
3a100de
to
f7c1dda
Compare
f7c1dda
to
330a0e0
Compare
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI is fully green |
@xkqian Have the changes you requested been addressed? |
All requested changes have been performed.
@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. |
Yes, all of them have been addressed, thanks for the reminder. |
Sure, all of my requests have been addressed. go ahead! |
Description
fix #5387
Add aes with Armv8 crypto extension
Gatekeeper checklist