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 4:
(1 comment)
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/56190/comment/a08e99b9_7fb25368 PS3, Line 14: looks_like_fsp_header
Posted a response here: https://review.coreboot.org/c/coreboot/+/56190/comment/b1cfa114_4da88906/. […]
I believe its kind of yes and no IMHO.
Yes, because it make sense to ensure that older FSP header specification or package can even work with latest EDK2. but internally, i don't see that happen, we have strong recommendation which FSP spec + EDK2 stable release and platform. So, this tells me that its bad idea to use older FSP spec with new EDK2 headers.
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 ?
In either case HeaderLength and SpecVersion checking is kind of sanity to ensure that you have integrated the correct combination.
And reading the help test for HeaderLength, it doesn't really sounds like its says minimum header length is 72 for FSP2.0 spec. "Length of the header in bytes. The current value for this field is 72."
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 ?
1. Read FSP_INFO_HEADER.SpecVersion 2. If (FSP_INFO_HEADER.SpecVersion == 0x20) // FSP 2.0 Header
- Check if FSP_INFO_HEADER.HeaderLength is either 0x48 and 0x4C as you might have some platform where EDK2 latest version cause FSP header length appear as 0x4C rather 0x48 as per spec 2.0. Consider we accommodate the violation as well. 3. If (FSP_INFO_HEADER.SpecVersion == 0x22) // FSP 2.2 Header - Check if FSP_INFO_HEADER.HeaderLength is 0x4c 4. Else return false and print error ?
-
Subrata Banik (Code Review)