LLEXT full ARM relocation support (adding executable) #70171

selescop · 2024-03-13T14:30:54Z

Add a full support for ARM relocations.
This allows to load a complete application (ARM ELF) from filesystem into RAM and execute it.
All symbols exported via EXPORT_SYMBOL() macro can be called, ex: thread creation
The "simple" sample has been updated.
Tests are provided making sure to not break the xtensa's one

We'd like to open a discussion about (cyber)security, especially on the USERSPACE mode -> new MPU section to be revisited

github-actions · 2024-03-13T14:31:30Z

Hello @selescop, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

teburd

First off this is a lot of great work, so thank you. There's many new things in this one PR and it will be rather time consuming to review all at once.

Some broad suggestions on what some easier to review PRs might be...

arch relocate API and relocations added for arm (great!) with some test extensions added to the existing test case
filesystem loader + test/sample updates (great!)
more explicit control over the location of the heap with the linker script/mpu region macros (not clear at a quick glance why this is needed, some further background information could be helpful)
cmake changes to the existing llext module as needed in each rather than an all new llext.cmake module I think might be a better route

bjarki-trackunit

Many lovely things in this PR, however, some notes:

This PR is quite large, and contains features that are not related to each other, like the added arm relocations and the fs_loader. This should be split into separate PRs
Each added relocation (except relocs that are treated equally) should get its own commit, and tests should be added to the test suite to ensure the added relocation is actually produced by one of the extensions so it can be tested.
Some changes are not needed, like changing function names from test_entry to start, the PR is easier to review if changes like these are not introduced.

Splitting this PR into multiple more focused PRs so we can make incremental changes is preferred over large (somewhat cluttered) PRs, which tend to end up hanging for a long time in review limbo :)

cmake/modules/llext.cmake

samples/subsys/llext/fs_loader/CMakeLists.txt

samples/subsys/llext/fs_loader/hello_world.llext/hello_world.c

nordicjm · 2024-03-14T08:47:23Z

samples/subsys/llext/fs_loader/makeimg.sh

+echo "===> Creating container image"
+fallocate -l ${DISK_SIZE_KB}k "$OUTPUT"
+echo "===> Creating FAT partition image"
+mkfs.fat -F12 -S"$LOGICAL_SECTOR_SIZE" "$OUTPUT" >/dev/null


How does a user on windows use any of this? Or mac?

I'll change 'fallocate' call for 'dd' call.
It works on MSYS2/MSYS and MSYS2/MinGW. Is this what you mean?

Well it needs instructions for windows/mac users on how to use this (e.g. readme.rst)

There is a README.rst for the fs_loader
I'll make this script more user-friendly (+usage & co)

Some general shell advice:

Use shellcheck. shellcheck saves lives.

