library_manager: Update module load flow #8544

softwarecki · 2023-11-28T14:45:07Z

The previous code have several issues:

Did not support empty data segment.
It assumed that certain types of segments would be at fixed indexes, without taking into account their flags/types.
Incorrect handling of mapping error. If the virtual address cannot be mapped because is already in use, the error handler will unmap this address.
If there is an error loading one of the modules marked as lib_code, previously loaded modules are not unloaded.

This commit fixes the above issues, but it probably introduces new problems :)

pjdobrowolski

Good move to split that long procedure into separate functions.

src/library_manager/lib_manager.c

keqiaozhang · 2023-11-30T11:44:27Z

SOFCI TEST

kv2019i

I'd have preferred separate commits as there are distinct separate issues you are addressing. Now the diff is really hard to follow so I instead just read the new code on its own and seems fine.

src/library_manager/lib_manager.c

lyakh · 2023-12-01T11:33:49Z

src/library_manager/lib_manager.c

+
+	for (idx = 0; idx < ARRAY_SIZE(mod->segment); ++idx) {
+		if (!mod->segment[idx].flags.r.load)
+			continue;


this skips .bss, so effectively you made a loop for 2 elements. Should we go a step further and integrate .bss here too by either copying data for .data and .text or clearing the buffer for .bss?

@lyakh The .bss section is handled differently. Its size is divided by the maximum number of module instances and only the selected fragment for a given instance is mapped.

well, that special .bss handling seems strange to me - it's too ad-hoc IMHO. This means, that you cannot have the same file used both as a loadable module and built-in, because its .bss will be handled differently. Besides it's too special - developers would need to think about it every time they put something in .bss. We discussed this with @pjdobrowolski briefly and maybe we can and should drop that behaviour.
And if we don't and we keep .bss out and only handle .text and .data here, we effectively get a loop over three elements to then throw away one of them, leaving a loop over two iterations - you could just keep it as is unfolded.

@softwarecki @lyakh any update here, should we put a big inline comment here spelling out this difference and its expected behavior from ROM i.e. its done for current compatibility reasons

any communication between modules, build-in or loadable should be done by using native-system-services. I'm no sure if making shared bss is good practice for such behavior. Especially if we load and unload modules on fly, what will happen with common bss? Each module will leave it own stuff in there until we will have an overflow/

DSP has restricted 3MB of ram which is shared between modules. BSS is defining how much of we use. Can you explain me how removing BSS sections from elf will improve working on such small space?

@lyakh You wrote: "This means, that you cannot have the same file used both as a loadable module and built-in...". Where you see use case to have the same code used as loadable and built-in at the same time? I think we do loadable to not have given module built into base FW all the time.
Another think, there are other reasons for having separate .bss section for each module.
One is backward compatibility with reference FW design.
The other even more important is possibility to use in the feature some memory protection features. Mixing up the module .bss with base FW general .bss section will block us from introducing such solution.
The third reason is the debuggability of base FW with presence of external modules. If we mix up the memory spaces, tracking any failures will be more difficult since external modules could corrupt base FW data.

@pjdobrowolski @jxstelter I think there's some confusion here. I'm not proposing to remove .bss or to share .bss between modules or anything special of that kind. I just propose to do what C does: you program

static int module_x;

and it's placed in .bss and it's available there to that file (because it's static). Or you have a global

int global_x;

and it's also in .bss and accessible globally. In all cases there's exactly 1 copy of that object. What I don't understand why loadable modules are linking those objects with module instances. That means, that e.g. if you make volume a loadable module. And you activate 3 pipelines and each of them has a volume component in it so you have 3 instances. So then you give to each instance i.e. to each volume component in each pipeline a separate part of that .bss, i.e. a part of those objects. I understand why it is done, but I don't find this a good idea. It is done to "emulate" a poor-man allocation, so you want to put something like

static struct module_private_data[MAX_MODULE_INSTANCES];

in those modules and then give each instance one of those objects. But I don't find that a good approach. I think it's confusing and misinterpreting the meaning of the ELF .bss section

If you would look more carefully in history of commits, you would find out, that connecting .bss with instances was introduce much before idea of "poor-man allocation".

ok, so I think we probably need to make relationship between bss and instances more visible with inline comments in the code here, this way we can avoid any misunderstandings as we continue and add logic for services API and IADK modules.

src/library_manager/lib_manager.c

softwarecki · 2023-12-07T15:13:25Z

I'd have preferred separate commits as there are distinct separate issues you are addressing.

I identified the mentioned issues and simply write a new code from scrach. There were no incremental changes that I could separate.

lgirdwood · 2023-12-20T09:48:06Z

@softwarecki fyi some CI failures, there has been a recent west update so you may want to rebase and rerun the CI.

I still don't find the .bss handling in loadable modules a good idea, but it seems to be the current convention in IADK

kv2019i · 2024-01-09T11:19:13Z

@softwarecki This is ready to go, but we'd need a green run for CI, can you check?

lgirdwood · 2024-01-09T15:07:58Z

@softwarecki This is ready to go, but we'd need a green run for CI, can you check?

@kv2019i sparse errors ? Do we need a west update to fix them ?

kv2019i · 2024-01-09T16:59:28Z

@lgirdwood wrote:

@softwarecki This is ready to go, but we'd need a green run for CI, can you check?

@kv2019i sparse errors ? Do we need a west update to fix them ?

"Internal Intel CI System/merge/build — Tests failed " -> https://sof-ci.01.org/sof-pr-viewer/#/build/PR8544/build13245909

lgirdwood

ok, so we are good to go as soon as CI passes.

softwarecki · 2024-01-15T11:26:37Z

Im working on it to make CI pass

The previous code have several issues: * Did not support empty data segment. * It assumed that certain types of segments would be at fixed indexes, without taking into account their flags/types. * Incorrect handling of mapping error. If the virtual address cannot be mapped because is already in use, the error handler will unmap this address. * If there is an error loading one of the modules marked as lib_code, previously loaded modules are not unloaded. This commit fixes the above issues. Signed-off-by: Adrian Warecki <[email protected]>

A loadable library can contain a several modules marked as lib_code. This modules contains code shared by a multiple modules. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki · 2024-01-24T13:53:59Z

@lgirdwood i think its ready

lgirdwood · 2024-01-24T14:05:19Z

Zephr main failures are known and unrelated.

softwarecki added the loadable modules label Nov 28, 2023

softwarecki requested review from lyakh, abonislawski, kv2019i, marcinszkudlinski, jxstelter, pjdobrowolski and tmleman November 28, 2023 14:45

softwarecki force-pushed the segments branch from 5aa3dbe to f72527f Compare November 28, 2023 15:03

jxstelter approved these changes Nov 28, 2023

View reviewed changes

softwarecki marked this pull request as ready for review November 28, 2023 15:36

softwarecki requested review from lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners November 28, 2023 15:36

pjdobrowolski approved these changes Nov 29, 2023

View reviewed changes

lyakh previously requested changes Nov 29, 2023

View reviewed changes

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

softwarecki force-pushed the segments branch from f72527f to a745984 Compare November 29, 2023 15:20

softwarecki mentioned this pull request Nov 30, 2023

rimage: manifest: Improve handling of empty segments #8531

Merged

softwarecki force-pushed the segments branch from a745984 to 7698ba5 Compare November 30, 2023 14:54

kv2019i approved these changes Nov 30, 2023

View reviewed changes

lyakh reviewed Dec 1, 2023

View reviewed changes

softwarecki force-pushed the segments branch from 7698ba5 to 8528265 Compare December 8, 2023 13:02

softwarecki requested a review from lyakh December 8, 2023 15:44

lgirdwood added this to the v2.9 milestone Dec 18, 2023

lgirdwood approved these changes Jan 10, 2024

View reviewed changes

softwarecki force-pushed the segments branch from 8528265 to 1211635 Compare January 12, 2024 12:34

softwarecki force-pushed the segments branch 2 times, most recently from 34628b3 to bd2059e Compare January 23, 2024 14:36

lyakh mentioned this pull request Jan 24, 2024

1st part of llext #8180 #8759

Merged

softwarecki added 2 commits January 24, 2024 11:39

lib_manager: Add libcode modules support

6fe60c5

A loadable library can contain a several modules marked as lib_code. This modules contains code shared by a multiple modules. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the segments branch from bd2059e to 6fe60c5 Compare January 24, 2024 10:39

lgirdwood approved these changes Jan 24, 2024

View reviewed changes

lgirdwood merged commit fec50da into thesofproject:main Jan 24, 2024
40 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

library_manager: Update module load flow #8544

library_manager: Update module load flow #8544

softwarecki commented Nov 28, 2023

pjdobrowolski left a comment

keqiaozhang commented Nov 30, 2023

kv2019i left a comment

lyakh Dec 1, 2023

softwarecki Dec 7, 2023

lyakh Dec 8, 2023

lgirdwood Dec 18, 2023

pjdobrowolski Dec 19, 2023

pjdobrowolski Dec 19, 2023

jxstelter Dec 19, 2023

lyakh Dec 19, 2023

pjdobrowolski Dec 19, 2023

lgirdwood Dec 20, 2023

softwarecki commented Dec 7, 2023

lgirdwood commented Dec 20, 2023

kv2019i commented Jan 9, 2024

lgirdwood commented Jan 9, 2024

kv2019i commented Jan 9, 2024

lgirdwood left a comment

softwarecki commented Jan 15, 2024

softwarecki commented Jan 24, 2024

lgirdwood commented Jan 24, 2024

library_manager: Update module load flow #8544

library_manager: Update module load flow #8544

Conversation

softwarecki commented Nov 28, 2023

pjdobrowolski left a comment

Choose a reason for hiding this comment

keqiaozhang commented Nov 30, 2023

kv2019i left a 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

Choose a reason for hiding this comment

softwarecki commented Dec 7, 2023

lgirdwood commented Dec 20, 2023

kv2019i commented Jan 9, 2024

lgirdwood commented Jan 9, 2024

kv2019i commented Jan 9, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

softwarecki commented Jan 15, 2024

softwarecki commented Jan 24, 2024

lgirdwood commented Jan 24, 2024