Attention is currently required from: Hung-Te Lin, Jason Glenesk, Raul Rangel, Marshall Dawson, Tim Wawrzynczak, Christian Walter, Julius Werner, Arthur Heymans, Andrey Petrov, Fred Reitberger, Yu-Ping Wu, Felix Held. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63375 )
Change subject: [RFC] CBMEM: Have INIT_HOOKS in every stage ......................................................................
Patch Set 2:
(9 comments)
Patchset:
PS1:
Not a fan of the extra binary size impact that comes from this. […]
I did not see an easy way to achieve the conditional parameter with preprocessor. Reviewing the changes, I am not sure we even need that.
File src/arch/x86/acpi_bert_storage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/8d8a20f7_eab5d26b PS2, Line 602: CBMEM_INIT_HOOK(bert_storage_setup); This does not have the characteristics of other CBMEM_INIT_HOOKS where something either migrates from CAR to CBMEM or from CBMEM to .bss. IMHO this can be delayed to any bootstate prior to OS_RESUME_CHECK.
File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/fc395d4d_c131bd59 PS2, Line 698: CBMEM_CREATION_HOOK(cbfs_mcache_migrate); This file will one hook anyways for switch_to_postram_cache(), so we could combine the two and avoid adding a no-op hook.
File src/lib/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/332f324a_202c6175 PS2, Line 170: /* Run the romstage hook early so that the console region is one of the earliest created, and s/romstage/creation
File src/security/tpm/tspi/log.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/f0b4b9c0_adba26a6 PS2, Line 157: CBMEM_CREATION_HOOK(recover_tcpa_log); We might need some more complex logic here.
File src/soc/amd/stoneyridge/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/41fd9c1a_04cd40b6 PS1, Line 218: CBMEM_INIT_HOOK(migrate_power_state)
This looks like a functional change? Intentional? Maybe still put it in a separate commit in that ca […]
Well the file was not built for postcar or ramstage, so lack of !ENV_ROMSTAGE would not have mattered. Marking resolved, the conditional will be reflected either as a parameter or different macro name.
File src/soc/intel/baytrail/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/0ee42064_8de9c13a PS1, Line 35: CBMEM_INIT_HOOK(migrate_power_state)
Same? This doesn't look like something you'd want to run more than once?
Done
File src/soc/intel/braswell/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/12ae90e6_674f6013 PS1, Line 29: CBMEM_INIT_HOOK(migrate_power_state);
Same.
Done
File src/soc/intel/quark/storage_test.c:
https://review.coreboot.org/c/coreboot/+/63375/comment/ae2a6f9c_14fe0fc5 PS2, Line 228: #if ENV_ROMSTAGE spurious