Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/24903 )
Change subject: security/tpm: Unify the coreboot TPM software stack ......................................................................
Patch Set 10:
(13 comments)
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Kconfig File src/drivers/i2c/tpm/Kconfig:
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Kconfig@2 PS7, Line 2:
Agreed, should be deleted. […]
Done
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Kconfig@14 PS7, Line 14: bool
I think this could also just select MAINBOARD_HAS_TPM2 right away, so you don't have to select that […]
Don't think that is the right way to do it
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Kconfig@18 PS7, Line 18: Board has a Cr50 I2C TPM support
You need to add this to all Google mainboards that don't select any other other MAINBOARD_HAS_xxx_TP […]
Yep already done
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Makefile.inc File src/drivers/i2c/tpm/Makefile.inc:
https://review.coreboot.org/#/c/24903/7/src/drivers/i2c/tpm/Makefile.inc@1 PS7, Line 1: ifneq ($(CONFIG_TPM1),$(CONFIG_TPM2),)
This whole file needs to be guarded by […]
Done
https://review.coreboot.org/#/c/24903/7/src/drivers/pc80/tpm/Kconfig File src/drivers/pc80/tpm/Kconfig:
https://review.coreboot.org/#/c/24903/7/src/drivers/pc80/tpm/Kconfig@2 PS7, Line 2:
Should also be removed
Done
https://review.coreboot.org/#/c/24903/7/src/drivers/pc80/tpm/Kconfig@5 PS7, Line 5: LPC TPM driver is enabled!
nit: This text could use updating (e.g. "LPC TPM drivers are enabled." or something like that).
Done
https://review.coreboot.org/#/c/24903/7/src/drivers/spi/tpm/Kconfig File src/drivers/spi/tpm/Kconfig:
https://review.coreboot.org/#/c/24903/7/src/drivers/spi/tpm/Kconfig@2 PS7, Line 2: bool
same as for I2C_TPM, no need for empty quotes
Done
https://review.coreboot.org/#/c/24903/7/src/security/tpm/Kconfig File src/security/tpm/Kconfig:
https://review.coreboot.org/#/c/24903/7/src/security/tpm/Kconfig@66 PS7, Line 66:
nit: Rather than an if block around one option, just do depends on TPM1.
Done
https://review.coreboot.org/#/c/24903/7/src/security/tpm/Kconfig@82 PS7, Line 82: select DRIVER_TPM_DISPLAY_TIS_BYTES
I don't think that's correct, actually. […]
Done
https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/tcg-2.0/tss.c File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/tcg-2.0/tss.c@2... PS7, Line 288: nv_policy_size;
...
Done
https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/vendor/cr50/Kco... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/vendor/cr50/Kco... PS7, Line 17: y
Shouldn't this be default y? If both TPM2 and MAINBOARD_HAS_SOME_SORT_OF_TPM_CR50 are already set, t […]
Done
https://review.coreboot.org/#/c/24903/7/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/24903/7/src/security/vboot/secdata_tpm.c@202 PS7, Line 202: uint32_t rv;
BTW this file looks stale, we recently landed a small change here, you'll have to rebase it.
Done
https://review.coreboot.org/#/c/24903/7/src/security/vboot/secdata_tpm.c@205 PS7, Line 205: nv_policy_size);
nit: Can we make this a global next to ro_space_attributes and call it rw_space_attributes?
Done