David Hendricks has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom) ......................................................................
Patch Set 22:
(20 comments)
Almost there...
https://review.coreboot.org/#/c/23203/19/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/19/cli_classic.c@619 PS19, Line 619: out;
I would go for flash_size(), keep things simple: […]
Sounds good to me. Since there are dozens of places that will need to change, I'll send a follow-up patch.
https://review.coreboot.org/#/c/23203/20/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/20/cli_classic.c@95 PS20, Line 95: int main(int argc, char *argv[]) I just remembered that we have a read_buf_from_file() function in flashrom.c. Would be nice to have a file handling library eventually to make this stuff more obvious.
https://review.coreboot.org/#/c/23203/20/cli_classic.c@102 PS20, Line 102: e
This should be -1!
Yes, it most certainly should!
https://review.coreboot.org/#/c/23203/20/cli_classic.c@107 PS20, Line 107: , e
"rb" for windows? not sure
Done
https://review.coreboot.org/#/c/23203/20/cli_classic.c@655 PS20, Line 655: lashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIF updated to use read_buf_from_file()
https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl@203 PS20, Line 203:
Maybe add "flashrom" here? or at least an "it" would make the sentence correct.
I've updated this to use similar language as the --ifd option.
https://review.coreboot.org/#/c/23203/20/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/20/fmap.h@69 PS20, Line 69: onst
`const` on the parameter itself is meaningless in the contract
True. I think I saw it elsewhere in the codebase, wasn't sure if it's intended for readability or something. Anyway, no need to clutter this.
https://review.coreboot.org/#/c/23203/20/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/20/fmap.c@44 PS20, Line 44: {
could be static now
Done
https://review.coreboot.org/#/c/23203/20/fmap.c@47 PS20, Line 47:
All callers seem to know that it's non-NULL but ignore possible […]
Makes sense, done.
https://review.coreboot.org/#/c/23203/20/fmap.c@59 PS20, Line 59:
What you actually check here is not the overall flashsize itself but the size of this FMAP instance […]
@Werner: fmap->size is supposed to be the size of the address space we're working with (e.g. the flash size). It's not very well documented I guess :-/
@Nico: No, fmap doesn't have any concept of nesting.
https://review.coreboot.org/#/c/23203/20/fmap.c@113 PS20, Line 113: changed to msg_gdbg() since caller will likely print an error message
https://review.coreboot.org/#/c/23203/20/fmap.c@151 PS20, Line 151: uint8_t *buf; : : if (!flashctx || !flashctx->chip) : return 1; : : if (prepare_flash_access(flashctx, true, false, false, false)) : goto _finalize_ret; : : /* likely more memory than we need, but it simplifies handling and : * printing offsets to keep them uniform with what's on the ROM */ : buf = malloc(rom_offset + len); : if (!buf) { : msg_gerr("Out of memory.\n"); : goto _finalize_ret;
Beside the error message, this is probably the same as calling […]
Yep, let's clean up the redundant code.
https://review.coreboot.org/#/c/23203/20/fmap.c@179 PS20, Line 179: } :
make both const?
Done
https://review.coreboot.org/#/c/23203/20/fmap.c@182 PS20, Line 182: size_t rom_offset, size_t len, int min_stride) : {
Or just move it up into fmap_read_from_rom()?
Moved to fmap_read_from_rom()
https://review.coreboot.org/#/c/23203/20/fmap.c@256 PS20, Line 256: ret = 2; : continue; : } : : fmap_len = fmap_size(fmap); : struct fmap *tmp = fmap; : f
realloc() frees the original memory if moved. So we only have […]
According to the man page: "If the area pointed to was moved, a free(ptr) is done."
https://review.coreboot.org/#/c/23203/20/libflashrom.h File libflashrom.h:
https://review.coreboot.org/#/c/23203/20/libflashrom.h@67 PS20, Line 67: buf,
the later const is not meaningful here
Done
https://review.coreboot.org/#/c/23203/20/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/20/libflashrom.c@396 PS20, Line 396: msg_gerr("Cannot add fmap entries to layout - Too many entries.\n");
we could also check in advance `if (fmap->nareas > MAX_ROMLAYOUT)` […]
I like that idea, so I went ahead and added the check before the loop (using l->num_entries + fmap->nareas).
https://review.coreboot.org/#/c/23203/20/libflashrom.c@405 PS20, Line 405: emcpy(l->entries[l->num_en
FMAP_STRLEN is shorter, use that or MIN() of both? […]
Good ideas. I went ahead and just memset() romentry's name to 0. It's overkill, but not having garbage in that memory may help in case we need to debug something.
https://review.coreboot.org/#/c/23203/20/libflashrom.c@477 PS20, Line 477: msg_gerr("Failed to read fmap from buffer.\n");
redundant
Done
https://review.coreboot.org/#/c/23203/20/libflashrom.c@484 PS20, Line 484: goto _free_ret;
redundant
Done