Attention is currently required from: Cliff Huang, Martin Roth, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Bernardo Perez Priego, Andrew McRae. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55503 )
Change subject: util/cse_serger: Add a new tool for stitching CSE components ......................................................................
Patch Set 23:
(11 comments)
File util/cbfstool/bpdt_formats/bpdt_1_7.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/6cf77a1d_48a8a7e7 PS22, Line 210: l->checksum = calculate_layout_checksum(l);
nit: both this line and `calculate_layout_checksum` set l->checksum
`calculate_layout_checksum()` stashes what is in l->checksum to curr_checksum, sets it to 0 to perform the calculation, restores l->checksum and returns the calculated value.
If it seems too confusing, I can see if there is a better way to write `calculate_layout_checksum`.
https://review.coreboot.org/c/coreboot/+/55503/comment/d56259d2_04dbd3ca PS22, Line 235: printf("BP4 size: 0x%x\n", l->bp4_size);
remove this line (extra copy of line 237)
Done
https://review.coreboot.org/c/coreboot/+/55503/comment/37ed25c8_a7297dc8 PS22, Line 317: crc32(0xffff
so the layout checksum is inverted but the header checksum is not?
It is on line 326 below once the checksum is calculated on header and all the entries.
File util/cbfstool/bpdt_formats/subpart_entry_1.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/9e8bf6fe_a1cdcb80 PS22, Line 49: printf("%-25s%-25s%-25s%-25s%-25s%-25s\n", "Entry #", "Name", "Offset", "Huffman Compressed?", : "Length", "Rsvd");
nit: the line is already over 96, either make it 1 line or break before 96
Done
File util/cbfstool/cse_serger.h:
https://review.coreboot.org/c/coreboot/+/55503/comment/0f86f7e6_3bd6cd3f PS21, Line 15:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/55503/comment/56b830fa_7cbd556a PS21, Line 17: write_member(_buff, &(_x), sizeof(_x))
nit: align with read_member above
Done
File util/cbfstool/cse_serger.c:
https://review.coreboot.org/c/coreboot/+/55503/comment/da16cc83_a9e9dc29 PS22, Line 79: ifwi
nit: I think the struct name is superfluous
Done
https://review.coreboot.org/c/coreboot/+/55503/comment/48ec7306_2782e7fd PS22, Line 99: subpart_info
also superfluous?
Done
https://review.coreboot.org/c/coreboot/+/55503/comment/71a40b99_e819d661 PS22, Line 167: : void write_member(struct buffer *buff, void *src, size_t size) : { : void *dst = buffer_get(buff); : : switch (size) { : case 1: : write_le8(dst, *(uint8_t *)src); : break; : case 2: : write_le16(dst, *(uint16_t *)src); : break; : case 4: : write_le32(dst, *(uint32_t *)src); : break; : case 8: : write_le64(dst, *(uint64_t *)src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : } : : void read_member(struct buffer *buff, void *dst, size_t size) : { : const void *src = buffer_get(buff); : : switch (size) { : case 1: : *(uint8_t *)dst = read_le8(src); : break; : case 2: : *(uint16_t *)dst = read_le16(src); : break; : case 4: : *(uint32_t *)dst = read_le32(src); : break; : case 8: : *(uint64_t *)dst = read_le64(src); : break; : default: : memcpy(dst, src, size); : break; : } : : buffer_seek(buff, size); : }
is it worth it to break these out into a separate file for cse_fpt and cse_serger to share? it's not […]
It makes sense. I will do that.
Done: CB:58201
https://review.coreboot.org/c/coreboot/+/55503/comment/5092b8b3_c0d0ef71 PS22, Line 304: int
size_t
Done
https://review.coreboot.org/c/coreboot/+/55503/comment/7c512477_6b4cdbc7 PS22, Line 857: if (c >= LONGOPT_START && c < LONGOPT_END) : return true; : : return false;
nit: […]
Done