Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31639 )
Change subject: sb/intel/common/firmware: Add an option to use ifdtool ......................................................................
Patch Set 5:
Patch Set 3:
I would pretty much prefer the opposite, i.e. only modify the descriptor if asked to.
In any case, as this implementation only affects the descriptor lock, a Kconfig choice would be better. Would make both Kconfig and Makefile simpler, I guess.
Just so I get it right, you mean that else branch of CONFIG_LOCK_MANAGEMENT_ENGINE in the Makefile doesn’t use ifdtool, right?
Um, no, there are three cases: 1. We want to leave the des- criptor untouched (I think this should be the default). 2. We want to make sure it's locked. 3. We want to make sure it's unlocked. Hence a Kconfig `choice` would reflect that much better.
In the Makefile this could be two separate `if` statements, one for 2. and one for 3. And if none are taken, 1. is implied.
I think the change to ‘choice’ has two hocks. The first is that if someone did not activate the ME/TXE lock until now and assumes that the regions are all unlocked, they will now come to the default branch – ‘don’t touch’. If now an already been edited descriptor region is used, these protect settings are applied. And the second issue we are getting with switching to ‘choice’ is a conflict with the lynxpoint Kconfig file. This also contains an option with the name ‘LOCK_MANAGEMENT_ENGINE’.