Change in coreboot[master]: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled()
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);
Hello Joel Kitching, Furquan Shaikh, Lee Leahy, Patrick Rudolph, Huang Jin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39221
to look at the new patch set (#2).
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 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.
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/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) These checks seem out of place and seem to be providing new semantics to the modules themselves. i.e. unless STARTS_IN_BOOTBLOCK=true then we don't support the paths that we used to. Isn't that leaking abstractions?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
These checks seem out of place and seem to be providing new semantics to the modules themselves. i. […]
Hmm... yes, maybe. Looks like some platforms used to check for recovery mode and changed their raminit behavior (choosing to always retrain if I understand that correctly) before the actual vboot code even runs. This is now broken and I think it already broke in CB:38780, I'm just trying to make the current behavior more obvious here.
We really want the outside accesses to VBNV gone, which means there is no way to determine whether recovery mode is enabled before running vboot. I really don't want to go back to hacky NVRAM parsing code in coreboot just to support this weird path on some ancient boards. Considering that Chrome OS is (presumably) never going back to STARTS_IN_ROMSTAGE and I don't think anybody else cares much about these details of recovery mode behavior, do you think changing policy like this is acceptable? (I think in practice it just means that these boards will use their MRC cache if available even in recovery mode.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
Hmm... yes, maybe. […]
(edit: Oh, CB:38780 is just the change that broke booting on these boards anyway, so I guess it's not really like it silently slipped in long ago. Still don't really want to revert that. :/ )
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... PS2, Line 16: depends on !VBOOT_STARTS_IN_BOOTBLOCK Is it required adding this dependency?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
(edit: Oh, CB:38780 is just the change that broke booting on these boards anyway, so I guess it's no […]
Ya. I was following along in the other CL where this was brought up. I feel like sprinkling in these checks doesn't help readability and is just working around an implementation problem in vboot_recovery_mode_enabled().
Can you think of another way of creating less-subtle semantics or putting other dependencies in place? I understand why you took this approach, but it doesn't look to be the correct answer.
Way it stands currently: No way to know if recovery mode is enabled until verified boot is ran.
As such all checks to recovery mode enabled prior to that should just return 0? Can we put that into vboot_recovery_mode_enabled()?
if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) && vboot_has_not_ran) return 0;
For vboot_has_not_ran I think we have that semantic lying around to use, no?
I also admit it doesn't fix the implicit semantic changes that have occurred, but it seems cleaner to put this logic in one place instead of throughout the code.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... PS2, Line 16: depends on !VBOOT_STARTS_IN_BOOTBLOCK
Is it required adding this dependency?
There seems to be no FSP1.1 board using STARTS_IN_BOOTBLOCK atm, so I didn't account for that case in the code and adding this here just documents that. I assume there are good reasons for the existing SoCs not offering this, and we won't be adding any new FSP1.1 SoCs (right?), so I thought this should be fine?
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
Ya. I was following along in the other CL where this was brought up. […]
Well, yeah, I could hide it all in vboot_recovery_mode_enabled(). I feel like that's hiding important details and risking mistakes due to incorrect assumptions, though. Writing the 'CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) && ' in front of it makes it clear that this check can only return true when that option is enabled, and leaving the implementation of vboot_recovery_mode_enabled() as it is right now (so it will assert() when called too early) makes sure people will notice very quickly if they're using it wrong. When I make it silently return 0 instead it becomes very unobvious that the MRC cache behavior is different between STARTS_IN_BOOTBLOCK and STARTS_IN_ROMSTAGE platforms, for example.
I can change it to that if you want but I feel like my version is safer and more readable (in the sense of helping people who read it actually understand what happens).
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) What are the semantics of CONFIG(HAS_RECOVERY_MRC_CACHE)?
Couldn't we add a dependency on CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) directly to that Kconfig option, and then disable CONFIG(HAS_RECOVERY_MRC_CACHE) on appropriate boards?
Way it stands currently: No way to know if recovery mode is enabled until verified boot is ran.
Unless I'm missing something -- hasn't this always been the case for non-manual recovery? So any MRC cache code relying on vboot_recovery_mode_enabled() prior to CB:38780 with CONFIG(VBOOT_STARTS_IN_ROMSTAGE) would be inconsistently choosing the recovery cache for manual recovery, and non-recovery cache for BROKEN recovery. This seems worse than just disallowing having two separate caches at all.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) This isn't the same thing, this is CONFIG(MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN). You're right, if I add a dependency on that I can get rid of the check here. But the other checks (above and in other files) would still be staying.
Unless I'm missing something -- hasn't this always been the case for non-manual recovery?
No, because it used to call get_recovery_mode_from_vbnv().
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
*ping*
Looks like I forgot about this patch and maybe reviewers did too. Is this something that we can still find agreement on? I paged out all context on this but I think according to my latest comments I still thought that this is the best option. If not, how else should we resolve the situation? As I understand, booting with vboot still breaks at runtime on a bunch of older boards without this patch right now (or did we have some alternative fix that I forgot about?).
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2:
(2 comments)
Patch Set 2:
*ping*
Looks like I forgot about this patch and maybe reviewers did too. Is this something that we can still find agreement on? I paged out all context on this but I think according to my latest comments I still thought that this is the best option. If not, how else should we resolve the situation? As I understand, booting with vboot still breaks at runtime on a bunch of older boards without this patch right now (or did we have some alternative fix that I forgot about?).
W
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/haswe... PS2, Line 130: if (!(CONFIG(HASWELL_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) Would using VBOOT_STARTS_IN_BOOTBLOCK on all chipset be an unified solution?
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... PS2, Line 210: if (!(CONFIG(SANDYBRIDGE_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) || Would using VBOOT_STARTS_IN_BOOTBLOCK on all chipset be an unified solution?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... PS2, Line 16: depends on !VBOOT_STARTS_IN_BOOTBLOCK
There seems to be no FSP1. […]
AFAIK, the only SoC that uses FSP 1.1 is Braswell, and none of the Braswell boards select VBOOT_STARTS_IN_BOOTBLOCK
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
This isn't the same thing, this is CONFIG(MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN). […]
About this problem: could we please explicitly mention the calls in raminit in the commit message?
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... PS2, Line 210: if (!(CONFIG(SANDYBRIDGE_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) ||
Would using VBOOT_STARTS_IN_BOOTBLOCK on all chipset be an unified solution?
Well, if the symbols already exist, I'd reuse them
https://review.coreboot.org/c/coreboot/+/39221/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/me_8.x.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/southbridge/intel/bd82x... PS2, Line 419: #if CONFIG(CHROMEOS) && 0 /* DISABLED */ I am not sure if this could be useful in the future. I'm leaving a resolved comment here just to remind myself that this patch got rid of it.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 2: Code-Review+1
Patrick Georgi has uploaded a new patch set (#3) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/39221 )
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 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.
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/3
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 3:
PS3 is just a rebase with some barely-above-trivial conflict resolution to make it mergeable again.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... PS2, Line 16: depends on !VBOOT_STARTS_IN_BOOTBLOCK
AFAIK, the only SoC that uses FSP 1.1 is Braswell, and none of the Braswell boards select VBOOT_STARTS_IN_BOOTBLOCK
The CAR setup of FSP1.1 Braswell is not large enough for the vboot2_work buffer, so there is not much to do about that. There is no other FSP1.1 platform supported atm, but vboot was supported in bootblock on FSP1.1 Skylake (deprecated). There does not seem to exist other FSP1.1 platforms besides these two.
So this change does not hurt, but it's a bit weird.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/39221/3/src/northbridge/intel/sandy... PS3, Line 142: SANDYBRIDGE_VBOOT_IN_BOOTBLOCK nit: for consistency over platforms, offering also a VBOOT_STARTS_IN_ROMSTAGE option for legacy reasons, just using VBOOT_STARTS_IN_BOOTBLOCK plainly seems better?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 3: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... File src/drivers/intel/fsp1_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/intel/fsp1_1/Kc... PS2, Line 16: depends on !VBOOT_STARTS_IN_BOOTBLOCK
AFAIK, the only SoC that uses FSP 1. […]
Like I said, this is just supposed to document that we're relying on that fact now. Let me know if you prefer I take it out, I don't care particularly.
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
About this problem: could we please explicitly mention the calls in raminit in the commit message?
So I'm going to add the dependency Joel mentioned so that I can take one of these checks back out. For the other one, I still prefer to write it like this due to the reasons above. Please let me know if this works for everyone or there are still concerns about the general approach.
(I'll also try to put more details into the commit message although I'm not 100% sure what you want me to write, Angel.)
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/haswe... File src/northbridge/intel/haswell/raminit.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/haswe... PS2, Line 130: if (!(CONFIG(HASWELL_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled())
Would using VBOOT_STARTS_IN_BOOTBLOCK on all chipset be an unified solution?
Do you mean using it on all Haswell boards? I guess so... I don't really know why for some of these chipsets some boards start in bootblock and some in romstage. Usually, if start in bootblock is supported for that chipset you'd want to use that for all boards. But I don't know any details about these chipsets so there may be good reasons for this (maybe some boards have larger bootblocks and then it wouldn't fit or something), so I'd rather not try to change that. With what this patch is doing at least I'm pretty confident that it shouldn't break anything.
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/northbridge/intel/sandy... PS2, Line 210: if (!(CONFIG(SANDYBRIDGE_VBOOT_IN_BOOTBLOCK) && vboot_recovery_mode_enabled()) ||
Well, if the symbols already exist, I'd reuse them
Yeah, I could use the generic symbol here as well I just thought this one fits a bit better.
https://review.coreboot.org/c/coreboot/+/39221/3/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/c/coreboot/+/39221/3/src/northbridge/intel/sandy... PS3, Line 142: SANDYBRIDGE_VBOOT_IN_BOOTBLOCK
nit: for consistency over platforms, offering also a VBOOT_STARTS_IN_ROMSTAGE option for legacy reas […]
Seems like there are different opinions about which one to prefer (see Angel's other comment), so I'm not sure what to pick. Personally, I think putting the chipset-specific symbol here helps let the reader know that it exists (e.g. that they shouldn't try to set VBOOT_STARTS_IN_BOOTBLOCK directly, or that they should maybe go read the chipset Kconfig to figure out why a separate symbol was made for this). I'm happy to do it either way though depending on what the majority prefers.
Hello build bot (Jenkins), Joel Kitching, Patrick Georgi, Furquan Shaikh, Frans Hendriks, Lee Leahy, Patrick Rudolph, Angel Pons, Huang Jin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39221
to look at the new patch set (#4).
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 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 --- 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(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/39221/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 4:
*ping*
Anybody want this in? (I don't have any older Intel boards so I don't really care...)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 4: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39221/2/src/drivers/mrc_cache/mrc_c... PS2, Line 522: if (!CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
So I'm going to add the dependency Joel mentioned so that I can take one of these checks back out. […]
Sounds like we're okay with going with the patch as is then.
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
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 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(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
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);
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39221 )
Change subject: vboot: Clean up pre-RAM use of vboot_recovery_mode_enabled() ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3275 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3274 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3273 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3272
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Frans Hendriks (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)