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.

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

Merged
merged 9 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions app/overlays/mtl/module_overlay.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONFIG_SAMPLE_SMART_AMP=m
4 changes: 4 additions & 0 deletions app/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,7 @@ CONFIG_SCHED_CPU_MASK_PIN_ONLY=y
CONFIG_SYS_CLOCK_TICKS_PER_SEC=15000
CONFIG_DAI=y
CONFIG_HEAP_MEM_POOL_SIZE=2048

CONFIG_LLEXT=y
CONFIG_LLEXT_STORAGE_WRITABLE=y
CONFIG_MODULES=y
Copy link
Contributor

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.

Copy link
Collaborator Author

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 need CONFIG_MODULES. And I'd propose to use tristate for any modules - whichever implementation they use, but that's up to respective authors / maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

modules:
The Symbol instance for the modules symbol. Currently hardcoded to
MODULES, which is backwards compatible. Kconfiglib will warn if
'option modules' is set on some other symbol. Tell me if you need proper
'option modules' support.

Can't we add another specific symbol for LLEXT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tell me if you need proper 'option modules' support.

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 because kconfiglib.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.html
I 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 the kconfiglib.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?

Copy link
Collaborator Author

@lyakh lyakh Jan 29, 2024

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

Tell me if you need proper 'option modules' support.

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.

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]

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.

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.

Copy link
Collaborator

@marc-hb marc-hb Jan 30, 2024

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.

56 changes: 56 additions & 0 deletions scripts/llext_link_helper.py
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tejlmand , any idea?

I think we want this to be done in Zephyr for any llext eventually, not just SOF. @lyakh this problem is not SOF specific or is it? Either way state it in this comment so we know where the solution for this should live eventually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we do it in linker scripts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why can't we do it in linker scripts?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

@marc-hb marc-hb Jan 26, 2024

Choose a reason for hiding this comment

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

FW is a bit more complex that software and adding python to it doesn't help.

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?

Because it keeps all memory locations and operations in one place

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

Copy link
Collaborator

Choose a reason for hiding this comment

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

FW is a bit more complex that software...

Depends how you look at it
https://en.wikipedia.org/wiki/Address_space_layout_randomization

# 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()
136 changes: 83 additions & 53 deletions src/audio/module_adapter/module/modules.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sof/lib_manager.h>
#include <sof/audio/module_adapter/module/module_interface.h>
#include <module/module/api_ver.h>
#include <zephyr/llext/llext.h>

/* Intel module adapter is an extension to SOF module adapter component that allows to integrate
* modules developed under IADK (Intel Audio Development Kit) Framework. IADK modules uses uniform
Expand Down Expand Up @@ -49,6 +50,67 @@ DECLARE_SOF_RT_UUID("modules", intel_uuid, 0xee2585f2, 0xe7d8, 0x43dc,
0x90, 0xab, 0x42, 0x24, 0xe0, 0x0c, 0x3e, 0x84);
DECLARE_TR_CTX(intel_codec_tr, SOF_UUID(intel_uuid), LOG_LEVEL_INFO);

static const struct module_interface interface;

static int modules_new(struct processing_module *mod, const void *buildinfo,
uintptr_t module_entry_point)
{
struct module_data *md = &mod->priv;
struct comp_dev *dev = mod->dev;
uint32_t module_id = IPC4_MOD_ID(dev->ipc_config.id);
uint32_t instance_id = IPC4_INST_ID(dev->ipc_config.id);
uint32_t log_handle = (uint32_t) dev->drv->tctx;
/* Connect loadable module interfaces with module adapter entity. */
/* Check if native Zephyr lib is loaded */
struct sof_man_fw_desc *desc = lib_manager_get_library_module_desc(module_id);

if (!desc) {
comp_err(dev, "modules_init(): Failed to load manifest");
return -ENOMEM;
}

const struct sof_module_api_build_info *mod_buildinfo;

if (buildinfo) {
mod_buildinfo = buildinfo;
} else {
struct sof_man_module *module_entry =
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0));

mod_buildinfo =
(struct sof_module_api_build_info *)
(module_entry->segment[SOF_MAN_SEGMENT_TEXT].v_base_addr);
}

byte_array_t mod_cfg = {
.data = (uint8_t *)md->cfg.init_data,
/* Intel modules expects DW size here */
.size = md->cfg.size >> 2,
};

