-
Notifications
You must be signed in to change notification settings - Fork 41
Use a single configuration file for internal and external headers #1352
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
Conversation
ff790df to
e44080d
Compare
e44080d to
a69582c
Compare
|
@davidchisnall @mkannwischer I'd be glad if you could take a look. The main goal here is to make integration simpler which @davidchisnall reported to be cumbersome in the previous split-config approach. |
mkannwischer
left a comment
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.
Thanks @hanno-becker. Seems to work like a charm.
I have a couple of comments/questions.
0128862 to
96cad0c
Compare
96cad0c to
1ea67c4
Compare
1ea67c4 to
94da62b
Compare
mkannwischer
left a comment
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.
Thanks @hanno-becker. Very nice improvement.
Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
Previously, mlkem_native.h required users to set MLK_CONFIG_API_* macros
before including it, separate from the MLK_CONFIG_* macros used for the
build configuration. This distinction between internal and external
configuration it confusing.
Now, mlkem_native.h derives all API settings from the same
build configuration file used internally.
- The legacy configuration via MLK_CONFIG_API_XXX still works for
backwards compatibility, but will be removed in v2.
- New Config Options:
* MLK_CONFIG_MULTILEVEL_BUILD: Explicit flag for multi-level builds
This makes it simpler to get headers for multi-level builds.
In the common case, the configs used in a multi-level build are
identical except for the setting of MLK_CONFIG_PARAMETER_SET and
MLK_CONFIG_MULTILEVEL_{NO,WITH}_SHARED. However, the latter only
affect the build, not the API.
With MLK_CONFIG_MULTILEVEL_BUILD exposed as a separate option,
mlkem_native.h can use it to determine whether to suffix the
namespace prefix with the parameter set (512/768/1024) or not,
so no adaptation of MLK_CONFIG_MULTILEVEL_{NO,WITH}_SHARED is
needed. Instead, a multi-level header simply becomes:
```c
#define MLK_CONFIG_PARAMETER_SET 512
#include "mlkem_native.h"
#undef MLK_CONFIG_PARAMETER_SET
#define MLK_CONFIG_PARAMETER_SET 768
#include "mlkem_native.h"
#undef MLK_CONFIG_PARAMETER_SET
#define MLK_CONFIG_PARAMETER_SET 1024
#include "mlkem_native.h"
#undef MLK_CONFIG_PARAMETER_SET
```
For backwards compatibility, MLK_CONFIG_MULTILEVEL_BUILD is not
used in the build, which continues to detect multilevel builds
via the existing options MLK_CONFIG_MULTILEVEL_{WITH,NO}_SHARED.
* MLK_CONFIG_NO_SUPERCOP: We had this before in mlkem_native.h as
MLK_CONFIG_API_NO_SUPERCOP; it's now in the config and documented.
* MLK_CONFIG_CONSTANTS_ONLY: We had this before in mlkem_native.h as
MLK_CONFIG_API_CONSTANTS_ONLY; it's now in the config and documented.
- Build-Internal vs API Config Separation: Config file now guards
build-only options with #if defined(MLK_BUILD_INTERNAL).
- Example Directory Restructuring: All examples refactored with mlkem/
subdirectories renamed to mlkem_native/ and CFLAGS configuration
replaced by custom configs.
Updated READMEs to reflect new paths and clarify which options are
set in config files vs passed via CFLAGS.
Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
94da62b to
f0668d2
Compare
|
@davidchisnall This is ready for merge now. Do you have time to give it a try? |
|
Thanks for this, I can confirm that it works nicely for my use case. #1351 is not necessary after it. Doing the same in mldsa would be amazing. |
|
@davidchisnall Thank you for confirming, yes, we'll do the same for mldsa-native. Out of interest, which repository are you targeting for your integration? https://github.com/CHERIoT-Platform/cheriot-rtos? |
|
Once it's a bit cleaner, I'll create a new one for PQC libraries. |
@mkannwischer is doing this here pq-code-package/mldsa-native#782. Should hopefully be ready some point today. |
|
@davidchisnall This is merged in mldsa-native as well, now. If you are interested in mldsa-native, I assume that the memory usage is a problem there as well (both the fact that it's still statically stack-allocated, and the mere amount of RAM). Feel free to open an issue so the desire for optimizations in this regard is publicly documented. Focusing on verification first, @mkannwischer has already made significant reductions in pq-code-package/mldsa-native#751; this is still experimental, but knowing that this would be useful for your use case would provide good impetus to continue with it. |
Variant of #1348 which aims to be API-compatible so we can merge it prior to v2.
Previously, mlkem_native.h required users to set MLK_CONFIG_API_* macros
before including it, separate from the MLK_CONFIG_* macros used for the
build configuration. This distinction between internal and external
configuration it confusing.
Now, mlkem_native.h derives all API settings from the same
build configuration file used internally. Also, the configuration
file is moved from
mlkem/src/config.htomlkem/mlkem_native_config.h.The legacy configuration via MLK_CONFIG_API_XXX still works for
backwards compatibility, but will be removed in v2.
New Config Options:
MLK_CONFIG_MULTILEVEL_BUILD: Explicit flag for multi-level builds
This makes it simpler to get headers for multi-level builds.
In the common case, the configs used in a multi-level build are
identical except for the setting of MLK_CONFIG_PARAMETER_SET and
MLK_CONFIG_MULTILEVEL_{NO,WITH}_SHARED. However, the latter only
affect the build, not the API.
With MLK_CONFIG_MULTILEVEL_BUILD exposed as a separate option,
mlkem_native.h can use it to determine whether to suffix the
namespace prefix with the parameter set (512/768/1024) or not,
so no adaptation of MLK_CONFIG_MULTILEVEL_{NO,WITH}_SHARED is
needed. Instead, a multi-level header simply becomes:
For backwards compatibility, MLK_CONFIG_MULTILEVEL_BUILD is not
used in the build, which continues to detect multilevel builds
via the existing options MLK_CONFIG_MULTILEVEL_{WITH,NO}_SHARED.
MLK_CONFIG_NO_SUPERCOP: We had this before in mlkem_native.h as
MLK_CONFIG_API_NO_SUPERCOP; it's now in the config and documented.
MLK_CONFIG_CONSTANTS_ONLY: We had this before in mlkem_native.h as
MLK_CONFIG_API_CONSTANTS_ONLY; it's now in the config and documented.
Build-Internal vs API Config Separation: Config file now guards
build-only options with #if defined(MLK_BUILD_INTERNAL).
Example Directory Restructuring: All examples refactored with mlkem/
subdirectories renamed to mlkem_native/ and CFLAGS configuration
replaced by custom configs.
Updated READMEs to reflect new paths and clarify which options are
set in config files vs passed via CFLAGS.