Use set -e ([sof-test] add set -e to all test cases (Unless You Love Debugging) thesofproject/sof-test#312)

All code in a function(); use a main() function (clean-up: move all shell script code to a function and use a "main" thesofproject/sof-test#740)

I don't think the Zephyr project has any shell guidelines (for the simple reason that it tries to support vanilla Windows?) but these are small changes that give huge rewards.

Hello @marc-hb,
I've written the script to be more friendly (usage & co). It can now runs on GNU OS.

For the 'set-e', I'm not really into that. There are many commands that does not return 0 on success. Moreover, a command that fails in a script does not necessarily means that the script should fail as well. I prefer use 'if'/'else' instead and try to give more information to the user whenever possible (like, why the command failed, maybe why the script fails and maybe give help. Silent failures are not great user experience in my opinion)

For the 'set-e', I'm not really into that. There are many commands that does not return 0 on success.

There are a few but it's very rare actually. Our entire test suite uses set -e with great success.

Moreover, a command that fails in a script does not necessarily means that the script should fail as well.

Yes of course. The fix is very simple: unusual_command || true

I prefer use 'if'/'else' instead and try to give more information to the user whenever possible (like, why the command failed, maybe why the script fails and maybe give help.

Yes of course. It's not "either ... or... ". set -e does absolutely not stop you from doing that. You can and should still do this even with set -e:

cmd1 "$arg1" || die "cmd1 %s failed\n" "$arg1"

set -e is for the other commands: the ones you did NOT expect to fail. It brings shell script closer to all other languages (except C)

Silent failures are not great user experience in my opinion)

Agreed 100%. The only thing worse is a silent failure AND a script that keeps running anyway - because no set -e.

How does a user on windows use any of this? Or mac?

The portability of Zephyr's production code and build system is amazing. On the other hand, test code is a much bigger challenge. Last time I checked, twister was quite limited on Windows.

This small shell script is in the samples/ directory.

samples/subsys/llext/fs_loader/src/main.c

subsys/llext/Kconfig

bjarki-trackunit

Most of the relocations are copied pretty directly from linux, which is GPL V2 https://github.com/torvalds/linux/blob/480e035fc4c714fb5536e64ab9db04fedc89e910/arch/arm/kernel/module.c#L326-L341
I am no licensing expert but I don't think we can copy and modify GPL V2 code without going through quite a few hoops, neither do I think we should do so.

It is quite evident from the inheritance of macros like ___opcode_identity32 and a host of undefined const values like (upper & 0x03ff) << 12 that this is not written in the Zephyr code style. static inline functions are preferred over macros if possible, const values are defined and used by name #define SOME_MASK 0xFF3, and macros are UPPER_CASE (unless they are shadowing actual functions like assert())

My current stance on the added relocations specifically is that we should write them in a Zephyr style, and not copy directly from Linux, to get a simpler and cleaner file with no licensing clash.

arch/arm/core/elf.c

nashif · 2024-03-14T15:21:37Z

Most of the relocations are copied pretty directly from linux, which is GPL V2 torvalds/linux@480e035/arch/arm/kernel/module.c#L326-L341
I am no licensing expert but I don't think we can copy and modify GPL V2 code without going through quite a few hoops, neither do I think we should do so.

agree and good catch @bjarki-trackunit.

selescop · 2024-03-14T15:22:03Z

My current stance on the added relocations specifically is that we should write them in a Zephyr style, and not copy directly from Linux, to get a simpler and cleaner file with no licensing clash.

Yes, not hiding it here, relocations are relocations so we got highly inspired from our big brother. Nevertheless, I'll redo a pass to make it more Zephyr-like

There's many new things in this one PR

Splitting this PR into multiple more focused PRs

Let me fix all "CI" issues first and then I'll split it in several PRs

marc-hb · 2024-03-14T22:59:39Z

I think it would be better to merge this small, 64bits compilation check first - before adding a lot of new code that may or may not compile in 64bits without warning. Please help review:

tests: llext: compile architectures not supported yet #70000

Splitting this PR into multiple more focused PRs

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

There's also some advice for reviewers on this page.

selescop · 2024-03-15T09:22:32Z

I think it would be better to merge this small, 64bits compilation check first - before adding a lot of new code that may or may not compile in 64bits without warning. Please help review:

tests: llext: compile architectures not supported yet #70000

It has been approved ;)

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

There's also some advice for reviewers on this page.

Well, that's what I tried to follow. Feature is LLEXT only. Tests have been added. Documentation have been done. PR is break down in several commits. I can break down this PR into multiple ones but it won't change the amount of code to review.

bjarki-trackunit · 2024-03-15T11:07:41Z

Well, that's what I tried to follow. Feature is LLEXT only. Tests have been added. Documentation have been done. PR is break down in several commits. I can break down this PR into multiple ones but it won't change the amount of code to review.

A PR should be reviewed thoroughly by every reviewer if possible, which takes time. Reviewers can only allocate a small timeslot to each review, so spreading it out over time in smaller chunks is essential

marc-hb · 2024-03-15T15:11:46Z

samples/subsys/llext/fs_loader/makeimg.sh

+fi
+echo -ne "done\nCopying input files..."
+mcopy -i "$OUTPUT" $INPUTS "::/"
+if [ $? -ne 0 ]; then


Suggested change

if [ $? -ne 0 ]; then

mcopy -i "$OUTPUT" $INPUTS "::/" || die "mcopy failed"

Where die is something like this:

die() { printf "$*"; exit 1; }

marc-hb · 2024-03-15T15:14:43Z

samples/subsys/llext/fs_loader/makeimg.sh

+DISK_SIZE_KB=128
+
+if [ $# -lt 2 ]; then
+    echo -e "Not enough arguments.\nUsage:\n\t`basename $0` output.img input1 [input2] [...]"


Avoid echo (and use shellcheck)

Suggested change

echo -e "Not enough arguments.\nUsage:\n\t`basename $0` output.img input1 [input2] [...]"

printf 'Not enough arguments.\nUsage:\n\t%s output.img input1 [input2] [...]' "$0"

Or much more readable for long strings:

cat <<EOFUSAGE Not enough arguments. Usage: $0 output.img input1 [input2] [...] EOFUSAGE

marc-hb · 2024-03-15T17:46:27Z

samples/subsys/llext/fs_loader/makeimg.sh

+fi
+
+echo -ne "Creating empty '`basename $OUTPUT`' image..."
+dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none 2>/dev/null || die "failed."


Suggested change

dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none 2>/dev/null || die "failed."

dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none || die "dd to $OUTPUT failed."

You are already using status=none so don't discard the error message. Unless some tool's design is extremely bad and absolutely requires it, never discard stderr. It's really bad practice and can cost hours and hours of debugging.

You wrote that you don't like silent failures but this line currently replaces a relevant error message with a meaningless "failed" which is no better than silence. Same below.

marc-hb · 2024-03-15T17:51:00Z

samples/subsys/llext/fs_loader/makeimg.sh

+echo -ne "done\nCreating FAT partition image..."
+mkfs.fat -F12 -S"$LOGICAL_SECTOR_SIZE" "$OUTPUT" >/dev/null || die "failed."
+echo -ne "done\nCopying input files..."
+mcopy -i "$OUTPUT" $INPUTS "::/" || die "failed."


Suggested change

mcopy -i "$OUTPUT" $INPUTS "::/" || die "failed."

mcopy -i "$OUTPUT" $INPUTS "::/" || die "mcopy "$OUTPUT" $INPUTS failed."

marc-hb · 2024-03-15T17:51:33Z

samples/subsys/llext/fs_loader/makeimg.sh

+echo -ne "Creating empty '`basename $OUTPUT`' image..."
+dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none 2>/dev/null || die "failed."
+echo -ne "done\nCreating FAT partition image..."
+mkfs.fat -F12 -S"$LOGICAL_SECTOR_SIZE" "$OUTPUT" >/dev/null || die "failed."


marc-hb · 2024-03-15T17:53:23Z

samples/subsys/llext/fs_loader/makeimg.sh

+# - dosfstools
+# - mtools
+
+PROGNAME=`basename $0`


backquotes are deprecated. Use shellcheck.

marc-hb · 2024-03-15T18:01:19Z

samples/subsys/llext/fs_loader/makeimg.sh

+mkfs.fat -F12 -S"$LOGICAL_SECTOR_SIZE" "$OUTPUT" >/dev/null || die "failed."
+echo -ne "done\nCopying input files..."
+mcopy -i "$OUTPUT" $INPUTS "::/" || die "failed."
+echo "done"


Suggested change

echo "done"

printf "$0 done\n"

What is "done"? This line will be buried in thousands of other lines in the build logs.

samples/subsys/llext/fs_loader/makeimg.sh

stale review

selescop · 2024-03-18T18:39:13Z

@bjarki-trackunit @nordicjm
Are you OK with the changes? If yes, I'll split this PR.

marc-hb · 2024-03-18T18:50:33Z

cmake changes to the existing llext module as needed in each rather than an all new llext.cmake module

I noticed this only now and it is quite puzzling. How is this not duplicating some of the work in #67997 and friends? Sorry no time and not enough knowledge to review it right now.

Tagging @pillo79 and @tejlmand

selescop · 2024-03-18T19:06:56Z

How is this not duplicating some of the work in #67997 and friends?

Well, not duplicating but it is the same kind of idea. We need a way to build an extension and we both built a cmake target for it (factorization). The other PR#67997 is based on shared library CMake mechanism as here we build a partially linked ELF: same need/idea, different way to implement it.

Adds support for all relocation type produced by GCC on ARM platform using partial linking (-r flag) or shared link (-fpic and -shared flag). Adds a section MPU according to allow using llext with MPU enabled. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

Adds a file loader in llext. With this loader, llext can load an extension using a file descriptor. Add shell command to use this loader. Adds a shell command to print llext heap usage. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

This patch adds a cmake macro named zephyr_llext to define an extension and build it. Source files are defined using zephyr_llext_sources cmake function. Include directories are defined using zephyr_llext_include_directories cmake function. Compilation and link flags are computed from zephyr_interfaces. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

Adds a sample how to use llext file loader. 2 extensions are defined and added in a fatfs image that can be flash on target. Zephyr application loads and calls start symbol on both extensions. Extension start symbol creates a thread printing thread id each seconds. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

Updates llext tests to use new cmake extension declaration. MPU is no longer disabled. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

pillo79 · 2024-03-18T19:40:09Z

@selescop, you did an amazing amount of work so first and foremost thank you! 🚀
In it I see lots of things that I would love to integrate with the current llext implementation.
However, I must add myself to the chorus and ask you to split this as the first step for merging it - it is simply too big to do any meaningful review at once. (See that PR above for how long it takes to get everyone on the same page for a 3 file change... 😉)

You could keep this big PR as a 'draft' (so you can run CI on it), and take out one bit at a time for review/inclusion. Once a bit is merged you can rebase and proceed with the next step.
Again, thanks for your contribution! 🙇

selescop · 2024-03-18T19:45:27Z

You could keep this big PR as a 'draft' (so you can run CI on it), and take out one bit at a time for review/inclusion. Once a bit is merged you can rebase and proceed with the next step. Again, thanks for your contribution! 🙇

Yep, that's my tomorrow's plan. Now it's family time 😄

tejlmand

Took a look at the build system related code.

Some early comments before going more in depths in some areas, but initial feeling is that the direction looks promising 👍

tejlmand · 2024-04-10T11:19:49Z

cmake/modules/llext.cmake

+#
+
+
+macro(zephyr_llext name)


why is this a macro and not a function ?

All variables defined inside a macro will keep living when macro returns, which can give some nasty surprises, so extra care must be taken when deciding to define a macro.

So far I've not seen a reason why this must be implemented as a macro, so would like to understand the reasons.

tejlmand · 2024-04-10T11:22:39Z

cmake/modules/llext.cmake

+    list(LENGTH ExtraMacroArgs NumExtraMacroArgs)
+    # Execute the following block only if the length is > 0
+    if(NumExtraMacroArgs GREATER 0)
+        foreach(ExtraArg ${ExtraMacroArgs})
+            if(${ExtraArg} STREQUAL PIC)
+                set(LLEXT_IS_PIC yes)
+            endif()
+        endforeach()
+    endif()


a bit unusual.

Why not a proper function and then use cmake_parse_arguments() ?

tejlmand · 2024-04-10T11:34:56Z

cmake/modules/llext.cmake

+    )
+
+    if(LLEXT_IS_PIC)
+        target_compile_options(${ZEPHYR_CURRENT_LLEXT} PRIVATE -fpic -fpie)


I know this code in general is GNU centric, but we actually have compiler flags defined for such cases to avoid custom toolchain flags / handling throughout the codebase.

This makes it easier to add better support for more toolchains in Zephyr.

For example:

zephyr/cmake/compiler/gcc/compiler_flags.cmake

Lines 232 to 234 in 9c05618

set_compiler_property(PROPERTY no_position_independent

-fno-pic

-fno-pie

tejlmand · 2024-04-10T11:35:32Z

cmake/modules/llext.cmake

+        target_compile_options(${ZEPHYR_CURRENT_LLEXT} PRIVATE -fpic -fpie)
+    else()
+        if("${ARCH}" STREQUAL "arm")
+            target_compile_options(${ZEPHYR_CURRENT_LLEXT} PRIVATE -mlong-calls)


see comment regarding compiler flag handling.

tejlmand · 2024-04-10T11:46:50Z

samples/subsys/llext/fs_loader/makeimg.sh

+printf "Creating empty '%s' image..." "$(basename "$OUTPUT")"
+dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none || die "dd to $OUTPUT"
+printf "done\nCreating FAT partition image..."
+mkfs.fat -F12 -S"$LOGICAL_SECTOR_SIZE" "$OUTPUT" >/dev/null || die "mkfs.vfat failed"
+printf "done\nCopying input files..."
+mcopy -i "$OUTPUT" "$INPUTS" "::/" || die "mcopy $OUTPUT $INPUTS"


afaict this can also be done in Python. Perhaps take a look at pyfatfs.

Could we have a Python implementation instead, and thereby be one step closer to supporting llext in windows and MacOS ?

zephyrbot added area: Build System area: Linker Scripts area: Architectures area: Samples Samples area: ARM ARM (32-bit) Architecture area: Linkable Loadable Extensions labels Mar 13, 2024

zephyrbot requested review from bbolen, carlocaione, galak, ithinuel, jeremybettis, kartben, lyakh, MaureenHelm, microbuilder, nashif, nordicjm, pillo79, stephanosio, teburd and tejlmand March 13, 2024 14:31

zephyrbot assigned teburd Mar 13, 2024

selescop force-pushed the llext_arm_executable branch from 6baff14 to 63348e9 Compare March 13, 2024 15:09

teburd reviewed Mar 13, 2024

View reviewed changes

bjarki-trackunit reviewed Mar 13, 2024

View reviewed changes

nordicjm requested changes Mar 14, 2024

View reviewed changes

selescop force-pushed the llext_arm_executable branch from 63348e9 to 60572d1 Compare March 14, 2024 12:53

bjarki-trackunit requested changes Mar 14, 2024

View reviewed changes

arch/arm/core/elf.c Outdated Show resolved Hide resolved

laurenmurphyx64 mentioned this pull request Mar 14, 2024

LLEXT: Add reloc support for R_ARM_THM_CALL (BL and BLX instructions) #70100

Merged

selescop force-pushed the llext_arm_executable branch from 60572d1 to 81e9134 Compare March 14, 2024 18:23

marc-hb reviewed Mar 15, 2024

View reviewed changes

selescop force-pushed the llext_arm_executable branch from 81e9134 to afba247 Compare March 15, 2024 16:46

marc-hb previously requested changes Mar 15, 2024

View reviewed changes

selescop force-pushed the llext_arm_executable branch 2 times, most recently from 2d36bf5 to 43ebe02 Compare March 18, 2024 17:09

selescop requested review from bjarki-trackunit and nordicjm March 18, 2024 18:36

selescop added 5 commits March 18, 2024 20:38

llext: tests: Update tests

0513e09

Updates llext tests to use new cmake extension declaration. MPU is no longer disabled. Signed-off-by: Cedric Lescop <cedric.lescop@se.com>

selescop force-pushed the llext_arm_executable branch from 43ebe02 to 0513e09 Compare March 18, 2024 19:42

selescop mentioned this pull request Mar 19, 2024

llext: Full ARM ELF relocation support #70452

Merged

tejlmand reviewed Apr 10, 2024

View reviewed changes

	if [ $? -ne 0 ]; then
	mcopy -i "$OUTPUT" $INPUTS "::/" \|\| die "mcopy failed"

	echo -e "Not enough arguments.\nUsage:\n\t`basename $0` output.img input1 [input2] [...]"
	printf 'Not enough arguments.\nUsage:\n\t%s output.img input1 [input2] [...]' "$0"

	dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none 2>/dev/null \|\| die "failed."
	dd if=/dev/zero of="$OUTPUT" bs=1k count=${DISK_SIZE_KB} status=none \|\| die "dd to $OUTPUT failed."

	set_compiler_property(PROPERTY no_position_independent
	-fno-pic
	-fno-pie

		#


		macro(zephyr_llext name)

LLEXT full ARM relocation support (adding executable) #70171

Are you sure you want to change the base?

LLEXT full ARM relocation support (adding executable) #70171

Conversation

selescop commented Mar 13, 2024

github-actions bot commented Mar 13, 2024

teburd left a comment • edited

Choose a reason for hiding this comment

bjarki-trackunit left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarki-trackunit left a comment • edited

Choose a reason for hiding this comment

nashif commented Mar 14, 2024

selescop commented Mar 14, 2024

marc-hb commented Mar 14, 2024 • edited

selescop commented Mar 15, 2024

bjarki-trackunit commented Mar 15, 2024

marc-hb Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Mar 15, 2024 • edited

Choose a reason for hiding this comment

selescop commented Mar 18, 2024

marc-hb commented Mar 18, 2024

selescop commented Mar 18, 2024

pillo79 commented Mar 18, 2024

selescop commented Mar 18, 2024

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment •

edited

bjarki-trackunit left a comment •

edited

marc-hb Mar 15, 2024 •

edited

bjarki-trackunit left a comment •

edited

marc-hb commented Mar 14, 2024 •

edited

marc-hb Mar 15, 2024 •

edited

marc-hb Mar 15, 2024 •

edited