[coreboot-gerrit] Change in coreboot[master]: security/tpm: Unify the coreboot TPM software stack

[Philipp Deppenwiese (Code Review)]

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@288
PS7, Line 288: nv_policy_size;
> ...
Done


https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/vendor/cr50/Kconfig
File src/security/tpm/tss/vendor/cr50/Kconfig:

https://review.coreboot.org/#/c/24903/7/src/security/tpm/tss/vendor/cr50/Kconfig@17
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



-- 
To view, visit https://review.coreboot.org/24903
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ec277e82a3c20c62a0548a1a2b013e6ce8f5b3f
Gerrit-Change-Number: 24903
Gerrit-PatchSet: 10
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Julius Werner <jwerner at chromium.org>
Gerrit-CC: Patrick Rudolph <siro at das-labor.org>
Gerrit-CC: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Comment-Date: Mon, 16 Apr 2018 19:49:59 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180416/51354276/attachment.html>