Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
vboot: Unify options to force display init
We used to let implementations for display init that honor display_init_required() explicitly declare that by selecting VBOOT_MUST_REQUEST_DISPLAY. However, to allow the user to force display initialization, it's simpler to invert this Kconfig option.
The new Kconfig VBOOT_FORCE_DISPLAY_INIT has the same effect as ALWAYS_RUN_OPROM for VGA OpRoms. Thus, we can also remove the latter.
The inversion adds a little Kconfig noise. However, this can be reduced in the future by honoring display_init_required() where it should be.
Change-Id: I52288b0d5f33cd11d84e609e039dc4ea16ff7bdf Signed-off-by: Nico Huber nico.h@gmx.de --- M src/device/Kconfig M src/device/pci_device.c M src/drivers/aspeed/ast2050/Kconfig M src/drivers/emulation/qemu/Kconfig M src/drivers/xgi/z9s/Kconfig M src/lib/bootmode.c M src/mainboard/google/daisy/Kconfig M src/mainboard/google/kahlee/Kconfig M src/mainboard/google/oak/Kconfig M src/mainboard/google/peach_pit/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/nehalem/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/sandybridge/Kconfig M src/northbridge/intel/x4x/Kconfig M src/security/vboot/Kconfig M src/security/vboot/vboot_logic.c M src/soc/intel/apollolake/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig M src/soc/mediatek/mt8173/Kconfig M src/soc/mediatek/mt8183/Kconfig M src/soc/nvidia/tegra124/Kconfig M src/soc/nvidia/tegra210/Kconfig M src/soc/qualcomm/sdm845/Kconfig M src/soc/rockchip/rk3288/Kconfig M src/soc/rockchip/rk3399/Kconfig 33 files changed, 51 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/34622/1
diff --git a/src/device/Kconfig b/src/device/Kconfig index e605bc2..d1037da 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -139,13 +139,6 @@ are needed for the kernel's display driver to know how a piece of hardware is configured to be used.
-config ALWAYS_RUN_OPROM - def_bool n - depends on VGA_ROM_RUN && ALWAYS_LOAD_OPROM - help - Always uncondtionally run the option regardless of other - policies. - config ON_DEVICE_ROM_LOAD bool "Load Option ROMs on PCI devices" default n if PAYLOAD_SEABIOS diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 7786043..98ec6fc 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -707,11 +707,6 @@ if (should_run >= 0) return should_run;
- if (CONFIG(ALWAYS_RUN_OPROM)) { - should_run = 1; - return should_run; - } - /* Don't run VGA option ROMs, unless we have to print * something on the screen before the kernel is loaded. */ diff --git a/src/drivers/aspeed/ast2050/Kconfig b/src/drivers/aspeed/ast2050/Kconfig index 337b181..137abd7 100644 --- a/src/drivers/aspeed/ast2050/Kconfig +++ b/src/drivers/aspeed/ast2050/Kconfig @@ -3,3 +3,10 @@ select DRIVERS_ASPEED_AST_COMMON select HAVE_VGA_TEXT_FRAMEBUFFER select MAINBOARD_HAS_NATIVE_VGA_INIT + +if DRIVERS_ASPEED_AST2050 + +config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT + +endif diff --git a/src/drivers/emulation/qemu/Kconfig b/src/drivers/emulation/qemu/Kconfig index 58daaa44..5ab487f 100644 --- a/src/drivers/emulation/qemu/Kconfig +++ b/src/drivers/emulation/qemu/Kconfig @@ -13,14 +13,19 @@ vga (cirrus) is *not* supported, so you have to pick another one explicitly via 'qemu -vga $card'.
+if DRIVERS_EMULATION_QEMU_BOCHS + config DRIVERS_EMULATION_QEMU_BOCHS_XRES int "bochs vga xres" default 800 depends on LINEAR_FRAMEBUFFER - depends on DRIVERS_EMULATION_QEMU_BOCHS
config DRIVERS_EMULATION_QEMU_BOCHS_YRES int "bochs vga yres" default 600 depends on LINEAR_FRAMEBUFFER - depends on DRIVERS_EMULATION_QEMU_BOCHS + +config VBOOT + select VBOOT_FORCE_DISPLAY_INIT + +endif diff --git a/src/drivers/xgi/z9s/Kconfig b/src/drivers/xgi/z9s/Kconfig index b8000c1..98ad1a1 100644 --- a/src/drivers/xgi/z9s/Kconfig +++ b/src/drivers/xgi/z9s/Kconfig @@ -1,3 +1,10 @@ config DRIVERS_XGI_Z9S bool select DRIVERS_XGI_Z79_COMMON + +if DRIVERS_XGI_Z9S + +config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT + +endif diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 2465966..69ac520 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -35,10 +35,6 @@ { /* 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; } diff --git a/src/mainboard/google/daisy/Kconfig b/src/mainboard/google/daisy/Kconfig index 55e0978..b1e8bd8 100644 --- a/src/mainboard/google/daisy/Kconfig +++ b/src/mainboard/google/daisy/Kconfig @@ -32,6 +32,7 @@ select MAINBOARD_HAS_TPM1
config VBOOT + select VBOOT_FORCE_DISPLAY_INIT select VBOOT_VBNV_EC
config MAINBOARD_DIR diff --git a/src/mainboard/google/kahlee/Kconfig b/src/mainboard/google/kahlee/Kconfig index b675f33..7437edc 100644 --- a/src/mainboard/google/kahlee/Kconfig +++ b/src/mainboard/google/kahlee/Kconfig @@ -17,7 +17,6 @@ bool select SOC_AMD_STONEYRIDGE_FT4 select ALWAYS_LOAD_OPROM - select ALWAYS_RUN_OPROM select BOARD_ROMSIZE_KB_16384 select DRIVERS_I2C_GENERIC select DRIVERS_I2C_HID @@ -101,6 +100,7 @@
config VBOOT select EC_GOOGLE_CHROMEEC_SWITCHES + select VBOOT_FORCE_DISPLAY_INIT select VBOOT_LID_SWITCH
config VBOOT_VBNV_OFFSET diff --git a/src/mainboard/google/oak/Kconfig b/src/mainboard/google/oak/Kconfig index c383fa4..99d206c 100644 --- a/src/mainboard/google/oak/Kconfig +++ b/src/mainboard/google/oak/Kconfig @@ -46,6 +46,7 @@
config VBOOT select EC_GOOGLE_CHROMEEC_SWITCHES + select VBOOT_FORCE_DISPLAY_INIT select VBOOT_VBNV_FLASH
config MAINBOARD_DIR diff --git a/src/mainboard/google/peach_pit/Kconfig b/src/mainboard/google/peach_pit/Kconfig index 75f0f0a..6dc414f 100644 --- a/src/mainboard/google/peach_pit/Kconfig +++ b/src/mainboard/google/peach_pit/Kconfig @@ -30,6 +30,7 @@ select MISSING_BOARD_RESET
config VBOOT + select VBOOT_FORCE_DISPLAY_INIT select VBOOT_VBNV_EC
config MAINBOARD_DIR diff --git a/src/northbridge/intel/gm45/Kconfig b/src/northbridge/intel/gm45/Kconfig index c3d2482..d3b7a9b 100644 --- a/src/northbridge/intel/gm45/Kconfig +++ b/src/northbridge/intel/gm45/Kconfig @@ -51,4 +51,7 @@ hex default 0x100000
+config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT + endif diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig index 3678cb8..9937e99 100644 --- a/src/northbridge/intel/haswell/Kconfig +++ b/src/northbridge/intel/haswell/Kconfig @@ -44,7 +44,7 @@ and back to the RW region after the binary is done.
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT select VBOOT_STARTS_IN_ROMSTAGE if !HASWELL_VBOOT_IN_BOOTBLOCK
config VGA_BIOS_ID diff --git a/src/northbridge/intel/i945/Kconfig b/src/northbridge/intel/i945/Kconfig index b151e8f..4a7b2a8 100644 --- a/src/northbridge/intel/i945/Kconfig +++ b/src/northbridge/intel/i945/Kconfig @@ -90,4 +90,7 @@ hex default 0x100000
+config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT + endif diff --git a/src/northbridge/intel/nehalem/Kconfig b/src/northbridge/intel/nehalem/Kconfig index 02b6e80..2b3b2a1 100644 --- a/src/northbridge/intel/nehalem/Kconfig +++ b/src/northbridge/intel/nehalem/Kconfig @@ -54,4 +54,7 @@ hex default 0x10000
+config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT + endif diff --git a/src/northbridge/intel/pineview/Kconfig b/src/northbridge/intel/pineview/Kconfig index 37959dd..bf5cafa 100644 --- a/src/northbridge/intel/pineview/Kconfig +++ b/src/northbridge/intel/pineview/Kconfig @@ -50,4 +50,7 @@ hex default 0x80000
+config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT + endif diff --git a/src/northbridge/intel/sandybridge/Kconfig b/src/northbridge/intel/sandybridge/Kconfig index 53725ca..d7810c8 100644 --- a/src/northbridge/intel/sandybridge/Kconfig +++ b/src/northbridge/intel/sandybridge/Kconfig @@ -26,6 +26,7 @@ if NORTHBRIDGE_INTEL_SANDYBRIDGE
config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT select VBOOT_STARTS_IN_ROMSTAGE
config USE_NATIVE_RAMINIT diff --git a/src/northbridge/intel/x4x/Kconfig b/src/northbridge/intel/x4x/Kconfig index ce43936..703e2133 100644 --- a/src/northbridge/intel/x4x/Kconfig +++ b/src/northbridge/intel/x4x/Kconfig @@ -51,4 +51,7 @@ hex default 0x100000
+config VBOOT + select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_USE_LIBGFXINIT + endif diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ea1f738..7c06ce5 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -154,15 +154,15 @@ reboots caused after vboot verification is run. e.g. reboots caused by FSP components on Intel platforms.
-config VBOOT_MUST_REQUEST_DISPLAY - bool - default y if VGA_ROM_RUN - default n +config VBOOT_FORCE_DISPLAY_INIT + bool "Force display initialization" + default y if !CHROMEOS help - Set this option to indicate to vboot that this platform will skip its - display initialization on a normal (non-recovery, non-developer) boot. - Unless display is specifically requested, the video option ROM is not - loaded, and any other native display initialization code is not run. + Set this option to indicate to vboot that display initialization shall + always be performed, even on a normal (non-recovery, non-developer) + boot. Without this option, unless display is specifically requested, + the video option ROM is not loaded, and any other native display + initialization code is not run.
config VBOOT_HAS_REC_HASH_SPACE bool diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 2468f5f..165facc 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -359,7 +359,7 @@ ctx.flags |= VB2_CONTEXT_NOFAIL_BOOT;
/* Mainboard/SoC always initializes display. */ - if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) + if (CONFIG(VBOOT_FORCE_DISPLAY_INIT)) ctx.flags |= VB2_CONTEXT_DISPLAY_INIT;
/* Do early init (set up secdata and NVRAM, load GBB) */ diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index cf3d244..796d406 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -117,7 +117,6 @@
config VBOOT select VBOOT_SEPARATE_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS diff --git a/src/soc/intel/baytrail/Kconfig b/src/soc/intel/baytrail/Kconfig index 4b816a2..2a576ce 100644 --- a/src/soc/intel/baytrail/Kconfig +++ b/src/soc/intel/baytrail/Kconfig @@ -42,7 +42,6 @@ select CPU_HAS_L2_ENABLE_MSR
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_ROMSTAGE
config BOOTBLOCK_CPU_INIT diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index 7887156..6afaf9b 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -63,7 +63,6 @@ default 0x8000
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_ROMSTAGE
config MMCONF_BASE_ADDRESS diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig index bf6b78c..bfe9519 100644 --- a/src/soc/intel/broadwell/Kconfig +++ b/src/soc/intel/broadwell/Kconfig @@ -81,7 +81,6 @@ and back to the RW region after the binary is done.
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_ROMSTAGE if !BROADWELL_VBOOT_IN_BOOTBLOCK
config MMCONF_BASE_ADDRESS diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index f859cd5..77f589e 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -244,7 +244,6 @@
config VBOOT select VBOOT_SEPARATE_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 99000bb..e43feb1 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -156,7 +156,6 @@
config VBOOT select VBOOT_SEPARATE_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index f36d5ca..e253596 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -108,7 +108,6 @@
config VBOOT select VBOOT_SEPARATE_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_SAVE_RECOVERY_REASON_ON_REBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_VBNV_CMOS diff --git a/src/soc/mediatek/mt8173/Kconfig b/src/soc/mediatek/mt8173/Kconfig index 6476d42..958d41f 100644 --- a/src/soc/mediatek/mt8173/Kconfig +++ b/src/soc/mediatek/mt8173/Kconfig @@ -14,7 +14,6 @@ if SOC_MEDIATEK_MT8173
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE
diff --git a/src/soc/mediatek/mt8183/Kconfig b/src/soc/mediatek/mt8183/Kconfig index c60cdea..dd710a8 100644 --- a/src/soc/mediatek/mt8183/Kconfig +++ b/src/soc/mediatek/mt8183/Kconfig @@ -12,7 +12,6 @@ if SOC_MEDIATEK_MT8183
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE
diff --git a/src/soc/nvidia/tegra124/Kconfig b/src/soc/nvidia/tegra124/Kconfig index c962aea..cc06486 100644 --- a/src/soc/nvidia/tegra124/Kconfig +++ b/src/soc/nvidia/tegra124/Kconfig @@ -16,7 +16,6 @@ if SOC_NVIDIA_TEGRA124
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE
diff --git a/src/soc/nvidia/tegra210/Kconfig b/src/soc/nvidia/tegra210/Kconfig index 0e1efd7..1c99bd6 100644 --- a/src/soc/nvidia/tegra210/Kconfig +++ b/src/soc/nvidia/tegra210/Kconfig @@ -16,7 +16,6 @@ config VBOOT select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY
config MAINBOARD_DO_DSI_INIT bool "Use dsi graphics interface" diff --git a/src/soc/qualcomm/sdm845/Kconfig b/src/soc/qualcomm/sdm845/Kconfig index dbe025e..e2eadbb 100644 --- a/src/soc/qualcomm/sdm845/Kconfig +++ b/src/soc/qualcomm/sdm845/Kconfig @@ -15,7 +15,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK
config SDM845_QSPI diff --git a/src/soc/rockchip/rk3288/Kconfig b/src/soc/rockchip/rk3288/Kconfig index 3aebab9..01c3f95 100644 --- a/src/soc/rockchip/rk3288/Kconfig +++ b/src/soc/rockchip/rk3288/Kconfig @@ -33,7 +33,6 @@ if SOC_ROCKCHIP_RK3288
config VBOOT - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE diff --git a/src/soc/rockchip/rk3399/Kconfig b/src/soc/rockchip/rk3399/Kconfig index 7e3c44b..0424664 100644 --- a/src/soc/rockchip/rk3399/Kconfig +++ b/src/soc/rockchip/rk3399/Kconfig @@ -17,7 +17,6 @@ config VBOOT select VBOOT_SEPARATE_VERSTAGE select VBOOT_RETURN_FROM_VERSTAGE - select VBOOT_MUST_REQUEST_DISPLAY select VBOOT_STARTS_IN_BOOTBLOCK
config PMIC_BUS
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/Kconfig@... PS1, Line 159: default y if !CHROMEOS Should be a separate commit.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
I think this doesn't really do what we want and there's a bit of a misunderstanding what the options are supposed to mean, see my comment on CB:33844. VBOOT_MUST_REQUEST_DISPLAY means "on this platform, the display must be requested explicitly if you want it", or in other words "this platform is capable of skipping the display initialization if it is not needed". It does *not* mean that the display will always be on. (Maybe we can rename it to something that makes that more clear... we picked that name before Intel came up with VBOOT_MAY_SKIP_DISPLAY_INIT, with both of those side-by-side I agree that the distinction isn't very clear.)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
I think this doesn't really do what we want and there's a bit of a misunderstanding what the options are supposed to mean, see my comment on CB:33844. VBOOT_MUST_REQUEST_DISPLAY means "on this platform, the display must be requested explicitly if you want it", or in other words "this platform is capable of skipping the display initialization if it is not needed".
I guess I have to work on the commit message. While the name might be odd (I got used to it already), the effect of VBOOT_MUST_REQUEST_DISPLAY is clear:
if (not "must request display") "report display enabled";
It does *not* mean that the display will always be on. (Maybe we can rename it to something that makes that more clear... we picked that name before Intel came up with VBOOT_MAY_SKIP_DISPLAY_INIT, with both of those side-by-side I agree that the distinction isn't very clear.)
The distinction is even less clear if we'd implement VBOOT_MAY_SKIP_ DISPLAY_INIT in vboot (and not around it), this way:
if (not "may skip display init") "force display enabled";
Because both translate to the same C code:
if (!CONFIG(...)) ctx.flags |= VB2_CONTEXT_DISPLAY_INIT;
And I think VBOOT_MAY_SKIP_DISPLAY_INIT should be implemented this way for roughly the same reasons that VBOOT_MUST_REQUEST_DISPLAY is implemented as it is: If vboot has the means to track the state of the display, we should keep that state tracking sync'ed with what core- boot does. And while we are at it, ALWAYS_RUN_OPROM should also be implemented this way, for the very same reason. While the three options don't do the same, yet, I think they all should simply control VB2_ CONTEXT_DISPLAY_INIT. That's what my change tries to do...
But I guess it goes to far at once. If that helps, I can break it down into smaller commits:
1. Replace VBOOT_MUST_REQUEST_DISPLAY with VBOOT_FORCE_DISPLAY_INIT (selected by the inverse set of gfx init drivers). 2. Give VBOOT_FORCE_DISPLAY_INIT a prompt (replaces VBOOT_MAY_SKIP_ DISPLAY_INIT). 3. Remove ALWAYS_RUN_OPROM.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(3 comments)
Thanks for the explanation, I get what you're trying to do now. I think it should work except for two issues:
1. It removes the safety-check for "did someone forget to set the right option". The problem is that when someone creates a new mainboard, they may not know that they were supposed to select FORCE_DISPLAY_INIT because they don't use display_init_required() anyway. That may lead to unnecessary extra resets in some edge cases that are hard to notice. (Then again, I don't think we're going to make a lot more Chromebooks that init their displays unconditionally in the future, so if you think this makes things cleaner I'm willing to let go of that extra assertion.) 2. It doesn't do the right thing for devices that don't have a display at all. In order to keep the vboot API small, we have the slightly weird requirement that a device which doesn't have a display must pass DISPLAY_INIT in the context of vb2api_fw_phase1() to avoid those extra resets (the current code just does that because they would never select MUST_REQUEST_DISPLAY). Maybe there's some other Kconfig we could check in addition to determine whether there's a display at all? (Maybe HAVE_LINEAR_FRAMEBUFFER? But that option doesn't seem to be super well-maintained because it doesn't really do much... e.g. MT8183/GOOGLE_KUKUI doesn't seem to set it. If we wanted to rely on that it would be good to have an assertion that reminds people to select it from their SoCs... maybe in set_vbe_mode_info_valid()?)
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... File src/mainboard/google/oak/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... PS1, Line 49: select VBOOT_FORCE_DISPLAY_INIT This seems wrong? Oak can skip display init (used to be selected from mt8173/Kconfig). (I think some of the other cases may also be wrong, didn't check them all. In general, unless there's explicit display code at the mainboard level, this option should probably be set at the SoC level.)
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/Kconfig@... PS1, Line 165: initialization code is not run. We should document somewhere that boards/SoCs that do not call display_init_required() need to select this option.
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/34622/1/src/security/vboot/vboot_lo... PS1, Line 361: /* Mainboard/SoC always initializes display. */ nit: comment could use updating (may be user-decided now)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(Maybe HAVE_LINEAR_FRAMEBUFFER? But that option doesn't seem to be super well-maintained because it doesn't really do much... e.g. MT8183/GOOGLE_KUKUI doesn't seem to set it. If we wanted to rely on that it would be good to have an assertion that reminds people to select it from their SoCs... maybe in set_vbe_mode_info_valid()?)
edit: I just noticed the Kukui display patches haven't landed yet, and they'll select that option when they do. So it's probably authoritative anyway (otherwise lb_framebuffer() won't work).
So I think we could solve this by just changing the vboot_logic.c code to
if (CONFIG(VBOOT_FORCE_DISPLAY_INIT) || !CONFIG(LINEAR_FRAMEBUFFER)) ctx.flags |= VB2_CONTEXT_DISPLAY_INIT;
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig@24 PS1, Line 24: config HAVE_VBE_LINEAR_FRAMEBUFFER A bit off-topic, but since I just looked through this again... what's the difference between HAVE_VBE_LINEAR_FRAMEBUFFER and HAVE_LINEAR_FRAMEBUFFER? As far as I can tell it doesn't seem to make any practical difference in behavior? Could we combine those two into one?
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... File src/drivers/aspeed/ast2050/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... PS1, Line 10: select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT Since we want to set the option when display init is just never enabled at all too, I think you don't need the 'if' here.
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... File src/mainboard/google/oak/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... PS1, Line 49: select VBOOT_FORCE_DISPLAY_INIT
This seems wrong? Oak can skip display init (used to be selected from mt8173/Kconfig). […]
edit: Okay, I remember now that Oak's display init starts from the mainboard. So this is probably fine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(3 comments)
- It removes the safety-check ...
I have a more thorough API in mind to check if some gfx init should be run or not. I'll start a discussion on the ML when I have the time. I'd hope that display_init_required() would be harder to miss when it's part of a bigger story (being able to handle multiple active gfx drivers and decide which one should be active etc.).
- It doesn't do the right thing for devices that don't
have a display at all...
Thanks for elaborating. Hidden feature that is easy to miss, indeed. CONFIG_LINEAR_FRAMEBUFFER should be the right thing. Just one question, is it `VB2_CONTEXT_DISPLAY_INIT` that is passed on or `VBOOT_WD_FLAG_DISPLAY_INIT`?
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig@24 PS1, Line 24: config HAVE_VBE_LINEAR_FRAMEBUFFER
A bit off-topic, but since I just looked through this again... […]
Difference is in Kconfig texts presented to the user in the "Framebuffer mode" choice below. In common code, only CONFIG_LINEAR_FRAMEBUFFER should matter.
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... File src/drivers/aspeed/ast2050/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... PS1, Line 10: select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT
Since we want to set the option when display init is just never enabled at all too, I think you don' […]
Welcome to the messy PC world :) That a board has this chip, doesn't mean it's the only one that might have a display attached. Other code that is effective at runtime might still query display_init_ required().
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... File src/mainboard/google/oak/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/mainboard/google/oak/Kc... PS1, Line 49: select VBOOT_FORCE_DISPLAY_INIT
edit: Okay, I remember now that Oak's display init starts from the mainboard. […]
No, you are right. Oak calls display_init_required(), just that the mainboard code does that and the soc Kconfig selects VBOOT_MUST_REQUEST_DISPLAY is confusing.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1:
(2 comments)
- It doesn't do the right thing for devices that don't
have a display at all...
Thanks for elaborating. Hidden feature that is easy to miss, indeed. CONFIG_LINEAR_FRAMEBUFFER should be the right thing. Just one question, is it `VB2_CONTEXT_DISPLAY_INIT` that is passed on or `VBOOT_WD_FLAG_DISPLAY_INIT`?
VB2_CONTEXT_DISPLAY_INIT is an in+out flag to vb2api_fw_phase1() (the code on the other side is here: https://review.coreboot.org/cgit/vboot.git/tree/firmware/2lib/2api.c#n92). If coreboot already sets it, it will definitely be on. If coreboot doesn't set it, vboot may still independently decide to set it anyway. The final value is then memorized by coreboot as VBOOT_WD_FLAG_DISPLAY_INIT and decides whether to actually initialize the display later in ramstage. (It is also memorized internally in vboot and later used in the payload to tell whether display is available or we need another reboot to turn it on.)
Note that we're planning to get rid of the VBOOT_WD_FLAGs again and instead just persist the vb2_context structure across stages. But that shouldn't make a difference for this. All you need to do is to make sure VB2_CONTEXT_DISPLAY_INIT is set before phase1() on all platforms that either always initialize their display unconditionally, or don't have a display to begin with.
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/device/Kconfig@24 PS1, Line 24: config HAVE_VBE_LINEAR_FRAMEBUFFER
Difference is in Kconfig texts presented to the user in the "Framebuffer […]
You mean the only difference is whether CONFIG_VBE_LINEAR_FRAMEBUFFER or CONFIG_GENERIC_LINEAR_FRAMEBUFFER is selected down there? But I don't see the difference between those two either. The only place either of them is ever used is in line 441 in this file, where they're ORed together. Why not merge them into one option?
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... File src/drivers/aspeed/ast2050/Kconfig:
https://review.coreboot.org/c/coreboot/+/34622/1/src/drivers/aspeed/ast2050/... PS1, Line 10: select VBOOT_FORCE_DISPLAY_INIT if MAINBOARD_DO_NATIVE_VGA_INIT
Welcome to the messy PC world :) That a board has this chip, doesn't […]
Okay, makes sense.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Patch Set 1: Code-Review-1
This is flawed, the reasons for ALWAYS_RUN_OPROM aren't what I thought they were.
Nico Huber has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34622 )
Change subject: vboot: Unify options to force display init ......................................................................
Abandoned
-
Julius Werner (Code Review) -
Nico Huber (Code Review)