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

Add BoringSSL Dispatch Test for aarch64 #1093

Merged
merged 25 commits into from
Jul 24, 2023

Conversation

billbo-yang
Copy link
Contributor

Issues:

Addresses CryptoAlg-1873 by unblocking #1045

Description of changes:

Currently AWS-LC only checks whether the correct hardware code is being reached on x86 and x86_64 and not aarch64. This PR fixes that so that we can ensure that the correct code path is taken whenever change OPENSSL_armcap on aarch64 systems.

Call-outs:

N/A

Testing:

Tests pass locally

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@billbo-yang billbo-yang force-pushed the add_arm_dispatch_test branch from 9f3f16f to c1055f7 Compare July 14, 2023 23:40
@billbo-yang billbo-yang marked this pull request as ready for review July 17, 2023 16:51
@billbo-yang billbo-yang requested review from nebeid and dkostic July 17, 2023 16:51
crypto/fipsmodule/cpucap/cpucap.c Outdated Show resolved Hide resolved
crypto/impl_dispatch_test.cc Outdated Show resolved Hide resolved
crypto/impl_dispatch_test.cc Outdated Show resolved Hide resolved
crypto/impl_dispatch_test.cc Outdated Show resolved Hide resolved
crypto/impl_dispatch_test.cc Show resolved Hide resolved
@billbo-yang billbo-yang requested a review from dkostic July 18, 2023 21:09
// 4: vpaes_encrypt
// 5: vpaes_set_encrypt_key
// 6: aesv8_gcm_8x_enc_128
#if defined(OPENSSL_AARCH64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to match the change made in cpucap.c to keep the larger array only?

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 must have missed this when removing the larger array, although it seems like it isn't causing a problem with the code working I'll remove it anyway so that the code will be cleaner / have fewer ifdefs :)

@billbo-yang billbo-yang force-pushed the add_arm_dispatch_test branch from 9f4e5e1 to d5bccb0 Compare July 19, 2023 22:10
@billbo-yang billbo-yang requested a review from nebeid July 19, 2023 22:41
Copy link
Contributor

@nebeid nebeid left a comment

Choose a reason for hiding this comment

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

The bits for SHA-256 and SHA-512 are not tested here. Do we want to include them or leave them to another PR?

@billbo-yang
Copy link
Contributor Author

billbo-yang commented Jul 20, 2023

The bits for SHA-256 and SHA-512 are not tested here. Do we want to include them or leave them to another PR?

These tests are looking to see if we're correctly hitting any special hardware instructions present on the CPU. We support the Intel SHA Extensions on supported x86 processors, hence why we test for it on x86 (as opposed to testing whether we hit the regular assembly instructions) in the dispatch test. However, looking at cpucap/internal.h, I don't see any support for any special SHA hardware extension on aarch64.

Given that on x86 we don't test to see if we hit the regular SHA assembly instructions, I don't think we need tests to hit the regular SHA assembly instruction on aarch64 either.

Edit: I had missed

tst w16,#ARMV8_SHA256
, which shows that we do in fact take advantage of hardware SHA instructions on armv8. Added the according tests.

@billbo-yang billbo-yang force-pushed the add_arm_dispatch_test branch from d5bccb0 to a8ca6e7 Compare July 20, 2023 21:35
@billbo-yang billbo-yang force-pushed the add_arm_dispatch_test branch from bf84411 to 1f542d8 Compare July 24, 2023 15:58
@billbo-yang billbo-yang requested a review from a team as a code owner July 24, 2023 15:58
@billbo-yang billbo-yang enabled auto-merge (squash) July 24, 2023 16:00
@billbo-yang billbo-yang merged commit 3d9a9ac into aws:main Jul 24, 2023
@skmcgrail skmcgrail mentioned this pull request Aug 1, 2023
@billbo-yang billbo-yang deleted the add_arm_dispatch_test branch January 11, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants