Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option
With CL:1940398, this option is no longer needed. Recovery requests are not cleared until kernel verification stage is reached. If the FSP triggers any reboots, recovery requests will be preserved. In particular:
- Manual requests will be preserved via recovery switch state, whose behaviour is modified in CB:XXXXXX. - Other recovery requests will remain in nvdata across reboot.
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I52d17a3c6730be5c04c3c0ae020368d11db6ca3c Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Kconfig M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_logic.c M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 12 files changed, 2 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/38780/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ea70e65..54e88dd 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -156,14 +156,6 @@ reused by the succeeding stage. This is useful if a RAM space is too small to fit both the verstage and the succeeding stage.
-config VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT - bool - default n - help - This option ensures that the recovery request is not lost because of - reboots caused after vboot verification is run. e.g. reboots caused by - FSP components on Intel platforms. - config VBOOT_MUST_REQUEST_DISPLAY bool default y if VGA_ROM_RUN diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 61b3d76..5f15790 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -50,62 +50,21 @@ return sd->recovery_reason; }
-void vboot_save_recovery_reason_vbnv(void) -{ - if (!CONFIG(VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT)) - return; - - int reason = vboot_get_recovery_reason_shared_data(); - if (!reason) - return; - - set_recovery_mode_into_vbnv(reason); -} - -static void vboot_clear_recovery_reason_vbnv(void *unused) -{ - if (!CONFIG(VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT)) - return; - - set_recovery_mode_into_vbnv(0); -} - -/* - * Recovery reason stored in VBNV needs to be cleared before the state of VBNV - * is backed-up anywhere or jumping to the payload (whichever occurs - * first). Currently, vbnv_cmos.c backs up VBNV on POST_DEVICE. Thus, we need to - * make sure that the stored recovery reason is cleared off before that - * happens. - * IMPORTANT: Any reboot occurring after BS_DEV_INIT state will cause loss of - * recovery reason on reboot. Until now, we have seen reboots occurring on x86 - * only in FSP stages which run before BS_DEV_INIT. - */ -BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, - vboot_clear_recovery_reason_vbnv, NULL); - /* * vb2_check_recovery_request looks up different components to identify if there * is a recovery request and returns appropriate reason code: * 1. Checks if recovery mode is initiated by EC. If yes, returns * VB2_RECOVERY_RO_MANUAL. - * 2. Checks if recovery request is present in VBNV and returns the code read - * from it. - * 3. Checks if vboot verification is done. If yes, return the reason code from + * 2. Checks if vboot verification is done. If yes, return the reason code from * shared data. - * 4. If nothing applies, return 0 indicating no recovery request. + * 3. If nothing applies, return 0 indicating no recovery request. */ int vboot_check_recovery_request(void) { - int reason = 0; - /* EC-initiated recovery. */ if (get_recovery_mode_switch()) return VB2_RECOVERY_RO_MANUAL;
- /* Recovery request in VBNV. */ - if ((reason = get_recovery_mode_from_vbnv()) != 0) - return reason; - /* Identify if vboot verification is already complete. */ if (vboot_logic_executed()) return vboot_get_recovery_reason_shared_data(); diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 2d5b084..c629ca5 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -52,11 +52,6 @@ int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw);
/* - * Source: security/vboot/bootmode.c - */ -void vboot_save_recovery_reason_vbnv(void); - -/* * The stage loading code is compiled and entered from multiple stages. The * helper functions below attempt to provide more clarity on when certain * code should be called. They are implemented inline for better compile-time diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index be598ac..a5a7806 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -101,26 +101,6 @@ vbnv_initialized = 0; }
-/* Save a recovery reason into VBNV. */ -void set_recovery_mode_into_vbnv(int recovery_reason) -{ - uint8_t vbnv_copy[VBOOT_VBNV_BLOCK_SIZE]; - - read_vbnv(vbnv_copy); - - vbnv_copy[RECOVERY_OFFSET] = recovery_reason; - vbnv_copy[CRC_OFFSET] = crc8_vbnv(vbnv_copy, CRC_OFFSET); - - save_vbnv(vbnv_copy); -} - -/* Read the recovery reason from VBNV. */ -int get_recovery_mode_from_vbnv(void) -{ - vbnv_setup(); - return vbnv[RECOVERY_OFFSET]; -} - /* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { diff --git a/src/security/vboot/vbnv.h b/src/security/vboot/vbnv.h index a2f0b4c..7d288d5 100644 --- a/src/security/vboot/vbnv.h +++ b/src/security/vboot/vbnv.h @@ -23,8 +23,6 @@ void save_vbnv(const uint8_t *vbnv_copy); int verify_vbnv(uint8_t *vbnv_copy); void regen_vbnv_crc(uint8_t *vbnv_copy); -int get_recovery_mode_from_vbnv(void); -void set_recovery_mode_into_vbnv(int recovery_reason);
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index eabf8f7..465c85b 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -429,8 +429,5 @@ vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit: - /* Save recovery reason in case of unexpected reboots on x86. */ - vboot_save_recovery_reason_vbnv(); - timestamp_add_now(TS_END_VBOOT); } diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index c3fcad9..7d69a92 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -93,7 +93,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 0d69da2..6c90294 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -113,7 +113,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index d098785..b68e93d 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -260,7 +260,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 15e895f..210fd0d 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -165,7 +165,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 6277cea..7382df0 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -93,7 +93,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index cef1fd0..56a0d74 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -189,7 +189,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
Hello Patrick Rudolph, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38780
to look at the new patch set (#2).
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option
With CL:1940398, this option is no longer needed. Recovery requests are not cleared until kernel verification stage is reached. If the FSP triggers any reboots, recovery requests will be preserved. In particular:
- Manual requests will be preserved via recovery switch state, whose behaviour is modified in CB:38779. - Other recovery requests will remain in nvdata across reboot.
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I52d17a3c6730be5c04c3c0ae020368d11db6ca3c Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Kconfig M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_logic.c M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 12 files changed, 2 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/38780/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 46: static int vboot_get_recovery_reason_shared_data(void) Since now this function is used only in one place, should we consider moving the 3-line code into vboot_check_recovery_request()?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
LGTM but would also like Furquan's (or Aaron's) second opinion.
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch()) BTW, this is slightly unrelated but for further improvement of this function I would consider moving this to the end of the function (to allow compile-time elimination in all relevant stages). (Or just take this and the vboot_logic_executed() check out so we'll hit an assertion in vboot_get_context() if any code calls this before recovery reason can be known, which they really shouldn't anyway.)
Hello Yu-Ping Wu, Patrick Rudolph, Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38780
to look at the new patch set (#3).
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option
With CL:1940398, this option is no longer needed. Recovery requests are not cleared until kernel verification stage is reached. If the FSP triggers any reboots, recovery requests will be preserved. In particular:
- Manual requests will be preserved via recovery switch state, whose behaviour is modified in CB:38779. - Other recovery requests will remain in nvdata across reboot.
These functions now only work after verstage has run: int vboot_check_recovery_request(void) int vboot_recovery_mode_enabled(void) int vboot_developer_mode_enabled(void)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I52d17a3c6730be5c04c3c0ae020368d11db6ca3c Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/Kconfig M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_logic.c M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 12 files changed, 10 insertions(+), 114 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/80/38780/3
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 46: static int vboot_get_recovery_reason_shared_data(void)
Since now this function is used only in one place, should we consider moving the 3-line code into vb […]
Done
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
BTW, this is slightly unrelated but for further improvement of this function I would consider moving […]
If we are going to rely on the assertion in vboot_get_context(), we may as well do the same for recovery_mode_enabled() and developer_mode_enabled(). That means these three functions should only ever be called *after* verstage has run.
vboot_check_recovery_request() is used for (1) deciding whether to retrain memory (in mt8183 from CB:36251), and (2) to log the recovery reason into elog.
vboot_recovery_mode_enabled() is used to decide which memory cache to use (normal or memory) as part of memory initialization. Also used in veyron_rialto/mainboard.c (CB:12135), and in disallowing Cr50 updates in recovery mode (cr50_enable_update.c). Not totally sure if those last two cases are guaranteed to be after verstage?
vboot_developer_mode_enabled() is used to decide whether or not to enable UDC (vboot_can_enable_udc), and is also saved to elog. Not totally sure about the UDC case.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
If we are going to rely on the assertion in vboot_get_context(), we may as well do the same for reco […]
Yes, all those cases you listed are running after verstage (at least for STARTS_IN_BOOTBLOCK which should be all affected platforms). I think it might be helpful to trigger that assertion to make people aware if they're calling an API that has no defined result at that point.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 3: Code-Review+2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
Yes, all those cases you listed are running after verstage (at least for STARTS_IN_BOOTBLOCK which s […]
Currently the assertion that would be triggered is inside vboot_get_context():
assert(verification_should_run());
Do you mean that we need something clearer? If so, probably we could just clarify in vboot_get_context() rather than each separate function for retrieving vboot modes? Something like:
if (!verification_should_run()) die("vboot context should not be retrieved before verstage has run")
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
Currently the assertion that would be triggered is inside vboot_get_context(): […]
No, I just meant we should directly call vboot_get_context() from all those functions depending on that, without worrying about vboot_logic_executed(). We want that assertion (I think it's fine as it is but feel free to update it as suggested if you prefer) to hit whenever someone calls those functions at a point where they shouldn't, rather than just return an incorrect value.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
No, I just meant we should directly call vboot_get_context() from all those functions depending on t […]
Sorry Julius -- I'm a little bit lost as to whether or not you're asking me to change something here. Could you clarify?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/38780/2/src/security/vboot/bootmode... PS2, Line 65: if (get_recovery_mode_switch())
Sorry Julius -- I'm a little bit lost as to whether or not you're asking me to change something here […]
No, I'm just trying to say that I think the way this patch does it now is good.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 9: Code-Review+2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 10:
Ready for commit
Joel Kitching has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option
With CL:1940398, this option is no longer needed. Recovery requests are not cleared until kernel verification stage is reached. If the FSP triggers any reboots, recovery requests will be preserved. In particular:
- Manual requests will be preserved via recovery switch state, whose behaviour is modified in CB:38779. - Other recovery requests will remain in nvdata across reboot.
These functions now only work after verstage has run: int vboot_check_recovery_request(void) int vboot_recovery_mode_enabled(void) int vboot_developer_mode_enabled(void)
BUG=b:124141368, b:35576380 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I52d17a3c6730be5c04c3c0ae020368d11db6ca3c Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38780 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/security/vboot/Kconfig M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vbnv.c M src/security/vboot/vbnv.h M src/security/vboot/vboot_logic.c M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/intel/tigerlake/Kconfig 12 files changed, 10 insertions(+), 114 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ea70e65..54e88dd 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -156,14 +156,6 @@ reused by the succeeding stage. This is useful if a RAM space is too small to fit both the verstage and the succeeding stage.
-config VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT - bool - default n - help - This option ensures that the recovery request is not lost because of - reboots caused after vboot verification is run. e.g. reboots caused by - FSP components on Intel platforms. - config VBOOT_MUST_REQUEST_DISPLAY bool default y if VGA_ROM_RUN diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 2a911cb..50b3cc3 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -24,79 +24,25 @@ #include <security/vboot/vbnv.h> #include <security/vboot/vboot_common.h>
-static int vboot_get_recovery_reason_shared_data(void) -{ - struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context()); - assert(sd); - return sd->recovery_reason; -} - -void vboot_save_recovery_reason_vbnv(void) -{ - if (!CONFIG(VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT)) - return; - - int reason = vboot_get_recovery_reason_shared_data(); - if (!reason) - return; - - set_recovery_mode_into_vbnv(reason); -} - -static void vboot_clear_recovery_reason_vbnv(void *unused) -{ - if (!CONFIG(VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT)) - return; - - set_recovery_mode_into_vbnv(0); -} - /* - * Recovery reason stored in VBNV needs to be cleared before the state of VBNV - * is backed-up anywhere or jumping to the payload (whichever occurs - * first). Currently, vbnv_cmos.c backs up VBNV on POST_DEVICE. Thus, we need to - * make sure that the stored recovery reason is cleared off before that - * happens. - * IMPORTANT: Any reboot occurring after BS_DEV_INIT state will cause loss of - * recovery reason on reboot. Until now, we have seen reboots occurring on x86 - * only in FSP stages which run before BS_DEV_INIT. + * Functions which check vboot information should only be called after verstage + * has run. Otherwise, they will hit the assertion in vboot_get_context(). */ -BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT, - vboot_clear_recovery_reason_vbnv, NULL);
-/* - * vb2_check_recovery_request looks up different components to identify if there - * is a recovery request and returns appropriate reason code: - * 1. Checks if recovery mode is initiated by EC. If yes, returns - * VB2_RECOVERY_RO_MANUAL. - * 2. Checks if recovery request is present in VBNV and returns the code read - * from it. - * 3. Checks if vboot verification is done. If yes, return the reason code from - * shared data. - * 4. If nothing applies, return 0 indicating no recovery request. - */ int vboot_check_recovery_request(void) { - int reason = 0; - - /* EC-initiated recovery. */ - if (get_recovery_mode_switch()) - return VB2_RECOVERY_RO_MANUAL; - - /* Recovery request in VBNV. */ - if ((reason = get_recovery_mode_from_vbnv()) != 0) - return reason; - - /* Identify if vboot verification is already complete. */ - if (vboot_logic_executed()) - return vboot_get_recovery_reason_shared_data(); - - return 0; + /* TODO: Expose vb2api_recovery_reason() and vb2api_need_train_and_reboot(). */ + return vb2_get_sd(vboot_get_context())->recovery_reason; }
int vboot_recovery_mode_enabled(void) { - return !!vboot_check_recovery_request(); + return vboot_get_context()->flags & VB2_CONTEXT_RECOVERY_MODE; +} + +int vboot_developer_mode_enabled(void) +{ + return vboot_get_context()->flags & VB2_CONTEXT_DEVELOPER_MODE; }
int __weak clear_recovery_mode_switch(void) @@ -133,12 +79,6 @@ return get_recovery_mode_retrain_switch(); }
-int vboot_developer_mode_enabled(void) -{ - return vboot_logic_executed() && - vboot_get_context()->flags & VB2_CONTEXT_DEVELOPER_MODE; -} - #if CONFIG(VBOOT_NO_BOARD_SUPPORT) /** * TODO: Create flash protection interface which implements get_write_protect_state. diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 324af5c..97944d9 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -50,11 +50,6 @@ int vboot_locate_firmware(struct vb2_context *ctx, struct region_device *fw);
/* - * Source: security/vboot/bootmode.c - */ -void vboot_save_recovery_reason_vbnv(void); - -/* * The stage loading code is compiled and entered from multiple stages. The * helper functions below attempt to provide more clarity on when certain * code should be called. They are implemented inline for better compile-time diff --git a/src/security/vboot/vbnv.c b/src/security/vboot/vbnv.c index be598ac..a5a7806 100644 --- a/src/security/vboot/vbnv.c +++ b/src/security/vboot/vbnv.c @@ -101,26 +101,6 @@ vbnv_initialized = 0; }
-/* Save a recovery reason into VBNV. */ -void set_recovery_mode_into_vbnv(int recovery_reason) -{ - uint8_t vbnv_copy[VBOOT_VBNV_BLOCK_SIZE]; - - read_vbnv(vbnv_copy); - - vbnv_copy[RECOVERY_OFFSET] = recovery_reason; - vbnv_copy[CRC_OFFSET] = crc8_vbnv(vbnv_copy, CRC_OFFSET); - - save_vbnv(vbnv_copy); -} - -/* Read the recovery reason from VBNV. */ -int get_recovery_mode_from_vbnv(void) -{ - vbnv_setup(); - return vbnv[RECOVERY_OFFSET]; -} - /* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void) { diff --git a/src/security/vboot/vbnv.h b/src/security/vboot/vbnv.h index a2f0b4c..7d288d5 100644 --- a/src/security/vboot/vbnv.h +++ b/src/security/vboot/vbnv.h @@ -23,8 +23,6 @@ void save_vbnv(const uint8_t *vbnv_copy); int verify_vbnv(uint8_t *vbnv_copy); void regen_vbnv_crc(uint8_t *vbnv_copy); -int get_recovery_mode_from_vbnv(void); -void set_recovery_mode_into_vbnv(int recovery_reason);
/* Read the USB Device Controller(UDC) enable flag from VBNV. */ int vbnv_udc_enable_flag(void); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 18c96d7..df2f002 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -408,8 +408,5 @@ vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit: - /* Save recovery reason in case of unexpected reboots on x86. */ - vboot_save_recovery_reason_vbnv(); - timestamp_add_now(TS_END_VBOOT); } diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index c3fcad9..7d69a92 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -93,7 +93,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_STARTS_IN_BOOTBLOCK - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 0d69da2..6c90294 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -113,7 +113,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index d098785..b68e93d 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -260,7 +260,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 42e86c7..15a5a31 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -165,7 +165,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index ae60a63..0340282 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -94,7 +94,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index 1b90d4b..79d74b4 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -189,7 +189,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_MUST_REQUEST_DISPLAY - select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 11:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/630 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/629 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/628
Please note: This test is under development and might not be accurate at all!
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 11:
That seems to cause a regression boards where VBOOT starts in romstage, as vboot_recovery_mode_enabled() calls vboot_get_context() that haven't been initialized yet.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 11:
That seems to cause a regression boards where VBOOT starts in romstage, as vboot_recovery_mode_enabled() calls vboot_get_context() that haven't been initialized yet.
Yeah, that was somewhat intentional... nobody should be calling vboot_recovery_mode_enabled() before vboot has run, so we decided to fail more aggressively there to recognize those cases so we can fix them. Can you let us know which boards exactly you're seeing this on and where it is called from so we can address those? (I mostly see it called from platform-specific code, and where vboot starts should be a per-platform decision, so I assume those are all correct. The only platform-independent case I see is in drivers/mrc_cache... for those, I'd suggest we explicitly check for CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) to make it clear that that case is not intended to execute on platforms which are still in RO when it runs.)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 11:
Patch Set 11:
That seems to cause a regression boards where VBOOT starts in romstage, as vboot_recovery_mode_enabled() calls vboot_get_context() that haven't been initialized yet.
Yeah, that was somewhat intentional... nobody should be calling vboot_recovery_mode_enabled() before vboot has run, so we decided to fail more aggressively there to recognize those cases so we can fix them. Can you let us know which boards exactly you're seeing this on and where it is called from so we can address those? (I mostly see it called from platform-specific code, and where vboot starts should be a per-platform decision, so I assume those are all correct. The only platform-independent case I see is in drivers/mrc_cache... for those, I'd suggest we explicitly check for CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) to make it clear that that case is not intended to execute on platforms which are still in RO when it runs.)
It looks like all x86 boards that select VBOOT_STARTS_IN_ROMSTAGE are broken as all of them call vboot_recovery_mode_enabled() in raminit, where vboot has run yet.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38780 )
Change subject: vboot: remove VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT option ......................................................................
Patch Set 11:
It looks like all x86 boards that select VBOOT_STARTS_IN_ROMSTAGE are broken as all of them call vboot_recovery_mode_enabled() in raminit, where vboot has run yet.
Here's my attempt at a fix: CB:39221
-
9elements QA (Code Review) -
Furquan Shaikh (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Yu-Ping Wu (Code Review)