Change in coreboot[master]: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE

[Joel Kitching (Code Review)]

Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE

Part of the design of vboot persistent context is that the
workbuf gets placed in CBMEM and stays there for depthcharge to
use in kernel verification.  As such, the space allocated in
CBMEM needs to be at least VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

In the VBOOT_STARTS_IN_ROMSTAGE case, prior to this CL,
vboot_get_context() would get invoked for the first time
after CBMEM comes up, and it would only allocate
VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.

Initialize the workbuf directly in vboot_setup_cbmem() instead
with the correct VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

BUG=b:124141368, chromium:994060
TEST=make clean && make test-abuild
TEST=boot on a system with VBOOT_STARTS_IN_ROMSTAGE
BRANCH=none

Change-Id: Ie09c39f960b3f14f3a64c648eee6ca3f23214d9a
Signed-off-by: Joel Kitching 
---
M src/security/vboot/common.c
1 file changed, 12 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/38778/1

diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c
index aeb4498..de3eba9 100644
--- a/src/security/vboot/common.c
+++ b/src/security/vboot/common.c
@@ -86,6 +86,7 @@

static void vboot_setup_cbmem(int unused)
 {
+	vb2_error_t rv;
    const size_t cbmem_size = VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE;
    void *wb_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size);
    assert(wb_cbmem != NULL);
@@ -96,7 +97,16 @@
     * from SRAM/CAR into CBMEM.
     */
    if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
-		assert(vb2api_relocate(wb_cbmem, _vboot2_work, cbmem_size,
-				       &vboot_ctx) == VB2_SUCCESS);
+		rv = vb2api_relocate(wb_cbmem, _vboot2_work, cbmem_size,
+				     &vboot_ctx);
+	/*
+	 * For platforms where VBOOT_STARTS_IN_ROMSTAGE, verification occurs
+	 * after CBMEM is brought online.  Directly initialize vboot data
+	 * structures in CBMEM, which will also be available downstream.
+	 */
+	else
+		rv = vb2api_init(wb_cbmem, cbmem_size, &vboot_ctx);
+
+	assert(rv == VB2_SUCCESS);
 }
 ROMSTAGE_CBMEM_INIT_HOOK(vboot_setup_cbmem)

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38778/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/38778/1/src/security/vboot/common.c... 
PS1, Line 108: 		rv = vb2api_init(wb_cbmem, cbmem_size, &vboot_ctx);
nit: not sure if it's a great idea for visibility to pull the if and the else so far apart from each other. I'd consider either using braces and moving the comments into the respective blocks, or putting the whole comment describing both cases atop the if.

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Raul Rangel,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38778

