Attention is currently required from: Arthur Heymans, Furquan Shaikh, Subrata Banik, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Julian Schroeder, Nathaniel L Desimone, Felix Held, Isaac W Oram.
Ed Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/66819 )
Change subject: Recently published Intel CedarIslandFSP binary contains PE images in FSP-M and FSP-S. This causes coreboot boot hang on DeltaLake servers. ......................................................................
Patch Set 1:
(11 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/66819/comment/b4527454_48485638 PS1, Line 8: FSP-M and FSP-S. This causes coreboot boot hang on DeltaLake servers.
You may also want to point to FSP spec that boot loader needs support PE image format.
I tried to search for PE and TE in the FSP spec but could not find the info in the spec anywhere. Do you have that information handy, else it may not exist in the spec to add as a reference?
https://review.coreboot.org/c/coreboot/+/66819/comment/8f1bc7f1_3b4c13f1 PS1, Line 10: The change required to add support for pe_relocate in fsp-relocate.c
Suppose to add message stating that PE32 format support is added, but PE64 format support is left fo […]
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/07965ace_edc9d6f5 PS1, Line 15: This code is tested with FSP version 33A for DeltaLake boot.
You could add test descriptions after "TEST=".
Done
Patchset:
PS1: Updated commit and responded to all comments
File src/commonlib/fsp_relocate.c:
https://review.coreboot.org/c/coreboot/+/66819/comment/82bcf849_79b94d83 PS1, Line 145: /* FSP_INFO_HEADER at first file in FV within first RAW section. */
FSP_INFO_HEADER (is located) at
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/43763486_43669537 PS1, Line 211: printk(FSP_DBG_LVL, "FSP InfoHdr Image Base is %x\n", image_base);
Does it make sense to say "FSP PE32 InfoHdr Image Base is... […]
FspInfoHdr is for the entire FSP image, not just for PE. There is one per FSP.
https://review.coreboot.org/c/coreboot/+/66819/comment/c016799f_2e27bfc2 PS1, Line 242: printk(FSP_DBG_LVL, "%d Relocs for RVA %x\n", rnum, vaddr);
Suggest to add a tab \t to make the debug message more clear.
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/ecafd0a4_9cf83392 PS1, Line 249: printk(FSP_DBG_LVL, "reloc type %x offset %x aoff %x, base-0x%x\n",
suggest to add two tabs \t\t
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/e75d1740_10f9978d PS1, Line 277: // printk(FSP_DBG_LVL, "FSP InfoHdr Image Base updated to %lx\n", new_addr);
These comments could be deleted.
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/5ce34ccf_800e369b PS1, Line 650: printk(FSP_DBG_LVL, "PE image at offset %zx\n",
PE32 image
Done
https://review.coreboot.org/c/coreboot/+/66819/comment/8088044a_52c5de40 PS1, Line 653: }
Suggest catching the case where the type is neither EFI_SECTION_TE nor EFI_SECTION_PE32.
This suggestion does not work. It halts BIOS execution for some reason. Perhaps there are other sections that are not either of these and don't need relocation but we cannot return an error from this loop since we need to still calculate those offsets for the next one that might need relocation.