[ranj063]

No description provided.

[marc-hb]

Cc: @KaiChuan-Hsieh from PR #3198 noticed by chance.

[juimonen]

@ranj063 @marc-hb just to confirm that this PR works really nicely for me at least:

Running with our CI docker and specifying absolute paths to meu and symbolic link to openssl:

./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -m /home/sof/work/sof.git/MEU15_TGL/Linux64 -s /home/sof/work/sof.git/MEU15_TGL/openssl -k /home/sof/work/sof.git/MEU15_TGL/private.pem tgl

(absolute paths are paths inside docker, so for example put "pwd" to the build script in meu error branch to figure out)

[lgirdwood]

@juimonen thanks for your confirmation - @ranj063 any update on review comments ?

[ranj063]

@lgirdwood @marc-hb @juimonen this PR is updated now. Sorry for the delay!

[lgirdwood]

@ranj063 can you check the build. CI reporting some issues.

[marc-hb]

Thanks! Just some minor changes that I think could each be done later but because of their number I'm going to "request" them, sorry!

Also, some CI is hosed right now so these changes are a good excuse to force-push a new version some time later hoping CI got back on its feet by then :-)

[marc-hb]

Just some minor changes that I think could each be done later but...

I take that back: see newer comment / request for a "real" change about otc_private_key_3k.pem

[marc-hb]

Please don't expect people to log in to docker. I mean our CI is not doing it, so I really like that I'm building the same way as CI.

Travis CI doesn't sign TGL yet. Jenkins CI does thanks to a private script that predates this one. So as far as TGL is concerned you can't be building TGL the same as CI yet.

CI is obviously not "interactive" like developers are. I totally agree CI and developers should always be using the exact same code, however developers will always use the build differently: in a more fine-grained and controlled way. For instance developers use incremental builds, otherwise they never see warnings at the start of the build. They need to change some flag or some make parameter and they can't do that from xtensa-build-all.sh and even less from ./docker-run.sh. Etc.

so I can't give the environment variable RIMAGE_PRIVATE_KEY outside of the scripts, without modifying them?

You could submit a PR with --env RIMAGE_PRIVATE_KEY addition to docker-run.sh, I don't see why not (are you confirming it works?)

Another option could be to "hardcode" export RIMAGE_PRIVATE_KEY=key somewhere inside your container, maybe some ~/.profile kind of file would work. If you really don't want to know how the build works and treat the container as a complete black box then everything including key configuration should be... contained in it and not awkwardly passed through docker run + 2-3 other layers of indirection.

[juimonen]

@ranj063 @marc-hb @xiulipan so can you give me the command line procedure how to build for tgl, with external key, with gcc, with our sof-docker (as I assume that is the purpose of this whole PR)?

After fighting with this for couple of hours, I need to move to some other platform to do some work.

[marc-hb]

After fighting with this for couple of hours,

I understand most of these hours seem to have been because of the addition of a submodule that broke non-TGL platforms too, which proves it was totally unrelated to this PR.

[juimonen]

@marc-hb "--env RIMAGE_PRIVATE_KEY=path" in docker-run doesn't work for me, compilation goes fine but in meu phase it is giving me empty path to meu tool for some reason. also I don't get sof_tgl_gcc directory anymore, just sof_tgl with empty RIMAGE_PRIVATE_KEY.

[marc-hb]

I understand you don't want to do this eventually but please use docker exec -it sof bash and try to build from inside the container first so we can solve one problem at a time.

[juimonen]

@marc-hb adding this else branch to xtensa-build-all.sh makes it work:

                    if [ -z "$PRIVATE_KEY_OPTION" ]
                    then
                            PRIVATE_KEY_OPTION="-D${SIGNING_TOOL}_PRIVATE_KEY=$pwd/keys/otc_private_key_3k.pem"
                    else
                            PRIVATE_KEY_OPTION="-D${SIGNING_TOOL}_PRIVATE_KEY=$RIMAGE_PRIVATE_KEY"
                    fi

Not sure if that's the proper way but I guess without something similar the RIMAGE_PRIVATE_KEY is not pushed to meu tool.

At least for me doesn't matter are you inside/outside docker.

[lgirdwood]

@juimonen can you create a PR with your update. Having a cmd line override is useful here when we need to make releases.

[juimonen]

@lgirdwood sure. So not yet sure/convinced why the private key needs to be environment variable and the MEU path tool not? Both are kind of external entities that we can't put inside public docker and they might change (so you sometimes use different keys etc. as I do). But I'm not shell script / parameter passing expert, maybe we are in trouble with long quoted/unquoted parameters etc.

