Attention is currently required from: Felix Held, Julius Werner, Lean Sheng Tan, Matt DeVillier, Maximilian Brune.
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 5:
(7 comments)
File src/include/fit.h:
https://review.coreboot.org/c/coreboot/+/84796/comment/0388d448_c540f889?usp... : PS4, Line 19: uintptr_t entry_offset;
Yeah, they are, and I will. […]
Done
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/a0122802_364837da?usp... : PS4, Line 121: be32_to_cpu(*(size_t *) I'm surprised there are two ways to convert endianness in coreboot. Is one just the internals for the other? Either way, that's done.
Also, if you do need to directly dereference (and hopefully we should have enough helpers for every case so you never do), never use architecture-dependent-length types like size_t or uintptr_t.
Ah, I think I see now. As I saw it, I was casting a data pointer (probably `void *`) into a natively sized pointer to dereference, then swapping the bytes for converting it. But this doesn't work when dealing with external data.
There are multiple problems though: - EDK2 has no address-cells and size-cells props - EDK2 might not comply with its own props: the implementation as of patchset 5 reflects EDK2's build. If data-offset is a kind of address, well, they generate it as a fdt32_t anyways - Are you sure about `cells != prop->size * 4`? I'd expect `cells * 4 != prop->size`. This seems to be the case, and it's fine if this was pseudo-code, but I can't actually test it because of the above
So, I have no compliant FIT to test. I've asked around for a FIT to look at. For the moment, I'm pushing this enhancement as a follow-up. Is that okay with you? Since all of these are additions, we know it won't break for previously supported FITs.
https://review.coreboot.org/c/coreboot/+/84796/comment/5b5c5e37_d46fd584?usp... : PS4, Line 136: else if (!strcmp("entry-start", prop->prop.name))
If there is no semantic difference between this and the normal entrypoint address that we always use […]
Done
https://review.coreboot.org/c/coreboot/+/84796/comment/f35d3756_1eb4dd17?usp... : PS4, Line 398: "processing config %s.\n", config->name);
It still feels weird returning 0 from here. […]
That's fair, we'll make each function responsible for its task. This one isn't driving the flow.
https://review.coreboot.org/c/coreboot/+/84796/comment/a003a076_12f3c3c5?usp... : PS4, Line 491: config->name);
This changes behavior for kernel loading. […]
My thought was that if somebody implemented UPL support for LinuxBoot, the names in the config nodes might be `(kernel, initrd)` rather than `(firmware, initrd)`. In which case an FDT will be generated by the follow-up, not inserted in the FIT. I've since confirmed that the node names would be `(firmware, initrd)` though, so I can change this.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/873ba2ed_e0f3fb14?usp... : PS4, Line 171: kernel
Now that this isn't always a kernel, should we name it something different (maybe `code`)?
Done
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/23391615_92cca6b9?usp... : PS5, Line 105: load_secondaries
This doesn't even make an effort to check if a load address was requested. […]
Done
-
Benjamin Doron (Code Review)