Hi Rasmus,
On Fri, 3 Mar 2023 at 03:25, Rasmus Villemoes rasmus.villemoes@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