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:
(1 comment)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/60bb9e88_fd1114de PS2, Line 666: is_cse_fw_update_enabled
Do you mean `&` (bitwise) or `&&` (logical) ?
Here I am referring English word "and".
Logical you mean.
So, you will always have SOC_INTEL_CSE_RW_UPDATE on platform and if would like to disable RW update then will perform override from strap and select SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE ?
Need not be the case always. Initially we don't enable CSE firmware Update(SOC_INTEL_CSE_RW_UPDATE) at least beginning of the program.
Then don't select SOC_INTEL_CSE_RW_UPDATE in SoC unless you are ready! In that case your is_cse_fw_update_enabled() function first check would be if (CONFIG(SOC_INTEL_CSE_RW_UPDATE)) `enabled`.
But, we would like to enable SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE from the beginning of the program and remove the SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE KConfig during FSI.
I don't understand why you need a special Kconfig to guard the softstrap `read` ?(https://review.coreboot.org/c/coreboot/+/61380/2/src/soc/intel/common/block/...)
is there any restriction that on after FSI, we can't read the softstrap ? 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)
IMO, you don't need additional SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE to guard the soft strap read operation.
On a production system, descriptor region is RO, so SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE flag is not enabled at all on production systems.
I have asked the same question above, what so special about production system descriptor read operation. Understand that is RO, but does that mean, one can't read the value?
Hence, guard is required.
Why? to protect it from reading ?
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.
Hence, I prefer the flow which I mentioned in my earlier comment. Agree?
Sorry, I don't.