Attention is currently required from: Jakub Czapiga, Julius Werner, Nico Huber, Philipp Hug, ron minnich.
Maximilian Brune has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/79907?usp=email )
Change subject: [RFC] region: Hide struct region members ......................................................................
Patch Set 8:
(1 comment)
File src/arch/arm/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/79907/comment/3c7fcd24_67b2b41a?usp... : PS8, Line 3: /* FIXME: should use the high-level region api */
I don't think we have an actual maintainer for the FIT payload support. I have only tried to use a FIT payload once (~1 year ago) and at the time it didn't even work (I think). I wouldn't be surprised if it still doesn't work and no one noticed, since no one uses it. If it were up to me I would just remove the FIT payload entirely and just use SELF (maybe generate a SELF from a FIT in build time like we do with everything else). But that is something for another time/patch.
Ok, now I'm surprised. I somehow expected it'd be used with RISC-V... what do
you use as payload there? I also had the impression that it would be needed for the UPL thing. But even then, adding it as SELF might be reasonable shrug IIRC, it was originally introduced with the canceled ARM server project (Cavium). When I saw it adopted by other architectures, I thought it's used.
I usually use Linuxboot (either `CONFIG_ELF` or `CONFIG_FLAT_BINARY`). So I start coreboot -> Linux (as payload). That way I can test if all devices work as expected early on in Linux without needing support from an extra payload (like grub, u-boot ...). I can also test it even if there is yet no storage interface working (e.g. USB, PCIe) for a given platform, since the payload is on the flash anyway. But even if I would use a different payload I would just use `CONFIG_ELF` and `CONFIG_FLAT_BINARY`. I don't know what the reasons were to add runtime support/parsing for a FIT payload, but for RISC-V on coreboot that is usually an unnecessary overhead. We always compile the payload into coreboot anyway, so I don't see a reason to parse an entire FIT image in runtime. I mean we don't even do that for ELF executable (hence SELF was created). But I guess there are/were reasons for doing it the way it is now. I know FIT has support for multiple payloads, so you can choose at runtime which one to load. So maybe that is the reason. Although I guess you could accomplish something similar with the `fallback/payload` stuff. But anyway I diverged.
It just wouldn't be right to leave the FIXME, I guess. I'll think about it.
I would also prefer if you fixed it (and maybe completely remove the macro), but I also don't mind if you currently don't have time for that.