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.