Change in coreboot[master]: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues
Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, Michael Niewöhner, Duncan Laurie, David Wu, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36328
to review the following change.
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
[RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues
To get some discussion started, list some heavy issues with FSP(-S). I've copy pasted some things together, and added a few words on top and bottom. Not sure yet, where this should lead. To convince Intel to contribute open-source code on their own, we might want to add a list of initialization steps, that should be open.
Feel free to amend this commit in any way :)
Change-Id: Ied0d8ad60fa487272a05fc73b16a7c343cc4d993 Signed-off-by: Nico Huber nico.huber@secunet.com --- A Documentation/fsp/fsp-s_discussion.md 1 file changed, 165 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/36328/1
diff --git a/Documentation/fsp/fsp-s_discussion.md b/Documentation/fsp/fsp-s_discussion.md new file mode 100644 index 0000000..7ac649a --- /dev/null +++ b/Documentation/fsp/fsp-s_discussion.md @@ -0,0 +1,165 @@ +Introduction +============ + +*FSP* is an ABI that allows silicon vendors to provide pre-compiled +binaries of their silicon-initialization code for the integration +into firmware frameworks such as coreboot. Current, 2.x versions +of *FSP* describe three binaries with rough responsibilities: + +* `FSP-T`: Sets up a temporary RAM environment (usually some sort + of Cache as RAM, CAR) among other things. + +* `FSP-M`: Initializes the memory (DRAM) controller, among other + things. + +* `FSP-S`: Performs various silicon-initialization steps left. + +However, what operations these binaries perform exactly and what +operations are left to the firmware framework, is neither specified +nor documented. + +Historically, all these responsibilities were left to coreboot. +Later, pre-compiled reference code for the memory-controller +initialization was introduced, then for other silicon init, and +finally there are attempts to replace coreboot's CAR setup with +FSP-T. + +While the FSP-M integration saves coreboot developers time, because +of the complex nature of the memory-controller initialization, the +integration of FSP-S turns out to be harder than implementing the +necessary code natively in coreboot. We'll discuss why that is the +case and provide an incomplete list of issues with current FSP-S +binaries. + + +Bug Fixing +---------- + +Many companies rely on the pre-built FSP binaries. Yet, there is +no common procedure to get bugs fixed or even acknowledged. This +leads to an increase of workarounds in firmware frameworks, +especially for older hardware generations. + + +Lack of Documentation +--------------------- + +Many of the initialization steps performed by FSP-S are merely +translating mainboard-specific configuration into register settings. +This is reflected by a list of hundreds of configuration options of +FSP-S. While these steps are quite simple, the additional translation +from FSP options to register values heavily increases the integration +costs. Not only are the register settings usually much better documented +than their FSP counterparts, but FSP also uses different terms so that +the existing hardware documentation often can't assist developers when +dealing with FSP. + +FSP-S also performs locking of several registers so that they can't +be written anymore afterwards. It is not documented what locks are +set by FSP. This creates heavy security issues for multiple reasons: + +* One either has to assume that FSP doesn't perform any locking + and do it redundantly in the firmware framework, or expensively + test what is locked and keep doing so for every new FSP revision. + +* Some registers may be locked before they are configured in a + secure manner. This may result in workarounds in the firmware + framework to configure things early. Sometimes it's even + impossible to do so, because relevant information isn't + available yet. + +This shows that no matter the many options that are implemented in +FSP-S, there will always be things that must be controlled by the +firmware framework. Locking should either be a separate, last step +in FSP, or be left entirely to the firmware framework. And most of +all, the locking has to be documented. + + +Arbitrary Interface Changes +--------------------------- + +Within coreboot, we try to keep code-compatibility with older hardware +generations. The FSP ABI isn't stable, however. Even when the hardware +doesn't change between minor platform updates, the configuration +settings of FSP are sometimes adapted without backwards compatibility +in mind. This increases costs, just to translate configuration data +from one representation to another only to let FSP translate it again. +In such cases, directly configuring registers from coreboot, would +save two layers of indirection. + +Another problem is the disappearance of options in newer FSP releases. +See [SpiFlashCfgLockDown UPD vanished after KBL] +(https://github.com/IntelFsp/FSP/issues/25), for instance. Current +coreboot code seems to still rely on this option. It boots, though, +only security assumptions are broken. Which again shows, locking +shouldn't be part of FSP, or at least optional. + + +FSP Switches SAI +---------------- + +A problem that multiplies with the lack of documentation is that FSP-S +switches to `POSTBOOT_SAI` on newer platforms. While it probably tries +to achieve the opposite, this undermines firmware quality and security. + +* In some firmware frameworks FSP may run long before End-of-Post, this + means most of the framework runs with `POSTBOOT_SAI`. + +* In the EDS, implications of the SAI switch are _not_ documented. + Virtually for every register, it is unknown if it is still accessible + after the SAI switch. This seems to be a problem for developers both + inside and outside of Intel. + +* The SAI switch seems to force the firmware framework to do some things + in SMM. However, the use of SMM as a more privileged execution mode is + generally discouraged. This means FSP forces firmware development to + go backwards. + +Especially the lack of documentation leads to many bugs in firmware. +Even Intel developers working on coreboot don't seem to know the +implications of the SAI switch or lack the resources to re-evaluate +all code wrt. the SAI switch. + + +FSP Creates Legal Uncertainty +----------------------------- + +Many companies sell coreboot together with FSP as part of their products. +Especially, the function pointers to GPL code that were introduced with +FSP 2.1 seem unprecedented. However, they could be compared with runtime +linking which seems troubling. + + +Mitigations +----------- + +None of the listed issues would exist if FSP were open-source and the +initialization steps could be integrated into coreboot, like they were +before. An open-source approach would not only cut costs heavily but +might also decrease time-to-market if the whole coreboot community +could work together on new platforms. + +However, in the absence of such a firmware-development utopia, there +may be more realistic approaches. As mentioned earlier, not all of FSP +has a negative impact on development performance. FSP-M, for instance, +rarely causes trouble. And due to its complex nature, it is no surprise +if memory-controller initialization is kept secret. The configuration +done by FSP-S, on the other hand, overlaps with many of coreboot's +responsibilities. The few lines of code in FSP-S that help to get a +mainboard running, don't outweigh the damage it does. However, one +can't have the cherries without taking the whole menu, which takes +years to digest. + +One approach could be to split FSP-S further up and identify parts +that are necessarily closed source and parts that could as well be +reimplemented in coreboot. However, if parts that are left closed +source change the state of the execution environment (e.g. switch +SAI), finalize any settings or raise legal concerns, the ABI would +have to change to solve all aforementioned issues. + +Another approach could be to align older platforms with open-source +coreboot, step by step. Currently, everything from Skylake on relies +on FSP-S. However, it seems Intel has already abandoned development +for this platform (while the coreboot integration was never finished). +It would seem reasonable to release abandoned code to the public, or +at least clear existing documentation for older platforms from NDAs.
Nico Huber has removed David Wu from this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Removed reviewer David Wu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG@15 PS1, Line 15: Feel free to amend this commit in any way :) I'm very tempted to talk badly about FSP-T integration since it became optional since 2.0, but I suppose it's not a major issue since there is an open source alternative :)
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 102: POSTBOOT_SAI Please explain briefly what this does/tries to do.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 128: function pointers Could you give some examples (mainly console + AP PPI, I guess) here?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Patch Set 1: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 165: at least clear existing documentation for older platforms from NDAs. I'd add "missing communication & responsibility"
* Currently Intel doesn't communicate with the coreboot project members (maybe behind closed doors). It doesn't follow discussions on IRC, GitHub, mailing lists or any other public communication mediums.
* Intel doesn't maintain their FSP interface code after dropping it into coreboot.
* Code added for "development purposes only" isn't removed once made public on Github.
* Intel barely reviews maintenance work done on their code contributed to coreboot.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Patch Set 1:
(7 comments)
I spilled some beans of mine, because I felt like it. I am probably mistaken, so don't resist the urge to correct my statements :D
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG@15 PS1, Line 15: Feel free to amend this commit in any way :)
I'm very tempted to talk badly about FSP-T integration since it became optional since 2. […]
I guess a "fsp-t_discussion.md" can also exist! :D
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 42: I wrote this:
The coreboot community uses these pre-built FSP binaries. Several FSP bugs have been found and reported, but they hardly ever get addressed. This means that the community has to awkwardly work around these issues and hope that the bug is fixed in a new FSP release, which may never happen if the platform is too old.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 56: On at least Skylake, configuring the bus/device/function for the HPET or the IOAPIC (related to VT-d) is a single write to a single register on the PCI config space of the P2SB. However, FSP-M has *four* configuration parameters for the HPET, and FSP-S has another four parameters for the IOAPIC. Just why.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 102: POSTBOOT_SAI
Please explain briefly what this does/tries to do.
I can talk about it.
Newer Intel chips have an interconnect system called IOSF (Intel On-chip System Interconnect). This is a rather complex architecture. I think it has three "levels" of interconnects: primary (where the gross volume of traffic goes through), secondary (related to the management engine and the P2SB), and tertiary (related to hardware debug capabilities).
The primary level is the most public one. It's what appears on block diagrams, connecting pretty much all the devices. This level seems to belong to the x86 cores.
The secondary/sideband level is wired to the Management Engine as well as other devices, and the P2SB is a gate from primary to sideband. This seems to be the Management Engine's own communication channel, for hell knows what purposes.
AFAIK, Security Attributes of Initiator are some bits that specify the privilege level of a certain "initiator". The one we care about is the host (x86 cores). the default setting is rather privileged, and is needed to do firmware things (init devices). Once this is done, the SAI privilege level should be dropped as a protective measure. This would be the post-boot setting, known as POSTBOOT_SAI.
At least on CoffeeLake, FSP-S sets that when it has finished running. This restricts later code's access to many devices (e.g., the damned P2SB) and forces the use of SMM because, of course, SMM is a special snowflake and is trusted with a higher privilege than non-SMM.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 108: In the EDS, implications of the SAI switch are _not_ documented. maybe one needs to go deeper when recursively NDA-wrapping oneself? \s
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 128: function pointers
Could you give some examples (mainly console + AP PPI, I guess) here?
mrc.bin also has a function pointer to do console output. I don't really mind a single console callback, but the PPI thing is much more complex...
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 165: at least clear existing documentation for older platforms from NDAs.
I'd add "missing communication & responsibility" […]
Last bullet point: sometimes there's friction when maintenance is done :S
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC, scratch] Documentation/fsp: Start discussion around FSP-S issues ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 43: Fixing bugs by binary patching or live patching is prohibited by the license
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 165: at least clear existing documentation for older platforms from NDAs.
Last bullet point: sometimes there's friction when maintenance is done :S
Github: Intel ignores new issues for months and then closes existing issues without a) willing to find a solution b) even confirming issues exist
Hello Patrick Rudolph, Aaron Durbin, Arthur Heymans, Michael Niewöhner, Christian Walter, David Hendricks, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36328
to look at the new patch set (#2).
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
[RFC] Documentation/fsp: Discuss FSP-S issues
To get some discussion started, list some heavy issues with FSP(-S). I've copy pasted some things together, and added a few words on top and bottom. Not sure yet, where this should lead. To convince Intel to contribute open-source code on their own, we might want to add a list of initialization steps, that should be open. But first we should ask if they can do anything for us!
Feel free to share this with your Intel representative :)
Change-Id: Ied0d8ad60fa487272a05fc73b16a7c343cc4d993 Signed-off-by: Nico Huber nico.huber@secunet.com --- A Documentation/fsp/fsp-s_discussion.docx A Documentation/fsp/fsp-s_discussion.md 2 files changed, 234 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/36328/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2:
(7 comments)
Integrated most of the comments and added some more... I think it's a little long now to be read by responsible folks :-/ anyway, I've pandoc'ed a .docx for convenient sharing with management.
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG@15 PS1, Line 15: Feel free to amend this commit in any way :)
I guess a "fsp-t_discussion. […]
Ack. Please rant about FSP-T in a separate document. While it's easy to avoid FSP-T in a product, all the ugly friction within the community could have been avoided by documenting it, I guess. So there is definitely more that Intel could do better (beside calling off their dogs).
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 42:
I wrote this: […]
Integrated somehow.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 43:
Fixing bugs by binary patching or live patching is prohibited by the license
Ack
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 56:
On at least Skylake, configuring the bus/device/function for the HPET or the IOAPIC (related to VT-d […]
Added a few lines about unnecessary complex options.
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 102: POSTBOOT_SAI
I can talk about it. […]
Done
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 128: function pointers I consider the console as an optional feature. And yeah, if somebody has legal concerns, they can just stub it out.
Could you give some examples (mainly console + AP PPI, I guess) here?
Done
https://review.coreboot.org/c/coreboot/+/36328/1/Documentation/fsp/fsp-s_dis... PS1, Line 165: at least clear existing documentation for older platforms from NDAs.
Github: Intel ignores new issues for months and then closes existing issues without a) willing to fi […]
I've integrated some of this under Patrick's heading `Missing Communication and Responsibility`. Not all of it, though, I want to keep it about FSP and a general complaint about some individual's behaviour.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2:
Thanks for the comments btw. :)
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
Good content overall, but I strongly advise against legal speculation.
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty Speculating about legal issues is a surefire way to prevent anybody from Intel from reading or commenting on this. I recommend removing the section, or updating the commit message to avoid misleading people about discussions or convincing Intel with this since that simply won't happen.
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 204: The few lines of code in FSP-S that help to get a : mainboard running, don't outweigh the damage it does. However, one : can't have the cherries without taking the whole menu, which takes : years to digest. Too much editorializing IMO
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
Speculating about legal issues is a surefire way to prevent anybody from Intel from reading or comme […]
Where do you see any speculation?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
Where do you see any speculation?
David, I don't quite follow. How is "uncertainty" speculation. Are we certain about anything?
Can you help me find the right words to express that we aren't sure one can ship coreboot+FSP without applying the GPL's copyleft to FSP (and without violating coreboot author's copyright ofc). Or in other words, how to tell Intel that we are afraid to sell their HW/SW.
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 204: The few lines of code in FSP-S that help to get a : mainboard running, don't outweigh the damage it does. However, one : can't have the cherries without taking the whole menu, which takes : years to digest.
Too much editorializing IMO
Yeah, let's scratch that.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 2: Code-Review-2
(1 comment)
Setting as -2 until the legal section is either removed or vetted by actual lawyers; If we're gonna pour this much gas on the fire we should have a very solid reason for doing so.
I don't have a problem with the rest of the doc - If you want to get everything else in sooner then I recommend splitting this into two patches with the legal section in its own patch.
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
Where do you see any speculation?
The heading and the last two sentences, though I suppose some might see them as more of an accusation than mere speculation.
In any case this section is counter-productive. Stick to technical aspects if the intention is to have useful dialogue with engineers about how to improve the situation. Introducing legal speculation/accusations will make it difficult or impossible.
I'm not sure how companies in Germany operate, but in the US discussing such legal matters is usually forbidden and must go thru legal council. I think even the non-legal parts of this document will be difficult to discuss if this section remains. (If somebody accuses my employer of wrongdoing my obligation is to avoid the discussion entirely and let communications and legal teams deal with it)
Can you help me find the right words to express that we aren't sure one can ship coreboot+FSP without applying the GPL's copyleft to FSP (and without violating coreboot author's copyright ofc). Or in other words, how to tell Intel that we are afraid to sell their HW/SW.
IANAL so I can't give you the right words. In the short term you should contact Secunet's legal department to open a discussion with Intel's legal department to address your concerns for selling HW/SW. If Secunet's legal team sees a problem please bring it to coreboot leadership so we can work towards a resolution.
Hello Patrick Rudolph, Aaron Durbin, Arthur Heymans, Michael Niewöhner, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Werner Zeh, David Hendricks, Christian Walter, Philipp Deppenwiese, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36328
to look at the new patch set (#3).
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
[RFC] Documentation/fsp: Discuss FSP-S issues
To get some discussion started, list some heavy issues with FSP(-S). I've copy pasted some things together, and added a few words on top and bottom. Not sure yet, where this should lead. To convince Intel to contribute open-source code on their own, we might want to add a list of initialization steps, that should be open. But first we should ask if they can do anything for us!
Feel free to share this with your Intel representative :)
Change-Id: Ied0d8ad60fa487272a05fc73b16a7c343cc4d993 Signed-off-by: Nico Huber nico.huber@secunet.com --- A Documentation/fsp/fsp-s_discussion.docx A Documentation/fsp/fsp-s_discussion.md 2 files changed, 218 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/36328/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 3:
(1 comment)
Patch Set 2: Code-Review-2
(1 comment)
Setting as -2 until the legal section is either removed or vetted by actual lawyers; If we're gonna pour this much gas on the fire we should have a very solid reason for doing so.
Ok, I've dropped it. Not that I would agree with you by a single percent. Just because it gives me a much too high blood pressure to discuss it with hotheads. There was no accusation whatsoever, not even a statement (beside the heading which was already true if a single person in the world has doubts). That the mere finding that somebody could be uncertain makes you uncomfortable makes me wonder what world you live in. If things are that bad in the US, sorry, you have my condolence.
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
Where do you see any speculation? […]
I am not a lawyer either, but kind of a language lawyer. I took great care not to speculate and not to provide any statement (beside the heading) in this section. So please help me with my English here:
* If a single person is uncertain because of FSP, then FSP creates uncertainty, no? * `(to) seem` is subjective. How can the sentence not be true? Isn't it enough if we (or I) don't know any preceding instant? * The last sentence provides a comparison and is written subjunctively. How the hell can it accuse of anything?
My intention was not to discuss any legal matters anyway. All I wanted was to create awareness of these concerns.
Hello Patrick Rudolph, Aaron Durbin, Arthur Heymans, Michael Niewöhner, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Werner Zeh, David Hendricks, Christian Walter, Philipp Deppenwiese, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36328
to look at the new patch set (#4).
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
[RFC] Documentation/fsp: Discuss FSP-S issues
To get some discussion started, list some heavy issues with FSP(-S). I've copy pasted some things together, and added a few words on top and bottom. Not sure yet, where this should lead. To convince Intel to contribute open-source code on their own, we might want to add a list of initialization steps, that should be open. But first we should ask if they can do anything for us!
Feel free to share this with your Intel representative :)
Change-Id: Ied0d8ad60fa487272a05fc73b16a7c343cc4d993 Signed-off-by: Nico Huber nico.huber@secunet.com --- A Documentation/fsp/fsp-s_discussion.docx A Documentation/fsp/fsp-s_discussion.md 2 files changed, 215 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/36328/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
I am not a lawyer either, but kind of a language lawyer. I took […]
Done
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 204: The few lines of code in FSP-S that help to get a : mainboard running, don't outweigh the damage it does. However, one : can't have the cherries without taking the whole menu, which takes : years to digest.
Yeah, let's scratch that.
Done
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Thanks for making the change. Also, sorry for any grief that I caused - We're on the same side, after all.
Ok, I've dropped it. Not that I would agree with you by a single percent. Just because it gives me a much too high blood pressure to discuss it with hotheads. There was no accusation whatsoever, not even a statement (beside the heading which was already true if a single person in the world has doubts). That the mere finding that somebody could be uncertain makes you uncomfortable makes me wonder what world you live in. If things are that bad in the US, sorry, you have my condolence.
I doubt it's just a US issue. Consider this: Have your complaints about FSP on the mailing list or Gerrit ever led to a positive outcome? Is there a German word for attempting the same thing many times over and expecting a different result?
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/2/Documentation/fsp/fsp-s_dis... PS2, Line 173: FSP Creates Legal Uncertainty
Done
Regardless of the semantics you present, the ambiguity is what scares lawyers and we must assume a defensive posture (on their side) by default. Unfortunately this overrides any engineering discussions/decisions.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
Patch Set 4: Code-Review+1
(1 comment)
Thanks for making the change. Also, sorry for any grief that I caused - We're on the same side, after all.
Ok, I've dropped it. Not that I would agree with you by a single percent. Just because it gives me a much too high blood pressure to discuss it with hotheads. There was no accusation whatsoever, not even a statement (beside the heading which was already true if a single person in the world has doubts). That the mere finding that somebody could be uncertain makes you uncomfortable makes me wonder what world you live in. If things are that bad in the US, sorry, you have my condolence.
I doubt it's just a US issue. Consider this: Have your complaints about FSP on the mailing list or Gerrit ever led to a positive outcome? Is there a German word for attempting the same thing many times over and expecting a different result?
Let me translate this... you say one should stop discussing a topic just because the opponent does not want to discuss it? This is what I'd call "giving up".
I *really* don't see where dropping that part should help in any way and I don't see why one wouldn't discuss the other topics in the document because there is *some* "legal speculation".
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: Code-Review+1
(1 comment)
Thanks for making the change. Also, sorry for any grief that I caused - We're on the same side, after all.
Ok, I've dropped it. Not that I would agree with you by a single percent. Just because it gives me a much too high blood pressure to discuss it with hotheads. There was no accusation whatsoever, not even a statement (beside the heading which was already true if a single person in the world has doubts). That the mere finding that somebody could be uncertain makes you uncomfortable makes me wonder what world you live in. If things are that bad in the US, sorry, you have my condolence.
I doubt it's just a US issue. Consider this: Have your complaints about FSP on the mailing list or Gerrit ever led to a positive outcome? Is there a German word for attempting the same thing many times over and expecting a different result?
Let me translate this... you say one should stop discussing a topic just because the opponent does not want to discuss it? This is what I'd call "giving up".
I *really* don't see where dropping that part should help in any way and I don't see why one wouldn't discuss the other topics in the document because there is *some* "legal speculation".
btw. I found at least five more paragraphs that would need removal too under your statements ;)
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+1
(5 comments)
Thank you for taking the time to write this up.
1. Please document how you created the DOCX file (I guess Pandoc) in the commit message. 2. Maybe the file could even be automatically when building the documentation.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 99: This shows that no matter the many options that are implemented in : FSP-S … no matter *how* the many options are implemented …
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 169: Sometimes they are closed without any resolution. Please give an example.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 182: in the absence of such a firmware-development utopia, Maybe remove to not take away the result before the actual discussion inside Intel? Maybe find words, that there a lot of parts, where free software is a reality, like the Linux kernel.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 185: And due to its complex nature, it is no surprise : if memory-controller initialization is kept secret. Why?
Maybe: … it is no surprise, that it is shared between frameworks and therefore developed and distributed separately.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns Why? Elaborate?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
Does the document need to be referenced from some other document to be found?
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(2 comments)
Let me translate this... you say one should stop discussing a topic just because the opponent does not want to discuss it?
No, I am saying that it's practically impossible for somebody from Intel to address legal concerns in a public forum. It has to do with typical corporate processes for handling these types of things, not whether or not they want to.
OTOH there are reputable companies in the community such as Google, Siemens, and others who have presumably done their due diligence and are obviously fine with shipping hardware with coreboot and FSP. The concerns mentioned in the earlier versions of this patch do not reflect those of the community as a whole, which is another reason they should not be in Documentation/.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 56: several FSP bugs, but they hardly ever got addressed. This means I've also been told that validating a new FSP binary is a very time-consuming and expensive process for Intel. Unfortunately this means that there is very little chance of getting a new FSP binary published to fix bugs or implement features that are deemed non-critical, especially on older products (even popular ones such as Baytrail and Broadwell-DE).
So while FSP has been useful in getting products up and running initially, the ongoing support model has not worked out well. It's difficult for Intel to implement a fix FSP, validate it, and release a new binary and it's also impossible for community members to fix FSP on their own.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Why? Elaborate?
let's not open that can of worms...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(4 comments)
Let me translate this... you say one should stop discussing a topic just because the opponent does not want to discuss it?
No, I am saying that it's practically impossible for somebody from Intel to address legal concerns in a public forum. It has to do with typical corporate processes for handling these types of things, not whether or not they want to.
OTOH there are reputable companies in the community such as Google, Siemens, and others who have presumably done their due diligence and are obviously fine with shipping hardware with coreboot and FSP. The concerns mentioned in the earlier versions of this patch do not reflect those of the community as a whole, which is another reason they should not be in Documentation/.
AFAIK, it's only cros devices that ship the more entangled FSP2.1. And it's not even Google selling them... I don't think every individual company tracks changes in the ABI, so there is potentially a lack of awareness on that end too.
The topic is delicate indeed. However, I didn't bring it up here because I want any (legal) questions answered. Rather, if we don't want to trial ABIs against legal departments, the responsible engineers need to be aware that licenses like the GPL may be involved and take that into account. Maybe I'll add something in a follow up... The message it should carry is that
an ABI design that may raise legal concerns increases (FSP) costs further.
All the discussions about it we had on Gerrit are some $k alone. ;)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 56: several FSP bugs, but they hardly ever got addressed. This means
I've also been told that validating a new FSP binary is a very time-consuming and expensive process […]
I think it's not that bad anymore. If one looks at their GitHub activity, there are a lot of updates (nowadays). But I guess you need to be a bigger customer or IBV (with direct contacts) to get issues addressed.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 99: This shows that no matter the many options that are implemented in : FSP-S
… no matter *how* the many options are implemented …
That wouldn't be what I meant... rather "no matter how many options are offered"...
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 182: in the absence of such a firmware-development utopia,
Maybe remove to not take away the result before the actual discussion inside Intel? Maybe find words […]
The idea was to make it clear that we don't expect them to go fully open-source. Of course, I would hope for it. But if you ask them for that you can be sure they'll ignore you.
First step is to make them open up what they don't care about... which, if you take FSP as a whole, is currently mixed with things they care about. If they make that step, we can still ask for more. Or prove to them, again, that open-source raminit is no doomsday device.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 185: And due to its complex nature, it is no surprise : if memory-controller initialization is kept secret.
Why?
(foreign) IP, for instance
Ofc, it's bullshit when somebody tells you that a single specific bit can't be set in open-source code because of IP. But in case of memory- controllers, there is really a lot involved that the silicon vendors don't want to share or are not allowed to (don't own).
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 99: This shows that no matter the many options that are implemented in : FSP-S
That wouldn't be what I meant... rather "no matter how many options […]
Ah, understood. I find it difficult to pass, but the native speakers should comment if a change is warranted or not.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 182: in the absence of such a firmware-development utopia,
The idea was to make it clear that we don't expect them to go fully […]
Reading the whole parapgraph, I still think it can be removed. But up to you.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 185: And due to its complex nature, it is no surprise : if memory-controller initialization is kept secret. Understood.
And due to its complex nature with, for example, foreign IP it is harder to open-source the memory-controller initialization.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
let's not open that can of worms...
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 169: Sometimes they are closed without any resolution.
Please give an example.
Oh, that's an easy one... "most issues" \o/
ignored for ~ 1 month or more: https://github.com/IntelFsp/FSP/issues/6 https://github.com/IntelFsp/FSP/issues/13 https://github.com/IntelFsp/FSP/issues/18 https://github.com/IntelFsp/FSP/issues/22 https://github.com/IntelFsp/FSP/issues/23 https://github.com/IntelFsp/FSP/issues/24 https://github.com/IntelFsp/FSP/issues/25 https://github.com/IntelFsp/FSP/issues/26 https://github.com/IntelFsp/FSP/issues/27 https://github.com/IntelFsp/FSP/issues/30 https://github.com/IntelFsp/FSP/issues/33 https://github.com/IntelFsp/FSP/issues/34 https://github.com/IntelFsp/FSP/issues/37 https://github.com/IntelFsp/FSP/issues/39 https://github.com/IntelFsp/FSP/issues/40 https://github.com/IntelFsp/FSP/issues/42
closed without solution: https://github.com/IntelFsp/FSP/issues/30 https://github.com/IntelFsp/FSP/issues/37 https://github.com/IntelFsp/FSP/issues/35
WONTFIX: https://github.com/IntelFsp/FSP/issues/41
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+2
The topic is delicate indeed. However, I didn't bring it up here because I want any (legal) questions answered. Rather, if we don't want to trial ABIs against legal departments, the responsible engineers need to be aware that licenses like the GPL may be involved and take that into account. Maybe I'll add something in a follow up... The message it should carry is that
an ABI design that may raise legal concerns increases (FSP) costs further.
As you mention it's a delicate topic, but it's worth discussing. We just need to find the right way to communicate it so we can get the desired result. How about we bring it up at the next leadership meeting and see if we can get a working group together?
All the discussions about it we had on Gerrit are some $k alone. ;)
Indeed!
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG@15 PS1, Line 15: Feel free to amend this commit in any way :)
Ack. Please rant about FSP-T in a separate document. While it's […]
Please moderate your language, referring to anyone as "dogs" is way out of line.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36328/1//COMMIT_MSG@15 PS1, Line 15: Feel free to amend this commit in any way :)
Please moderate your language, referring to anyone as "dogs" is way out of line.
Yep, sorry. This was a very unfortunate and too literal translation of a German phrase.
What I meant was calling off people that mostly act on somebody else's behalf and usually say `No!` when you try to fix the problems they introduced (somebody else being folks that are invisible and anonymous to the coreboot community).
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision. The direction from Intel's security team is that there have been many examples of OEM firmware that forgets to set lock bits, so that have made it mandatory that we do so in the FSP. I'm open to having a dialog for when this is done, but keep in mind that people above me require it be done in the FSP binary.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ---------------- My comment above applies for this entire section.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility To be crystal clear, FSP binaries are provided by Intel as-is. They do not come with a gratis Service Agreement. As far as "responsibility" goes, we make a good faith effort to provide community support but please realize it will be lower priority.
If you want professional level service, then it is possible to purchase support contracts with Intel.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before. To be open and direct with all of you, this is extremely unlikely.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Done
Please take this out, otherwise I run the risk of getting in trouble for talking to you at all.
This is feeling like a broken record, but I would like to point out...
1. The MP_SERVICES pointer is completely optional. You can set it to NULL and FSP will use its built-in MP implementation, which negates this whole argument. I am aware of several platforms that use coreboot with this set to NULL. It is the user's choice to configure coreboot in this way, not Intel's.
2. The MP_SERVICES pointer is NOT part of the FSP 2.1 specification, it is an implementation level UPD that is included in the FSP for some platforms but not all.
3. coreboot is not the only bootloader that uses FSP binaries, there are plenty of other solutions with different licensing paradigms.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(5 comments)
Hi Nate, thanks for joining the discussion.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision.
The direction from Intel's security team is that there have been many examples of OEM firmware that […]
Well, about the "when": How about last? Also, having it documented at least when what lock bit is set would be a step forward.
It's obvious that we got here because of somebody's agenda. But can you please get in touch with those people above you. Tell them that you complied and while it made things better for the OEMs that forgot to lock things, it made it worse for some that actually care about security.
A good example is the locking of Flash Protected Regions. Locking 0 values doesn't seem like a good solution. Now, firmware that tries to use such protection also has to test if the writes are effective (because it's not documented when FSP locks it), and lock it anyway (because it's not documented if FSP might overwrite the values). On top, tools like chipsec give you a green light for this, even if a borked configuration is locked.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
My comment above applies for this entire section.
Though, wouldn't you agree that the first FSP-S stage is a little to early for something called POSTBOOT?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
To be crystal clear, FSP binaries are provided by Intel as-is. […]
Many of us don't want anything but writing our own code. It's Intel that wants us to use FSP instead, making everything ten times more expensive currently... However, I'd agree to offer Intel professional service to help fixing FSP, if they'd want to purchase a contract.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
To be open and direct with all of you, this is extremely unlikely.
Sorry, what do you refer to? That we could have open-source silicon init again? or the whole sentence (relation to the mentioned issues)?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Please take this out, otherwise I run the risk of getting in trouble for talking to you at all.
Please let's not mix this up with the removed paragraph that was talking about FSP2.1. All that this sentence says is that "if something would raise legal concerns, it should change". Not targeting anything in particular. I can drop it though, if the word "legal" alone is problematic.
This is feeling like a broken record, but I would like to point out...
- The MP_SERVICES pointer is completely optional. You can set it to NULL and FSP will use its built-in MP implementation, which negates this whole argument. I am aware of several platforms that use coreboot with this set to NULL. It is the user's choice to configure coreboot in this way, not Intel's.
Thanks for pointing this out. As mentioned somewhere above, there is a lack of documentation. It's a shame that information like this needs to be "leaked".
- The MP_SERVICES pointer is NOT part of the FSP 2.1 specification, it is an implementation level UPD that is included in the FSP for some platforms but not all.
Correct. I was told otherwise by Intel employees (that it would be part of 2.1 before that was written), but I checked and you are right.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision.
Well, about the "when": How about last? Also, having it documented at least […]
@Nathaniel Well, why not default to locking an leave the possibility to disable it?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
Sorry, what do you refer to? That we could have open-source silicon init […]
... if you mean "it is very unlikely FSP will become open-source"... wait, what? What happend to Raja Koduri's (Intel) work on open-sourcing FSP?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
If you want professional level service, then it is possible to purchase support contracts with Intel.
Many people don't want professional level service but simply run their hardware they bought without any proprietary blobs. It feels like Intel tries to sell a license for using a CPU/SoC instead of selling the hardware itself...
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision.
@Nathaniel Well, why not default to locking an leave the possibility to disable it?
Having a UPD to disable the SAI lock is not something that I will be able to get approved.
As for the "when" part, from a security standpoint the latest it can be done is EnumInitPhaseAfterPciEnumeration. After that, on a UEFI firmware you will start executing 3rd party OpROMs. It would be a security issue if an OpROM had unrestricted write access to SPI.
There is no doubt in my mind that the documentation around this could be better and I will endeavor to help improve that on my side.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
Though, wouldn't you agree that the first FSP-S stage is a little to […]
Keep in mind the wording on this stuff was written by silicon architects. In their minds, anything that happens after MRC is done is "post boot."
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
Many people don't want professional level service but simply run their hardware they bought without any proprietary blobs.
I am not aware of a single modern performance competitive chipset/SoC that does not require software/firmware provided by the silicon vendor in order to function. Once upon a time these blobs were written in ROM fuses, but that has severe consequences. The infamous Pentium FDIV bug could have been resolved by a microcode update, but the original Pentium did not have updatable microcode. Having in-field re-programmability is essential for the modern world. Spectre/Meltdown was able to be mitigated with in-field updates instead of a recall because of Intel's focus on making as much of the SoC behavior updatable through firmware as possible.
All of Intel's competitors use blobs. Even the Raspberry Pi uses blobs. When you buy silicon regardless of vendor, you are buying a bundle that includes hardware, firmware, and software.
It feels like Intel tries to sell a license for using a CPU/SoC instead of selling the hardware itself...
We have zero interest in license revenue for our firmware. The cost of developing the firmware for the SoC is included in the price of the SoC itself.
However, we do not have any obligation to provide some sort of FSP help desk. I try to help as I am able, but keep in mind that I have a regular day job as well working on getting the firmware for our upcoming chips ready to ship. It is unreasonable to expect me to be able to answer every question in my very limited spare time, and management is unwilling to formally assign people to something that won't directly impact chip sales.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Please let's not mix this up with the removed paragraph that was talking about FSP2.1. All that this sentence says is that "if something would raise legal concerns, it should change". Not targeting anything in particular.
If you focus on just the "legal" part, it would be reasonable to read this sentence as follows:
"However, if parts that are left closed source raise legal concerns, the ABI would have to change to solve aforementioned issues."
The reference to the ABI causing a potential legal concern brings to mind the MP_SERVICES example, that is the only one I can think of that would meet the parameters of this sentence.
Regardless, I would like to respectfully request that we remove it out of an abundance of caution. If the lawyers get involved, I think progress here will become impossible.
Thanks for pointing this out. As mentioned somewhere above, there is a lack of documentation. It's a shame that information like this needs to be "leaked".
Apologize this was not clear. I thought it was mentioned in the integration guide but it appears to not be. Can you file a issue on IntelFsp to remind me to go fix that?
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
However, we do not have any obligation to provide some sort of FSP help desk. I try to help as I am able, but keep in mind that I have a regular day job as well working on getting the firmware for our upcoming chips ready to ship. It is unreasonable to expect me to be able to answer every question in my very limited spare time, and management is unwilling to formally assign people to something that won't directly impact chip sales.
So on a certain level we agree: Intel does not have the resources to provide a full featured support for the FSP. The people inside Intel taking care of FSP have a lot of different tasks to do so their time is limited. On the other hand there is this already huge grown coreboot community which would love to write code in high quality for every new platform that arrives. So the only thing that hinders the community to help Intel out is the lack of information and the mindset at Intel to hide more and more of the internals of the chipsets. I never got the real reason for this hiding mentality, what is it? - Is it that a competitor may have a more detailed view to the internals? I guess this audience (other CPU vendors) would have much more powerful ways of getting the implementation details and would not rely on open source firmware alone. And, speaking of the FSP-S part, I doubt that there are _the_ most important assets in there (it might be different for FSP-M) - Is it hiding a possibly awkward way of a given implementation because of limited time or resource at design time? This would be nothing new for the community and nobody would really blame Intel for that if the access to the needed information would be open. In contrast, if this will be open, the community would have a much better way to deal with it and the most of us will be happy.
So finally I am sure that opening up more stuff to the community would help both sides and make all of us happy again.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
It feels like Intel tries to sell a license for using a CPU/SoC instead of selling the hardware itself...
We have zero interest in license revenue for our firmware. The cost of developing the firmware for the SoC is included in the price of the SoC itself.
That's why I said "it feels like". I just wanted to say that it feels like the hardware I bought isn't mine, really.
I am not aware of a single modern performance competitive chipset/SoC that does not require software/firmware provided by the silicon vendor in order to function.
Just like Werner I wonder why this is needed at all? Just make it completely open-source. This would be a perfect sales argument for Intel! All open! No secrets! ;-)
However, we do not have any obligation to provide some sort of FSP help desk.
This is exactly the problem. Intel forces us to use undocumented blobs but doesn't provide (or too little) help. Make it open, then we can write the code and Intel does not need to provide FSP help - win, win.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 99: This shows that no matter the many options that are implemented in : FSP-S
Ah, understood. […]
I would rephrase this a bit:
This shows that, no matter how many options are implemented in FSP-S, there will always be...
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Thanks for pointing this out. As mentioned somewhere above, there is a lack of documentation. It's a shame that information like this needs to be "leaked".
Apologize this was not clear. I thought it was mentioned in the integration guide but it appears to not be. Can you file a issue on IntelFsp to remind me to go fix that?
Done: https://github.com/IntelFsp/FSP/issues/47
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
Thanks for pointing this out. As mentioned somewhere above, there is a lack of documentation. […]
I was originally thinking the words here were too vague to really matter... However I tend to agree with Nate that we should err on the side of caution and avoid any potentially confusing references to legal matters. Nico might know exactly what he's referring to, but another observer (a manager, lawyer, etc) who does not immediately understand the nuances can quickly shut down discussion.
These few words are simply not worth the hassle, and there is practically nothing to be gained by having them here IMO. We can find other ways to discuss underlying concerns, just not here.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review-2
I agree with David. Blocked until we remove the few words or rewrite the sentences.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: Code-Review-2
Just to make it clear that I'm going to do what I already said in the comments: remove the offending words. Apparently, it drives some people nuts if they don't see a -2, or they can't read or whatever.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: -Code-Review
Hello Patrick Rudolph, Aaron Durbin, Arthur Heymans, Michael Niewöhner, Duncan Laurie, Paul Menzel, Stefan Reinauer, build bot (Jenkins), Furquan Shaikh, Werner Zeh, David Hendricks, Christian Walter, Philipp Deppenwiese, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36328
to look at the new patch set (#5).
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
[RFC] Documentation/fsp: Discuss FSP-S issues
To get some discussion started, list some heavy issues with FSP(-S). I've copy pasted some things together, and added a few words on top and bottom. Not sure yet, where this should lead. To convince Intel to contribute open-source code on their own, we might want to add a list of initialization steps, that should be open. But first we should ask if they can do anything for us!
The .docx version was generated with
pandoc -t docx -o fsp-s_discussion.docx fsp-s_discussion.md
Feel free to share this with your Intel representative :)
Change-Id: Ied0d8ad60fa487272a05fc73b16a7c343cc4d993 Signed-off-by: Nico Huber nico.huber@secunet.com --- A Documentation/fsp/fsp-s_discussion.docx A Documentation/fsp/fsp-s_discussion.md 2 files changed, 215 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/36328/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 4: -Code-Review
(7 comments)
- Please document how you created the DOCX file (I guess Pandoc) in the commit message.
Done. Still not sure if it's useful at all. I just got used to run things through pandoc when they are for a broader audience.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 89: * One either has to assume that FSP doesn't perform any locking : and do it redundantly in the firmware framework, or expensively : test what is locked and keep doing so for every new FSP revision.
Having a UPD to disable the SAI lock is not something that I will be able to get approved.
Then, somebody will have to document when the switch is done and what needs to be configured before that. And to make sure that nobody runs into any pitfalls, it would be wise to add to every register in the EDS' which SAI is necessary to access it. Please note that also Intel employees sometimes lose track of this.
As for the "when" part, from a security standpoint the latest it can be done is EnumInitPhaseAfterPciEnumeration. After that, on a UEFI firmware you will start executing 3rd party OpROMs. It would be a security issue if an OpROM had unrestricted write access to SPI.
We could gain more flexibility here if locking would be a separate notification phase.
There is no doubt in my mind that the documentation around this could be better and I will endeavor to help improve that on my side.
Thanks.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 99: This shows that no matter the many options that are implemented in : FSP-S
I would rephrase this a bit: […]
Done
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
Keep in mind the wording on this stuff was written by silicon architects. […]
Interesting. While the BIOS Spec doesn't mention any specific point when it should be done, it mentions how to act if it's done after PCI enumeration. That's not part of MRC yet, is it?
Also, I don't understand what the fuss is about. It seems most registers are still accessible with SMM_SAI. And let's be honest, if one expects the average IAFW to have SMM secured properly (no exploitable handlers), they probably also believe in fairy tales.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 155: Missing Communication and Responsibility
It feels like Intel tries to sell a license for using a CPU/SoC instead of selling the hardware […]
Let's not forget the original message. There is no need to invest into additional communication channels unless Intel creates it (currently they do).
Of course, all competitive chips contain blobs at some point. But that's no excuse to put everything into blobs. What we want is to find a better balance between what is open and what is in a blob, to increase firmware quality. The current tradeoff with FSP-S is unbearable. And, personally, I'm convinced that even Intel employees suffer because of some of the issues with FSP.
And, let's also not forget, that this is not only about the coreboot community asking Intel for something, but also the other way around. In the past (FSP) years, Intel has many times asked the coreboot community to maintain their FSP integration (explicitly for Intel customers that are not part of the community). So if Intel doesn't want to help the community, I guess the community shouldn't maintain their integration at all costs.
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 169: Sometimes they are closed without any resolution.
Oh, that's an easy one... "most issues" \o/ […]
Done
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 185: And due to its complex nature, it is no surprise : if memory-controller initialization is kept secret.
Understood. […]
It's an example... but I wouldn't want to write it down here because one could think we speculate too much about Intel's reasons. I could just take out the whole sentence if that makes things better?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 194: or raise legal concerns
I was originally thinking the words here were too vague to really matter... […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 185: And due to its complex nature, it is no surprise : if memory-controller initialization is kept secret.
It's an example... but I wouldn't want to write it down here because […]
I'll try rephrasing the paragraph:
However, in the absence of such a firmware-development utopia, there may be more realistic approaches. As mentioned earlier, not all of FSP has a negative impact on development performance. For instance, FSP-M rarely causes trouble. Also, given the complex nature of high-speed interfaces, it is no surprise that information about memory controller initialization is kept secret. On the other hand, the configuration done by FSP-S overlaps or even conflicts with many of coreboot's duties.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
... if you mean "it is very unlikely FSP will become open-source"... […]
@Nate any comment on this?
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5: Code-Review+2
Thanks for bearing with us and making the changes, Nico. If you wanted attention from somebody at Intel it seems to have worked ;-) Now let's keep the discussion constructive...
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
Since this primarily seems Intel-centric, I'll offer an Ack instead of a code-review score. Nice writeup though.
I don't want to risk sounding like I'm taking shots at a competitor. Having been an FSP consumer for years, I'm very sensitive to the challenges the coreboot community and customers face. Ironically, now back at AMD and trying to package reference code again, I have some newfound sympathy for Intel engineers that needs to support their feature sets.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
Can the [RFC] be removed from the commit message summary line, and the change-set be committed?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
@Nate any comment on this?
Michael, are you referring to https://www.phoronix.com/scan.php?page=news_item&px=Intel-Open-Source-FS... ?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 176: None of the listed issues would exist if FSP were open-source and the : initialization steps could be integrated into coreboot, like they were : before.
Michael, are you referring to https://www.phoronix.com/scan. […]
@Tim yes, exactly
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 15: * `FSP-M`: Initializes the memory (DRAM) controller, among other Frankly, FSP-M has begun to do more and more over the generations. The 'M' distinction is slightly misleading. Obviously it does initialize the DRAM controller (and that's a big job), but it also does a decent amount of early silicon init too (check out the FSP-M UPDs to get some idea).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 15: * `FSP-M`: Initializes the memory (DRAM) controller, among other
Frankly, FSP-M has begun to do more and more over the generations. […]
Besides memory init, FSP also does HSIO programming (plus related PCIe, SATA, xHCI init), some MEI messaging stuff, DMI init, overclocking stuff, some TXT init, Trace Hub init... The list goes on.
FSP-S does some PCIe, SATA and USB configuration (IIRC, link training, capability programming and function swapping) that isn't about PHY tuning, some IOAPIC/HPET config (which I did in CB:35170 and is just programming a few registers), HDA config, configuring thermal throttling stuff, power management init, Serial I/O init, interrupt routing, CIO2 (camera) config, SCS (SD/eMMC) config, flash protection settings, some more MEI messages to signal End Of POST, BIOS Guard setup, SGX setup, CPU power management and VR settings...
The difference between FSP-M stuff and FSP-S stuff is that the former touches PHY stuff much more than the latter. And stuff surrounding PHY technology is usually kept very secret. Maybe PHYs are made out of arcane magic?
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
Interesting. While the BIOS Spec doesn't mention any specific point […]
Reminds me of something called M-Check, which happens at some point after memory is initialized and consists of locking down lots of stuff.
Nate DeSimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 126: FSP Switches SAI : ----------------
Interesting. While the BIOS Spec doesn't mention any specific point when it should be done, it mentions how to act if it's done after PCI enumeration. That's not part of MRC yet, is it?
The PCI enumeration language is probably discussing the ramifications around 3rd party OpROM dispatch, since that typically happens as part of the PCI enumeration proceedure. The BIOS spec is primarily written by the UEFI team, so yeah we inject some things that make sense as part of the firmware side flow.
If FspmUpd->FspmConfig.MmioSize == 0, then we will count the amount of address space needed to fit all MMIO. This involves a lightweight "PCI enumeration" that counts the amount of address space needed by any PCI devices installed on the system without actually assigning resources or touching the BAR values or trying to setup required alignment. That way the memory map that MRC constructs will be dynamically tailored to work on any system you throw at it automagically.
Also, I don't understand what the fuss is about. It seems most registers are still accessible with SMM_SAI. And let's be honest, if one expects the average IAFW to have SMM secured properly (no exploitable handlers), they probably also believe in fairy tales.
We have been trying to deprecate the "IAFW" term in favor of more generic ones like "system firmware." Personally, I consider "BIOS" to be somewhat of a genericism... kind of like how many people refer to facial tissues as "kleenex" or how searching the Internet is "googling."
We do see a path to SMM being secured on newer platforms with Intel chipsets. We have a new virtualization technology that sandboxes SMM.
Nate DeSimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
The new FSP 2.2 spec has changes intended to address some of the issues brought up here.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36328 )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... File Documentation/fsp/fsp-s_discussion.md:
https://review.coreboot.org/c/coreboot/+/36328/4/Documentation/fsp/fsp-s_dis... PS4, Line 15: * `FSP-M`: Initializes the memory (DRAM) controller, among other
The difference between FSP-M stuff and FSP-S stuff is that the former touches PHY stuff much more than the latter. And stuff surrounding PHY technology is usually kept very secret. Maybe PHYs are made out of arcane magic?
If I had to hazard a guess, SoC vendors are probably licensing at least some of this IP from other companies, and so I'm sure their licensing agreements would not allow publishing lots of details. I would also bet the same is true for the memory controllers, hence secrecy around MRC.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36328?usp=email )
Change subject: [RFC] Documentation/fsp: Discuss FSP-S issues ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
David Hendricks (Code Review)
-
Duncan Laurie (Code Review)
-
Marshall Dawson (Code Review)
-
Michael Niewöhner (Code Review)
-
Nate DeSimone (Code Review)
-
Nathaniel L Desimone (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Stefan Reinauer (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Werner Zeh (Code Review)