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:
(3 comments)
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/1c20a0bb_5955f105?usp... : PS9, Line 250: if (!bootmem_walk(fit_place_mem, firmware))
You shouldn't need to walk if you only want one specific address, you can just use `bootmem_region_t […]
The advantage of walking is that if no specific load address is requested, then `bootmem_walk` gets the 'minimum supported address' as 0 and just gets us any piece of memory available. It supports the behaviour of both functions in one.
https://review.coreboot.org/c/coreboot/+/84796/comment/c514870f_ef8fd33d?usp... : PS9, Line 307: if (secondary_image->load_address && secondary_image->load_address != region_offset(&secondary_region)) {
Same here, I'd suggest to first check if the load address is given, and if it is just check `bootmem […]
I don't think this is going to end up looking similar to what it looks like at the moment. Especially when `bootmem_allocate_buffer` already does a `bootmem_add_range`, so we'd have to put more code under an if-block (whereas at the moment, we're just selecting an offset).
If we could guarantee that secondary images would *never* request a load address (and bring relocation information if needed), this does simplify down to `bootmem_allocate_buffer`, yes. But I'm not sure I can guarantee that
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/200393fe_1694ba1b?usp... : PS11, Line 203: if (config->ramdisk &&
Can merge this into previous `if` now.
True, but it means nesting if-statements or a `if (!config->ramdisk) goto end;`
I think it looks neater as is, do you really prefer this?
-
Benjamin Doron (Code Review)