From fad2da39aaaf8117181643ff97bc3be376ca6fc7 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Fri, 24 Feb 2023 07:13:23 +0000 Subject: [PATCH] intel_adsp: move `west sign` from `west flash` to earlier `west build` 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. 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 --- doc/develop/west/sign.rst | 51 +++++++++++++++++++++ scripts/west_commands/runners/intel_adsp.py | 30 +++++++----- soc/xtensa/intel_adsp/common/CMakeLists.txt | 19 ++++++++ 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/doc/develop/west/sign.rst b/doc/develop/west/sign.rst index d0d17992b3d83f..13dfaa4d0d5092 100644 --- a/doc/develop/west/sign.rst +++ b/doc/develop/west/sign.rst @@ -117,3 +117,54 @@ signing system or extending the default behaviour. .. _imgtool: https://pypi.org/project/imgtool/ + + +rimage +****** + +rimage configuration uses a different approach that does not rely on Kconfig or CMake +but on :ref:`west config` instead, similar to +:ref:`west-building-cmake-config`. + +Signing involves a number of "wrapper" scripts stacked on top of each other: ``west +flash`` invokes ``west build`` which invokes ``cmake`` and ``ninja`` which invokes +``west sign`` which invokes ``imgtool`` or `rimage`_. As long as the signing +parameters desired are the default ones and fairly static, these indirections are +not a problem. On the other hand, passing ``imgtool`` or ``rimage`` options through +all these layers can causes issues typical when the layers don't abstract +anything. First, this usually requires boilerplate code in each layer. Quoting +whitespace or other special characters through all the wrappers can be +difficult. Reproducing a lower ``west sign`` command to debug some build-time issue +can be very time-consuming: it requires at least enabling and searching verbose +build logs to find which exact options were used. Copying these options from the +build logs can be unreliable: it may produce different results because of subtle +environment differences. Last and worst: new signing feature and options are +impossible to use until more boilerplate code has been added in each layer. + +To avoid these issues, ``rimage`` parameters can bet set in ``west config`` +instead. Here's a ``workspace/.west/config`` example: + +.. code-block:: ini + + [sign] + # Not needed when invoked from CMake + tool = rimage + + [rimage] + # Quoting is optional and works like in Unix shells + # Not needed when rimage can be found in the default PATH + path = "/home/me/zworkspace/build-rimage/rimage" + + # Not needed when using the default development key + extra-args = -i 4 -k 'keys/key argument with space.pem' + +In order to support quoting, values are parsed by Python's ``shlex.split()`` like in +:ref:`west-building-cmake-args`. + +The ``extra-args`` are passed directly to the ``rimage`` command. The example +above has the same effect as appending them on command line after ``--`` like this: +``west sign --tool rimage -- -i 4 -k 'keys/key argument with space.pem'``. In case +both are used, the command-line arguments go last. + +.. _rimage: + https://github.com/thesofproject/rimage diff --git a/scripts/west_commands/runners/intel_adsp.py b/scripts/west_commands/runners/intel_adsp.py index 82fa4baf2ffeeb..4a0c83c2f2c067 100644 --- a/scripts/west_commands/runners/intel_adsp.py +++ b/scripts/west_commands/runners/intel_adsp.py @@ -31,7 +31,8 @@ def __init__(self, default_key, key, pty, - tool_opt + tool_opt, + do_sign, ): super().__init__(cfg) @@ -50,6 +51,7 @@ def __init__(self, self.key = os.path.join(DEFAULT_KEY_DIR, default_key) self.tool_opt_args = tool_opt + self.do_sign = do_sign @classmethod def name(cls): @@ -82,6 +84,18 @@ def tool_opt_help(cls) -> str: @classmethod def do_create(cls, cfg, args): + # We now have `west flash` -> `west build` -> `west sign` so + # `west flash` -> `west sign` is not needed anymore; it's also + # slower because not concurrent. However, for backwards + # compatibility keep signing here if some explicit rimage + # --option was passed. Some of these options may differ from the + # current `west sign` configuration; we take "precedence" by + # running last. + do_sign = ( + args.rimage_tool != DEFAULT_RIMAGE_TOOL or + args.config_dir != DEFAULT_CONFIG_DIR or + args.key is not None + ) return IntelAdspBinaryRunner(cfg, remote_host=args.remote_host, rimage_tool=args.rimage_tool, @@ -89,21 +103,15 @@ def do_create(cls, cfg, args): default_key=args.default_key, key=args.key, pty=args.pty, - tool_opt=args.tool_opt + tool_opt=args.tool_opt, + do_sign=do_sign, ) def do_run(self, command, **kwargs): self.logger.info('Starting Intel ADSP runner') - # Always re-sign because here we cannot know whether `west - # flash` was invoked with `--skip-rebuild` or not and we have no - # way to tell whether there was any code change either. - # - # Signing does not belong here; it should be invoked either from - # some CMakeLists.txt file or an optional hook in some generic - # `west flash` code but right now it's in neither so we have to - # do this. - self.sign(**kwargs) + if self.do_sign: + self.sign(**kwargs) if re.search("intel_adsp_cavs", self.platform): self.require(self.cavstool) diff --git a/soc/xtensa/intel_adsp/common/CMakeLists.txt b/soc/xtensa/intel_adsp/common/CMakeLists.txt index c9ed156b891b22..77a0de3e614dda 100644 --- a/soc/xtensa/intel_adsp/common/CMakeLists.txt +++ b/soc/xtensa/intel_adsp/common/CMakeLists.txt @@ -118,3 +118,22 @@ add_custom_command( $ ) endif() + + +# west sign +add_custom_target(zephyr.ri ALL + DEPENDS ${CMAKE_BINARY_DIR}/zephyr/zephyr.ri +) + +add_custom_command( + OUTPUT ${CMAKE_BINARY_DIR}/zephyr/zephyr.ri + COMMENT "west sign --if-tool-available --tool rimage ..." + # Use --if-tool-available so we don't force every CI to install + # rimage. We don't want to break build-only and other tests that don't + # require signing. When rimage is missing, `west flash` fails with a + # clear "zephyr.ri missing" error with an "rimage not found" warning + # from west sign immediately before it. + COMMAND west sign --if-tool-available --tool rimage --build-dir ${CMAKE_BINARY_DIR} + DEPENDS gen_modules + ${CMAKE_BINARY_DIR}/zephyr/boot.mod ${CMAKE_BINARY_DIR}/zephyr/main.mod +)