[marc-hb]

PRIVATE_KEY_OPTION="-D${SIGNING_TOOL}_PRIVATE_KEY=$RIMAGE_PRIVATE_KEY"

Thanks, so you seem to be mis-using PRIVATE_KEY_OPTION as a PRIVATE_KEY variable that doesn't exist. The feedback should then be much clearer, sorry about that. Please try removing your xtensa-build-all.sh patch and defining `PRIVATE_KEY_OPTION=-DMEU_PRIVATE_KEY=something/keys/otc_private_key_3k.pem' as the script expects.

Remember CMakeList.txt uses both MEU_PRIVATE_KEY and RIMAGE_PRIVATE_KEY simultaneously. I asked about this above, no answer.

So not yet sure/convinced why the private key needs to be environment variable and the MEU path tool not?

I already gave a tentative answer to this, see above.

can you create a PR with your update.

Instead, please create a github issue to summarize and discuss the design issues. I think @juimonen patch would break users who use PRIVATE_KEY_OPTION as expected. A number of the usability and compatibility points brought up by @juimonen were already discussed above but he missed them pre-merge and they're now buried in other minor review comments or resolved. We're now in the typical "time loop" where the discussion is too long so the same questions get asked again and again. Ask them in a new, cleaner place and I'll extract the information from this PR.

PRs are really bad places for high level or user interface discussions because comments get hidden when resolved or lost when force pushing.

[juimonen]

@marc-hb I think I'm lost with this, can you please explain me how the private key "-k" is supposed to be used?

in the beginning the "-k" is checked and if you have not defined RIMAGE_PRIVATE_KEY you get error, otherwise
RIMAGE_PRIVATE_KEY get appended to PRIVATE_KEY_OPTION:

if [ "x$USE_PRIVATE_KEY" == "xyes" ]
then
if [ -z ${RIMAGE_PRIVATE_KEY+x} ]
then
echo "Error: No variable specified for RIMAGE_PRIVATE_KEY"
exit 1
fi
PRIVATE_KEY_OPTION="-DRIMAGE_PRIVATE_KEY=${RIMAGE_PRIVATE_KEY}"
fi

then later in tgl section we check that if PRIVATE_KEY_OPTION is empty, we set it to otc_private_key_3k.pem

            tgl)
                    # default key for TGL
                    if [ -z "$PRIVATE_KEY_OPTION" ]
                    then
                            PRIVATE_KEY_OPTION="-D${SIGNING_TOOL}_PRIVATE_KEY=$pwd/keys/otc_private_key_3k.pem"
                    fi
                    ;;

But it can't be empty with -k ?

later the PRIVATE_KEY_OPTION is given to CMAKE:

   cmake -DTOOLCHAIN="$TOOLCHAIN" \
            -DROOT_DIR="$ROOT" \
            -DMEU_OPENSSL="${MEU_OPENSSL}" \
            "${MEU_PATH_OPTION}" \
            "${PRIVATE_KEY_OPTION}" \
            ..

So what is the PRIVATE_KEY_OPTION "-k" used for? How will I ever be able to specify some key with -k?

[marc-hb]

Don't use the -k option nor its RIMAGE_PRIVATE_KEY companion, this was left unchanged for backward compatibility.

Define PRIVATE_KEY_OPTION directly instead, it's more flexible because it lets you do anything you want.

You cannot use both -k and a direct PRIVATE_KEY_OPTION at the same time.

If you don't define anything, TGL has a sensible default value for PRIVATE_KEY_OPTION.

@ranj063 could you submit a print_usage() patch along those lines?

Repeating previous comments of mine: PRIVATE_KEY_OPTION is simply passed-through to CMake, no logic. xtensa-build-all.sh is a wrapper script, which means it merely adds one layer of indirection over CMake (which is already a layer of indirection). Docker is another layer of indirection. The best approach to keep these too many layers of indirections somewhat maintainable is the "pass-through" approach where each layer has as little logic as possible. This is especially true when no one is interested in maintaining these layers, which I think has been demonstrated here.

[ranj063]

Don't use the -k option nor its RIMAGE_PRIVATE_KEY companion, this was left unchanged for backward compatibility.

Define PRIVATE_KEY_OPTION directly instead, it's more flexible because it lets you do anything you want.

You cannot use both -k and a direct PRIVATE_KEY_OPTION at the same time.

