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 payloads/Kconfig:
https://review.coreboot.org/c/coreboot/+/84796/comment/614d2330_1fe889af?usp... : PS9, Line 186: default y if PAYLOAD_LINUX && (ARCH_ARM64 || ARCH_RISCV || ARCH_ARM)
nit: unclear why these shift around?
Oh, I think that made it align with other instances of this dependency in the file, which allowed me to search for it more easily. If a shorter diff is preferred, I can change it back.
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/d9b69d6e_05e68ef3?usp... : PS6, Line 136: image->load_address = be64dec(prop->prop.data);
These two still need to be #address-cells dependent, since in this case it actually says so in the s […]
Right. I didn't realise at first that `dt_find_node_by_path` actually initialises addr_cells and size_cells to (2, 1) immediately, then assigns them again if it finds the properties. This is why I held off on making the change. Anyways, done.
https://review.coreboot.org/c/coreboot/+/84796/comment/013c4c40_0406b009?usp... : PS6, Line 509: if (fit_update_compat(config) && strcmp(config->name, default_config_name))
Let's rearrange these lines so you don't need to `strcmp()` twice: […]
Fair, I may as well not de-optimise the code. Done.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/59fe1f00_ff7f3279?usp... : PS6, Line 26: 64
Checking the return value would ensure that somebody will notice if you overrun the buffer for some […]
Right. That would be painful, especially when we don't dump out the entire tree. This code is gone, so I've just implemented the fix in the follow-up.
https://review.coreboot.org/c/coreboot/+/84796/comment/34b6147c_78fa0991?usp... : PS6, Line 31: dt_add_reg_prop(image_node, &addr, &size, 1, 2, 1);
My understanding is that every node that has children with `reg` properties represents a bus, and th […]
Ah, I see. Thanks for the explanation! I've dropped the implementation from here, so I'll just implement this in the follow-up.
https://review.coreboot.org/c/coreboot/+/84796/comment/0717f94a_a3895a9d?usp... : PS6, Line 131: continue;
Hmm... well, you're the expert on this boot mode but in general that seems fishy to me. […]
As it happens, I'm not sure how much meaning `require-fit` has anymore, now that I've made EDK2 no longer need to read its own FIT header if we provide it with the addresses of its secondary images (FVs). But I agree, it's hard to make certain guarantees if we fail to load some images.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/704ae649_a93c3859?usp... : PS9, Line 22: printk(BIOS_CRIT, "WEAK: Generic code called to load a FIT kernel!\n");
nit: to make it a bit more clearer what it means to see this I'd maybe print something like "Booting […]
Done
https://review.coreboot.org/c/coreboot/+/84796/comment/719c3656_9d43335c?usp... : PS9, Line 29: printk(BIOS_ERR, "WEAK: %s:%s() called to add %s to FDT\n", __FILE__, __func__, name); The big picture that I have in mind is to get it into the FIT spec as a counterpart of `loadables`. Then, a FIT config could request additional images to be loaded into memory, and if they were position-independent or needed relocation, we'd tell the entrypoint where they are. I've asked Simon about this, and we'll look into a way to do it, if possible. Do you have any ideas for paths? Possibly, `/firmware/secondary-images`. If we decide on some path and a PR for the spec is up, I'll change this code to reflect that and drop the callout.
To answer your questions more directly:
Is this an error? If so, why does it need to be a weak function at all,
rather than just a normal function that causes a compile error if not implemented?
Whether it's an error or not is intertwined with our conversation at https://review.coreboot.org/c/coreboot/+/84796/comment/54e3f31a_aaaaec60/.
I still don't quite get why you want to make this look like a platform
hook tbh, it looks like this is a UPL thing, but UPL isn't really platform-specific.
I'm probably overusing the word "platform," yes. If it isn't something specific, like 'board,' 'silicon,' or 'architecture' code, and it's instead a driver, a payload specific interface, etc, I'd probably call it "platform" code. I see all these as the things that build up a specific platform for its use-case, in a way that's different from any other physical system. There might be a better word for this, but this is the context for what I mean so that we can avoid miscommunicating here. I can rename it if you've got something clearer in mind (assuming it should continue to be a weak function).
I've answered part of this above: I'd like it to be part of the FIT spec, but it's not (yet). So instead, I've written the function name so that it reads like it needs payload/platform feature implementors to write an implementation.
Isn't all this "firmware payload" stuff UPL-specific anyway? What else would it be used for?
No, see section 4.1 of the FIT spec. It's also for ATF, OpenSBI, U-Boot, and somehow, FPGAs. I have only been testing this with UPL though, yes.
https://review.coreboot.org/c/coreboot/+/84796/comment/e9304ef7_9402329b?usp... : PS9, Line 403: &fdt, &initrd
Do either of these make sense for the firmware case? FDT is always going to be the runtime-generated […]
Yes. `fdt` is the region where the runtime-generated FDT will be packed, in the end, and I've checked `initrd` with Sheng. If LinuxBoot ever implements a UPL interface, we'll implement it in userspace, like u-root. But we still think it'll be a `"firmware"`, in the sense of its entrypoint ABI conforming to the UPL spec.
-
Benjamin Doron (Code Review)