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 13:
(7 comments)
Patchset:
PS13: Thank you for all the reviews, Julius, and for helping me fix up EDK2's bugs/quirks by showing me coreboot's implementation and view on FITs!
File payloads/Kconfig:
https://review.coreboot.org/c/coreboot/+/84796/comment/1b29e6fa_14449ad9?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 pa […]
Done
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/1cec744d_52948c87?usp... : PS11, Line 512: strcmp(config->name, default_config_name)
I thought I pushed that, it's in my local copy. It might've gotten lost in my git stashes. […]
Ugh, no, this was a rebase error: it landed in the follow-up instead. Fixed now.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/ea4e4e35_75d9ca09?usp... : PS6, Line 271: code.size += image_chain->image->size;
In the kernel case `fit_payload_arch()` determines the uncompressed size and and modifies the `kerne […]
UefiPayload doesn't support compressing FIT image nodes yet (it's my belief that EDK2-based Platform Init treats the whole .FIT file as a raw file inside a UEFI FV, and applies compression/decompression at that level), so I hadn't gotten around to this earlier, because we wouldn't encounter it yet anyways.
However, since we decided that "firmware" can refer to UPL at the moment, because we don't have another project/ABI to support at the moment, I'm decoding `uncomp-size` if present and we'll allocate that much memory instead.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/c439952e_434e62fe?usp... : PS9, Line 22: printk(BIOS_CRIT, "WEAK: Generic code called to load a FIT kernel!\n");
Right, like in the other comment, I think I answered some of these before pushing my work on new yea […]
Accidentally slipped into the follow-up instead.
https://review.coreboot.org/c/coreboot/+/84796/comment/2f1f61e2_eba42b72?usp... : PS9, Line 401: bool status = false;
FWIW, the use of a boolean to report the result of an "action" type function here was always wrong a […]
I'd rather not continue to expand the scope of this patch by changing function prototypes both here and elsewhere in the tree, this was about 50 lines once. Granted, that's because I imported a bug + an assumption that a bootloader/Platform Init doesn't have adequate FIT support from EDK2 (and so I wrote code that copied the FIT header into memory too), so this was necessary and I agree that the patch is better this as it is than before. Genuinely, thank you for the help, and the help in finding EDK2's bugs through showing me coreboot's perspective on FITs!
At this point, I'd rather prioritise bigger issues and then get this in, if that's alright
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/9ca65f1a_ec858121?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 shou […]
Oh, it's as good a time as any, and it's not a big deal. Done.
-
Benjamin Doron (Code Review)