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 7:
(10 comments)
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/33602ac6_c4988251?usp... : PS4, Line 523: }
Hmm... […]
True, and the current solution runs into the issue of how images could have clashing load addresses, but we'll 'load' all if the config node has a "firmware." I'll bring this up with UPL and FIT people, although it runs into the issue of handling project specific customisations in coreboot's common code, and I don't like that. It'd be preferable to upstream this to the FIT spec. For now, I'll leave this open, but I don't want to be chasing perfection and delay getting this in and ready for people to use as a result. If it's taking too long, I'll likely mark a comment in the code, close this, and we'll deal with it in a follow-up someday.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/37e8b8f5_7ed2c308?usp... : PS6, Line 26: 64
nit: `sizeof(node_name)` […]
It's more maintainable, sure. Done.
I don't see a point in checking the return value though. We're not querying and allocating strings on the heap.
https://review.coreboot.org/c/coreboot/+/84796/comment/2940675a_af22a975?usp... : PS6, Line 31: dt_add_reg_prop(image_node, &addr, &size, 1, 2, 1);
Also, adding a `reg` prop to `/firmware/coreboot/image@... […]
I realised this (about the cells) after carefully looking through the UPL FDT, and I've now implemented it over there. I don't understand why `ranges` is required though. I've checked the devicetree spec, and it says it's for describing the addresses supported or mapped on a bus. `/firmware/coreboot/`/`options/upl-images/` aren't a bus, they're backed by `/memory` nodes
https://review.coreboot.org/c/coreboot/+/84796/comment/14b774a0_8d6e63b2?usp... : PS6, Line 105: kernel
`code` (or maybe `target_region` or something like that if you want it to be clearer in context; eit […]
Done
https://review.coreboot.org/c/coreboot/+/84796/comment/d5c33f90_a4caffc7?usp... : PS6, Line 112: return;
Why is this here? If there are secondaries defined and they don't fit, shouldn't that be a hard erro […]
No, the comment explains what's happening: if the region is the same size as the primary, clearly we never grew it to account for some secondaries. I thought this was clear, but it doesn't matter. The implementation I'll push next makes a stricter separation between kernel/firmware, which means we can avoid `fit_payload_arch()`, which means the code loading the secondaries can allocate regions itself, rather than how I pre-grew the size of the requested `code` region.
https://review.coreboot.org/c/coreboot/+/84796/comment/28b68341_61d3d970?usp... : PS6, Line 116: secondaries.size = kernel->size - primary_image->size;
I know Nico never quite finished his work on hiding region struct members (CB:79907), unfortunately, […]
Done. What makes you think of `region_create_untrusted()` though? Are you worried about an underflow/overflow situation? I wouldn't expect it from a payload, it shouldn't be malformed (although, famous last words considering how many bugs or bad assumptions I've been addressing while working on this project. Moving on). And even if a `size_t` is 32-bit, a 4G payload is simply impossible, so we're not going to
https://review.coreboot.org/c/coreboot/+/84796/comment/f24aaea4_95c6fdc8?usp... : PS6, Line 134: secondaries
Your use of this in the function above implies that it should be the space taken up by the image jus […]
Right. This is a mistake I caused by changing the design in the middle of development without rethinking the assumptions elsewhere. Anyways, the firmware/kernel split fixes this, because I'm allocating regions for each, rather than shrinking one big one.
We'll talk about compressed images in #271, so I'm marking this as done.
https://review.coreboot.org/c/coreboot/+/84796/comment/3056ebf4_9b4d0280?usp... : PS6, Line 137: secondaries.size -= image_chain->image->size;
Maybe we should create a new helper called `region_shrink(®ion, size)` for this kind of thing in […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/84796/comment/685f9333_702b442d?usp... : PS6, Line 271: code.size += image_chain->image->size;
Yeah, this also doesn't do the right thing with compression. […]
How is this not a concern for the primary image? We read in the data size and compression type the same way. I'd ask how this isn't a concern for any other project, but if only Linux is using it at the moment, then yes, it does have its own header.
Either way, I'd rather not use a UPL customisation in common code. I think the FIT spec is lacking here, so I've asked about upstreaming the `uncomp-size` property to FIT. I'll leave this open for now.
https://review.coreboot.org/c/coreboot/+/84796/comment/42dcaef1_37ccd180?usp... : PS6, Line 274: if (!fit_payload_arch(payload, config, &code, &fdt, &initrd)) {
After looking through all of this some more I'm not really convinced that reusing this function real […]
This makes sense. It also addresses future issues I was postponing: perhaps the x86 code I had written could also extract a kernel, but the AARCH64 code was going to get more difficult: how do we implement support for quirked and unquirked code together?
Done, and this addresses several other comments. Although this solution redefines what `CONFIG_PAYLOAD_FIT_SUPPORT` should mean: now, all architectures support FITs, but not necessarily Linux kernels. We should make this clearer in Kconfig.