RFR: 8278757: [s390] Implement AES Counter Mode Intrinsic [v5]

[Martin Doerr]

On Wed, 20 Apr 2022 14:53:03 GMT, Lutz Schmidt <lucy at openjdk.org> wrote:

>> Please review (and approve, if possible) this pull request.
>> 
>> This is a s390-only enhancement. It introduces the implementation of an AES-CTR intrinsic, making use of the specific s390 instruction for AES counter-mode encryption.
>> 
>> Testing: SAP does no longer maintain a full build and test environment for s390. Testing is therefore limited to running some test suites (SPECjbb*, SPECjvm*) manually. But: identical code is contained in SAP's commercial product and thoroughly tested in that context. No issues were uncovered.
>> 
>> @backwaterred Could you please conduct some "official" testing for this PR? 
>> 
>> Thank you all!
>> 
>> Note: some performance figures can be found in the JBS ticket.
>
> Lutz Schmidt has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into JDK-8278757
>  - Merge branch 'master' into JDK-8278757
>  - 8278757: resolve merge conflict
>  - 8278757: update copyright year
>  - 8278757: [s390] Implement AES Counter Mode Intrinsic

Looks basically correct, but I still need to check a few things. I already have some change requests.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1755:

> 1753:       __ should_not_reach_here();
> 1754:     }
> 1755: 

Please assert that VM_Version::has_Crypto_AES() instead. Otherwise, the stub shouldn't get generated at all (handled by vm_version_s390.cpp).

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1826:

> 1824:       __ should_not_reach_here();
> 1825:     }
> 1826: 

Like above.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1902:

> 1900:   //   +--------+ <-- Z_SP + alignment loss (part 1+2), octoword-aligned
> 1901:   //   |        |
> 1902:   //   :        :  additional alignment loss. Blocks above can't tolerate unusabe DW @SP.

"unusabe"?

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1919:

> 1917:   //                parmBlk-4: ctrVal_len (as retrieved from iv array), in bytes, as HW
> 1918:   //                parmBlk-8: msglen length (in bytes) of crypto msg, as passed in by caller
> 1919:   //                              return value is calculated from this: rv = msglen - processed.

Strange indentation.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1965:

> 1963:     // check length against expected.
> 1964:     __ z_chi(scratch, AES_ctrVal_len);
> 1965:     __ asm_assert_eq("counter value needs same size as data block", 0xb00b);

Why do we need to copy it to memory if we just want to compare it? Is this debugging code worth keeping it this way?

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1984:

> 1982:       int offset = j * AES_ctrVal_len;
> 1983:       __ z_algsi(offset + 8, counter, j);                        // increment iv by index value
> 1984:                                                                  // TODO: for correctness, use 128-bit add

Does this TODO need to get resolved?

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 1999:

> 1997:       int offset = j * AES_ctrVal_len;
> 1998:       __ z_algsi(offset + 8, counter, AES_ctrVec_len);           // calculate new ctr vector elements (simple increment)
> 1999:                                                                  // TODO: for correctness, use 128-bit add

TODO as above.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 2012:

> 2010:     BLOCK_COMMENT(err_msg("push_Block counterMode_AESCrypt%d {", parmBlk_len*8));
> 2011: 
> 2012:     AES_dataBlk_space    = (2*dataBlk_len + AES_parmBlk_align - 1) & (~(AES_parmBlk_align - 1)); // space for data blocks (src and dst, one each) for partial block processing)

Line too long!

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 2199:

> 2197:     if (! VM_Version::has_Crypto_AES_CTR()) {
> 2198:       __ should_not_reach_here();
> 2199:     }

Like above.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 2387:

> 2385:     if (! VM_Version::has_Crypto_AES_CTR()) {
> 2386:       __ should_not_reach_here();
> 2387:     }

Like above.

-------------

Changes requested by mdoerr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8142