Attention is currently required from: Martin Roth, Michał Żygowski, Marshall Dawson, Subrata Banik, Nikolai Vyssotski, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone. Furquan Shaikh 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/de7b7d8e_8c6acce3 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. 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?
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?