Skip to content
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

AES: Add accelerator only mode #7384

Merged
Merged
Changes from 1 commit
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
0d4f4e5
Add option to disable built-in aes implementation.
yuhaoth Mar 31, 2023
d767cc4
Add accelerator only tests.
yuhaoth Mar 31, 2023
2f26a59
Add std output information for AESCE in gcm
yuhaoth Mar 31, 2023
315fd30
Rename plain c disable option
yuhaoth Apr 18, 2023
4d030f3
Add check for no aes implementation provided
yuhaoth Apr 18, 2023
1b3ab36
Update comments
yuhaoth Apr 18, 2023
3fcf2b5
Rename HAS_NO_PLAIN_C to DONT_USE_SOFTWARE_CRYPTO
yuhaoth Apr 18, 2023
8840a8c
fix wrong checks
yuhaoth Apr 19, 2023
3660623
Rename plain c option and update comments
yuhaoth Apr 19, 2023
d76ded0
fix various issues
yuhaoth Apr 19, 2023
4dfbb2e
add changelog entry
yuhaoth Apr 23, 2023
02b1519
move accelerator checks to `aes.c`
yuhaoth Apr 23, 2023
9e3e3dd
Fix code-style too-long line fail
yuhaoth Apr 24, 2023
e77c4d9
Mention the crash risk without runtime detection
yuhaoth Apr 24, 2023
6943681
Improve error message and documents
yuhaoth Apr 25, 2023
1414029
improve document about hardware only
yuhaoth Aug 1, 2023
69dd441
Remove test_aes_*
yuhaoth Aug 2, 2023
1221a31
Run aes tests only for test_aesni
yuhaoth Aug 3, 2023
17a9d2e
Add MBEDTLS_AES_USE_HADWARE_ONLY for test_aesni
yuhaoth Aug 3, 2023
8a599c0
Add aesni only test
yuhaoth Aug 3, 2023
193cbc0
Add aesce build test
yuhaoth Aug 3, 2023
c935aa6
Add via padlock build test
yuhaoth Aug 3, 2023
2700ef6
Add aesce test string filter
yuhaoth Aug 3, 2023
29c91ba
fix unreachable code warnings
yuhaoth Aug 4, 2023
b241db3
remove padlock only mode
yuhaoth Aug 4, 2023
fce351d
improve platform relative check
yuhaoth Aug 4, 2023
9c0b7d1
Remove unnecessary name check tag
yuhaoth Aug 4, 2023
7802f65
Add negative test for aesni only
yuhaoth Aug 7, 2023
5fcdd6a
remove unnecessary definition
yuhaoth Aug 7, 2023
c4508c0
improve error message and config check for padlock
yuhaoth Aug 8, 2023
a7de78d
improve test
yuhaoth Aug 8, 2023
76a51b9
replace strings command with grep
yuhaoth Aug 8, 2023
ba42b07
Remove asm check for aarch64 aesce
yuhaoth Aug 10, 2023
13696bb
improve check config option for i386
yuhaoth Aug 10, 2023
8189f32
improve aesni check for x86_64
yuhaoth Aug 10, 2023
240bb11
Add gnu check for aseni assembly code
yuhaoth Aug 11, 2023
e62ff09
Restore aesni for i386
yuhaoth Aug 16, 2023
cc068ae
fix `-Werror=return-type` when runtime detection enabled and plain c …
yuhaoth Aug 16, 2023
c628486
enable runtime detection when padlock enabled and plain c disabled
yuhaoth Aug 16, 2023
b6d39c2
Add aesni test for i386
yuhaoth Aug 16, 2023
506759f
fix build fail for via padlock test
yuhaoth Aug 16, 2023
3ce0398
Add compiler cflags error message
yuhaoth Aug 16, 2023
516cf27
fix msvc build fail on i386 target
yuhaoth Aug 16, 2023
bdd96b9
disable aesni for componets without cpu modifiers
yuhaoth Aug 16, 2023
35b59d7
exclude arm64ec mode for aesni
yuhaoth Aug 17, 2023
2319af0
Change the order of runtime detection
yuhaoth Aug 17, 2023
9e62862
Add via padlock detection macro
yuhaoth Aug 17, 2023
1b4c7ed
add hardware only check for padlock
yuhaoth Aug 17, 2023
f258d17
remove aesni + padlock - plain c tests
yuhaoth Aug 17, 2023
e9c6b53
remove return-type when runtime detection enabled without plain c
yuhaoth Aug 17, 2023
6c6b9f6
Change document to match real status
yuhaoth Aug 17, 2023
3a0f044
improve readability
yuhaoth Aug 17, 2023
9608447
replace padlock_c with padlock_have_code
yuhaoth Aug 17, 2023
372f7a0
Add missing check
yuhaoth Aug 18, 2023
61fc5ed
improve readability of error message
yuhaoth Aug 18, 2023
0a6272d
revert padlock from aesni module
yuhaoth Aug 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix msvc build fail on i386 target
Signed-off-by: Jerry Yu <[email protected]>
yuhaoth committed Aug 16, 2023
commit 516cf27d45cfb3e44960641e0924ca0a8461360f
10 changes: 5 additions & 5 deletions library/aes.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to auto-enable MBEDTLS_AES_[EN|DE]CRYPT_ALT to remove the plain C implementations when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. MBEDTLS_AES_[EN|DE]CRYPT_ALT are for user provided functions. If it is provided, MBEDTLS_AES_USE_HARDWARE_ONLY will not work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When MBEDTLS_AES_[EN|DE]CRYPT_ALT are enabled, user need to provide their own implementations for mbedtls_internal_aes_decrypt and mbedtls_internal_aes_encrypt, so MBEDTLS_AES_USE_HARDWARE_ONLY would not be affected.