to look at the new patch set (#2).

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE

Part of the design of vboot persistent context is that the
workbuf gets placed in CBMEM and stays there for depthcharge to
use in kernel verification.  As such, the space allocated in
CBMEM needs to be at least VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

In the VBOOT_STARTS_IN_ROMSTAGE case, prior to this CL,
vboot_get_context() would get invoked for the first time
after CBMEM comes up, and it would only allocate
VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.

Initialize the workbuf directly in vboot_setup_cbmem() instead
with the correct VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

BUG=b:124141368, chromium:994060
TEST=make clean && make test-abuild
TEST=boot on a system with VBOOT_STARTS_IN_ROMSTAGE
BRANCH=none

Change-Id: Ie09c39f960b3f14f3a64c648eee6ca3f23214d9a
Signed-off-by: Joel Kitching 
---
M src/security/vboot/common.c
1 file changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/38778/2

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38778/1/src/security/vboot/common.c 
File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/38778/1/src/security/vboot/common.c... 
PS1, Line 108: 		rv = vb2api_init(wb_cbmem, cbmem_size, &vboot_ctx);
> nit: not sure if it's a great idea for visibility to pull the if and the else so far apart from each […]

Good point, it does look a bit awkward.  I'll merge the comments into one block atop the if.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG@17 
PS2, Line 17: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.
Please reflow for 75 characters per line.

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG@24 
PS2, Line 24: TEST=boot on a system with VBOOT_STARTS_IN_ROMSTAGE
Which system did you use?

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Raul Rangel,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38778

to look at the new patch set (#3).

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE

Part of the design of vboot persistent context is that the workbuf gets
placed in CBMEM and stays there for depthcharge to use in kernel
verification.  As such, the space allocated in CBMEM needs to be at least
VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

In the VBOOT_STARTS_IN_ROMSTAGE case, prior to this CL, vboot_get_context()
would get invoked for the first time after CBMEM comes up, and it would
only allocate VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.

Initialize the workbuf directly in vboot_setup_cbmem() instead with the
correct VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

BUG=b:124141368, chromium:994060
TEST=make clean && make test-abuild
TEST=boot on GOOGLE_EVE with VBOOT_STARTS_IN_ROMSTAGE set
BRANCH=none

Change-Id: Ie09c39f960b3f14f3a64c648eee6ca3f23214d9a
Signed-off-by: Joel Kitching 
---
M src/security/vboot/common.c
1 file changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/38778/3

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG@17 
PS2, Line 17: VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.
> Please reflow for 75 characters per line.

Done

https://review.coreboot.org/c/coreboot/+/38778/2//COMMIT_MSG@24 
PS2, Line 24: TEST=boot on a system with VBOOT_STARTS_IN_ROMSTAGE
> Which system did you use?

Done

[Aaron Durbin (Code Review)]

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

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 3: Code-Review+2

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 3: Code-Review+2

Thanks

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE

Part of the design of vboot persistent context is that the workbuf gets
placed in CBMEM and stays there for depthcharge to use in kernel
verification.  As such, the space allocated in CBMEM needs to be at least
VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

In the VBOOT_STARTS_IN_ROMSTAGE case, prior to this CL, vboot_get_context()
would get invoked for the first time after CBMEM comes up, and it would
only allocate VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE.

Initialize the workbuf directly in vboot_setup_cbmem() instead with the
correct VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE.

BUG=b:124141368, chromium:994060
TEST=make clean && make test-abuild
TEST=boot on GOOGLE_EVE with VBOOT_STARTS_IN_ROMSTAGE set
BRANCH=none

Change-Id: Ie09c39f960b3f14f3a64c648eee6ca3f23214d9a
Signed-off-by: Joel Kitching 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38778
Reviewed-by: Aaron Durbin 
Reviewed-by: Raul Rangel 
Tested-by: build bot (Jenkins) 
---
M src/security/vboot/common.c
1 file changed, 11 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Raul Rangel: Looks good to me, approved

diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c
index aeb4498..ffd9353 100644
--- a/src/security/vboot/common.c
+++ b/src/security/vboot/common.c
@@ -86,6 +86,7 @@

static void vboot_setup_cbmem(int unused)
 {
+	vb2_error_t rv;
    const size_t cbmem_size = VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE;
    void *wb_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size);
    assert(wb_cbmem != NULL);
@@ -94,9 +95,17 @@
     * occurs before CBMEM is brought online, using pre-RAM. In order to
     * make vboot data structures available downstream, copy vboot workbuf
     * from SRAM/CAR into CBMEM.
+	 *
+	 * For platforms where VBOOT_STARTS_IN_ROMSTAGE, verification occurs
+	 * after CBMEM is brought online.  Directly initialize vboot data
+	 * structures in CBMEM, which will also be available downstream.
     */
    if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
-		assert(vb2api_relocate(wb_cbmem, _vboot2_work, cbmem_size,
-				       &vboot_ctx) == VB2_SUCCESS);
+		rv = vb2api_relocate(wb_cbmem, _vboot2_work, cbmem_size,
+				     &vboot_ctx);
+	else
+		rv = vb2api_init(wb_cbmem, cbmem_size, &vboot_ctx);
+
+	assert(rv == VB2_SUCCESS);
 }
 ROMSTAGE_CBMEM_INIT_HOOK(vboot_setup_cbmem)

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38778 )

Change subject: vboot: correct workbuf size when VBOOT_STARTS_IN_ROMSTAGE
......................................................................

Patch Set 4:

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/532
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/531
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/530

Please note: This test is under development and might not be accurate at all!