Skip to content
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

llext: add support for signing additional binaries #66573

Closed
wants to merge 1 commit into from

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Dec 15, 2023

This allows the use of "west sign" command for signing of additional binaries with rimage, specifically this is used to sign llext extensions for SOF.

@teburd
Copy link
Collaborator

teburd commented Dec 15, 2023

I think the feature add is fine, it would be helpful to expand a bit on the problem this solves, how it solves it, and perhaps even provide a simple snippet of how west sign might be used with llext in the commit message.

Without it, I'm going to look at this one day and wonder why did we need it, and how was it intended to be used without knowing enough from the commit.

I know this because I've ran into it already with something else recently.

@marc-hb marc-hb added the platform: Intel ADSP Intel Audio platforms label Dec 18, 2023
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

This looks like working around the fact that CONFIG_KERNEL_BIN_NAME is either wrong or the wrong constant to look at. It looks like the CMake code does not even know what filename it produces? How come? If that's the case then there is "a bigger CMake problem". At the very least there should be comments explaining what CMake design issue this is a workaround for.

"peeking" at rimage options is the main, pre-existing design problem here and it is what makes this script so complicated, brittle and hard to test. So we should try to reduce "peeking", not add more of it.

Please try to find some config.get(CONFIG_SOMETHING_ELSE) that works in both cases?

