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?