Attention is currently required from: Mattias Nissler. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62639 )
Change subject: util/cbmem: add an option to append timestamp ......................................................................
Patch Set 2:
(5 comments)
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62639/comment/4964fe47_eb4d6b50 PS2, Line 546: return __rdtsc(); It looks like this isn't correct for all x86 platforms... e.g. src/soc/amd/common/block/cpu/tsc/monotonic_timer.c overrides it differently. It's probably fine to keep it like this for the first version, but we should keep that in mind when expanding to more platforms (and maybe add a comment here to point it out). Getting this right across all platforms will be tricky.
(We've been thinking for a while it would be better to normalize all timestamps for x86 to microseconds anyway, it's just that nobody had time to tackle that yet.)
https://review.coreboot.org/c/coreboot/+/62639/comment/40f728fa_aaee2ef7 PS2, Line 713: if (timestamps.tag != LB_TAG_TIMESTAMPS) { nit: could consider moving this into main() to deduplicate
https://review.coreboot.org/c/coreboot/+/62639/comment/18468824_3fe4802e PS2, Line 728: fprintf(stderr, "Not enough space to add timestamp.\n"); nit: should this return an error code (i.e. use die())?
https://review.coreboot.org/c/coreboot/+/62639/comment/2db13995_90622d5b PS2, Line 1218: static void print_usage(const char *name, int exit_code) Please update to describe new mode.
https://review.coreboot.org/c/coreboot/+/62639/comment/ad8a99f3_8c2194ed PS2, Line 1470: mem_fd = open("/dev/mem", O_RDWR, 0); You probably know this better than me: are there configurations where opening /dev/mem read-only would be allowed, but read-write not? If so we should depend this on timestamp_id to make sure other modes still work on restricted configurations.
-
Julius Werner (Code Review)