Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31260 )
Change subject: coreboot: check Cr50 PM mode on normal boot ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h File src/security/tpm/tss_errors.h:
https://review.coreboot.org/#/c/31260/3/src/security/tpm/tss_errors.h@45 PS3, Line 45: #define TPM_E_NO_SUCH_COMMAND ((uint32_t)0x00005010) nit: I think the train for keeping this in sync with the vboot version has long since left the station, so might as well use 500e here.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 69: cr50_must_reset = 1; If this is never supposed to happen under normal circumstances, you should print a warning here.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 96: if (vboot_recovery_mode_enabled()) I don't think the problem here is solved yet? We want to make sure we don't boot in recovery mode in a state where NVRAM changes won't be committed. Maybe that doesn't need to be solved in this CL, but it should be fixed before any board supporting this is shipped, so please at least file a bug about it.
https://review.coreboot.org/#/c/31260/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 127: * to the FW version check. nit: Can you please file a bug for this and assign it to someone to make sure we won't forget it?