Change in ...coreboot[master]: src/security/vboot: Add config option to skip display init
Amol N Sukerkar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33844
Change subject: src/security/vboot: Add config option to skip display init ......................................................................
src/security/vboot: Add config option to skip display init
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode boot for chromeos. When not set, it allows the platform to initialize the display in non-chromeos vboot mode.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 --- M src/security/vboot/Kconfig 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33844/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 66bcc1e..f734194 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -236,6 +236,17 @@ Add a space delimited list of filenames that should only be in the RO section.
+config VBOOT_MAY_SKIP_DISPLAY_INIT + bool "Skip display initialization in normal mode" + default y if CHROMEOS + default n + help + Set this option to indicate that the configuration is for CHROMEOS + and the platform will skip display initialization on a normal (non- + recovery, non-developer) boot. When not set, the platform assumes + a non-CHROMEOS configuration with vboot and allows the display to + initialize on a normal boot. + menu "GBB configuration"
config GBB_HWID
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add config option to skip display init ......................................................................
Patch Set 1: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add config option to skip display init ......................................................................
Patch Set 1:
(3 comments)
LGTM other than some wording cleanup
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@172 PS1, Line 172: config VBOOT_MUST_REQUEST_DISPLAY nit: maybe move it right above this option because they're related to the same thing
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@177 PS1, Line 177: Set this option to indicate to vboot that this platform will skip its Should update this text to clarify this only happens when MAY_SKIP_DISPLAY_INIT is set. (We could also declare a formal 'depends on', although it wouldn't really do anything other than document the intended relationship because this is meant to be selected by mainboards and that overrides depends anyway.)
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@244 PS1, Line 244: CHROMEOS The option isn't really CHROMEOS specific so this text shouldn't mention it -- it makes sense for any platform that has no firmware UI other than for vboot special cases. I'd maybe say something like
Set this option to indicate that coreboot should skip display initialization on a normal (non-recovery, non-developer) boot. This is useful for platforms that don't have firmware UI in normal mode.
Hello Aaron Durbin, Julius Werner, Subrata Banik, Sachin Agrawal, Lean Sheng Tan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33844
to look at the new patch set (#2).
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
src/security/vboot: Add option to skip display init with vboot 2.0
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode boot for chromeos. When not set, it allows the platform to initialize the display in non-chromeos vboot mode.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 --- M src/lib/bootmode.c M src/security/vboot/Kconfig 2 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33844/2
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@172 PS1, Line 172: config VBOOT_MUST_REQUEST_DISPLAY
nit: maybe move it right above this option because they're related to the same thing
Ack
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@177 PS1, Line 177: Set this option to indicate to vboot that this platform will skip its
Should update this text to clarify this only happens when MAY_SKIP_DISPLAY_INIT is set. […]
Ack
https://review.coreboot.org/#/c/33844/1/src/security/vboot/Kconfig@244 PS1, Line 244: CHROMEOS
The option isn't really CHROMEOS specific so this text shouldn't mention it -- it makes sense for an […]
Ack
Hello Aaron Durbin, Julius Werner, Subrata Banik, Sachin Agrawal, Lean Sheng Tan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33844
to look at the new patch set (#3).
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
src/security/vboot: Add option to skip display init with vboot 2.0
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode. This allows platforms that do not implement firmware UI in normal mode to skip the display init in firmware.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 --- M src/lib/bootmode.c M src/security/vboot/Kconfig 2 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33844/3
Hello Aaron Durbin, Julius Werner, Subrata Banik, Sachin Agrawal, Lean Sheng Tan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33844
to look at the new patch set (#4).
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
src/security/vboot: Add option to skip display init with vboot 2.0
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode. This allows platforms that do not implement firmware UI in normal mode to skip the display init in firmware.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 --- M src/lib/bootmode.c M src/security/vboot/Kconfig 2 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33844/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 4:
(1 comment)
LGTM after you fix the typo
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig@191 PS4, Line 191: i ???
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig@191 PS4, Line 191: i
???
Oops! Will fix it.
Hello Aaron Durbin, Julius Werner, Subrata Banik, Sachin Agrawal, Lean Sheng Tan, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33844
to look at the new patch set (#5).
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
src/security/vboot: Add option to skip display init with vboot 2.0
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode. This allows platforms that do not implement firmware UI in normal mode to skip the display init in firmware.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 --- M src/lib/bootmode.c M src/security/vboot/Kconfig 2 files changed, 13 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/33844/5
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/#/c/33844/4/src/security/vboot/Kconfig@191 PS4, Line 191: i
Oops! Will fix it.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 5: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
src/security/vboot: Add option to skip display init with vboot 2.0
This config option, when set, will allow the platform to skip display initialization in normal (non-developer, non-recovery) mode. This allows platforms that do not implement firmware UI in normal mode to skip the display init in firmware.
TEST=Set option CONFIG_VBOOT and clear CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display should initialize in ramstage when platform boots. Set CONFIG_VBOOT and set CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT and the display initialization should be skipped in coreboot.
Signed-off-by: Sukerkar, Amol N amol.n.sukerkar@intel.com Change-Id: Icadad6da34dcb817af02868e89a94ea62dbfa7b3 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33844 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/Kconfig 2 files changed, 13 insertions(+), 2 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 737dcf9..51bbbe5 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2011 The ChromiumOS Authors. All rights reserved. + * Copyright (C) 2019 Intel Corporation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -33,8 +34,7 @@
int display_init_required(void) { - /* For vboot, always honor VBOOT_WD_FLAG_DISPLAY_INIT. */ - if (CONFIG(VBOOT)) { + if (CONFIG(VBOOT_MAY_SKIP_DISPLAY_INIT)) { /* Must always select MUST_REQUEST_DISPLAY when using this function. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ea1f738..fa98935 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -154,10 +154,21 @@ reboots caused after vboot verification is run. e.g. reboots caused by FSP components on Intel platforms.
+config VBOOT_MAY_SKIP_DISPLAY_INIT + bool "Skip display initialization in normal mode" + default y if CHROMEOS + default n + help + Set this option to indicate that coreboot should skip display + initialization on a normal (non-recovery, non-developer) boot. + This is useful for platforms that do not support firmware + user-interface in normal mode. + config VBOOT_MUST_REQUEST_DISPLAY bool default y if VGA_ROM_RUN default n + depends on VBOOT_MAY_SKIP_DISPLAY_INIT help Set this option to indicate to vboot that this platform will skip its display initialization on a normal (non-recovery, non-developer) boot.
Christian Walter has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Beside breaking non-chromeos boards (CB:34308) what is the effect of this? I have to ask because the commit message says roughly the opposite of what the patch does:
* Patch *limits* display-init skipping in `bootmode.c`. * Commit message talks about adding it.
So:
* Why is the Kconfig option added? * Why does it have a prompt? * Why have vendor specific defaults in vboot/? * How does this relate to ALWAYS_RUN_OPROM? is that obsolete?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
I have the feeling that both options, VBOOT_MAY_SKIP_DISPLAY_INIT and VBOOT_MUST_REQUEST_DISPLAY, should be one and named inversely. e.g.
config VBOOT_FORCE_DISPLAY bool "Force display initialization"
which would be selected by all platforms that currently don't select VBOOT_MUST_REQUEST_DISPLAY. This could also obsolete ALWAYS_RUN_OPROM.
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Patch Set 7:
I have the feeling that both options, VBOOT_MAY_SKIP_DISPLAY_INIT and VBOOT_MUST_REQUEST_DISPLAY, should be one and named inversely. e.g.
config VBOOT_FORCE_DISPLAY bool "Force display initialization"
which would be selected by all platforms that currently don't select VBOOT_MUST_REQUEST_DISPLAY. This could also obsolete ALWAYS_RUN_OPROM.
I have added Philipp Deppenwiese (document link: https://docs.google.com/document/d/1MB5MaIc9p9yfbe88Ua7Hnb64ra7Y1BomEI4wYYO9...) to the problem statement and solution discussion I had with some other folks on this thread. Please go through it and let me know if there are questions. I couldn't add you since you don't have Google docs account.
Also, note that once reverted, this will break "all" boards that use vboot but also initialize display in FW as opposed to skipping it. Maybe we can brainstorm about what the fix should look like that will satisfy all use-cases.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
I have the feeling that both options, VBOOT_MAY_SKIP_DISPLAY_INIT and VBOOT_MUST_REQUEST_DISPLAY, should be one and named inversely. e.g.
config VBOOT_FORCE_DISPLAY bool "Force display initialization"
which would be selected by all platforms that currently don't select VBOOT_MUST_REQUEST_DISPLAY. This could also obsolete ALWAYS_RUN_OPROM.
I have added Philipp Deppenwiese (document link: https://docs.google.com/document/d/1MB5MaIc9p9yfbe88Ua7Hnb64ra7Y1BomEI4wYYO9...) to the problem statement and solution discussion I had with some other folks on this thread. Please go through it and let me know if there are questions. I couldn't add you since you don't have Google docs account.
If it's not public, I won't read it.
Also, note that once reverted, this will break "all" boards that use vboot but also initialize display in FW as opposed to skipping it.
Would that be different from before this change? If this change regressed something, it doesn't matter what it fixed...
Maybe we can brainstorm about what the fix should look like that will satisfy all use-cases.
If you could start with what this change was supposed to fix, that would help.
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Patch Set 7:
I have the feeling that both options, VBOOT_MAY_SKIP_DISPLAY_INIT and VBOOT_MUST_REQUEST_DISPLAY, should be one and named inversely. e.g.
config VBOOT_FORCE_DISPLAY bool "Force display initialization"
which would be selected by all platforms that currently don't select VBOOT_MUST_REQUEST_DISPLAY. This could also obsolete ALWAYS_RUN_OPROM.
I have added Philipp Deppenwiese (document link: https://docs.google.com/document/d/1MB5MaIc9p9yfbe88Ua7Hnb64ra7Y1BomEI4wYYO9...) to the problem statement and solution discussion I had with some other folks on this thread. Please go through it and let me know if there are questions. I couldn't add you since you don't have Google docs account.
If it's not public, I won't read it.
Also, note that once reverted, this will break "all" boards that use vboot but also initialize display in FW as opposed to skipping it.
Would that be different from before this change? If this change regressed something, it doesn't matter what it fixed...
Maybe we can brainstorm about what the fix should look like that will satisfy all use-cases.
If you could start with what this change was supposed to fix, that would help.
Sure. Here is the problem statement since my google account does not permit me to open it to the web: ---- Start Problem Statement ---- In the current implementation of vboot 2.0 verification mechanism in coreboot, the display initialization is specific to chromebook. The display initialization is skipped until the control is passed to the payload, DepthCharge. The following code in src/lib/bootmode.c explains the logic,
int display_init_required(void) { /* For vboot, always honor VBOOT_WD_FLAG_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; }
/* By default always initialize display. */ return 1; }
The issue with this logic is as follows. If vboot 2.0 were to be used in use-cases where the requirement is to initialize the display in ramstage (non-chromebook product), the logic above is problematic since even if the config option VBOOT_MUST_REQUEST_DISPLAY is turned off, the code will assert during build and build will fail (due to dead_code() call). Also, it is not immediately clear what the purpose of the call to dead_code is. ---- End Problem Statement ----
The solution that was discussed was: Introducing a new config option VBOOT_MAY_SKIP_DISPLAY_INIT. So, in terms of functionality,
To skip display init in FW - VBOOT_MAY_SKIP_DISPLAY_INIT==y (your use-case) To init display in FW (coreboot) - VBOOT_MAY_SKIP_DISPLAY_INIT==n (our use-case)
Now, as per my knowledge at the time of implementing this solution, the display init was skipped in FW ONLY in case of chromebook. So, the line "default y if CHROMEOS" was added.
From this revert, it appears that it could be skipped in other use-cases as well.
So, could the solution be as simple as adding "CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT=y" to your defconfig? Or is it necessary to revert the commit?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
---- Start Problem Statement ---- In the current implementation of vboot 2.0 verification mechanism in coreboot, the display initialization is specific to chromebook. The display initialization is skipped until the control is passed to the payload, DepthCharge. The following code in src/lib/bootmode.c explains the logic,
AFAIK, this is not true. On Chromebooks, it is decided per boot if the display is initialized by the firmware at all. If not, it's up to the OS to do so (this speeds up regular boots). However, if the display is initialized, the same (ramstage) code is used as usual. So this is not a matter of when the display is initialized but if.
int display_init_required(void) { /* For vboot, always honor VBOOT_WD_FLAG_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; }
/* By default always initialize display. */ return 1;}
The issue with this logic is as follows. If vboot 2.0 were to be used in use-cases where the requirement is to initialize the display in ramstage (non-chromebook product), the logic above is problematic since even if the config option VBOOT_MUST_REQUEST_DISPLAY is turned off, the code will assert during build and build will fail (due to dead_code() call).
I don't see a problem in that logic. Simply, if you need the display pre-OS, VBOOT_WD_FLAG_DISPLAY_INIT should be set. if the condition is "vboot must request display" then let vboot request the display?
Also, it is not immediately clear what the purpose of the call to dead_code is.
It's there as a reminder that display_init_required() should never be called with `VBOOT && !VBOOT_MUST_REQUEST_DISPLAY`.
---- End Problem Statement ----
The solution that was discussed was: Introducing a new config option VBOOT_MAY_SKIP_DISPLAY_INIT. So, in terms of functionality,
Introducing new Kconfig options often turns out wrong. If you discuss such things, why not on the coreboot mailing list? You'd get much more input, I suppose.
To skip display init in FW - VBOOT_MAY_SKIP_DISPLAY_INIT==y (your use-case)
Who is `you` in "your use-case"? I've never used vboot. I merely digest code and make sure that our Kconfig options don't get messier.
To init display in FW (coreboot) - VBOOT_MAY_SKIP_DISPLAY_INIT==n (our use-case)
Rather than adding a Kconfig option that reflects the pro- blem, how about adding a Kconfig option that describes the different vboot interpretations? From what you are telling, I assume your vboot has no developer mode and no recovery mode. Is that correct?
Now, as per my knowledge at the time of implementing this solution, the display init was skipped in FW ONLY in case of chromebook. So, the line "default y if CHROMEOS" was added.
From this revert, it appears that it could be skipped in other use-cases as well.
So, could the solution be as simple as adding "CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT=y" to your defconfig? Or is it necessary to revert the commit?
I don't think that the Kconfig option added here is a good solution. The vboot code (no matter if compiled for chromeos or not) has a feature, it can decide if the display will be initialized in the firmware. With the current implementation here, you work around that feature, settings like VBOOT_MUST_REQUEST_DISPLAY make no sense anymore. IMHO, it would be much better to set the respective flag in vboot, then it's also noted in vboot that the display was initialized.
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT This line is what is causing the breakage btw.
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
---- Start Problem Statement ---- In the current implementation of vboot 2.0 verification mechanism in coreboot, the display initialization is specific to chromebook. The display initialization is skipped until the control is passed to the payload, DepthCharge. The following code in src/lib/bootmode.c explains the logic,
AFAIK, this is not true. On Chromebooks, it is decided per boot if the display is initialized by the firmware at all. If not, it's up to the OS to do so (this speeds up regular boots). However, if the display is initialized, the same (ramstage) code is used as usual. So this is not a matter of when the display is initialized but if.
[ANS] Your statement is true in case of recovery and/or developer mode. However, the display is ALWAYS skipped in non-recovery, non-developer mode. And that is the issue we are trying to resolve here.
int display_init_required(void) { /* For vboot, always honor VBOOT_WD_FLAG_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; }
/* By default always initialize display. */ return 1;}
The issue with this logic is as follows. If vboot 2.0 were to be used in use-cases where the requirement is to initialize the display in ramstage (non-chromebook product), the logic above is problematic since even if the config option VBOOT_MUST_REQUEST_DISPLAY is turned off, the code will assert during build and build will fail (due to dead_code() call).
I don't see a problem in that logic. Simply, if you need the display pre-OS, VBOOT_WD_FLAG_DISPLAY_INIT should be set. if the condition is "vboot must request display" then let vboot request the display?
[ANS] VBOOT_WD_FLAG_DISPLAY_INIT is set only in recovery and/or developer mode. Some rework will be needed in core vboot functions such as vb2api_fw_phase1(), verstage_main() to make sure the flag can be used in non-recovery, non-developer mode. I feel, Introducing a new config option is still a better option and reduces the possibility of unintended artifacts. Also, one issue with this is that if we set VBOOT_MUST_REQUEST_DISPLAY=y, dead_code(), which, btw, is an artifact of CHROMEOS specific vboot flow, will execute and the build will fail.
Also, it is not immediately clear what the purpose of the call to dead_code is.
It's there as a reminder that display_init_required() should never be called with `VBOOT && !VBOOT_MUST_REQUEST_DISPLAY`.
[ANS] Agree. However, it does not consider the case where a platform with vboot enabled and initializes display in FW needs to be able to boot in non-recovery, non-developer mode.
---- End Problem Statement ----
The solution that was discussed was: Introducing a new config option VBOOT_MAY_SKIP_DISPLAY_INIT. So, in terms of functionality,
Introducing new Kconfig options often turns out wrong. If you discuss such things, why not on the coreboot mailing list? You'd get much more input, I suppose.
[ANS] I think you are correct here. Newbee to coreboot open source forum. :-)
To skip display init in FW - VBOOT_MAY_SKIP_DISPLAY_INIT==y (your use-case)
Who is `you` in "your use-case"? I've never used vboot. I merely digest code and make sure that our Kconfig options don't get messier.
[ANS] I meant the affected person who initiated the revert. Sorry for the confusion.
To init display in FW (coreboot) - VBOOT_MAY_SKIP_DISPLAY_INIT==n (our use-case)
Rather than adding a Kconfig option that reflects the pro- blem, how about adding a Kconfig option that describes the different vboot interpretations? From what you are telling, I assume your vboot has no developer mode and no recovery mode. Is that correct?
[ANS] non-recovery, non-developer mode is the normal mode. Recovery/developer mode(s) are used only under specific conditions and, AFAIK, are used only in chromebook.
Now, as per my knowledge at the time of implementing this solution, the display init was skipped in FW ONLY in case of chromebook. So, the line "default y if CHROMEOS" was added.
From this revert, it appears that it could be skipped in other use-cases as well.
So, could the solution be as simple as adding "CONFIG_VBOOT_MAY_SKIP_DISPLAY_INIT=y" to your defconfig? Or is it necessary to revert the commit?
I don't think that the Kconfig option added here is a good solution. The vboot code (no matter if compiled for chromeos or not) has a feature, it can decide if the display will be initialized in the firmware. With the current implementation here, you work around that feature, settings like VBOOT_MUST_REQUEST_DISPLAY make no sense anymore. IMHO, it would be much better to set the respective flag in vboot, then it's also noted in vboot that the display was initialized.
[ANS] Here is the logic from src/security/vboot/vboot_logic.c:
void verstage_main(void) { . . . /* Mainboard/SoC always initializes display. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) ctx.flags |= VB2_CONTEXT_DISPLAY_INIT; . . . if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT) /* Mainboard/SoC should initialize display. */ vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT; . . . }
So, you are correct that using this flag makes sense. However, if we set CONFIG_VBOOT_MUST_REQUEST_DISPLAY=n, we run into dead_code() as mentioned above.
So, I still think introducing a new config option makes more sense. Thoughts?
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT
This line is what is causing the breakage btw.
Ah. As per the discussion between me and Julian Werner, we thought it would be good FYI (similar to documentation) to the user. If removing this line will fix the issue, I am OK with it, since, this line is not supposed to affect any functionality.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT
Ah. […]
That should fix Christian's build issue. But not my third-Kconfig-option-for- the-same-thing issue ;) I'll try to implement my single Kconfig approach later.
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT
That should fix Christian's build issue. But not my third-Kconfig-option-for- […]
I would be really interested if you can come up with a better solution. Hopefully, quickly, since we would like the solution up-streamed for our customers as soon as possible. :-)
So, until then, the commit remains reverted?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT
I would be really interested if you can come up with a better solution. Hopefully, quickly, since we would like the solution up-streamed for our customers as soon as possible. :-)
That's not very encouraging. Unless that customer supports upstream coreboot, of course. I've pushed a first sketch: CB:34622. Feel free to test if it suits your needs.
So, until then, the commit remains reverted?
You are free to push a new version without that dependency line. I wouldn't block it.
Amol N Sukerkar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/33844/7/src/security/vboot/Kconfig@... PS7, Line 171: depends on VBOOT_MAY_SKIP_DISPLAY_INIT
I would be really interested if you can come up with a better solution. […]
Thanks. Let me go through CB:34622 and if that works for the non-chromebook, normal mode with vboot and FW display init, I believe my changes will not be needed.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Hi folks,
First of all, sorry for the breakage. I didn't know that our Kconfig actually has an error-warning for unmet dependencies, I thought it just ignores them. If so many boards are using vboot in different ways now, it would be great to make Jenkins aware of that so we can find these things before patches go in. (I think you just need to submit a config into $(top)/configs?)
The purpose of this change was to keep supporting the different modes that Chrome OS needs as well as the new stuff Intel wants to do with vboot. You're right that the commit message is just wrong, should've read that more closely. Essentially, we're trying to support all the following things:
1. Chrome OS boards want to skip display init when they're in normal (non-dev, non-recovery) mode, for boot speed. 2. Only some Chrome OS boards actually *can* skip display init (those that call the display_init_required() function from SoC/mainboard code and act differently based on the result). For those that cannot, it's beneficial that vboot knows the display is on anyway (that avoids an extra reboot to "turn the display on" in some cases). 3. Intel wants to use vboot but wants the display to be always on, no matter what. 4. Chrome OS and Intel use cases may share the same SoC code (i.e. the part that wants to call display_init_required() and act differently based on the result) and having a CONFIG_CHROMEOS check in there would be ugly.
So we came up with two Kconfigs: 1. VBOOT_MUST_REQUEST_DISPLAY has been there for a long time (used to be called VBOOT_OPROM_MATTERS, which was a somewhat outdated name for the same thing) and essentially means VBOOT_CAN_SKIP_DISPLAY_INIT (I argued with Joel a lot about which name would be clearer for that one, maybe we picked wrong). This means that the mainboard/SoC is technically capable of skipping display init (meaning that it calls display_init_required() somewhere). This one just depends on what the mainboard/SoC code does and is therefore not user-selectable. 2. VBOOT_MAY_SKIP_DISPLAY_INIT is the new one for Intel's use case. That one is supposed to decide whether we actually *want* to skip display init in any case, or just always init unconditionally. I thought that Chrome OS case is sort of a special case (that we know that our payload never needs the display in normal mode), and initializing the display has always been the normal default for coreboot, so I thought that defaulting this to false makes more sense. It's a thing the user needs to decide based on what their payload wants to do, that's why it is user selectable.
We use CONFIG_CHROMEOS here (and in plenty of other cases) to change behavior as a convenience. If we know that we want a Kconfig to be a certain way for all our devices, doing it this way is much easier than having to track this in our custom defconfig for every single board (and having it there doesn't really hurt anyone else, so I hope you guys are okay with that practice). This wasn't intentionally trying to break things for every non-Chrome OS board or anything, of course... just a patch that changes default behavior (for what I believed to be good reasons that should make sense for most), that patch being unfortunately broken in a way none of us noticed, and independent from that Chrome OS preferring the other (now non-standard) behavior. (Note that I did CC Philipp to try to make sure every vboot-using group I'm aware of had a chance to see the change.)
So I think the easiest fix would be to reupload the patch without the 'depends on' line and that should do the right thing everywhere. The alternative would be to switch all mainboard/SoC Kconfigs to control this via default rather than select, or to qualify the select with an 'if VBOOT_MAY_SKIP_DISPLAY_INIT'. But that just adds extra boilerplate to all those files so I think the former solution is probably better.
I'll also take a look at Nico's suggestion.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Patch Set 7:
Hi folks,
First of all, sorry for the breakage. I didn't know that our Kconfig actually has an error-warning for unmet dependencies, I thought it just ignores them. If so many boards are using vboot in different ways now, it would be great to make Jenkins aware of that so we can find these things before patches go in. (I think you just need to submit a config into $(top)/configs?)
Done in CB:34609 for at least one non-chrome os board.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33844 )
Change subject: src/security/vboot: Add option to skip display init with vboot 2.0 ......................................................................
Patch Set 7:
Hi Julius,
no worries. Build breakage is just noise, no harm done.
So I think the easiest fix would be to reupload the patch without the 'depends on' line and that should do the right thing everywhere. The alternative would be to switch all mainboard/SoC Kconfigs to control this via default rather than select, or to qualify the select with an 'if VBOOT_MAY_SKIP_DISPLAY_INIT'. But that just adds extra boilerplate to all those files so I think the former solution is probably better.
I still think this leaves us with three Kconfig options where only one is needed. While they are all implemented in different places, they could all be boiled down to forcing VB2_CONTEXT_DISPLAY_INIT.
I'll also take a look at Nico's suggestion.
I'll continue discussion there.
-
Amol N Sukerkar (Code Review) -
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Lean Sheng Tan (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)