Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update build system for native loadable modules #8399

Merged
merged 8 commits into from
Nov 9, 2023
Merged

Update build system for native loadable modules #8399

merged 8 commits into from
Nov 9, 2023

Conversation

softwarecki
Copy link
Collaborator

  • Changed the compiler used to build loadable modules to clang.
  • Update path to rimage include directory
  • Added a necessary symbols in the linker script which allows use a memory allocation functions from libc (malloc, calloc, etc.).
  • Changed linker options to enable linking with libgcc and libc.
  • Suggested rimage to ignore lack of the .bss section in loadable modules.
  • Removed alignment of .rodata section,
  • Changed .module section flags using objcopy to ignore it by rimage

@softwarecki
lmdk: Switch to a clang compiler
dc1bc00 
Changed the compiler used to build loadable modules to clang. It has been
used for a long time to build sof. The latest xtensa toolchain no longer
has xcc compiler.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki
lmdk: Update rimage include path
1293e26 
As rimage was moved to sof repository it is necessary to update path to
rimage include directory in lmdk build system.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki
lmdk: Add support for dynamic memory allocation by a loadable modules
439d3aa 
This commit adds a necessary symbols in the linker script which allows use
a memory allocation functions from libc (malloc, calloc, etc.) in loadable
modules.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
lyakh
lyakh reviewed Oct 27, 2023
View reviewed changes
lmdk/cmake/ldscripts/bss_linker_script.txt Show resolved Hide resolved
lmdk/cmake/ldscripts/bss_linker_script.txt Show resolved Hide resolved
lmdk/cmake/build.cmake Outdated Show resolved Hide resolved
jxstelter
jxstelter reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@jxstelter jxstelter left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok except the .rodata alignment removal question.

@softwarecki
lmdk: Enable linking with standard libraries
454e982 
Changed linker options to enable linking with libgcc and libc. This change
is required to allow use compiler builtins functions (like __divdi3) and
dynamic memory allocation functions.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki
rimage: manifest: Ignore lack of .bss section for a loadable modules
c667977 
Loadable libraries do not need to have a .bss section. After this change,
its absence will be ignored when generating a image of the loadable module.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki
lmdk: Remove align of .rodata section
53c092d 
The .data and .rodata sections are placed by rimage into one segment of
a resulting firmware image. So aligning the .rodata section to the page
boundary is just a waste of memory.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki
lmdk: Remove content of the .module section from final module image
2943138 
The .module section contains the module manifest read by rimage. It should
not be included in the final loadable module image. Flags of this section
are now modified using objcopy to ignore it by rimage.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@softwarecki softwarecki mentioned this pull request Oct 27, 2023
Merged
lgirdwood
lgirdwood reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to keep our client module API consistent between built-in and module, having reduced API surface and limitations around resources for modules vs built-in is going to be difficult. The latter could be solved by making sure we have some resource allocation API available in the services.

lmdk/cmake/xtensa-toolchain.cmake Show resolved Hide resolved
lmdk/cmake/build.cmake Outdated Show resolved Hide resolved
lmdk/cmake/ldscripts/data_linker_script.txt Outdated Show resolved Hide resolved
@softwarecki
lmdk: Instruct linker to keep .buildinfo section
d817f97 
At the beginning of the .text section there must be a structure containing
information about the version of the api used by the library. It's absence
will cause sof to interpret a random literal as a version and incorrectly
load a library.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@marc-hb marc-hb requested a review from dcpleung October 30, 2023 23:44
lgirdwood
lgirdwood reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just need to Kconfig and Cmake comments (and maybe in main lmdk header), more of a reminder for developers when they debug i.e. have you manually confirmed you are using same config for base FW and modules before further debug

kv2019i
kv2019i approved these changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumptions on xtensa and particular toolchain limit applicability of lmdk quite a bit, but I don't see this PR really making this aspect more/less severe, so no have no particular objections to this PR.

@kv2019i kv2019i merged commit 3bd9991 into thesofproject:main Nov 9, 2023
44 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjdobrowolski pjdobrowolski pjdobrowolski approved these changes

@jxstelter jxstelter jxstelter left review comments

@marc-hb marc-hb marc-hb left review comments

@lgirdwood lgirdwood lgirdwood left review comments

@lyakh lyakh lyakh left review comments

@andyross andyross andyross left review comments

@serhiy-katsyuba-intel serhiy-katsyuba-intel serhiy-katsyuba-intel approved these changes

@kv2019i kv2019i kv2019i approved these changes

@abonislawski abonislawski Awaiting requested review from abonislawski

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski

@tmleman tmleman Awaiting requested review from tmleman

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@dcpleung dcpleung Awaiting requested review from dcpleung

Assignees
No one assigned
Labels
loadable modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@softwarecki @andyross @lyakh @lgirdwood @kv2019i @marc-hb @jxstelter @pjdobrowolski @serhiy-katsyuba-intel