Skip to content

Conversation

@ktrzcinx
Copy link
Member

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 not signed.

Related with https://github.com/thesofproject/linux/pull/1818/commits

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the rational behind all this work ? i.e. what does this give us over existing metadata ? MAJOR ABI updates are not free.

@ktrzcinx
Copy link
Member Author

Whats the rational behind all this work ? i.e. what does this give us over existing metadata ?

Two major benefits are:

  1. Possibility to check firmware compatibility with host before fw load and run
  2. Extra information won't be stored in DSP flash -> memory/power optimization and adding new information won't have almost any size restriction

@lgirdwood
Copy link
Member

lgirdwood commented Feb 26, 2020

Whats the rational behind all this work ? i.e. what does this give us over existing metadata ?

Two major benefits are:

  1. Possibility to check firmware compatibility with host before fw load and run

We already have FW versioning in the standard manifest, but we dont use it. It's just hard coded to values used by CAVS FW. We should probably use this. A lot lot easier than changing MAJOR ABI.

  1. Extra information won't be stored in DSP flash -> memory/power optimization and adding new information won't have almost any size restriction

Ok, this is the part we need. I do like using an ELF section, and we should put all our FW boot data in this section. We should also put all our init TEXT in this section too (any code that is run once during FW boot). Once boot is complete we then add this section to the heap (and can power it OFF etc if it's not used).

@mmaka1
Copy link

mmaka1 commented Feb 27, 2020

@lgirdwood I am not sure if putting this "boot data" into a section that is loaded to the DSP memory is the right thing as the FW does not use it at all. The only reasonwhy it is loaded, as I see it, is to report it back to the driver through the IPC since the driver does not know how to extract it on the host side before loading. So loading to sram, reporting back, freeing memory seems to be pretty expensive process vs. just reading it from the fw binary on the host side.

@paulstelian97
Copy link
Collaborator

@lgirdwood I am not sure if putting this "boot data" into a section that is loaded to the DSP memory is the right thing as the FW does not use it at all. The only reasonwhy it is loaded, as I see it, is to report it back to the driver through the IPC since the driver does not know how to extract it on the host side before loading. So loading to sram, reporting back, freeing memory seems to be pretty expensive process vs. just reading it from the fw binary on the host side.

Maybe two sections, one for data that is only interesting for kernel alone, another for stuff used by firmware but only during the boot process.

@mmaka1
Copy link

mmaka1 commented Feb 27, 2020

@paulstelian97 Of course, +1 for boot section in general. Btw, cAVS platforms are quite optimized regarding memory occupation by the boot code and data already since the boot loader stays in DRAM (called IMR), only the real run-time part of the fw is loaded to SRAM.

@lgirdwood
Copy link
Member

@mmaka1 in general I dont wont to update MAJOR for this, but I would accept a MINOR update that could prefix the data in extended manifest. I prefer to build as a new section and let rimage prepend it in the output so that kernel can read.
@ktrzcinx can you rework to avoid the MAJOR change ? We dont need to delete the current extended IPC data or structures. Structures can be reused and current data can be assigned to the extended section.

@mmaka1
Copy link

mmaka1 commented Feb 27, 2020

@lgirdwood Yes, we recently talked about a way to keep backward compatibility with the driver form a previous line, already deployed by distros and it is important indeed. However from the other side we'd like to copy only really required things and move as much as possible including things that are already used by the driver's code that could be safely changed (no plans for backport).

@ktrzcinx
Copy link
Member Author

@lgirdwood I'm currently working with this PR, but let clarify.

I dont wont to update MAJOR for this, but I would accept a MINOR update

MINOR change I understand as keep all structs in mailbox, as it was before, am I wrong?

Structures can be reused and current data can be assigned to the extended section.

So old structs with sof_ipc_ prefix located in .fw_ready section should be reused to build ext_man (fw_version, cc_version, sram_window and probe_support). Right?
Rimage should scan only .fw_ready section during building ext_man or scan .fw_redy + .fw_metadata for future data?

@lgirdwood
Copy link
Member

@lgirdwood I'm currently working with this PR, but let clarify.

I dont wont to update MAJOR for this, but I would accept a MINOR update

MINOR change I understand as keep all structs in mailbox, as it was before, am I wrong?

We can keep the same structs, but put them in ext manifest and not in the mailbox. The ABI change is smaller now. We should be able to use the same kernel functions for parsing too.

Structures can be reused and current data can be assigned to the extended section.

So old structs with sof_ipc_ prefix located in .fw_ready section should be reused to build ext_man (fw_version, cc_version, sram_window and probe_support). Right?

Yes.

Rimage should scan only .fw_ready section during building ext_man or scan .fw_redy + .fw_metadata for future data?

Yes.

Btw, have we considered (for backwards compat reasons) copying this data (as RO data) as a manifest module ? i.e. the codeloader will copy it to mailbox address (like it does with base FW sections). i.e. rimage creates a new module that should be copied by codeloader to mailbox (this modules only contains the legacy data only - no new structures). This way older kernels will be able to run new FW.

@ktrzcinx ktrzcinx force-pushed the ext-mnf branch 3 times, most recently from c73c21b to 3bf7903 Compare February 28, 2020 14:02
@lgirdwood lgirdwood added this to the v1.6 milestone Apr 16, 2020
@lgirdwood
Copy link
Member

@ktrzcinx will be merged after v1.5 is tagged this week.

@xiulipan
Copy link
Contributor

@kv2019i Waiting for QEMU update, the QEMU patch got some build error. And I will try to update the QEMU Docker image after the build error is fixed.

@xiulipan
Copy link
Contributor

@kv2019i @ktrzcinx @lgirdwood Docker image for QEMU updated, I re-trigger the QEMU test

@ktrzcinx
Copy link
Member Author

It seems CI stuck, I re-run tests by re-pushing last commit

@ktrzcinx ktrzcinx force-pushed the ext-mnf branch 2 times, most recently from 63daf4c to 576baae Compare April 17, 2020 18:41
@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

@ktrzcinx how does this PR differ from your TEST PR ?

@ktrzcinx
Copy link
Member Author

@lgirdwood I found an issue with new NOTE section introduction for byt, everything looks good for me - full analogy to others platforms - but xcc linker returns bin/xt-ld: sof: warning: allocated section '.fw_metadata' not in segment. I can't catch root cause. Unfortunately on my build platform RD-2012.5-linux and Intel_HiFiEP is not installed, and there is some trouble with installation, so I used TEST PR to look for solution, but without result.
In this PR, todays morning I removed ext_mnf for byt and cht then build system and CI was but @mmaka1 refuse such a solution with argumentation that byt may quickly loose sync with the rest when some information from ext_mnf will be obligatory.
Now this PR is in 'ready' state but with linker warning for byt.

@lgirdwood
Copy link
Member

@ktrzcinx looks like a linker issue, btw what happens if you use the GNU GCC linker on BYT when XCC is used ?

This section will be used to store information about firmware known
during compilation time.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
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>
Include such an information in extended manifest, to make it accessible
from host side before firmware load and run.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
This information is known at build time so should
be passed by extended manifest.
Introduced structure is designed to be fully compatible
with information provided through mailbox to make
parsing code on host side as much reusable as possible.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
This is information is known during compilation time,
so it should be passed by extended manifest.
Reuse sof_ipc_probe_support struct to make parsing function
from host side as much universal as possible.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
Copy link
Member Author

ktrzcinx commented Apr 22, 2020

@lgirdwood @mmaka1 issue with linker is resolved. For RD-2012.5-linux (byt and cht) I had to change

  .fw_metadata (COPY) : ALIGN(1024)
  {
    KEEP (*(.fw_metadata))
    . = ALIGN(_EXT_MAN_ALIGN_);
  } >fw_metadata_seg :metadata_entries_phdr

to

  .fw_metadata (COPY) : ALIGN(1024)
  {
    KEEP (*(.fw_metadata))
  } >fw_metadata_seg :metadata_entries_phdr

@lgirdwood
Copy link
Member

CI usual issues on Jenkins

@lgirdwood
Copy link
Member

Kernel support already merged.

@lgirdwood lgirdwood merged commit b51e67f into thesofproject:master Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ABI ABI change involved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants