Attention is currently required from: Nico Huber, Sridhar Siricilla, Patrick Rudolph. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61380 )
Change subject: soc/inte/common: Add support to control coreboot and Intel SoC features ......................................................................
Patch Set 2:
(2 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/bd49633b_8fec46d4 PS2, Line 666: is_cse_fw_update_enabled
is there any restriction that on after FSI, we can't read the softstrap ?
There is no point enabling the SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE since we don't want override native coreboot/SoC functionality/features (for example CSE FW update get triggered through debug flag) post FSI and Descriptor Region is Read-only. The intention of SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE for debug purposes. BTW,SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE doesn't use soft strap area. It uses unused area (OEM section) as scratch pad.
So, if I read it correctly there is no restriction while reading OEM sections prior or post FSI. hence, why we would like to introduce a Kconfig to guard the read operation which would eventually get drop during FSI. Any owner for ensuring the same across program ?
256 Bytes are reserved at the top of the Flash Descriptor for use by the OEM. The information stored by the OEM can only be written during the manufacturing process as the Flash Descriptor read/write permissions must be set to Read Only when the computer leaves the manufacturing floor. The PCH Flash controller does not read this information. FFh is suggested to reduce programming time.
I could sense there are some underlying assumption which is not documented anything, either please raise a bug or write a .md file to clear out those assumptions.
Are you thinking if this region is locked and what value we will read, will that be always `1` (meaning CSE FW update is enable? in that case, you should opt for cse_fw_update_enable instead `disable`, so if softstrap is locked on production system, it will still read as `enable`/1)
By default, OEM SECTION contains all 0XFF, empty area. If nothing is written to OEM section, when coreboot reads, it gets 0xFF. But, our override logic looks for 1 to control the specific CB/SoC feature.
Hence, guard is required.
Why? to protect it from reading ?
No need to read as we want to withdraw the logic during FSI.
Also, please note SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE will be used to control >>other SOC/coreboot features as well going forward.
Any pointer about those *other SOC/coreboot features*, unless we have some >visibility it just a vision that you have and we are missing that might be.
I will try to share the feature set we want to control through SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE flag by next week.
I could sense there are some underlying assumption which is not documented anything, either please raise a bug or write a .md file to clear out those assumptions.
File src/soc/intel/common/block/debug/debug_feature.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/08ef2ae7_c0119f45 PS2, Line 28: cse_fw_update_disable
Assumption is, coreboot enables a feature by default and debug logic look for OEM SECTION if the feature is disabled or not. Hence, so identifier is chosen to reflect it's purpose. When OEM section indicates a feature has to be disabled, then override logic reverts the feature.
I agree with everything expect the need for a config that meant to guard reading OEM section. You can achieve all you intended here without that additional Kconfig guard even.
Also, if I understood it correctly the default value for OEM region is 0xFF so that way, unless someone override it for debug purpose, it would always read as `1` and consider `update is enable` and upon resetting the desire bit for debug purpose (to skip CSE update), the code logic would read it `0`. Not sure if it make sense to refer default `1` as CSE FW update disable.
return pre_mem_debug.cse_fw_update_disable == 1;