RFR: 8365732: RISC-V: implement AES CTR intrinsics [v26]

[Anjian Wen]

On Tue, 18 Nov 2025 15:10:49 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Anjian Wen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   modify stub_id name
>
> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2651:
> 
>> 2649:   address generate_counterMode_AESCrypt() {
>> 2650:     assert(UseAESCTRIntrinsics, "need AES instructions (Zvkned extension) support");
>> 2651:     assert(UseZbb, "need basic bit manipulation (Zbb extension) support");
> 
> also needs an `assert(UseZvkn, "");`?

fixed!

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2663:
> 
>> 2661:     const Register input_len           = c_rarg4;
>> 2662:     const Register saved_encrypted_ctr = c_rarg5;
>> 2663:     const Register used_ptr            = c_rarg6;
> 
> Suggestion:
> 
>     const Register used_len_addr            = c_rarg6;

done

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2670:
> 
>> 2668:     __ enter();
>> 2669: 
>> 2670:     Label L_EXIT;
> 
> Suggestion:
> 
>     Label L_exit;

I try to make it different from the L_exit in counterMode_AESCrypt function, should I change this to L_exit2 or L_exit_main?

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2676:
> 
>> 2674:     // Compute #rounds for AES based on the length of the key array
>> 2675:     __ lwu(keylen, Address(key, arrayOopDesc::length_offset_in_bytes() - arrayOopDesc::base_offset_in_bytes(T_INT)));
>> 2676:     __ mv(t0, 52);
> 
> what's this `52`? I see it also in `generate_aescrypt_encryptBlock`, do they mean similar things?
> Can you add some comment about it? and give a name rather than use the magic number.

key length could be only {11, 13, 15} * 4 = {44, 52, 60}，I notice that x86 and aarch64 use directly 52，I think add some more comment will be enough？

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2700:
> 
>> 2698:   }
>> 2699: 
>> 2700:   void counterMode_AESCrypt(int round, Register in, Register out, Register key, Register counter,
> 
> Maybe move this `counterMode_AESCrypt` above `generate_counterMode_AESCrypt`?

good point, fixed it!

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2708:
> 
>> 2706:     //   L_encrypt_next:
>> 2707:     //     while (used < BLOCK_SIZE) {
>> 2708:     //       if (len == 0) goto L_exit;
> 
> The logic of the code here is different from the logic of assembly code.
> Here it checks `len == 0` at the beginning of while loop; assembly code checks `len == 0` at the end of while loop.
> Will this difference bring some logic difference in some corner case? If not, why make it a bit different from each other? Does it bring some performance difference with following change?
> 
>     Label L_next, L_main_loop, L_exit; // remove L_encrypt_next
>     ...
>     __ bind(L_next);
>     __ bgeu(used, block_size, L_main_loop);
>     __ beqz(len, L_exit);
>     ... // scalar processing
>     __ subi(len, len, 1);
>     __ j(L_next);
>     ...
>     __ mv(used, 0);
>     // Check if we have a full block_size
>     __ bltu(len, block_size, L_next); // remove L_encrypt_next
>     ...
> 
> 
> Ask this question because the change make the comment and assembly implementation consistent, and the code easy to understand.

Oh sure, There is indeed a bit of Pseudocode logic here that is consistent with the previous version, and I'll make some modifications.

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2709:
> 
>> 2707:     //     while (used < BLOCK_SIZE) {
>> 2708:     //       if (len == 0) goto L_exit;
>> 2709:     //       out = in ^ saved_encrypted_ctr[used]);
> 
> do you mean `*out = *in ^ saved_encrypted_ctr[used]);`?

yes, fixed it

> src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 2813:
> 
>> 2811:     __ bind(L_exit);
>> 2812:     __ sw(used, Address(used_ptr));
>> 2813:     __ mv(x10, input_len);
> 
> is this mv necessary?

it's a return value saved to x10, it seems necessary according to aarch64 and x86, aarch64 used r0 to save it and x86 used rax

> src/hotspot/cpu/riscv/vm_version_riscv.cpp line 447:
> 
>> 445:         FLAG_SET_DEFAULT(UseAESCTRIntrinsics, false);
>> 446:       }
>> 447:     }
> 
> Suggestion:
> 
>     if (FLAG_IS_DEFAULT(UseAESCTRIntrinsics) && UseZbb) {
>       FLAG_SET_DEFAULT(UseAESCTRIntrinsics, true);
>     }
> 
>     if (UseAESCTRIntrinsics && !UseZbb) {
>         warning("Cannot enable UseAESCTRIntrinsics on cpu without UseZbb support.");
>         FLAG_SET_DEFAULT(UseAESCTRIntrinsics, false);
>     }

Thanks, fixed!

> src/hotspot/cpu/riscv/vm_version_riscv.cpp line 458:
> 
>> 456:     }
>> 457:     if (UseAESCTRIntrinsics) {
>> 458:       warning("AES/CTR intrinsics are not available on this CPU");
> 
> Suggestion:
> 
>       warning("Cannot enable UseAESCTRIntrinsics on cpu without UseZvkn support.");

Thanks, fixed it

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540834580
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540838391
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540836978
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540838774
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540835345
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540839090
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540845734
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540844372
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540834756
PR Review Comment: https://git.openjdk.org/jdk/pull/25281#discussion_r2540835074