xtensa-build-zephyr: pass sof build version to rimage

[dnikodem]

Rimage expects to receive fw_ver_build to prepare the manifest.

Signed-off-by: Damian Nikodem damian.nikodem@intel.com

[mwasko]

SOF CI

[aborisovich]

This solution is generally bad - we have a version variables in CMake, we write them to header file and then we parse the header file to get variables back to CMake with regexes...
I do not like this at all and looking for better solutions to passing version to rimage.
I've tried using configure_file to create a sof_versions.h.in file replacing existing sof_check_version_h function but failed as Xtensa compilers (both commercial and from Zephyr SDK) do not have extensions to actually replace #cmakedefine preprocessor (but that was just a refactor attempt). Maybe we can try something better with west sign command overload in sof?

@dnikodem where did you find informations about west sign -b flag? Can't get it anywhere...

[marc-hb]

95% copy/paste/diverge of the function immediately above for no obvious reason.

[marc-hb]

This solution is generally bad - we have a version variables in CMake, we write them to header file and then we parse the header file to get variables back to CMake with regexes...

Yes this is a bit of a hack but it's been working fine and hasn't caused any problem for months. We all have more urgent things to do but if you have time: the real problem is west sign. We shouldn't use west sign, we should sign directly from CMake just like XTOS still does.

I think we could not sign directly from CMake in the past because SOF was not a real Zephyr application, SOF used to hijack zephyr/samples/audio/sof and zephyr/samples/ cannot have a dependency on rimage; they're just samples. But these samples/sof days are over, since commit 00c407f SOF is now a real Zephyr application and having a CMake-time dependency on rimage would be fine.

I've tried using [configure_file (https://cmake.org/cmake/help/latest/command/configure_file.html)...

I'm afraid a configure_file is generated at CMake configure-time, not at ninja build-time. So it would not be suitable.

[marc-hb]

We shouldn't use west sign, we should sign directly from CMake just like XTOS still does.

Another problem caused by west sign: zephyrproject-rtos/zephyr@9782b2cc0597fbe

[dnikodem]

About "west sign -b" - according to west sign help, the west allows us to pass any parameter to the rimage tool directly after the "--" character (we are passing in the same way fw version with '-f' flag).
According to the rimage code base, the rimage tool recognizes several command flags. One of them is '-b'.

[dnikodem]

95% copy/paste/diverge of the function immediately above for no obvious reason.

I agree with you Marc - this is a copy/paste of the above function. I did not combine them into a single function due to the fact that rimage uses the fw version and the build version separately. I can merge them into one function, and separate the fw version and the build version when submitting the command. Thanks

[lgirdwood]

@dnikodem I'm guessing you will validate on Windows ?
@marc-hb I assume CI will validate on Linux ?

[aborisovich]

I'm afraid a configure_file is generated at CMake configure-time, not at ninja build-time. So it would not be suitable.

So is sof_check_version_h (despite its description claiming otherwise) - it runs at configure time.
"Running at build time" is a direct effect of the updating sof_versions.h timestamp during configure time what is default Cmake behavior. In short - cofigure_file would work exactly the same as the current sof_check_version_h function.
I even found following code in zephyrs gen_version_h.cmake:

and it keeps me wondering how does it work with Xtensa compilers? (or it probably does not...).
But as @marc-hb indicated there are more urgent cases to deal with right now.
Back to the topic - next idea:
Use CMake Cache to write a variables from SOF to Rimage directly.
According to Cmake cache - Setting Initial Values for CMake we can simple write cache variables with build version to rimage project build tree. This way running west sign will use the build numbers provided in cache despite lacking -b flag in the invocation. This would require 0 modifications to xtensa-build-zephyr.py.
Silly me - -b argument is to already build rimage executable...

[aborisovich]

We shouldn't use west sign, we should sign directly from CMake just like XTOS still does.

Well, that would be actually the easiest solution. Just use add_custom_target that DEPENDS on rimage_ep target (this target is defined in src/arch/xtensa/CMakeLists.txt: ExternalProject_Add(rimage_ep... Then you can trigger signing without using west at all with all needed cmake variables.
Then we could remove west sign command from xtensa-build-zephyr.py and forget about this problem for now.

[lgirdwood]

Then we could remove west sign command from xtensa-build-zephyr.py and forget about this problem for now.

I think only Intel and AMD sign FW currently. We should migrate to west though, and is the -b reuse between rimage and west stopping us today ?

[aborisovich]

Then we could remove west sign command from xtensa-build-zephyr.py and forget about this problem for now.

I think only Intel and AMD sign FW currently. We should migrate to west though, and is the -b reuse between rimage and west stopping us today ?

No, nothing is stoping us. I'm not blocking anything just trying to provide better quality solutions because this one is not very good. As far as I spoke with @dnikodem this PR is not super urgent one or anything thus I'm trying to take a step in right direction. I'm not sure whether I understood correctly your question.
If you ask "what are the issues with integrating west sign with sof" it can be described in following statement:
"It does not make sense to use command (west sign) that expects manual input of the build version while your build system defines a build version in the header that should be used in the rimage". Is it blocking anything? No.
To sum up:
Existing west sign implementation from Zephyr does not make sense in SOF.
We should either provide our own extension commands to west where we would override script for west sign or get rid of west sign at all. It technically would also be correct as you can not currently build a firmware image without signing (only elf file is created). So it makes sense that our west build would also do west sign "inside" as it is a part of creating actually usable output file. @marc-hb what do you think?

outdated

[marc-hb]

I'm fairly confident you don't need these global declarations. Otherwise looks good to me.

EDIT: just realized the first global was already there. Still, please take the opportunity to fix this while you're changing this code (or prove me wrong).

EDIT2: wrong!

[marc-hb]

I think only Intel and AMD sign FW currently.

This has been already solved with some CMake conditionals in the XTOS build, we could do more or less the same thing in the Zephyr application build.

@marc-hb what do you think?

I think we're on the same page for most or all topics. Good brainstorming for the longer term, in the mean time let's get this small and quick change merged.

[marc-hb]

just realized the first global was already there. Still, please take the opportunity to fix this while you're changing this code (or prove me wrong).

I was wrong, apologies. the global keyword is needed (and confusing IMHO). The code in your previous commit bfc29a2db30b55e was correct, I should have approved it. Please restore it. Sorry for the hassle.

In your current commit 3371aad526b8c you made the variables local and removed the caching feature. I wasn't asking you to remove caching, I just thought the keyword was not needed to achieve caching. But the global keyword is required to implement caching, my bad.

I find global confusing because it is not required to read a global variable, BUT it is required to write to a global variable!

https://python-course.eu/python-tutorial/global-local-variables-namespaces.php

You can also (mis?)use global to declare a global inside a function. I wrongly assumed it was it's only purpose.

PS: global is not required to use mutator methods like mylist.append(elt); what a mess. I guess this mess achieves one of the purposes: discourage us from using globals.

[marc-hb]

Unrelated suspend/resume failure in https://sof-ci.01.org/sofpr/PR6276/build1560/devicetest/
Unrelated license server failure in https://sof-ci.01.org/sofpr/PR6276/build1560/build/cnl_xcc_error.txt

[marc-hb]

SOFCI TEST

EDIT: everything green now including https://sof-ci.01.org/sofpr/PR6276/build1565/devicetest and https://sof-ci.01.org/sofpr/PR6276/build1565/build