Attention is currently required from: Anil Kumar K, Jérémy Compostella, Krishna P Bhat D, Paul Menzel, Subrata Banik, sridhar siricilla.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email )
Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade ......................................................................
Patch Set 52:
(2 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/e51862b1_50211af6 : PS50, Line 1081: cse_boot_to_rw
as explained above we do not want to prevent the system to boot whenever there is a failure to PSR […]
While I understand you concern about losing the data and not being bothered, please have look at my reply below. I hope you understand.
https://review.coreboot.org/c/coreboot/+/74577/comment/22ac6b48_5b0edb13 : PS50, Line 1125: An attempt to send PSR back-up command has been made.
What do you mean by false logic? cse_boot_to_rw is not a false logic. if the device is unable to boot to RW and you aren't able to send PSR_HECI_FW_DOWNGRADE_BACKUP cmd, hence, you finally marked things as PSR backup done ? i don't understand the motivation here.
if you started ignoring all the failure cases and corner cases then I'm afraid we shall miss the original intent. What I'm asking is don't call the PSR backup done when it's not actually attempted or succeeded. Rather use something as PSR_BACKUP_ABORTED. At least it will help us to know whether the attempt is successful genuinely or not
It makes sense, We can add definitely add another status to the CMOS variable state. something like "Back-up failed", but the action that will be taken subsequently would be same i.e., set the value to PSR back-up pending after doing a data clear. So the intermediate new state is anyway not visible or can be acted upon. For a back-up failed scenario there is no credible action that the firmware can take, but to allow boot and let the application in OS figure out the failure.
Note that we are working under constraints. This behavior is only in chrome system where a downgrade has to be supported, PSR in windows doesn't need the back-up flow since they do not support downgrade and hence no data clear.
We had brainstormed how we can preserve the data without this implementation. Multiple options were considered and none of them can be scoped in the current platform, require significant work and security reviews. Eg: option 1: Store the PSR data outside of CSE purview, H1 can get the data and store in it's flash option 2: CSE will store the data outside of the normal data partition, which means it will not be encrypted and be decoded by old and new firmware.
I understand your concern and while we work with Google and Intel teams to improve the design which better fits the chrome-intel system constraints, for the current platform this is the best way forward.
-
Rizwan Qureshi (Code Review)