llext: Fix wrong literals placement in modules

[serhiy-katsyuba-intel]

Xtensa L32R instruction requires the literal it loads to be located in memory before the L32R address. Such a requirement is usually satisfied by linker script rules which put literals sections before text sections in which they are used. LLEXT module builds do not use a linker script, and so correct placement of literals is ensured by a compilation option.

[serhiy-katsyuba-intel]

This fixes SRC crash, a regression that was triggered by this commit 597e16f. That commit is legitimate, it just accidentally made the .cold.literal section to be located after the .cold section in the SRC module. Since no linker script is used to build LLEXT modules (a Python script generates the linker command line instead), the positions of .cold.literal and .cold sections could be random relative to each other.

Moreover, for LLEXT modules, additional linking is done when the module is loaded. L32R instructions are patched at runtime with relative addresses (distance from the PC) of the literals they use. If a literal is located in memory after an L32R instruction, instead of reporting an error, Zephyr patches that instruction incorrectly, resulting in wrong firmware behavior. In the SRC case, it was a crash. This incorrect L32R patching without reporting any error should be fixed by another PR in Zephyr.

[lgirdwood]

Thanks @serhiy-katsyuba-intel good find.
Adding @teburd as this would need to be fixed at the source in Zephyr (rather that in the application here).

[teburd]

We can add a linker script pass I believe even when building extensions though this requires the linker be willing to produce partially linked ELF files. The gnu linker does this just fine from what I recall. This would let you move things around in the ELF while leaving missing symbols unlinked potentially. Lauren Murphy added something like this I believe here, let me know if this helps you and we can work towards it for xtensa
zephyrproject-rtos/zephyr#93899

[serhiy-katsyuba-intel]

@teburd , runtime patching of L32R instruction is done in Zephyr xtensa_elf_relocate(). It would be nice to extend current implementation with some sanity check for literal address. If literal address is bad (like in our case it was placed after it is used by L32R) xtensa_elf_relocate() should report an error and fail and should not patch L32R incorrectly producing unexpected runtime behaviour.

[teburd]

@teburd , runtime patching of L32R instruction is done in Zephyr xtensa_elf_relocate(). It would be nice to extend current implementation with some sanity check for literal address. If literal address is bad (like in our case it was placed after it is used by L32R) xtensa_elf_relocate() should report an error and fail and should not patch L32R incorrectly producing unexpected runtime behaviour.

So to be clear here a l32r op was updated with an invalid address to load, and this occured because the literals (e.g. 32) weren't placed in the text section by the compiler? Yeah to me the change makes sense to tell the compiler to inline the literals in .text, now if you want them placed separately then we would need to support that section in llext in some manner which we likely don't do today.

[serhiy-katsyuba-intel]

Before this fix, literals were placed into separate section. So we had section .cold with code and section .cold.literals with literals used by the code in .cold. In a produced loadable module ELF .cold.literals was put after .cold. That's a first problem, literals should come before they are used as L32R only supports offsets from -262141 to -4. This commit fixes this first problem by placing literals to same section as the code, so there is no more .cold.literals section and now compiler ensures literals are placed before their L32R.

However, the second problem is that Zephyr xtensa_elf_relocate() does not make any check for literal address it receives and patches L32R instruction even if that produce wrong code. Like in our case, literal was put in memory after L32R it is used, so offset was out of -262141 to -4 range supported by L32R.

I think, it would be nice if xtensa_elf_relocate() will check if supplied literal address conforms to -262141 to -4 offset range and report an error and fail if not. So if we will have in future some bug related to linking and improper sections location that will be properly reported as error instead of silently producing weirdly working code.

[teburd]

Before this fix, literals were placed into separate section. So we had section .cold with code and section .cold.literals with literals used by the code in .cold. In a produced loadable module ELF .cold.literals was put after .cold. That's a first problem, literals should come before they are used as L32R only supports offsets from -262141 to -4. This commit fixes this first problem by placing literals to same section as the code, so there is no more .cold.literals section and now compiler ensures literals are placed before their L32R.

However, the second problem is that Zephyr xtensa_elf_relocate() does not make any check for literal address it receives and patches L32R instruction even if that produce wrong code. Like in our case, literal was put in memory after L32R it is used, so offset was out of -262141 to -4 range supported by L32R.

I think, it would be nice if xtensa_elf_relocate() will check if supplied literal address conforms to -262141 to -4 offset range and report an error and fail if not. So if we will have in future some bug related to linking and improper sections location that will be properly reported as error instead of silently producing weirdly working code.

This is very clear, thank you.

1. Please file an issue in Zephyr to check the relocation is correct for l32r in the arch specific relocate function. Ultimately though the assumption llext makes is correctly constructed ELFs. This ELF was incorrectly constructed if the relocation was wrong. That doesn't mean we shouldn't fail gracefully though, @edersondisouza found some other similar issues for this sort of thing.
2. The ordering of sections is exactly what the PR from @laurenmurphyx86 fixes llext: add a section reordering script zephyrproject-rtos/zephyr#93899 and as noted there it isn't just a problem for you and xtensa but we should clearly be doing some more work to enable linker scripts for this reason.

[lyakh]

The ordering of sections is exactly what the PR from @laurenmurphyx86 fixes

@teburd as discussed privately, previously I tried to use a linker script for SOF LLEXT modules, but it didn't work well enough - different toolchains and different compiler options generate different sections, additionally SOF uses custom sections, so each time the section design changes, the linker script would need to be changed too. Therefore we ended up using a script to generate linker command line parameters. Note, that maybe it was my low expertise with linker scripts that drew me to that choice, or maybe we can generate them dynamically too, or maybe simply the situation now has stabilised sufficiently to make that doable. But at least it would be a rather large (and therefore expensive) change, so I'd try to avoid that if possible.

[serhiy-katsyuba-intel]

1. Please file an issue in Zephyr to check the relocation is correct for l32r in the arch specific relocate function

Created Enhancement issue for Zephyr: zephyrproject-rtos/zephyr#96210

[teburd]

The ordering of sections is exactly what the PR from @laurenmurphyx86 fixes

@teburd as discussed privately, previously I tried to use a linker script for SOF LLEXT modules, but it didn't work well enough - different toolchains and different compiler options generate different sections, additionally SOF uses custom sections, so each time the section design changes, the linker script would need to be changed too. Therefore we ended up using a script to generate linker command line parameters. Note, that maybe it was my low expertise with linker scripts that drew me to that choice, or maybe we can generate them dynamically too, or maybe simply the situation now has stabilised sufficiently to make that doable. But at least it would be a rather large (and therefore expensive) change, so I'd try to avoid that if possible.

Well if you need specific ordering, placement, padding, etc then the linker scripts are the tool to do this. I too always struggle with them when I first hack one up again, because I don't do it very often. But mostly.. the rules are simple enough once you need to relearn it yet again. Like you I tend to avoid dealing with them when possible. But... in this case its really sounded like the right tool for the job.