8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #410

theRealAph · 2021-09-24T12:19:15Z

This is a backport of the big AES/GCM patch from JDK head. It's a
major change and it's had very little time (almost a day) to mature in
head, so perhaps it shouldn't be backported to 11 for some time; I
wouldn't be at all surprised if some reviewers' reaction was "What
have you been smoking?" However, there is a good reason for a
backport: OpenJDK on x86 has a major advantage. AES/GCM is an
important cipher, the current AArch64 implementation is much slower
than x86, and some workloads are severely impacted.

I'm open to all arguments about why this should or shouldn't be pushed,
and I'm quite happy to wait for another release cycle or two if people
think that's the best course of action.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Xin Liu (@navyxliu - no project role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/410/head:pull/410
$ git checkout pull/410

Update a local copy of the PR:
$ git checkout pull/410
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/410/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 410

View PR using the GUI difftool:
$ git pr show -t 410

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/410.diff

bridgekeeper · 2021-09-24T12:20:08Z

👋 Welcome back aph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-09-24T12:21:18Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2021-09-24T12:27:14Z

Webrevs

simonis · 2021-09-24T15:14:28Z

I have no problem with you smoking whatsoever you like :)

But more serious. This isn't a full review but from a quick check it looks like this feature can easily turned off with -XX:-UseAESCTRIntrinsics, right? So in that case I have no objections of downporting this instantly to 11u. We might even be ultra-conservative and make -XX:-UseAESCTRIntrinsics the default in the downport, but I'm not sure it that's necessary.

Thanks for implementing this intrinsic and dowporting to 11u. It closes a real gap between x64 and aarch64,
Volker

theRealAph · 2021-09-24T15:43:39Z

But more serious. This isn't a full review but from a quick check it looks like this feature can easily turned off with -XX:-UseAESCTRIntrinsics, right? So in that case I have no objections of downporting this instantly to 11u. We might even be ultra-conservative and make -XX:-UseAESCTRIntrinsics the default in the downport, but I'm not sure it that's necessary.

I hadn't considered that idea, but it makes sense to turn it off by default. The problem is, I guess, that many customers won't know about it, would have poor performance, and maybe blame AArch64 servers.

shipilev · 2021-10-05T08:35:05Z

Since this is math-heavy crypto code, I would rather wait for somebody to use/real-test the upstream implementation first. Unfortunately, timing is against us for JDK 18, as it would release only in March, and so if we want JDK 18 to be proven to work first, this would mean slipping the 11u backport to July 2022.

So I would propose this: wait another 6..8 weeks to see if AArch64 regressions are reported in mainline, then backport this to 11u for January 2022, disabled by default. Have a 11u-specific issue to remember enabling it by default after JDK 18 graduates and people get exposed to this code by default in JDK 18. This way we could also see some opt-in 11u testing (which is presumably a larger population than JDK 18 adopters).

It would also help if we were able to throw some targeted testing for mainline code. Are there known good crypto test suites that could poke the holes in this implementation?

shipilev · 2021-10-05T08:39:17Z

Now that I wrote this, another question: what about 17u? I think it is easier/cleaner to backport there first.

theRealAph · 2021-10-05T08:52:26Z

Since this is math-heavy crypto code, I would rather wait for somebody to use/real-test the upstream implementation first. Unfortunately, timing is against us for JDK 18, as it would release only in March, and so if we want JDK 18 to be proven to work first, this would mean slipping the 11u backport to July 2022.

So I would propose this: wait another 6..8 weeks to see if AArch64 regressions are reported in mainline, then backport this to 11u for January 2022, disabled by default.

OK. I'm not sure that disabling by default much reduces risk, because this patch does a fair bit of refactoring and commoning, even if the fast paths aren't enabled.

Have a 11u-specific issue to remember enabling it by default after JDK 18 graduates and people get exposed to this code by default in JDK 18. This way we could also see some opt-in 11u testing (which is presumably a larger population than JDK 18 adopters).

Seems reasonable.

It would also help if we were able to throw some targeted testing for mainline code. Are there known good crypto test suites that could poke the holes in this implementation?

Authenticated encryption is mostly self-testing: that's the idea.
I'm trying to think of where the errors might be.

Buffer overruns / other heap corruption.
Incorrect encryption/decryption when update() is called with oddly-sized byte[].
Failure to detect bad authentication tag.
Timing vulnerabilities due to plaintext-dependent branches or loads.

The upper layers are passed the auth data, which they check, and the jtreg tests run the intrinsic against the pure-Java code. That should be enough to make sure we don't have an incorrect (but self-consistent) implementation.

External test suites might not run for long enough to test the intrinsic. I'll have a look at Wycheproof.

mlbridge · 2021-10-05T08:58:41Z

Mailing list message from Andrew Haley on jdk-updates-dev:

On 10/5/21 09:42, Aleksey Shipilev wrote:

Now that I wrote this, another question: what about 17u? I think it is easier/cleaner to backport there first.

https://github.com/openjdk/jdk17u/pull/87

theRealAph · 2021-11-02T12:15:50Z

I've reworked this to leave the existing code, so that unless -XX:+UseAESCTRIntrinsics is used on the command line this patch is entirely disabled.

shipilev

Looks fine to me.

shipilev · 2021-11-03T09:31:26Z

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

+    if (FLAG_IS_DEFAULT(UseAESCTRIntrinsics)) {
+      FLAG_SET_DEFAULT(UseAESCTRIntrinsics, false);
+    }


UseAESCTRIntrinsics is already false by default, so this seems to be excessive. No harm doing this anyway.

openjdk · 2021-11-03T09:34:06Z

@theRealAph This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions

Reviewed-by: shade, xliu

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 243 new commits pushed to the master branch:

2fb8ca2: 8273026: Slow LoginContext.login() on multi threading application
e2b7f1e: 8210198: Clean up JNI_ENV_ARG for vmTestbase/jvmti/Get[A-F] tests
770a78a: 8210182: Remove macros for C compilation from vmTestBase but non jvmti
63277a4: 8226943: compile error in libfollowref003.cpp with XCode 10.2 on macosx
4b591db: 8209611: use C++ compiler for hotspot tests
5e08548: 8274860: gcc 10.2.1 produces an uninitialized warning in sharedRuntimeTrig.cpp
9b94409: 8277029: JMM GetDiagnosticXXXInfo APIs should verify output array sizes
ed9a41b: 8276536: Update TimeZoneNames files to follow the changes made by JDK-8275766
ccd7a6f: 8275766: (tz) Update Timezone Data to 2021e
d9b2c4c: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
... and 233 more: https://git.openjdk.java.net/jdk11u-dev/compare/cbe5a58e3ac3941d6bda33b6353c1ced41a12ae7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

RealCLanger · 2021-11-03T22:04:17Z

@theRealAph, could you trigger a GHA run by first checking that it is enabled here - https://github.com/theRealAph/jdk11u-dev/actions - and then merging in current master branch?

navyxliu

LGTM.
I wish this is on by default, but I understand your concern. UseAESCTRIntrinsics is a diagnostic parameter. At least, it will allow bravehearts to try it out.

theRealAph · 2021-11-24T16:44:51Z

/integrate

openjdk · 2021-11-24T16:46:22Z

Going to push as commit b3ef0ea.
Since your change was applied there have been 251 commits pushed to the master branch:

6a7a2d2: 8276139: TestJpsHostName.java not reliable, better to expand HostIdentifierCreate.java test
cd40782: 8210700: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti/unit tests
2e16341: 8210481: Remove #ifdef cplusplus from vmTestbase
4b5075e: 8210665: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti[R-U] tests
8fcfaef: 8210593: Clean up JNI_ENV_ARG and factorize the macros for vmTestbase/jvmti[N-R] tests
46c41b6: 8210429: Clean up JNI_ENV_ARG for vmTestbase/jvmti/Get[G-Z] tests
06e9f96: 8210242: [TESTBUG] vmTestbase/nsk/stress/jni/jnistress001.java crashes with EXCEPTION_ACCESS_VIOLATION on windows-x86
dcc010d: 8273235: tools/launcher/HelpFlagsTest.java Fails on Windows 32bit
2fb8ca2: 8273026: Slow LoginContext.login() on multi threading application
e2b7f1e: 8210198: Clean up JNI_ENV_ARG for vmTestbase/jvmti/Get[A-F] tests
... and 241 more: https://git.openjdk.java.net/jdk11u-dev/compare/cbe5a58e3ac3941d6bda33b6353c1ced41a12ae7...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-24T16:46:46Z

@theRealAph Pushed as commit b3ef0ea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

This PR adds options -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics to the jvm.config files to enable intrinsic support for AES CTR/GCM on ARM64. It improves performance of network communication with S3 a lot on graviton instances. It's enabled in Java 18 by default, which is already released. Therefore risk is minimal. In latest Java (OpenJDK) versions the performance of AES CTR/GCM for AARCH64 was significally improved and it was backported to OpenJDK 11. Backport PR: openjdk/jdk11u-dev#410. Some additional explanation is here: aws/aws-graviton-getting-started#110 (comment) The original OpenJDK issue is here: https://bugs.openjdk.java.net/browse/JDK-8267993 To use that backport we need to enable it explicitly by enabling -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics. It was not enabled by default in the backport because of conservative approach.

This PR adds options -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics to the JVM's options to enable intrinsic support for AES CTR/GCM on ARM64. It improves performance of network communication with S3 a lot on graviton instances. It's enabled in Java 18 by default, which is already released. Therefore risk is minimal. In latest Java (OpenJDK) versions the performance of AES CTR/GCM for AARCH64 was significally improved and it was backported to OpenJDK 11. Backport PR: openjdk/jdk11u-dev#410. Some additional explanation is here: aws/aws-graviton-getting-started#110 (comment) The original OpenJDK issue is here: https://bugs.openjdk.java.net/browse/JDK-8267993 To use that backport we need to enable it explicitly by enabling -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics. It was not enabled by default in the backport because of conservative approach.

