Skip to content

library_manager: Update module load flow #8544

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

Merged
lgirdwood merged 2 commits into from
Jan 24, 2024
Merged

library_manager: Update module load flow #8544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
029b922
lib_manager: Update module load flow
softwarecki Nov 15, 2023
6fe60c5
lib_manager: Add libcode modules support
softwarecki Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/include/sof/lib_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ struct ipc_lib_msg {
struct ext_library {
struct k_spinlock lock; /* last locking CPU record */
struct sof_man_fw_desc *desc[LIB_MANAGER_MAX_LIBS];
#ifdef CONFIG_LIBCODE_MODULE_SUPPORT
uint32_t mods_exec_load_cnt;
#endif
struct ipc_lib_msg *lib_notif_pool;
uint32_t lib_notif_count;

Expand Down
13 changes: 13 additions & 0 deletions src/library_manager/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,17 @@ config LIBRARY_MANAGER
Externally developed modules both for SOF and Zephyr
could be used if enabled.
If unsure say N.


config LIBCODE_MODULE_SUPPORT
bool "Add support for libcode modules"
default n
depends on LIBRARY_MANAGER
help
A loadable library can contain a several modules marked
as lib_code. This modules contains code shared by
a multiple modules. This option adds support for modules
of this type.
If unsure say N.

endmenu
233 changes: 139 additions & 94 deletions src/library_manager/lib_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ static struct ext_library loader_ext_lib;

#define PAGE_SZ CONFIG_MM_DRV_PAGE_SIZE

static int lib_manager_load_data_from_storage(void __sparse_cache *vma, void *s_addr,
uint32_t size, uint32_t flags)
static int lib_manager_load_data_from_storage(void __sparse_cache *vma, void *s_addr, uint32_t size,
uint32_t flags)
{
int ret = sys_mm_drv_map_region((__sparse_force void *)vma, POINTER_TO_UINT(NULL),
size, flags);
/* Region must be first mapped as writable in order to initialize its contents. */
int ret = sys_mm_drv_map_region((__sparse_force void *)vma, POINTER_TO_UINT(NULL), size,
flags | SYS_MM_MEM_PERM_RW);
if (ret < 0)
return ret;

Expand All @@ -65,119 +66,144 @@ static int lib_manager_load_data_from_storage(void __sparse_cache *vma, void *s_

dcache_writeback_region(vma, size);

/* TODO: Change attributes for memory to FLAGS */
/* TODO: Change attributes for memory to FLAGS. Implementation of required function in tlb
* driver is in progress.
* sys_mm_drv_update_region_flags(vma, size, flags);
*/
return 0;
}

static int lib_manager_load_module(uint32_t module_id, struct sof_man_module *mod,
struct sof_man_fw_desc *desc)
static int lib_manager_load_module(const uint32_t module_id,
const struct sof_man_module *const mod)
{
struct ext_library *ext_lib = ext_lib_get();
uint32_t lib_id = LIB_MANAGER_GET_LIB_ID(module_id);
size_t load_offset = (size_t)((void *)ext_lib->desc[lib_id]);
void __sparse_cache *va_base_text = (void __sparse_cache *)
mod->segment[SOF_MAN_SEGMENT_TEXT].v_base_addr;
void *src_txt = (void *)(mod->segment[SOF_MAN_SEGMENT_TEXT].file_offset + load_offset);
size_t st_text_size = mod->segment[SOF_MAN_SEGMENT_TEXT].flags.r.length;
void __sparse_cache *va_base_rodata = (void __sparse_cache *)
mod->segment[SOF_MAN_SEGMENT_RODATA].v_base_addr;
void *src_rodata =
(void *)(mod->segment[SOF_MAN_SEGMENT_RODATA].file_offset + load_offset);
size_t st_rodata_size = mod->segment[SOF_MAN_SEGMENT_RODATA].flags.r.length;
const struct ext_library *const ext_lib = ext_lib_get();
const uint32_t lib_id = LIB_MANAGER_GET_LIB_ID(module_id);
const uintptr_t load_offset = POINTER_TO_UINT(ext_lib->desc[lib_id]);
void *src;
void __sparse_cache *va_base;
size_t size;
uint32_t flags;
int ret, idx;

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

@lyakh lyakh Dec 1, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@softwarecki softwarecki Dec 7, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@lyakh lyakh Dec 8, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lgirdwood lgirdwood Dec 18, 2023

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@pjdobrowolski pjdobrowolski Dec 19, 2023

Choose a reason for hiding this comment

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

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/

Copy link
Contributor

@pjdobrowolski pjdobrowolski Dec 19, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@jxstelter jxstelter Dec 19, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@lyakh lyakh Dec 19, 2023

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@pjdobrowolski pjdobrowolski Dec 19, 2023

Choose a reason for hiding this comment

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

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".

Copy link
Member

@lgirdwood lgirdwood Dec 20, 2023

Choose a reason for hiding this comment

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

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.


flags = 0;

if (mod->segment[idx].flags.r.code)
flags = SYS_MM_MEM_PERM_EXEC;
else if (!mod->segment[idx].flags.r.readonly)
flags = SYS_MM_MEM_PERM_RW;

src = UINT_TO_POINTER(mod->segment[idx].file_offset + load_offset);
va_base = (void __sparse_cache *)UINT_TO_POINTER(mod->segment[idx].v_base_addr);
size = mod->segment[idx].flags.r.length * PAGE_SZ;
ret = lib_manager_load_data_from_storage(va_base, src, size, flags);
if (ret < 0)
goto err;
}

return 0;

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

va_base = (void __sparse_cache *)UINT_TO_POINTER(mod->segment[idx].v_base_addr);
size = mod->segment[idx].flags.r.length * PAGE_SZ;
sys_mm_drv_unmap_region((__sparse_force void *)va_base, size);
}

return ret;
}

static int lib_manager_unload_module(const struct sof_man_module *const mod)
{
void __sparse_cache *va_base;
size_t size;
uint32_t idx;
int ret;

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

/* Copy Code */
ret = lib_manager_load_data_from_storage(va_base_text, src_txt, st_text_size,
SYS_MM_MEM_PERM_RW | SYS_MM_MEM_PERM_EXEC);
if (ret < 0)
goto err;
va_base = (void __sparse_cache *)UINT_TO_POINTER(mod->segment[idx].v_base_addr);
size = mod->segment[idx].flags.r.length * PAGE_SZ;
ret = sys_mm_drv_unmap_region((__sparse_force void *)va_base, size);
if (ret < 0)
return ret;
}

/* Copy RODATA */
ret = lib_manager_load_data_from_storage(va_base_rodata, src_rodata,
st_rodata_size, SYS_MM_MEM_PERM_RW);
if (ret < 0)
goto err;
return 0;
}

