Attention is currently required from: Jason Glenesk, Raul Rangel, Jakub Czapiga, Matt DeVillier, Paul Menzel, Grzegorz Bernacki, Arthur Heymans, Fred Reitberger, Felix Held.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74266 )
Change subject: amdfwtool: Add --output-manifest option ......................................................................
Patch Set 3:
(4 comments)
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/74266/comment/8b9fdea3_855d88da PS3, Line 853: fopen( Recommend to use open system call to be consistent with the rest of the use-cases in this utility.
https://review.coreboot.org/c/coreboot/+/74266/comment/10c5743e_4270bdb5 PS3, Line 867: blob_fd = open(index->filename, O_RDONLY); : read(blob_fd, &hdr, sizeof(hdr)); : fprintf(version_file, "type: 0x%02x ver:%02x.%02x.%02x.%02x\n", : index->type, hdr.version[3], hdr.version[2], : hdr.version[1], hdr.version[0]); : close(blob_fd); You can add a function pointer/callback and a human-readable name of the binary into amd_fw_entry.
``` typedef struct _amd_fw_entry { amd_fw_type type; ... amd_fw_entry_hash *hash_entries; void (*output_manifest)(int manifest_fd, amd_fw_entry *fw_table); char blob_acronym[MAX_BLOB_ACRONYM_LEN]; } amd_fw_entry; ```
Then you can move the part of dumping the firmware version into a separate function and register that function as a output_manifest callback for the required blobs along with the blob_acronym.
Then you can update this for-loop as: ``` for (index = fw_table; index->type != AMD_FW_INVALID; index++) { if (!index->filename || !index->output_manifest ) continue; index->output_manifest(manifest_fd, index); } ```
I feel this will help to scale well when you to dump the version for additional blobs in the future.
https://review.coreboot.org/c/coreboot/+/74266/comment/76c1e914_62544ea7 PS3, Line 1509: _SIG _SIG suffix not required.
https://review.coreboot.org/c/coreboot/+/74266/comment/369125fa_72fac82c PS3, Line 2145: if (manifest_file) { : dump_blob_version(manifest_file, amd_psp_fw_table); : } This can be done before the amdfw_tool_cleanup. There is nothing in keeping it here. But it is too premature to dump the manifest file before the amdfw*.rom is ready.