LLEXT: clean data and code segmentation #9190
Conversation
Re-group headers more logically in eq_iir.c and mixin_mixout.c. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When dynamically mapping memory, we need to update access flags according to the type of the mapping. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When an LLEXT module contains multiple Module Adapter instances, their manifests are stored in an array in the .module section. Those array entries contain per-instance information like module entry points, names, UUIDs, but ELF information is common for all instances. Store it in the first array entry to avoid confusion. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
singalsu
as code owners
|
this also fixes the breakage caused by zephyrproject-rtos/zephyr#73571 and this builds with both Zephyr versions - before and after that change |
|
Before updating this PR to fix sparse warnings, just to record the present CI result: all clean apart from sparse (to be fixed) and jenkins on-device tests. The letter on MTL only had one device unavailable and the other two green, on LNL the result is strange, but must be unrelated https://sof-ci.01.org/sofpr/PR9190/build5052/devicetest/index.html - is this a known issue? |
lyakh
force-pushed
the
llext-segment
branch
2 times, most recently
from
8e1570b to
2ed4f79
Compare
llext_find_section() returns a negative error code when it cannot find the requested section, not 0. Fix error checking. Reported-by: Luca Burelli <l.burelli@arduino.cc> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Looks good except one thing in the second-last patch in the series, please see inline.
src/library_manager/llext_manager.c
Outdated
|
|
||
| /* Writable data (.data, .bss, etc.) */ | ||
| void __sparse_cache *va_base_data = (void __sparse_cache *) | ||
| ctx->segment[LIB_MANAGER_RODATA].addr; |
There was a problem hiding this comment.
@lyakh Shouldn't this be LIB_MANAGER_DATA ? va_base_rodata already set above.
There was a problem hiding this comment.
@kv2019i oops, thanks for catching! The clean-up path - easy to miss without wide-scale automated testing...
| static int llext_manager_update_flags(void __sparse_cache *vma, size_t size, uint32_t flags) | ||
| { | ||
| size_t pre_pad_size = (uintptr_t)vma & (PAGE_SZ - 1); | ||
| void *aligned_vma = (__sparse_force uint8_t *)vma - pre_pad_size; |
There was a problem hiding this comment.
@softwarecki you could use it, but I also need pre_pad_size, so I open code it.
| ((uintptr_t)bss_addr + bss_size <= (uintptr_t)va_base_data || | ||
| (uintptr_t)bss_addr >= (uintptr_t)va_base_data + data_size)) { | ||
| if ((uintptr_t)bss_addr + bss_size == (uintptr_t)va_base_data && | ||
| !((uintptr_t)bss_addr & (PAGE_SZ - 1))) { |
There was a problem hiding this comment.
I don't need to align, I check if it is aligned. So I could use IS_ALIGNED() but it's generic, using division, whereas here it's a power of 2, so a faster masking can be used.
There was a problem hiding this comment.
So you can use IS_ALIGNED
There was a problem hiding this comment.
I could, but I don't want to use division where masking works too.
src/library_manager/llext_manager.c
Outdated
| void __sparse_cache *va_base = llext_manager_get_bss_address(module_id, mod); | ||
| ret = llext_manager_align_unmap(va_base_data, data_size); | ||
| if (ret < 0) | ||
| return ret; |
There was a problem hiding this comment.
Shouldn't we try to unmap the rodata even if unmapping the data fails?
There was a problem hiding this comment.
@softwarecki ok, this actually might make sense. Will update, thanks!
We map memory pages for loaded modules dynamically and we're able to set memory flags for write access or for code execution. This commit takes advantage of the recently added section grouping and maps the three module parts separately: executable code, read-only data and writable data, including zero-initialised .bss. This also cleans up references, pointing into module storage in IMR instead of the mapped addresses. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Zephyr places .bss into a separate section element, still if it's immediately adjacent to writable data we can merge and allocate them together. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
CI failures:
|
yes, that's issue thesofproject/linux#4823. Clearly not related to the LLEXT work... |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Looks like all comments are addressed. @softwarecki ok for you now ?
5 participants
Cleanly separate LLEXT code and data segments, handle .bss together with .data. Using Zephyr's automatic section grouping, based on feature flags.