Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
security/vboot: Remove flags from struct vboot_working_data
Since now we have persistent context, the usage of the flags can be replaced with vb2_context.flags.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I8e5757a8cc09712c3acde9cbaab910b7498681b4 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/lib/bootmode.c M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 4 files changed, 4 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/36808/1
diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 2465966..06f6d05 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -16,6 +16,7 @@ #include <assert.h> #include <bootmode.h> #include <security/vboot/misc.h> +#include <vb2_api.h>
static int gfx_init_done = -1;
@@ -33,14 +34,13 @@
int display_init_required(void) { - /* For vboot, always honor VBOOT_WD_FLAG_DISPLAY_INIT. */ + /* For vboot, always honor VB2_CONTEXT_DISPLAY_INIT. */ if (CONFIG(VBOOT)) { /* Must always select MUST_REQUEST_DISPLAY when using this function. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) dead_code(); - return vboot_get_working_data()->flags - & VBOOT_WD_FLAG_DISPLAY_INIT; + return vboot_get_context()->flags & VB2_CONTEXT_DISPLAY_INIT; }
/* By default always initialize display. */ diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 0ab0431..1f4a263 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -149,7 +149,7 @@ int vboot_developer_mode_enabled(void) { return cbmem_possibly_online() && - vboot_get_working_data()->flags & VBOOT_WD_FLAG_DEVELOPER_MODE; + vboot_get_context()->flags & VB2_CONTEXT_DEVELOPER_MODE; }
#if CONFIG(VBOOT_NO_BOARD_SUPPORT) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index b45fc9c..455773c 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -37,21 +37,12 @@ */ struct vboot_working_data { struct selected_region selected_region; - uint32_t flags; /* offset of the buffer from the start of this struct */ uint16_t buffer_offset; uint16_t buffer_size; };
/* - * Definitions for vboot_working_data.flags values. - */ -/* vboot requests display initialization from coreboot. */ -#define VBOOT_WD_FLAG_DISPLAY_INIT (1 << 0) -/* vboot has selected developer mode. */ -#define VBOOT_WD_FLAG_DEVELOPER_MODE (1 << 1) - -/* * Source: security/vboot/common.c */ struct vboot_working_data *vboot_get_working_data(void); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 5110147..c4389a9 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -376,14 +376,6 @@ printk(BIOS_INFO, "Phase 1\n"); rv = vb2api_fw_phase1(ctx);
- /* Jot down some information from vboot which may be required later on - in coreboot boot flow. */ - if (ctx->flags & VB2_CONTEXT_DISPLAY_INIT) - /* Mainboard/SoC should initialize display. */ - vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT; - if (ctx->flags & VB2_CONTEXT_DEVELOPER_MODE) - vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE; - if (rv) { /* * If vb2api_fw_phase1 fails, check for return value.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online() If this function is executed after verstage, but before cbmem comes up, I think it will always return 0.
Perhaps we should change it to look something like vboot_check_recovery_request()?
return vboot_possibly_executed() && vboot_logic_executed() && (flags & DEV_MODE);
(Although I'm still not entirely sure why we need vboot_possibly_executed() here.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online()
If this function is executed after verstage, but before cbmem comes up, I think it will always retur […]
vboot_logic_executed() should give a correct answer at all times, it shouldn't need to be predicated by something else. I think we can just get rid of vboot_possibly_executed(), it serves no purpose that vboot_logic_executed() doesn't serve better. (It was probably added for better compile-time elimination before CB:32716 made vboot_logic_executed() capable of that directly.)
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36808
to look at the new patch set (#2).
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
security/vboot: Remove flags from struct vboot_working_data
Since now we have persistent context, the usage of the flags can be replaced with vb2_context.flags.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I8e5757a8cc09712c3acde9cbaab910b7498681b4 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/lib/bootmode.c M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 4 files changed, 5 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/36808/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online()
vboot_logic_executed() should give a correct answer at all times, it shouldn't need to be predicated […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 2: Code-Review+2
https://qa.coreboot.org/job/coreboot-gerrit/108894/ : UNSTABLE
I believe this should go away when you rebase.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
security/vboot: Remove flags from struct vboot_working_data
Since now we have persistent context, the usage of the flags can be replaced with vb2_context.flags.
BRANCH=none BUG=chromium:1021452 TEST=emerge-kukui coreboot
Change-Id: I8e5757a8cc09712c3acde9cbaab910b7498681b4 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36808 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/lib/bootmode.c M src/security/vboot/bootmode.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 4 files changed, 5 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 2465966..06f6d05 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -16,6 +16,7 @@ #include <assert.h> #include <bootmode.h> #include <security/vboot/misc.h> +#include <vb2_api.h>
static int gfx_init_done = -1;
@@ -33,14 +34,13 @@
int display_init_required(void) { - /* For vboot, always honor VBOOT_WD_FLAG_DISPLAY_INIT. */ + /* For vboot, always honor VB2_CONTEXT_DISPLAY_INIT. */ if (CONFIG(VBOOT)) { /* Must always select MUST_REQUEST_DISPLAY when using this function. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) dead_code(); - return vboot_get_working_data()->flags - & VBOOT_WD_FLAG_DISPLAY_INIT; + return vboot_get_context()->flags & VB2_CONTEXT_DISPLAY_INIT; }
/* By default always initialize display. */ diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 0ab0431..bc89e73 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -148,8 +148,8 @@
int vboot_developer_mode_enabled(void) { - return cbmem_possibly_online() && - vboot_get_working_data()->flags & VBOOT_WD_FLAG_DEVELOPER_MODE; + return vboot_logic_executed() && + vboot_get_context()->flags & VB2_CONTEXT_DEVELOPER_MODE; }
#if CONFIG(VBOOT_NO_BOARD_SUPPORT) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index b45fc9c..455773c 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -37,21 +37,12 @@ */ struct vboot_working_data { struct selected_region selected_region; - uint32_t flags; /* offset of the buffer from the start of this struct */ uint16_t buffer_offset; uint16_t buffer_size; };
/* - * Definitions for vboot_working_data.flags values. - */ -/* vboot requests display initialization from coreboot. */ -#define VBOOT_WD_FLAG_DISPLAY_INIT (1 << 0) -/* vboot has selected developer mode. */ -#define VBOOT_WD_FLAG_DEVELOPER_MODE (1 << 1) - -/* * Source: security/vboot/common.c */ struct vboot_working_data *vboot_get_working_data(void); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 5110147..c4389a9 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -376,14 +376,6 @@ printk(BIOS_INFO, "Phase 1\n"); rv = vb2api_fw_phase1(ctx);
- /* Jot down some information from vboot which may be required later on - in coreboot boot flow. */ - if (ctx->flags & VB2_CONTEXT_DISPLAY_INIT) - /* Mainboard/SoC should initialize display. */ - vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT; - if (ctx->flags & VB2_CONTEXT_DEVELOPER_MODE) - vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE; - if (rv) { /* * If vb2api_fw_phase1 fails, check for return value.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online()
Done
CL:36863
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online()
CL:36863
CB:36863 ;-) (CL:36863 points to a cros change from 2012)
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36808 )
Change subject: security/vboot: Remove flags from struct vboot_working_data ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36808/1/src/security/vboot/bootmode... PS1, Line 151: cbmem_possibly_online()
CB:36863 ;-) (CL:36863 points to a cros change from 2012)
Right -- thanks =)
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Yu-Ping Wu (Code Review)