LLEXT dependencies: add support for linking between LLEXT objects #77473

lyakh · 2024-08-23T12:04:49Z

This adds support for exporting symbols between extensions while building dependencies based on such symbol references

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

faxe1008 · 2024-08-23T13:52:22Z

@lyakh sorry for marking this as ready for review, I misclicked in the GitHub mobile app. Apologies for the noise.

teburd

Looks mostly reasonable to me, some oddities that are maybe caused by using llext_iterate when perhaps instead a different approach would avoid some strangeness

Should fix the formatting issues otherwise misra checks will fail and someone just fixed many issues in the tree related to misra styling.

include/zephyr/llext/symbol.h

subsys/llext/llext_load.c

teburd · 2024-08-23T13:59:16Z

include/zephyr/llext/llext.h

@@ -111,6 +114,8 @@ struct llext {

 	/** Extension use counter, prevents unloading while in use */
 	unsigned int use_count;
+
+	struct llext *dependency[LLEXT_MAX_DEPENDENCIES];


This could use a doc string

subsys/llext/llext_link.c

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

mathieuchopstm

Neat piece of work!

A few comments from me, but otherwise looks pretty solid:

subsys/llext/llext_load.c

include/zephyr/llext/symbol.h

mathieuchopstm · 2024-08-23T15:36:44Z

subsys/llext/llext_link.c

Nit: this sounds prone to race conditions - but the whole subsystem is, anyways, as struct llext lacks a lock... This is well outside PR scope though 😄

subsys/llext/llext_link.c

pillo79

Agree with all comments, I'm late to the party 🙂
Otherwise LGTM as well! 👍 Thanks!

Remove a loop counter, that isn't actually used for anything. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Currently LLEXT has two ways to export symbols: EXPORT_SYMBOL() to exort symbols from the main firmware to extensions LL_EXTENSION_SYMBOL() to export symbols from extensions but it is possible to write code, that needs to export symbols regardless of whether it is built into the main image or an extension. And in fact there's no real need in two distinct macros - we can use one for both cases and recognise at build time which direction exporting is happening. This patch extends EXPORT_SYMBOL() to also work from extensions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Replace LL_EXTENSION_SYMBOL() with EXPORT_SYMBOL() in all tests and samples. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

LLEXT objects can also export symbols for use by other such objects. That means, that when linking an LLEXT object we have to look for unresolved symbols in previously loaded objects too. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Using the immediate logging option from LLEXT modules requires one more symbol export. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

When deciding which sections to group together, only the low 3 section attribute bits are relevant, ignore the rest. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

When an LLEXT object uses symbols of another such object, it depends on it. Such dependencies have to be tracked to prevent their accidental unloading. Ideally we should be able to track arbitrary numbers of such dependencies, but this is a bit difficult. In this first implementation we use a fixed-size array, currently consisting of 8 entries. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

pillo79

LGTM, thanks for this!
Just mentioning a tiny refactor issue, maybe fix it if you ever have to rework commits.

pillo79 · 2024-08-30T13:17:41Z

subsys/llext/llext_link.c

+				if (link_addr == 0) {
+					/* Try loaded tables */
+					struct llext *dep;
+
+					link_addr = (uintptr_t)llext_find_extension_sym(name, &dep);
+					if (link_addr) {
+						llext_dependency_add(ext, dep);
+					}
+				}
+


Very minor nit - if you ever have to refactor anything: this bit of code is added in the last commit, which only deals with dependency tracking. Similar to what happens for Xtensa, it could be split between the commit that introduces the actual lookup and the one adding tracking.

mathieuchopstm

LGTM.

Nit: I feel like llext_find_extension_sym would be a more fitting name for what we currently call llext_find_sym, and vice-versa... just food for thought, changing the API is not in scope for this PR 😄

lyakh · 2024-08-30T15:12:01Z

#77738 adds a (draft but should be already working) test for this new functionality, which will be finalised and released for review and merging once this has been merged

teburd

Very nice set of changes

teburd · 2024-08-30T17:21:53Z

include/zephyr/llext/llext.h

@@ -62,6 +62,9 @@ enum llext_mem {
 struct llext_loader;
 /** @endcond */

+/* Maximim number of dependency LLEXTs */
+#define LLEXT_MAX_DEPENDENCIES 8


This should be a Kconfig, not a blocker but would be good to see a follow up for

aescolar · 2024-09-02T09:56:27Z

Added dnm given @lyakh's initial comment indicating this was a draft, to avoid accidental merge. Please confirm you'd like to have this merged. You (or anybody else) is welcome to remove the dnm.

lyakh · 2024-09-02T10:16:27Z

Added dnm given @lyakh's initial comment indicating this was a draft, to avoid accidental merge. Please confirm you'd like to have this merged. You (or anybody else) is welcome to remove the dnm.

@aescolar sorry, forgot to update the PR description, done now, so, yes, this is targeted for merge

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh requested review from teburd, pillo79 and mathieuchopstm August 23, 2024 12:05

lyakh added a commit to lyakh/sof that referenced this pull request Aug 23, 2024

[DNM] test Zephyr PR 77473

4fe43a7

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh added a commit to lyakh/sof that referenced this pull request Aug 23, 2024

[DNM] test Zephyr PR 77473

783dbd4

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh added a commit to lyakh/sof that referenced this pull request Aug 23, 2024

test Zephyr PR 77473

e37a334

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

faxe1008 marked this pull request as ready for review August 23, 2024 13:45

zephyrbot added area: llext Linkable Loadable Extensions area: Logging area: Formatting Output labels Aug 23, 2024

zephyrbot requested review from dcpleung and nordic-krch August 23, 2024 13:46

zephyrbot assigned teburd Aug 23, 2024

faxe1008 marked this pull request as draft August 23, 2024 13:51

teburd reviewed Aug 23, 2024

View reviewed changes

lyakh added a commit to lyakh/sof that referenced this pull request Aug 23, 2024

test Zephyr PR 77473

2728725

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

mathieuchopstm reviewed Aug 23, 2024

View reviewed changes

pillo79 reviewed Aug 28, 2024

View reviewed changes

lyakh added 5 commits August 28, 2024 15:06

llext: remove an unused variable

7deb037

Remove a loop counter, that isn't actually used for anything. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: use EXPORT_SYMBOL() universally

d596509

Replace LL_EXTENSION_SYMBOL() with EXPORT_SYMBOL() in all tests and samples. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: export a symbol needed for immediate logging

9a1f217

Using the immediate logging option from LLEXT modules requires one more symbol export. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the depend branch from b3705b3 to 9f908d4 Compare August 28, 2024 14:51

llext: fix flag evaluation for section grouping

b2114ea

When deciding which sections to group together, only the low 3 section attribute bits are relevant, ignore the rest. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the depend branch 2 times, most recently from 99679e0 to 89d27f7 Compare August 29, 2024 07:41

lyakh marked this pull request as ready for review August 29, 2024 08:56

zephyrbot added the area: Samples Samples label Aug 29, 2024

zephyrbot requested review from kartben and nashif August 29, 2024 08:57

lyakh mentioned this pull request Aug 29, 2024

LLEXT: dependency test #77738

Merged

lyakh force-pushed the depend branch from 89d27f7 to 3ecc166 Compare August 29, 2024 15:31

lyakh force-pushed the depend branch from 3ecc166 to c110441 Compare August 29, 2024 15:34

lyakh requested review from pillo79 and teburd August 30, 2024 13:03

pillo79 approved these changes Aug 30, 2024

View reviewed changes

mathieuchopstm approved these changes Aug 30, 2024

View reviewed changes

teburd approved these changes Aug 30, 2024

View reviewed changes

aescolar added the DNM This PR should not be merged (Do Not Merge) label Sep 2, 2024

lyakh removed the DNM This PR should not be merged (Do Not Merge) label Sep 2, 2024

lyakh added a commit to lyakh/sof that referenced this pull request Sep 2, 2024

test Zephyr PR 77473

4bc7ca5

Test zephyrproject-rtos/zephyr#77473 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

nashif merged commit dd50ff5 into zephyrproject-rtos:main Sep 2, 2024
26 of 27 checks passed

lyakh deleted the depend branch September 3, 2024 05:59

pillo79 mentioned this pull request Sep 3, 2024

llext: add support for init arrays - revised #77641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLEXT dependencies: add support for linking between LLEXT objects #77473

LLEXT dependencies: add support for linking between LLEXT objects #77473

lyakh commented Aug 23, 2024 •

edited

Loading

faxe1008 commented Aug 23, 2024

teburd left a comment

teburd Aug 23, 2024

mathieuchopstm left a comment

mathieuchopstm Aug 23, 2024

pillo79 left a comment

pillo79 left a comment

pillo79 Aug 30, 2024

mathieuchopstm left a comment

lyakh commented Aug 30, 2024

teburd left a comment

teburd Aug 30, 2024

aescolar commented Sep 2, 2024 •

edited

Loading

lyakh commented Sep 2, 2024 •

edited

Loading

LLEXT dependencies: add support for linking between LLEXT objects #77473

LLEXT dependencies: add support for linking between LLEXT objects #77473

Conversation

lyakh commented Aug 23, 2024 • edited Loading

faxe1008 commented Aug 23, 2024

teburd left a comment

Choose a reason for hiding this comment

teburd Aug 23, 2024

Choose a reason for hiding this comment

mathieuchopstm left a comment

Choose a reason for hiding this comment

mathieuchopstm Aug 23, 2024

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 Aug 30, 2024

Choose a reason for hiding this comment

mathieuchopstm left a comment

Choose a reason for hiding this comment

lyakh commented Aug 30, 2024

teburd left a comment

Choose a reason for hiding this comment

teburd Aug 30, 2024

Choose a reason for hiding this comment

aescolar commented Sep 2, 2024 • edited Loading

lyakh commented Sep 2, 2024 • edited Loading

lyakh commented Aug 23, 2024 •

edited

Loading

aescolar commented Sep 2, 2024 •

edited

Loading

lyakh commented Sep 2, 2024 •

edited

Loading