Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33532
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
vboot: remove vboot_handoff_get_recovery_reason
Two functions retrieve vboot recovery_reason: * vboot_handoff_get_recovery_reason * vboot_get_recovery_reason_shared_data
Previously, when CBMEM comes online, a vboot_handoff data structure is created, and depending on the architecture, coreboot may eventually lose access to vboot_working_data.
After implementing vboot_working_data CBMEM migration, vboot_working_data is always guaranteed to be accessible.
vboot_get_recovery_reason_shared_data is corrected to also allow accessing vboot_working_data in ramstage and postcar.
Now, vboot_handoff_get_recovery reason returning a valid recovery reason implies that vboot_get_recovery_reason_shared_data should *also* return a valid recovery reason. Thus we may remove the former.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iac216dc968dd155d9d4f8bd0f2dfd5034762f9a0 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/bootmode.c M src/security/vboot/vboot_common.c M src/security/vboot/vboot_common.h 3 files changed, 0 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/33532/1
diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 68749f0..0d9721b 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -26,10 +26,6 @@
static int vboot_get_recovery_reason_shared_data(void) { - /* Shared data does not exist for Ramstage and Post-CAR stage. */ - if (ENV_RAMSTAGE || ENV_POSTCAR) - return 0; - struct vb2_shared_data *sd = vboot_get_shared_data(); assert(sd); return sd->recovery_reason; @@ -115,16 +111,6 @@ return reason;
/* - * Check recovery flag in vboot_handoff for stages post CBMEM coming - * online. Since for some stages there is no way to know if cbmem has - * already come online, try looking up handoff anyways. If it fails, - * flow will fallback to looking up shared data. - */ - if (cbmem_possibly_online() && - ((reason = vboot_handoff_get_recovery_reason()) != 0)) - return reason; - - /* * For stages where CBMEM might not be online, identify if vboot * verification is already complete and no slot was selected * i.e. recovery path was requested. diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c index ff8e6c8..a18bf23 100644 --- a/src/security/vboot/vboot_common.c +++ b/src/security/vboot/vboot_common.c @@ -93,19 +93,6 @@ return vboot_get_handoff_flag(VB_INIT_OUT_ENABLE_RECOVERY); }
-int vboot_handoff_get_recovery_reason(void) -{ - struct vboot_handoff *vbho; - VbSharedDataHeader *sd; - - if (vboot_get_handoff_info((void **)&vbho, NULL)) - return 0; - - sd = (VbSharedDataHeader *)vbho->shared_data; - - return sd->recovery_reason; -} - /* ============================ VBOOT REBOOT ============================== */ void __weak vboot_platform_prepare_reboot(void) { diff --git a/src/security/vboot/vboot_common.h b/src/security/vboot/vboot_common.h index a785a8b..a3180c1d 100644 --- a/src/security/vboot/vboot_common.h +++ b/src/security/vboot/vboot_common.h @@ -59,10 +59,8 @@ * If vboot handoff is available: * Returns 1 for flag if true * Returns 0 for flag if false - * Returns value read for other fields */ int vboot_handoff_check_recovery_flag(void); -int vboot_handoff_get_recovery_reason(void);
/* ============================ VBOOT REBOOT ============================== */ /*
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c File src/security/vboot/bootmode.c:
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@95 PS1, Line 95: * 3. Checks recovery request in handoff for stages post-cbmem. Not correct anymore.
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@96 PS1, Line 96: For non-CBMEM stages, This too.
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@114 PS1, Line 114: For stages where CBMEM might not be online, This comment needs update too?
https://review.coreboot.org/#/c/33532/1/src/security/vboot/vboot_common.h File src/security/vboot/vboot_common.h:
https://review.coreboot.org/#/c/33532/1/src/security/vboot/vboot_common.h@57 PS1, Line 57: functions Just one function now.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c File src/security/vboot/bootmode.c:
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@95 PS1, Line 95: * 3. Checks recovery request in handoff for stages post-cbmem.
Not correct anymore.
Done
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@96 PS1, Line 96: For non-CBMEM stages,
This too.
Done
https://review.coreboot.org/#/c/33532/1/src/security/vboot/bootmode.c@114 PS1, Line 114: For stages where CBMEM might not be online,
This comment needs update too?
Done
https://review.coreboot.org/#/c/33532/1/src/security/vboot/vboot_common.h File src/security/vboot/vboot_common.h:
https://review.coreboot.org/#/c/33532/1/src/security/vboot/vboot_common.h@57 PS1, Line 57: functions
Just one function now.
Done
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33532
to look at the new patch set (#2).
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
vboot: remove vboot_handoff_get_recovery_reason
Two functions retrieve vboot recovery_reason: * vboot_handoff_get_recovery_reason * vboot_get_recovery_reason_shared_data
Previously, when CBMEM comes online, a vboot_handoff data structure is created, and depending on the architecture, coreboot may eventually lose access to vboot_working_data.
After implementing vboot_working_data CBMEM migration, vboot_working_data is always guaranteed to be accessible.
vboot_get_recovery_reason_shared_data is corrected to also allow accessing vboot_working_data in ramstage and postcar.
Now, vboot_handoff_get_recovery reason returning a valid recovery reason implies that vboot_get_recovery_reason_shared_data should *also* return a valid recovery reason. Thus we may remove the former.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iac216dc968dd155d9d4f8bd0f2dfd5034762f9a0 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/bootmode.c M src/security/vboot/vboot_common.c M src/security/vboot/vboot_common.h 3 files changed, 6 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/33532/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33532/2/src/security/vboot/bootmode.c File src/security/vboot/bootmode.c:
https://review.coreboot.org/#/c/33532/2/src/security/vboot/bootmode.c@95 PS2, Line 95: 4 nit: 3?
Hello Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33532
to look at the new patch set (#3).
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
vboot: remove vboot_handoff_get_recovery_reason
Two functions retrieve vboot recovery_reason: * vboot_handoff_get_recovery_reason * vboot_get_recovery_reason_shared_data
Previously, when CBMEM comes online, a vboot_handoff data structure is created, and depending on the architecture, coreboot may eventually lose access to vboot_working_data.
After implementing vboot_working_data CBMEM migration, vboot_working_data is always guaranteed to be accessible.
vboot_get_recovery_reason_shared_data is corrected to also allow accessing vboot_working_data in ramstage and postcar.
Now, vboot_handoff_get_recovery reason returning a valid recovery reason implies that vboot_get_recovery_reason_shared_data should *also* return a valid recovery reason. Thus we may remove the former.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iac216dc968dd155d9d4f8bd0f2dfd5034762f9a0 Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/bootmode.c M src/security/vboot/vboot_common.c M src/security/vboot/vboot_common.h 3 files changed, 7 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/33532/3
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33532/2/src/security/vboot/bootmode.c File src/security/vboot/bootmode.c:
https://review.coreboot.org/#/c/33532/2/src/security/vboot/bootmode.c@95 PS2, Line 95: 4
nit: 3?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 3: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
Patch Set 3: Code-Review+2
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33532 )
Change subject: vboot: remove vboot_handoff_get_recovery_reason ......................................................................
vboot: remove vboot_handoff_get_recovery_reason
Two functions retrieve vboot recovery_reason: * vboot_handoff_get_recovery_reason * vboot_get_recovery_reason_shared_data
Previously, when CBMEM comes online, a vboot_handoff data structure is created, and depending on the architecture, coreboot may eventually lose access to vboot_working_data.
After implementing vboot_working_data CBMEM migration, vboot_working_data is always guaranteed to be accessible.
vboot_get_recovery_reason_shared_data is corrected to also allow accessing vboot_working_data in ramstage and postcar.
Now, vboot_handoff_get_recovery reason returning a valid recovery reason implies that vboot_get_recovery_reason_shared_data should *also* return a valid recovery reason. Thus we may remove the former.
BUG=b:124141368, b:124192753 TEST=make clean && make test-abuild BRANCH=none
Change-Id: Iac216dc968dd155d9d4f8bd0f2dfd5034762f9a0 Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33532 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/bootmode.c M src/security/vboot/vboot_common.c M src/security/vboot/vboot_common.h 3 files changed, 7 insertions(+), 38 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/bootmode.c b/src/security/vboot/bootmode.c index 68749f0..4d4dc0d 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -26,10 +26,6 @@
static int vboot_get_recovery_reason_shared_data(void) { - /* Shared data does not exist for Ramstage and Post-CAR stage. */ - if (ENV_RAMSTAGE || ENV_POSTCAR) - return 0; - struct vb2_shared_data *sd = vboot_get_shared_data(); assert(sd); return sd->recovery_reason; @@ -96,11 +92,10 @@ * VB2_RECOVERY_RO_MANUAL. * 2. Checks if recovery request is present in VBNV and returns the code read * from it. - * 3. Checks recovery request in handoff for stages post-cbmem. - * 4. For non-CBMEM stages, check if vboot verification is done and look-up - * selected region to identify if vboot_reference library has requested recovery - * path. If yes, return the reason code from shared data. - * 5. If nothing applies, return 0 indicating no recovery request. + * 3. Checks if vboot verification is done and looks up selected region + * to identify if vboot_reference library has requested recovery path. + * 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) { @@ -115,19 +110,8 @@ return reason;
/* - * Check recovery flag in vboot_handoff for stages post CBMEM coming - * online. Since for some stages there is no way to know if cbmem has - * already come online, try looking up handoff anyways. If it fails, - * flow will fallback to looking up shared data. - */ - if (cbmem_possibly_online() && - ((reason = vboot_handoff_get_recovery_reason()) != 0)) - return reason; - - /* - * For stages where CBMEM might not be online, identify if vboot - * verification is already complete and no slot was selected - * i.e. recovery path was requested. + * Identify if vboot verification is already complete and no slot + * was selected i.e. recovery path was requested. */ if (vboot_possibly_executed() && vboot_logic_executed() && !vboot_is_slot_selected()) diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c index ff8e6c8..a18bf23 100644 --- a/src/security/vboot/vboot_common.c +++ b/src/security/vboot/vboot_common.c @@ -93,19 +93,6 @@ return vboot_get_handoff_flag(VB_INIT_OUT_ENABLE_RECOVERY); }
-int vboot_handoff_get_recovery_reason(void) -{ - struct vboot_handoff *vbho; - VbSharedDataHeader *sd; - - if (vboot_get_handoff_info((void **)&vbho, NULL)) - return 0; - - sd = (VbSharedDataHeader *)vbho->shared_data; - - return sd->recovery_reason; -} - /* ============================ VBOOT REBOOT ============================== */ void __weak vboot_platform_prepare_reboot(void) { diff --git a/src/security/vboot/vboot_common.h b/src/security/vboot/vboot_common.h index a785a8b..65a543c 100644 --- a/src/security/vboot/vboot_common.h +++ b/src/security/vboot/vboot_common.h @@ -54,15 +54,13 @@ int vboot_get_handoff_info(void **addr, uint32_t *size);
/* - * The following functions read vboot_handoff structure to obtain requested + * The following function reads vboot_handoff structure to obtain requested * information. If vboot handoff is not available, 0 is returned by default. * If vboot handoff is available: * Returns 1 for flag if true * Returns 0 for flag if false - * Returns value read for other fields */ int vboot_handoff_check_recovery_flag(void); -int vboot_handoff_get_recovery_reason(void);
/* ============================ VBOOT REBOOT ============================== */ /*
-
Furquan Shaikh (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)