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).