What I want to ask is, do we need to eliminate the plain C implementation when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled so that both symbols won't be built into the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I want to ask is, do we need to eliminate the plain C implementation when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled so that both symbols won't be built into the library.

Yes.

The first name of MBEDTLS_AES_USE_HARDWARE_ONLY is MBEDTL_AES_DISABLE_PLAIN_C . :) .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think it should just remove plain C . MBEDTLS_AES_[EN|DE]CRYPT_ALT will remove both hardware and software built-in aes

Original file line number Diff line number Diff line change
@@ -71,7 +71,7 @@

#if !defined(MBEDTLS_AES_ALT)

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86) && defined(__GNUC__)
static int aes_padlock_ace = -1;
#endif

@@ -578,7 +578,7 @@ static unsigned mbedtls_aes_rk_offset(uint32_t *buf)
#if defined(MAY_NEED_TO_ALIGN)
int align_16_bytes = 0;

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86) && defined(__GNUC__)
if (aes_padlock_ace == -1) {
aes_padlock_ace = mbedtls_padlock_has_support(MBEDTLS_PADLOCK_ACE);
}
@@ -1102,7 +1102,7 @@ int mbedtls_aes_crypt_ecb(mbedtls_aes_context *ctx,
}
#endif

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86) && defined(__GNUC__)
if (aes_padlock_ace > 0) {
return mbedtls_padlock_xcryptecb(ctx, mode, input, output);
}
@@ -1148,7 +1148,7 @@ int mbedtls_aes_crypt_cbc(mbedtls_aes_context *ctx,
return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH;
}

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86) && defined(__GNUC__)
if (aes_padlock_ace > 0) {
if (mbedtls_padlock_xcryptcbc(ctx, mode, length, iv, input, output) == 0) {
return 0;
@@ -1900,7 +1900,7 @@ int mbedtls_aes_self_test(int verbose)
#if defined(MBEDTLS_AES_ALT)
mbedtls_printf(" AES note: alternative implementation.\n");
#else /* MBEDTLS_AES_ALT */
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86) && defined(__GNUC__)
if (mbedtls_padlock_has_support(MBEDTLS_PADLOCK_ACE)) {
mbedtls_printf(" AES note: using VIA Padlock.\n");
} else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my reading of the code, if padlock is enabled, and AESNI is supported by the configuration and the hardware, we will print "using VIA padlock" (correct), we will not print "AES note: AESNI code present" (incorrect), and we will print "AES note: using AESNI" (incorrect).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's base on the CPU feature sets. If AESNI is enabled, it will print "AES note: AESNI code present"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have change the order to match order in mbedtls_aes_crypt_ecb and mbedtls_aes_crypt_cbc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issues here are:

  1. The order of the outputing implementation indication message don't match the implementation selection order in mbedtls_aes_crypt_*.
  2. As I have asked in Prefer intrinsics over asm for AES-NI #7630 (comment), if we built with MBEDTLS_PADLOCK_C and MBEDTLS_AESNI_C, and run on padlock supported hardware, we would get AES note: using VIA Padlock. and AES note: built-in implementation..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now LGTM