Attention is currently required from: Furquan Shaikh, Martin Roth, Michał Żygowski, Marshall Dawson, Nikolai Vyssotski, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56190 )
Change subject: src/drivers/intel/fsp2_0: allow larger FSP 2.0 header ......................................................................
Patch Set 5:
(1 comment)
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/56190/comment/b275e003_c18eab92 PS3, Line 14: looks_like_fsp_header
No, the moment, we switch to new EDK2 header and introduced the Multi-SI package it would expects that your bootloader and FSP all are aligned with FSP2.2 spec. And unless we do that, we are expected to see this kind of issue. isn't it ?
It is basically adding an entry for the MultiSi which FSP 2.2 spec says:
"Offset for the API for the optional MultiPhase processor and chipset initialization defined in Section 8.10. This value is only valid if FSP HeaderRevision is >= 5. If the value is set to 0x00000000, then this API is not available in this component."
My understanding of this is - the header field is valid only when header revision is >= 5 which is when FSP version is >= 2.2. If the header field exists before that, then it is not valid and can be ignored. This means that the API is not available in the component.
Yes, you are right, and that's the reason, we didn't make calling to MultiSi mandatory unlike other FSP APIs https://github.com/coreboot/coreboot/blob/master/src/drivers/intel/fsp2_0/si... but the only discrepancy is that, irrespective of you are considering this new API or not, inclusion of MultiSi API into your EDK2 header actually claim the FSP header size == 76 as align with FSP 2.2
Also, I could able to see May 2020 Change list suggested that HeaderRevision is actually 5 using platform override FSP_INFO_HEADER changes o Updated SpecVersion from 0x21 to 0x22 o Updated HeaderRevision from 4 to 5 o Added FspMultiPhaseSiInitEntryOffset
Thus, the consumer which is coreboot in this case will be expected to ignore that field if FSP version is not >= 2.2. What kind of issues do you expect?
If we had compile type option then yes, we can say that based on HeaderRevision (PcdFspHeaderRevision) < 5, during compilation time EDK2 would drop inclusion of FspMultiPhaseSiInitEntryOffset hence the output FSP header would be 76-4 = 72. But unfortunately its static hence this kind of mismatch issue i was referring.
I believe you have tweaked "current" with "min" to accommodate the W/A that FSP2.0 might still supports a header that length is > 72? Very similar to what i have mentioned earlier to accommodate such W/A when you have older FSP package and latest EDK2 ?
Yes, it is similar to the W/A that you had posted. But instead of checking for specific spec versions and allowing combinations of lengths for each, the idea is that:
- Ensure that any bytes that will be accessed for given FSP version are valid. This can be done by ensuring that the header length is at least what is expected by the reported FSP version. If it is less, then there will be a problem because we cannot read required information from the header.
- Rest of the code can freely access the bytes it wants because we have already sanity checked that the header is big enough to provide the required information. When using an older FSP version with header larger than required, coreboot will not care to look at the extra bytes. So, it should just work fine.
What do you think?
patch looks good, but only consideration point imo is that, now its easy that one could add more APIs (using EDK2 override in platform code) and can grow the FSP header beyond what spec had mentioned and bootloader won't have any way to catch that as it would always pass the min size limit.