RE: RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic
Hi, > -Original Message- > From: hotspot-dev [mailto:hotspot-dev-r...@openjdk.java.net] On Behalf > Of Andrew Haley > Sent: Friday, September 18, 2020 9:48 PM > To: Fei Yang ; hotspot-...@openjdk.java.net; > security-...@openjdk.java.net > Subject: Re: RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic > > On 17/09/2020 05:26, Fei Yang wrote: > > 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. > > Given that there's no real hardware, it's extra-important to add the new > instructions to cpu/aarch64/aarch64-asmtest.py and regenerate the test in > assembler_aarch64.cc:asm_check. I have added on commit in PR resolving this: https://github.com/openjdk/jdk/pull/207/commits/04bdb42e971aa1c2f78bb5c916db62910e167053?file-filters%5B%5D= I grouped SHA512SIMDOp, SHA3SIMDOp and SVEVectorOp after the # ARMv8.2A comment. So anticipate more changes in file assembler_aarch64.cpp. BTW: If this feature is not auto-enabled when the SHA3 hardware feature is there, we will have one failure for the following test: test/hotspot/jtreg/compiler/intrinsics/sha/cli/TestUseSHA3IntrinsicsOptionOnSupportedCPU.java 15 #-testresult- 16 description=file\:/home/yangfei/github/jdk/test/hotspot/jtreg/compiler/intrinsics/sha/cli/TestUseSHA3IntrinsicsOptionOnSupportedCPU.java 17 elapsed=31546 0\:00\:31.546 18 end=Mon Sep 21 10\:27\:58 CST 2020 19 environment=regtest 20 execStatus=Failed. Execution failed\: `main' threw exception\: java.lang.AssertionError\: Option 'UseSHA3Intrinsics' is expected to have 'true' value Option 'UseSHA3Intrinsics' should be enabled by default Any suggestions for this? Thanks, Felix
RE: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic
Hi all, > -Original Message- > From: Sean Mullan [mailto:sean.mul...@oracle.com] > Sent: Wednesday, September 9, 2020 12:16 AM > To: Andrew Haley ; Yangfei (Felix) > ; hotspot-compiler-...@openjdk.java.net; core- > libs-...@openjdk.java.net > Cc: aarch64-port-...@openjdk.java.net; Stuart Monteith > > Subject: Re: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3 > accelerator/intrinsic > > Since this change affects security code, please make sure you add security- > d...@openjdk.java.net on any followup code reviews. Thanks for reminding that. I have just switched to github and created a PR for this issue: https://github.com/openjdk/jdk/pull/207 Let's switch to this PR for the followup code reviews. I see security and hotspot labels was automatically added by the bots. I also added labels for core-libs and hotspot-compiler. > > On 9/1/20 10:44 AM, Andrew Haley wrote: > > On 01/09/2020 11:53, Yangfei (Felix) wrote: > >> Sure, I am happy if the original author of the assembly code or someone > else from Linaro could help here. > >> I wasn't aware there was such an requirement here given that assembly > code is licensed under GPL. > > > > There sure is. All code must be contributed by its owner and put on > > the cr.openjdk site. Especially GPL code. I have added ard.biesheu...@linaro.org to Contributed-by: in the git commit msg. And the newly created PR was Acked-by: Ard Biesheuvel ard.biesheu...@linaro.org Hope this addresses Andrew's concern. Best regards, Felix
RE: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic
Hi, > -Original Message- > From: Andrew Haley [mailto:a...@redhat.com] > Sent: Tuesday, September 1, 2020 3:52 PM > To: Yangfei (Felix) ; hotspot-compiler- > d...@openjdk.java.net; core-libs-dev@openjdk.java.net > Cc: aarch64-port-...@openjdk.java.net; Stuart Monteith > > Subject: Re: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3 > accelerator/intrinsic > > On 31/08/2020 10:46, Yangfei (Felix) wrote: > > > >> -Original Message- > >> From: Andrew Haley [mailto:a...@redhat.com] > >> Sent: Monday, August 31, 2020 4:41 PM On 31/08/2020 07:50, Yangfei > >> (Felix) wrote: > >>> > >> > >> This looks like a direct copy of the sha3-cecore.S file.You'll need > >> Linaro to contribute it. I don't imagine they'll have any problem > >> with that: they are OCA signatories > > > >> Also, given that we've got the assembly source file, why not just > >> copy that into OpenJDK? I can't see the point rewriting it into the HotSpot > assembler. > > > > Actually, we referenced the existing intrinsics implementation and > > took a similar way. It looks strange to have one intrinsic that goes > > differently. And we won't be able to emit this code on demand if we > > go that different way. Some cpu does not support these special sha3 > > instructions and thus does need this code at all. I think that's one > > advantage of using a stub. > > OK. But you'll still need Linaro to contribute it to OpenJDK. We could ask > Stuart to help with that. Sure, I am happy if the original author of the assembly code or someone else from Linaro could help here. I wasn't aware there was such an requirement here given that assembly code is licensed under GPL. Should we separate the patch into two parts: changes for the shared code part and the aarch64 port-specific changes? Thanks, Felix
RE: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic
> -Original Message-
> From: Andrew Haley [mailto:a...@redhat.com]
> Sent: Monday, August 31, 2020 4:41 PM
> To: Yangfei (Felix) ; hotspot-compiler-
> d...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Cc: aarch64-port-...@openjdk.java.net
> Subject: Re: [aarch64-port-dev ] RFR: 8252204: AArch64: Implement SHA3
> accelerator/intrinsic
>
> On 31/08/2020 07:50, Yangfei (Felix) wrote:
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8252204
> > Webrev: http://cr.openjdk.java.net/~fyang/8252204/webrev.00/
> >
> > 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/ar
> m64/crypto/sha3-cecore.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.
> >
> >Comments?
>
> This looks like a direct copy of the sha3-cecore.S file.You'll need Linaro to
> contribute it. I don't imagine they'll have any problem with that: they are
> OCA signatories
Since the code in sha3-cecore.S works in kernel space, we need several
modifications here to makes it work in hotspot.
First, we need to add callee-save & restore for d8 - d15 according to the
aarch64 abi.
Also, the following code snippet is not needed for user-space:
if_will_cond_yield_neon
add x8, x19, #32
st1 { v0.1d- v3.1d}, [x19]
st1 { v4.1d- v7.1d}, [x8], #32
st1 { v8.1d-v11.1d}, [x8], #32
st1 {v12.1d-v15.1d}, [x8], #32
st1 {v16.1d-v19.1d}, [x8], #32
st1 {v20.1d-v23.1d}, [x8], #32
st1 {v24.1d}, [x8]
do_cond_yield_neon
b 0b
endif_yield_neon
And we need to handle the multi-block case differently for
StubRoutines::sha3_implCompressMB:
3485 if (multi_block) {
3486 // block_size = 200 - 2 * digest_length, ofs += block_size
3487 __ add(ofs, ofs, 200);
3488 __ sub(ofs, ofs, digest_length, Assembler::LSL, 1);
3489
3490 __ cmp(ofs, limit);
3491 __ br(Assembler::LE, sha3_loop);
3492 __ mov(c_rarg0, ofs); // return ofs
3493 }
And StubRoutines::sha3_implCompress does not even need this multi-block check
logic.
> Also, given that we've got the assembly source file, why not just copy that
> into OpenJDK? I can't see the point rewriting it into the HotSpot assembler.
Actually, we referenced the existing intrinsics implementation and took a
similar way. It looks strange to have one intrinsic that goes differently.
And we won't be able to emit this code on demand if we go that different way.
Some cpu does not support these special sha3 instructions and thus does need
this code at all.
I think that's one advantage of using a stub.
Thanks,
Felix
RFR: 8252204: AArch64: Implement SHA3 accelerator/intrinsic
Hi, Bug: https://bugs.openjdk.java.net/browse/JDK-8252204 Webrev: http://cr.openjdk.java.net/~fyang/8252204/webrev.00/ 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. Comments? Thanks, Felix
RE: [PING?] RFR(S): 8248910: NPE when freeing the memory for a slice from a buffer
Hi Alan, > -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Friday, July 31, 2020 4:35 PM > To: Yangfei (Felix) ; core-libs- > d...@openjdk.java.net > Subject: Re: [PING?] RFR(S): 8248910: NPE when freeing the memory for a > slice from a buffer > > On 31/07/2020 02:45, Yangfei (Felix) wrote: > > Ping... Any suggestions? > > > Can you bring this to nio-dev for discussion? I don't know what "Randoop" is > but it seems to be using sun.nio.ch.Util directly. I think I'd like to > understand > if there is a case where we would actually attempt to free a slice when using > the exported buffer APIs. Thanks for the suggestion. I posted a new mail here on nio-dev: https://mail.openjdk.java.net/pipermail/nio-dev/2020-August/007419.html Felix
[PING?] RFR(S): 8248910: NPE when freeing the memory for a slice from a buffer
Ping... Any suggestions? Thanks, Felix > -Original Message- > From: Yangfei (Felix) > Sent: Wednesday, July 8, 2020 11:35 AM > To: core-libs-dev@openjdk.java.net > Subject: RFR(S): 8248910: NPE when freeing the memory for a slice from a > buffer > > Hi, > > Bug: https://bugs.openjdk.java.net/browse/JDK-8248910 > Webrev: http://cr.openjdk.java.net/~fyang/8248910/webrev.00/ > > The overall procedure is described by the newly add test. > For a slice from a ByteBuffer, it does not have a cleaner, which leads to > the > NPE. > Here, I thinks we should not try to free the memory of a slice. > Proposed fix adds a null check for cleaner in Util.free before invoking > its > clean() method. > Another possible way would be catching slice in Util. > releaseTemporaryDirectBuffer and exclude it. > > Tier1-3 tested on x86_64-linux-gnu. Newly add test fail without the fix > and > pass otherwise. > Comments? > > Thanks, > Felix
RFR(S): 8248910: NPE when freeing the memory for a slice from a buffer
Hi, Bug: https://bugs.openjdk.java.net/browse/JDK-8248910 Webrev: http://cr.openjdk.java.net/~fyang/8248910/webrev.00/ The overall procedure is described by the newly add test. For a slice from a ByteBuffer, it does not have a cleaner, which leads to the NPE. Here, I thinks we should not try to free the memory of a slice. Proposed fix adds a null check for cleaner in Util.free before invoking its clean() method. Another possible way would be catching slice in Util. releaseTemporaryDirectBuffer and exclude it. Tier1-3 tested on x86_64-linux-gnu. Newly add test fail without the fix and pass otherwise. Comments? Thanks, Felix
RE: RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
Hi, > -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Thursday, March 12, 2020 5:40 PM > To: Yangfei (Felix) ; core-libs-dev@openjdk.java.net > Subject: Re: RFR(S): 8240734: ModuleHashes attribute not reproducible > between builds > > On 12/03/2020 09:30, Yangfei (Felix) wrote: > > So for the hashed to be stored in A, we may need relations like: B requires > > A, > and C requires A. Am I right? > > > Right, I see there was a typo in my mail, sorry about that. It might help to > draw > the DAG and then reverse the edges to easily identify the modules where the > hashes will be stored. > We did a quick try with three modules. The new test is uploaded to [1]. Two problems here: a> hash values are different for two different test run. This is illustrate in [2]. I think this is introduced by the SHA-256 algorithm. I didn't mention that our local consistency check is carried out in a specialized environment which avoids such a problem. This means we cannot compare the jmod files with Files.mismatch directly in regular testing environment. b> Looks like the reported issue cannot be reproduced by a small number of modules. We tried [1] many times and it seems that the order of the two module hashes is stable. Note: java.base module records hashes of 67 other modules. Suggestions? [1] https://bugs.openjdk.java.net/secure/attachment/87281/ModuleHashOrderTest.java [2] Classfile /home/sunjianye/openjdk13-community/jdk/JTwork/scratch/jmods-first/cl| Classfile /home/sunjianye/openjdk13-community/jdk/JTwork/scratch/jmods-second/ Last modified Mar 12, 2020; size 399 bytes |Last modified Mar 12, 2020; size 399 bytes SHA-256 checksum 1a8ea671cd45ffb96287d690e383a0f80303111ac51519271a097007b8e0|SHA-256 checksum b7b0ce1af32eb74d2185e4feced2f25ae23ef2b3830b1fe2b7471ba9b34 Compiled from "module-info.java" |Compiled from "module-info.java" module ma | module ma minor version: 0 |minor version: 0 major version: 57 |major version: 57 flags: (0x8000) ACC_MODULE |flags: (0x8000) ACC_MODULE this_class: #2 // "module-info" |this_class: #2 // "module-info" + +-- 51 lines: super_class: + +#0--|+ +-- 51 lines: + +super_class: #0- #17 // src.ma |#17 // src.ma ModuleHashes: | ModuleHashes: algorithm: #25 // SHA-256 |algorithm: #25 // SHA-256 2 // hashes |2 // hashes #27 // mb |#27 // mb hash_length: 32 |hash_length: 32 hash: [633a60ff4fa542d082dba41ced181633385574c6a801c843a23538559b63c239] |hash: [951bb41ce54aa1bc93edde4226eeaf38d73f6a54b9e9e150c73e8a48d109de57] #29 // mc |#29 // mc hash_length: 32 |hash_length: 32 hash: [815cb047f93ec3d22ae2889d32a7f687e00b41507abbb1b66a27d87c5c38459a] |hash: [e2c37c902bf8b84ebbb11a765b365ba0fc241772596a8e4091e0345d596a86e5]
RE: RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
Hi, > -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Thursday, March 12, 2020 4:47 PM > To: Yangfei (Felix) ; core-libs-dev@openjdk.java.net > Subject: Re: RFR(S): 8240734: ModuleHashes attribute not reproducible > between builds > > The `jmod` and `jar` tools can be used to compute the hashes of a set of > intimately connected modules. If modules A, B and C are tightly connected, and > A requires B and C, then you can use the tooling to create a packaged form of > A > that includes the hashes of B and C. The hashes are checked at link-time (and > in According to [1], if A requires B and C, then I suppose the hashes will be included in B and C instead of A when we do 'jmod hash --module-path jmods --hash-modules .*'. So for the hashed to be stored in A, we may need relations like: B requires A, and C requires A. Am I right? > the case of modular JARs on the modular path, at run-time too). So I think > you'll > need a new test. It might be that running `jmod --hash-modules` with just two > dependences is enough to have the entries written to the ModuleHashes > attribute in random order. A buddy test to JLinkReproducibleTest that creates > a > run-time image containing "user modules" that are tightly connected would be > a good test. OK, we will try to add a buddy test accordingly. Thanks, Felix [1] https://docs.oracle.com/javase/9/tools/jmod.htm#JSWOR-GUID-0A0BDFF6-BE34-461B-86EF-AAC9A555E2AE
RE: RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
Hi Alan,
> -Original Message-
> From: Alan Bateman [mailto:alan.bate...@oracle.com]
> Sent: Wednesday, March 11, 2020 3:03 PM
> To: Yangfei (Felix) ; core-libs-dev@openjdk.java.net
> Subject: Re: RFR(S): 8240734: ModuleHashes attribute not reproducible
> between builds
>
> On 11/03/2020 01:11, Yangfei (Felix) wrote:
> > Thanks for reviewing this. Pushed.
> Okay but I think we need to add a test for this. Can you look at
> JLinkReproducibleTest and see if it can be extended to use
> --keep-packaged-modules so that you have the JMOD files in the run-time
> image.
I looked into the JLinkReproducibleTest.
There are only two modules contained in lib/modules of the image: main and
java.base.
I changed the test specifying --keep-packaged-modules:
diff -r 8c5697ed51b2 test/jdk/tools/jlink/JLinkReproducibleTest.java
--- a/test/jdk/tools/jlink/JLinkReproducibleTest.java Wed Mar 11 08:34:14
2020 +0100
+++ b/test/jdk/tools/jlink/JLinkReproducibleTest.java Thu Mar 12 15:30:45
2020 +0800
@@ -49,6 +49,8 @@
cmd.addAll(List.of(
"--module-path", JMODS_DIR.toString() + File.pathSeparator +
CLASS_DIR.toString(),
"--add-modules", "main",
+"--keep-packaged-modules",
+image.resolve("jmods").toString(),
"--compress=2",
"--output", image.toString()
));
One extra java.base.jmod file will be emitted for each image created:
image-second/jmods/java.base.jmod
image-fourth/jmods/java.base.jmod
image-third/jmods/java.base.jmod
image-first/jmods/java.base.jmod
These jmod files are the same with the one located in the JDK install
directory.
The ModuleHashes attribute is written in java.base.jmod file when building JDK.
So the java.base.jmod will always be the same for each run of the
JLinkReproducibleTest.
Maybe I missed something? Could you please say something more on how extending
the test?
Thanks,
Felix
RE: RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
> -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Wednesday, March 11, 2020 3:03 PM > To: Yangfei (Felix) ; core-libs-dev@openjdk.java.net > Subject: Re: RFR(S): 8240734: ModuleHashes attribute not reproducible > between builds > > On 11/03/2020 01:11, Yangfei (Felix) wrote: > > Thanks for reviewing this. Pushed. > Okay but I think we need to add a test for this. Can you look at > JLinkReproducibleTest and see if it can be extended to use > --keep-packaged-modules so that you have the JMOD files in the run-time > image. Sure. We will take a look. Thanks, Felix
RE: RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
> -Original Message- > From: Alan Bateman [mailto:alan.bate...@oracle.com] > Sent: Tuesday, March 10, 2020 3:53 PM > To: Yangfei (Felix) ; core-libs-dev@openjdk.java.net > Subject: Re: RFR(S): 8240734: ModuleHashes attribute not reproducible > between builds > > On 10/03/2020 02:54, Yangfei (Felix) wrote: > > Hi, > > > >We found module-info.class in java.base.jmod is not always consistent > across different builds. > >The ModuleHashes attribute in this module-info.class is not reproducible > between builds. > >Patch fixes the issue by using TreeMap instead of HashMap when > computing ModuleHashes. > > > >Bug: https://bugs.openjdk.java.net/browse/JDK-8240734 > >Webrev: http://cr.openjdk.java.net/~fyang/8240734/webrev.00/ > > > This looks okay (and to Bernd question, this is used at link time). I think > we need > to extend the test coverage for reproducible builds, the existing > JLinkReproducibleTest.java would ideally have caught the difference due to the > random ordering of hashes. Thanks for reviewing this. Pushed. We see two other issues which affect building reproducible jdk are resolved in 12: 1. 8214230: Classes generated by SystemModulesPlugin.java are not reproducible. 2. 8211057: Gensrc step CompileProperties generates unstable CompilerProperties output. I am inclined to backport these three to 11u so that we can get consistent 11u builds for verification. What do you think? Best regards, Felix
RFR(S): 8240734: ModuleHashes attribute not reproducible between builds
Hi, We found module-info.class in java.base.jmod is not always consistent across different builds. The ModuleHashes attribute in this module-info.class is not reproducible between builds. Patch fixes the issue by using TreeMap instead of HashMap when computing ModuleHashes. Bug: https://bugs.openjdk.java.net/browse/JDK-8240734 Webrev: http://cr.openjdk.java.net/~fyang/8240734/webrev.00/ Passed tier1 test. Please review. Thanks, Felix