/* Check if module is FDK */
if (mod_buildinfo->format == IADK_MODULE_API_BUILD_INFO_FORMAT &&
mod_buildinfo->api_version_number.full == IADK_MODULE_API_CURRENT_VERSION) {
md->module_adapter = (void *)system_agent_start(module_entry_point,
module_id, instance_id,
0, log_handle, &mod_cfg);
} else if (mod_buildinfo->format == SOF_MODULE_API_BUILD_INFO_FORMAT &&
mod_buildinfo->api_version_number.full == SOF_MODULE_API_CURRENT_VERSION) {
/* The module is native: start agent for sof loadable */
mod->is_native_sof = true;
md->ops = native_system_agent_start(mod->sys_service, module_entry_point,
module_id, instance_id,
0, log_handle, &mod_cfg);
} else {
return -ENOEXEC;
}

md->module_entry_point = module_entry_point;
md->private = mod;

return 0;
}

/**
* \brief modules_init.
* \param[in] mod - processing module pointer.
Expand All @@ -58,64 +120,28 @@ DECLARE_TR_CTX(intel_codec_tr, SOF_UUID(intel_uuid), LOG_LEVEL_INFO);
*/
static int modules_init(struct processing_module *mod)
{
uint32_t module_entry_point;
struct module_data *md = &mod->priv;
struct comp_dev *dev = mod->dev;
const struct ipc4_base_module_cfg *src_cfg = &md->cfg.base_cfg;
byte_array_t mod_cfg;

mod_cfg.data = (uint8_t *)md->cfg.init_data;
/* Intel modules expects DW size here */
mod_cfg.size = md->cfg.size >> 2;
md->private = mod;

struct comp_ipc_config *config = &(dev->ipc_config);

/* At this point module resources are allocated and it is moved to L2 memory. */
module_entry_point = lib_manager_allocate_module(dev->drv, config, src_cfg);
const void *buildinfo = NULL;
int ret;
uintptr_t module_entry_point = lib_manager_allocate_module(mod, config, src_cfg,
&buildinfo);

if (module_entry_point == 0) {
comp_err(dev, "modules_init(), lib_manager_allocate_module() failed!");
return -EINVAL;
}
md->module_entry_point = module_entry_point;
comp_info(dev, "modules_init() start");

uint32_t module_id = IPC4_MOD_ID(dev->ipc_config.id);
uint32_t instance_id = IPC4_INST_ID(dev->ipc_config.id);
uint32_t log_handle = (uint32_t) dev->drv->tctx;
/* Connect loadable module interfaces with module adapter entity. */
/* Check if native Zephyr lib is loaded */
struct sof_man_fw_desc *desc;

desc = lib_manager_get_library_module_desc(module_id);
if (!desc) {
comp_err(dev, "modules_init(): Failed to load manifest");
return -ENOMEM;
if (!md->module_adapter && md->ops == &interface) {
/* First load */
ret = modules_new(mod, buildinfo, module_entry_point);
if (ret < 0)
return ret;
}
struct sof_man_module *module_entry =
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0));

struct sof_module_api_build_info *mod_buildinfo =
(struct sof_module_api_build_info *)
(module_entry->segment[SOF_MAN_SEGMENT_TEXT].v_base_addr);

/* Check if module is FDK */
if (mod_buildinfo->format == IADK_MODULE_API_BUILD_INFO_FORMAT &&
mod_buildinfo->api_version_number.full == IADK_MODULE_API_CURRENT_VERSION) {
md->module_adapter = (void *)system_agent_start(md->module_entry_point, module_id,
instance_id, 0, log_handle,
&mod_cfg);
} else
/* Check if module is native */
if (mod_buildinfo->format == SOF_MODULE_API_BUILD_INFO_FORMAT &&
mod_buildinfo->api_version_number.full == SOF_MODULE_API_CURRENT_VERSION) {
/* If start agent for sof loadable */
mod->is_native_sof = true;
md->ops = native_system_agent_start(mod->sys_service, md->module_entry_point,
module_id, instance_id, 0, log_handle,
&mod_cfg);
} else
return -ENOEXEC;

/* Allocate module buffers */
md->mpd.in_buff = rballoc(0, SOF_MEM_CAPS_RAM, src_cfg->ibs);
Expand All @@ -133,8 +159,6 @@ static int modules_init(struct processing_module *mod)
}
md->mpd.out_buff_size = src_cfg->obs;

