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/f1d630d2_3457eb97 PS3, Line 14: looks_like_fsp_header
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
I think that is fine. When the particular FSP version is defined, it captures the size of the header at that point. So, coreboot as consumer needs to ensure that we have at least that many bytes to consume. If it is greater, we don't really need to care about the additional bytes. The additional bytes will have to be just considered invalid. Also, coreboot shouldn't really look at those bytes anyways since the reported version doesn't know how to use those bytes.
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.
We don't really need any compile time difference. Like I explained above, the additional bytes in the header are considered invalid by the consumer and will be ignored.
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.
If more fields are added to the header but the version is still older, bootloader should conform to the reported version and ignore rest of the fields as invalid. If the bytes are unused, do we care about what is in those fields at all?