llext: use the new inspection API #9831

pillo79 · 2025-02-13T09:50:22Z

This PR tries to improve SOF by minimizing its dependency on LLEXT internals.
Instead of relying on direct field access, the code in llext_manager.c is refactored to use a set of new, stable LLEXT APIs that expose the same information. These code changes should have no measurable effects in the rest of SOF.

LNL test passed as of commit 23478ef.

src/library_manager/llext_manager.c

lyakh · 2025-02-21T08:24:55Z

src/library_manager/llext_manager.c

-				return ret;
-		}
+		ret = memcpy_s((__sparse_force void *)shdr->sh_addr, size - s_offset,
+			       (const uint8_t *)region_addr + s_offset, shdr->sh_size);


we still need the .bss check, don't we?

Actually, that is not needed anymore. Only sections for the requested region will be considered by the check above, so BSS will always be skipped regardless of its placement in memory.

nnno, I think this is breaking SOF. Look at

sof/src/library_manager/llext_manager.c

Lines 150 to 158 in 09f4d94

if ((uintptr_t)bss_addr + bss_size == (uintptr_t)va_base_data &&

!((uintptr_t)bss_addr & (PAGE_SZ - 1))) {

/* .bss directly in front of writable data and properly aligned, prepend */

va_base_data = bss_addr;

data_size += bss_size;

} else if ((uintptr_t)bss_addr == (uintptr_t)va_base_data +

ALIGN_UP(data_size, bss_align)) {

/* .bss directly behind writable data, append */

data_size += bss_size;

. What we do is we merge .bss and .data to map them in one go. We can map memory only in units of pages, so if we were to map them separately we would potentially waste a page of memory. By merging them we avoid that. And we can merge them because they're mapped with the same flags - as writable data. And here I think now that is broken.

Sorry, I do not understand how that is relevant. The changes did not impact the mapping with the vma and size parameters in any way, so while mapping writable data, the same "BSS-and-DATA" area is mapped in one go, as before.

Instead of having to rely only on the address, the copy code now checks the actual region LLEXT mapped each section to. So, yes, previously that check was needed, but now LLEXT will report .bss in LLEXT_MEM_BSS and that loop will definitely ignore it when looking for LLEXT_MEM_DATA.

EDIT: CI is flaky, but the LNL + HDA test passed again. Can you describe a possible failing situation?

@pillo79 I think you're right. The thing is that even now calls to llext_manager_load_data_from_storage() pass two (related but) different "memory descriptors:" (1) memory that has to be mapped and (2) memory, that has to be copied. Before your PR the call for .bss + .data was

sof/src/library_manager/llext_manager.c

Lines 183 to 184 in a895eb2

ret = llext_manager_load_data_from_storage(ext, va_base_data, ext->mem[LLEXT_MEM_DATA],

data_size, SYS_MM_MEM_PERM_RW);

where function arguments 2 and 4 referred to the memory, that had to be mapped, while argument 3 to the memory, that had to be copied. For .rodata and .text those were the same, but not for .bss + .data. After your change this different meanings remain, only parameters change: now argument 3 is for copying and 4 and 5 for mapping. We need a comment about that at least. Let's do this after this PR is merged - either you can submit it or I can do it.

src/library_manager/llext_manager.c

lyakh · 2025-02-21T08:36:01Z

src/library_manager/llext_manager.c

+	if (ret >= 0) {
+		llext_get_section_info(ldr, *llext, ret, &hdr, NULL, NULL);
+		*mod_manifest = llext_peek(ldr, hdr->sh_offset);
+	}


why is this now better than a single call to llext_find_section(llext_find_section()`?

My intention for LLEXT is to ask for deprecation of llext_find_section, as it has a fundamental flaw - it does not provide the section length, so it's basically impossible to use it safely.

The new functions in the inspection API expose all details (when useful to the application) at the same runtime cost, and require the user to provide both ldr and ext parameters, allowing future freedom in Zephyr for internal LLEXT structure management.

src/library_manager/llext_manager.c

pillo79

@lyakh I question the comments on "unneeded changes". Since the same areas are otherwise heavily modified, IMO those tiny clarity improvements will not have any additional impact on other outstanding work.
But that's just my point of view, will of course revert them if you feel they are blockers.

src/library_manager/llext_manager.c

pillo79 · 2025-02-21T08:51:22Z

src/library_manager/llext_manager.c

-				return ret;
-		}
+		ret = memcpy_s((__sparse_force void *)shdr->sh_addr, size - s_offset,
+			       (const uint8_t *)region_addr + s_offset, shdr->sh_size);


Actually, that is not needed anymore. Only sections for the requested region will be considered by the check above, so BSS will always be skipped regardless of its placement in memory.

src/library_manager/llext_manager.c

pillo79 · 2025-02-21T08:58:42Z

src/library_manager/llext_manager.c

+	if (ret >= 0) {
+		llext_get_section_info(ldr, *llext, ret, &hdr, NULL, NULL);
+		*mod_manifest = llext_peek(ldr, hdr->sh_offset);
+	}


My intention for LLEXT is to ask for deprecation of llext_find_section, as it has a fundamental flaw - it does not provide the section length, so it's basically impossible to use it safely.

The new functions in the inspection API expose all details (when useful to the application) at the same runtime cost, and require the user to provide both ldr and ext parameters, allowing future freedom in Zephyr for internal LLEXT structure management.

src/library_manager/llext_manager.c

llext_manager_link() is only interested in the 'llext_loader' part of the 'llext_buf_loader' structure. This patch refactors the code to pass a pointer to the llext_loader directly, making the code more readable. In preparation for the new inspection API, this patch also changes the checks for the buildinfo and mod_manifest pointers to be more explicit. No functional change is intended. Signed-off-by: Luca Burelli <[email protected]>

For inspection API to work, it is imperative to keep the LLEXT loader object available. This patch adds a pointer to the full loader object in the lib_manager_module structure. The loader object is allocated once and never freed. Signed-off-by: Luca Burelli <[email protected]>

pillo79 · 2025-02-21T11:09:59Z

Applied initial notes from @lyakh after a Discord chat.
Making this ready for full review.

lyakh

looks correct, taking it for a short test before approving

lyakh · 2025-02-24T08:37:59Z

src/library_manager/llext_manager.c

-				return ret;
-		}
+		ret = memcpy_s((__sparse_force void *)shdr->sh_addr, size - s_offset,
+			       (const uint8_t *)region_addr + s_offset, shdr->sh_size);


@pillo79 I think you're right. The thing is that even now calls to llext_manager_load_data_from_storage() pass two (related but) different "memory descriptors:" (1) memory that has to be mapped and (2) memory, that has to be copied. Before your PR the call for .bss + .data was

sof/src/library_manager/llext_manager.c

Lines 183 to 184 in a895eb2

ret = llext_manager_load_data_from_storage(ext, va_base_data, ext->mem[LLEXT_MEM_DATA],

data_size, SYS_MM_MEM_PERM_RW);

where function arguments 2 and 4 referred to the memory, that had to be mapped, while argument 3 to the memory, that had to be copied. For .rodata and .text those were the same, but not for .bss + .data. After your change this different meanings remain, only parameters change: now argument 3 is for copying and 4 and 5 for mapping. We need a comment about that at least. Let's do this after this PR is merged - either you can submit it or I can do it.

lyakh

After some further testing turns out, that a bit more work is needed

This patch converts the llext_manager to use the new LLEXT inspection API. The new API allows to get information about sections and regions without the need to access the internal structures of the LLEXT loader, decoupling SOF and LLEXT code and making it easier to maintain. NOTE: Once loaded the first time, the extensions are never unloaded, so the inspection data is also never freed. If this behavior needs to be modified to allow extensions to be fully removed from memory, the inspection data in the loader must be freed as well by calling the llext_free_inspection_data() function before the final llext_unload(). Signed-off-by: Luca Burelli <[email protected]>

lyakh

issue fixed, and thanks for documenting it

pillo79 changed the title ~~Pr llext inspect api~~ llext: use the new inspection API (continued) Feb 13, 2025

pillo79 mentioned this pull request Feb 13, 2025

llext: use the new inspection API #9823

Closed

pillo79 force-pushed the pr-llext-inspect-api branch from 2606529 to 4c8d65c Compare February 13, 2025 10:04

pillo79 mentioned this pull request Feb 13, 2025

llext: add new inspection API zephyrproject-rtos/zephyr#85498

Merged

pillo79 force-pushed the pr-llext-inspect-api branch 3 times, most recently from 21dcc83 to e625dcb Compare February 14, 2025 09:08

pillo79 force-pushed the pr-llext-inspect-api branch 2 times, most recently from c3108a6 to 23478ef Compare February 21, 2025 08:08

pillo79 changed the title ~~llext: use the new inspection API (continued)~~ llext: use the new inspection API Feb 21, 2025

lyakh reviewed Feb 21, 2025

View reviewed changes

pillo79 commented Feb 21, 2025

View reviewed changes

pillo79 added 2 commits February 21, 2025 11:47

pillo79 force-pushed the pr-llext-inspect-api branch from 23478ef to 09f4d94 Compare February 21, 2025 10:53

pillo79 marked this pull request as ready for review February 21, 2025 11:10

pillo79 requested review from lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners February 21, 2025 11:10

lyakh reviewed Feb 24, 2025

View reviewed changes

lyakh requested changes Feb 24, 2025

View reviewed changes

pillo79 force-pushed the pr-llext-inspect-api branch from 09f4d94 to c449256 Compare February 24, 2025 11:51

pillo79 mentioned this pull request Feb 24, 2025

[DNM] better llext inspection API CI test #9853

Closed

lyakh approved these changes Feb 24, 2025

View reviewed changes

lgirdwood approved these changes Feb 24, 2025

View reviewed changes

lgirdwood merged commit 67ebb10 into thesofproject:main Feb 24, 2025
45 of 49 checks passed

pillo79 mentioned this pull request Feb 26, 2025

llext: support section alignment requirements zephyrproject-rtos/zephyr#85175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llext: use the new inspection API #9831

llext: use the new inspection API #9831

pillo79 commented Feb 13, 2025 •

edited

Loading

lyakh Feb 21, 2025

pillo79 Feb 21, 2025 •

edited

Loading

lyakh Feb 21, 2025

pillo79 Feb 21, 2025 •

edited

Loading

lyakh Feb 24, 2025

lyakh Feb 21, 2025

pillo79 Feb 21, 2025

pillo79 left a comment

pillo79 Feb 21, 2025 •

edited

Loading

pillo79 Feb 21, 2025

pillo79 commented Feb 21, 2025

lyakh left a comment

lyakh Feb 24, 2025

lyakh left a comment

lyakh left a comment

	if ((uintptr_t)bss_addr + bss_size == (uintptr_t)va_base_data &&
	!((uintptr_t)bss_addr & (PAGE_SZ - 1))) {
	/* .bss directly in front of writable data and properly aligned, prepend */
	va_base_data = bss_addr;
	data_size += bss_size;
	} else if ((uintptr_t)bss_addr == (uintptr_t)va_base_data +
	ALIGN_UP(data_size, bss_align)) {
	/* .bss directly behind writable data, append */
	data_size += bss_size;

	ret = llext_manager_load_data_from_storage(ext, va_base_data, ext->mem[LLEXT_MEM_DATA],
	data_size, SYS_MM_MEM_PERM_RW);

llext: use the new inspection API #9831

llext: use the new inspection API #9831

Conversation

pillo79 commented Feb 13, 2025 • edited Loading

lyakh Feb 21, 2025

Choose a reason for hiding this comment

pillo79 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

lyakh Feb 21, 2025

Choose a reason for hiding this comment

pillo79 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

lyakh Feb 24, 2025

Choose a reason for hiding this comment

lyakh Feb 21, 2025

Choose a reason for hiding this comment

pillo79 Feb 21, 2025

Choose a reason for hiding this comment

pillo79 left a comment

Choose a reason for hiding this comment

pillo79 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

pillo79 Feb 21, 2025

Choose a reason for hiding this comment

pillo79 commented Feb 21, 2025

lyakh left a comment

Choose a reason for hiding this comment

lyakh Feb 24, 2025

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

pillo79 commented Feb 13, 2025 •

edited

Loading

pillo79 Feb 21, 2025 •

edited

Loading

pillo79 Feb 21, 2025 •

edited

Loading

pillo79 Feb 21, 2025 •

edited

Loading