Julius Werner submitted this change.

View Change

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled()

vboot_recovery_mode_enabled() was recently changed to assert() when it
is called before vboot logic has run, because we cannot determine
whether we're going to be in recovery mode at that point and we wanted
to flush out existing uses that pretended that we could. Turns out there
are a bunch of uses like that, and there is some code that is shared
across configurations that can and those that can't.

This patch cleans them up to either remove checks that cannot return
true, or add explicit Kconfig guards to clarify that the code is shared.
This means that using a separate recovery MRC cache is no longer
supported on boards that use VBOOT_STARTS_IN_ROMSTAGE (this has already
been broken with CB:38780, but with this patch those boards will boot
again using their normal MRC caches rather than just die). Skipping the
MRC cache and always regenerating from scratch in recovery mode is
likewise no longer supported for VBOOT_STARTS_IN_ROMSTAGE.

For FSP1.1 boards, none of them support VBOOT_STARTS_IN_BOOTBLOCK and
that is unlikely to change in the future so we will just hardcode that
fact in Kconfig (otherwise, fsp1.1 raminit would also have to be fixed
to work around this issue).

Signed-off-by: Julius Werner <jwerner@chromium.org>
Change-Id: I31bfc7663724fdacab9955224dcaf650d1ec1c3c
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39221
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>
---
M src/drivers/intel/fsp1_1/Kconfig
M src/drivers/intel/fsp1_1/raminit.c
M src/drivers/mrc_cache/Kconfig
M src/drivers/mrc_cache/mrc_cache.c
M src/northbridge/intel/haswell/raminit.c
M src/northbridge/intel/sandybridge/raminit_mrc.c
M src/soc/intel/baytrail/romstage/raminit.c
M src/soc/intel/broadwell/romstage/raminit.c
M src/southbridge/intel/bd82x6x/me_8.x.c
9 files changed, 12 insertions(+), 57 deletions(-)

diff --git a/src/drivers/intel/fsp1_1/Kconfig b/src/drivers/intel/fsp1_1/Kconfig
index 7c69888..304cbae 100644
--- a/src/drivers/intel/fsp1_1/Kconfig
+++ b/src/drivers/intel/fsp1_1/Kconfig
@@ -13,6 +13,7 @@

config PLATFORM_USES_FSP1_1
bool
+ depends on !VBOOT_STARTS_IN_BOOTBLOCK
select UEFI_2_4_BINDING
select INTEL_GMA_ADD_VBT if RUN_FSP_GOP
select MICROCODE_UPDATE_PRE_RAM
diff --git a/src/drivers/intel/fsp1_1/raminit.c b/src/drivers/intel/fsp1_1/raminit.c
index a090a1f..edae755 100644
--- a/src/drivers/intel/fsp1_1/raminit.c
+++ b/src/drivers/intel/fsp1_1/raminit.c
@@ -12,7 +12,6 @@
#include <lib.h> /* hexdump */
#include <string.h>
#include <timestamp.h>
-#include <security/vboot/vboot_common.h>

