Re: [PATCH v2 19/25] binman: Keep a separate list of entries for fit

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 24/02/2022 02:00, Simon Glass wrote:
> The current implementation sets up the FIT entries but then deletes the
> 'generator' ones so they don't appear in the final image.

They still show up in the fdtmap if I add one to rockchip-u-boot.dtsi:

  $ binman ls -i u-boot-rockchip.bin
  Name                Image-pos  Size     Entry-type    Offset
  -------------------------------------------------------------
  main-section                0   103520  section             0
    mkimage                   0    1a000  mkimage             0
    fit                   1a000    e8d74  fit             1a000
      u-boot              1a644    b1898  section           644
        u-boot-nodtb      1a644    b1898  u-boot-nodtb        0
      @atf-SEQ                0        0  section             0
        atf-bl31              0        0  atf-bl31            0
      @tee-SEQ                0        0  section             0
        tee-os                0        0  tee-os              0
      @fdt-SEQ                0        0  section             0
    fdtmap               102d74      79c  fdtmap         102d74

But not simple-bin.map:

  ImagePos    Offset      Size  Name
  00000000  00000000  00103520  main-section
  00000000   00000000  0001a000  mkimage
  0001a000   0001a000  000e8d74  fit
  0001a644    00000644  000b1898  u-boot
  0001a644     00000000  000b1898  u-boot-nodtb
  00102d74   00102d74  0000079c  fdtmap

This happens in v1 as well, but I hadn't checked it then.

(The "fit" size and "fdtmap" offset are off by 0x10 too, but I'm not
sure why yet.)

> This is a bit clumsy. We cannot build the image more than once, since the
> generator entries are lost during the first build. Binman requires that
> calling BuildSectionData() multiple times returns a valid result each
> time.

I think the generator entries should be turned into concrete entries and
be removed in _gen_entries(), so BuildSectionData() should work on the
entries that were generated. This way the individual entries would show
up in fdtmap and could be binman-extracted/replaced as well.

For FDTs we can generate blob entries for each file, for ELFs maybe we
can split them into files like the script used to do (bl31_0x<addr>.bin)
and do the same?

Maybe some new entry types, "data" for arbitrary data unrelated to a
file whose contents we can set, or "elf-segment" that can extract a
segment from an ELF file (but I don't like the information loss there).

> Keep a separate, private list which includes the generator nodes and use
> that where needed, to correct this problem. Ensure that the missing list
> includes removed generator entries too.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)

I'm more and more convinced that generator nodes needs a thorough
redesign, but the patch is consistent with status quo, so:

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

>  tools/binman/etype/fit.py | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index e1b056f56e..30b20a07a2 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -163,10 +163,15 @@ class Entry_fit(Entry_section):
>                  key: relative path to entry Node (from the base of the FIT)
>                  value: Entry_section object comprising the contents of this
>                      node
> +            _priv_entries: Internal copy of _entries which includes 'generator'
> +                entries which are used to create the FIT, but should not be
> +                processed as real entries. This is set up once we have the
> +                entries

Maybe this could be "_templates" or "_entry_generators" and only keep
track of the generator entries.

>          """
>          super().__init__(section, etype, node)
>          self._fit = None
>          self._fit_props = {}
> +        self._priv_entries = {}
>  
>          for pname, prop in self._node.props.items():
>              if pname.startswith('fit,'):
>  
> [...]