Repository navigation
Allow disabling ML-DSA AVX2/512 specifically with WC_MLDSA_NO_AVX2. Disable AVX512 AES code with NO_VAES_SUPPORT, as it is also VAES. - #11931
Conversation
Disable AVX512 AES code with NO_VAES_SUPPORT, as it is also VAES.
|
@claude review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the ML-DSA side. With WC_MLDSA_NO_AVX2 set, every AVX512 dispatch in wc_mldsa.c sits under WOLFSSL_MLDSA_HAVE_INTEL_AVX512, so it compiles out and nothing reaches it at runtime through IS_INTEL_AVX512. The IS_INTEL_AVX2 and SHA3_USE_AVX2 overrides come after their definitions in cpuid.h and sha3.h and before their first use. The header gate in wc_mldsa.h only removes function prototypes and doesn't change any struct layout.
Extended reasoning...
One finding was already reported, about the AES asm guards. This note covers the ML-DSA half of the change, which was checked separately and showed no issue: the AVX512 code is fully compiled out, the macro overrides are in the right place, and the header gate changes no struct layout.
| /* The AVX512 AES kernels are VAES code. */ | ||
| #if !defined(NO_VAES_SUPPORT) && !defined(NO_AVX512_SUPPORT) && \ | ||
| !defined(HAVE_INTEL_AVX512) | ||
| #define HAVE_INTEL_AVX512 | ||
| #endif |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing, partial fix: builds with NO_VAES_SUPPORT still assemble the AVX512 VAES AES kernels. Only aes.c and the linuxkm glue were updated. aes_gcm_asm.S:40-44 and aes_xts_asm.S:40-44 still define HAVE_INTEL_AVX512 whenever NO_AVX512_SUPPORT is unset. Fix: in every generated AES asm file, define HAVE_INTEL_AVX512 only when NO_VAES_SUPPORT is also unset, as aes.c now does. That covers the .S files plus their .asm and x86 variants, and is best done in the asm generator so it survives regeneration. [also at: wolfcrypt/src/aes.c:974 - Builds with NO_VAES_SUPPORT still assemble the VAES AVX512 XTS kernels, so a toolchain without VAES support still fails.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user defines NO_VAES_SUPPORT but leaves NO_AVX512_SUPPORT unset, typically because their assembler or target cannot handle VAES. aes.c:971-975 now leaves HAVE_INTEL_AVX512 undefined, so the C code no longer calls the AVX512 kernels. aes_gcm_asm.S:40-44 and aes_xts_asm.S:40-44 still define HAVE_INTEL_AVX512 independently of NO_VAES_SUPPORT. The AVX512 VAES blocks at aes_gcm_asm.S:26236-37242 and aes_xts_asm.S:4993-7233 are therefore still assembled. An assembler without VAES support still fails exactly as on the base branch. With a capable assembler, the binary ships about 13k lines of unreachable VAES code. The PR title promises to disable the AVX512 AES code with NO_VAES_SUPPORT, but that only holds on the C side.
Verification: Trigger: a build that defines NO_VAES_SUPPORT, leaves NO_AVX512_SUPPORT unset, and uses an assembler that does not know VAES. aes_gcm_asm.S:40-44 and aes_xts_asm.S:40-44 still have their own #ifndef NO_AVX512_SUPPORT / #define HAVE_INTEL_AVX512, with no check of NO_VAES_SUPPORT. So the VAES AVX512 blocks are still assembled. The base branch fails the same way by the same route.
Description
Fixes zd#22620
Testing
Built in tests
Checklist