llext: support section alignment requirements #85175

pillo79 · 2025-02-04T17:46:05Z

This PR adds support in LLEXT for symbol / section alignment requirements.

Symbols that require precise alignment will cause the compiler to generate proper padding inside each section so that they are properly placed; also, the section itself will be flagged to be aligned to the largest alignment requirement of any symbol in the section. The ELF spec also guarantees that symbol alignment is respected in the file offsets, so a suitably placed ELF file could be directly used.

When LLEXT maps similar sections into a contiguous region, each section's alignment is checked to ensure offsets and alignments are coherent, so that every section will be properly allocated once the region is fully defined.
Special care is taken when a section is appended to a region which starts at an address that does not match the current aligment requirements. In that case, the region start must be moved backwards to satisfy the new, stricter alignment requirements, and this may result in false positives in the overlap detection.
Finally, memory is allocated also taking into account the existing MMU/MPU requirements, if applicable. When the ELF file buffer can be directly reused, the section addresses are simply checked for compliance.

Review notes

The first 3 commits are preparatory refactors that introduce no functional changes (except for some log messages), but minimize the changes in the following commits.

subsys/llext/llext_mem.c

pillo79 · 2025-02-06T12:01:41Z

v2:

fixed a bug in the overlap checks refactor that made the test ignore partial overlaps - previous behavior restored
applied suggestion from @lyakh and used a direct calculation for the required section size/alignment in the CONFIG_ARM_MPU case
reworded a message that was suggesting a workaround was always possible (... it's not in the SOF pre_located case)

pillo79 · 2025-02-07T08:46:16Z

DNM since an issue was identified during SOF tests and a separate fix needs to be pushed before this could be merged.

teburd · 2025-02-07T14:27:26Z

If SoF is using llext in some manner that isn't tested by Zephyr's testing we are hopelessly going to be falling into the trap of breaking SoF.

This idea that any llext contributor needs to submit patches to SoF and test there first isn't workable long term.

teburd

Some minor comments around macro usage here, but the changes to me look great. I'm assuming this has been tested on NXP hardware as well where the MPU isn't quite like the ARM MPU? E.g. kinetis parts. I found issues here previously that should be verified.

subsys/llext/llext_load.c

pillo79 · 2025-02-26T16:46:37Z

SOF has now been converted to access extension layout details via the inspection API in thesofproject/sof#9831. This new push adjusts those APIs to compensate for the alignment changes, and so should not have any adverse effect on it. I have ran the SOF CI and it looks okay to me, but I'm holding my breath for a more competent analysis 😉

Will wait for other pending LLEXT PRs to be merged before rebasing this a final time and re-running both CIs anyway.

lyakh · 2025-03-05T11:58:36Z

subsys/llext/llext_mem.c

+
+		block_size = 1 << LOG2CEIL(block_size); /* align to next power of two */
+		region_alloc = block_size;
+		region_align = block_size;


I see that it's copied, but why would we need alignment == size? If we need some 16 pages of memory, we also align it at the same border? Does anybody really need it?

This is the way ARM's MPU works, the MPU regions need to be aligned based on the region size. https://developer.arm.com/documentation/dui0552/a/cortex-m3-peripherals/optional-memory-protection-unit/mpu-region-base-address-register?lang=en

There's no fixed page size like with an MMU. The size of the region determines the alignment requirements.

The ADDR field is bits[31:N] of the MPU_RBAR. The region size, as specified by the SIZE field in the MPU_RASR, defines the value of N:

N = Log2(Region size in bytes)

Meaning the alignment of the region is some multiple of the region size.

Yep, it simplifies logic at the hardware level, because if align == size then you can look up matches in the MPU tables via very quick bit mask operations instead of full-on number comparators.

just to be clear, there's an ifdef here to deal with page vs mpu region alignment

This is the way ARM's MPU works, the MPU regions need to be aligned based on the region size. https://developer.arm.com/documentation/dui0552/a/cortex-m3-peripherals/optional-memory-protection-unit/mpu-region-base-address-register?lang=en

thanks, didn't know that

lyakh · 2025-03-05T13:04:50Z

subsys/llext/llext_load.c

+
+				if (ldr->hdr.e_type == ET_DYN) {
+					address -= prepad;
+				}


I think it would look a bit clearer as

if (ldr->hdr.e_type == ET_DYN) { if (prepad > address) { ... } address -= prepad; }

Also could you maybe add a comment to explain, why address doesn't need to be adjusted for ET_REL?

Sure, valid either way, my rational was to split the safety check from the actual update operation.

lyakh · 2025-03-05T13:12:11Z

subsys/llext/llext_load.c

@@ -368,6 +369,38 @@ static int llext_map_sections(struct llext_loader *ldr, struct llext *ext,
 			size_t top_ofs = MAX(region->sh_offset + region->sh_size,
 					     shdr->sh_offset + shdr->sh_size);


btw, would it be possible to move address, bot_ofs, top_ofs outside of the loop, calculate them internally and only assign to respective region fields once after the loop?
BTW # 2: looking at this function, could we place continue under the if under the memcpy() in line 316 and then reduce indentation of this large else block?

Thanks for the suggestions! I implemented the second. The first one would require additional array variables in the stack to store the intermediate results (sections headers in that loop are not ordered by region), and I think the current layout is easier to read.

lyakh · 2025-03-05T13:13:55Z

subsys/llext/llext_mem.c

-				ext->mem_on_heap[mem_idx] = false;
-				return 0;
+				if (!IS_ALIGNED(ext->mem[mem_idx], region_align) &&
+				    !(ldr_parm && ldr_parm->pre_located)) {


you could revert the condition and remove the else because now the if would contain a return

lyakh · 2025-03-05T13:50:50Z

subsys/llext/llext_load.c

@@ -398,6 +405,7 @@ static int llext_map_sections(struct llext_loader *ldr, struct llext *ext,
 					address -= prepad;
 				}
 				bot_ofs -= prepad;
+				region->sh_info = prepad;


maybe a single line comment here too, referring to the detailed explanation above

Add a 'continue' statement when the first section of a region is detected, to avoid unnecessary indentation. No functional change intended. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This commit performs a number of refactors on the LLEXT memory allocation routine: * uses 'region' instead of 'section' in the function name and in the internal variables to better reflect their purpose; * replaces 'ldr->sects[mem_idx]' with 'region' to improve readability; * defines 'align' and 'alloc' variables once, adjusting them in the following code; * swaps the condition, converts it to an 'if(IS_ENABLED(...))' and uses a direct calculation for the size to make the code more readable. No functional change is introduced in this commit. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Display the section names when relocations refer to sections. Also, use a clearer phrasing for relocation info log entries. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Rewrite the overlap checks in llext_map_sections() to use a pair of macros to calculate the top and bottom of a region. This makes the code more readable and less error-prone. No functional change is introduced in this commit apart from a log message format change. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Since commit 0aa6b1c, the 'ldr_parm' pointer is guaranteed to be valid inside all the functions of the llext_load() call tree. This commit fixes the only exception of llext_copy_strings(), which was not passed the 'ldr_parm pointer', and remove the redundant checks. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

This patch adds support for alignment requirements for sections in the loader. The alignment requirements are taken from the ELF file and checked when the section is copied to memory. The section's offset is also checked for alignment when it is mapped to a memory region. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Several operations in the LLEXT module were not taking into account the alignment padding now added to the start of each region, which could lead to incorrect results when referring to the memory regions. - The loading code would read the padding bytes from the ELF file as part of the section data, instead of zeroing them. - The overlap checks in the section mapping code need to ignore the padding to avoid false positives when checking for overlaps. - The inspect API did not adjust the returned region address, region size and section offsets to compensate for the padding, resulting in regions and sections larger than the actual specified memory area. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

Add a test case for the alignment support in loadable extensions. This test case creates a set of constants with specific alignment requirements and verifies that they are placed in memory as expected. Fix the detached section test to use a more standard syntax for the section attribute, avoiding issues with different toolchains. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 · 2025-03-06T11:27:48Z

Update still before final rebase to apply review comments. I also cleaned up lots of ldr_parm != NULL checks, which were now redundant everywhere (but one sneaky exception!) since 0aa6b1c was applied.

pillo79 marked this pull request as ready for review February 5, 2025 12:59

zephyrbot added the area: llext Linkable Loadable Extensions label Feb 5, 2025

zephyrbot requested review from lyakh and teburd February 5, 2025 12:59

zephyrbot assigned teburd Feb 5, 2025

lyakh previously approved these changes Feb 6, 2025

View reviewed changes

subsys/llext/llext_mem.c Outdated Show resolved Hide resolved

pillo79 dismissed lyakh’s stale review via c3d698c February 6, 2025 11:40

pillo79 force-pushed the pr-llext-align branch from 18686b1 to c3d698c Compare February 6, 2025 11:40

pillo79 force-pushed the pr-llext-align branch from c3d698c to f2d631a Compare February 6, 2025 17:01

pillo79 added the DNM This PR should not be merged (Do Not Merge) label Feb 7, 2025

teburd reviewed Feb 7, 2025

View reviewed changes

subsys/llext/llext_load.c Outdated Show resolved Hide resolved

subsys/llext/llext_load.c Outdated Show resolved Hide resolved

pillo79 force-pushed the pr-llext-align branch from f2d631a to 4d2680d Compare February 25, 2025 17:31

teburd previously approved these changes Feb 26, 2025

View reviewed changes

pillo79 mentioned this pull request Feb 27, 2025

[DNM] llext: test new Zephyr alignment PR thesofproject/sof#9860

Draft

lyakh reviewed Mar 5, 2025

View reviewed changes

pillo79 added 8 commits March 6, 2025 09:37

llext: llext_map_sections() cleanups

db99b02

Add a 'continue' statement when the first section of a region is detected, to avoid unnecessary indentation. No functional change intended. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

llext: llext_link() log cleanups

466c1d6

Display the section names when relocations refer to sections. Also, use a clearer phrasing for relocation info log entries. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>

pillo79 dismissed teburd’s stale review via aebdbdd March 6, 2025 11:25

pillo79 force-pushed the pr-llext-align branch from 4d2680d to aebdbdd Compare March 6, 2025 11:25

zephyrbot added the area: Xtensa Xtensa Architecture label Mar 6, 2025

zephyrbot requested review from andyross, ceolin, dcpleung and nashif March 6, 2025 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llext: support section alignment requirements #85175

llext: support section alignment requirements #85175

pillo79 commented Feb 4, 2025 •

edited

Loading

pillo79 commented Feb 6, 2025

pillo79 commented Feb 7, 2025

teburd commented Feb 7, 2025 •

edited

Loading

teburd left a comment

pillo79 commented Feb 26, 2025

lyakh Mar 5, 2025

teburd Mar 5, 2025

pillo79 Mar 5, 2025

teburd Mar 5, 2025

lyakh Mar 6, 2025

lyakh Mar 5, 2025

pillo79 Mar 5, 2025

lyakh Mar 5, 2025

pillo79 Mar 6, 2025

lyakh Mar 5, 2025

lyakh Mar 5, 2025

pillo79 commented Mar 6, 2025 •

edited

Loading

		@@ -368,6 +369,38 @@ static int llext_map_sections(struct llext_loader ldr, struct llext ext,
		size_t top_ofs = MAX(region->sh_offset + region->sh_size,
		shdr->sh_offset + shdr->sh_size);

llext: support section alignment requirements #85175

Are you sure you want to change the base?

llext: support section alignment requirements #85175

Conversation

pillo79 commented Feb 4, 2025 • edited Loading

Review notes

pillo79 commented Feb 6, 2025

pillo79 commented Feb 7, 2025

teburd commented Feb 7, 2025 • edited Loading

teburd left a comment

Choose a reason for hiding this comment

pillo79 commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pillo79 commented Mar 6, 2025 • edited Loading

pillo79 commented Feb 4, 2025 •

edited

Loading

teburd commented Feb 7, 2025 •

edited

Loading

pillo79 commented Mar 6, 2025 •

edited

Loading