Attention is currently required from: Paul Menzel, Julius Werner. Mattias Nissler 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 3:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/62639/comment/6396c4aa_3cd278c8 PS2, Line 16: invoke new cbmem option
Please give an example.
Done
Patchset:
PS3: Ready for another round of review, thanks!
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62639/comment/b7301234_8501920a PS2, Line 88: char *v = mapping->virt;
Excuse my ignorance, why is the const removed?
Because this code is now also used to return writable addresses in map_memory_with_prot below (line 135). Note that map_memory is still const, so the existing code that sets up read-only mappings still gets const poitners.
https://review.coreboot.org/c/coreboot/+/62639/comment/a6ed8954_a8905a05 PS2, Line 546: return __rdtsc();
It looks like this isn't correct for all x86 platforms... e.g. […]
Thanks for calling this out, very good point. It took me a while to think about how to handle this correctly, but I think I've found a decent way now: Get the TSC frequency from the LB_TAG_TSC_INFO record and use it to convert the TSC value to whatever tick_freq_mhz scaling the timestamp table uses.
I've verified that this works correctly on zork and brya Chrome OS hardware.
https://review.coreboot.org/c/coreboot/+/62639/comment/33d095fe_359417ff PS2, Line 713: if (timestamps.tag != LB_TAG_TIMESTAMPS) {
nit: could consider moving this into main() to deduplicate
I decided not to. Rationale: Only some actions require the timestamp table to be present, and we don't want main to bail if there is no timestamp table.
https://review.coreboot.org/c/coreboot/+/62639/comment/c81ba5c9_967a7871 PS2, Line 728: fprintf(stderr, "Not enough space to add timestamp.\n");
nit: should this return an error code (i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/62639/comment/b25f6d9c_e1a4fca7 PS2, Line 1218: static void print_usage(const char *name, int exit_code)
Please update to describe new mode.
Done
https://review.coreboot.org/c/coreboot/+/62639/comment/6d7ffa34_bdcdda24 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 wou […]
Good point! The kernel doesn't make a distinction, but there might be environments in which /dev/mem is locked down via e.g. SELinux. Changed now to only open RW if needed.