[coreboot-gerrit] [M] Change in coreboot[master]: x86: Add .data section support for pre-memory stages

[Kyösti Mälkki (Code Review)]

Attention is currently required from: Anil Kumar K, Arthur Heymans, Bora Guvendik, Cliff Huang, Jérémy Compostella, Martin L Roth, Patrick Georgi, Paul Menzel, Yu-Ping Wu.

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77289?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: x86: Add .data section support for pre-memory stages
......................................................................

Patch Set 35: Code-Review+1

(1 comment)

File src/lib/bootblock.c:

https://review.coreboot.org/c/coreboot/+/77289/comment/b6327351_f8c7d8d3 :
PS35, Line 90: 		base_timestamp = timestamp_get();
Seems like implementations of init_timer() and timestamp_get() must not have non-zero static variables, since .data_load to .data copy happens later? I cannot remember who calls this main() function, though.

However the same applies to any caller of bootblock_main_with_timestamp(), execution path prior to that will see uninitialised (or zero-initialised) .data for globals? If the variable was written to, memcpy() will unconditionally overwrite it. I cannot think of an easy way to detect this buildtime and I am worried sometime in future someone will waste fair amount of time debugging a runtime failure due to this.

Ideally the memcpy() would happen before C entry to make sure this cannot happen?