Attention is currently required from: Jakub Czapiga, Tim Wawrzynczak, Paul Menzel, Alex Levin, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62474 )
Change subject: util/cbmem: Add FlameGraph-compatible timestamps output ......................................................................
Patch Set 11:
(6 comments)
File src/commonlib/include/commonlib/timestamp_serialized.h:
https://review.coreboot.org/c/coreboot/+/62474/comment/2b1adfe7_0d5c82a3 PS7, Line 454: uint32_t id_end;
It won't work. Look at TS_COPYVPD_START. It has two possible ends. […]
Hmm... okay, that's an odd case. Is that the only one? Even though the code is written like that I don't think there's actually any board that has an RW VPD but no RO VPD in practice.
I think having them all in one macro has the benefit that when people add new timestamps, they are more explicitly prompted to add a possible end symbol by the structure of the table. If you have the ranges hidden somewhere else in another table, I think it's a lot more likely people will forget to add new entries down there.
If you lay out the struct as uint32_t, uint32_t, char *, char *, then the second uint32_t will be hidden by padding anyway on 64-bit systems and doesn't make the table larger. Also, I wouldn't worry about the size too much... coreboot shouldn't really be including that list anyway. (CONFIG_TIMESTAMPS_ON_CONSOLE is a really terrible design one way or another tbh. If anyone actually wants to use that and have it be efficient, they should redesign it to a point where it only includes strings for timestamps that each stage actually uses, rather than every possible special vendor timestamp there is.)
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62474/comment/74dd0894_fd4f680f PS8, Line 629: ::
yeah I kinda like the `::`, I can't think of anything better
Oh, I thought this was given by the flame graph tool? Or is this how it will look like in the flame graph (i.e. it will show `TS_VBOOT_START::TS_VBOOT_END` as the label of one section)? Could maybe also use `->` or `<->`? (I assume spaces are illegal? Otherwise some spaces would probably help readability.)
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62474/comment/4c6cdd3c_96b78b23 PS11, Line 630: if (i < stacklvl) nit: I think if you add ` || last_part` here...
https://review.coreboot.org/c/coreboot/+/62474/comment/758c6476_5dfdb168 PS11, Line 634: if (stacklvl > 0) ...you can take this out.
https://review.coreboot.org/c/coreboot/+/62474/comment/d4185804_c4f9479e PS11, Line 727: sorted_tst_p->entries[match].entry_stamp; nit: I think adding base_time here would make it easier to understand than subtracting it below (because then stamp and match_stamp are constructed in a more symmetrical way).
https://review.coreboot.org/c/coreboot/+/62474/comment/961388ff_d7f54bf3 PS11, Line 1384: enum timestamps_print_type timestamp_type = TIMESTAMPS_PRINT_NORMAL; If you have an enum anyway maybe also roll print_timestamps into that (i.e. default to TIMESTAMPS_PRINT_NONE)?