Attention is currently required from: Benjamin Doron, Felix Held, Lean Sheng Tan, Matt DeVillier, Maximilian Brune.
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 6:
(19 comments)
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/ee4e7333_3adaf0e9?usp... : PS4, Line 121: be32_to_cpu(*(size_t *)
I'm surprised there are two ways to convert endianness in coreboot.
The difference is just about whether you already have an integer or want to read from a pointer. For cases like this the enc/dec variants can make things look a little cleaner.
EDK2 has no address-cells and size-cells props
They should fix that, but until they do we'll assume the default of 2/1.
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
Well... to be fair, the FIT spec doesn't exactly say what length the data-xxx values are supposed to be (it does say for `load` and `entry`, though). I kind of assumed it would make sense to use #xxx-cells, but it does seem dangerously underspecified. Maybe you should reach out to the FIT spec maintainers to get some clarification.
Just blindly assuming 32-bit seems wrong to me unless the spec explicitly says so. But since we know it should always be a single value, I guess we could just read the size and decode as 32 or 64 based on that (basically the function I suggested above, just without checking the `cells` value and matching `prop->size == 4` and `prop->size == 8` instead? If you're introducing it as a new helper, maybe we could say that passing `0` for the `cells` parameter makes it use that mode, so that it's also useful for cases like `load` below).
Are you sure about `cells != prop->size * 4`? I'd expect `cells * 4 != prop->size`.
Right, that was a typo, sorry.
https://review.coreboot.org/c/coreboot/+/84796/comment/bcd4c98e_e3d5627b?usp... : PS4, Line 523: }
Yeah, that's pretty much what's going on here. […]
Hmm... can't say I really like that design tbh, it doesn't really match how FIT images normally work as a container of images that the configuration node picks the right ones out of. It would have fit in better with the existing design if they added a prop that lists those names explicitly, e.g. `extra-images = "uefi-fv", "bds-fv";`.
But okay, if they want it that way I guess we can load it that way.
File src/lib/fit.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/58770343_e748cb52?usp... : PS6, Line 19: static void *fit_bin_base; (FWIW the use of all these static globals seems like a bad idea. They would actually break things if we ever try to load a FIT, fail because of some parsing error half-way through, and then try to load some other fallback FIT instead. But the existing lists here are a bigger problem than this new base pointer, so I guess it doesn't make things worse to add it for now, but it would be great to eventually refactor all of that into a state structure that gets created in `fit_load()` and passed through all the functions instead.)
https://review.coreboot.org/c/coreboot/+/84796/comment/c30f1999_3e3a191d?usp... : PS6, Line 119: if (!strcmp("data-offset", prop->prop.name)) nit: IIRC coding style says that if one if-else branch has curly braces, all of them must use them (except for the `else if` on a single line, that's allowed).
https://review.coreboot.org/c/coreboot/+/84796/comment/273d7988_2c724e72?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 spec and the default cells value matches what you need here.
https://review.coreboot.org/c/coreboot/+/84796/comment/af68d47e_6f58a885?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: ``` if (default_config_name && !strcmp(config->name, default_config_name)) default_config = config;
if (fit_update_compat(config) && config != default_config) continue;
printk(BIOS_DEBUG, "FIT: config %s", config->name); if (config == default_config) printk(BIOS_DEBUG, " (default)"); ```
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/3a63fe8c_86f0f3fe?usp... : PS6, Line 18: __weak void platform_fdt_add_secondaries(struct device_tree *tree, const char *name, struct region *secondary) I think weak functions should only be used when the base case is a very trivial "default" implementation (e.g. a no-op, or returning some hardcoded value). It's not good to implement significant logic in weak functions because it is unintuitive that just implementing that function in the platform part removes stuff that would otherwise happen.
This goes way beyond a no-op or default value so I would say it shouldn't be implemented like this. I'm not exactly sure where the `/firmware/coreboot/image@...` stuff is coming from (note that for the Linux kernel, `/firmware/coreboot` bindings are clearly specced in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu..., you shouldn't just invent new ones that aren't upstreamed there)... if they're part of UPL, maybe guard this by the UPL Kconfig, or if they're their own thing maybe add a new Kconfig.
https://review.coreboot.org/c/coreboot/+/84796/comment/d9defc06_bed6b9cc?usp... : PS6, Line 26: 64 nit: `sizeof(node_name)`
Also, maybe `assert()` that return value is `<= sizeof(node_name)`
https://review.coreboot.org/c/coreboot/+/84796/comment/cd7ceb6f_676f2b9d?usp... : PS6, Line 31: dt_add_reg_prop(image_node, &addr, &size, 1, 2, 1); Also, adding a `reg` prop to `/firmware/coreboot/image@...` would just formally require you to add `#address-cells`, `#size-cells` and `ranges` props to `/firmware/coreboot` which it normally doesn't have.
https://review.coreboot.org/c/coreboot/+/84796/comment/f99c9fa1_d4cd5da1?usp... : PS6, Line 105: kernel `code` (or maybe `target_region` or something like that if you want it to be clearer in context; either way, it's usually not a kernel here)
https://review.coreboot.org/c/coreboot/+/84796/comment/d8319adb_ae35b520?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 error?
https://review.coreboot.org/c/coreboot/+/84796/comment/7977230d_a0e92ca3?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, and the FIT code was one of the remaining issues... but at least when adding new code we should try to stick to the external APIs (e.g. `struct region secondaries = region_create(region_offset(kernel) + primary_image->size, region_size(kernel) - primary_image->size);`; you could also use `region_create_untrusted()` to integrate the size check right here).
https://review.coreboot.org/c/coreboot/+/84796/comment/a43cfe5a_487307f7?usp... : PS6, Line 118: /* We may want to check if relocation is needed and then at least do something: nit: 5 lines is a bit long for short multiline comment style, maybe better use ``` /* * ... */ ``` here.
(Also, I don't quite understand what the comment is trying to tell me tbh. Are you saying that this code is not yet doing the right thing here? Then why not write code that does instead?)
https://review.coreboot.org/c/coreboot/+/84796/comment/54e3f31a_aaaaec60?usp... : PS6, Line 131: continue; Shouldn't you return a hard error that makes the parent function fail in these cases? That's what we do elsewhere when the image is broken.
https://review.coreboot.org/c/coreboot/+/84796/comment/4546c568_2204f3ae?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 just added, but that's not what this is — this is the entire remaining space for adding more images. You'd have to modify `extract()` to return the decompressed size if you want to know the exact size of only that image.
Actually, you need to do that anyway, because your `secondaries.size -= image_chain->image->size` below is not correct in the case of compressed images.
https://review.coreboot.org/c/coreboot/+/84796/comment/f97e90c3_1e9b53b5?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 `<region.h>`. Could also use that above then, to initially create this as ``` struct region secondary = kernel; if (region_shrink(&secondary, primary_image->size)) ...error... ```
https://review.coreboot.org/c/coreboot/+/84796/comment/2ce82212_dbff06f4?usp... : PS6, Line 271: code.size += image_chain->image->size; Yeah, this also doesn't do the right thing with compression. In general I think you need to decide whether you either want to forbid compression for these secondary images entirely (then that should be tested), or decode and make use of that `uncomp-size` prop in the UPL spec, or just allocate a huge area in bootmem to begin with and later shrink that back down once you have decompressed everything and know the actual size.
https://review.coreboot.org/c/coreboot/+/84796/comment/787f2613_0832ea80?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 really makes sense for your firmware image node. The point of calling into arch code here is to implement all the Linux kernel loading restrictions and quirks for different architectures. But you're not loading a kernel and none of those make sense for you. The only reason you're getting away with this right now is that you just disallow your case for Arm and Arm64, and only implement it for x86 which didn't have any FIT arch code before.
I think it would make more sense to split this between the two cases: maybe rename `fit_payload_arch()` to something like `fit_linux_arch()` or `fit_place_linux()`, and create a different function (e.g. `fit_place_firmware(struct prog *payload, struct fit_config_node *config, struct region *code)` — shouldn't need the FDT or initrd parts which your case doesn't have anyway) to implement the firmware case where you just find a random free spot in bootmem. That probably doesn't even need to be arch-specific then. You could also do the secondary image stuff in there.
https://review.coreboot.org/c/coreboot/+/84796/comment/19ce45f9_92364bb2?usp... : PS6, Line 306: dt_print_node(dt->root); This prints the whole DT, right? I don't think we should do that by default? If you think it is useful for debugging, maybe add a Kconfig for it?
-
Julius Werner (Code Review)