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 20:
(39 comments)
Thanks for the thorough review - great suggestions as usual.
I finally had some time to look through this all. Some of the proposed changes required substantial refactoring, so the code looks quite a bit different than the previous patch. It looks less like the code in cbfstool and elsewhere, but by the same token should look more like flashrom code.
I re-tested --fmap with ROM images that had were bsearch-able and ones that required lsearch. --fmap-file was simplified to only use lsearch since bsearch doesn't really matter there anyway.
LMK what you think.
https://review.coreboot.org/#/c/23203/19/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/19/cli_classic.c@58 PS19, Line 58: fmap in <fmapfile>
s/fmap contained in ? […]
Dropped the word "binary" to make it <80.
https://review.coreboot.org/#/c/23203/19/cli_classic.c@619 PS19, Line 619:
NB. […]
*nod*
What should we call it? FLASH_SIZE_BYTES()? Or maybe flash_size_bytes() as a static inline in flash.h?
https://review.coreboot.org/#/c/23203/19/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/19/fmap.h@33 PS19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free
Is the above license not GPL compatible?
It's dual-licensed, as the last paragraph states. Also, in general BSD licensed code can be used in GPLv2 projects.
Or maybe I'm missing something?
https://review.coreboot.org/#/c/23203/19/fmap.h@44 PS19, Line 44: #define FMAP_VER_MAJOR 1 /* this header's FMAP minor version */
If we only support v1.1 but there were lower versions, please […]
Done
https://review.coreboot.org/#/c/23203/19/fmap.h@55 PS19, Line 55:
writing it in big endian seems weird
agreed
https://review.coreboot.org/#/c/23203/19/fmap.c File fmap.c:
PS19:
The mixed usage of `size_t`, `unsigned int` and `unsigned long […]
OK, I went ahead and updated that in a few places.
Some functions return <0 to indicate failure and >=0 if successful e.g. to indicate offset. To avoid mixing types they could be updated to take an off_t buffer as an output and return 0 or 1 to indicate failure or success. But that would make the API inconsistent with how it's used elsewhere, and probably not worth it for now.
https://review.coreboot.org/#/c/23203/19/fmap.c@40 PS19, Line 40: #include "flash.h"
mmh? […]
Yes.
https://review.coreboot.org/#/c/23203/19/fmap.c@51 PS19, Line 51:
this is `fmap_size(fmap)`? […]
Done
https://review.coreboot.org/#/c/23203/19/fmap.c@57 PS19, Line 57: if (fmap->ver_major != FMAP_VER_MAJOR)
seems weird to not use a `for` loop
Usually I'd agree, but I think the author may have been trying to clarify how the conditions within the loop affect the flow.
I don't have a strong preference either way, so I went ahead and updated it.
https://review.coreboot.org/#/c/23203/19/fmap.c@62 PS19, Line 62:
seems weird to check this inside the loop
I suspect the intention here is to say that if we reach FMAP_STRLEN - 1 without quitting due to the above if-statement, then then this should return failure. Another way to write this would be: if ((i == FMAP_STRLEN - 1) && (fmap->name[i] != 0)
Or perhaps these two if-statements could be combined into: if (!isgraph(fmap->name[i]) && (fmap->name[i] != 0)
Anyway, even if it seems a bit awkward to you or I it might be best to leave it as is unless there is a real problem to address.
https://review.coreboot.org/#/c/23203/19/fmap.c@88 PS19, Line 88: bool fmap_found =
`size_t`?
Done
https://review.coreboot.org/#/c/23203/19/fmap.c@89 PS19, Line 89:
`bool`?
Meh. Done.
https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: c
<= ?
Yes, it should be inclusive. Done.
https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: *)&image[offset])) {
`sizeof(struct fmap)`
Yep, done.
https://review.coreboot.org/#/c/23203/19/fmap.c@92 PS19, Line 92: ap at
casting to `const` is unnecessary
Done
https://review.coreboot.org/#/c/23203/19/fmap.c@120 PS19, Line 120: msg_gerr("Out of memory.\n"); : return
should be below the inner loop for enhanced readability
Gratuitous, but will make the change anyway.
https://review.coreboot.org/#/c/23203/19/fmap.c@124 PS19, Line 124: p, fmap_size(fmap)); same as above where this should be inclusive and subtract the sizeof an fmap struct
https://review.coreboot.org/#/c/23203/19/fmap.c@126 PS19, Line 126:
so `&image[0]` is always checked? that doesn't seem to make much sense
True, I guess it was never a problem earlier, but of course now that we might have a slow programmers it's important to not waste cycles...
Anyway, I tried a couple of things and I think the simplest/cleanest solution to address that corner case is to have a variable that tracks whether we've checked offset 0.
https://review.coreboot.org/#/c/23203/19/fmap.c@129 PS19, Line 129: struct flashctx *const flashctx, size_t rom_offset, size_t len)
no need to break the line
Done
https://review.coreboot.org/#/c/23203/19/fmap.c@158 PS19, Line 158: malloc
initialization seems never used
Done.
https://review.coreboot.org/#/c/23203/19/fmap.c@166 PS19, Line 166: _free_ret:
I don't want to overoptimize things, but I have the feeling that […]
After thinking about this a bit, I think we should just get rid of fmap_bsearch in this file. We really only care about the bsearch if we're reading directly from ROM, but in this codepath we're only going to read from memory.
https://review.coreboot.org/#/c/23203/19/libflashrom.h File libflashrom.h:
https://review.coreboot.org/#/c/23203/19/libflashrom.h@64 PS19, Line 64: ,
qualification has no effect here and the name shouldn't be […]
OK.
https://review.coreboot.org/#/c/23203/19/libflashrom.h@67 PS19, Line 67: struct flashrom_flashctx *, const uint8_t *const buf, size_t len);
The library was intentionally designed to be free from file i/o. […]
Sure. It took a bit of refactoring, but done.
https://review.coreboot.org/#/c/23203/19/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/19/libflashrom.c@428 PS19, Line 428: 1 o
`!*fmap_out`
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@442 PS19, Line 442: msg_gdbg("Adding fmap layout to global layout.\n");
please break this line
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: c
<= ?
Yep, this should be inclusive. Done.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: t flashctx *const flashctx, const uint8_t *cons
could be asserted at the beginning of the function, right?
Done. I had a check in the calling function, but it's probably clearer to put it at the beginning of this function.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@468 PS19, Line 468:
both conditions don't make much sense, imho. it should be […]
Yes - I've added a variable to track whether or not we have checked offset 0. It's a bit of a kludge, but I think it addresses this corner case in the simplest manner.
This function always reads from flash so beginning at rom_offset is the most intuitive way.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@473 PS19, Line 473:
Since this does not bail out on the stride loop we might as well use continue instead of break? if w […]
Yes, that sounds like a good idea.
While we're at it, I demoted this debug print to msg_cdbg(). If there's one failure due to a locked region, there will likely be many dozens or hundreds of similar benign failures.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@479 PS19, Line 479: }
seems impossible by loop condition
Agreed, removed.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@485 PS19, Line 485: goto _free_ret;
missing check for NULL
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@486 PS19, Line 486: }
we could skip reading the signature again
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@491 PS19, Line 491:
I'm sure I've seen a function for this.
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@492 PS19, Line 492: return ret;
missing check for NULL
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@494 PS19, Line 494:
we could skip reading the header again
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@499 PS19, Line 499: */
IIRC, this can be checked on the header alone
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@515 PS19, Line 515: * @param layout Layout to bet set.
potentially leaking `fmap` if we didn't succeed
Done
https://review.coreboot.org/#/c/23203/19/libflashrom.c@442 PS19, Line 442: msg_gdbg("Adding fmap layout to global layout.\n"); : if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { : msg_gerr("Failed to add fmap regions to layout.\n"); : ret = 1; : } : : free(fmap); : return ret; : } : : /** : * @brief Read a layout by searching buffer for fmap. : * : * @param[out] layout Points to a struct flashrom_layout pointer that : * gets set if the fmap is read and parsed successfully. : * @param[in] flashctx Flash context : * @param[in] buffer Buffer to search in : * @param[in] size Size of buffer to search : * : * @return 0 on success, : * 2 if the fmap couldn't be read, : * 1 on any other error. : */ : int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **const layout, : struct flashctx *const flashctx, const uint8_t *const buf, size_t size) : { : struct fmap *fmap = NULL; : int ret = 1; : : if (!buf || !size) : goto _ret; : : msg_gdbg("Attempting to read fmap from buffer.\n"); : if (fmap_read_from_buffer(&fmap, buf, size)) { : msg_gerr("Failed to read fmap from buffer.\n"); : ret = 1; : goto _ret; : } : : msg_gdbg("Adding fmap layout to global layout.\n"); : if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { : msg_gerr("Failed to add fmap regions to layout.\n"); : ret = 1; : goto _free_ret; : } : : ret = 0; : _free_ret: : free(fmap); : _ret: : return ret; : } : : /** : * @brief Free a layout. : * : * @param layout Layout to free. : */ : void flashrom_layout_release(struct flashrom_layout *const layout) : { : if (layout == get_global_layout()) : return; : : free(layout); : } : : /** : * @brief Set the active layout for a flash context. : * : * Note: This just sets a pointer. The caller must not release the layout : * as long as he uses it through the given flash context. : * : * @param flashctx Flash context whose layout will be set. : * @param layout Layout to bet set. : This whole thing should go in fmap.c.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@534 PS19, Line 534:
We should either allocate it or add a check below to not overflow the […]
Done
-
David Hendricks (Code Review)