thesofproject / rimage

DSP firmware image creation and signing tool
Other
7 stars 62 forks source link

Revert "ext_manifest: Fix incorrect signature" #72

Closed marc-hb closed 3 years ago

marc-hb commented 3 years ago

This reverts commit 241af313dab49db06d6e6f0af3330f1a916de664 so the DSP can boot again.

On my APL UP2 this fixes:

10:06:38: 00:0e.0: unstall/run core: core_mask = 1
10:06:38: 00:0e.0: DSP core(s) enabled? 1 : core_mask 1
10:06:38: 00:0e.0: FW Poll Status: reg[0x4c]=0x40000000 successful
10:06:38: 00:0e.0: FW Poll Status: reg[0x4]=0x3030202 successful
10:06:38: 00:0e.0: FW Poll Status: reg[0x4]=0x1010202 successful
10:06:38: 00:0e.0: DSP core(s) enabled? 0 : core_mask 2
10:06:38: 00:0e.0: FW Poll Status: reg[0x80000]=0x5000001 successful
10:06:41: 00:0e.0: FW Poll Status: reg[0x80000]=0x80000007 timedout
10:06:41: 00:0e.0: error: cl_copy_fw: timeout HDA_DSP_SRAM_REG_ROM_STATUS read
10:06:41: 00:0e.0: FW Poll Status: reg[0x160]=0x140000 successful
10:06:41: 00:0e.0: ------------[ DSP dump start ]------------
10:06:41: 00:0e.0: unknown ROM status value 80000007
10:06:41: 00:0e.0: error: extended rom status:  0x80000007 0x2f 0x0 0x0 \
                                                0x0 0x0 0x1522100 0x0
10:06:41: 00:0e.0: ------------[ DSP dump end ]------------
10:06:41: 00:0e.0: error: load fw failed ret: -110
10:06:41: 00:0e.0: error: failed to start DSP
10:06:41: 00:0e.0: error: failed to boot DSP firmware -110
10:06:41: 00:0e.0: FW Poll Status: reg[0x4]=0x1d003c timedout
10:06:41: 00:0e.0: error: hda_dsp_core_reset_enter: timeout on  \
                                                  HDA_DSP_REG_ADSPCS read
10:06:41: 00:0e.0: error: dsp core reset failed: core_mask 3
10:06:41: probe of 0000:00:0e.0 failed with error -110

Test PR https://github.com/thesofproject/sof/pull/4782 show this commit broke the DSP boot on ALL platforms https://sof-ci.01.org/sofpr/PR4782/build10387/devicetest/

[    7.798750] kernel: sof-audio-acpi-intel-bdw INT3438:00: error: invalid firmware signature
[    7.798827] kernel: sof-audio-acpi-intel-bdw INT3438:00: error: invalid FW header
[    7.798943] kernel: sof-audio-acpi-intel-bdw INT3438:00: error: failed to load DSP firmware -22
[    7.810348] kernel: sof-audio-acpi-intel-bdw INT3438:00: error:
sof_probe_work failed err: -22

This also broke QEMU boot on a number of platforms: BDW, BYT and CHT, see error: invalid firmware signature in

https://sof-ci.01.org/sofpr/PR4782/build10387/boottest/ and https://github.com/thesofproject/sof/pull/4782/checks?check_run_id=3634915322

How was it tested?

Signed-off-by: Marc Herbert marc.herbert@intel.com

marc-hb commented 3 years ago

Unlike the original commit, this revert has been tested in TEST PR https://github.com/thesofproject/sof/pull/4783 and everything is passing.

plbossart commented 3 years ago

merging to keep the daily tests (starts in 15mn) and week-end stress tests operational.

marc-hb commented 3 years ago

merging to keep the daily tests (starts in 15mn) and week-end stress tests operational.

Thanks for merging. There was no rush for validation because the rimage version in SOF is lagging a few commits behind and right now it has neither the regression (otherwise it would have been tested and caught) nor this revert.

It's still great that developers can use the latest rimage branch again.

Reminder: the README.md now has instructions on how to run SOF tests on rimage changes before rimage merge. See examples above.

plbossart commented 3 years ago

There was no rush for validation because the rimage version in SOF is lagging a few commits behind and right now it has neither the regression (otherwise it would have been tested and caught) nor this revert.

I don't get why CI does not test the latest rimage, or why with a delay? This is nuts.

marc-hb commented 3 years ago

I don't get why CI does not test the latest rimage, or why with a delay? This is nuts.

This is how git submodules work. They're designed for relatively static libraries that get upgraded rarely and manually.

From https://docs.zephyrproject.org/latest/guides/west/why.html#rationale-for-a-custom-tool

git submodules do not support continuous tracking of the latest HEAD in external repositories (R4)

See also https://github.com/thesofproject/sof/issues/3517 [FEATURE] drop git submodules and switch to Zephyr's "west" multi-repo tool

The counterpart of validating a moving dependency is you're never quite sure what you're testing because it can be changing at any moment; even in the middle of a MIDDLE of test run! This is not theoretical, it happened to me once in the middle of a Zephyr PR: two shards tested slightly different versions. One failed the other not. Fortunately I had added some version logging in the build logs before it happened, otherwise I would have lost much more hair.

plbossart commented 3 years ago

I don't get why CI does not test the latest rimage, or why with a delay? This is nuts.

This is how git submodules work. They're designed for relatively static libraries that get upgraded rarely and manually.

Maybe, but if it's know to be isolated from the automatic tests, then the author should know better and trigger a manual test.

marc-hb commented 3 years ago

Maybe, but if it's know to be isolated from the automatic tests, then the author should know better and trigger a manual test.

Yes. Except the submodule indirection requires some effort and submodule proficiency.

I just added a guide in the README that should help a bit.

lgirdwood commented 3 years ago

@plbossart think we are good now, we have a process for testing rimage updates now. @mrajwa fyi - can you re-submit with the cmd line / Kconfig option. Thanks.

mrajwa commented 3 years ago

@lgirdwood yep, I have it on my TODO list will upstream soon.

plbossart commented 3 years ago

will this command line option be required only for the new stuff? it'd be a problem if we have to update the build and CI scripts for existing platforms.

marc-hb commented 3 years ago

will this command line option be required only for the new stuff? it'd be a problem if we have to update the build and CI scripts for existing platforms.

I know only two places invoking rimage right now: https://github.com/thesofproject/sof/blob/main/src/arch/xtensa/CMakeLists.txt https://github.com/thesofproject/sof/blob/main/scripts/xtensa-build-zephyr.sh

Great opportunity to review and cleanup the first one @mrajwa . Ideally, please submit the cleanup first and the new feature later; think Continuous Integration.

marc-hb commented 3 years ago

Actually: we already have per-platform config files with sizes, offsets and what not: https://github.com/thesofproject/rimage/tree/main/config

Does this really require a new command line option?