Repository navigation
Fix randomx_get_flags not reporting hardware AES on RISC-V - #370
thomasbuilds wants to merge 1 commit into
Conversation
hyc
left a comment
hyc
left a comment
There was a problem hiding this comment.
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.
|
Yes, HAVE_AES should be 1 on RISC-V because there are AES instruction extensions there. |
614502d to
6788dd3
Compare
| #else | ||
| constexpr bool hardAesCheckedAtRuntime = !HAVE_AES; | ||
| #endif | ||
| runTest("Hardware AES not supported", hardAesCheckedAtRuntime && !randomx::cpu.hasAes(), []() { |
There was a problem hiding this comment.
This change seems completely unnecessary. Why single out RISC-V?
There was a problem hiding this comment.
With HAVE_AES now 1 on RISC-V, the old condition would skip this test on every RISC-V build, including the riscv64 CI job, where it runs today (QEMU's CPU has VLEN 128 so cpu.hasAes() is false). RISC-V is singled out because it's the only platform where allocate() checks cpu.hasAes(). Elsewhere, with HAVE_AES 1, allocate() executes an AES instruction so with a plain !cpu.hasAes() the test would fail in the ppc64le and Win32 CI jobs, where cpu.hasAes() is never set but the CPU has AES.
| #ifdef __riscv | ||
| if (!softAes && !cpu.hasAes()) | ||
| throw std::runtime_error(platformError); | ||
| throw std::runtime_error("Platform doesn't support hardware AES"); |
There was a problem hiding this comment.
Completely unrelated and unnecessary change.
There was a problem hiding this comment.
platformError is only declared inside the #ifndef HAVE_AES block of intrin_portable.h so with HAVE_AES now defined on RISC-V the original line fails to compile ('platformError' was not declared in this scope). The message text is unchanged.
| freg[0] = aesenc<softAes>(freg[0], ekey[i]); | ||
| freg[1] = aesdec<softAes>(freg[1], ekey[i]); | ||
| freg[2] = aesenc<softAes>(freg[2], ekey[i]); | ||
| freg[3] = aesdec<softAes>(freg[3], ekey[i]); |
There was a problem hiding this comment.
Why remove it? If HAVE_AES is 0 and softAes is false, this will compile to a code that will throw an exception in intrin_portable.h
b63be62 to
d21bbd8
Compare
randomx_get_flags()reportsRANDOMX_FLAG_HARD_AESonly ifHAVE_AESis 1, which it never was on RISC-V:rx_aesenc_vec_i128/rx_aesdec_vec_i128had no RISC-V implementation. So callers that take their flags from it, such as Monero andrandomx-benchmark --auto, used software AES even where hardware AES is detected.This implements both functions with Zvkned in
aes_hash_rv64_zvkned.cpp, the file already compiled with Zvkned, and setsHAVE_AESto 1 on RISC-V. They are only called fromRANDOMX_FLAG_HARD_AESVMs, whichVmBase::allocate()rejects unlesscpu.hasAes(). In those VMs, the interpreter's v2 AES mix now uses them instead of software AES.platformErroronly exists whenHAVE_AESis 0, soallocate()now uses the string directly. The "Hardware AES not supported" test keeps running on RISC-V.