8252204: AArch64: Implement SHA3 accelerator/intrinsic #207
Conversation
Contributed-by: ard.biesheuvel@linaro.org, dongbo4@huawei.com
|
👋 Welcome back fyang! A progress list of the required criteria for merging this PR into |
|
@RealFYang The following labels will be automatically applied to this pull request: When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the |
openjdk
bot
added
security
|
@ardbiesheuvel : Ard, could you please ack this patch? Thanks. |
|
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
|
|
/cc aarch64-port core-libs hotspot-compiler |
|
@RealFYang The label
|
|
/cc core-libs hotspot-compiler |
|
@RealFYang The |
|
/contributor add Ard Biesheuvel ard.biesheuvel@linaro.org |
|
@RealFYang Could not parse
|
|
/contributor add Bo Dong dongbo4@huawei.com |
|
@RealFYang Could not parse
|
|
/contributor add Ard Biesheuvel ard.biesheuvel@linaro.org |
|
@RealFYang |
|
/contributor add Bo Dong dongbo4@huawei.com |
|
@RealFYang |
|
Mailing list message from Andrew Haley on security-dev: On 17/09/2020 05:26, Fei Yang wrote:
Given that there's no real hardware, it's extra-important to add the -- |
|
@RealFYang this pull request can not be integrated into git checkout 8252204
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
openjdk
bot
removed
ready
|
/test |
|
@RealFYang you need to get approval to run the tests in tier1 for commits up until 0555170 |
Thanks for looking at this. We did run graalunit testing and added the following change in our first commit: diff --git a/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java b/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java
|
openjdk
bot
added
ready
| @@ -601,6 +601,7 @@ public CheckGraalIntrinsics() { | |||
| if (!config.useSHA512Intrinsics()) { | |||
| add(ignore, "sun/security/provider/SHA5." + shaCompressName + "([BI)V"); | |||
| } | |||
| add(toBeInvestigated, "sun/security/provider/SHA3." + shaCompressName + "([BI)V"); | |||
There was a problem hiding this comment.
This should be under if (isJDK16OrHigher()) check. Something like this:
https://github.com/openjdk/jdk/pull/650/files#diff-d1f378fc1b7fe041309e854d40b3a95a91e63fdecf0ecd9826b7c95eaeba314eR527
You can wait when Aleksey push it and update your changes
There was a problem hiding this comment.
OK. Will update with the following change after Aleksey's PR is integrated:
--- a/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java
+++ b/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java
@@ -608,6 +608,10 @@ public class CheckGraalIntrinsics extends GraalTest {
if (!config.useSHA512Intrinsics()) {
add(ignore, "sun/security/provider/SHA5." + shaCompressName + "([BI)V");
}
+
+ if (isJDK16OrHigher()) {
+ add(toBeInvestigated, "sun/security/provider/SHA3." + shaCompressName + "([BI)V");
+ }
}
There was a problem hiding this comment.
Done.
Commit: b43f919
I think this will not conflict with Aleksey's PR as we modify in different places of CheckGraalIntrinsics.java
That's appreciated. |
I started testing of 09: version. |
|
/contributor remove Bo Dong dongbo4@huawei.com |
|
@RealFYang |
|
/contributor add Dong Bo dongbo4@huawei.com |
|
@RealFYang |
Great to hear that :-) |
|
/integrate |
openjdk
bot
added
integrated
|
@RealFYang Since your change was applied there have been 65 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit b25d894. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
| __ bcax(v23, __ T16B, v23, v31, v24); | ||
| __ bcax(v24, __ T16B, v24, v8, v31); | ||
|
|
||
| __ ld1r(v31, __ T2D, __ post(rscratch1, 8)); |
There was a problem hiding this comment.
is it intentional to load 16 bytes and post-increment by 8?
There was a problem hiding this comment.
Actually, with the ld1r instruction the post increment should be the same as the size of the memory accessed. So T2D requires 8 as it reads 8 bytes(and duplicates it into both halves of the SIMD register).
There was a problem hiding this comment.
Thanks for the clarification!
Contributed-by: ard.biesheuvel@linaro.org, dongbo4@huawei.com
This added an intrinsic for SHA3 using aarch64 v8.2 SHA3 Crypto Extensions.
Reference implementation for core SHA-3 transform using ARMv8.2 Crypto Extensions:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/crypto/sha3-ce-core.S?h=v5.4.52
Trivial adaptation in SHA3. implCompress is needed for the purpose of adding the intrinsic.
For SHA3, we need to pass one extra parameter "digestLength" to the stub for the calculation of block size.
"digestLength" is also used in for the EOR loop before keccak to differentiate different SHA3 variants.
We added jtreg tests for SHA3 and used QEMU system emulator which supports SHA3 instructions to test the functionality.
Patch passed jtreg tier1-3 tests with QEMU system emulator.
Also verified with jtreg tier1-3 tests without SHA3 instructions on aarch64-linux-gnu and x86_64-linux-gnu, to make sure that there's no regression.
We used one existing JMH test for performance test: test/micro/org/openjdk/bench/java/security/MessageDigests.java
We measured the performance benefit with an aarch64 cycle-accurate simulator.
Patch delivers 20% - 40% performance improvement depending on specific SHA3 digest length and size of the message.
For now, this feature will not be enabled automatically for aarch64. We can auto-enable this when it is fully tested on real hardware. But for the above testing purposes, this is auto-enabled when the corresponding hardware feature is detected.
Progress
Testing
Failed test task
Issue
Reviewers
Contributors
<ard.biesheuvel@linaro.org><dongbo4@huawei.com>Download
$ git fetch https://git.openjdk.java.net/jdk pull/207/head:pull/207$ git checkout pull/207