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 6:
(1 comment)
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/e288248c_ec8a20dd?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 […]
As it happens, I've just realised that our secondary images never need relocation: they're UEFI FVs, which I believe are position-independent, and they contain PEs. Those are probably (read: hopefully?) using RIP-relative addressing on x86, and in general, EDK2's loader will be doing relocation if the FV's PEs need it.
Yes, that's pretty much it. I didn't want to lose the Gerrit comment so I added it to the code. Sure, we could spend time coding up a perfect solution, but it doesn't seem worth it: we at 9elements want to get UPL support in, and other people clearly haven't thought that FIT relocation is worth supporting, since the existing code didn't have it.
And regarding the other props: I had seen a number of props in our FITs that weren't supported, and I was planning on digging into them sometime. I've done that now, and they're either UPL customisations or don't have all that much information to offer. For example, sure, we could start parsing `arch` (and EDK2's build script is getting that wrong too, so I'd have yet one more thing to fix...) but it doesn't matter. We really shouldn't be getting that wrong at build time.
I'm going to write some code to detect if we need relocation, and then just have it print an error. Clearly, relocation isn't something we need all that often.
-
Benjamin Doron (Code Review)