intel_adsp: move west sign from west flash to earlier west build #54700

marc-hb · 2023-02-10T08:55:16Z

Invoking west sign in west build accelerates twister because west build is run in parallel, see rationale in superseded and very different (CMake-based) PR #52942

intel_adsp: Add signed image target to cmake #52942.

To maximize backwards compatibility:

west sign is optional in west build
west flash will sign (again) if any rimage --option is passed

This PR started with "v3", v1 and v2 happened in #54189. v3 is significantly smaller and simpler than v2

Should fix thesofproject/sof#6917

Unlike #52942, this moves the rimage configuration logic not to CMake but to west sign configuration instead.

You should read and understand #52942 before looking at this.

Also take a look at thesofproject/sof#6917

Signed-off-by: Marc Herbert [email protected]

mbolivar-nordic

Thanks for the update. The overall design makes sense to me. I would ask that you think about a separate rimage-extra-args instead of tool-extra-args, so that we could add imgtool-extra-args in the future. This in turn would enable quick switching between west sign --tool imgtool and west sign --tool rimage in situations where users have to do both.

Zero functional change. Next commit will build rimage _before_ building the firmware which will be required when `west build` starts signing by default (zephyrproject-rtos/zephyr#54700) Signed-off-by: Marc Herbert <[email protected]>

This will be required when signing is transferred to `west build`, see zephyrproject-rtos/zephyr#54700 Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2023-02-24T16:15:23Z

I'm guessing the failures are because CI does not have rimage. Maybe I'll make signing at build time optional, downgrade a missing rimage to a warning (and make sure there's a good error message at later flash time)

Zero functional change. Next commit will build rimage _before_ building the firmware which will be required when `west build` starts signing by default (zephyrproject-rtos/zephyr#54700) Signed-off-by: Marc Herbert <[email protected]>

This will be required when signing is transferred to `west build`, see zephyrproject-rtos/zephyr#54700 Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2023-03-02T01:59:22Z

I just discovered the pretty hard way[*] that CONFIG_CLEANUP_INTERMEDIATE_FILES=y voids any twister performance benefit brought by this PR.

CONFIG_CLEANUP_INTERMEDIATE_FILES=y was added by commit 7dc1ef0 and a few, similar other commits.

Workaround:

# only the first time! Passing extra CMake arguments rebuilds from scratch always
west build -b intel_adsp_cavs15 samples/hello_world/ -- -DCONFIG_CLEANUP_INTERMEDIATE_FILES=n
west build

prj.conf or some overlay can also do this.

The example above increases the build directory size from 15M to 16M.

[*] out of desperation I ended up comparing generated/configs.c across working and non working boards.

marc-hb · 2023-03-02T06:42:45Z

I just discovered the pretty hard way[*] that CONFIG_CLEANUP_INTERMEDIATE_FILES=y voids any twister performance benefit brought by this PR.

intel_adsp: cmake: add_custom_command(.mod) to fix incremental build #55347 is also required to fix the incremental build but that one was easy and expected.

testing zephyrproject-rtos/zephyr#54700 Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2023-03-16T02:05:21Z

removing all those [west flash] options makes it less flexible and breaks existing users depending on those options.

Yes, from the beginning the idea was to stop west flash from signing, transferred to west build instead. The much earlier #52942 was also dropping these signing options too with a... "little" difference: in #52942 west flash was silently dropping signing options :-)

In v4 bd4a5aecd63031 I think I found a simple way to preserve backwards compatibility and keep these options while achieving the other goals, especially signing in parallel => west flash now (re-)signs if any signing option was passed to it, otherwise it does not.

@nashif could you try this latest version? Thanks!

Failures fixed

nashif · 2023-04-09T10:42:22Z

you need to rebase.

Don't force 99% of the users to pass the same value. Signed-off-by: Marc Herbert <[email protected]>

No functional change yet, will be used in the next commits. Signed-off-by: Marc Herbert <[email protected]>

The --tool parameter is not required anymore thanks a [sign] entry in west config like this one: ``` [sign] tool = imgtool ``` Signed-off-by: Marc Herbert <[email protected]>

Reduce duplication, no functional change except for a shorter log statement with slightly less information. This is required by the next commits. Signed-off-by: Marc Herbert <[email protected]>

Add a 3rd option besides --tool-path and $PATH Signed-off-by: Marc Herbert <[email protected]>

Make RIMAGE_SIGN_KEY a CMake CACHE variable so `west sign` can find it and use it as a default value. Fixes thesofproject/sof#6917 Signed-off-by: Marc Herbert <[email protected]>

Zero functional change, pure preparation for the next commit. Signed-off-by: Marc Herbert <[email protected]>

We never want to leave stale outputs behind after failing. Signed-off-by: Marc Herbert <[email protected]>

Moving `west sign` from `west flash` to `west build` for rimage has multiple advantages (including a bit more consistency with imgtool). However it makes `west build` fail when rimage is missing. To avoid forcing every CI and developer who never used it before to install rimage, make signing optional when passing new `west sign` option --if-tool-available. A clear warning is printed. Signed-off-by: Marc Herbert <[email protected]>

Invoking `west sign` in `west build` accelerates twister because `west build` is run in parallel, see rationale in superseded and very different (CMake-based) PR zephyrproject-rtos#52942. To maximize backwards compatibility: - `west sign` is optional in `west build` - `west flash` will sign (again) if any rimage --option is passed Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2023-04-09T23:50:58Z

Thanks @nashif for spotting this, looks like it's not possible to be notified of new conflicts without... Slack! ploomber/contributing#16
EDIT: upvote https://github.com/orgs/community/discussions/17258

So v5 b3b327d is a pure rebase, no code change besides resolving the minor board.cmake conflicts with the removal of older platforms in PR #56588. All tests passed in https://github.com/zephyrproject-rtos/zephyr/actions/runs/4652415270/jobs/8232619338?pr=54700 and internal tests passed as well.

The force-push lost all code reviews, would the previous reviewers be kind enough to re-approve? Sorry for the inconvenience.

The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - thesofproject#155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>

The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - #155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>

marc-hb mentioned this pull request Feb 10, 2023

WIP: add new west sign configuration file #54189

Closed

marc-hb requested review from teburd and mbolivar-nordic February 10, 2023 08:56

mbolivar-nordic reviewed Feb 10, 2023

View reviewed changes

marc-hb requested review from kv2019i, lyakh and juimonen February 10, 2023 22:04

teburd mentioned this pull request Feb 14, 2023

intel_adsp: Add signed image target to cmake #52942

Closed

marc-hb mentioned this pull request Feb 24, 2023

xtensa-build-platforms.py: build_rimage() only once at start thesofproject/sof#7170

Merged

marc-hb force-pushed the build-time-rimage2 branch from a16e097 to 10ad19b Compare February 24, 2023 08:01

marc-hb changed the title ~~WIP: move rimage earlier to west build time instead of west flash~~ intel_adsp: move west sign from west flash to earlier west build Feb 24, 2023

marc-hb force-pushed the build-time-rimage2 branch from 10ad19b to 49bf97f Compare March 3, 2023 01:59

marc-hb mentioned this pull request Mar 3, 2023

[TEST IGNORE] testing west sign move to west build thesofproject/sof#7205

Closed

marc-hb added a commit to marc-hb/sof that referenced this pull request Mar 3, 2023

[TEST IGNORE] testing west sign move to west build

44dd3f2

testing zephyrproject-rtos/zephyr#54700 Signed-off-by: Marc Herbert <[email protected]>

marc-hb force-pushed the build-time-rimage2 branch from 49bf97f to 21ecaed Compare March 3, 2023 05:18

marc-hb marked this pull request as ready for review March 3, 2023 05:35

marc-hb requested review from nashif, dcpleung and andyross as code owners March 3, 2023 05:35

zephyrbot added area: Documentation platform: Intel ADSP Intel Audio platforms area: West West utility labels Mar 3, 2023

marc-hb force-pushed the build-time-rimage2 branch from ec424fb to bd4a5ae Compare March 16, 2023 00:48

marc-hb requested a review from nashif March 16, 2023 03:28

This was referenced Mar 22, 2023

[SKIP CI] .github/zephyr.yml: add a temporary submodules/west consistency check thesofproject/sof#7320

Merged

Revert "xtensa: remove ELF section address rewriting" #56099

Merged

teburd previously approved these changes Apr 5, 2023

View reviewed changes

marc-hb added 10 commits April 9, 2023 18:47

west: sign: rimage: add default value '-c sof/rimage/config'

155ece5

Don't force 99% of the users to pass the same value. Signed-off-by: Marc Herbert <[email protected]>

west: sign: add ability to read parameters from west config

ee61bc6

No functional change yet, will be used in the next commits. Signed-off-by: Marc Herbert <[email protected]>

west: sign: add west config [sign].tool = rimage/imgtool

b8704a4

The --tool parameter is not required anymore thanks a [sign] entry in west config like this one: ``` [sign] tool = imgtool ``` Signed-off-by: Marc Herbert <[email protected]>

west: sign: rimage: simplify rimage --tool-data code

4bc45d1

Reduce duplication, no functional change except for a shorter log statement with slightly less information. This is required by the next commits. Signed-off-by: Marc Herbert <[email protected]>

west: sign: add west config [rimage].path = /path/to/rimage

2112731

Add a 3rd option besides --tool-path and $PATH Signed-off-by: Marc Herbert <[email protected]>

west: sign: add new west config [rimage].extra-args and a default key

c94070b

Make RIMAGE_SIGN_KEY a CMake CACHE variable so `west sign` can find it and use it as a default value. Fixes thesofproject/sof#6917 Signed-off-by: Marc Herbert <[email protected]>

west: sign: define filename constants earlier in rimage signer

6954d89

Zero functional change, pure preparation for the next commit. Signed-off-by: Marc Herbert <[email protected]>

west: sign: clean any stale rimage output

c83c495

We never want to leave stale outputs behind after failing. Signed-off-by: Marc Herbert <[email protected]>

marc-hb dismissed teburd’s stale review via b3b327d April 9, 2023 20:39

marc-hb force-pushed the build-time-rimage2 branch from bd4a5ae to b3b327d Compare April 9, 2023 20:39

teburd approved these changes Apr 10, 2023

View reviewed changes

nashif approved these changes Apr 11, 2023

View reviewed changes

nashif merged commit fad2da3 into zephyrproject-rtos:main Apr 11, 2023

marc-hb deleted the build-time-rimage2 branch April 11, 2023 06:14

marc-hb mentioned this pull request Apr 18, 2023

cmake: remove "install" target thesofproject/rimage#159

Merged

marc-hb mentioned this pull request Apr 26, 2023

add environment support for defaulting arguments zephyrproject-rtos/west#429

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intel_adsp: move west sign from west flash to earlier west build #54700

intel_adsp: move west sign from west flash to earlier west build #54700

marc-hb commented Feb 10, 2023 •

edited

Loading

mbolivar-nordic left a comment

marc-hb commented Feb 24, 2023

marc-hb commented Mar 2, 2023 •

edited

Loading

marc-hb commented Mar 2, 2023

marc-hb commented Mar 16, 2023

nashif commented Apr 9, 2023

marc-hb commented Apr 9, 2023 •

edited

Loading

intel_adsp: move west sign from west flash to earlier west build #54700

intel_adsp: move west sign from west flash to earlier west build #54700

Conversation

marc-hb commented Feb 10, 2023 • edited Loading

mbolivar-nordic left a comment

Choose a reason for hiding this comment

marc-hb commented Feb 24, 2023

marc-hb commented Mar 2, 2023 • edited Loading

marc-hb commented Mar 2, 2023

marc-hb commented Mar 16, 2023

nashif commented Apr 9, 2023

marc-hb commented Apr 9, 2023 • edited Loading

marc-hb commented Feb 10, 2023 •

edited

Loading

marc-hb commented Mar 2, 2023 •

edited

Loading

marc-hb commented Apr 9, 2023 •

edited

Loading