Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31493 )
Change subject: cbfstool: Add ifittool ......................................................................
Patch Set 11:
(23 comments)
There is some oddity around `fit_offset_converter_t`, otherwise the code looks pretty good :)
Some inline comments were written before I realized that the code was only shuffled around. And I didn't get to `ifittool.c` yet.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.h File util/cbfstool/fit.h:
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.h@49 PS11, Line 49: */ I honestly don't get it. This seems to sufficiently specify what the function does. If there is no room for alternative behavior, there is no need for a function pointer?
Can somebody help me here?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c File util/cbfstool/fit.c:
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@a132 PS11, Line 132: We can still assume that it contains at least the header.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@45 PS11, Line 45: and must be set to 0 Only until a platform starts to use them. It seems at least APL does.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@58 PS11, Line 58: * Bit 7 indicates whether component has valid checksum. mention C_V? as it's referred to as that below
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@182 PS11, Line 182: but only mentioned on page 22 of It is mentioned below "FIT Ordering Rules" in all specifications I know.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@188 PS11, Line 188: if (fit_table_entries(fit) <= 1) : return; What difference does it make?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@249 PS11, Line 249: fit_header It seems odd not to comment this function (but others with a self explanatory name). OTOH, maybe just give it a better name, e.g. fit_location_from_cbfs_header()?
Generally, I would expect the output parameter(s) to come first. Shouldn't `ptr` be `const void *`?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@256 PS11, Line 256: cbfs_file_get_header(&buf, &header); `buf.offset` (and `.name`) are still uninitialized
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@263 PS11, Line 263: ssize_t why not `size_t`?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@264 PS11, Line 264: total_entries isn't this more like `max_entries`?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@272 PS11, Line 272: mcode_file = cbfs_get_entry(image, blob_name); NB. while `struct cbfsfile` is used for a deserialized copy above, this seems to point into the serialized image...
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@300 PS11, Line 300: if (total_size < sizeof(*mcu_header)) Also check if it fits within the CBFS file?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@309 PS11, Line 309: file_length -= mcus[num_mcus].size; Potential overflow (see above)
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@332 PS11, Line 332: zero or multiple or one?
The existing comment comments the line directly below it. Your comment here and all the comments in the further added functions below would be better placed above or at the top of the functions.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@443 PS11, Line 443: entires entries
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@443 PS11, Line 443: ERROR("Maximum of FIT entires reached.\n"); free(mcus);
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@450 PS11, Line 450: mcus[i].offset), NB. Looking through the code this still only works if the CBFS is memory mapped at the top of 4GiB (e.g. not with the usual APL layouts). Despite the annoying abstraction. I assume it wasn't noticed because on APL the CSME creates the effective FIT.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@459 PS11, Line 459: sort?
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@480 PS11, Line 480: fit_pointer[1] != 0 Any hint that it's a 64-bit pointer? otherwise it wouldn't be specified what is at this location.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@533 PS11, Line 533: break; `break` is usually indented.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/fit.c@571 PS11, Line 571: lx PRIx64
https://review.coreboot.org/#/c/31493/11/util/cbfstool/ifittool.c File util/cbfstool/ifittool.c:
https://review.coreboot.org/#/c/31493/11/util/cbfstool/ifittool.c@118 PS11, Line 118: Cover the situation where a negative base address is given by the : * user. This doesn't seem to make sense in the new context.
https://review.coreboot.org/#/c/31493/11/util/cbfstool/ifittool.c@123 PS11, Line 123: return region->size - offset; NB. All calls I've seen so far seem to run into this case.
-
Nico Huber (Code Review)