[S] Change in coreboot[master]: {driver, soc/intel/cmn/cse}: Refactor ISH FW Version implementation

[Subrata Banik (Code Review)]

Attention is currently required from: Julius Werner.

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77176?usp=email )

Change subject: {driver, soc/intel/cmn/cse}: Refactor ISH FW Version implementation
......................................................................

Patch Set 1:

(1 comment)

File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/77176/comment/f51d3866_10f29d87 :
PS1, Line 1303: 		&version->cur_cse_fw_version, sizeof(struct fw_version))) {
> Oh, so you actually do need this to persist across multiple boots?

we will use CMOS to back up this entire VSD hence, we don't need to worry about backing it up in the same CL.
> Then maybe you do need an EARLY hook. But in that case you need to make sure the space is added at the same point with the same size regardless of boot mode (e.g. also in recovery mode), or it could screw up other EARLY hook spaces.

We can't get ISH version in recovery mode as the HECI cmd that support responding to the ISH query cmd is only available in non-recovery boot.
> I would also suggest maybe adding a checksum (e.g. `CRC(..., crc32_byte)`, which it looks like we already link into ramstage for some ACPI stuff anyway) for this so you can be somewhat confident you're actually looking at valid data when you just read a random leftover region in memory.

There could be instanced where different callers are adding entries into the VSD at different point (like CSE version, ISH version and/or downgrade CSE etc.) and adding CRC support would requires to recalculate the CRC with addition of every new data into the VSD. We don't ned to complicate a simple DS so much.

https://review.coreboot.org/c/coreboot/+/74995/47/src/soc/intel/common/block... , this code will ensure about implementing the CRC for persistent stores across the boot.
> (Also, has anyone ever looked at the security implications of this?

Can you highlight what is the security implications ?
> What are the potential implications if ISH version is misreported?)

zero implication, this is just one additional info. Btw, can you also highlight the scenario when u think the ISH version will misreport. if ISH is supported, we are sending a cmd to get the version and store into the CBMEM (in future into the CMOS) to avoid any additional boot time impact while reading this data across boot.