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:

1. 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.

2. 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.

3. 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.