-
Notifications
You must be signed in to change notification settings - Fork 321
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
1st part of llext #8180 #8759
1st part of llext #8180 #8759
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Lot of good changes, thanks for splitting these out. One question about the config define.
src/include/sof/lib_manager.h
Outdated
@@ -81,7 +81,9 @@ struct ipc_lib_msg { | |||
struct ext_library { | |||
struct k_spinlock lock; /* last locking CPU record */ | |||
struct sof_man_fw_desc *desc[LIB_MANAGER_MAX_LIBS]; | |||
#ifdef __SOF_MODULE_SERVICE_BUILD__ |
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.
This is first time this define is used in basefw build (this is defined when building modules with lmdk but not set for base fw build). I think this has to be a Kconfig for the main build to select loadable module behaviour and/or support both.
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.
@kv2019i right, yes, I agree that ideally we should be able to build firmware that can load all kinds of modules: IADK, native system-services, and native dynamic. But initially I'd integrate all three approaches into the firmware and then check how we can identify modules at run-time
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.
in fact indeed it isn't correct. -D__SOF_MODULE_SERVICE_BUILD__=1
is only used to build the module itself, not the complete firmware. Converted to draft for a fix
88e1418
to
d6b2028
Compare
cb81f84
to
bd9a877
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.
Thanks, looks good now!
src/library_manager/lib_manager.c
Outdated
@@ -47,6 +47,42 @@ struct lib_manager_dma_ext { | |||
|
|||
static struct ext_library loader_ext_lib; | |||
|
|||
struct lib_manager_mod_ctx { | |||
struct sof_man_fw_desc *desc; | |||
size_t segment_size[3]; |
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.
Minor: The magic 3 sticks out a but, some SOF_MAN_SEGMENT_COUNT ?
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.
@kv2019i well, here's how they're defined
sof/tools/rimage/src/include/rimage/sof/user/manifest.h
Lines 47 to 51 in d324af5
#define SOF_MAN_SEGMENT_TEXT 0 | |
#define SOF_MAN_SEGMENT_RODATA 1 | |
#define SOF_MAN_SEGMENT_DATA 1 | |
#define SOF_MAN_SEGMENT_BSS 2 | |
#define SOF_MAN_SEGMENT_EMPTY 15 |
struct sof_man_segment_desc segment[3]; |
5afff19
to
1fbc349
Compare
src/library_manager/lib_manager.c
Outdated
size_t st_rodata_size = mod->segment[SOF_MAN_SEGMENT_RODATA].flags.r.length; | ||
void *src_rodata = (void *)(load_base + | ||
mod->segment[SOF_MAN_SEGMENT_RODATA].file_offset); | ||
size_t st_rodata_size = ctx->segment_size[SOF_MAN_SEGMENT_RODATA]; | ||
int ret; | ||
|
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.
Page alignment for sections is caused by secirity reasons. TEXT should be read_only while BSS read/write. Additionally if we want to introduce module level access rights, the PAGE size is minimal quantity we can control.
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.
Page alignment for sections is caused by secirity reasons. TEXT should be read_only while BSS read/write. Additionally if we want to introduce module level access rights, the PAGE size is minimal quantity we can control.
@jxstelter this isn't taking away any current capabilities, not forcing any changes onto LMDK / IADK / system-services modules. This merely makes it possible to have, say, .bss start at a non-page-aligned address, but it isn't forcing it. And if .bss or .rodata begin at such an address, anyway page-aligned memory will be allocated for them and any mapping flags will be applied to them.
fd2d2ea
to
ab01601
Compare
SOFCI TEST |
src/library_manager/lib_manager.c
Outdated
if (ret < 0) | ||
goto err; | ||
return ret; |
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.
In error case, we don't want to free the already loaded modules?
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.
@softwarecki It should be freed, yes, but I just moved the code from below. And I think some other PRs will rewrite this anyway. So I just moved it out of the way for now to let those other PRs work on this.
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.
...and yes, I meant #8544 by @softwarecki
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.
In that case, let's not modify the code that will be changed by another PR. There is no point in introducing a merge conflicts in another PR.
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.
@softwarecki I cannot and shall not sit and wait because I know that somebody else's work is in progress. If your PR gets merged first - I'll update mine, if mine is merged first - you'll update yours
src/library_manager/lib_manager.c
Outdated
goto e_text; | ||
|
||
/* Some data can be accessed as uncached, in fact that's the default */ | ||
dcache_writeback_region(va_base_rodata, st_rodata_size); |
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.
Move writeback to lib_manager_load_data_from_storage
function
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.
@softwarecki I'd rather not do that. As you see there's a difference between .text and .data - we need to additionally invalidate instruction cache for .text. If I only move this write-back call into the mapping function, it will become less logical - for .text the writeback will be inside the function and the icache invalidation will be outside
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.
Why do you moved a call to dcache_writeback_region
from the lib_manager_load_data_from_storage
to this function? i'm ok with icache.
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.
@softwarecki to make it logical - lib_manager_load_data_from_storage()
allocates memory and copies whatever data it's asked to handle into it, it doesn't know cache requirements of that data, that is handled by the caller, because the caller knows what data that is. Alternatively we'd need to add some kind of flags to the function to let it know what cache handling it has to perform
src/library_manager/lib_manager.c
Outdated
ctx->segment_size[i] = mod->segment[i].flags.r.length * PAGE_SZ; | ||
|
||
if (i == SOF_MAN_SEGMENT_BSS && mod->type.load_type != SOF_MAN_MOD_TYPE_LLEXT) | ||
ctx->segment_size[i] /= mod->instance_max_count; |
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.
I would like move this if to lib_manager_allocate_module_instance
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.
@softwarecki I guess this could be done, and it wouldn't affect the LLEXT flow anyway, but I think it's more robust to have final sizes in this table and to be able to use them with no further modifications wherever needed?
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.
Is ctx->segment_size calculated value for module or is it size of loaded binary? Can you clarify it?
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.
I would prefer all bss-related calculations to be in one place. In my opinion, the name and location (in ctx) of this array suggests storing values read from the manifest. While reviewing the changes in the lib_manager_allocate_module_instance
function, I was surprised that the division by the number of instances disappeared.
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.
Is ctx->segment_size calculated value for module or is it size of loaded binary? Can you clarify it?
@pjdobrowolski that should be rather obvious from the code, sorry. It's calculated from section sizes of the loaded binary.
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.
I would prefer all bss-related calculations to be in one place. In my opinion, the name and location (in ctx) of this array suggests storing values read from the manifest. While reviewing the changes in the
lib_manager_allocate_module_instance
function, I was surprised that the division by the number of instances disappeared.
@softwarecki currently that division is performed in two locations: in lib_manager_allocate_module_instance()
and in lib_manager_free_module_instance()
, after this change it's done only in one location, I think it's better.
src/library_manager/lib_manager.c
Outdated
icache_invalidate_region(va_base_text, st_text_size); | ||
|
||
/* Copy RODATA */ | ||
ret = lib_manager_load_data_from_storage(va_base_rodata, src_rodata, |
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.
Lack of support for readonly segment and empty data segment.
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.
@softwarecki sorry, is this something that this commit changes? That's how it is now, isn't it?
CI: a suspend-resume test failed on TGL and a pause-resume-capture failed on MTL, none should be related. Most importantly QB tests are successful and they do test system-service module loading, so this PR isn't breaking that |
I have a mixed feeling about that llext. I only can guess that llext means that it is loadable libraries external however I didn't see any draft of documentation whatsoever. Changes themselves are cosmetic or irrelevant from IADK/Native loadable modules perspective and I would like to review that PR properly. For me without any documentation it is no-go. |
src/library_manager/lib_manager.c
Outdated
ctx->segment_size[i] = mod->segment[i].flags.r.length * PAGE_SZ; | ||
|
||
if (i == SOF_MAN_SEGMENT_BSS && mod->type.load_type != SOF_MAN_MOD_TYPE_LLEXT) | ||
ctx->segment_size[i] /= mod->instance_max_count; |
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.
Is ctx->segment_size calculated value for module or is it size of loaded binary? Can you clarify it?
src/library_manager/lib_manager.c
Outdated
struct sof_man_fw_desc *lib_manager_get_library_module_desc(int module_id) | ||
{ | ||
struct lib_manager_mod_ctx *ctx = lib_manager_get_mod_ctx(module_id); | ||
uint8_t *buffptr = (uint8_t *)(ctx ? ctx->desc : NULL); |
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.
superfluous double checks. Why not just if (!ctx) return NULL;
?
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.
@softwarecki I wouldn't really call this superfluous. An alternative would look like
if (!ctx || !ctx->desc)
return NULL;
uint8_t *buffptr = ctx->desc;
so you anyway have to check up to two conditions - just as well as now. I'd say it's a matter of preference
src/library_manager/lib_manager.c
Outdated
goto e_text; | ||
|
||
/* Some data can be accessed as uncached, in fact that's the default */ | ||
dcache_writeback_region(va_base_rodata, st_rodata_size); |
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.
Why do you moved a call to dcache_writeback_region
from the lib_manager_load_data_from_storage
to this function? i'm ok with icache.
src/library_manager/lib_manager.c
Outdated
ctx->segment_size[i] = mod->segment[i].flags.r.length * PAGE_SZ; | ||
|
||
if (i == SOF_MAN_SEGMENT_BSS && mod->type.load_type != SOF_MAN_MOD_TYPE_LLEXT) | ||
ctx->segment_size[i] /= mod->instance_max_count; |
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.
I would prefer all bss-related calculations to be in one place. In my opinion, the name and location (in ctx) of this array suggests storing values read from the manifest. While reviewing the changes in the lib_manager_allocate_module_instance
function, I was surprised that the division by the number of instances disappeared.
https://docs.zephyrproject.org/latest/services/llext/index.html |
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.
Minor comments from a completely clueless reviewer...
@@ -25,6 +25,7 @@ | |||
/* module type load type */ | |||
#define SOF_MAN_MOD_TYPE_BUILTIN 0 | |||
#define SOF_MAN_MOD_TYPE_MODULE 1 | |||
#define SOF_MAN_MOD_TYPE_LLEXT 2 |
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.
maybe explain what LL and EXT stand for? it's not obvious to me. Low Latency, Loadable Library, External, Extension, Exploding, who knows?
Acronyms are evil.
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.
@plbossart yeah, could add a comment, but it's really just a Zephyr API. I complained about the name, proposed "plugin," "applet," and a bunch of other names, but they wanted "llext"
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.
this conversation made me curious, what is llext stands for? ;) @lyakh
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 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.
But it's a fair point this file has nothing to do with Zephyr, so it's not so obvious to figure out this is a term coming from Zephyr.
src/library_manager/lib_manager.c
Outdated
struct ext_library *ext_lib = ext_lib_get(); | ||
|
||
/* There are modules marked as lib_code. This is code shared between several modules inside | ||
* the library. Unload all lib_code modules with last none lib_code module unload. |
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.
@ujfalusi didn't we make the assumption that a library contains ONE module?
IIRC the firmwares are identified with a UUID, not sure how the library is identified in Linux
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.
@plbossart in an earlier version I removed this whole "lib_code" support because, well, it's unused so far and it's unclear if and when and how it would be used. I got complains. So, I keep it as is, just move out to functions
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.
No, the kernel side does not assume anything, it is prepared for the module bundles. The kernel tracks the library itself and then checks the module(s) inside:
b700672e2250 ("ASoC: SOF: Intel/IPC4: Support for external firmware libraries")
The lib_code does not have UUID, but in case a library have multiple modules then there should be symlinks:
module_bundle.bin
<UUID1>.bin -> module_bundle.bin
<UUID2>.bin -> module_bundle.bin
<UUID3>.bin -> module_bundle.bin
The kernel will load a library for a specific module UUID, so multiple UUID pointing to the same library will load that library with all of it's modules once.
src/library_manager/lib_manager.c
Outdated
struct sof_man_module *module_entry = | ||
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0)); | ||
|
||
for (size_t idx = 0; idx < desc->header.num_module_entries; |
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.
why the size_t for an index incremented by one?
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.
@plbossart no idea, not my code, moved it as is from the current implementation
src/library_manager/lib_manager.c
Outdated
@@ -229,7 +250,7 @@ static void __sparse_cache *lib_manager_get_instance_bss_address(uint32_t module | |||
static int lib_manager_allocate_module_instance(uint32_t module_id, uint32_t instance_id, | |||
uint32_t is_pages, struct sof_man_module *mod) | |||
{ | |||
uint32_t bss_size = | |||
size_t bss_size = |
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.
could be a separate patch
It is just explanation what is llext, there is no sofdocs of its implementation in sof. |
@@ -554,9 +619,6 @@ static void __sparse_cache *lib_manager_allocate_store_mem(uint32_t size, | |||
return NULL; | |||
} | |||
|
|||
dcache_invalidate_region(local_add, size); | |||
icache_invalidate_region(local_add, size); | |||
|
|||
return local_add; | |||
} | |||
|
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.
lib_manager.c is slowly hitting 1k lines of code. It is hard to understand what and where is jumping, especially when new functions were introduce. Is it a good time to split lib_manager.c for more readable format?
@pjdobrowolski SOF uses many Zephyr APIs: threads, mutexes, work queues, timers, etc., we don't have sofdocs for each of them. But sure, we can add it as a paragraph in SOF loadable module implementation. In short - we use LLEXT for ELF parsing and linking. Also keep in mind, that this PR is just a preparation, it doesn't use LLEXT yet |
We need to be able to distinguish LLEXT loadable dynamically linkable modules at run-time. Add a new type for them. Compatibility is preserved. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
This forks a slightly earlier version of library loading code for upcoming LLEXT support. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Currently only page size-aligned .text and .rodata sections are supported. Remove this limitation by prepadding the address to restore the alignment. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
.bss splitting is specific to system-service module loading, LLEXT uses a traditional shared .bss ELF section. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Currently segment sizes are calculated from page counts, stored in module manifests. This restricts us to only using page size-aligned segment sizes. In case of not page size-aligned ELF sections this can lead to wasted memory. To avoid this store segment sizes in full-size per-module variables to access them at any time. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Buffers, where modules are stored originally, don't need their caches to be synchronised - they're only used as a data storage and only accessed via cached aliases. But when final buffers are allocated, from which modules are actually run, those buffers do indeed need cache synchronisation. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
A part of mtl.toml.h is also needed for loadable modules, built for MTL. Extract it into a separate file. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Instead of including TOML files for each platform explicitly, add a generic platform.toml, that will contain all respective platform TOML files, and select the correct one, based on build configucation. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
ab01601
to
9978873
Compare
New version. I'm forking lib_manager.c for LLEXT. Sharing it with other loadable module implementations didn't work out. @pjdobrowolski @softwarecki you're personally cordially invited to review this version. |
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.
LGTM - @pjdobrowolski @softwarecki I think all comments are addressed now. Pls review.
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.
Some minor things, but nothing blocking merge. I do wonder how we share #8741 -- that's needed here as well, right?
@@ -25,6 +25,7 @@ | |||
/* module type load type */ | |||
#define SOF_MAN_MOD_TYPE_BUILTIN 0 | |||
#define SOF_MAN_MOD_TYPE_MODULE 1 | |||
#define SOF_MAN_MOD_TYPE_LLEXT 2 |
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.
But it's a fair point this file has nothing to do with Zephyr, so it's not so obvious to figure out this is a term coming from Zephyr.
// Pawel Dobrowolski<pawelx.dobrowolski@intel.com> | ||
|
||
/* | ||
* Dynamic module loading functions. |
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.
So this could be more explict and mention "Dynamic module loading functions using Zephyr Linkable Loadable Extensions (LLEXT) interface".
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.
@kv2019i I propose to address any cosmetic improvements in a follow-up PR, will do one first thing after this is merged
@kv2019i it's not needed for this to work, it's needed for SOF to make sure we check what we run, right? And yes, it should work with this PR too, since auth is added to the IPC copying path, which is shared with system-services |
CI failures: a single known multiple-pause-resume failure https://sof-ci.01.org/sofpr/PR8759/build2309/devicetest/index.html?model=TGLU_RVP_NOCODEC-ipc4&testcase=multiple-pause-resume-50 |
We got the auth bit merged, and now this can go it. @pjdobrowolski @softwarecki you didn't respond the pings this week but Guennadi has now addressed the main concerns and the code is now in separate file, so I assume we are good now. The documentation bit definitely needs to be supported but I think the Zephyr llext docs plus #8180 gives a pretty good view of what this is -- Linux-kernel style loadbale modules support using Zephyr llext mechanism. This won't be for all purposes, but is one option to build modules. |
Comments addressed in 9978873 on Thursday
7 first commits from #8180 not requiring a Zephyr update