/* There are modules marked as lib_code. This is code shared between several modules inside
* the library. Load all lib_code modules with first none lib_code module load.
*/
if (!mod->type.lib_code)
ext_lib->mods_exec_load_cnt++;

if (ext_lib->mods_exec_load_cnt == 1) {
struct sof_man_module *module_entry =
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0));
for (size_t idx = 0; idx < desc->header.num_module_entries;
++idx, ++module_entry) {
if (module_entry->type.lib_code) {
ret = lib_manager_load_module(lib_id << LIB_MANAGER_LIB_ID_SHIFT |
idx, mod, desc);
if (ret < 0)
goto err;
}
#ifdef CONFIG_LIBCODE_MODULE_SUPPORT
/* There are modules marked as lib_code. This is code shared between several modules inside
* the library. Load all lib_code modules with first none lib_code module load.
*/
static int lib_manager_load_libcode_modules(const uint32_t module_id,
const struct sof_man_fw_desc *const desc)
{
struct ext_library *const ext_lib = ext_lib_get();
const struct sof_man_module *module_entry = (struct sof_man_module *)
((char *)desc + SOF_MAN_MODULE_OFFSET(0));
const uint32_t lib_id = LIB_MANAGER_GET_LIB_ID(module_id);
int ret, idx;

if (++ext_lib->mods_exec_load_cnt > 1)
return 0;

for (idx = 0; idx < desc->header.num_module_entries; ++idx, ++module_entry) {
if (module_entry->type.lib_code) {
ret = lib_manager_load_module(lib_id << LIB_MANAGER_LIB_ID_SHIFT | idx,
module_entry);
if (ret < 0)
goto err;
}
}

return 0;

err:
sys_mm_drv_unmap_region((__sparse_force void *)va_base_text, st_text_size);
sys_mm_drv_unmap_region((__sparse_force void *)va_base_rodata, st_rodata_size);
for (--idx, --module_entry; idx >= 0; --idx, --module_entry) {
if (module_entry->type.lib_code) {
ret = lib_manager_unload_module(module_entry);
if (ret < 0)
goto err;
}
}

return ret;
}

static int lib_manager_unload_module(uint32_t module_id, struct sof_man_module *mod,
struct sof_man_fw_desc *desc)
/* There are modules marked as lib_code. This is code shared between several modules inside
* the library. Unload all lib_code modules with last none lib_code module unload.
*/
static int lib_manager_unload_libcode_modules(const uint32_t module_id,
const struct sof_man_fw_desc *const desc)
{
struct ext_library *ext_lib = ext_lib_get();
uint32_t lib_id = LIB_MANAGER_GET_LIB_ID(module_id);
void __sparse_cache *va_base_text = (void __sparse_cache *)
mod->segment[SOF_MAN_SEGMENT_TEXT].v_base_addr;
size_t st_text_size = mod->segment[SOF_MAN_SEGMENT_TEXT].flags.r.length;
void __sparse_cache *va_base_rodata = (void __sparse_cache *)
mod->segment[SOF_MAN_SEGMENT_RODATA].v_base_addr;
size_t st_rodata_size = mod->segment[SOF_MAN_SEGMENT_RODATA].flags.r.length;
int ret;
struct ext_library *const ext_lib = ext_lib_get();
const struct sof_man_module *module_entry = (struct sof_man_module *)
((char *)desc + SOF_MAN_MODULE_OFFSET(0));
int ret, idx;

st_text_size = st_text_size * PAGE_SZ;
st_rodata_size = st_rodata_size * PAGE_SZ;

ret = sys_mm_drv_unmap_region((__sparse_force void *)va_base_text, st_text_size);
if (ret < 0)
return ret;

ret = sys_mm_drv_unmap_region((__sparse_force void *)va_base_rodata, st_rodata_size);
if (ret < 0)
return ret;

/* There are modules marked as lib_code. This is code shared between several modules inside
* the library. Unload all lib_code modules with last none lib_code module unload.
*/
if (mod->type.lib_code)
return ret;
if (--ext_lib->mods_exec_load_cnt > 0)
return 0;

if (!mod->type.lib_code && ext_lib->mods_exec_load_cnt > 0)
ext_lib->mods_exec_load_cnt--;

if (ext_lib->mods_exec_load_cnt == 0) {
struct sof_man_module *module_entry =
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0));
for (size_t idx = 0; idx < desc->header.num_module_entries;
++idx, ++module_entry) {
if (module_entry->type.lib_code) {
ret =
lib_manager_unload_module(lib_id << LIB_MANAGER_LIB_ID_SHIFT |
idx, mod, desc);
}
for (idx = 0; idx < desc->header.num_module_entries; ++idx, ++module_entry) {
if (module_entry->type.lib_code) {
ret = lib_manager_unload_module(module_entry);
if (ret < 0)
return ret;
}
}

return ret;
return 0;
}
#endif /* CONFIG_LIBCODE_MODULE_SUPPORT */

static void __sparse_cache *lib_manager_get_instance_bss_address(uint32_t module_id,
uint32_t instance_id,
Expand Down Expand Up @@ -262,18 +288,31 @@ uint32_t lib_manager_allocate_module(const struct comp_driver *drv,

mod = (struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(entry_index));

ret = lib_manager_load_module(module_id, mod, desc);
ret = lib_manager_load_module(module_id, mod);
if (ret < 0)
return 0;

#ifdef CONFIG_LIBCODE_MODULE_SUPPORT
ret = lib_manager_load_libcode_modules(module_id, desc);
if (ret < 0)
goto err;
#endif /* CONFIG_LIBCODE_MODULE_SUPPORT */

ret = lib_manager_allocate_module_instance(module_id, IPC4_INST_ID(ipc_config->id),
base_cfg->is_pages, mod);
if (ret < 0) {
tr_err(&lib_manager_tr,
"lib_manager_allocate_module(): module allocation failed: %d", ret);
return 0;
#ifdef CONFIG_LIBCODE_MODULE_SUPPORT
lib_manager_unload_libcode_modules(module_id, desc);
#endif /* CONFIG_LIBCODE_MODULE_SUPPORT */
goto err;
}
return mod->entry_point;

err:
lib_manager_unload_module(mod);
return 0;
}

int lib_manager_free_module(const struct comp_driver *drv,
Expand All @@ -290,9 +329,15 @@ int lib_manager_free_module(const struct comp_driver *drv,
desc = lib_manager_get_library_module_desc(module_id);
mod = (struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(entry_index));

ret = lib_manager_unload_module(module_id, mod, desc);
ret = lib_manager_unload_module(mod);
if (ret < 0)
return ret;

#ifdef CONFIG_LIBCODE_MODULE_SUPPORT
ret = lib_manager_unload_libcode_modules(module_id, desc);
if (ret < 0)
return ret;
#endif /* CONFIG_LIBCODE_MODULE_SUPPORT */

ret = lib_manager_free_module_instance(module_id, IPC4_INST_ID(ipc_config->id), mod);
if (ret < 0) {
Expand Down