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