Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Martin Roth, Paul Menzel, Zheng Bao, Felix Held. Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54901 )
Change subject: amdfwtool: Add a function to extract firmwares ......................................................................
Patch Set 6:
(10 comments)
File util/amdfwtool/extract.c:
https://review.coreboot.org/c/coreboot/+/54901/comment/3fe9f2ed_2063eaf1 PS6, Line 15: 0 Use amd_fw_type enums here?
https://review.coreboot.org/c/coreboot/+/54901/comment/4ad26995_aecc240d PS6, Line 17: 8
Can this be sorted?
That's the order they go into the image 😄 The PSP needs the items in the order it plans to consume them.
https://review.coreboot.org/c/coreboot/+/54901/comment/2b49c595_1b89dad5 PS6, Line 68: psp_cookie = ppsp->header.cookie; Since this is only used in if/else decisions for naming below, would it look cleaner to make the decision here instead?
https://review.coreboot.org/c/coreboot/+/54901/comment/dc749790_5ba0b080 PS6, Line 75: if (token == 6 || token == 0xb) Use the amd_fw_type enums? BTW what is 6?
https://review.coreboot.org/c/coreboot/+/54901/comment/00898b73_38a69526 PS6, Line 91: PspDirL10_Typex What's the 0 there, i.e. L10? Is the plan to replace that with the subprogram value?
Oh, you have token as uint32, so you're already getting type/subprog/rsvd. In that case, I'd change it to %04x to be clearer (I assume you'll have a script later that can parse the filenames and use them as arguments back into amdfwtool).
https://review.coreboot.org/c/coreboot/+/54901/comment/2dc34d84_85a04e72 PS6, Line 110: extract_bios_fws All comments in extract_psp_fws() apply here too
https://review.coreboot.org/c/coreboot/+/54901/comment/47f058fb_d4c36521 PS6, Line 178: romsig_offset This has a strange look here after the init of romsig_offsets. I'd put it on the next line.
https://review.coreboot.org/c/coreboot/+/54901/comment/f08dd056_59a89a5d PS6, Line 206: romsig_offsets[index] != 0xFFFFFFFF Does this for() loop also need a check for not reading past the size of the buffer? For example, if you have an image size of 0x800000, you wouldn't want to read from offset 0xFA0000.
https://review.coreboot.org/c/coreboot/+/54901/comment/53911b00_dc391439 PS6, Line 238: ppsp = (psp_directory_table *)&buffer[psp2_offset]; : extract_psp_fws(buffer, ppsp, image_base, NULL); Probably surround these with
if (psp2_offset) { }
https://review.coreboot.org/c/coreboot/+/54901/comment/efdaf53a_d6454241 PS6, Line 242: pbdt = (bios_directory_table *)&buffer[bhd2_offset]; : extract_bios_fws(buffer, pbdt, image_base, NULL); same as above
-
Marshall Dawson (Code Review)