Repository navigation
[FEATURE] pass vendor specific FW signing options to west sign #6917
Description
Is your feature request related to a problem? Please describe.
On Intel platforms, we need to sign production releases with west sign options "-i" and "-p". Currently SOF's xtensa-build-zephyr.py does not allow passing these options.
Describe the solution you'd like
Option to pass a list of options that suits multivendor usage with "west sign".
@marc-hb May 2022 EDIT: this almost complete. It has NOT been implemented by "passing a list of options to xtensa-build-zephyr.py". It's using west config for west sign instead.
cc:
Activity
kv2019i commentedon Jan 5, 2023
FYI @abonislawski @marc-hb @lgirdwood @juimonen
marc-hb commentedon Jan 5, 2023
I will try to address this (the small rest of) this week. I think the very first step is to actually review this PR in detail:
lgirdwood commentedon Jan 6, 2023
@nashif fyi
marc-hb commentedon Jan 9, 2023
So as I suspected, zephyrproject-rtos/zephyr#52942 is trying to solve this problem (among others). I just finished a (detailed) review of it.
If upstream Zephyr solves this problem with some new configuration file or something and we add some hack in
xtensa-build-zephyr.pyat the same time then we're very likely to end-up in some forward/backward compatibility nightmare with upstream.So temporarily for this 2.4 release would it be possible to (re-)run
west signmanually with the modifiedrimageparameters? AFAIK production releases are NOT automated anyway. This will overwrite the.rifile in thebuild-tgldirectory, then it only needs to be copied tobuild-sof-staging/.After running
xtensa-build-zephyr.py, scrolling up 1 or 2 screens shows the exactwest signcommand it has run.kv2019i commentedon Jan 9, 2023
@marc-hb Let's leave the 2.4 release aside and consider just the specific question of ability to pass signing options. I think a generic ability to pass FW signing options seems like a generic thing we will need now and future. I don't see Zephyr PR52942 changing this.
If I look up Zephyr docs on west sign:
https://docs.zephyrproject.org/latest/develop/west/sign.html
... so generic ability to pass extra options from xtensa-build-zephyr.py to "ARGS_FOR_YOUR_TOOL" seems like a good capability to have as it's something "west sign" tool has.
If signing is merged to build step on Zephyr side, then it would seem the extra options would need to be passed to build step, but otherwise still applicable.
Whether changing the signing options via the script parameters is a good approach or not, is another topic, but it's something SOF has in the past offered and at least the Intel binary release process has dependent upon. So moving to e.g. some in-tree configuration file to describe different signing targets, would be a very large change to the release model and something that should be discussed separately.
marc-hb commentedon Jan 9, 2023
Yes.
It does - and more.
The configuration file I suggested in Zephyr 52942 does not have to be in git. It's just a suggestion, work in progress.
2.4 workaround In the meantime:
marc-hb commentedon Feb 2, 2023
Proof of concept available in draft PR:
20 remaining items
lgirdwood commentedon May 10, 2023
@marc-hb still trending for v2.6 or can we push later ?
marc-hb commentedon May 11, 2023
This should make it to v2.6 depending on SOF maintainer and code review bandwidth:
xtensa-build-zephyr.py: don't extract SOF_BUILD from generated .h file #7546
west: sign: rimage: drop default -c option when the user provides one zephyrproject-rtos/zephyr#57820
soc: xtensa: nxp: invoke
west signatwest buildtime zephyrproject-rtos/zephyr#57912xtensa-build-zephyr.py: stop calling west sign, rely on west build #7684
New WEST_SIGN_OPTS for board-specific customization. intel_adsp_generic.rst update. zephyrproject-rtos/zephyr#58215
aborisovich commentedon May 20, 2023
Ok, I'm looking at all of this and I do not agree with what you are trying to do.
It is not a good capability. Please note, that SOF and Rimage and two different cmake projects.
By passing user input in the
xtensa-build-zephyr.pyto project 1 (SOF) then project 2 (rimage), you are expanding functionalities ofxtensa-build-system.pyscript, which should not exist at all and is a legacy burden from bash project configuration. Scripts are not a part of a build system. What we should do, is pass information to SOF CMake, which will pass information to Rimage CMake during configuration and build. This wayxtensa-build-zephyr.pystill acts like a wrapper only and passes arguments to a single cmake project (SOF).One of bad ideas is treating
westas build system. It is not one. It is a wrapper Zephyr tool. Neither is xtensa-build-zephyr.py script. Importing python west packages and doing stuff around Cmake is not a right/quality solution and will bring many problems in future integration tasks (like loadable modules for example). I'm refering to @marc-hb did in the #7595 . This is not right approach in my opinion. We should build the cmake architecture not scripts.To start doing this, we need following
rimageusing given arguments (either usingwrapper westsign or directly)This way we do not need to parse any data in xtensa-build-zephyr.py as it would be passed in side CMake (this incudes regex reading of version files!).
marc-hb commentedon May 22, 2023
I think this is basically what I did in zephyrproject-rtos/zephyr@5ae7bd84d339 , copying what others did for intel_adsp before me. It does not do everything you suggested yet but I think it's a significant step in the direction you want.
This custom target does invoke
rimagedirectly, it callswest signinstead becausewest signis smarter, more flexible and it is the standard Zephyr interface for this but from a build flow perspective I think it's the same logic as you described.I agree parsing version numbers in
xtensa-build-zephyr.pyis bad but I'm not adding that; "It wasn't me!", I'm only moving this parsing to a different place in the script because it's blocking me from doing something else.Back to this #6917 request, my (almost completed) plan is NOT to make the "vendor specific FW signing options" go through
xtensa-build-zephyr.py, quite the opposite. I've EDITed the description at the top. These options are configured either withwest config / west signor in the CMake files that invoke them.marc-hb commentedon May 23, 2023
One of the complexities is: Zephyr depends on rimage for hardware testing but Zephyr does not depend SOF. So the Zephyr build cannot pass SOF versions to rimage because there may not be any SOF version when Zephyr gets built. I bet this problem can be solved but that's out of scope here and if anything I think the changes have been making go in the right direction. In Zephyr,
west signused to be part ofwest flashONLY, now it can be part of the actual build.aborisovich commentedon May 24, 2023
Ok I get it now, I'm just unhappy that we can not move straight to the point with this.
marc-hb commentedon May 30, 2023
Everything's merged except some documentation update and "bonus" feature zephyrproject-rtos/zephyr#58215
Passing extra rimage arguments is performed like this:
Let's close this issue after it's been used at least once for a real release candidate.
EDIT: in other words, this can be closed once v2.6-rc1 has been signed with a production key and booted on a production-only device.
kv2019i commentedon Jun 22, 2023
We can close this once the 2.6 release is signed utilizing this feature (validating the feature).
marc-hb commentedon Jun 28, 2023
What a surprise (not!): a last-minute discovery that a Quickbuild change is required.
Shouldn't be a big deal but a good reminder that if it's not tested then it does not work.
marc-hb commentedon Jul 12, 2023
After one last minute
rimageregression and panic (thesofproject/rimage#168) we got confirmation from @abonislawski that this feature works as intended. Closing.west: sign: add new west config [rimage].extra-args and a default key