int ret;

/* Call module specific init function if exists. */
if (mod->is_native_sof) {
const struct module_interface *mod_in = md->ops;
Expand Down Expand Up @@ -271,7 +295,7 @@ static int modules_free(struct processing_module *mod)
struct comp_dev *dev = mod->dev;
struct module_data *md = &mod->priv;
struct comp_ipc_config *config = &(mod->dev->ipc_config);
int ret = 0;
int ret;

comp_info(dev, "modules_free()");
if (mod->is_native_sof) {
Expand All @@ -281,13 +305,19 @@ static int modules_free(struct processing_module *mod)
} else {
ret = iadk_wrapper_free(mod->priv.module_adapter);
}

if (ret < 0)
comp_err(dev, "Failed to free a module: %d", ret);

rfree(md->mpd.in_buff);
rfree(md->mpd.out_buff);

/* Free module resources allocated in L2 memory. */
ret = lib_manager_free_module(dev->drv, config);
if (ret < 0)
comp_err(dev, "modules_free(), lib_manager_free_module() failed!");
if (!md->llext || !llext_unload(&md->llext)) {
/* Free module resources allocated in L2 memory. */
ret = lib_manager_free_module(mod, config);
if (ret < 0)
comp_err(dev, "modules_free(), lib_manager_free_module() failed!");
}

return ret;
}
Expand Down
5 changes: 4 additions & 1 deletion src/include/module/module/base.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ struct module_config {
#endif
};

struct llext;

/*
* A structure containing a module's private data, intended for its exclusive use.
*
Expand All @@ -60,7 +62,8 @@ struct module_data {
struct module_memory memory; /**< memory allocated by module */
struct module_processing_data mpd; /**< shared data comp <-> module */
void *module_adapter; /**<loadable module interface handle */
uint32_t module_entry_point; /**<loadable module entry point address */
uintptr_t module_entry_point; /**<loadable module entry point address */
struct llext *llext; /**< Zephyr loadable extension context */
#endif /* SOF_MODULE_PRIVATE */
};

Expand Down
11 changes: 6 additions & 5 deletions src/include/sof/lib_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ struct ipc_lib_msg {
};

struct lib_manager_mod_ctx {
struct sof_man_fw_desc *desc;
void *base_addr;
size_t segment_size[3];
};

Expand Down Expand Up @@ -144,6 +144,7 @@ int lib_manager_register_module(struct sof_man_fw_desc *desc, int module_id);
*/
struct sof_man_fw_desc *lib_manager_get_library_module_desc(int module_id);

struct processing_module;
/*
* \brief Allocate module
*
Expand All @@ -154,9 +155,9 @@ struct sof_man_fw_desc *lib_manager_get_library_module_desc(int module_id);
* Function is responsible to allocate module in available free memory and assigning proper address.
* (WIP) These feature will contain module validation and proper memory management.
*/
uint32_t lib_manager_allocate_module(const struct comp_driver *drv,
struct comp_ipc_config *ipc_config,
const void *ipc_specific_config);
uintptr_t lib_manager_allocate_module(struct processing_module *proc,
struct comp_ipc_config *ipc_config,
const void *ipc_specific_config, const void **buildinfo);

/*
* \brief Free module
Expand All @@ -167,7 +168,7 @@ uint32_t lib_manager_allocate_module(const struct comp_driver *drv,
*
* Function is responsible to free module resources in HP memory.
*/
int lib_manager_free_module(const struct comp_driver *drv,
int lib_manager_free_module(struct processing_module *proc,
struct comp_ipc_config *ipc_config);
/*
* \brief Load library
Expand Down
8 changes: 4 additions & 4 deletions src/include/sof/llext_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
struct comp_driver;
struct comp_ipc_config;

uint32_t llext_manager_allocate_module(const struct comp_driver *drv,
struct comp_ipc_config *ipc_config,
const void *ipc_specific_config);
int llext_manager_free_module(const struct comp_driver *drv,
uintptr_t llext_manager_allocate_module(struct processing_module *proc,
struct comp_ipc_config *ipc_config,
const void *ipc_specific_config, const void **buildinfo);
int llext_manager_free_module(struct processing_module *proc,
struct comp_ipc_config *ipc_config);

#endif
Loading
Loading