ASoC: SOF: Introduce extended manifest #1818

ktrzcinx · 2020-02-21T10:33:37Z

Extended manifest is a place to store metadata about firmware, known during
compilation time - for example firmware version or used compiler.
Given information are read on host side before firmware startup.
This part of output binary is located as first struct in binary file and
is not signed. This part of file shouldn't be loaded into DSP memory.

Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com

thesofproject/sof#2427

sound/soc/sof/loader.c

plbossart

Interesting idea, but this needs a lot of work... See comments below

I think it'd be worth documenting what information remains provided by the firmware after it boots, and what is provided by the extended manifest.

Also all the firmware handling is really questionable.

include/sound/sof/xman.h

plbossart · 2020-02-22T00:20:43Z

include/sound/sof/xman.h

type, size of blob, binary blob.
Why add a size and remove 2*sizeof(uint32_t) to get to the array...

Reordered. Using struct is much more descriptive solution than raw array reading and remembering data format

sound/soc/sof/loader.c

include/sound/sof/xman.h

sound/soc/sof/ipc.c

sound/soc/sof/loader.c

include/sound/sof/xman.h

lyakh · 2020-02-24T08:44:53Z

There's probably a corresponding PR for the rimage tool to generate that manifest - could you point out to it, please? I don't know how high on the agenda (or even how realistic) this is, but we're talking from time to time about adding support for or entirely moving to RPMSG for IPC (@lgirdwood). RPMSG includes support for loading firmware and the only currently supported firmware format is ELF. In ELF format a part of this information is contained in .comment section, specifically the compiler version. So, if we're moving over to RPMSG / ELF - this shouldn't be done in this way. If we aren't moving over at least for now, maybe we can at least reuse the .comment section for this purpose?

ktrzcinx · 2020-02-24T11:31:16Z

@lyakh

thesofproject/sof#2427

I design extended manifest to be very extensible, you will can add there some extra without big effort and impact on firmware flash size. But section name .comment doesn't sounds good for me so I will stay with my name.

sound/soc/sof/loader.c

include/uapi/sound/sof/ext_manifest.h

sound/soc/sof/loader.c

ranj063

@ktrzcinx looking almost good but still some typos, minor changes and clarifications in comments needed.

It makes possible to provide extra information to host before downloading firmware. Extra data should be put at the beginning of firmware binary. Exchange is done without any effort on DSP side. This mechanism will be used in extended manifest. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

kv2019i

Looks still good to me -- waiting for an ack from @ranj063 . To me there was one very minor spelling error, but otherwise looks everything else is addressed now.

Extended manifest is a place to store build time known firmware metadata, for example firmware version or used compiler description. Given information is read on host side before firmware startup. This part of output binary is located as a first structure in binary file. Extended manifest should be skipped in firmware loading routine. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

The firmware version can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

The window description can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

The compiler version and description can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

ranj063

Thanks @ktrzcinx for being patient with so many review comments. Looks good now!

kv2019i · 2020-03-17T09:00:55Z

Thank you @ktrzcinx , merging now!

lyakh

just noticed it's been merged in the meantime. Never mind.

lyakh · 2020-03-17T08:32:37Z

sound/soc/sof/intel/hda-loader.c


