Hello Furquan Shaikh, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39221
to review the following change.
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
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 configuratiions 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.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I31bfc7663724fdacab9955224dcaf650d1ec1c3c --- M src/drivers/intel/fsp1_1/Kconfig M src/drivers/intel/fsp1_1/raminit.c 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 8 files changed, 13 insertions(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/39221/1
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 208ebb5..9e9ef70 100644 --- a/src/drivers/intel/fsp1_1/raminit.c +++ b/src/drivers/intel/fsp1_1/raminit.c @@ -22,7 +22,6 @@ #include <lib.h> /* hexdump */ #include <string.h> #include <timestamp.h> -#include <security/vboot/vboot_common.h>
void raminit(struct romstage_params *params) { @@ -256,11 +255,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/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index d4a4aab..860ba20 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -105,7 +105,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; @@ -519,6 +519,8 @@ const uint32_t invalid = ~MRC_DATA_SIGNATURE;
/* Invalidate only on recovery mode with retraining enabled. */ + if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) + return; if (!vboot_recovery_mode_enabled()) return; if (!vboot_recovery_mode_memory_retrain()) diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 8267833..ab47e5e 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -127,7 +127,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(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 8daa9aa..ff1c580 100644 --- a/src/northbridge/intel/sandybridge/raminit_mrc.c +++ b/src/northbridge/intel/sandybridge/raminit_mrc.c @@ -207,7 +207,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 c21a0c4..deaab51 100644 --- a/src/soc/intel/baytrail/romstage/raminit.c +++ b/src/soc/intel/baytrail/romstage/raminit.c @@ -125,9 +125,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 3759c1f..0683e1c 100644 --- a/src/soc/intel/broadwell/romstage/raminit.c +++ b/src/soc/intel/broadwell/romstage/raminit.c @@ -48,7 +48,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 f13ced9..c7c16b1 100644 --- a/src/southbridge/intel/bd82x6x/me_8.x.c +++ b/src/southbridge/intel/bd82x6x/me_8.x.c @@ -416,38 +416,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) { @@ -696,20 +664,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);