[RFC] make sof_abi_hdr optional in binary configs

[juimonen]

Currently we require SOF blobs to be wrapped inside sof_abi_hdr, thus other blobs that you try to send to firmware will stop at abi checks etc. in SOF driver. So make this optional.

[juimonen]

absolutely not tested thoroughly...

[plbossart]

@cujomalainey @dgreid can you comment on this? Is it really a problem to have an ABI header information in binary blobs?

[dgreid]

@cujomalainey @dgreid can you comment on this? Is it really a problem to have an ABI header information in binary blobs?

Are the ABI headers useful for ensuring the blob being loaded is compatible with the running firmware? Or, are there other constraints on config blob format?

Are blobs containing code considered in the same paths?

Sorry for silly questions, just paging in some context...

[cujomalainey]

@cujomalainey @dgreid can you comment on this? Is it really a problem to have an ABI header information in binary blobs?

Are the ABI headers useful for ensuring the blob being loaded is compatible with the running firmware? Or, are there other constraints on config blob format?

Are blobs containing code considered in the same paths?

Sorry for silly questions, just paging in some context...

personally I think it might be worth keeping the header for anyone who doesn't have the forethought to put something like versioning system into their blob. Also the magic prevents anyone from stuffing random bytes down the control by accident.

What do we gain by removing these checks other than the space the header occupies?

[plbossart]

@juimonen can you clarify which group was asking for this removal? I thought it was related to hotwording and EQ blobs but now it's unclear to me given the answers from @dgreid and @cujomalainey

[keyonjie]

Let me add some information about this.

1. Background:
   The requirement came from our Chrome team directly, the detail is that we should make using UCM set-tlv with Google existed KWD Blob files location specified works directly, which will be friendly for algorithm porting onto SOF.
   I suggested an alternative solution that using sof-ctl to do a conversion(adding struct sof_abi_hdr and struct snd_ctl_tlv to the head of the blob) for all blobs, but keeping blobs intact is the first choice from our friends. @sathya-nujella @sathyap-chrome can you help to comment?
   From what @cujomalainey commented here, looks we need more alignment on the preferred solution here?

2. The ABI header detail:

struct sof_abi_hdr {
	uint32_t magic;		/**< 'S', 'O', 'F', '\0' */
	uint32_t type;		/**< component specific type */
	uint32_t size;		/**< size in bytes of data excl. this struct */
	uint32_t abi;		/**< SOF ABI version */
	uint32_t reserved[4];	/**< reserved for future use */
	uint32_t data[0];	/**< Component data - opaque to core */
} __attribute__((packed));

magic:
This could help to prevent some random errors as @cujomalainey mentioned, though I don't think it could be helpful for security.
type:
This is component specific definition, IIRC, only detect_test is using it until now.
size:
This can be generated from snd_ctl_tlv.length actually, so it is not a must to our driver actually.
abi:
Per my understanding, we haven't used the ABI version(sof_abi_hdr.abi) for ABI alignment yet(I checked EQ, detect_test, and selector only though), but this could be added in someday.
data:
The blob raw data.

sof-ctl tool:
What we added last year to our tool named sof-ctl(sof/tools/ctl/ctl.c) was aimed to generate (the struct sof_abi_hdr + struct snd_ctl_tlv) and add them to the head of the blob, and set/get them to/from the tlv kcontrol. The tool itself can help to do the blob conversion also.

3. What @juimonen changed here:
   We had discussion in the team meeting and aligned to add this change to meet the requirement came from item 1, we don't intend to change too much, but only make the sof_abi_hdr optional in the blob itself. With this change, we can accept the blob format like this:

(snd_ctl_tlv) [sof_abi_hdr] (blob raw data)

Where the sof_abi_hdr is optional, but the snd_ctl_tlv is mandatory and should be included in the blob binary files.

So it should be fine for @singalsu, it won't affect his EQ use case.

Hi @cujomalainey @dgreid @bzhg, looks some of Google's blob files don't have the snd_ctl_tlv prefix, so it is still an open to me that how can we make them works without sof-ctl tool.

@xiulipan is also working on this. @juimonen @xiulipan please correct me if I am wrong.

[sathya-nujella]

[sof_abi_hdr] (snd_ctl_tlv) (blob raw data)

Where the sof_abi_hdr is optional, but the snd_ctl_tlv is mandatory and should be included in the blob binary files.