If you don't define anything, TGL has a sensible default value for PRIVATE_KEY_OPTION.

@ranj063 could you submit a print_usage() patch along those lines?

Repeating previous comments of mine: PRIVATE_KEY_OPTION is simply passed-through to CMake, no logic. xtensa-build-all.sh is a wrapper script, which means it merely adds one layer of indirection over CMake (which is already a layer of indirection). Docker is another layer of indirection. The best approach to keep these too many layers of indirections somewhat maintainable is the "pass-through" approach where each layer has as little logic as possible. This is especially true when no one is interested in maintaining these layers, which I think has been demonstrated here.

@marc-hb Im not sure Im the best person to write about the usage of the -k option. @xiulipan you're the one who opposed to changing this? Could you please add that?

@juimonen btw for building TGL you shouldnt need to use the -k option if thats what you're asking. The script takes care of it as long as you specify that you're using meu to sign with the -m option.

[marc-hb]

@juimonen btw for building TGL you shouldnt need to use the -k option if thats what you're asking.

s/shouldnt/cannot/

@ranj063 this is I think what's missing in print_usage() more than details about the old -k. Someone who wants to build TGL and looks at the -h usage will naturally try to use the -k option - and fail. I think the main thing that @juimonen 's feedback shows about this PR is its lack of "documentation". The second thing is probably the lack of good error messages.

[juimonen]

@ranj063 @marc-hb only thing I'm missing is the ability to define the key in the command line of this script. I mean for me it is changing as i need to use the debug key with devices, and it can't be included in our distros. But now I need to give it as env variable to docker-run -> every time I rebase sof I need to add that line there -> I don't like it.

[juimonen]

so I can build it now with:
./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -m /home/sof/work/sof.git/MEU15_TGL/Linux64 tgl

and adding this to docker-run.sh:
--env PRIVATE_KEY_OPTION="-DMEU_PRIVATE_KEY=/home/sof/work/sof.git/MEU15_TGL/private.pem"

[marc-hb]

Please try the docker-run.sh patch I wrote in #3187 (comment) and submit it if it works for you.

[juimonen]

@marc-hb sorry don't get how your patch is supposed to be used? it exports empty env variables into the docker?

[juimonen]

@marc-hb sorry just reading the docs so it should then take my local env vars with same name and move it to docker, ok that could be really nice actually...

[marc-hb]

just reading the docs so it should then take my local env vars with same name and move it to docker, ok that could be really nice actually...

= pass-through.

[juimonen]

with @marc-hb patch idea this is working also (and without me touching the scripts):
export PRIVATE_KEY_OPTION=-DMEU_PRIVATE_KEY=/home/sof/work/sof.git/MEU15_TGL/private.pem && ./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -m /home/sof/work/sof.git/MEU15_TGL/Linux64 tgl

Anyway I would still argue the grounds of having the other as env variable and other as command line param, as they both are changing and not very static. So I would then prefer that most things would be env variables or vice versa.

Anyway this works for me, so no worries. @marc-hb please put the patch in, it is good, I want it.

[marc-hb]

Anyway I would still argue the grounds of having the other as env variable and other as command line param, as they both are changing and not very static.

I think you can use export MEU_TOOL_PATH too.

By the way did you just confirm that the default, undefined openssl location works for you too?

please put the patch in

I have neither a docker image nor a TGL environment and bar exceptional circumstances I never submit and put my name on something I cannot at least smoke test myself.

[xiulipan]

@marc-hb @juimonen Jenkins CI do not update with this change yet. Will try to confirm the change could work first and then update our scripts.

[juimonen]

@marc-hb yes the sof-docker has openssl built into it and it works at least for this tgl signing. I was not sure about this, because in the previous version of this PR it was given as external parameter...

I'm not sure about the MEU_TOOL_PATH, because if I read the docs correctly the -m you need to give to choose between rimage and meu signing. So I guess I anyway need to give the path...?

Ok, I'll put the patch, seems that I'm anyway the only person using it.

[marc-hb]

After a little bit more water under the bridge I still think this TGL / MEU user interface was the right one, however there's something that was definitely missing from this PR: documentation! This can be observed in PR #3511 attempt to "fix TGL MEU custom key" which looks like about to repeat the (too long) discussion above.

So I just submitted a 10 lines, pure print_usage() patch in PR #3512 . @ranj063 , @juimonen , @fredoh9 , @xiulipan and anyone else who has ever attempted to use xtensa-build-all to sign TGL, your review help is needed.