Skip to content
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
14 changes: 8 additions & 6 deletions .github/workflows/llext.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
strategy:
fail-fast: false
matrix:
platform: [mtl, lnl]
platform: [mtl]

steps:
- name: git clone sof
Expand All @@ -34,8 +34,10 @@ jobs:
- name: llext build
run: |
cd workspace && ./sof/zephyr/docker-run.sh /bin/sh -c \
'ln -s /opt/toolchains/zephyr-sdk-* ~/;
west build --board intel_adsp_ace15_mtpm sof/app \
-- -DEXTRA_CFLAGS=-Werror -DEXTRA_CXXFLAGS=-Werror \
-DEXTRA_AFLAGS=-Werror \
-DOVERLAY_CONFIG=overlays/mtl/module_overlay.conf'
"ln -s /opt/toolchains/zephyr-sdk-* ~/;
python sof/scripts/xtensa-build-zephyr.py \
--cmake-args=-DEXTRA_CFLAGS=-Werror \
--cmake-args=-DEXTRA_CXXFLAGS=-Werror \
--cmake-args=-DEXTRA_AFLAGS='-Werror -Wa,--fatal-warnings' \
--cmake-args=--warn-uninitialized \
--overlay=sof/app/overlays/mtl/module_overlay.conf ${{ matrix.platform }}"
3 changes: 2 additions & 1 deletion scripts/llext_link_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ def main():
try:
offset = elf.get_section_by_name(sections[i]).header.sh_offset
size = elf.get_section_by_name(sections[i]).header.sh_size
except:
except AttributeError:
print("section " + sections[i] + " not found in " + args.file)
continue

if last_increment == 0:
Expand Down
84 changes: 76 additions & 8 deletions scripts/xtensa-build-zephyr.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@

sof_fw_version = None

signing_key = None

if py_platform.system() == "Windows":
xtensa_tools_version_postfix = "-win32"
elif py_platform.system() == "Linux":
Expand Down Expand Up @@ -601,7 +603,7 @@ def clean_staging(platform):
def rimage_west_configuration(platform_dict, dest_dir):
"""Configure rimage in a new file `dest_dir/westconfig.ini`, starting
from the workspace .west/config.
Returns the pathlib.Path to the new file.
Returns a tuple (west ConfigFile, pathlib.Path to that new file).
"""

saved_local_var = os.environ.get('WEST_CONFIG_LOCAL')
Expand Down Expand Up @@ -641,7 +643,7 @@ def rimage_west_configuration(platform_dict, dest_dir):

platform_wconfig.set("rimage.extra-args", shlex.join(extra_args))

return platform_west_config_path
return platform_wconfig, platform_west_config_path


def build_rimage():
Expand Down Expand Up @@ -671,12 +673,12 @@ def rimage_options(platform_dict):
example: [ (-f, 2.5.0), (-b, 1), (-k, key.pem),... ]

"""
global signing_key
opts = []

if args.verbose > 0:
opts.append(("-v",) * args.verbose)

signing_key = None
if args.key:
key_path = pathlib.Path(args.key)
assert key_path.exists(), f"{key_path} not found"
Expand Down Expand Up @@ -831,10 +833,11 @@ def build_platforms():
see https://docs.zephyrproject.org/latest/guides/west/build-flash-debug.html#one-time-cmake-arguments
Try "west config build.cmake-args -- ..." instead.""")

platf_build_environ['WEST_CONFIG_LOCAL'] = str(rimage_west_configuration(
platform_wcfg, wcfg_path = rimage_west_configuration(
platform_dict,
STAGING_DIR / "sof-info" / platform
))
)
platf_build_environ['WEST_CONFIG_LOCAL'] = str(wcfg_path)