These are some of the examples of cset-tlv we have from previous platforms in UCM:
1) cset-tlv "name='' /opt/google/dsm/dsmparam.bin"
2) cset-tlv "name='<hwd....mdl params>' /opt/google/kbl-hotword-support/..hwd-blob"

So, to get cset-tlv working on current blobs, we need to add header like below and probably for Intel platforms blobs can be updated by Google like below: @keyonjie , is this right understanding?
[sof_abi_hdr] (snd_ctl_tlv) (original blob)

[keyonjie]

[sof_abi_hdr] (snd_ctl_tlv) (blob raw data)
Where the sof_abi_hdr is optional, but the snd_ctl_tlv is mandatory and should be included in the blob binary files.

These are some of the examples of cset-tlv we have from previous platforms in UCM:

1. cset-tlv "name='' /opt/google/dsm/dsmparam.bin"
2. cset-tlv "name='<hwd....mdl params>' /opt/google/kbl-hotword-support/..hwd-blob"

So, to get cset-tlv working on current blobs, we need to add header like below and probably for Intel platforms blobs can be updated by Google like below: @keyonjie , is this right understanding?
[sof_abi_hdr] (snd_ctl_tlv) (original blob)

Yes, to get cset-tlv work, without @juimonen 's change here, we should make the blobs to be in the format of "(snd_ctl_tlv) (sof_abi_hdr) (original blob)", all are mandatory, with this PR applied, it should be "(snd_ctl_tlv) [sof_abi_hdr] (original blob)", the sof_abi_hdr is optional. (I made a typo previously, the sof_abi_hdr should be put after the snd_ctl_tlv).

[cujomalainey]

@keyonjie did this come from @bzhg for CML hotwording?

[xiulipan]

@sathya-nujella Yes it should be like @keyonjie said, to make cset-tlv working we need
(snd_ctl_tlv) [sof_abi_hdr] (original blob). (snd_ctl_tlv) is need by alsa-ucm. [sof_abi_hdr] is needed by SOF driver.

I think this PR is can make the cset-tlv to work with (snd_ctl_tlv)(original blob), which is the format of the old binary blob you shared. It just removed the check for [sof_abi_hdr] in SOF driver.

[keyonjie]

@keyonjie did this come from @bzhg for CML hotwording?

I am not in the sync meeting with Google, it came from @sathyap-chrome so I guess it eventually from @bzhg ?

@sathyap-chrome correct me if I am wrong.

[sathyap-chrome]

my request is based on how it is working for dsm param and wov works through UCM in KBL platforms.

[lyakh]

this looks like a bug to me and it was there in the original code. tlvd is a __user pointer, so, it shouldn't be dereferenced. You have a copy of tlvd->tlv now in header.tlv i.e. in kernel memory.

[lyakh]

This doesn't look particularly robust to me, sorry. We check for a magic number, and if we don't find one, we still use the data. I understand that we want to rely on the firmware in such cases to check whether the data is valid. Maybe we should then give the firmware an explicit indication that this data isn't known to the kernel? How about cdata->data->magic = ~SOF_ABI_MAGIC;? Or cdata->data->magic = SOF_ABI_BINARY_LEGACY; or something?

[lyakh]

same bug as above

[dgreid]

Is there a reason to avoid using the sof-ctl tool? It seems like a fine solution to me. It could be used at load time or at build time so the image on the board already has the headers.

[cujomalainey]

@dgreid my plan was to bypass the tool and use the controls directly in CRAS to avoid having to run code via the command line and incorporating another package.

[sathya-nujella]

Is there a reason to avoid using the sof-ctl tool? It seems like a fine solution to me. It could be used at load time or at build time so the image on the board already has the headers.

Hi @cujomalainey , @dgreid ,
In my understanding it is like this, can you please confirm or correct:

1. using sof-ctl tool, we need to update Google's hdw blobs to have this format: (snd_ctl_tlv) [sof_abi_hdr] (original blob) [Proabably same with DSM blobs]
2. those updated blobs will be in Chrome repo and same in DUT... then alsa-tlv will be used to load from UCM.

[cujomalainey]

Is there a reason to avoid using the sof-ctl tool? It seems like a fine solution to me. It could be used at load time or at build time so the image on the board already has the headers.

Hi @cujomalainey , @dgreid ,
In my understanding it is like this, can you please confirm or correct:

