Skip to content

Fix randomx_get_flags not reporting hardware AES on RISC-V - #370

Open
thomasbuilds wants to merge 1 commit into
tevador:masterfrom
thomasbuilds:fix-riscv-get-flags-aes
Open

thomasbuilds wants to merge 1 commit into
tevador:masterfrom
thomasbuilds:fix-riscv-get-flags-aes

Conversation

@thomasbuilds

@thomasbuilds thomasbuilds commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

randomx_get_flags() reports RANDOMX_FLAG_HARD_AES only if HAVE_AES is 1, which it never was on RISC-V: rx_aesenc_vec_i128/rx_aesdec_vec_i128 had no RISC-V implementation. So callers that take their flags from it, such as Monero and randomx-benchmark --auto, used software AES even where hardware AES was detected.

This implements both functions with Zvkned in aes_hash_rv64_zvkned.cpp, the file already compiled with Zvkned, and sets HAVE_AES to 1 on RISC-V. They are only called from RANDOMX_FLAG_HARD_AES VMs, which VmBase::allocate() rejects unless cpu.hasAes(). In those VMs, the interpreter's v2 AES mix now uses them instead of software AES. platformError stays defined on RISC-V, where allocate() uses it, and is now a const pointer so GCC's -Wall doesn't flag it as unused. The "Hardware AES not supported" test still runs on RISC-V whenever cpu.hasAes() is false.

@hyc hyc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CPU has hardware AES but HAVE_AES is not being defined, then the right thing to do is to fix that, not muck around here.

@SChernykh

Copy link
Copy Markdown
Collaborator

Yes, HAVE_AES should be 1 on RISC-V because there are AES instruction extensions there.

@thomasbuilds
thomasbuilds force-pushed the fix-riscv-get-flags-aes branch 2 times, most recently from 614502d to 6788dd3 Compare October 31, 2026 09:27
@thomasbuilds
thomasbuilds requested a review from hyc October 31, 2026 11:05
Comment thread src/tests/tests.cpp Resolved
Comment thread src/virtual_machine.cpp Resolved
Comment thread src/vm_interpreted.cpp Resolved
@thomasbuilds
thomasbuilds force-pushed the fix-riscv-get-flags-aes branch 2 times, most recently from b63be62 to d21bbd8 Compare October 31, 2026 14:56
@thomasbuilds
thomasbuilds force-pushed the fix-riscv-get-flags-aes branch 2 times, most recently from 987069b to 0892094 Compare November 1, 2026 10:14
Comment thread src/tests/tests.cpp

runTest("Hardware AES not supported", !HAVE_AES && !randomx::cpu.hasAes(), []() {
#ifdef __riscv
constexpr bool hardAesCheckedAtRuntime = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is unnecessary. hard AES will always be checked at runtime on platforms that have hardware AES support.

The HAVE_AES define means this particular chip family has hardware AES support, but we always have to check at runtime whether the current chip has it or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test covers that runtime check on RISC-V: allocate() rejects RANDOMX_FLAG_HARD_AES there unless cpu.hasAes() (#349). With HAVE_AES now 1 on RISC-V, the old condition would skip it, including in the riscv64 CI job, where it runs today. Other platforms don't check cpu.hasAes() in allocate() so a plain !cpu.hasAes() would fail the Win32, ppc64le and two arm64 CI jobs, where cpu.hasAes() is always false but the CPU has AES. #369 will replace __riscv here with HAVE_AES_CHECKER.

Comment thread src/intrin_portable.h

#ifndef HAVE_AES
static const char* platformError = "Platform doesn't support hardware AES";
#if !defined(HAVE_AES) || defined(__riscv)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, I think this change is unnecessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it RISC-V doesn't build: allocate() throws platformError there (#349) and with HAVE_AES now 1, #ifndef HAVE_AES no longer defines it. SChernykh suggested this guard rather than changing that throw - #369 will replace defined(__riscv) with defined(HAVE_AES_CHECKER).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants