[L] Change in coreboot[main]: lib/{fit,fit_payload}.c: Enhance support for FIT images

[Benjamin Doron (Code Review)]

Attention is currently required from: Felix Held, Julius Werner, Lean Sheng Tan, Matt DeVillier, Maximilian Brune, Philipp Hug, ron minnich.

Benjamin Doron has posted comments on this change by Benjamin Doron. ( https://review.coreboot.org/c/coreboot/+/84796?usp=email )

Change subject: lib/{fit,fit_payload}.c: Enhance support for FIT images
......................................................................

Patch Set 10:

(9 comments)

File src/lib/fit.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/209a886b_15797b65?usp... :
PS4, Line 121: be32_to_cpu(*(size_t *)
> > I'm surprised there are two ways to convert endianness in coreboot. […]

Done

https://review.coreboot.org/c/coreboot/+/84796/comment/f2a1dea2_bee13cdd?usp... :
PS4, Line 523: 			}
> Simon Glass told me elsewhere that `loadables` in the config node is the supported way to do this. […]

https://github.com/tianocore/edk2/pull/10570/commits/50583c8509539d943575b1a... is not ready. I'll mark this as a TODO comment in the code, tag myself, and mark this as done for now.

File src/lib/fit_payload.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/bc522ddd_27b37854?usp... :
PS6, Line 18: __weak void platform_fdt_add_secondaries(struct device_tree *tree, const char *name, struct region *secondary)
> Ah, that's fair. […]

We discussed this on another comment. Done

https://review.coreboot.org/c/coreboot/+/84796/comment/af8f91f4_8a6bab0a?usp... :
PS6, Line 116: 	secondaries.size = kernel->size - primary_image->size;
> You could underflow in the size if `region_size(kernel)` is smaller than `primary_image->size` for w […]

Since splitting firmware/kernel handling, I believe this doesn't apply anymore.

https://review.coreboot.org/c/coreboot/+/84796/comment/b89376e2_c1700164?usp... :
PS6, Line 118: 	/* We may want to check if relocation is needed and then at least do something:
> As it happens, I've just realised that our secondary images never need relocation: they're UEFI FVs, […]

Done

https://review.coreboot.org/c/coreboot/+/84796/comment/cc47b02b_bafc82e0?usp... :
PS6, Line 131: 			continue;
> As it happens, I'm not sure how much meaning `require-fit` has anymore, now that I've made EDK2 no l […]

Done

https://review.coreboot.org/c/coreboot/+/84796/comment/786e138c_7e1ee9ac?usp... :
PS6, Line 274: 	if (!fit_payload_arch(payload, config, &code, &fdt, &initrd)) {
> This makes sense. […]

I think this is valid for now: I don't enable building Linux kernel payloads as FITs for x86, which is the one missing the support

File src/lib/fit_payload.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/30bdf87c_9842b034?usp... :
PS9, Line 29: 	printk(BIOS_ERR, "WEAK: %s:%s() called to add %s to FDT\n", __FILE__, __func__, name);
Right, I see your point. I've occasionally made this argument myself, that device trees are containers for data, so it doesn't and can't specify every kind of configuration parameter that exists. Since there's no spec to contain this idea, I'll just drop the code from here and have the follow-up add a callout in.
> You mean the one that only goes up to section 3.4 here: https://fitspec.osfw.foundation/ ? ;)
>
> It sounds like I need some extra context that's not yet officially published(?) and maybe then I'll have an easier time following what all this is supposed to do.

Oh, I see what happened. I was looking at the PDF, where the introduction is "chapter 4." I'm referring to the introduction, subsection .1, the second paragraph.

https://review.coreboot.org/c/coreboot/+/84796/comment/ebfa1d3d_32eab895?usp... :
PS9, Line 403: &fdt, &initrd
> Oh, okay, I see how the FDT thing gets added in the later patches. […]

Ah, yes, that's true. I didn't think of that, thanks. I'm going to hope that we didn't discover a ramdisk property previously, since a FIT with a firmware, rather than a kernel, should never be indicating a formal ramdisk. That's a Linux concept, and even if Linux is the internal environment of a firmware, the detail of its initrd should probably be hidden.

I'm going to add an `assert(!config->ramdisk)` statement into fit_extract_firmware, and move the ramdisk code in with fit_extract_kernel. That should do it, and draw attention in case someone specs or implements differently.