Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32714
to review the following change.
Change subject: vboot: Make vboot_logic_executed() a bit more precise ......................................................................
vboot: Make vboot_logic_executed() a bit more precise
This patch adds another check to vboot_logic_executed() to make sure we only do a runtime check for verstage_should_load() if CONFIG_VBOOT_RETURN_FROM_VERSTAGE is enabled. That's the only case where the stage that's loading the verstage can execute after verification has run (because the verstage will return to it when it's done). In the other case, the stage that loads verstage really just loads it and will never do anything again after hand-off, so it's guaranteed to always execute before verification.
This change may allow extra dead-code elimination in some cases.
Change-Id: I7019b6f7b0acfbf0a8173914b53364751b08f2cf Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/vboot_loader.c 1 file changed, 5 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32714/1
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 0640ebd..3bbb3da 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -64,9 +64,11 @@
int vboot_logic_executed(void) { - /* If we are in a stage that would load the verstage or execute the - vboot logic directly, we store the answer in a global. */ - if (verstage_should_load() || verification_should_run()) + /* If we are in the stage that runs verification, or in the stage that + both loads the verstage and is returned to from it afterwards, we + need to check a global to see if verfication has run. */ + if (verification_should_run() || + (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) return car_get_var(vboot_executed);
if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) {
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32714 )
Change subject: vboot: Make vboot_logic_executed() a bit more precise ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32714/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32714/1//COMMIT_MSG@18 PS1, Line 18: extra lots! ;)
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32714 )
Change subject: vboot: Make vboot_logic_executed() a bit more precise ......................................................................
vboot: Make vboot_logic_executed() a bit more precise
This patch adds another check to vboot_logic_executed() to make sure we only do a runtime check for verstage_should_load() if CONFIG_VBOOT_RETURN_FROM_VERSTAGE is enabled. That's the only case where the stage that's loading the verstage can execute after verification has run (because the verstage will return to it when it's done). In the other case, the stage that loads verstage really just loads it and will never do anything again after hand-off, so it's guaranteed to always execute before verification.
This change may allow extra dead-code elimination in some cases.
Change-Id: I7019b6f7b0acfbf0a8173914b53364751b08f2cf Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32714 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/security/vboot/vboot_loader.c 1 file changed, 5 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 0640ebd..3bbb3da 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -64,9 +64,11 @@
int vboot_logic_executed(void) { - /* If we are in a stage that would load the verstage or execute the - vboot logic directly, we store the answer in a global. */ - if (verstage_should_load() || verification_should_run()) + /* If we are in the stage that runs verification, or in the stage that + both loads the verstage and is returned to from it afterwards, we + need to check a global to see if verfication has run. */ + if (verification_should_run() || + (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE))) return car_get_var(vboot_executed);
if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) {
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)