Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38779 )
Change subject: vboot: push clear recovery mode switch until later ......................................................................
vboot: push clear recovery mode switch until later
On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 2 files changed, 19 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/1
diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 83baa81..61b3d76 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -24,6 +24,25 @@ #include <security/vboot/vbnv.h> #include <security/vboot/vboot_common.h>
+static void vboot_log_and_clear_recovery_mode_switch(void *unused) +{ + /* Log the recovery mode switches if required, before clearing them. */ + log_recovery_mode_switch(); + + /* + * The recovery mode switch is cleared (typically backed by EC) here + * to allow multiple queries to get_recovery_mode_switch() and have + * them return consistent results during the verified boot path as well + * as dram initialization. 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. + */ + clear_recovery_mode_switch(); +} +BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, + vboot_log_and_clear_recovery_mode_switch, NULL); + static int vboot_get_recovery_reason_shared_data(void) { struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context()); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 1d17a17..eabf8f7 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -269,26 +269,6 @@ vboot_extend_pcr(ctx, 1, HWID_DIGEST_PCR); }
-static void vboot_log_and_clear_recovery_mode_switch(int unused) -{ - /* Log the recovery mode switches if required, before clearing them. */ - log_recovery_mode_switch(); - - /* - * The recovery mode switch is cleared (typically backed by EC) here - * to allow multiple queries to get_recovery_mode_switch() and have - * them return consistent results during the verified boot path as well - * as dram initialization. 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. - */ - clear_recovery_mode_switch(); -} -#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) -ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch) -#endif - /** * Verify and select the firmware in the RW image * @@ -449,11 +429,6 @@ vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit: - /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care - of running this function. */ - if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - vboot_log_and_clear_recovery_mode_switch(0); - /* Save recovery reason in case of unexpected reboots on x86. */ vboot_save_recovery_reason_vbnv();
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38779 )
Change subject: vboot: push clear recovery mode switch until later ......................................................................
Patch Set 1:
(1 comment)
Moving this to ramstage (and simplifying further, please!) LGTM in general, but I think +Furquan knows what this is good for and what other constraints we might be missing best.
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); I think this should be combined with google_chromeec_elog_add_recovery_event(). What you're doing right now is creating a CBMEM entry in early ramstage that is then consumed by code later in ramstage. There's no need for those two steps to be separate and for this weird information-passing anymore. Let's just do it all in one go and get rid of one more transient-but-reserved-forever CBMEM region.
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#2).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 7 files changed, 20 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/2
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 2:
(2 comments)
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);
I think this should be combined with google_chromeec_elog_add_recovery_event(). […]
Ah... I did not realize that this was being stowed away in CBMEM as well.
Although now I'm a bit worried I'm not fully understanding why we were clearing the recovery mode switch at ROMSTAGE_CBMEM_INIT_HOOK, rather than just moving it later like we're doing in this CL now? Furquan, do you have any more background on this? Seems like the CLs link to the same bug: https://b/35582650
If we move the logic to google_chromeec_elog_add_recovery_event, then this "clear" step will move from BS_DEV_INIT later on to BS_WRITE_TABLES.
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. Furquan / Aaron, I'm not sure I fully understand this statement. Please let me know if I'm doing something horribly wrong.
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#3).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 7 files changed, 20 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/3
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 3:
This patch is now failing due to compiling with CHROMEOS and EC_GOOGLE_CHROMEEC, but not EC_GOOGLE_CHROMEEC_SWITCHES.
Should the last two be interdependent? Or should google_chromeec_add_recovery_event() move into ec/google/chromeec/switches.c?
When I try that, I get an error about BOOT_STATE_INIT_ENTRY not working properly:
src/ec/google/chromeec/switches.c:89:49: error: expected ')' before '(' token google_chromeec_elog_add_recovery_event, NULL); ^ ) CC romstage/lib/gpio.o src/ec/google/chromeec/switches.c:60:13: error: 'google_chromeec_elog_add_recovery_event' defined but not used [-Werror=unused-function] static void google_chromeec_elog_add_recovery_event(void *unused) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Why can't BOOT_STATE_INIT_ENTRY be used in this file? For reference, switches.c is included via:
verstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SWITCHES) += switches.c romstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SWITCHES) += switches.c ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SWITCHES) += switches.c
Furquan Shaikh 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 3:
(1 comment)
Patch Set 3:
This patch is now failing due to compiling with CHROMEOS and EC_GOOGLE_CHROMEEC, but not EC_GOOGLE_CHROMEEC_SWITCHES.
I think the problem is that call to clear_recovery_mode_switch() moved from src/security/vboot/ to src/ec/. CONFIG_VBOOT gets selected only when CONFIG_CHROMEOS is selected and hence I believe you are running into this issue. There is a weak clear_recovery_mode_switch() which is present in src/security/vboot/bootmode.c. Probably that needs to move to src/ec as well?
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.
Furquan / Aaron, I'm not sure I fully understand this statement. […]
This comment basically meant that: on x86, recovery mode switch was queried by more than just vboot and it was necessary that get_recovery_mode_switch() returned consistent values. Since ROMSTAGE_CBMEM_INIT_HOOK runs after memory is up, it was considered a safe place to clear out the switches since nothing is supposed to rely on the switch after this point.
Julius Werner 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 3:
(1 comment)
You cannot assume that there's an EC in the system, this needs to work when the recovery mode switch is implemented by a different backend as well. So you'll want to put a
BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_ENTRY, clear_recovery_mode_switch, NULL);
into vboot/bootmode.c, so that it gets called in all cases. For configurations that don't override clear_recovery_mode_switch(), that will simply be a no-op. chromeec/switches.c should override that function to do the logging and the clearing for the EC.
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). Same for intel/kblrvp.
Furquan Shaikh 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 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.
BTW, can this be done as part of a callback from vboot when it decides to clear the recovery reason? i.e. recovery reason in VBNV and recovery mode switches can potentially be cleared at the same time?
Julius Werner 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 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.
BTW, can this be done as part of a callback from vboot when it decides to clear the recovery reason? i.e. recovery reason in VBNV and recovery mode switches can potentially be cleared at the same time?
We decided to move recovery reason clearing into depthcharge recently (because that was just the easiest place to put it). Moving this there as well would be a bit complicated because it's not directly handled by vboot (so if it should be initiated by vboot code that would require a new vb2ex callback), and if you want to keep your ELOG entry we can't do that from depthcharge either. I hope spreading it out like this (clearing this in ramstage and nvdata recovery reason in depthcharge) should be fine, and it's the easiest way to get it into our existing boot flow for now.
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#4).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 24 insertions(+), 114 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/4
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#5).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 38 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/5
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!
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#6).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/Kconfig M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 9 files changed, 39 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/6
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 6:
I had to add VBOOT_NO_BOARD_SUPPORT in intel/galileo in order to get the weak get_recovery_mode_switch definition... not sure on the background behind this Kconfig option, but there's probably another cleanup to be done here?
Let's see if Jenkins is happy this time around.
Julius Werner 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 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38779/6/src/include/bootmode.h File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/38779/6/src/include/bootmode.h@24 PS6, Line 24: void clear_recovery_mode_switch(void *unused); It's kinda ugly to have the (void *) argument here, and I also realized that it would be nice to check for VB2_CONTEXT_FORCE_RECOVERY_MODE in the context before calling this (so that we avoid all that extra EC communication when we know there's nothing there to clear anyway). So maybe add a small stub function in bootmode.c to do that check and then call this without the unused argument?
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... File src/mainboard/intel/kblrvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... PS6, Line 64: You forgot to update the Kconfig for this one?
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#7).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 38 insertions(+), 99 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/7
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 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38779/6/src/include/bootmode.h File src/include/bootmode.h:
https://review.coreboot.org/c/coreboot/+/38779/6/src/include/bootmode.h@24 PS6, Line 24: void clear_recovery_mode_switch(void *unused);
It's kinda ugly to have the (void *) argument here, and I also realized that it would be nice to che […]
Done
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... File src/mainboard/intel/kblrvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... PS6, Line 64:
You forgot to update the Kconfig for this one?
Err... maybe I'll just put get_write_protect_state back and stop messing around with Kconfig files.
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#8).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 37 insertions(+), 99 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/8
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 8:
Failed to read test report file /home/coreboot/slave-root/workspace/coreboot-gerrit/abuild-chromeos.xml org.dom4j.DocumentException: Error on line 87733 of document : The element type "testcase" must be terminated by the matching end-tag "</testcase>".
I hope this isn't my fault...
Julius Werner 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 8:
Doesn't seem like your fault. Try to rebase onto the latest ToT and see if that fixes it.
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 9:
Patch Set 8:
Doesn't seem like your fault. Try to rebase onto the latest ToT and see if that fixes it.
Hmm, still no cigar.
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 9:
Seems to be working now? If someone did something to get Jenkins happy, then thank you!
Julius Werner 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 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38779/9/src/ec/google/chromeec/swit... File src/ec/google/chromeec/switches.c:
https://review.coreboot.org/c/coreboot/+/38779/9/src/ec/google/chromeec/swit... PS9, Line 76: return 0; nit: why are we no longer returning the status here? I think we should either change the function signature to return void, or keep returning a value that represents whether it was actually cleared (even if the caller doesn't necessarily use it).
Julius Werner 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 10: Code-Review+2
Hello Yu-Ping Wu, Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38779
to look at the new patch set (#11).
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 34 insertions(+), 98 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/11
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 11:
(1 comment)
Ready for commit after fixing nit
https://review.coreboot.org/c/coreboot/+/38779/9/src/ec/google/chromeec/swit... File src/ec/google/chromeec/switches.c:
https://review.coreboot.org/c/coreboot/+/38779/9/src/ec/google/chromeec/swit... PS9, Line 76: return 0;
nit: why are we no longer returning the status here? I think we should either change the function si […]
Unintentional... let me pass the value through.
Julius Werner 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 11: Code-Review+2
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 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... File src/mainboard/intel/kblrvp/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38779/6/src/mainboard/intel/kblrvp/... PS6, Line 64:
Err... maybe I'll just put get_write_protect_state back and stop messing around with Kconfig files.
Done
Joel Kitching has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38779 )
Change subject: vboot: push clear recovery mode switch until BS_WRITE_TABLES ......................................................................
vboot: push clear recovery mode switch until BS_WRITE_TABLES
Serves two purposes:
(1) On some platforms, FSP initialization may cause a reboot. Push clearing the recovery mode switch until after FSP code runs, so that a manual recovery request (three-finger salute) will function correctly under this condition.
(2) The recovery mode switch value is needed at BS_WRITE_TABLES for adding an event to elog. (Previously this was done by stashing the value in CBMEM_ID_EC_HOSTEVENT.)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38779 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/commonlib/include/commonlib/cbmem_id.h M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/switches.c M src/include/bootmode.h M src/mainboard/intel/galileo/vboot.c M src/mainboard/intel/kblrvp/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/vboot_logic.c 8 files changed, 34 insertions(+), 98 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index b063cd1..93d1464 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -72,7 +72,7 @@ #define CBMEM_ID_VBOOT_WORKBUF 0x78007343 #define CBMEM_ID_VPD 0x56504420 #define CBMEM_ID_WIFI_CALIBRATION 0x57494649 -#define CBMEM_ID_EC_HOSTEVENT 0x63ccbbc3 +#define CBMEM_ID_EC_HOSTEVENT 0x63ccbbc3 /* deprecated */ #define CBMEM_ID_EXT_VBT 0x69866684 #define CBMEM_ID_ROM0 0x524f4d30 #define CBMEM_ID_ROM1 0x524f4d31 diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 4bf41ac..1d351c5 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -81,41 +81,6 @@ }, };
-void log_recovery_mode_switch(void) -{ - uint64_t *events; - - if (cbmem_find(CBMEM_ID_EC_HOSTEVENT)) - return; - - events = cbmem_add(CBMEM_ID_EC_HOSTEVENT, sizeof(*events)); - if (!events) - return; - - *events = google_chromeec_get_events_b(); -} - -static void google_chromeec_elog_add_recovery_event(void *unused) -{ - uint64_t *events = cbmem_find(CBMEM_ID_EC_HOSTEVENT); - uint8_t event_byte = EC_HOST_EVENT_KEYBOARD_RECOVERY; - - if (!events) - return; - - if (!(*events & EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY))) - return; - - if (*events & - EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT)) - event_byte = EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT; - - elog_add_event_byte(ELOG_TYPE_EC_EVENT, event_byte); -} - -BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_ENTRY, - google_chromeec_elog_add_recovery_event, NULL); - uint8_t google_chromeec_calc_checksum(const uint8_t *data, int size) { int csum; diff --git a/src/ec/google/chromeec/switches.c b/src/ec/google/chromeec/switches.c index 3fd3808..1eb2f2f 100644 --- a/src/ec/google/chromeec/switches.c +++ b/src/ec/google/chromeec/switches.c @@ -14,8 +14,8 @@ */
#include <bootmode.h> -#include <cbmem.h> #include <ec/google/chromeec/ec.h> +#include <elog.h>
#if CONFIG(EC_GOOGLE_CHROMEEC_LPC) int get_lid_switch(void) @@ -41,29 +41,33 @@
int get_recovery_mode_retrain_switch(void) { - uint64_t events; - const uint64_t mask = - EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT); - /* * Check if the EC has posted the keyboard recovery event with memory * retrain. */ - events = google_chromeec_get_events_b(); + return !!(google_chromeec_get_events_b() & + EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT)); +}
- if (cbmem_possibly_online()) { - const uint64_t *events_save; +static void elog_add_recovery_mode_switch_event(void) +{ + uint64_t events = google_chromeec_get_events_b(); + uint8_t event_byte = EC_HOST_EVENT_KEYBOARD_RECOVERY;
- events_save = cbmem_find(CBMEM_ID_EC_HOSTEVENT); - if (events_save != NULL) - events |= *events_save; - } + if (!(events & EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY))) + return;
- return !!(events & mask); + if (events & EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT)) + event_byte = EC_HOST_EVENT_KEYBOARD_RECOVERY_HW_REINIT; + + elog_add_event_byte(ELOG_TYPE_EC_EVENT, event_byte); }
int clear_recovery_mode_switch(void) { + /* Log elog event before clearing */ + elog_add_recovery_mode_switch_event(); + /* Clear all host event bits requesting recovery mode. */ return google_chromeec_clear_events_b( EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY) | diff --git a/src/include/bootmode.h b/src/include/bootmode.h index 33148dc..3ae8746 100644 --- a/src/include/bootmode.h +++ b/src/include/bootmode.h @@ -22,7 +22,6 @@ int get_recovery_mode_switch(void); int get_recovery_mode_retrain_switch(void); int clear_recovery_mode_switch(void); -void log_recovery_mode_switch(void); int get_wipeout_mode_switch(void); int get_lid_switch(void);
diff --git a/src/mainboard/intel/galileo/vboot.c b/src/mainboard/intel/galileo/vboot.c index 8b6706c..3ec5bb3 100644 --- a/src/mainboard/intel/galileo/vboot.c +++ b/src/mainboard/intel/galileo/vboot.c @@ -24,12 +24,6 @@ #include "gen1.h" #include "gen2.h"
-int clear_recovery_mode_switch(void) -{ - /* Nothing to do */ - return 0; -} - int get_recovery_mode_switch(void) { return 0; @@ -41,10 +35,6 @@ return 0; }
-void log_recovery_mode_switch(void) -{ -} - void verstage_mainboard_init(void) { const struct reg_script *script; diff --git a/src/mainboard/intel/kblrvp/chromeos.c b/src/mainboard/intel/kblrvp/chromeos.c index 9db4674..647a430 100644 --- a/src/mainboard/intel/kblrvp/chromeos.c +++ b/src/mainboard/intel/kblrvp/chromeos.c @@ -62,16 +62,6 @@ return 0; }
-int clear_recovery_mode_switch(void) -{ - if (CONFIG(EC_GOOGLE_CHROMEEC)) - /* Clear keyboard recovery event. */ - return google_chromeec_clear_events_b( - EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEYBOARD_RECOVERY)); - - return 0; -} - int get_write_protect_state(void) { /* No write protect */ diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 83baa81..2a911cb 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -101,14 +101,27 @@
int __weak clear_recovery_mode_switch(void) { - // Weak implementation. Nothing to do. return 0; }
-void __weak log_recovery_mode_switch(void) +static void do_clear_recovery_mode_switch(void *unused) { - // Weak implementation. Nothing to do. + if (vboot_get_context()->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE) + clear_recovery_mode_switch(); } +/* + * The recovery mode switch (typically backed by EC) is not cleared until + * BS_WRITE_TABLES for two reasons: + * + * (1) On some platforms, FSP initialization may cause a reboot. Push clearing + * the recovery mode switch until after FSP code runs, so that a manual recovery + * request (three-finger salute) will function correctly under this condition. + * + * (2) To give the implementation of clear_recovery_mode_switch a chance to + * add an event to elog. See the function in chromeec/switches.c. + */ +BOOT_STATE_INIT_ENTRY(BS_WRITE_TABLES, BS_ON_ENTRY, + do_clear_recovery_mode_switch, NULL);
int __weak get_recovery_mode_retrain_switch(void) { diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 182128c..18c96d7 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -248,26 +248,6 @@ vboot_extend_pcr(ctx, 1, HWID_DIGEST_PCR); }
-static void vboot_log_and_clear_recovery_mode_switch(int unused) -{ - /* Log the recovery mode switches if required, before clearing them. */ - log_recovery_mode_switch(); - - /* - * The recovery mode switch is cleared (typically backed by EC) here - * to allow multiple queries to get_recovery_mode_switch() and have - * them return consistent results during the verified boot path as well - * as dram initialization. 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. - */ - clear_recovery_mode_switch(); -} -#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) -ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch) -#endif - /** * Verify and select the firmware in the RW image * @@ -428,11 +408,6 @@ vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit: - /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care - of running this function. */ - if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) - vboot_log_and_clear_recovery_mode_switch(0); - /* Save recovery reason in case of unexpected reboots on x86. */ vboot_save_recovery_reason_vbnv();
-
Furquan Shaikh (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)