From 039e5ef1b8134a672ffa5b8d3e3264b53e354422 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Fri, 8 Dec 2023 00:34:21 +0000 Subject: [PATCH] intel_adsp: remove rimage sign() from `west flash` `west sign` has been invoked by `west build` (through CMake) since commit fad2da39aaaf, almost one year ago. During that time, this new workflow has been refined and successfully used by at least two vendors, multiple CIs across both SOF and Zephyr and many developers. At the time, the ability to sign from `west flash` was preserved for backwards compatibility. This means rimage parameters can come from many different places at once and that rimage can be invoked twice during a single `west flash` invocation! Now that Zephyr 3.5 has been released, we need to reduce the number of rimage use cases and the corresponding validation complexity and maintenance workload to simplify and accelerate new features like splitting rimage configuration files (#65411) Signed-off-by: Marc Herbert --- boards/common/intel_adsp.board.cmake | 5 -- .../xtensa/intel_adsp_ace15_mtpm/board.cmake | 2 +- boards/xtensa/intel_adsp_cavs25/board.cmake | 2 +- .../doc/intel_adsp_generic.rst | 10 ++- scripts/west_commands/runners/intel_adsp.py | 66 ++++--------------- 5 files changed, 17 insertions(+), 68 deletions(-) delete mode 100644 boards/common/intel_adsp.board.cmake diff --git a/boards/common/intel_adsp.board.cmake b/boards/common/intel_adsp.board.cmake deleted file mode 100644 index 813ece3fb995dc..00000000000000 --- a/boards/common/intel_adsp.board.cmake +++ /dev/null @@ -1,5 +0,0 @@ -# SPDX-License-Identifier: Apache-2.0 - -board_runner_args(intel_adsp "--default-key=${RIMAGE_SIGN_KEY}") - -board_finalize_runner_args(intel_adsp) diff --git a/boards/xtensa/intel_adsp_ace15_mtpm/board.cmake b/boards/xtensa/intel_adsp_ace15_mtpm/board.cmake index 63777239a1d888..e9778da4d8449f 100644 --- a/boards/xtensa/intel_adsp_ace15_mtpm/board.cmake +++ b/boards/xtensa/intel_adsp_ace15_mtpm/board.cmake @@ -6,4 +6,4 @@ board_set_rimage_target(mtl) set(RIMAGE_SIGN_KEY "otc_private_key_3k.pem" CACHE STRING "default in ace15_mtpm/board.cmake") -include(${ZEPHYR_BASE}/boards/common/intel_adsp.board.cmake) +board_finalize_runner_args(intel_adsp) diff --git a/boards/xtensa/intel_adsp_cavs25/board.cmake b/boards/xtensa/intel_adsp_cavs25/board.cmake index 8d7f36e07bd2aa..1bdb2698c12feb 100644 --- a/boards/xtensa/intel_adsp_cavs25/board.cmake +++ b/boards/xtensa/intel_adsp_cavs25/board.cmake @@ -17,4 +17,4 @@ if(CONFIG_BOARD_INTEL_ADSP_CAVS25_TGPH) board_set_rimage_target(tgl-h) endif() -include(${ZEPHYR_BASE}/boards/common/intel_adsp.board.cmake) +board_finalize_runner_args(intel_adsp) diff --git a/boards/xtensa/intel_adsp_cavs25/doc/intel_adsp_generic.rst b/boards/xtensa/intel_adsp_cavs25/doc/intel_adsp_generic.rst index fe97ed6466af0d..6b0a4526fe4009 100644 --- a/boards/xtensa/intel_adsp_cavs25/doc/intel_adsp_generic.rst +++ b/boards/xtensa/intel_adsp_cavs25/doc/intel_adsp_generic.rst @@ -143,12 +143,10 @@ undocumented rimage precedence rules it's best to use only one way at a time. ``boards/my/board/board.cmake``, see example in ``soc/xtensa/intel_adsp/common/CMakeLists.txt`` -For backwards compatibility reasons, you can also pass rimage parameters like -the path to the tool binary as arguments to -``west flash`` if the flash target exists for your board. To see a list -of all arguments to the Intel ADSP runner, run the following after you have -built the binary. There are multiple arguments related to signing, including a -key argument. +Starting with Zephyr 3.6.0, ``west flash`` does not invoke ``west sign`` +anymore and you cannot pass rimage parameters to ``west flash`` anymore. To +see an up-to-date list of all arguments to the Intel ADSP runner, run the +following after you have built the binary: .. code-block:: console diff --git a/scripts/west_commands/runners/intel_adsp.py b/scripts/west_commands/runners/intel_adsp.py index 18b1462ef0516a..f7587331dc36cd 100644 --- a/scripts/west_commands/runners/intel_adsp.py +++ b/scripts/west_commands/runners/intel_adsp.py @@ -4,6 +4,7 @@ '''Runner for flashing with the Intel ADSP boards.''' +import argparse import os import sys import re @@ -14,11 +15,12 @@ from zephyr_ext_common import ZEPHYR_BASE DEFAULT_CAVSTOOL='soc/xtensa/intel_adsp/tools/cavstool_client.py' -DEFAULT_SOF_MOD_DIR=os.path.join(ZEPHYR_BASE, '../modules/audio/sof') -DEFAULT_RIMAGE_TOOL=shutil.which('rimage') -DEFAULT_CONFIG_DIR=os.path.join(DEFAULT_SOF_MOD_DIR, 'tools/rimage/config') -DEFAULT_KEY_DIR=os.path.join(DEFAULT_SOF_MOD_DIR, 'keys') +class SignParamError(argparse.Action): + 'User-friendly feedback when trying to sign with west flash' + def __call__(self, parser, namespace, values, option_string=None): + parser.error(f'Cannot use "west flash {option_string} ..." any more. ' + + '"west sign" is now called from CMake, see "west sign -h"') class IntelAdspBinaryRunner(ZephyrBinaryRunner): '''Runner front-end for the intel ADSP boards.''' @@ -26,32 +28,19 @@ class IntelAdspBinaryRunner(ZephyrBinaryRunner): def __init__(self, cfg, remote_host, - rimage_tool, - config_dir, - default_key, - key, pty, tool_opt, - do_sign, ): super().__init__(cfg) self.remote_host = remote_host - self.rimage_tool = rimage_tool - self.config_dir = config_dir self.bin_fw = os.path.join(cfg.build_dir, 'zephyr', 'zephyr.ri') self.cavstool = os.path.join(ZEPHYR_BASE, DEFAULT_CAVSTOOL) self.platform = os.path.basename(cfg.board_dir) self.pty = pty - if key: - self.key = key - else: - 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): @@ -65,18 +54,14 @@ def capabilities(cls): def do_add_parser(cls, parser): parser.add_argument('--remote-host', help='hostname of the remote targeting ADSP board') - parser.add_argument('--rimage-tool', default=DEFAULT_RIMAGE_TOOL, - help='path to the rimage tool') - parser.add_argument('--config-dir', default=DEFAULT_CONFIG_DIR, - help='path to the toml config file') - parser.add_argument('--default-key', - help='the default basename of the key store in board.cmake') - parser.add_argument('--key', - help='specify where the signing key is') parser.add_argument('--pty', nargs='?', const="remote-host", type=str, help=''''Capture the output of cavstool.py running on --remote-host \ and stream it remotely to west's standard output.''') + for old_sign_param in [ '--rimage-tool', '--config-dir', '--default-key', '--key']: + parser.add_argument(old_sign_param, action=SignParamError, + help='do not use, "west sign" is now called from CMake, see "west sign -h"') + @classmethod def tool_opt_help(cls) -> str: return """Additional options for run/request service tool, @@ -84,35 +69,15 @@ 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, - config_dir=args.config_dir, - default_key=args.default_key, - key=args.key, pty=args.pty, tool_opt=args.tool_opt, - do_sign=do_sign, ) def do_run(self, command, **kwargs): self.logger.info('Starting Intel ADSP runner') - if self.do_sign: - self.sign(**kwargs) - if re.search("intel_adsp", self.platform): self.require(self.cavstool) self.flash(**kwargs) @@ -120,17 +85,8 @@ def do_run(self, command, **kwargs): self.logger.error("No suitable platform for running") sys.exit(1) - def sign(self, **kwargs): - path_opt = ['-p', f'{self.rimage_tool}'] if self.rimage_tool else [] - sign_cmd = ( - ['west', 'sign', '-d', f'{self.cfg.build_dir}', '-t', 'rimage'] - + path_opt + ['-D', f'{self.config_dir}', '--', '-k', f'{self.key}'] - ) - self.logger.info(" ".join(sign_cmd)) - self.check_call(sign_cmd) - def flash(self, **kwargs): - # Generate a hash string for appending to the sending ri file + 'Generate a hash string for appending to the sending ri file' hash_object = hashlib.md5(self.bin_fw.encode()) random_str = f"{random.getrandbits(64)}".encode() hash_object.update(random_str)