[coreboot-gerrit] Change in coreboot[master]: vboot: Disallow separate verstage after romstage, try to cla...
Julius Werner (Code Review)
gerrit at coreboot.org
Sat Mar 25 02:58:38 CET 2017
Hello Aaron Durbin, Patrick Georgi, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/18983
to review the following change.
Change subject: vboot: Disallow separate verstage after romstage, try to clarify logic
......................................................................
vboot: Disallow separate verstage after romstage, try to clarify logic
No board has ever tried to combine CONFIG_SEPARATE_VERSTAGE with
CONFIG_VBOOT_STARTS_IN_ROMSTAGE. There are probably many reasons why
this wouldn't work (e.g. x86 CAR migration logic currently always
assumes verstage code to run pre-migration). It would also not really
make sense: the reason we use separate verstages is to decrease
bootblock size (mitigating the boot speed cost of slow boot ROM SPI
drivers) and to allow the SRAM-saving RETURN_FROM_VERSTAGE trick,
neither of which would apply to the after-romstage case. It is better to
just forbid that case explicitly and give programmers more guarantees
about what the verstage is (e.g. now the assumption that it runs pre-RAM
is always valid).
Since Kconfig dependencies aren't always guaranteed in the face of
'select' statements, also add some explicit compile-time assertions to
the vboot code. We can simplify some of the loader logic which now no
longer needs to provide for the forbidden case. In addition, also try to
make some of the loader logic more readable by writing it in a more
functional style that allows us to put more assertions about which cases
should be unreachable in there, which will hopefully make it more robust
and fail-fast with future changes (e.g. addition of new stages).
Change-Id: Iaf60040af4eff711d9b80ee0e5950ce05958b3aa
Signed-off-by: Julius Werner <jwerner at chromium.org>
---
M src/include/memlayout.h
M src/vboot/Kconfig
M src/vboot/vboot_loader.c
3 files changed, 50 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/18983/1
diff --git a/src/include/memlayout.h b/src/include/memlayout.h
index a68b21f..9ad3b56 100644
--- a/src/include/memlayout.h
+++ b/src/include/memlayout.h
@@ -158,7 +158,10 @@
#define VERSTAGE(addr, sz) \
REGION(verstage, addr, sz, 1)
- #define OVERLAP_VERSTAGE_ROMSTAGE(addr, size) ROMSTAGE(addr, size)
+ #define OVERLAP_VERSTAGE_ROMSTAGE(addr, size) \
+ _ = ASSERT(IS_ENABLED(CONFIG_RETURN_FROM_VERSTAGE) == 1, \
+ "Must set RETURN_FROM_VERSTAGE to overlap romstage."); \
+ ROMSTAGE(addr, size)
#endif
#if ENV_POSTCAR
diff --git a/src/vboot/Kconfig b/src/vboot/Kconfig
index 0c72d47..a92a2fa 100644
--- a/src/vboot/Kconfig
+++ b/src/vboot/Kconfig
@@ -83,7 +83,13 @@
config SEPARATE_VERSTAGE
bool "Vboot verification is built into a separate stage"
default n
- depends on VBOOT
+ depends on VBOOT && VBOOT_STARTS_IN_BOOTBLOCK
+ help
+ If this option is set, vboot verification runs in a standalone stage
+ that is loaded from the bootblock and exits into romstage. If it is
+ not set, the verification code is linked directly into the bootblock
+ or the romstage and runs as part of that stage (cf. related options
+ VBOOT_STARTS_IN_BOOTBLOCK/_ROMSTAGE and RETURN_FROM_VERSTAGE).
config RETURN_FROM_VERSTAGE
bool "The separate verification stage returns to its caller"
diff --git a/src/vboot/vboot_loader.c b/src/vboot/vboot_loader.c
index 9593358..4011f56 100644
--- a/src/vboot/vboot_loader.c
+++ b/src/vboot/vboot_loader.c
@@ -25,75 +25,69 @@
#include <vboot/symbols.h>
#include <vboot/vboot_common.h>
+/* Ensure vboot configuration is valid: */
+_Static_assert(IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK) +
+ IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) == 1,
+ "vboot must either start in bootblock or romstage (not both!)");
+_Static_assert(!IS_ENABLED(CONFIG_SEPARATE_VERSTAGE) ||
+ IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK),
+ "stand-alone verstage must start in (i.e. after) bootblock");
+_Static_assert(!IS_ENABLED(CONFIG_RETURN_FROM_VERSTAGE) ||
+ IS_ENABLED(CONFIG_SEPARATE_VERSTAGE),
+ "return from verstage only makes sense for separate verstages");
+
/* 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. */
static int verification_should_run(void)
{
- if (ENV_VERSTAGE && IS_ENABLED(CONFIG_SEPARATE_VERSTAGE))
- return 1;
-
- if (!IS_ENABLED(CONFIG_SEPARATE_VERSTAGE)) {
- if (ENV_ROMSTAGE &&
- IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE))
- return 1;
- if (ENV_BOOTBLOCK &&
- IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))
- return 1;
- }
-
- return 0;
+ if (IS_ENABLED(CONFIG_SEPARATE_VERSTAGE))
+ return ENV_VERSTAGE;
+ else if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE))
+ return ENV_ROMSTAGE;
+ else if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))
+ return ENV_BOOTBLOCK;
+ else
+ die("impossible!");
}
static int verstage_should_load(void)
{
- if (!IS_ENABLED(CONFIG_SEPARATE_VERSTAGE))
+ if (IS_ENABLED(CONFIG_SEPARATE_VERSTAGE))
+ return ENV_BOOTBLOCK;
+ else
return 0;
-
- if (ENV_ROMSTAGE && IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE))
- return 1;
-
- if (ENV_BOOTBLOCK && IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))
- return 1;
-
- return 0;
}
static int vboot_executed CAR_GLOBAL;
int vb2_logic_executed(void)
{
- /* If this stage is supposed to run the vboot logic ensure it has been
- * executed. */
- if (verification_should_run() && car_get_var(vboot_executed))
+ /* 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())
+ return car_get_var(vboot_executed);
+
+ if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK)) {
+ /* All other stages are "after the bootblock" */
+ return !ENV_BOOTBLOCK;
+ } else if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)) {
+ /* Post-RAM stages are "after the romstage" */
+#ifdef __PRE_RAM__
+ return 0;
+#else
return 1;
-
- /* If this stage is supposed to load verstage and verstage is returning
- * back to the calling stage check that it has been executed. */
- if (verstage_should_load() && IS_ENABLED(CONFIG_RETURN_FROM_VERSTAGE))
- if (car_get_var(vboot_executed))
- return 1;
-
- /* Handle all other stages post vboot execution. */
- if (!ENV_BOOTBLOCK) {
- if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK))
- return 1;
- if (IS_ENABLED(CONFIG_VBOOT_STARTS_IN_ROMSTAGE) &&
- !ENV_ROMSTAGE)
- return 1;
+#endif
+ } else {
+ die("impossible!");
}
-
- return 0;
}
static void vboot_prepare(void)
{
if (verification_should_run()) {
- /*
- * Note that this path isn't taken when
- * CONFIG_RETURN_FROM_VERSTAGE is employed.
- */
+ /* Note: this path is not used for SEPARATE_VERSTAGE */
verstage_main();
car_set_var(vboot_executed, 1);
vb2_save_recovery_reason_vbnv();
--
To view, visit https://review.coreboot.org/18983
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf60040af4eff711d9b80ee0e5950ce05958b3aa
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
More information about the coreboot-gerrit
mailing list