Attention is currently required from: Stefan Ott, Felix Singer, Patrick Rudolph, Jonathan Zhang, Nick Vaccaro, Arthur Heymans, Andrey Petrov, Piotr Król, Jes Klinke, Nico Huber, Sean Rhodes, Michał Żygowski, Johnny Lin, Christian Walter, Werner Zeh, Alexander Couzens, Yu-Ping Wu, Tim Chu, Frans Hendriks, Tristan Corrick, Jeremy Soller, Angel Pons, Michael Niewöhner, Tim Crawford, Maxim Polyakov, Tim Wawrzynczak. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63424 )
Change subject: tpm: Refactor TPM Kconfig dimensions ......................................................................
Patch Set 6:
(12 comments)
Patchset:
PS6: Thanks Jes, this looks great!
File src/drivers/i2c/tpm/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63424/comment/9224ce55_1a6b8ebe PS6, Line 3: ifeq ($(CONFIG_I2C_TPM),y) Might as well wrap all lines in the file in this?
In fact, I think it would be good to wrap all this in both
ifeq ($(CONFIG_TPM)$(CONFIG_I2C_TPM),yy)
and do the same for the SPI, CRB and LPC drivers. CONFIG_TPM being off should ensure that none of this code ever runs, but right now there's not really much static checking for that. If we avoid even compiling it we both save build time and make it easier to see if someone accidentally calls TPM functions in the wrong config.
File src/drivers/tpm/Makefile.inc:
PS6: This whole Makefile should also be wrapped in a CONFIG_TPM check.
File src/mainboard/google/herobrine/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/9c62ff21_e5d514a0 PS6, Line 28: select TPM_GOOGLE_CR50 This needs an `if !BOARD_GOOGLE_SENOR`
File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/44304c26_521388e2 PS6, Line 87: if (!CONFIG(SPI_TPM)) { This doesn't seem to make a difference for the currently available configurations, but I think it would be cleaner to also check && !CR50 here.
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/gpio.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/5c4ea59b_3f6e4f08 PS6, Line 594: /* CLK_25M_MEMORY_MAPPED_TPM_CPU */ You probably don't want to replace this.
File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/79a6510c_949d3879 PS6, Line 10: depends on TPM2 These shouldn't actually depend on TPM2 now, and neither should TPM_GOOGLE. The goal is that even if the mainboard has a Cr50/Ti50 TPM, the user can still disable it by selecting NO_TPM in menuconfig.
https://review.coreboot.org/c/coreboot/+/63424/comment/231f26f6_6586c305 PS6, Line 12: default n nit: you don't need to specify `default n`, but if you do it's customary to put it above the `select`s.
https://review.coreboot.org/c/coreboot/+/63424/comment/a1b02d52_6b76c65a PS6, Line 20: OOGLE_CR50 || TPM_GOOGLE_TI50 Just TPM_GOOGLE?
File src/soc/intel/common/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63424/comment/30ac40ed_aa3d157a PS6, Line 23: all-$(CONFIG_TPM_GOOGLE) += tpm_tis.c nit: a bit off-topic, but does Ti50 actually still require waiting for the sideband interrupt before doing the next transaction? I was hoping we would have gotten rid of that requirement now... (because that's what this code here is for)
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/63424/comment/426cd070_74804dfb PS6, Line 25: config TPM_GOOGLE_IMMEDIATELY_COMMIT_FW_SECDATA Also a bit off-topic but it would probably be good to move this to tss/vendor/google. It shouldn't really depend on CHROMEOS.
File src/vendorcode/google/chromeos/cse_board_reset.c:
https://review.coreboot.org/c/coreboot/+/63424/comment/3d623ed1_8869ecc0 PS6, Line 19: if (CONFIG(SPI_TPM) && CONFIG(TPM_GOOGLE_CR50)) { Is there a reason to specifically check SPI_TPM here? Looks to me like the important factor here was just Cr50, but the only boards affected by this all used SPI.
We should probably also check if the TPM is enabled here, which should always have been done but is an edge case people tend to forget. So check (CONFIG(TPM2) && CONFIG(TPM_GOOGLE_CR50))?