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?