Change in coreboot[master]: util/ifdtool: Enable CPU read for ME region

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46441 )

Change subject: util/ifdtool: Enable CPU read for ME region
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46441/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46441/7//COMMIT_MSG@8 
PS7, Line 8: 
           : We are implementing a mechanism in coreboot to update CSME firmware,
           : this requires coreboot to be able to read CSME region.
> Do you have a pointer to this? The current mechanism relies on querying the CSE to provide its versi […]

The current mechanism is not relying on determining the version by reading CSE region, however we are exploring another way to determine the version either by reading a fixed offset (like the OEM Build version field) or parsing the CSE region. For that purpose we need a mechanism to enable the read on ME after it is locked.
There is internal test case which uses ifdtool to lock ME region using and test write protection on that. When running this tests with the new CSE update implementation we run into problem since the CSE region is not readable.

Providing an option in IFD tool to enable CPU read of CSME, will enable the test case to validate the new approach.

https://review.coreboot.org/c/coreboot/+/46441/7//COMMIT_MSG@13 
PS7, Line 13: This patch will enable read access to CSME region when locking.
> Should ifdtool honor the settings that are already present in the descriptor if it is not all ffs? I […]

A check in ifdtool to see if there is already a non-default region access applied can be implemented. However, during the development phase where the ME is not locked and the automated test cases use ifdtool to lock ME, we will need an option to skip disabling read.

I agree that when ifdtool is used for locking it has to honor non-default region access values if already present. And we can add a -f (force) option to override the existing settings. May be another patch for that.