void raminit(struct romstage_params *params)
{
@@ -246,11 +245,10 @@

/* Locate the memory configuration data to speed up the next reboot */
mrc_hob = get_next_guid_hob(&mrc_guid, hob_list_ptr);
- if (mrc_hob == NULL)
+ if (mrc_hob == NULL) {
printk(BIOS_DEBUG,
"Memory Configuration Data Hob not present\n");
- else if (!vboot_recovery_mode_enabled()) {
- /* Do not save MRC data in recovery path */
+ } else {
params->data_to_save = GET_GUID_HOB_DATA(mrc_hob);
params->data_to_save_size = ALIGN_UP(
((u32)GET_HOB_LENGTH(mrc_hob)), 16);
diff --git a/src/drivers/mrc_cache/Kconfig b/src/drivers/mrc_cache/Kconfig
index 543f310..79cc205 100644
--- a/src/drivers/mrc_cache/Kconfig
+++ b/src/drivers/mrc_cache/Kconfig
@@ -19,6 +19,7 @@

config MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN
bool
+ depends on VBOOT_STARTS_IN_BOOTBLOCK
default n

config MRC_SETTINGS_VARIABLE_DATA
diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c
index 2b63129..d7fd328 100644
--- a/src/drivers/mrc_cache/mrc_cache.c
+++ b/src/drivers/mrc_cache/mrc_cache.c
@@ -95,7 +95,7 @@
int i;
int flags;

- if (vboot_recovery_mode_enabled())
+ if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && vboot_recovery_mode_enabled())
flags = RECOVERY_FLAG;
else
flags = NORMAL_FLAG;
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c
index 9fff58e..ddb2f83 100644
--- a/src/northbridge/intel/haswell/raminit.c
+++ b/src/northbridge/intel/haswell/raminit.c
@@ -112,7 +112,8 @@
printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");

/* Do not pass MRC data in for recovery mode boot, always pass it in for S3 resume */
- if (!vboot_recovery_mode_enabled() || pei_data->boot_mode == 2)
+ if (!(CONFIG(HASWELL_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled())
+ || pei_data->boot_mode == 2)
prepare_mrc_cache(pei_data);

/* If MRC data is not found, we cannot continue S3 resume */
diff --git a/src/northbridge/intel/sandybridge/raminit_mrc.c b/src/northbridge/intel/sandybridge/raminit_mrc.c
index ae95efa8..2178c9d 100644
--- a/src/northbridge/intel/sandybridge/raminit_mrc.c
+++ b/src/northbridge/intel/sandybridge/raminit_mrc.c
@@ -139,7 +139,8 @@
* Do not pass MRC data in for recovery mode boot,
* Always pass it in for S3 resume.
*/
- if (!vboot_recovery_mode_enabled() || pei_data->boot_mode == 2)
+ if (!(CONFIG(SANDYBRIDGE_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) ||
+ pei_data->boot_mode == 2)
prepare_mrc_cache(pei_data);

/* If MRC data is not found we cannot continue S3 resume. */
diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c
index 68b8c0e..88b61de 100644
--- a/src/soc/intel/baytrail/romstage/raminit.c
+++ b/src/soc/intel/baytrail/romstage/raminit.c
@@ -132,9 +132,7 @@
if (!mp->io_hole_mb)
mp->io_hole_mb = 2048;

- if (vboot_recovery_mode_enabled()) {
- printk(BIOS_DEBUG, "Recovery mode: not using MRC cache.\n");
- } else if (!mrc_cache_get_current(MRC_TRAINING_DATA, 0, &rdev)) {
+ if (!mrc_cache_get_current(MRC_TRAINING_DATA, 0, &rdev)) {
mp->saved_data_size = region_device_sz(&rdev);
mp->saved_data = rdev_mmap_full(&rdev);
/* Assume boot device is memory mapped. */
diff --git a/src/soc/intel/broadwell/romstage/raminit.c b/src/soc/intel/broadwell/romstage/raminit.c
index 78081fe..c593908 100644
--- a/src/soc/intel/broadwell/romstage/raminit.c
+++ b/src/soc/intel/broadwell/romstage/raminit.c
@@ -36,7 +36,8 @@

broadwell_fill_pei_data(pei_data);

- if (vboot_recovery_mode_enabled()) {
+ if (CONFIG(BROADWELL_VBOOT_IN_BOOTBLOCK) &&
+ vboot_recovery_mode_enabled()) {
/* Recovery mode does not use MRC cache */
printk(BIOS_DEBUG, "Recovery mode: not using MRC cache.\n");
} else if (!mrc_cache_get_current(MRC_TRAINING_DATA, 0, &rdev)) {
diff --git a/src/southbridge/intel/bd82x6x/me_8.x.c b/src/southbridge/intel/bd82x6x/me_8.x.c
index d240bf6..b529fe2 100644
--- a/src/southbridge/intel/bd82x6x/me_8.x.c
+++ b/src/southbridge/intel/bd82x6x/me_8.x.c
@@ -403,38 +403,6 @@
print_cap("Wireless LAN (WLAN)", cap->wlan);
}

-#if CONFIG(CHROMEOS) && 0 /* DISABLED */
-/* Tell ME to issue a global reset */
-static int mkhi_global_reset(void)
-{
- struct me_global_reset reset = {
- .request_origin = GLOBAL_RESET_BIOS_POST,
- .reset_type = CBM_RR_GLOBAL_RESET,
- };
- struct mkhi_header mkhi = {
- .group_id = MKHI_GROUP_ID_CBM,
- .command = MKHI_GLOBAL_RESET,
- };
- struct mei_header mei = {
- .is_complete = 1,
- .length = sizeof(mkhi) + sizeof(reset),
- .host_address = MEI_HOST_ADDRESS,
- .client_address = MEI_ADDRESS_MKHI,
- };
-
- /* Send request and wait for response */
- printk(BIOS_NOTICE, "ME: %s\n", __FUNCTION__);
- if (mei_sendrecv(&mei, &mkhi, &reset, NULL, 0) < 0) {
- /* No response means reset will happen shortly... */
- halt();
- }
-
- /* If the ME responded it rejected the reset request */
- printk(BIOS_ERR, "ME: Global Reset failed\n");
- return -1;
-}
-#endif
-
/* Send END OF POST message to the ME */
static int __unused mkhi_end_of_post(void)
{
@@ -683,20 +651,6 @@
if (intel_me_read_mbp(&mbp_data))
break;

-#if CONFIG(CHROMEOS) && 0 /* DISABLED */
- /*
- * Unlock ME in recovery mode.
- */
- if (vboot_recovery_mode_enabled()) {
- /* Unlock ME flash region */
- mkhi_hmrfpo_enable();
-
- /* Issue global reset */
- mkhi_global_reset();
- return;
- }
-#endif
-
if (CONFIG_DEFAULT_CONSOLE_LOGLEVEL >= BIOS_DEBUG) {
me_print_fw_version(&mbp_data.fw_version_name);
me_print_fwcaps(&mbp_data.fw_caps_sku);

To view, visit change 39221. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I31bfc7663724fdacab9955224dcaf650d1ec1c3c
Gerrit-Change-Number: 39221
Gerrit-PatchSet: 5
Gerrit-Owner: Julius Werner <jwerner@chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks@eltan.com>
Gerrit-Reviewer: Furquan Shaikh <furquan@google.com>
Gerrit-Reviewer: Huang Jin <huang.jin@intel.com>
Gerrit-Reviewer: Joel Kitching <kitching@google.com>
Gerrit-Reviewer: Julius Werner <jwerner@chromium.org>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy@intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph@9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Arthur Heymans <arthur@aheymans.xyz>
Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net>
Gerrit-MessageType: merged