Change in ...coreboot[master]: vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases
Hello Furquan Shaikh, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32114 to review the following change. Change subject: vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases ...................................................................... vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases This patch enables CONFIG_VBOOT_OPROM_MATTERS in a few more cases where I think(?) it should be. Haswell, Broadwell and Baytrail Chromebooks have this enabled in their old depthcharge firmware branches -- we presumably just forgot to move it over when vboot2 migrated the option to coreboot. Braswell didn't, but it seems like this requirement was added when it was migrated to FSP 1.1...? (Not very sure about that one, but it does call load_vbt() right now which executes things based on display_init_required().) Additionally, it seems to make sense to enable it whenever the user explicitly selects VGA_ROM_RUN in menuconfig (like one of the Intel defconfigs does). Once we have all this, one could take a step back and ask whether this option still makes sense at all anymore. It's enabled for almost all devices (that work with vboot at all), it will presumably be enabled for all future devices, and it seems that most devices that don't enable it use libgfxinit, which as far as I can tell isn't gated on display_init_required() but probably should be. Realistically, whatever kind of display init a board needs to do (native or option ROM), it's probably expensive enough that it's worth skipping on a normal mode vboot boot, and we'd want to have this enabled by default on everything except boards that actually don't have a display. So maybe we should flip it around to CONFIG_VBOOT_OPROM_DOESNT_MATTER, but doing that would probably lead to nobody ever selecting it at all. Not sure what the best solution there is yet, but I think this patch at least moves things in the more correct direction. Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Signed-off-by: Julius Werner <jwerner@chromium.org> --- M src/northbridge/intel/haswell/Kconfig M src/security/vboot/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 5 files changed, 5 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/32114/1 diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig index 0362ffe..082f2d6 100644 --- a/src/northbridge/intel/haswell/Kconfig +++ b/src/northbridge/intel/haswell/Kconfig @@ -26,6 +26,7 @@ if NORTHBRIDGE_INTEL_HASWELL config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_NORTHBRIDGE_INIT diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ca25423..a0cfca5 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -171,6 +171,7 @@ config VBOOT_OPROM_MATTERS bool + default y if VGA_ROM_RUN default n help Set this option to indicate to vboot that this platform will skip its diff --git a/src/soc/intel/baytrail/Kconfig b/src/soc/intel/baytrail/Kconfig index e824ee4..a83a7e9 100644 --- a/src/soc/intel/baytrail/Kconfig +++ b/src/soc/intel/baytrail/Kconfig @@ -43,6 +43,7 @@ select CPU_HAS_L2_ENABLE_MSR config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index fda5a6d..8db4795 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -53,6 +53,7 @@ select SOUTHBRIDGE_INTEL_COMMON_SMBUS config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig index 1a8349d0..5f503da 100644 --- a/src/soc/intel/broadwell/Kconfig +++ b/src/soc/intel/broadwell/Kconfig @@ -65,6 +65,7 @@ default y config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT -- To view, visit https://review.coreboot.org/c/coreboot/+/32114 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Gerrit-Change-Number: 32114 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: newchange
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32114 ) Change subject: vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/32114 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Gerrit-Change-Number: 32114 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 29 Mar 2019 11:59:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32114 ) Change subject: vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/32114 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Gerrit-Change-Number: 32114 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Mon, 01 Apr 2019 07:56:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32114 ) Change subject: vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases ...................................................................... vboot: Select CONFIG_VBOOT_OPROM_MATTERS in more cases This patch enables CONFIG_VBOOT_OPROM_MATTERS in a few more cases where I think(?) it should be. Haswell, Broadwell and Baytrail Chromebooks have this enabled in their old depthcharge firmware branches -- we presumably just forgot to move it over when vboot2 migrated the option to coreboot. Braswell didn't, but it seems like this requirement was added when it was migrated to FSP 1.1...? (Not very sure about that one, but it does call load_vbt() right now which executes things based on display_init_required().) Additionally, it seems to make sense to enable it whenever the user explicitly selects VGA_ROM_RUN in menuconfig (like one of the Intel defconfigs does). Once we have all this, one could take a step back and ask whether this option still makes sense at all anymore. It's enabled for almost all devices (that work with vboot at all), it will presumably be enabled for all future devices, and it seems that most devices that don't enable it use libgfxinit, which as far as I can tell isn't gated on display_init_required() but probably should be. Realistically, whatever kind of display init a board needs to do (native or option ROM), it's probably expensive enough that it's worth skipping on a normal mode vboot boot, and we'd want to have this enabled by default on everything except boards that actually don't have a display. So maybe we should flip it around to CONFIG_VBOOT_OPROM_DOESNT_MATTER, but doing that would probably lead to nobody ever selecting it at all. Not sure what the best solution there is yet, but I think this patch at least moves things in the more correct direction. Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://review.coreboot.org/c/coreboot/+/32114 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Paul Menzel <paulepanter@users.sourceforge.net> Reviewed-by: Patrick Georgi <pgeorgi@google.com> --- M src/northbridge/intel/haswell/Kconfig M src/security/vboot/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig 5 files changed, 5 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig index 0362ffe..082f2d6 100644 --- a/src/northbridge/intel/haswell/Kconfig +++ b/src/northbridge/intel/haswell/Kconfig @@ -26,6 +26,7 @@ if NORTHBRIDGE_INTEL_HASWELL config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_NORTHBRIDGE_INIT diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ca25423..a0cfca5 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -171,6 +171,7 @@ config VBOOT_OPROM_MATTERS bool + default y if VGA_ROM_RUN default n help Set this option to indicate to vboot that this platform will skip its diff --git a/src/soc/intel/baytrail/Kconfig b/src/soc/intel/baytrail/Kconfig index e824ee4..a83a7e9 100644 --- a/src/soc/intel/baytrail/Kconfig +++ b/src/soc/intel/baytrail/Kconfig @@ -43,6 +43,7 @@ select CPU_HAS_L2_ENABLE_MSR config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index fda5a6d..8db4795 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -53,6 +53,7 @@ select SOUTHBRIDGE_INTEL_COMMON_SMBUS config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig index 1a8349d0..5f503da 100644 --- a/src/soc/intel/broadwell/Kconfig +++ b/src/soc/intel/broadwell/Kconfig @@ -65,6 +65,7 @@ default y config VBOOT + select VBOOT_OPROM_MATTERS select VBOOT_STARTS_IN_ROMSTAGE config BOOTBLOCK_CPU_INIT -- To view, visit https://review.coreboot.org/c/coreboot/+/32114 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id96a88296ddb9cfbb58ea67d93e1638d95570e2c Gerrit-Change-Number: 32114 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: merged
participants (3)
-
Julius Werner (Code Review) -
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)