-
Notifications
You must be signed in to change notification settings - Fork 8.4k
llext: explicitly define loader storage types #81016
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
Changes from all commits
9249e32
b3cfc01
8b75a7f
6dd7974
f4f67eb
e55e2d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,17 @@ Loading an extension | |
| An extension may be loaded using any implementation of a :c:struct:`llext_loader` | ||
| which has a set of function pointers that provide the necessary functionality | ||
| to read the ELF data. A loader also provides some minimal context (memory) | ||
| needed by the :c:func:`llext_load` function. An implementation over a buffer | ||
| containing an ELF in addressable memory in memory is available as | ||
| :c:struct:`llext_buf_loader`. | ||
| needed by the :c:func:`llext_load` function. Several loaders are already provided: | ||
|
|
||
| * An implementation over a buffer containing an ELF in addressable memory in | ||
| memory is available as :c:struct:`llext_buf_loader`. To use this kind of | ||
| loader, it is helpful to use one of the :c:macro:`LLEXT_TEMPORARY_BUF_LOADER`, | ||
| :c:macro:`LLEXT_PERSISTENT_BUF_LOADER`, or :c:macro:`LLEXT_WRITABLE_BUF_LOADER` | ||
| macros to tell LLEXT the appropriate type of memory buffer. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we mention the wrappers directly?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. This is introducing LLEXT, so the first thing you need to know is how to start tinkering with it... IMHO they are even more useful than the struct reference 🙂 |
||
|
|
||
| * An implementation that reads data from a file in the filesystem is available | ||
| as the :c:struct:`llext_fs_loader`. The path to the file must be provided | ||
| when creating the loader with the :c:macro:`LLEXT_FS_LOADER` macro. | ||
|
|
||
| The extensions are loaded with a call to the :c:func:`llext_load` function, | ||
| passing in the extension name and the configured loader. Once that completes | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,15 +39,8 @@ struct llext_buf_loader { | |
| int llext_buf_read(struct llext_loader *ldr, void *buf, size_t len); | ||
| int llext_buf_seek(struct llext_loader *ldr, size_t pos); | ||
| void *llext_buf_peek(struct llext_loader *ldr, size_t pos); | ||
| /** @endcond */ | ||
|
|
||
| /** | ||
| * @brief Initializer for an llext_buf_loader structure | ||
| * | ||
| * @param _buf Buffer containing the ELF binary | ||
| * @param _buf_len Buffer length in bytes | ||
| */ | ||
| #define LLEXT_BUF_LOADER(_buf, _buf_len) \ | ||
| #define Z_LLEXT_BUF_LOADER(_buf, _buf_len, _storage) \ | ||
| { \ | ||
| .loader = \ | ||
| { \ | ||
|
|
@@ -56,11 +49,72 @@ void *llext_buf_peek(struct llext_loader *ldr, size_t pos); | |
| .seek = llext_buf_seek, \ | ||
| .peek = llext_buf_peek, \ | ||
| .finalize = NULL, \ | ||
| .storage = _storage, \ | ||
| }, \ | ||
| .buf = (_buf), \ | ||
| .len = (_buf_len), \ | ||
| .pos = 0, \ | ||
| } | ||
| /** @endcond */ | ||
|
|
||
| /** | ||
| * @brief Initializer for an llext_buf_loader structure | ||
| * | ||
| * The storage type for the provided buffer depends on the value of the | ||
| * @kconfig{CONFIG_LLEXT_STORAGE_WRITABLE} option: if it is defined, the | ||
| * buffer is assumed to be writable; otherwise it is assumed to be persistent. | ||
| * | ||
| * Consider using one of the alternative macros instead. | ||
| * | ||
| * @see LLEXT_TEMPORARY_BUF_LOADER | ||
| * @see LLEXT_PERSISTENT_BUF_LOADER | ||
| * @see LLEXT_WRITABLE_BUF_LOADER | ||
| * | ||
| * @param _buf Buffer containing the ELF binary | ||
| * @param _buf_len Buffer length in bytes | ||
| */ | ||
| #define LLEXT_BUF_LOADER(_buf, _buf_len) \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor of deprecating this given the variants are now available at construction time
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would love to, but it's a little tricky as it is used everywhere in the test suite (and TBH it really works great there!). |
||
| Z_LLEXT_BUF_LOADER(_buf, _buf_len, \ | ||
| IS_ENABLED(CONFIG_LLEXT_STORAGE_WRITABLE) ? \ | ||
| LLEXT_STORAGE_WRITABLE : LLEXT_STORAGE_PERSISTENT) | ||
|
|
||
| /* @brief Initialize an llext_buf_loader structure for a temporary buffer | ||
| * | ||
| * ELF data from the specified buffer can only be used during llext_load(). | ||
| * The LLEXT subsystem will copy all necessary data to internal buffers at load | ||
| * time. | ||
| * | ||
| * @param _buf Buffer containing the ELF binary | ||
| * @param _buf_len Buffer length in bytes | ||
| */ | ||
| #define LLEXT_TEMPORARY_BUF_LOADER(_buf, _buf_len) \ | ||
| Z_LLEXT_BUF_LOADER(_buf, _buf_len, LLEXT_STORAGE_TEMPORARY) | ||
|
|
||
| /** | ||
| * @brief Initialize an llext_buf_loader structure for a persistent, read-only buffer | ||
| * | ||
| * ELF data from the specified buffer is guaranteed to be accessible for as | ||
| * long as the extension is loaded. The LLEXT subsystem may directly access the | ||
| * ELF data, as long as no modification is required during loading. | ||
| * | ||
| * @param _buf Buffer containing the ELF binary | ||
| * @param _buf_len Buffer length in bytes | ||
| */ | ||
| #define LLEXT_PERSISTENT_BUF_LOADER(_buf, _buf_len) \ | ||
| Z_LLEXT_BUF_LOADER(_buf, _buf_len, LLEXT_STORAGE_PERSISTENT) | ||
|
|
||
| /** | ||
| * @brief Initialize an llext_buf_loader structure for a persistent, writable buffer | ||
| * | ||
| * ELF data from the specified buffer is guaranteed to be accessible for as | ||
| * long as the extension is loaded. The LLEXT subsystem may directly access and | ||
| * modify the ELF data. | ||
| * | ||
| * @param _buf Buffer containing the ELF binary | ||
| * @param _buf_len Buffer length in bytes | ||
| */ | ||
| #define LLEXT_WRITABLE_BUF_LOADER(_buf, _buf_len) \ | ||
| Z_LLEXT_BUF_LOADER(_buf, _buf_len, LLEXT_STORAGE_WRITABLE) | ||
|
|
||
| /** | ||
| * @} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,44 @@ extern "C" { | |
| struct llext_elf_sect_map; /* defined in llext_priv.h */ | ||
| /** @endcond */ | ||
|
|
||
| /** | ||
| * @brief Storage type for the ELF data to be loaded. | ||
| * | ||
| * This enum defines the storage type of the ELF data that will be loaded. The | ||
| * storage type determines which memory optimizations can be tried by the LLEXT | ||
| * subsystem during the load process. | ||
| * | ||
| * @note Even with the most permissive option, LLEXT might still have to copy | ||
| * ELF data into a separate memory region to comply with other restrictions, | ||
| * such as hardware memory protection and/or alignment rules. | ||
| * Sections such as BSS that have no space in the file will also be allocated | ||
| * in the LLEXT heap. | ||
| */ | ||
| enum llext_storage_type { | ||
| /** | ||
| * ELF data is only available during llext_load(); even if the loader | ||
| * supports directly accessing the memory via llext_peek(), the buffer | ||
| * contents will be discarded afterwards. | ||
| * LLEXT will allocate copies of all required data into its heap. | ||
| */ | ||
| LLEXT_STORAGE_TEMPORARY, | ||
| /** | ||
| * ELF data is stored in a *read-only* buffer that is guaranteed to be | ||
| * always accessible for as long as the extension is loaded. LLEXT may | ||
| * directly reuse constant data from the buffer, but may still allocate | ||
| * copies if relocations need to be applied. | ||
| */ | ||
| LLEXT_STORAGE_PERSISTENT, | ||
| /** | ||
| * ELF data is stored in a *writable* memory buffer that is guaranteed | ||
| * to be always accessible for as long as the extension is loaded. | ||
| * LLEXT may freely modify and reuse data in the buffer; so, after the | ||
| * extension is unloaded, the contents should be re-initialized before | ||
| * a subsequent llext_load() call. | ||
| */ | ||
| LLEXT_STORAGE_WRITABLE, | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Linkable loadable extension loader context | ||
| * | ||
|
|
@@ -95,6 +133,11 @@ struct llext_loader { | |
| */ | ||
| void (*finalize)(struct llext_loader *ldr); | ||
|
|
||
| /** | ||
| * @brief Storage type of the underlying data accessed by this loader. | ||
| */ | ||
| enum llext_storage_type storage; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me this should rather go to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed in Discord, IMO the way the data is handled must be strictly tied to the loader. Consider the case of If you move this field into
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pillo79 wouldn't it be possible for the user to select a wrong macro with your approach too? Also we can define several macros for the parameters to extend
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, the default initializer for
The whole point of this PR is to unambiguously state the storage type of the data exposed by each loader. Moving this to an optional struct defeats the purpose IMO (and my question above would still apply).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@pillo79 ok, I rest my case :-) The point - as you say - seems to be, that loader parameters are optional and we really want an explicit selection of a loader type to be compulsory. Looks a bit confusing IMHO to have "parameters" in two separate locations, but if others are fine with this - I can live with this too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is the right spot for this. It's an attribute of the loader type. Imagine a network loader, usb loader, uart loader, etc. We want to know as part of the loader API this attribute. Making this const perhaps helps signal that its a non-mutable attribute? Like struct device we could also split this struct up into its immutable parts (this enum and the function pointers) and the mutable parts (hdrs/sects/sect_map) which may further help differentiate the intent as well as how they get placed into an ELF image themselves (ROM vs RAM)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense, although it should apply to all function pointers as well. But that last part will wait for the more comprehensive splitting of const/non-const fields which I think is also useful. |
||
|
|
||
| /** @cond ignore */ | ||
| elf_ehdr_t hdr; | ||
| elf_shdr_t sects[LLEXT_MEM_COUNT]; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,9 +299,14 @@ static void llext_link_plt(struct llext_loader *ldr, struct llext *ext, elf_shdr | |
| * beginning of the .text section in the ELF file can be | ||
| * applied to the memory location of mem[LLEXT_MEM_TEXT]. | ||
| * | ||
| * This is valid only when CONFIG_LLEXT_STORAGE_WRITABLE=y | ||
| * and peek() is usable on the source ELF file. | ||
| * This is valid only for LLEXT_STORAGE_WRITABLE loaders | ||
| * since the buffer will be directly modified. | ||
| */ | ||
| if (ldr->storage != LLEXT_STORAGE_WRITABLE) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be great to have a nice table somewhere of what the effects of linkages (relocatable vs dynamic) and -fPIC/-fPIE do to the ELF in terms of where relocations are needed. Like you noted in the PR, it'd be great to support a XIP setup but if this requires rewriting the elf sections into a new on-flash image that's kind of painful. I know at EOSS one idea that came up was having something that would likely be useful for SOF as well which is once again seperating linking and loading, so you could have like a usb mass storage type thing taking images, linking and storing on flash for XIP usage for example.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't that also platform- or even toolchain-specific? It would be great to have fully documented what sections are created for each of those build types but it certainly is well out of scope of this PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed I'd love to read up some kind of documentation on that as well 😁
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is! I follow the wild linker development as it’s neat, and have used mold, gnu ld, and Linux’s ELF loading code as reference but the lack of information in one spot is a sore point. |
||
| LOG_ERR("PLT: cannot link read-only ELF file"); | ||
| continue; | ||
| } | ||
|
|
||
| uint8_t *rel_addr = (uint8_t *)ext->mem[LLEXT_MEM_TEXT] - | ||
| ldr->sects[LLEXT_MEM_TEXT].sh_offset; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,11 +494,20 @@ static int llext_map_sections(struct llext_loader *ldr, struct llext *ext, | |
| /* | ||
| * Calculate each ELF section's offset inside its memory region. This | ||
| * is done as a separate pass so the final regions are already defined. | ||
| * Also mark the regions that include relocation targets. | ||
| */ | ||
| for (i = 0; i < ext->sect_cnt; ++i) { | ||
| elf_shdr_t *shdr = ext->sect_hdrs + i; | ||
| enum llext_mem mem_idx = ldr->sect_map[i].mem_idx; | ||
|
|
||
| if (shdr->sh_type == SHT_REL || shdr->sh_type == SHT_RELA) { | ||
| enum llext_mem target_region = ldr->sect_map[shdr->sh_info].mem_idx; | ||
|
|
||
| if (target_region != LLEXT_MEM_COUNT) { | ||
| ldr->sects[target_region].sh_flags |= SHF_LLEXT_HAS_RELOCS; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sh_flags today is mostly there to store the ELF flags. Would it be too disasterous to do something like ldr->sect_has_relocs[target_region] = true;instead here? you could even instead set the relocation section if you like... ldr->sect_to_rel_sect[target_region] = i;which might be useful, using UINT32_MAX or something to store "no relocation section"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chose some unused ELF bits to avoid another allocation, but I like the idea and will try to refactor that as well 🙂 |
||
| } | ||
| } | ||
|
|
||
| if (mem_idx != LLEXT_MEM_COUNT) { | ||
| ldr->sect_map[i].offset = shdr->sh_offset - ldr->sects[mem_idx].sh_offset; | ||
| } | ||
|
|
@@ -846,8 +855,8 @@ int do_llext_load(struct llext_loader *ldr, struct llext *ext, | |
| ext->exp_tab.sym_cnt = 0; | ||
| ext->exp_tab.syms = NULL; | ||
| } else { | ||
| LOG_DBG("loaded module, .text at %p, .rodata at %p", ext->mem[LLEXT_MEM_TEXT], | ||
| ext->mem[LLEXT_MEM_RODATA]); | ||
| LOG_DBG("Loaded llext: %zu bytes in heap, .text at %p, .rodata at %p", | ||
| ext->alloc_size, ext->mem[LLEXT_MEM_TEXT], ext->mem[LLEXT_MEM_RODATA]); | ||
| } | ||
|
|
||
| llext_finalize(ldr); | ||
|
|
||
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 typo here, did you mean "if?"
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.
Reused the text without looking, thanks for spotting! 👍