# Make sure the build logs don't leave anything hidden
execute_command(['west', 'config', '-l'], cwd=west_top,
Expand Down Expand Up @@ -869,7 +872,7 @@ def build_platforms():
if platform not in RI_INFO_UNSUPPORTED:
reproducible_checksum(platform, west_top / platform_build_dir_name / "zephyr" / "zephyr.ri")

install_platform(platform, sof_platform_output_dir, platf_build_environ)
install_platform(platform, sof_platform_output_dir, platf_build_environ, platform_wcfg)

src_dest_list = []
tools_output_dir = pathlib.Path(STAGING_DIR, "tools")
Expand Down Expand Up @@ -902,7 +905,69 @@ def build_platforms():
symlinks=True, ignore_dangling_symlinks=True, dirs_exist_ok=True)


def install_platform(platform, sof_output_dir, platf_build_environ):
def install_lib(sof_lib_dir, abs_build_dir, platform_wconfig):
"""[summary] Sign loadable llext modules, if any, copy them to the
deployment tree and create UUID links for the kernel to find and load
them."""

global signing_key
Copy link
Member

Choose a reason for hiding this comment

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

should this be a module_signing_key? Is this different from the base firmware key? why is this global?


with os.scandir(str(abs_build_dir)) as iter:
if args.key_type_subdir != "none":
sof_lib_dir = sof_lib_dir / args.key_type_subdir

sof_lib_dir.mkdir(parents=True, exist_ok=True)

for entry in iter:
if (not entry.is_dir or
not entry.name.endswith('_llext')):
continue

entry_path = pathlib.Path(entry.path)

uuids = entry_path / 'llext.uuid'
if not os.path.exists(uuids):
print(f"Directory {entry.name} has no llext.uuid file. Skipping.")
continue

# replace '_llext' with '.llext', e.g.
# eq_iir_llext/eq_iir.llext
llext_base = entry.name[:-6]
llext_file = llext_base + '.llext'

dst = sof_lib_dir / llext_file

rimage_cfg = entry_path / 'rimage_config.toml'
llext_input = entry_path / (llext_base + '.so')
llext_output = entry_path / llext_file

