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 24:
(17 comments)
I had some time to hack on this some more and test using a Chromebook using --fmap-file (linear search), --fmap with the fmap offset at a weird location (try bsearch and fall back to linear search), and --fmap with the fmap at a bsearchable offset.
I'm not sure what to do about the man page... The line break seems weird.
https://review.coreboot.org/#/c/23203/22/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/22/cli_classic.c@619 PS22, Line 619: out
out_shutdown
Done
https://review.coreboot.org/#/c/23203/22/cli_classic.c@642 PS22, Line 642: && (flashrom_
I stared at this for a moment, now I'm sure the `!fmapfile` is […]
OK. I think it helps with readability since it flows nicely with the above `fmap && fmapfile`, but I don't have a strong preference.
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@52 PS22, Line 52: --fmap\fR|\fB--fmap-file\fR <file>) \
This line is unnecessarily long now. Please break before the […]
Yeah, it's not the longest line but it is quite long. I don't understand why you want to break before the `-n` though. The spacing doesn't make much sense to me, is there a standard or some general guidelines? I just took a guess.
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@202 PS22, Line 202: hrom suppo
par*ti*tioning
Done
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@217 PS22, Line 217: hrom suppo
partitioning
Done
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@219 PS22, Line 219: used to generate the layout.
leftover
oops, done.
https://review.coreboot.org/#/c/23203/22/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/22/fmap.h@3 PS22, Line 3: -present
I know it has been discussed already. But this is incredibly […]
Nah, I kinda like it actually. It just says explicitly what we already assume, and prevents issues with people updating copyright notices needlessly to match the present year whenever they touch a file.
In any case our non-lawyer opinions are irrelevant, so no point in arguing.
https://review.coreboot.org/#/c/23203/22/fmap.h@45 PS22, Line 45: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */
Didn't check earlier versions, but now this is unused? shouldn't […]
Sure.
FWIW, the only difference IIRC between 1.0 and 1.1 was that 1.1 required alignment to a 64-byte boundary so that bsearch wouldn't be too expensive on the hardware we had available at the time. We fallback to doing brute-force linear search on all bytes anyway so it doesn't matter too much, it was intended more as a guideline for implementation.
https://review.coreboot.org/#/c/23203/22/fmap.h@67 PS22, Line 67: int fmap_read_from_buffer(struct fmap **fmap_out, const uint8_t *buf, size_t len);
nit, could be static now
Done
https://review.coreboot.org/#/c/23203/22/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/22/fmap.c@1 PS22, Line 1:
nit, missing initial line break
Done
https://review.coreboot.org/#/c/23203/22/fmap.c@153 PS22, Line 153: int ret = -1; : uint8_t *b
Redundant, probably leftover?
Done
https://review.coreboot.org/#/c/23203/22/fmap.c@203 PS22, Line 203: }
leaking `fmap`
moved above malloc
https://review.coreboot.org/#/c/23203/22/fmap.c@260 PS22, Line 260: if (fmap_found) : break; : } : : if (!fmap_found) : goto _free_ret; : : fmap_len = fmap_size(fmap); : struct fmap *tmp = fmap; : fmap = realloc(fmap, fmap_len); : if (!fmap) { : msg_gerr("Failed to realloc.\n"); : free(tmp); : goto _free_ret; : } : : if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap), : offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) { : msg_cerr("Cannot read %zu bytes at offset %06zx\n", : fmap_len - sizeof(*fmap), offset + sizeof(*fmap)); : /* Treat read failure to be fatal since this : * should be a valid, usable fmap. */ : goto _free_ret; : } :
This path is flawed a little. At least it's missing an unconditional […]
The break seems to have been lost during refactoring :-/ Sigh.
I like the idea of doing more stuff outside of the inner loop. I don't think we should ever hit the realloc() a second time, and it should be fine to treat those error paths as fatal.
I suppose one could make an argument that if we see a valid fmap header but can't read the whole thing then we should continue trying to find another fmap. But I think in practice that is indicative of a hardware failure or error in the way the firmware ROM is laid out and it's useful to treat it as fatal. Maybe we should return a different code to the caller to indicate this and avoid doing the linear search.
https://review.coreboot.org/#/c/23203/22/fmap.c@292 PS22, Line 292: ret = 0;
This would still be set after a read failure, for instance. So it's […]
agreed
https://review.coreboot.org/#/c/23203/22/fmap.c@321 PS22, Line 321: an fma
How do you pronounce it? Is it `*a* fmap` or `*an* eff-map`?
I say "an" in my head. Usually 'a' is used before a noun beginning with a consonant, but that sounds awkward to me in this case so I've updated the comment.
https://review.coreboot.org/#/c/23203/22/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/22/libflashrom.c@433 PS22, Line 433: struct flashctx *const flashctx, off_t offset, size_t len)
Ugly, but as we were too lazy for proper deserialization again, […]
done
https://review.coreboot.org/#/c/23203/22/libflashrom.c@468 PS22, Line 468: * 3 if fmap parsing isn't implemented for the host,
Same guard here.
Done
-
David Hendricks (Code Review)