1. using sof-ctl tool, we need to update Google's hdw blobs to have this format: (snd_ctl_tlv) [sof_abi_hdr] (original blob) [Proabably same with DSM blobs]
2. those updated blobs will be in Chrome repo and same in DUT... then alsa-tlv will be used to load from UCM.

@bzhg i feel like you are probably a good person to comment on this. @chiang831 FYI

[bzhg]

Hi @sathya-nujella I think your understanding is correct. I agree with @cujomalainey that we can modify the existing blobs using sof-ctl at build time as we move each older platform to SOF. The modified blobs with the two headers will also live in the ChromeOS localmirror and get installed to DUT image. No change to CRAS or SOF driver is needed.

For development, sof-ctl can be used to test new blobs quickly. For production, we'll have an extra step to append the headers when updating the blobs. I feel the extra step is worth it to keep the basic sanity check and to be consistent across old and new platforms. @chiang831 What do you think?

[juimonen]

@bzhg @cujomalainey @dgreid @sathya-nujella

Currently sof-ctl tool packages the blob inside snd_ctl_tlv before sending. So the blob that you are giving to sof-ctl doesn't have the snd_ctl_tlv in it. Like we have sof examples currently with eq's and demux. But sof blobs require also the sof_abi_hdr. So when sof-ctl sends the blob it looks like
(snd-ctl-tlv -> sof_abi_hdr -> blob)

OTH "old" chrome blobs have this snd_ctl_tlv already around it. So the UCM is not wrapping anything at runtime, just sending the blob as it is. And they don't have sof_abi_hdr. So when old ucm chrome sends the blob it looks like (snd-ctl-tlv -> blob).

So it is a question which approach is better.

1. Old chrome blobs needs this PR's change if you want to send those over.
2. OTH if you are willing to add sof_abi_hdr to old chrome blobs, this PR is not needed.
3. if you do the 2, we could also change sof-ctl and existing sof blobs to include the snd_ctl_tlv and modify the sof-ctl not to do the wrapping. -> we would have unified solution for ucm and sof-ctl, but obviously more changes.

[mengdonglin]

@juimonen I checked with @lgirdwood he thinks sof_abi_hdr should be mandatory. So when sof-ctl sends the blob it looks like (snd-ctl-tlv -> sof_abi_hdr -> blob). He commented the parameters can change for different ABI versions of a component. A version mismatch can cause unexpected results.

[juimonen]

@mengdonglin ok this means then that "old" chrome blobs should be modified to have sof_abi_hdr after sof_ctl_tlv. There's still a question should we modify the "old" sof blobs to use also sof_ctl_tlv and remove the wrapping from sof-ctl? This would then "unify" the blobs in ucm and sof topology.

[juimonen]

chrome folks, if you want to experiment blob generation with the above suggestion, you could try thesofproject/sof#2536. With an option it should be able to generate a blob containing snd_ctl_tlv -> sof_abi_hdr -> blob

[bzhg]

We can modify the old chrome blobs from (snd-ctl-tlv)(blob) to (snd-ctl-tlv)(sof_abi_hdr)(blob).

For the sof-ctl cmdline tool, is it possible to keep the -r option to allow wrap/unwrap of both headers? It'll make development/testing easier when experimenting new blobs.

[juimonen]

@bzhg yes no problem. If you change the "old" blobs to include sof_abi_hdr for ucm, I think we don't have to do do any changes for the driver (so this PR is obsolete). The driver strips the snd_ctl_tlv away and checks after that for sof_abi_hdr.

I'm little bit now uncertain if the alsatplg will also wrap the topology blob with snd_ctl_tlv, but that can be changed later. It is just little bit inconsistent if the binary hex blobs in topology and ucm files look different, well you can't do diffs etc.

@bzhg I haven't used sof-ctl that much myself, but if there are any issues, it is easy to modify/update for your needs...

[mengdonglin]

@juimonen so can we close this PR?

[juimonen]

@mengdonglin yes let's close it. We can revisit later if needed. The sof_abi_hdr chrome issue was cleared and @lyakh found&fixed a bug in the control copy, so it was not for nothing :)

[singalsu]

The sof-ctl tool has a switch -r to work without headers. I'm not using it since I really prefer to have the header there in my own generated configuration files to ensure compatibility. The EQ tool creates the header automatically so there's no extra effort for user.