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 3:
(2 comments)
File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/56190/comment/a884f1e3_c797371a PS2, Line 13: however, some FSP 2.0 variants have it as well.
I don't think we should have the header length hardcoded, and should instead rely on that length field (to a reasonable value), even if it's longer than the structure we're using. Just treat anything past the end of the structure as reserved.
Reading the spec again and rethinking how this is handled, I agree with what you said about not assuming a hardcoded header length for a specific version. IIUC, EDK2 FSP2Pkg is supposed to work with different FSP 2.x variants. Given that, as you mentioned, we will have to ensure that the header length is set to a reasonable value and treat anything beyond the expected length as reserved.
What do you think about this:
1. Rename FSP_HDR_LEN to FSP_MIN_HDR_LEN or better convert this into a helper: ``` static uint32_t fsp_hdr_get_expected_min_length(void) { if (CONFIG(PLATFORM_USES_FSP2_2)) return 76; else if (CONFIG(PLATFORM_USES_FSP2_1)) return 72; else if (CONFIG(PLATFORM_USES_FSP2_0)) return 72; else return dead_code_t(uint32_t); } ```
2. In `looks_like_fsp_header()`, ensure that `fsp_hdr_length >= fsp_hdr_get_expected_min_length()`, else fail.
This will sanity check that the header is not too small for the specific header revision and any extra bytes at the end of header will automatically be ignored.
File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/56190/comment/988554c5_0e11b758 PS3, Line 14: looks_like_fsp_header
Thanks Subrata!
Posted a response here: https://review.coreboot.org/c/coreboot/+/56190/comment/b1cfa114_4da88906/. I think the reason why EDK2 hasn't been updated is because it is compatible with different 2.x variants. Hence, the default is set to 2.0 and it is upto the FSP package to update the header version PCD if it supports a newer revision. It would be good to get a confirmation if that understanding/expectation is correct.