Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 22:52:14 GMT, Anthony Scarpino wrote: >> Yes, I know. Basically, we are trying to optimize performance by trying to >> write into the supplied buffers (out) as much as we can. But then when tag >> verification failed, the "written" bytes are erased w/ 0. Ideal case would >> be not to touch the output buffer until after the tag verification succeeds. >> Isn't this the previous approach? Verify the tag first and then write out >> the plain text afterwards. > > With this new intrinsic doing both ghash and gctr at the same time, I cannot > do the that ghash check first before the gctr op. I wish I could Oh-well, ok. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 30 Jul 2021 18:40:14 GMT, Smita Kamath wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 717: >> >>> 715: in = new byte[Math.min(PARALLEL_LEN, srcLen)]; >>> 716: out = new byte[Math.min(PARALLEL_LEN, srcLen)]; >>> 717: } >> >> Move this down to else-block below just like the 'ct' variable. > > I've kept this code as is and not moved as recommended. If we move this line > to the else part, the case where srcLen is less than PARALLEL_LEN but greater > than BlockSize, in[] is null. As a result, three tests in > test/jdk/../Cipher/AEAD were failing on src.get(in, 0, rlen) line. Do let me > know if that's okay. Thanks. Hmm, I see. Sure, fine to keep it as is then. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 17:57:13 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 761: >> >>> 759: } >>> 760: >>> 761: dst.put(out, 0, rlen); >> >> This looks belong to the above if-block? I wonder how this have not affected >> the operation to fail. Perhaps the existing regression tests did not cover >> the 'rlen < blockSize' case. If the code in the above if-block is not run, >> this outsize dst.put(...) call would put extra output bytes into the output >> buffer. > > Yes... this one and the ct offset problem earlier I would have expected the > regression test it pick the mistake. There should be tests that catch this.. > I'm not sure what's up. This shall be addressed in next update I assume? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 30 Jul 2021 18:23:18 GMT, Valerie Peng wrote: >> Ok.. Moving it into GCMEncrypt makes sense. Now that I look at the code >> GCMDecrypt only uses it when passed to a method. GCMEncrypt uses it > > This is still present in the latest update. Is there another update coming? Yes. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Mon, 19 Jul 2021 19:18:54 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 717: > >> 715: in = new byte[Math.min(PARALLEL_LEN, srcLen)]; >> 716: out = new byte[Math.min(PARALLEL_LEN, srcLen)]; >> 717: } > > Move this down to else-block below just like the 'ct' variable. I've kept this code as is and not moved as recommended. If we move this line to the else part, the case where srcLen is less than PARALLEL_LEN but greater than BlockSize, in[] is null. As a result, three tests in test/jdk/../Cipher/AEAD were failing on src.get(in, 0, rlen) line. Do let me know if that's okay. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 30 Jul 2021 18:23:44 GMT, Valerie Peng wrote: >> ok > > This is still present in the latest update. Is there another update coming? Yes. There will be another update. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 17:16:45 GMT, Anthony Scarpino wrote: >> Seems strange to have GCMOperation op defined in GCMEngine but not >> initialized, nor used. The methods in GCMEngine which use op has an argument >> named op anyway. Either you just use the "op" field (remove the "op" >> argument) or the "op" argument (move the op field to GCMEncrypt/GCMDecrypt >> class). Having both looks confusing. > > Ok.. Moving it into GCMEncrypt makes sense. Now that I look at the code > GCMDecrypt only uses it when passed to a method. GCMEncrypt uses it This is still present in the latest update. Is there another update coming? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 17:19:20 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 650: >> >>> 648: int originalOutOfs = 0; >>> 649: byte[] in; >>> 650: byte[] out; >> >> The name "in", "out" are almost used in all calls, it's hard to tell when >> these two are actually used. Can we rename them to make them more unique? > > ok This is still present in the latest update. Is there another update coming? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 18:30:50 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 762: >> >>> 760: >>> 761: dst.put(out, 0, rlen); >>> 762: processed += srcLen; >> >> It seems that callers of this implGCMCrypt() method such as >> GCMEngine.doLastBlock() adds the returned value to the "processed" field >> which looks like double counting? However, some caller such as >> GCMEncrypt.doUpdate() does not. Seems inconsistent and may lead to wrong >> value for the "processed" field? > > All the callers that use GCMOperations, ie op.update(...), have the processed > value updated. implGCMCrypt() calls op.update() and updates the value. It > cannot double count 'processed' is not updated after implGCMCrypt(). I can > see your point, but the other methods do not have access to 'processed' and > would mean I copy that line 3 times elsewhere. I'd rather keep it as is As long the "processed" value is correct, it's fine. Not sure if I may be missing some subtle things, GCMEngine.implGCMCrypt(GCMOperation op, ByteBuffer src, ByteBuffer dst) impl would increment "processed" with "srcLen". But then in GCMEngine.doLastBlock(GCMOperation op, ByteBuffer buffer, ByteBuffer src, ByteBuffer dst) impl, it calls the GCMEngine.implGCMCrypt(GCMOperation op, ByteBuffer src, ByteBuffer dst) method and stores the return value into "resultLen" and then again increment "processed" with "resultLen" after op.doFinal(...) call. Since "resultLen" contains the number of bytes processed by GCMEngine.implGCMCrypt(GCMOperation op, ByteBuffer src, ByteBuffer dst) method already, adding it to "prcessed" looks like double counting. Not sure what did I miss. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 22:41:03 GMT, Valerie Peng wrote: >> This is able in-place, not about two separate buffers.. zeroing happens >> somewhere else for all decryption bad buffers > > Yes, I know. Basically, we are trying to optimize performance by trying to > write into the supplied buffers (out) as much as we can. But then when tag > verification failed, the "written" bytes are erased w/ 0. Ideal case would be > not to touch the output buffer until after the tag verification succeeds. > Isn't this the previous approach? Verify the tag first and then write out the > plain text afterwards. With this new intrinsic doing both ghash and gctr at the same time, I cannot do the that ghash check first before the gctr op. I wish I could - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 22 Jul 2021 18:36:16 GMT, Anthony Scarpino wrote: >> Hmm ok, so if it's not decryption in-place, then output buffer would still >> be zero'ed when the auth tag failed, but this is ok? > > This is able in-place, not about two separate buffers.. zeroing happens > somewhere else for all decryption bad buffers Yes, I know. Basically, we are trying to optimize performance by trying to write into the supplied buffers (out) as much as we can. But then when tag verification failed, the "written" bytes are erased w/ 0. Ideal case would be not to touch the output buffer until after the tag verification succeeds. Isn't this the previous approach? Verify the tag first and then write out the plain text afterwards. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 00:09:37 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 611: > >> 609: outOfs + len); >> 610: ghash.update(ct, ctOfs, segments); >> 611: ctOfs = len; > > This does not look right when the initial value of ctOfs != 0. Yeah that doesn't look right - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Mon, 19 Jul 2021 23:41:49 GMT, Valerie Peng wrote: >> If decryption fails with a bad auth tag, the in is not overwritten because >> it's in-place. Encryption is not needed because there is nothing to check. >> I can add a comment. > > Hmm ok, so if it's not decryption in-place, then output buffer would still be > zero'ed when the auth tag failed, but this is ok? This is able in-place, not about two separate buffers.. zeroing happens somewhere else for all decryption bad buffers - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Tue, 20 Jul 2021 01:35:04 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 762: > >> 760: >> 761: dst.put(out, 0, rlen); >> 762: processed += srcLen; > > It seems that callers of this implGCMCrypt() method such as > GCMEngine.doLastBlock() adds the returned value to the "processed" field > which looks like double counting? However, some caller such as > GCMEncrypt.doUpdate() does not. Seems inconsistent and may lead to wrong > value for the "processed" field? All the callers that use GCMOperations, ie op.update(...), have the processed value updated. implGCMCrypt() calls op.update() and updates the value. It cannot double count 'processed' is not updated after implGCMCrypt(). I can see your point, but the other methods do not have access to 'processed' and would mean I copy that line 3 times elsewhere. I'd rather keep it as is - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Mon, 19 Jul 2021 19:35:16 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 761: > >> 759: } >> 760: >> 761: dst.put(out, 0, rlen); > > This looks belong to the above if-block? I wonder how this have not affected > the operation to fail. Perhaps the existing regression tests did not cover > the 'rlen < blockSize' case. If the code in the above if-block is not run, > this outsize dst.put(...) call would put extra output bytes into the output > buffer. Yes... this one and the ct offset problem earlier I would have expected the regression test it pick the mistake. There should be tests that catch this.. I'm not sure what's up. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Mon, 19 Jul 2021 19:22:53 GMT, Valerie Peng wrote:
>> Smita Kamath has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 729:
>
>> 727:
>> 728: if (src.hasArray() && dst.hasArray()) {
>> 729: int l = rlen;
>
> Remove this "l" variable since it's not used?
The code needs some reorganizing
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 00:31:43 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 650: > >> 648: int originalOutOfs = 0; >> 649: byte[] in; >> 650: byte[] out; > > The name "in", "out" are almost used in all calls, it's hard to tell when > these two are actually used. Can we rename them to make them more unique? ok - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Tue, 20 Jul 2021 22:36:28 GMT, Valerie Peng wrote: >> Initializing op in abstract GCMEngine would mean another 'if(encryption)', >> when that would not be needed in the GCMEncrypt() or GCMDecrypt(). I don't >> see why that is clearer. >> >> GaloisCounterMode.implGCMCrypt(...) is the intrinsic, so I have to use what >> is used by hotspot. > > Seems strange to have GCMOperation op defined in GCMEngine but not > initialized, nor used. The methods in GCMEngine which use op has an argument > named op anyway. Either you just use the "op" field (remove the "op" > argument) or the "op" argument (move the op field to GCMEncrypt/GCMDecrypt > class). Having both looks confusing. Ok.. Moving it into GCMEncrypt makes sense. Now that I look at the code GCMDecrypt only uses it when passed to a method. GCMEncrypt uses it - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 19:41:53 GMT, Valerie Peng wrote: >> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java >> line 629: >> >>> 627: GCTR gctr; >>> 628: GHASH ghash; >>> 629: GCMOperation op; >> >> It seems clearer to initialize "op" in GCMEngine ctor since it's declared >> here. There is already logic in its method checking whether we are doing >> encryption or decryption. > > Now that you have GCMOperation op, but there is still if-else blocks checking > whether it's encryption/decryption and uses gctr and ghash instead of op. > Looks like a bit adhoc? Can GaloisCounterMode.implGCMCrypt(...) just take > GCMOperation op instead, no need for ct, ctOfs, gctr and ghash? Initializing op in abstract GCMEngine would mean another 'if(encryption)', when that would not be needed in the GCMEncrypt() or GCMDecrypt(). I don't see why that is clearer. GaloisCounterMode.implGCMCrypt(...) is the intrinsic, so I have to use what is used by hotspot. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 20:49:20 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 146: > >> 144: } >> 145: state = new long[2]; >> 146: subkeyHtbl = new long[2*57]; // 48 keys for the interleaved >> implementation, 8 for avx-ghash implementation and one for the original key > > nit: the comment is too long, i.e. > 80 Ah.. I forgot I didn't change GHASH with my changes.. but I'll fix that thanks > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 743: > >> 741: dst.array(), dst.arrayOffset() + >> dst.position(), >> 742: gctr, ghash); >> 743: } > > Can we use another ByteBuffer variable to avoid almost-duplicate calls? > > ByteBuffer ct = (encryption? dst : src); > rlen -= GaloisCounterMode.implGCMCrypt(src.array(), > src.arrayOffset() + src.position(), > src.remaining(), > ct.array(), ct.arrayOffset() + ct.position(), > dst.array(), dst.arrayOffset() + dst.position(), > gctr, ghash); That maybe a better choice - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 00:10:52 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 992:
>>
>>> 990: */
>>> 991: byte[] overlapDetection(byte[] in, int inOfs, byte[] out, int
>>> outOfs) {
>>> 992: if (in == out && (!encryption || inOfs < outOfs)) {
>>
>> So, we will always allocate an output buffer for decryption if in==out? Why
>> just decryption? Update the javadoc for this method with the reason?
>
> If the crypto is decryption in-place, an internal output buffer is needed in
> case the auth tag fails, otherwise the input buffer would be zero'ed.
If decryption fails with a bad auth tag, the in is not overwritten because it's
in-place. Encryption is not needed because there is nothing to check. I can
add a comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated AES-GCM intrinsic to match latest Java Code src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 146: > 144: } > 145: state = new long[2]; > 146: subkeyHtbl = new long[2*57]; // 48 keys for the interleaved > implementation, 8 for avx-ghash implementation and one for the original key nit: the comment is too long, i.e. > 80 - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated AES-GCM intrinsic to match latest Java Code
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
629:
> 627: GCTR gctr;
> 628: GHASH ghash;
> 629: GCMOperation op;
It seems clearer to initialize "op" in GCMEngine ctor since it's declared here.
There is already logic in its method checking whether we are doing encryption
or decryption.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
650:
> 648: int originalOutOfs = 0;
> 649: byte[] in;
> 650: byte[] out;
The name "in", "out" are almost used in all calls, it's hard to tell when these
two are actually used. Can we rename them to make them more unique?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
724:
> 722: } else {
> 723: ct = in;
> 724: }
This can just be:
byte[] ct = (encryption? out : in);
Since you only use this 'ct' variable inside the else block on line 746, move
this down to that block.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
743:
> 741: dst.array(), dst.arrayOffset() +
> dst.position(),
> 742: gctr, ghash);
> 743: }
Can we use another ByteBuffer variable to avoid almost-duplicate calls?
ByteBuffer ct = (encryption? dst : src);
rlen -= GaloisCounterMode.implGCMCrypt(src.array(),
src.arrayOffset() + src.position(), src.remaining(),
ct.array(), ct.arrayOffset() + ct.position(),
dst.array(), dst.arrayOffset() + dst.position(),
gctr, ghash);
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Fri, 16 Jul 2021 00:32:16 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated AES-GCM intrinsic to match latest Java Code > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 629: > >> 627: GCTR gctr; >> 628: GHASH ghash; >> 629: GCMOperation op; > > It seems clearer to initialize "op" in GCMEngine ctor since it's declared > here. There is already logic in its method checking whether we are doing > encryption or decryption. Now that you have GCMOperation op, but there is still if-else blocks checking whether it's encryption/decryption and uses gctr and ghash instead of op. Looks like a bit adhoc? Can GaloisCounterMode.implGCMCrypt(...) just take GCMOperation op instead, no need for ct, ctOfs, gctr and ghash? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated AES-GCM intrinsic to match latest Java Code src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 611: > 609: outOfs + len); > 610: ghash.update(ct, ctOfs, segments); > 611: ctOfs = len; This does not look right when the initial value of ctOfs != 0. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Thu, 15 Jul 2021 22:44:05 GMT, Valerie Peng wrote:
>> Smita Kamath has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 170:
>
>> 168:
>> 169: // always encrypt mode for embedded cipher
>> 170: blockCipher.init(false, key.getAlgorithm(), keyValue);
>
> Is this change intentional? Looks like we are reverting to older version of
> source and undo newer changes.
Nope.. unintentional
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 472:
>
>> 470: engine = null;
>> 471: if (encodedKey != null) {
>> 472: Arrays.fill(encodedKey, (byte)0);
>
> Looks like another unintentional newer->older change.
I don't remember an old comment about that, dunno if that was reverted
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 992:
>
>> 990: */
>> 991: byte[] overlapDetection(byte[] in, int inOfs, byte[] out, int
>> outOfs) {
>> 992: if (in == out && (!encryption || inOfs < outOfs)) {
>
> So, we will always allocate an output buffer for decryption if in==out? Why
> just decryption? Update the javadoc for this method with the reason?
If the crypto is decryption in-place, an internal output buffer is needed in
case the auth tag fails, otherwise the input buffer would be zero'ed.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated AES-GCM intrinsic to match latest Java Code src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 589: > 587: * Requires 768 bytes (48 AES blocks) to efficiently use the > intrinsic > 588: * @param in input buffer > 589: * @param inOfs input offset missed @param inLen src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 594: > 592: * @param out output buffer > 593: * @param outOfs output offset > 594: * @param gctr object for the CTR operation typo: CTR->GCTR? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated AES-GCM intrinsic to match latest Java Code
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
992:
> 990: */
> 991: byte[] overlapDetection(byte[] in, int inOfs, byte[] out, int
> outOfs) {
> 992: if (in == out && (!encryption || inOfs < outOfs)) {
So, we will always allocate an output buffer for decryption if in==out? Why
just decryption? Update the javadoc for this method with the reason?
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated AES-GCM intrinsic to match latest Java Code src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 170: > 168: > 169: // always encrypt mode for embedded cipher > 170: blockCipher.init(false, key.getAlgorithm(), keyValue); Is this change intentional? Looks like we are reverting to older version of source and undo newer changes. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated AES-GCM intrinsic to match latest Java Code
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
472:
> 470: engine = null;
> 471: if (encodedKey != null) {
> 472: Arrays.fill(encodedKey, (byte)0);
Looks like another unintentional newer->older change.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated AES-GCM intrinsic to match latest Java Code
First, you need review from Tony for Java side changes.
Second, you need to extend tests in `test/hotspot/jtreg/compiler/codegen/aes/`
to cover this implementation.
And, third, I think we need to put this on hold until the issue of big
intrinsics stubs generation effect on startup is solved. See discussion in
https://bugs.openjdk.java.net/browse/JDK-8270323
- code_size1 = 2 LP64_ONLY(+1), // simply increase if too
small (assembler will crash if too small)
- code_size2 = 35300 LP64_ONLY(+25000) // simply increase if too
small (assembler will crash if too small)
+ code_size1 = 2 LP64_ONLY(+12000), // simply increase if too
small (assembler will crash if too small)
+ code_size2 = 35300 LP64_ONLY(+37000) // simply increase if too
small (assembler will crash if too small)
@sviswa7 please, note these changes too for our discussion.
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 7644:
> 7642: }
> 7643: if (UseAESIntrinsics) {
> 7644: if (VM_Version::supports_avx512_vaes() &&
> VM_Version::supports_avx512vl() && VM_Version::supports_avx512dq()) {
Why duplicate already existing checks? Move code there and add comment for
which intrinsic code is generated.
src/hotspot/cpu/x86/stubRoutines_x86.hpp line 36:
> 34: enum platform_dependent_constants {
> 35: code_size1 = 2 LP64_ONLY(+12000), // simply increase if too
> small (assembler will crash if too small)
> 36: code_size2 = 35300 LP64_ONLY(+37000) // simply increase if too
> small (assembler will crash if too small)
This is almost 50% increase !!!
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 333:
> 331: static_field(StubRoutines,_bigIntegerRightShiftWorker,
> address) \
> 332: static_field(StubRoutines,_bigIntegerLeftShiftWorker,
> address) \
> 333: static_field(StubRoutines,_galoisCounterMode_AESCrypt,
> address) \
Move up to other AESCrypt lines.
src/hotspot/share/opto/escape.cpp line :
> 1109: strcmp(call->as_CallLeaf()->_name,
> "bigIntegerLeftShiftWorker") == 0 ||
> 1110: strcmp(call->as_CallLeaf()->_name,
> "vectorizedMismatch") == 0 ||
> : strcmp(call->as_CallLeaf()->_name,
> "galoisCounterMode_AESCrypt") == 0 ||
Please, move new line where other AEScrypt methods listed.
src/hotspot/share/runtime/stubRoutines.cpp line 130:
> 128: address StubRoutines::_base64_encodeBlock = NULL;
> 129: address StubRoutines::_base64_decodeBlock = NULL;
> 130: address StubRoutines::_galoisCounterMode_AESCrypt = NULL;
Move up few lines
src/hotspot/share/runtime/stubRoutines.hpp line 212:
> 210: static address _base64_encodeBlock;
> 211: static address _base64_decodeBlock;
> 212: static address _galoisCounterMode_AESCrypt;
Move up few lines
src/hotspot/share/runtime/vmStructs.cpp line 592:
> 590: static_field(StubRoutines,_unsafe_arraycopy,
> address) \
> 591: static_field(StubRoutines,_generic_arraycopy,
> address) \
> 592: static_field(StubRoutines,
> _galoisCounterMode_AESCrypt, address)
> \
Move up to other AESCrypt declarations.
-
Changes requested by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
On Wed, 14 Jul 2021 21:02:01 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated AES-GCM intrinsic to match latest Java Code Looks like you have some issues: wrong file property. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]
> I would like to submit AES-GCM optimization for x86_64 architectures > supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES > and GHASH operations. > Performance gain of ~1.5x - 2x for message sizes 8k and above. Smita Kamath has updated the pull request incrementally with one additional commit since the last revision: Updated AES-GCM intrinsic to match latest Java Code - Changes: - all: https://git.openjdk.java.net/jdk/pull/4019/files - new: https://git.openjdk.java.net/jdk/pull/4019/files/4b6e881e..4a36816f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4019=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4019=02-03 Stats: 469 lines in 11 files changed: 226 ins; 128 del; 115 mod Patch: https://git.openjdk.java.net/jdk/pull/4019.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4019/head:pull/4019 PR: https://git.openjdk.java.net/jdk/pull/4019