This PR adds options -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics to the jvm.config files to enable intrinsic support for AES CTR/GCM on ARM64. It improves performance of network communication with S3 a lot on graviton instances. It's enabled in Java 18 by default, which is already released. Therefore risk is minimal. In latest Java (OpenJDK) versions the performance of AES CTR/GCM for AARCH64 was significally improved and it was backported to OpenJDK 11. Backport PR: openjdk/jdk11u-dev#410. Some additional explanation is here: aws/aws-graviton-getting-started#110 (comment) The original OpenJDK issue is here: https://bugs.openjdk.java.net/browse/JDK-8267993 To use that backport we need to enable it explicitly by enabling -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics. It was not enabled by default in the backport because of conservative approach.

This PR adds options -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics to the JVM's options to enable intrinsic support for AES CTR/GCM on ARM64. It improves performance of network communication with S3 a lot on graviton instances. It's enabled in Java 18 by default, which is already released. Therefore risk is minimal. In latest Java (OpenJDK) versions the performance of AES CTR/GCM for AARCH64 was significally improved and it was backported to OpenJDK 11. Backport PR: openjdk/jdk11u-dev#410. Some additional explanation is here: aws/aws-graviton-getting-started#110 (comment) The original OpenJDK issue is here: https://bugs.openjdk.java.net/browse/JDK-8267993 To use that backport we need to enable it explicitly by enabling -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics. It was not enabled by default in the backport because of conservative approach.

theRealAph added 2 commits September 23, 2021 10:35

Backport 4f3b626a36319cbbbbdcb1c02a84486a3d4eddb6

bcdedfc

Backport 4f3b626a36319cbbbbdcb1c02a84486a3d4eddb6

a603065

openjdk bot changed the title ~~Backport 4f3b626a36319cbbbbdcb1c02a84486a3d4eddb6~~ 8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions Sep 24, 2021

openjdk bot added backport rfr Pull request is ready for review labels Sep 24, 2021

Whitespace

17baf7d

theRealAph added 2 commits October 27, 2021 07:13

Cleanup, delete dead function declarations.

df0b6f5

Backport 4f3b626a36319cbbbbdcb1c02a84486a3d4eddb6

88e7124

shipilev approved these changes Nov 3, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2021

navyxliu approved these changes Nov 22, 2021

View reviewed changes

navyxliu mentioned this pull request Nov 22, 2021

Performance Benchmarking Result of Kafka with CLient Encryption in Graviton is Worse compared to Non-Graviton aws/aws-graviton-getting-started#110

Closed

openjdk bot closed this Nov 24, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 24, 2021

radek-kondziolka mentioned this pull request May 5, 2022

Remove -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics from jvm.config. trinodb/trino#12253

Closed

findepi mentioned this pull request May 5, 2022

Add -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics trinodb/trino#12251

Merged

radek-kondziolka mentioned this pull request May 11, 2022

Add -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics trinodb/charts#47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #410

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #410

theRealAph commented Sep 24, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 24, 2021

openjdk bot commented Sep 24, 2021

mlbridge bot commented Sep 24, 2021 •

edited

Loading

simonis commented Sep 24, 2021

theRealAph commented Sep 24, 2021

shipilev commented Oct 5, 2021 •

edited

Loading

shipilev commented Oct 5, 2021

theRealAph commented Oct 5, 2021 •

edited

Loading

mlbridge bot commented Oct 5, 2021

theRealAph commented Nov 2, 2021 •

edited

Loading

shipilev left a comment

shipilev Nov 3, 2021

navyxliu Nov 22, 2021

openjdk bot commented Nov 3, 2021 •

edited

Loading

RealCLanger commented Nov 3, 2021

navyxliu left a comment

theRealAph commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #410

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #410

Conversation

theRealAph commented Sep 24, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 24, 2021

openjdk bot commented Sep 24, 2021

mlbridge bot commented Sep 24, 2021 • edited Loading

Webrevs

simonis commented Sep 24, 2021

theRealAph commented Sep 24, 2021

shipilev commented Oct 5, 2021 • edited Loading

shipilev commented Oct 5, 2021

theRealAph commented Oct 5, 2021 • edited Loading

mlbridge bot commented Oct 5, 2021

theRealAph commented Nov 2, 2021 • edited Loading

shipilev left a comment

Choose a reason for hiding this comment

shipilev Nov 3, 2021

Choose a reason for hiding this comment

navyxliu Nov 22, 2021

Choose a reason for hiding this comment

openjdk bot commented Nov 3, 2021 • edited Loading

RealCLanger commented Nov 3, 2021

navyxliu left a comment

Choose a reason for hiding this comment

theRealAph commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

theRealAph commented Sep 24, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 24, 2021 •

edited

Loading

shipilev commented Oct 5, 2021 •

edited

Loading

theRealAph commented Oct 5, 2021 •

edited

Loading

theRealAph commented Nov 2, 2021 •

edited

Loading

openjdk bot commented Nov 3, 2021 •

edited

Loading