Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36622 )
Change subject: drivers/fsp2_0: drop support for FSP-T ......................................................................
Patch Set 4:
So I just saw this change.
I am fully aware that emotions are hard to control, and that it's too easy for them to run out of control, and end up taking matters personally. Because attacking each other is futile, I would encourage everyone to be constructive: work towards finding a solution that satisfies everyone's needs as much as possible.
After reading the conversations, I understood the following:
Reasons to drop FSP-T: - An open-source alternative already exists in coreboot. - FSP-T is undocumented, and its integration is rather fragile. - Largely untested on upstream. Sometimes, changes that were build-tested by Jenkins did not build when selecting FSP-T.
Reasons against dropping FSP-T: - The native CAR init code does not work with all possible configurations. For example, Bootguard.
I may have missed something from either side, or understood something the wrong way. Any comments, clarifications and suggestions are very welcome :)
One of the points that were brought up is the microcode update problem Nico explained. As I understand it, an invalid configuration, which could be detected at build-time, instead results in a silent boot failure. Most people can only debug coreboot using console logs and a postcode card, usually by placing debug statements around to narrow down where things go wrong. Since FSP's source code is not public, adding debug prints to FSP is not an option for most people. Moreover, there are next to no known-working configurations with FSP-T to compare against. As a result, trying to make FSP-T work without privileged information is extremely frustrating.
Another thing to consider: Bootguard. As I understand it, coreboot's CAR init code does not take Bootguard into account, and would not work correctly if Bootguard is enabled. It doesn't seem to be very complicated: basically, the Bootguard ACM sets up CAR already, so coreboot code should not do it again. However, getting a Bootguard-enabled board on which to test this on is pretty much impossible. Since Bootguard requires firmware to be signed, and the signing key is kept secret, running a random coreboot image on a commercially available board with Bootguard is just not possible. From another perspective, the current situation means that that dropping FSP-T support also implies dropping Bootguard support.
Personally, I would prefer the open-source approach. I would not mind having the option to use FSP-T, as long as it doesn't rot away. This means that it should be build-tested automatically. In addition, there should be known-good configurations to use as a reference: regularly-updated successful boot records for several boards of each platform with FSP-T support, for example board status reports. This is especially useful when doing major overhauls to coreboot's structure, like C_ENVIRONMENT_BOOTBLOCK. Otherwise, the technical debt quickly builds up, and then rewriting everything from scratch is easier than fixing the resulting mess, which nobody wants to do.
So, I think that getting CB:36682 boot-tested and merged in will be good for everyone. If so, what are we waiting for?
-
Angel Pons (Code Review)