sign_cmd = [str(platform_wconfig.get("rimage.path")), "-o", str(llext_output),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sign_cmd = [str(platform_wconfig.get("rimage.path")), "-o", str(llext_output),
sign_cmd = [platform_wconfig.get("rimage.path"), "-o", str(llext_output),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed in de87535

Copy link
Collaborator

@marc-hb marc-hb May 7, 2024

Choose a reason for hiding this comment

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

addressed in de87535

"This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository."

I found it in followup #9090

"-e", "-c", str(rimage_cfg),
"-k", str(signing_key), "-l", "-r",
str(llext_input)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't hardcode rimage.path and arguments like that, the user has the ability to override those, see rimage_west_configuration(). See top-level comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to CMake, I still can't see what is temporary versus not here. Please add the relevant TODOs. Even more important considering the high-level signing design sounds like "do whatever you want".

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 everything is temporary... I'm far from being a Python expert, so I expect improvements throughout this code - both during this review and after this is (hopefully soon) merged

Copy link
Collaborator

@marc-hb marc-hb May 2, 2024

Choose a reason for hiding this comment

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

everything is temporary...

Yes of course, we all die in the end. Yet there is a huge difference between:

  1. This code MAY change some day - or not, who knows?
  2. We SHOULD change this ASAP and move it to Zephyr, using new API X, Z and Z.

Seeing comment 2. stops people from wasting too much time with the corresponding code.

execute_command(sign_cmd, cwd=west_top)

# An intuitive way to make this multiline would be
# with (open(dst, 'wb') as fdst, open(llext_output, 'rb') as fllext,
# open(llext_output.with_suffix('.llext.xman'), 'rb') as fman):
# but a Python version, used on Windows errored out on this.
# Thus we're left with a choice between a 150-character
# long line and an illogical split like this
with open(dst, 'wb') as fdst, open(llext_output, 'rb') as fllext, open(
llext_output.with_suffix('.llext.xman'), 'rb') as fman:
# Concatenate the manifest and the llext
shutil.copyfileobj(fman, fdst)
shutil.copyfileobj(fllext, fdst)

# Create symbolic links for all UUIDs
with open(uuids, 'r') as uuids_f:
for uuid in uuids_f:
linkname = uuid.strip() + '.bin'
symlink_or_copy(sof_lib_dir, llext_file,
sof_lib_dir, linkname)


def install_platform(platform, sof_output_dir, platf_build_environ, platform_wconfig):

# Keep in sync with caller
platform_build_dir_name = f"build-{platform}"
Expand Down Expand Up @@ -948,6 +1013,10 @@ def install_platform(platform, sof_output_dir, platf_build_environ):
for p_alias in platform_configs[platform].aliases:
symlink_or_copy(install_key_dir, output_fwname, install_key_dir, f"sof-{p_alias}.ri")

if args.deployable_build and platform_configs[platform].ipc4:
install_lib(sof_output_dir / '..' / 'sof-ipc4-lib' / platform, abs_build_dir,
platform_wconfig)


# sof-info/ directory

Expand Down Expand Up @@ -1033,7 +1102,6 @@ class InstFile:
gzip_res.result() # throws exception if gzip unexpectedly failed
gzip_threads.shutdown()


# Zephyr's CONFIG_KERNEL_BIN_NAME default value
BIN_NAME = 'zephyr'

Expand Down
15 changes: 11 additions & 4 deletions src/library_manager/llext_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@
#include <stddef.h>
#include <stdint.h>

/*
* FIXME: this definition is copied from tools/rimage/src/include/rimage/manifest.h
* which we cannot easily include here, because it also pulls in
* tools/rimage/src/include/rimage/elf.h which then conflicts with
* zephyr/include/zephyr/llext/elf.h
*/
#define FILE_TEXT_OFFSET_V1_8 0x8000

LOG_MODULE_DECLARE(lib_manager, CONFIG_SOF_LOG_LEVEL);

extern struct tr_ctx lib_manager_tr;
Expand Down Expand Up @@ -172,11 +180,10 @@ static int llext_manager_link(struct sof_man_fw_desc *desc, struct sof_man_modul
uint32_t module_id, struct module_data *md, const void **buildinfo,
const struct sof_man_module_manifest **mod_manifest)
{
size_t mod_size = desc->header.preload_page_count * PAGE_SZ - 0x8000;
/* FIXME: where does the module begin?? */
size_t mod_size = desc->header.preload_page_count * PAGE_SZ - FILE_TEXT_OFFSET_V1_8;
struct llext_buf_loader ebl = LLEXT_BUF_LOADER((uint8_t *)desc -
SOF_MAN_ELF_TEXT_OFFSET + 0x8000,
mod_size);
SOF_MAN_ELF_TEXT_OFFSET + FILE_TEXT_OFFSET_V1_8,
mod_size);
struct lib_manager_mod_ctx *ctx = lib_manager_get_mod_ctx(module_id);
/* Identify if this is the first time loading this module */
struct llext_load_param ldr_parm = {!ctx->segment_size[SOF_MAN_SEGMENT_TEXT]};
Expand Down
6 changes: 5 additions & 1 deletion src/samples/audio/smart_amp_test.toml
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
#ifndef LOAD_TYPE
#define LOAD_TYPE "0"
#endif

REM # smart amp test module config
[[module.entry]]
name = "SMATEST"
uuid = "167A961E-8AE4-11EA-89F1-000C29CE1635"
affinity_mask = "0x1"
instance_count = "1"
domain_types = "0"
load_type = "0"
load_type = LOAD_TYPE
init_config = "1"
module_type = "0xD"
auto_start = "0"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Copyright (c) 2023 Intel Corporation.
# SPDX-License-Identifier: Apache-2.0

# FIXME: This *WILL* be converted to add_llext_target() as long as that's
# reasonably possible, and if it isn't, a *significant* effort will be made to
# make that possible

cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(smart_amp_test)
Expand All @@ -10,6 +14,16 @@ SET_PROPERTY(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
set(MODULE "smart_amp_test")
cmake_path(SET SOF_BASE NORMALIZE ${PROJECT_SOURCE_DIR}/../../../..)

file(STRINGS ${CMAKE_CURRENT_LIST_DIR}/../${MODULE}.toml uuids REGEX "^[ \t]*uuid *=")

file(WRITE ${PROJECT_BINARY_DIR}/llext.uuid "")

foreach(line IN LISTS uuids)
# extract UUID value - drop the 'uuid = ' part of the assignment line
string(REGEX REPLACE "^[ \t]*uuid *= \"([0-9A-F\-]*)\"" "\\1" uuid ${line})
file(APPEND ${PROJECT_BINARY_DIR}/llext.uuid "${uuid}\n")
endforeach()
Copy link
Member

Choose a reason for hiding this comment

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

why is this a smart_amp specific operation? Are we going to replicate this for each and every module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart so far this CMakeLists.txt would be largely repeated for each new module, yes. But we plan to switch to using recently added Zephyr cmake support for LLEXT code ASAP

Copy link
Collaborator

Choose a reason for hiding this comment

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

But we plan to switch to using recently added Zephyr cmake support for LLEXT code ASAP

Corresponding FIXME now added in later commit, thanks! (I reviewed commits one by one and initially missed it)


add_library(${MODULE} SHARED)

target_sources(${MODULE} PRIVATE
Expand Down Expand Up @@ -55,12 +69,12 @@ target_compile_options(${MODULE} PRIVATE

if("${ZEPHYR_TOOLCHAIN_VARIANT}" STREQUAL "zephyr")
set(MODULE_LINKER_PARAMS -nostdlib -nodefaultlibs)
set(EXTRA_LINKED_PARAMS -shared)
set(COPY_CMD ${CMAKE_STRIP} -R .xt.* -o ${MODULE}_out.so ${MODULE}_llext.so)
set(EXTRA_LINKER_PARAMS -shared)
set(COPY_CMD ${CMAKE_STRIP} -R .xt.* -o ${MODULE}.so ${MODULE}_pre.so)
else()
set(MODULE_LINKER_PARAMS -nostdlib -nodefaultlibs -r)
set(EXTRA_LINKED_PARAMS)
set(COPY_CMD ${CMAKE_OBJCOPY} -R .xt.* ${MODULE}_llext.so ${MODULE}_out.so)
set(EXTRA_LINKER_PARAMS)
set(COPY_CMD ${CMAKE_OBJCOPY} -R .xt.* ${MODULE}_pre.so ${MODULE}.so)
endif()

Copy link
Member

Choose a reason for hiding this comment

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

you should split code, script changes. Not sure why they belong in the same patch?

Also there's no reason why smart_amp should be included here, this is a separate addition IMHO.

Copy link
Collaborator Author

@lyakh lyakh Apr 10, 2024

Choose a reason for hiding this comment

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

@plbossart smart-amp is currently the only component in "main," that can be built as a module. So while adding generic relocatable format support to SOF, we also add it to smart-amp, but this can be split, yes

target_link_options(${MODULE} PRIVATE
Expand All @@ -69,10 +83,14 @@ target_link_options(${MODULE} PRIVATE

add_custom_command(OUTPUT lib${MODULE}_out.so
DEPENDS ${MODULE}
COMMAND ${CMAKE_C_COMPILER} -E ${CMAKE_CURRENT_LIST_DIR}/llext.toml.h -P -DREM=
-I${SOF_BASE} -I${SOF_BASE}src
-imacros ../include/generated/autoconf.h
-o rimage_config.toml
COMMAND ${SOF_BASE}scripts/llext_link_helper.py
-f lib${MODULE}.so -t "0xa06ca000" ${CMAKE_C_COMPILER} --
${MODULE_LINKER_PARAMS} ${EXTRA_LINKED_PARAMS} -fPIC
-o ${MODULE}_llext.so $<TARGET_OBJECTS:${MODULE}>
${MODULE_LINKER_PARAMS} ${EXTRA_LINKER_PARAMS} -fPIC
-o ${MODULE}_pre.so $<TARGET_OBJECTS:${MODULE}>
Copy link
Member

Choose a reason for hiding this comment

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

problem statement/commit message:

a) why do we need to sign modules?
b) what are the rules for selecting the key used by different modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart here the same key is used as the one for the base firmware. In principle any key, accepted by the DSP can be used. Why - for security, for the same reason why we sign the base firmware

Copy link
Member

Choose a reason for hiding this comment

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

@lyakh we do need the ability to specify that alternate key for modules that are not part of the base firmware.

Actually maybe it's more complicated even, e.g. if Intel releases modules then presumably they are signed with the same key than the base firmware, but a 3rd party would be required to use a separate key, so the key would need to be specified on a module-by-module basis.

It's not 9am and that already gives me a migraine.

Copy link
Collaborator

@marc-hb marc-hb Apr 11, 2024

Choose a reason for hiding this comment

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

so the key would need to be specified on a module-by-module basis.

Right.

I don't mind having a "demo" with some hacks and hard-coding for early testing purposes but we need:

  • a mutually agreed, written down design of how this should look in the future.
  • Clearly marking temporary hacks with the appropriate TODOs and FIXMEs so readers don't waste hours and hours wondering why things are done this way and trying to reverse-engineer a design that does not exist (cause something is just a temporary hack)

Otherwise we have an instant maintenance nightmare.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so the key would need to be specified on a module-by-module basis.

Right.

@plbossart @marc-hb would 3rd parties even use the xtensa-build-zephyr.sh script to build their modules?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plbossart @marc-hb would 3rd parties even use the xtensa-build-zephyr.sh script to build their modules?

You tell us! Either in some documentation or in the source code. Then everyone can review and agree or disagree. At the end we're all on the same page.

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 I feel reaching a bit beyond the scope of this work, but let's say - some will and some won't. Those who won't - they'll work out their own way of doing this.
Those who will, they will integrate their modules into their local copy of sof.git in order to be able to use the script. They might or might not want to build both a base image for the distribution and any modules within the same build. If they do - I see no reason for them to use different keys. If they don't, i.e. if they only want to build their modules to be passed to an integrator, who will build a base image with their own key - in that case they anyway will execute a complete build, but will only use their modules from it and discard the rest. Either way I so far see no case when it would be essential for anyone to be able to run a single build while signing different parts of it with different keys. If for some unimaginable reason somebody has to do that, I'd say, that this is unsupported which unfortunately for them will mean, that they have to build multiple times - once with each key. And I really don't see this as an insurmountable problem.

Copy link
Member

Choose a reason for hiding this comment

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

ack - beyond scope of this PR. All will be signed with a single key initially.

COMMAND ${COPY_CMD}
COMMAND_EXPAND_LISTS
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <tools/rimage/config/platform.toml>
#define LOAD_TYPE "2"
#include "../smart_amp_test.toml"

[module]
Expand Down
4 changes: 2 additions & 2 deletions zephyr/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -794,8 +794,8 @@ if(CONFIG_IPC_MAJOR_3)
)
elseif(CONFIG_IPC_MAJOR_4)
if(CONFIG_SAMPLE_SMART_AMP STREQUAL "m")
add_subdirectory(${SOF_SAMPLES_PATH}/audio/smart_amp_llext
${PROJECT_BINARY_DIR}/smart_amp_llext)
add_subdirectory(${SOF_SAMPLES_PATH}/audio/smart_amp_test_llext
${PROJECT_BINARY_DIR}/smart_amp_test_llext)
Copy link
Member

Choose a reason for hiding this comment

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

why the rename? was this a mistake that should be corrected separated, something done in a previous patch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart previously this name wasn't a problem, since each module had its own supporting processing code, so names could be arbitrary. Now we need automation, so names must be unified as ${MODULE}_llext

add_dependencies(app smart_amp_test_llext)
elseif(CONFIG_SAMPLE_SMART_AMP)
zephyr_library_sources(
Expand Down