Change in coreboot[master]: cbfs: Do not use vboot_locator in bootblock with separate verstage

[Hung-Te Lin (Code Review)]

Hung-Te Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37838 )

Change subject: cbfs: Do not use vboot_locator in bootblock with separate verstage
......................................................................

cbfs: Do not use vboot_locator in bootblock with separate verstage

When separate verstage is available, all the vboot logic is done in
verstage and bootblock should load verstage directly. This helps
minimizing bootblock size on devices with limited storage.

For example, on Kodama, the bootblock is reduced from 21504 to
20480 bytes (-4%), where the system can only allow 20992 bytes.

BUG=b:146542160
TEST=emerge-kukui coreboot chromeos-ec chromeos-bootimage

Change-Id: Id17cd151b4426231704b1c9845348763126a7870
Signed-off-by: Hung-Te Lin 
---
M src/lib/cbfs.c
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/37838/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 636ff70..e3c4692 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -302,11 +302,13 @@
 extern const struct cbfs_locator vboot_locator;

static const struct cbfs_locator *locators[] = {
-#if CONFIG(VBOOT)
+#if CONFIG(VBOOT) && !(ENV_BOOTBLOCK && CONFIG(VBOOT_SEPARATE_VERSTAGE))
    /*
     * NOTE: Does not link in SMM, as the vboot_locator isn't compiled.
     * ATM there's no need for VBOOT functionality in SMM and it's not
     * a problem.
+	 * Also don't use vboot_locator in bootblock when separate verstage is
+	 * available, to help reducing bootblock size.
     */
    &vboot_locator,
 #endif

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37838 )

Change subject: cbfs: Do not use vboot_locator in bootblock with separate verstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37838/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/37838/1/src/lib/cbfs.c@305 
PS1, Line 305: #if CONFIG(VBOOT) && !(ENV_BOOTBLOCK && CONFIG(VBOOT_SEPARATE_VERSTAGE))
If you are using VBOOT_RETURN_FROM_VERSTAGE then this change is incorrect as you need to steer correctly through vboot_locator for loading romstage. It might make more sense to have a quick bail out in vboot_locate() for the condition you care about. It won't save the full 1KiB, but it should basically be the size of the cbfs_locator object plus a a few instructions.

What does the following look like?

diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c
index b72c82ba4a..477995ec00 100644
--- a/src/security/vboot/vboot_loader.c
+++ b/src/security/vboot/vboot_loader.c
@@ -74,6 +74,11 @@ static int vboot_locate(struct region_device *rdev)
 {
        struct vb2_context *ctx;

+       /* Don't carry the steering logic when verstage is separate. Bootblock
+        * will just chainload verstage and never return back to bootblock.  */
+       if (verstage_should_load() && !CONFIG(VBOOT_RETURN_FROM_VERSTAGE))
+               return -1;
+
        /* Don't honor vboot results until the vboot logic has run. */
        if (!vboot_logic_executed())
                return -1;

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37838 )

Change subject: cbfs: Do not use vboot_locator in bootblock with separate verstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37838/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/37838/1/src/lib/cbfs.c@305 
PS1, Line 305: #if CONFIG(VBOOT) && !(ENV_BOOTBLOCK && CONFIG(VBOOT_SEPARATE_VERSTAGE))
> If you are using VBOOT_RETURN_FROM_VERSTAGE then this change is incorrect as you need to steer corre […]

Thanks for the feedback. But just realized I also need to enable VBOOT_RETURN_FROM_VERSTAGE so this actually won't help my problem...
I'll abandon this one and think about other approaches to reduce size.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37838 )

Change subject: cbfs: Do not use vboot_locator in bootblock with separate verstage
......................................................................

Abandoned

This won't help for devices using overlapped verstage/romstage.