Change in coreboot[master]: security/vboot: Add config option to always enable the display
Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
security/vboot: Add config option to always enable the display
In order to always show the bootlogo very early in coreboot we need the option to always enable the display when VBOOT is enabled.
To do this a config option is added to make sure this functionality can be provided without interfering with systems that require the standard VBOOT display handing.
BUG=N/A TEST=tested on facebook fbg1701.
Change-Id: I3ffaac85d2082717bb9608d536f7cec66a583789 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c 2 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/36547/1
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 7059fde..44f3cb9 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -167,6 +167,12 @@ Unless display is specifically requested, the video option ROM is not loaded, and any other native display initialization code is not run.
+config VBOOT_ALWAYS_ENABLE_DISPLAY + bool "Force display enbled" + default n + help + Set this option to indicate to vboot that display should always be enabled. + config VBOOT_HAS_REC_HASH_SPACE bool default n diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index da6231a..a3a6584 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -369,7 +369,7 @@ ctx.flags |= VB2_CONTEXT_NOFAIL_BOOT;
/* Mainboard/SoC always initializes display. */ - if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) + if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY) || CONFIG(VBOOT_ALWAYS_ENABLE_DISPLAY)) ctx.flags |= VB2_CONTEXT_DISPLAY_INIT;
/* Do early init (set up secdata and NVRAM, load GBB) */
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 171: bool "Force display enbled" enabled
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 171: bool "Force display enbled"
enabled
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 170: config VBOOT_ALWAYS_ENABLE_DISPLAY I think I'm following, but can you please explain why VBOOT_MUST_REQUEST_DISPLAY cannot be modified in how it's currently used. I understand all of our chipsets seem to be selecting VBOOT_MUST_REQUEST_DISPLAY which is preventing you from ensuring the display is requested to be initialized. But doesn't that just make the current setup incorrect for your purposes? And why is adding another Kconfig better? Did you try other approaches?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 1:
(1 comment)
There is also CB:34622. Which, apparently, I forgot about after some vacation. Will try to finish it soon.
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 171: bool "Force display enbled"
Done
Where?
Hello Aaron Durbin, Julius Werner, Frans Hendriks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36547
to look at the new patch set (#2).
Change subject: security/vboot: Add config option to always enable the display ......................................................................
security/vboot: Add config option to always enable the display
In order to always show the bootlogo very early in coreboot we need the option to always enable the display when VBOOT is enabled.
To do this a config option is added to make sure this functionality can be provided without interfering with systems that require the standard VBOOT display handing.
BUG=N/A TEST=tested on facebook fbg1701.
Change-Id: I3ffaac85d2082717bb9608d536f7cec66a583789 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c 2 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/36547/2
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 170: config VBOOT_ALWAYS_ENABLE_DISPLAY
I think I'm following, but can you please explain why VBOOT_MUST_REQUEST_DISPLAY cannot be modified […]
I looked into a couple of options.
First clear the VBOOT_MUST_REQUEST_DISPLAY. This caused a hang in the display_init_required(). As far as I could see from the file history this is the desired behavior. So overriding it would require an additional config.
Second keeping the standard mechanism as is and control the display by populating the NV data with the display enabled. This would require quite some changes to the vboot code. So I didn't think it would work out as a valid path to get to a solution.
So this is why I ended up adding the additional Kconfig for this purpose. I have the impression that this indicates clear enough that by enabling the option the default VBOOT behavior is modified.
If you have a better suggestion this is of course welcome.
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 171: bool "Force display enbled"
Where?
I am sorry, forgot to publish the edit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 170: config VBOOT_ALWAYS_ENABLE_DISPLAY
I looked into a couple of options.
First clear the VBOOT_MUST_REQUEST_DISPLAY. This caused a hang in the display_init_required(). As far as I could see from the file history this is the desired behavior. So overriding it would require an additional config.
What was causing the hang? It looks to me that dead_code() would only trigger a linker error.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 170: config VBOOT_ALWAYS_ENABLE_DISPLAY
I looked into a couple of options. […]
You are right, I recalled incorrectly. It indeed triggers a linker error AND I have to override the select in the soc/intel/braswell (in this case) Kconfig to achieve this functionality.
So basically this doesn't help avoiding an additional config item.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/1/src/security/vboot/Kconfig@... PS1, Line 170: config VBOOT_ALWAYS_ENABLE_DISPLAY
You are right, I recalled incorrectly. […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/2/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/2/src/security/vboot/Kconfig@... PS2, Line 171: bool "Force display enabled"
Always enable display
Force to always enable display
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 2:
Nico, what are your thoughts on this CL vs your CL?
Hello Aaron Durbin, Julius Werner, Frans Hendriks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36547
to look at the new patch set (#3).
Change subject: security/vboot: Add config option to always enable the display ......................................................................
security/vboot: Add config option to always enable the display
In order to always show the bootlogo very early in coreboot we need the option to always enable the display when VBOOT is enabled.
To do this a config option is added to make sure this functionality can be provided without interfering with systems that require the standard VBOOT display handing.
BUG=N/A TEST=tested on facebook fbg1701.
Change-Id: I3ffaac85d2082717bb9608d536f7cec66a583789 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c 2 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/36547/3
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36547/2/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36547/2/src/security/vboot/Kconfig@... PS2, Line 171: bool "Force display enabled"
Always enable display […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 3: Code-Review+2
Hello Aaron Durbin, Julius Werner, Frans Hendriks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36547
to look at the new patch set (#6).
Change subject: security/vboot: Add config option to always enable the display ......................................................................
security/vboot: Add config option to always enable the display
In order to always show the bootlogo very early in coreboot we need the option to always enable the display when VBOOT is enabled.
To do this a config option is added to make sure this functionality can be provided without interfering with systems that require the standard VBOOT display handing.
BUG=N/A TEST=tested on facebook fbg1701.
Change-Id: I3ffaac85d2082717bb9608d536f7cec66a583789 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c 2 files changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/36547/6
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 6: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 6: Code-Review+1
Nico, what are your thoughts on this CL vs your CL?
I can just work on top of this. The problem I see is that with this additional Kconfig, we have, on some paths (e.g. VBOOT + VGA OpRom), three Kconfigs controlling the very same thing. But my approach has maintainability issues as well. Needs some more thought.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
Patch Set 6: Code-Review+2
Nico, what are your thoughts on this CL vs your CL?
I can just work on top of this. The problem I see is that with this additional Kconfig, we have, on some paths (e.g. VBOOT + VGA OpRom), three Kconfigs controlling the very same thing. But my approach has maintainability issues as well. Needs some more thought.
Even worse, it seems I misinterpreted ALWAYS_RUN_OPROM back then (IIRC, because people started using it in the wrong context). So there are actually only the two options MUST_REQUEST_DISPLAY and ALWAYS_ENABLE_DISPLAY to consider, and squeezing them into one as I tried, makes it harder to check for wrong selections (the dead_code() in `lib/bootmode.c`). Sorry for the noise.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36547 )
Change subject: security/vboot: Add config option to always enable the display ......................................................................
security/vboot: Add config option to always enable the display
In order to always show the bootlogo very early in coreboot we need the option to always enable the display when VBOOT is enabled.
To do this a config option is added to make sure this functionality can be provided without interfering with systems that require the standard VBOOT display handing.
BUG=N/A TEST=tested on facebook fbg1701.
Change-Id: I3ffaac85d2082717bb9608d536f7cec66a583789 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36547 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c 2 files changed, 7 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Frans Hendriks: Looks good to me, approved
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 70180c7..89e1232 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -167,6 +167,12 @@ Unless display is specifically requested, the video option ROM is not loaded, and any other native display initialization code is not run.
+config VBOOT_ALWAYS_ENABLE_DISPLAY + bool "Force to always enable display" + default n + help + Set this option to indicate to vboot that display should always be enabled. + config VBOOT_HAS_REC_HASH_SPACE bool default n diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index c4389a9..5facd28 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -369,7 +369,7 @@ ctx->flags |= VB2_CONTEXT_NOFAIL_BOOT;
/* Mainboard/SoC always initializes display. */ - if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) + if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY) || CONFIG(VBOOT_ALWAYS_ENABLE_DISPLAY)) ctx->flags |= VB2_CONTEXT_DISPLAY_INIT;
/* Do early init (set up secdata and NVRAM, load GBB) */
-
Aaron Durbin (Code Review) -
Frans Hendriks (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Wim Vervoorn (Code Review)