-
Notifications
You must be signed in to change notification settings - Fork 134
[RFC] ML-KEM: Import AArch64 backend (from mlkem-native) #2498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9f583c4
to
2749386
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2498 +/- ##
==========================================
- Coverage 78.86% 78.85% -0.01%
==========================================
Files 640 640
Lines 109560 109560
Branches 15522 15521 -1
==========================================
- Hits 86402 86395 -7
- Misses 22461 22465 +4
- Partials 697 700 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a81eb55
to
417e91c
Compare
417e91c
to
65eda0d
Compare
65eda0d
to
9c42220
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does mlkem-native do the assembly dispatching? This raises an interesting gap we might have in our CI: building the arm implementation once and testing on all CPUs like we do for x86 with the Intel SDE.
crypto/fipsmodule/CMakeLists.txt
Outdated
if(BORINGSSL_PREFIX) | ||
# NOTE: mlkem-native has its own symbol prefixing, but AWS-LC's post-hoc prefixing | ||
# is compatible with that. | ||
set_source_files_properties(${MLKEM_NATIVE_AARCH64_ASM_SOURCES} PROPERTIES COMPILE_FLAGS "--include=\"${AWSLC_BINARY_DIR}/symbol_prefix_include/openssl/boringssl_prefix_symbols_asm.h\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be an issue with the way this approach is prefixing the symbols:
https://github.com/aws/aws-lc/actions/runs/15842126232/job/44656634333?pr=2498#step:17:4425
Undefined symbols for architecture arm64:
"_aws_lc_0_30_0_mlkem_intt_asm", referenced from:
_mlk_intt_native in libaws_lc_sys-f5b29bebed91b9af.rlib[114](f8e4fd781484bd36-bcm.o)
"_aws_lc_0_30_0_mlkem_ntt_asm", referenced from:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two mechanisms do work together; the issue here is that the mlkem-native assembly isn't subject to the prefix header in this specific test (only).
The reason is this: cc_builder has a patch which adds --include <PREFIX_HEADER>
to the build of s2n-bignum assembly files. This patch mimicks this line in the CMakeLists.txt. A similar addition to CMakeLists.txt is made for mlkem-native in this PR, and that's why the (a) prefix builds work, (b) the aws-lc-rs test succeed with CMAKE_BUILDER=1.
One could solve this issue following precedent, by adding another --include clause to cc_builder.
However, taking a step back: Why do we do this --include patching in the first place? Why don't we just add #include <openssl/boringssl_prefix_symbols_asm.h>
to the assembly files? @nebeid @torben-hansen @dtapuska @andrewhop If someone could enlighten this, I'd be grateful.
For the time being, I pushed a test-commit attempting to add #include <openssl/boringssl_prefix_symbols_asm.h>
to the mlkem-native assembly files upon import. While this is a patch, I think this is no worse -- rather better, because more transparent -- than the --include
patches in the CMakeLists / cc_builder.
echo "Fixup include paths" | ||
sed "${SED_I[@]}" 's/#include "src\/\([^"]*\)"/#include "\1"/' $SRC/mlkem_native_bcm.c | ||
|
||
echo "Fixup AArch64 assembly backend to use s2n-bignum macros" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update aws-lc's code to not need to modify the mlkem-native code? I would like to keep this as simple as git clone and copying some directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delocator seems to have issues with nested relative #include's, so trying to have it process the ASM as it is in mlkem-native seems to be a challenge, at best. I'm not convinced that patching the delocator and build is better than patching the assembly headers to be compatible with the existing tooling.
What we're currently doing is replacing
#include "../../../common.h"
#if defined(MLK_ARITH_BACKEND_AARCH64) && !defined(MLK_CONFIG_MULTILEVEL_NO_SHARED)
.text
.balign 4
.global MLK_ASM_NAMESPACE(ntt_asm)
MLK_ASM_FN_SYMBOL(ntt_asm)
by
#include "_internal_s2n_bignum.h"
.text
.balign 4
S2N_BN_SYM_VISIBILITY_DIRECTIVE(mlkem_ntt_asm)
S2N_BN_SYM_PRIVACY_DIRECTIVE(mlkem_ntt_asm)
S2N_BN_SYMBOL(mlkem_ntt_asm):
This doesn't look too bad to me. Also, there is still room for simplification, since in mlkem-native we could use a different macro structure (e.g. wrapping .global MLK_ASM_NAMESPACE(...)
as MLK_ASM_GLOBAL(...)
which would further simplify the search-and-replace transformation to the s2n-bignum directives.
9c42220
to
bf167e4
Compare
Context: The ML-KEM implementation in AWS-LC is imported from mlkem-native. mlkem-native comes in a "C-only" version, but also offers AArch64 and x86_64 backends for (a) arithmetic, and (b) FIPS-202. Currently, only the "C-only" version is imported into AWS-LC. Summary: This commit extends the mlkem-native->AWS-LC import to include the AArch64 arithmetic backend. Details: - `crypto/fipsmodule/ml_kem/importer.sh` now imports the arithmetic backend API header `native/api.h` as well as the native backend `native/aarch64/*`. - The backend is imported as-is, with one exception: `importer.sh` converts the preprocessor directives used by mlkem-native into the ones used by s2n-bignum. This is to piggy-back on the adjustments made to the delocator to work with s2n-bignum assembly; otherwise, similar adjustments would likely be needed for mlkem-native assembly files. - All imported functions are formally verified for functional correctness using HOL-Light. The proofs run as part of mlkem-native's CI. The HOL-Light specs are manually translated into CBMC specs in the header accompanying the ASM, and all higher level CBMC proofs conducted against those specs. Again, those are part of the mlkem-native CI. - A backend header crypto/fipsmodule/ml_kem/mlkem_native_backend.h is added, activating the AArch64 arithmetic backend on Linux and MacOS AArch64 system, except if the NO_ASM directive is set (same as for s2n-bignum). Once the x86_64 arithmetic backend is ready for integration, it will be added to `mlkem_native_backend.h` as well. - The backend header is registered in the configuration file `crypto/fipsmodule/ml_kem/mlkem_native_config.h`. - The importer.sh is re-run. Signed-off-by: Hanno Becker <[email protected]>
bf167e4
to
10605b7
Compare
Signed-off-by: Hanno Becker <[email protected]>
9ffcf39
to
f53824c
Compare
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
The purpose of this PR is to demonstrate and gather feedback on one option for integrating an AArch64 arithmetic backend for mlkem-native into AWS-LC.
Alternative option: #2500
Context: The ML-KEM implementation in AWS-LC is imported from
mlkem-native. mlkem-native comes in a "C-only" version, but also
offers AArch64 and x86_64 backends for (a) arithmetic,
and (b) FIPS-202. Currently, only the "C-only" version is
imported into AWS-LC.
Summary: This commit extends the mlkem-native->AWS-LC import to include the AArch64 arithmetic backend.
Details:
crypto/fipsmodule/ml_kem/importer.sh
now importsthe arithmetic backend API header
native/api.h
as wellas the native backend
native/aarch64/*
.The backend is imported as-is, with one exception:
importer.sh
converts the preprocessor directives used bymlkem-native into the ones used by s2n-bignum. This is to
piggy-back on the adjustments made to the delocator to work
with s2n-bignum assembly; otherwise, similar adjustments would
likely be needed for mlkem-native assembly files.
All imported functions are formally verified for functional
correctness using HOL-Light. The proofs run as part of
mlkem-native's CI. The HOL-Light specs are manually translated
into CBMC specs in the header accompanying the ASM, and
all higher level CBMC proofs conducted against those specs.
Again, those are part of the mlkem-native CI.
A backend header crypto/fipsmodule/ml_kem/mlkem_native_backend.h
is added, activating the AArch64 arithmetic backend on Linux and
MacOS AArch64 system, except if the NO_ASM directive is set
(same as for s2n-bignum).
Once the x86_64 arithmetic backend is ready for integration,
it will be added to
mlkem_native_backend.h
as well.The backend header is registered in the configuration file
crypto/fipsmodule/ml_kem/mlkem_native_config.h
.The importer.sh is re-run.
Performance
Example for header patching during import:
Before (mlkem-native):
After (imported):