using binman sym for size of u-boot itb

[Simon Glass]

Hi Rasmus,

On Fri, 3 Mar 2023 at 03:25, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On imx8mp, binman is heavily used for the assembling of all the various
> pieces. Partly for my own future reference:
>
> There's u-boot-spl-ddr which is u-boot-spl.bin with the DDR firmware
> blobs tacked on at the end, and the sizes and positions of those blobs
> then updated via the binman syms mechanism.
>
> This file, u-boot-spl-ddr.bin, is then passed through mkimage to compute
> and generate the imx-specific header, resulting in the final spl.bin.
>
> Parallel to that, U-Boot proper and its .dtb along with ATF/bl31.bin is
> wrapped in an itb image.
>
> And finally, spl.bin and u-boot.itb are concatenated, with u-boot.itb
> landing on a known offset, to produce the final "flash.bin", which can
> then be written to eMMC (user or boot partition) or even booted over usb
> with uuu. So far so good.
>
> Currently, the spl_imx_romapi.c contains a somewhat delicate
> tiptoeing-around algorithm for determining the size of the itb, in the
> case where we're booting over USB. (1) read from the stream 1K at a time
> looking for the 0xd00dfeed marker. (2) if that was found too close to
> the end of the 1K block we just fetched so we don't have a full struct
> fdt_header, we fetch another 1K. (3) So now we have a struct fdt_header
> and can thus read the .totalsize member, so now we can go on and read
> from the stream until we've read that much in total. (4) But that just
> gives us the size of the DTB structure; the itb is built with "external
> data", so now we abuse spl_load_simple_fit() and call it twice, the
> first time with some extra private data that make the .read callback
> stash the maximum offset it ever saw while most likely also copying
> garbage (never initialized RAM) to whatever the loadaddr= properties
> specify, then use that info to actually read that much data from the usb
> stream so we now do have a full, including all the external data, FIT
> image at CONFIG_SPL_IMX_ROMAPI_LOADADDR, and then do
> spl_load_simple_fit() again.
>
> Now I have two ideas: First, all of this complexity could go away if we
> could just do
>
> binman_sym_declare(ulong, itb, size);
>
> in spl_imx_romapi.c. But that fails with
>
> binman: Section '/binman/u-boot-spl-ddr': Symbol '_binman_itb_prop_size'
>    in entry '/binman/u-boot-spl-ddr/u-boot-spl/u-boot-spl-nodtb': Entry
> 'itb' not found in list
> (u-boot-spl-nodtb,u-boot-spl-dtb,u-boot-spl,ddr-1d-imem-fw,ddr-1d-dmem-fw,ddr-2d-imem-fw,ddr-2d-dmem-fw,main-section)

At present you are not allows to use a symbol from a different section.

See Image.LookupImageSymbol() which calls section._CollectEntries() to
find all entries (and subentries) in a section. It could be expanded
to look through the whole image, perhaps.

>
> So I know that tools/binman/binman.rst says that binman currently
> doesn't support images that depend on each other, but as can be seen
> above, imx8mp actually defines several images that do depend on each other:
>
>   u-boot-spl-ddr.bin <- spl.bin <---\
>                                      flash.bin
>               u-boot.itb <----------/
>
> Perhaps this just works by chance, because binman happens to build the
> images in the order defined in the binman node. I want to add a new
> dependency

Yes, it only works because a file is written out for each image and
the images are processed in order.

>
>        u-boot-spl-ddr.bin <- spl.bin <---\
>            v                              flash.bin
>         u-boot.itb <---------------------/
>
> and I don't mind if one has to manually do a topological sort of
> subnodes of the binman node [so this would mean moving the itb node to
> the top]. Or we could introduce an
>
>   image-deps = <&itb>;
>
> property and let binman do the topological sorting for us. Regardless,
> obviously some work on the python side would be needed; perhaps (and I'm
> really not familiar with the binman code) Entry_section should grow an
> ".external_entries" member that could be populated from such an

I think image-deps is a good idea. You might just need to change
LookupImageSymbol() to call _CollectEntries() for itself and for all
the external images?

One question is what to do about duplicates. I suppose the current
image should get priority and we should emit a warning.

> image-deps property, and then LookupSymbol() would need to be taught to
> also look through .external_entries - only allowing prop_name==size for
> such.

Probably no need to restrict it just to that, since the position might
be useful also.

>
> And now to the second idea: If we could to this, then we could
> definitely also do
>
> typedef struct { uchar x[32]; } sha256_digest;
> binman_sym_declare(sha256_digest, itb, sha256);
>
> and teach LookupSymbol also to handle size==32 and prop_name=='sha256'
> (I guess the pack format would be 32B). And then SPL could verify the
> whole ITB payload with just a single sha256 call, and one wouldn't have
> to deal with all the complexity of FIT image verification in SPL and
> code size constraints and whatnot. This hash would be baked into
> u-boot-spl-ddr.bin before mkimage is invoked to produce the imx header
> and possibly HAB signature etc., so we do have chain of trust preserved.

That seems reasonable to me. One challenge is the dance we do around
adding the properties in one pass, then setting the values in a second
pass. See entry.AddMissingProperties() and
entry.SetCalculatedProperties() so you would need to update those.
There is already an 'update-hash' property but it is for a slightly
different purpose.

>
> Thoughts? Any pointers on how I could start doing that external_entries
> thing?

See above. Note that testSymbolsSubsection() is a test which checks
symbols from a subsection of a sibling node. You could add a new test
building on that.

Regards,
Simon