Attention is currently required from: Mattias Nissler, Paul Menzel. 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 3:
(5 comments)
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62639/comment/f09de891_f391145e PS2, Line 546: return __rdtsc();
Thanks for calling this out, very good point. […]
That should do it for all x86 platforms that use the TSC, yeah. Technically there's nothing preventing x86 platforms from using other time sources (e.g. see CONFIG_LAPIC_MONOTONIC_TIMER). I guess that might not be a problem in practice for the platforms we care about.
Arm is still going to be a problem eventually, though. It would be good if we could also converge on all platforms using the architectural timer there. We're already doing that for Qualcomm, the MediaTek folks are still using a custom timer right now, I'll ask them if they can change that.
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62639/comment/640ffb44_9ce6bc60 PS3, Line 32: #if defined(__x86_64__) Can we make this
(defined(__i386__) || defined(__x86_64__)
? Chrome OS doesn't use 32-bit x86 userspace anymore, but some other coreboot users still might.
https://review.coreboot.org/c/coreboot/+/62639/comment/41522f1f_07ff6041 PS3, Line 467: return tsc_freq_khz / 1000; I'm not sure this is correct, tbh. This code is ancient and from before my time, so I don't really know on which platforms we would even expect the frequency entry to be missing from the table... but for those where it does, the TSC frequency may not necessarily be the right answer. coreboot doesn't always use the TSC for timestamps (especially on older platforms). So either we have to go hunting for someone who still remembers why this code path is here, or maybe just avoid touching it for now.
https://review.coreboot.org/c/coreboot/+/62639/comment/e2121a28_d07b748b PS3, Line 557: #if defined(__x86_64__) here too
https://review.coreboot.org/c/coreboot/+/62639/comment/9ba7c3ad_5affe9d3 PS3, Line 583: if (!strcmp(timestamp_ids[i].name, name)) How do you expect this to be used...
cbmem -a "starting to load romstage"
? That seems impractical. We recently added another field `enum_name` (see CB:62709... because `name` is really more of a `description`). I think using that would make more sense here, e.g.
cbmem -a TS_COPYROM_START