Hi Julius
So FDT has a few advantages over C structs for passing information between 2 separate programs: - reflection is easy - no problem of 'header mismatch' to have the right definition of a struct - easy to update with a text editor without needing to write code - No need to be careful about ABI, writing structs e.g. 64bit alignment
I agree with you that these, while nice, are not very compelling arguments to replace coreboot tables, which are very established by now. For instance the header mismatch problem does not really exist given how stable coreboot tables have actually been. There is also no use case of wanting to edit the payload handoff by hand. At last x86 Linux can be launched in around 200 bytes of stackless assembly code... That's not possible with FDT or at least I'm not aware of any stackless assembly written FDT parser.
For FSP it makes a lot more sense to improve the binary interface over the existing C struct: - reflection is a problem as FSP comes with default and printing out a struct in C is a pain - You never know whether the FSP binary will match a given header - FSP validation is a manual process afaict. The people doing it don't want to write code. That is why coreboot is not used in FSP validation. Intel uses UEFI so that there is a menu to change FSP values for validation.
So it's not that FSP would need the same information as the payload. The intersection is indeed almost 0. It's just that FDT is actually a nice format for 2 separate programs to talk to each other. For instance u-boot also uses it to pass information from spl to u-boot (bootloader) afaik.
I think Intel only wanted to try FDT as an interface for FSP if it's also used in the payload (read EDK2) handoff (don't ask me why). Maybe someone needs to convince Intel that using FDT for just FSP is a good idea regardless of how payload handoff is handled.
So basically, I'd like to make sure that we can agree to keep coreboot
tables as a first-class citizen now and in the future, and don't force a new format on anyone. I'm not trying to stand in the way of experimenting with new alternatives if people want to do that.
Having multiple ways to do the same thing is a maintenance problem. It was a fun experiment, but it has no place in the master branch if the end goal is just to have FDT live side by side with coreboot tables.
Arthur
On Fri, Nov 4, 2022 at 3:41 AM Julius Werner jwerner@chromium.org wrote:
Hi Arthur,
Thanks for the detailed response. I went ahead and watched your presentation as well. FWIW, I think there were a few technical inaccuracies about the current state of coreboot tables in your presentation that might be good to get out of the way first:
- You said that coreboot tables were GPLv2 licensed, and this could
be an adoption problem for payloads... however, while src/commonlib/include/commonlib/coreboot_tables.h has a GPLv2 header, the same definitions for all structures have always been available in payloads/libpayload/include/coreboot_tables.h with the BSD license, so there's no actual issue for payloads. This is just a consequence of the historic duplication we always had between coreboot and libpayload. Now that we have the infrastructure to share commonlib/bsd code between the two, I hope sooner or later someone will find the time to delete coreboot's old (GPLv2) headers and move the libpayload (BSD) headers into commonlib/bsd to be shared by both.
- You said that there was an ABI problem due to different alignment
requirements for 64-bit types on different architectures. This has actually never been true (unless someone made a mistake and slipped a raw uint64_t into the table definitions, but we are generally pretty vigilant in reviews about this so if it did happen it must have been fixed quickly). If you look at commit 0d304c18 (so old that I can't even link to Gerrit for it), we used to use a struct solution with custom accessor functions to create 32-bit-aligned 64-bit types in the past. More recently in https://review.coreboot.org/63494 we replaced that with __attribute__((aligned, 4)) which does the same thing in a less cumbersome way.
- You also said that the coreboot tables aren't versioned, which is
of course technically true. However, the general guideline that I think we have been trying to maintain in reviews is that coreboot tables should be backwards-compatible, meaning that once a tag has been widely used its structure layout should not change anymore (at most new fields might be added at the end so that the size field can be used to differentiate between both versions). Older versions of libpayload should always be compatible with newer versions of coreboot and as far as I'm aware we have done a pretty good job of maintaining that standard. When a structure needs to be updated, a simple solution is to just create a new tag and stop using the old one, and this has been done in the past (e.g. LB_TAG_VBOOT_HANDOFF has been deprecated and LB_TAG_VBOOT_WORKBUF basically contains the equivalent stuff in a different format today). Fundamentally, this is pretty similar to how FDT is used by Linux -- there's no explicit versioning for nodes or properties, but bindings are meant to be stable, and when you really need to change stuff up you can just make a new binding with different compatible string and deprecate the old one.
I think the fundamental question here is still: what do we want to achieve, and how does replacing coreboot tables with something else help with that. One of the things you mentioned was so that we might be able to use the same handoff format for FSP and the payload, and the first question that comes to my mind is... why? What does that actually give us? I think it's easy to sometimes fall into the xkcd.com/927 trap of unifying things just for unification's sake, because it feels "right", but ending up with a solution that's less suited for either target than the sum of the burden of carrying all individual existing solutions. The information that needs to be passed to the FSP is (according to my understanding, I'm admittedly not an FSP expert) completely different from the information that needs to be passed to the payload (except maybe for one or two tiny specks like console information?), so it's not like you'd actually want to reuse the actual data structure you built and pass both things the union of the stuff they each need to know, when the intersection of that is almost empty. So what other reason is there to unify the format, then? It's not like we have a ton of coreboot table support libraries or tooling that we could drop if we unified them with something else -- that's the beauty about a very simple C structure format like that, it's basically free to use.
The other aspect is the payload itself, and that whole "universal payload" idea (I assume that's what Intel is calling "USF" now, or is that something else?). I have to admit I'm just fundamentally very skeptical of that whole concept, and I just don't believe it's gonna work out in a way that's actually going to generate more benefit than problems for anybody. The way we use payloads nowadays is often very deeply tied to coreboot, with things like CBFS, verification, console, timestamps, VPD, etc. all implemented in a very coreboot-specific way (not just the handoff format but the very implementation of the thing itself). You can't unify those things without unifying those implementations which is practically impossible (i.e. if you unify every aspect of how everything works between different firmware projects then there's little point in having separate projects at all anymore, because they'd all be doing the same thing anyway). So in practice, I think the only thing a universal handoff would accomplish is that e.g. you can then have a payload that will boot from coreboot and start parsing coreboot's handoff, but then hangs because the flash access bindings it is looking for don't exist in coreboot's handoff and it doesn't know what to do with CBFS bindings. If you want a payload that can support more than one firmware, with or without universal handoff you'll have to implement specific support for each anyway (e.g. like GRUB has implemented CBFS support), and once you're doing that the extra cost of implementing a parser for a very simple, dependency-less handoff format like the coreboot tables is not really noticeable on top.
That said, I understand Intel wants to push this and there are people who want to try this idea out, and I don't want to block them just because I don't believe it'll work. So if you want to implement this as an experimental, default-off Kconfig option that can hopefully stay pretty isolated and self-contained in the code base, I'm fine with that (and I don't particularly care about CBOR vs FDT... I think both are pretty similar, both have tiny advantages and disadvantages for your goals, and both add a similarly large overhead compared to coreboot tables). But I wouldn't want this added as a replacement for the coreboot tables either right away or with some implicit agreement that we will transition there eventually. We don't even have any practical use cases for universal payloads yet, but we have plenty of big existing use cases that only combine coreboot with a single, coreboot-specific payload and don't have any plans to require any more flexibility on either of those sides in the future (ChromeOS is one of them), and I wouldn't want those use cases to suffer from being preemptively forward-ported onto a new solution that just adds a lot of extra weight without having any proven benefits as of yet.
So basically, I'd like to make sure that we can agree to keep coreboot tables as a first-class citizen now and in the future, and don't force a new format on anyone. I'm not trying to stand in the way of experimenting with new alternatives if people want to do that.