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

[Julius Werner (Code Review)]

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

Julius Werner 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 11:

(6 comments)

File payloads/Kconfig:

https://review.coreboot.org/c/coreboot/+/84796/comment/16d555a5_a7681982?usp... :
PS11, Line 33: 	depends on ARCH_ARM64 || ARCH_RISCV || ARCH_ARM || ARCH_X86
Actually, does this line still make sense at all? You aren't adding any x86-specific code in this patch anymore, and the firmware loading support would work on all architectures (e.g. also PPC64 or some new one we might add in the future). I'd say just take out the architecture dependency completely here (and below).

File src/lib/fit.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/eaaed53f_50b080df?usp... :
PS11, Line 512: strcmp(config->name, default_config_name)
This can crash if `default_config_name` remains `NULL` because there is no `default` prop. I'd recommend writing this like I suggested in https://review.coreboot.org/c/coreboot/+/84796/6..11/src/lib/fit.c#b509

File src/lib/fit_payload.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/6783034d_dfb7973d?usp... :
PS9, Line 22: 	printk(BIOS_CRIT, "WEAK: Generic code called to load a FIT kernel!\n");
> Done

String doesn't seem to be changed in latest patch set yet?

https://review.coreboot.org/c/coreboot/+/84796/comment/27d7d293_e683846a?usp... :
PS9, Line 29: 	printk(BIOS_ERR, "WEAK: %s:%s() called to add %s to FDT\n", __FILE__, __func__, name);
> 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.

Okay, but that doesn't really mean much. I think it is true that e.g. U-Boot uses FIT images to package and load later components in its boot process, but that doesn't make these components universal in a way that coreboot could use them as well. They are specific to U-Boot both in what they're actually doing and in their calling convention. Just because two code bases may both be using the FIT image container doesn't mean what they're putting in that container could be compatible.

So I don't think it makes sense to try to write any coreboot code that just allows booting "any" kind of FIT image — that is impossible. A FIT image must always come with a defined calling convention (which tells you which images to load and how to enter them). There may be some common practices that all those calling conventions tend to follow but that doesn't mean you could properly boot an image without them.

Right now AFAIK there are only two well-defined, portable FIT image calling conventions, Linux kernel and UPL. So I'd say let's focus on those and not worry about any others until they're officially specced and someone has a need for them in coreboot. That means that whenever `config->firmware` is set, you know you're in the UPL calling convention and you can just run whatever code you need to run to e.g. describe secondaries in the FDT in whatever way UPL wants to... you don't need to build an extensible interface where other non-UPL code paths could hook in yet.

(Also, I just noticed another problem with the implementation you used to have here: you aren't actually allowed to add DT nodes and properties at this point because you had already called `dt_flat_size()` to determine the total FDT size to place in memory beforehand. You can only overwrite properties that you know are already there with the same data size (or less), or you need to come up with some heuristic to calculate how much space you need to reserve extra in advance to ensure there'll be enough space for what you're planning to add later.)

File src/lib/fit_payload.c:

https://review.coreboot.org/c/coreboot/+/84796/comment/70dc51c6_b6a54e92?usp... :
PS11, Line 181: 	/* Collect info for fit_linux_arch_quirks */
This is totally off-topic to your patch (already a bug in the existing code), but I think there should be a
```
if (config->ramdisk)
  fit_add_ramdisk(dt, 0, 0);
```
here before the `dt_flat_size()`. Otherwise, it's possible that adding the ramdisk props below will grow the FDT beyond what was placed by the arch code. I think the current code was written with the assumption that FIT images would always have those props existing (with bogus values) already, but it's bad to blindly assume that, it would be better to just make sure.

https://review.coreboot.org/c/coreboot/+/84796/comment/451b194e_cfb65325?usp... :
PS11, Line 203: 	if (config->ramdisk &&
Can merge this into previous `if` now.