-
Notifications
You must be signed in to change notification settings - Fork 321
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
loadable libraries and smart-amp-test as an example #8180
Changes from all commits
1b2c9e3
a861ae4
ec0038e
76edab3
387bd4f
a2fd06b
19dee61
92d84b1
c067427
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
CONFIG_SAMPLE_SMART_AMP=m |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#!/usr/bin/env python3 | ||
# SPDX-License-Identifier: BSD-3-Clause | ||
|
||
# We need to calculate ELF section addresses of an LLEXT module and use them to | ||
# run the linker. We could just use Python to calculate addresses and pass them | ||
# back to cmake to have it call the linker. However, there doesn't seem to be a | ||
# portable way to do that. Therefore we pass the linker path and all the command | ||
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. 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. @marc-hb it is SOF-specific. It isn't needed for generic llext loading on Xtensa under Zephyr. It's only needed to match SOF's loadable module / manifest design 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. Why can't we do it in linker scripts? 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.
@pjdobrowolski why should we? If you use a linker script you need to define everything, a complete module linking layout. I see no need for that. The only thing that we need is a fixed memory location where we want to map respective modules, that's the only parameter I'm using. The rest is calculated automatically. In fact ideally even the load address shouldn't be needed. Think about OS executables, libraries, Linux kernel modules, etc. The OS automatically finds a location to copy the code and the data to and automatically selects addresses to map it to. 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. Because it keeps all memory locations and operations in one place. I understand that it is easier to do it but FW is a bit more complex that software and adding python to it doesn't help. 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.
This is quite vague and cryptic, can you elaborate? Python has great support for ELF. It is portable, popular, interactive and high-level which means it does not crash https://github.com/thesofproject/rimage/issues?q=crash So it's hard to find another tool that checks so many boxes for such a task. It's also consistent with Zephyr which uses all over the place for similar jobs. Maybe your comment wasn't about the choice of the Python language specifically but more generally about computing addresses versus hardcoding them?
This is a bit confusing too: addresses dynamically computed are not kept in a different place... would it help to make sure they get printed and saved? 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. BTW there is on-going work to add a post-processing step in CMake: 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.
Depends how you look at it |
||
# line parameters to this script and call the linker directly. | ||
|
||
import argparse | ||
import subprocess | ||
from elftools.elf.elffile import ELFFile | ||
|
||
args = None | ||
def parse_args(): | ||
global args | ||
|
||
parser = argparse.ArgumentParser(description='Helper utility to run a linker command ' | ||
'with calculated ELF section addresses') | ||
|
||
parser.add_argument('command', type=str, help='Linker command to execute') | ||
parser.add_argument('params', nargs='+', help='Additional linker parameters') | ||
parser.add_argument("-f", "--file", required=True, type=str, | ||
help='Object file name') | ||
parser.add_argument("-t", "--text-addr", required=True, type=str, | ||
help='.text section address') | ||
|
||
args = parser.parse_args() | ||
|
||
def main(): | ||
parse_args() | ||
|
||
elf = ELFFile(open(args.file, 'rb')) | ||
|
||
text_addr = int(args.text_addr, 0) | ||
|
||
text_offset = elf.get_section_by_name('.text').header.sh_offset | ||
rodata_offset = elf.get_section_by_name('.rodata').header.sh_offset | ||
data_offset = elf.get_section_by_name('.data').header.sh_offset | ||
|
||
upper = rodata_offset - text_offset + text_addr + 0xfff | ||
rodata_addr = upper - (upper % 0x1000) | ||
|
||
upper = data_offset - rodata_offset + rodata_addr + 0xf | ||
data_addr = upper - (upper % 0x10) | ||
|
||
command = [args.command, | ||
f'-Wl,-Ttext=0x{text_addr:x}', | ||
f'-Wl,--section-start=.rodata=0x{rodata_addr:x}', | ||
f'-Wl,-Tdata=0x{data_addr:x}'] | ||
command.extend(args.params) | ||
|
||
subprocess.run(command) | ||
|
||
if __name__ == "__main__": | ||
main() |
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.
If we have 3 different approaches to loading modules, I wouldn't use such generic name. It cause misunderstanding.
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.
@pjdobrowolski it isn't a name that we choose, it's hard-coded in kconfig tools. We want to use
tristate
in module Kconfig and set it to "m" to build as a module. And for that you needCONFIG_MODULES
. And I'd propose to usetristate
for any modules - whichever implementation they use, but that's up to respective authors / maintainers.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.
Can't we add another specific symbol for LLEXT?
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.
Please source quotations. This one comes from here:
https://github.com/ulfalizer/Kconfiglib/blame/061e71f7d78cb0/kconfiglib.py#L671
This comment is 7 years old. Zephyr modules are months old and still in the making. I guess this text should be updated.
The reason @teburd and @lyakh could implement Zephyr modules without making any
kconfiglib.py
change is becausekconfiglib.py
has stuck to bug-for-bug compatibility with the original Kconfig implementation in C. https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.htmlI suspect the
kconfiglib.py
test suite covered modules even when Zephyr didn't support them yet.Diverging
kconfiglib.py
away from the original Linux kernel implementation could break thekconfiglib.py
test suite and make them slightly incompatible with each other, require slightly different documentations, disrupt other Zephyr users besides SOF,...A potentially huge amount of hassle for a small naming gain.BTW
kconfiglib.py
is also used outside Zephyr.I agree calling one of SOF's "3 different approaches" with a generic "CONFIG_MODULES" sucks, but Zephyr seems to have only one approach so it has no reason to disambiguate. This is hopefully just an implementation detail that most users will not pay much attention to: Kconfig has a user interface that shows the
help
text.Note we have "HOST" abbreviated to "HST" and register sets called
DfPMCCH
. This is not an excuse not to care about names but it put things in a bit of perspective :-)In any case this sounds like a Zephyr topic, not an SOF topic?
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.
Thanks for the context, @marc-hb
Just to clarify a bit - it isn't just about the text (documentation), it's in the kconfiglib implementation. And yes, your text below actually alludes to that too.
[snip]
This isn't an option for one of the 3 different approaches. Other approaches are welcome and encouraged to use it too! IMHO it's a simple and natural way to switch a module between a built-in and modular build, so, any Kconfig options, that want this functionality, are free to use this! E.g. if we modularise our Maths libraries. As for which of the "three approaches" is used - I'd propose to use other means to make those decisions, if we ever have modules, that can be built with more than one option. E.g. indeed with this PR the smart-amp-test module would be buildable in three ways - built-in, as a module using system services or as an LLEXT. Currently The "y" Kconfig option selects whether it's built-in, selecting "m" build it as an LLEXT immediately during the base-firmware build, and to build it as a system-services module you run a separate cmake command, which ignores this Kconfig option completely, so you can build such a module regardless of this Kconfig value.
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.
Thanks @lyakh . I didn't expect system-services modules to ignore the Kconfig of the base firmware (sounds quite risky... even
autoconf.h
?) but if they do then this entire discussion seems indeed irrelevant.