-	stripped_firmware.data = plat_data->fw->data;
-	stripped_firmware.size = plat_data->fw->size;
+	if (plat_data->fw->size < plat_data->fw_offset) {


it should ne <= probably?

At least "<" guarantees the wrap around, that's the purpose of this check.

@kv2019i sorry, don't understand. The difference between <= and < is the == case. With <= == would also trigger the error path. And indeed, that would fit the error message ("must be larger") and 0 size after the offset doesn't indeed seem to make much sense. As for wrapping around 4GiB - wouldn't you have to catch that earlier - when performing calculations, that potentially can wrap?

lyakh · 2020-03-17T08:32:56Z

sound/soc/sof/loader.c

 	struct snd_sof_fw_header *header;
+	size_t fw_size = fw->size - fw_offset;
+
+	if (fw->size < fw_offset) {


lyakh · 2020-03-17T08:42:39Z

include/uapi/sound/sof/ext_manifest.h

+#ifndef __SOF_FIRMWARE_EXT_MANIFEST_H__
+#define __SOF_FIRMWARE_EXT_MANIFEST_H__
+
+#include <linux/const.h>


you also need <linux/types.h> and <linux/bits.h>. Strictly speaking also <linux/compiler.h> but it looks like nobody does that, that will be pulled in for you somewhere along the way.

I will keep it in mind and make a proper fix.

lyakh · 2020-03-17T08:59:06Z

sound/soc/sof/loader.c


 #include <linux/firmware.h>
 #include <sound/sof.h>
+#include <uapi/sound/sof/ext_manifest.h>


I'm not quite clear about this myself and I've done this myself that way before, but I think now, that it should be sufficient to just #include <sound/sof/ext_manifest.h>

This header if designed to be read from host side and giving possibility for example to check fw version from user space is not so bad. Moreover thanks to keeping this file in uapi/, include/kernel folder from sof repo is mapped in a whole to uapi/.

sorry, I didn't mean that the header was wrongly placed, I just meant, that I think, just #include <sound/sof/ext_manifest.h> would find the header under uapi/ too.

lyakh · 2020-03-17T09:00:02Z

sound/soc/sof/loader.c


+static ssize_t snd_sof_ext_man_size(const struct firmware *fw)
+{
+	const struct sof_ext_man_header *head = (void *)fw->data;


would be much better to cast to the target type, i.e. (const struct sof_ext_man_header *)fw->data

Is there any difference?

I think so, yes. Type-casting should be avoided whenever possible and where not possible, done in the most appropriate and careful way.

kv2019i · 2020-03-17T11:04:44Z

@lyakh wrote:

just noticed it's been merged in the meantime. Never mind.

My apologies. GH showed you had no changes requested, so given the many approvals, I went and merged it. I did have to use maintainer override, so I think GH tried to warn me that you had review in progress, but it was not visible in the issue status, so I thought it was just a glitch and went ahead. I'll do better next time.

In general, we should be more synchronized with the review process. This has received almost 300 comments and in more than 3 big waves spanning over three weeks, which is not very efficient. I miss the "author+1" feature from Gerrit, to have a more clear sign that the author thinks the change is now ready for merging and is a queue for reviews to happen.

ktrzcinx · 2020-03-17T11:13:28Z

Thanks @plbossart @ranj063 @kv2019i @lyakh

ktrzcinx requested review from juimonen, keyonjie, lgirdwood, lyakh, plbossart and ranj063 as code owners February 21, 2020 10:33

ktrzcinx force-pushed the ext-mnf branch from 466b14e to 8702c50 Compare February 21, 2020 11:33

ktrzcinx requested a review from dbaluta as a code owner February 21, 2020 11:33

ktrzcinx force-pushed the ext-mnf branch 2 times, most recently from 551a2d0 to b5754d8 Compare February 21, 2020 13:14

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Feb 21, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

plbossart requested changes Feb 22, 2020

View reviewed changes

ktrzcinx force-pushed the ext-mnf branch from b5754d8 to 67e5d25 Compare February 24, 2020 08:40

ktrzcinx requested a review from kv2019i as a code owner February 24, 2020 08:40

ktrzcinx force-pushed the ext-mnf branch from 67e5d25 to ec7b780 Compare February 24, 2020 10:08

ktrzcinx changed the title ~~WIP: ASoC: SOF: Introuce extended manifest~~ WIP: ASoC: SOF: Introduce extended manifest Feb 24, 2020

ktrzcinx force-pushed the ext-mnf branch 3 times, most recently from de84923 to 2abbb33 Compare February 24, 2020 11:26

ranj063 reviewed Mar 13, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Mar 13, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Mar 13, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 reviewed Mar 13, 2020

View reviewed changes

include/uapi/sound/sof/ext_manifest.h Outdated Show resolved Hide resolved

ranj063 reviewed Mar 13, 2020

View reviewed changes

include/uapi/sound/sof/ext_manifest.h Outdated Show resolved Hide resolved

ranj063 reviewed Mar 13, 2020

View reviewed changes

sound/soc/sof/loader.c Outdated Show resolved Hide resolved

ranj063 requested changes Mar 13, 2020

View reviewed changes

ktrzcinx dismissed kv2019i’s stale review via 53e58c5 March 16, 2020 10:53

ktrzcinx force-pushed the ext-mnf branch from 4917353 to 53e58c5 Compare March 16, 2020 10:53

ktrzcinx requested a review from ranj063 March 16, 2020 10:56

kv2019i previously approved these changes Mar 16, 2020

View reviewed changes

ktrzcinx added 4 commits March 16, 2020 16:06

ASoC: SOF: ext_manifest: parse firmware version

a1d10c2

The firmware version can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

ASoC: SOF: ext_manifest: parse windows

c0ba626

The window description can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

ASoC: SOF: ext_manifest: parse compiler version

8fb90fe

The compiler version and description can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>

ktrzcinx dismissed kv2019i’s stale review via 8fb90fe March 16, 2020 15:11

ktrzcinx force-pushed the ext-mnf branch from 53e58c5 to 8fb90fe Compare March 16, 2020 15:11

ranj063 approved these changes Mar 16, 2020

View reviewed changes

plbossart approved these changes Mar 16, 2020

View reviewed changes

ktrzcinx requested a review from kv2019i March 17, 2020 05:49

kv2019i approved these changes Mar 17, 2020

View reviewed changes

kv2019i merged commit 8ffa879 into thesofproject:topic/sof-dev Mar 17, 2020

lyakh reviewed Mar 17, 2020

View reviewed changes

ktrzcinx mentioned this pull request Mar 18, 2020

ASoC: SOF: ext_manifest: Initial submission #1903

Merged

ktrzcinx mentioned this pull request Mar 25, 2020

Create extended manifest thesofproject/sof#2427

Merged

ktrzcinx mentioned this pull request May 13, 2020

fixup! ASoC: SOF: Introduce extended manifest #2095

Merged

ASoC: SOF: Introduce extended manifest #1818

ASoC: SOF: Introduce extended manifest #1818

Uh oh!

Conversation

ktrzcinx commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh commented Feb 24, 2020

Uh oh!

ktrzcinx commented Feb 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Mar 17, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ktrzcinx commented Feb 21, 2020 •

edited

Loading