if '-r' in sign_config_extra_args + args.tool_args:
kernel_name = 'llext'
else:
kernel_name = build_conf.get('CONFIG_KERNEL_BIN_NAME', 'zephyr')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the bootloader code below still apply to this new llext thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marc-hb no, that's why it also checks for '-r' there and skips the bootloader for ELF / llext

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed that. Then this would be clearer and better (assuming peeking at -r is a good idea in the first place:

is_llext = '-r' in sign_config_extra_args + args.tool_args

kernel_name = 'llext' if is_llext else ...

...

if is_llext:

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW CONFIG_KERNEL_BIN_NAME used to be hardcoded to zephyr.elf in this script. In #59721 such hardcoding was deemed a bad idea removing. Hardcoding something again to llext seems equally bad (but hard to tell for sure without a complete use case).

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 21, 2023

I think the feature add is fine, it would be helpful to expand a bit on the problem this solves, how it solves it, and perhaps even provide a simple snippet of how west sign might be used with llext in the commit message.

@teburd sure, good idea, thanks! Example added

teburd
teburd previously approved these changes Dec 21, 2023
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

west sign -d build-mtl -t rimage -p build-rimage/rimage -- \ -k key.pem -c modules/audio/sof/src/samples/audio/smart_amp_test.toml \ -r build-mtl/zephyr/smart_amp_llext/libsmart_amp_test_out.so

west sign has two main purposes:

  1. A convenience wrapper that saves figuring out parameters and typing very long command lines
  2. An abstraction layer that CMake can invoke at build time.

In other words, west sign exists to make signing seamless and invisible, not requiring any knowledge or documentation in typical use cases.

This example from the commit message:

  1. is at least as complex as invoking rimage directly. Maybe even more complex because of the layer of indirection (what exactly does the indirection do, when etc.)
  2. does not look like something CMake/the Zephyr build system can perform right now. For now it looks like this line has to be manually inserted in some private build script. So why not add a direct and simpler rimage command in such a private build script?

Unless I missed something, this new signing looks everything but "seamless and invisible".

So @lyakh, @teburd can you please provide a complete use case? Either here or in a new GH issue. Something detailed enough so anyone (and especially me) can try to build and see for themselves how all the different parts fit together.

EDIT:

I spent a few minutes searching and I found only this example:

https://docs.zephyrproject.org/latest/samples/subsys/llext/shell_loader/README.html

west build -b tdk_robokit1 samples/subsys/llext/shell_loader

I ran it and I got the usual .elf files and no .so file. So this only example I found is probably off-topic here.

if '-r' in sign_config_extra_args + args.tool_args:
kernel_name = 'llext'
else:
kernel_name = build_conf.get('CONFIG_KERNEL_BIN_NAME', 'zephyr')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed that. Then this would be clearer and better (assuming peeking at -r is a good idea in the first place:

is_llext = '-r' in sign_config_extra_args + args.tool_args

kernel_name = 'llext' if is_llext else ...

...

if is_llext:

if '-r' in sign_config_extra_args + args.tool_args:
kernel_name = 'llext'
else:
kernel_name = build_conf.get('CONFIG_KERNEL_BIN_NAME', 'zephyr')
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW CONFIG_KERNEL_BIN_NAME used to be hardcoded to zephyr.elf in this script. In #59721 such hardcoding was deemed a bad idea removing. Hardcoding something again to llext seems equally bad (but hard to tell for sure without a complete use case).

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 2, 2024

So why not add a direct and simpler rimage command in such a private build script?

@marc-hb in principle - yes, I also was contemplating whether this should be added to west sign or "rimage" should be called directly. My motivation for adding it is that a script like xtensa-build-zephyr.py would first call west sign to sign the main image, which calls rimage, and then it would call rimage directly for each module. I thought that wouldn't be too pretty, but if you prefer that, I can live with it too.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 2, 2024

So @lyakh, @teburd can you please provide a complete use case? Either here or in a new GH issue. Something detailed enough so anyone (and especially me) can try to build and see for themselves how all the different parts fit together.

@marc-hb An example use-case is thesofproject/sof#8180 where thesofproject/sof@60678bd#diff-1c0726b9322bfeb194da9b8b5f8de441d40799bf18fce33afe8f5ad6a6cd7170 is invoked when the smart-amp sample is configured with "m" in Kconfig. That builds a .so file.
And in fact I've just tried to move that PR to the new split TOML system and I realised, that with to use it for module building and signing instead of thesofproject/sof@60678bd#diff-05cb0368fc451b7889929862fda3e42a78e86f2f473aae34bfb2110e9b5ebea5 we need a bit of scripting: we need to combine a platform TOML preamble with the module own TOML to generate a TOML for signing. So we need a file ${module}.toml.h like

#include <${platform}.toml>

[module]
count = 1

#include <${module}.toml>

and then it has to be preprocessed and used for module signing. We can either automatically generate such a mod.toml.h or use symbolic links or something. In any case module signing becomes a multi-step process, so support for in in west sign seems meaningful?

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 3, 2024

My motivation for adding it is that a script like xtensa-build-zephyr.py would first call west sign to sign the main image, which calls rimage, and then it would call rimage directly for each module.

sof/xtensa-build-zephyr.py hasn't called west sign for a long time, I removed that almost one year ago. sof/xtensa-build-zephyr.py is very SOF-specific and Zephyr needs to be able to sign SOF-free unit-tests to run them on hardware (and it can and does). Generally speaking sof/xtensa-build-zephyr.py is a configuration and installation script, not a build script or as little as possible.

To make sure everyone and everything signs the same, west sign is now invoked in CMake, immediately after building. So I feel like modules should be signed from their respective CMake files too for consistency, clarity and simplicity. Of course these CMakefiles could/should share some common CMake functions and code if needed.

I'm not sure the CMake files of llext modules need the west sign indirection, it feels like "too many layers of indirection" getting in the way of fine-grained rimage control and parameters. Just an early gut feeling.

we need a bit of scripting: we need to combine a platform TOML preamble with the module own TOML to generate a TOML for signing.

As I already wrote in #65411 (comment), pre-processing and generating files normally belongs to the build system/CMake: that's exactly what build systems are designed for. We made an exception for zephyr.elf to rush something out of the door but I thought this was a once off...

I need a bit more time to think about all this, so far this looks like "bug-driven build system (lack of) design"

@marc-hb marc-hb requested a review from tejlmand January 6, 2024 00:39
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 6, 2024

After experimenting a bit more I've come to the conclusion that the Zephyr build system is simply not ready for llext:

For now the llext developer is very much left on their own with respect to building the library. Signing is the last step in the (llext) build, so trying to make west sign work while all the previous steps are still in flux does really not look like a good idea.

My motivation for adding it is that a script like xtensa-build-zephyr.py would first call west sign to sign the main image, which calls rimage, and then it would call rimage directly for each module. I thought that wouldn't be too pretty, but if you prefer that, I can live with it too.

Let's please start with such a cruder approach for now, we can revisit later once we have more build system experience with llext. But please try to invoke the preprocessor and rimage from your the llext's CMake file rather than from xtensa-build-zephyr.py. That's because the build system belongs to CMake, xtensa-build-zephyr.py is just a convenience wrapper + default configuration + workarounds on top of Zephyr build system. We must always minimize differences between the outputs of xtensa-build-zephyr.py and invoking west build directly. I realize writing Python is easy and CMake is horrible but we want to make progress towards building llext using Zephyr's actual build system and not just add workarounds outside of it.

west sign has for now been designed with a strong "single binary" assumption which clashes with llext. Consider for instance the west config for rimage which is used by xtensa-build-zephyr or some end users. Which parameters there apply to which binary? No one knows yet.

west sign is an abstraction layer that provides good defaults. At this moment it would not abstract anything and just get in the way.

This allows the use of "west sign" command for signing of additional
binaries with rimage, specifically this is used to sign llext
extensions for SOF.
Example:

west sign -d build-mtl -t rimage -p build-rimage/rimage -- \
-k key.pem -c modules/audio/sof/src/samples/audio/smart_amp_test.toml \
-r build-mtl/zephyr/smart_amp_llext/libsmart_amp_test_out.so

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 17, 2024

After experimenting a bit more I've come to the conclusion that the Zephyr build system is simply not ready for llext. For now the llext developer is very much left on their own with respect to building the library.

Awesome news: this is being addressed in #67431 + future PRs.

EDIT, more now in #67997

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: West West utility platform: Intel ADSP Intel Audio platforms Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants