Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38779 )
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
Patch Set 5:
(3 comments)
Patch Set 3:
BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_ENTRY, clear_recovery_mode_switch, NULL);
You might want to do this at BS_ON_EXIT so that it does not clear the switch before it is used to log.
I don't care where exactly it runs, but my intention was that all of this should be done in a single boot state callback. So the clear_recovery_mode_switch() function in chromeec/switches.c should both log (as an EC-specific implementation detail) and then clear in one go. That makes it easy to get the right ordering without having to pay attention to which boot state comes before which. There should be no separate boot state callback in the chromeec code itself.
I wrote PS4 before I had fully comprehended what Julius meant... the patchset splits functionality into two callbacks as Furquan suggested: - BS_ON_ENTER google_chromeec_elog_add_recovery_event() chromeec/ec.c - BS_ON_EXIT clear_recovery_mode_switch vboot/bootmode.c
Then I switched to putting the BOOT_STATE_INIT_ENTRY in bootmode.c, and the implementation (including logging and clearing) in chromeec/switches.c. It seems to work fine.
https://review.coreboot.org/c/coreboot/+/38779/3/src/mainboard/intel/galileo... File src/mainboard/intel/galileo/vboot.c:
https://review.coreboot.org/c/coreboot/+/38779/3/src/mainboard/intel/galileo... PS3, Line 27: int clear_recovery_mode_switch(void)
nit: unrelated, but this could also be removed (there's a weak stub in security/vboot already). […]
Done
https://review.coreboot.org/c/coreboot/+/38779/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38779/1/src/security/vboot/bootmode... PS1, Line 44: vboot_log_and_clear_recovery_mode_switch, NULL);
Ah... I did not realize that this was being stowed away in CBMEM as well. […]
Ack
https://review.coreboot.org/c/coreboot/+/38779/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/38779/1/src/security/vboot/vboot_lo... PS1, Line 281: x86 systems ignore the saved dram settings : * in the recovery path in order to start from a clean slate. Therefore : * clear the state here since this function is called when memory : * is known to be up.
This comment basically meant that: on x86, recovery mode switch was queried by more than just vboot […]
Got it, thanks!