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

Alper Nebi Yasak
2022-03-03T21:19:03Z

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,'):
>
> …