Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39340 )
Change subject: chromeos: remove get_write_protect_state function ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39340/3/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/39340/3/src/drivers/mrc_cache/mrc_c... PS3, Line 438: /* Read Write Protect GPIO if available */ : wp_gpio = get_write_protect_state();
Hmmm... +Hung-Te for updater expertise. […]
We do see cases during dogfooding where SPI flash layout changes and so the entire SPI flash has to be re-written. In such cases, MRC cache could have actually moved to a completely different offset within the SPI flash. So, protecting the original MRC region of SPI flash using a PRR is going to prevent updates to the region which could be belonging to something else in the new layout.
There are couple of options if we want to get rid of this extra piece of coupling in every single board:
Option 1: We can potentially make a case that during dogfooding we should not really see SW WP being enabled. However, I think we should align the expectations in the firmware updater as well. i.e. if HW or SW WP is enabled, skip writing to entire SPI flash and instead just write to RW-A/B regions. The assumption here would be:
a) If SW WP is enabled, then there are no changes expected in RO as well as the SPI flash layout. b) Thus, it is okay for coreboot to lock down MRC region using PRR. c) And it is okay for firmware updater to just update RW-A/B region. d) If there are changes required to RO and/or flash layout, then SW WP should be disabled as well.
Option 2: Looking at this code, I realized that we never really exercise the PRR path in dogfooding until we FSI. This is because we do not enable SW WP on development builds. We can completely decouple PRR protection from WP and instead use a separate config CONFIG_MRC_CACHE_LOCKDOWN that boards can set once they are past early development phase. In this way they can always control when to set the config and also have knowledge about any changes that might happen in SPI flash layout. Obviously, there can be some problems here: a) If SPI flash layout needs to change after the lock down config is enabled, then there will have to be a special dance of disabling config -> layout change -> enabling config. b) Ensuring that the config is always set before FSI is critical. Appropriate tests will have to be added to ensure that it doesn't get missed.
+Duncan in case there are more cases I missed.