Attention is currently required from: Erik van den Bogaert, Jason Glenesk, Raul Rangel, Michał Żygowski, Frans Hendriks, Matt DeVillier, Christian Walter, Krystian Hebel, Fred Reitberger, Yu-Ping Wu, Sergii Dmytruk, Felix Held.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69160 )
Change subject: security/tpm: resolve conflicts in TSS implementations ......................................................................
Patch Set 4:
(5 comments)
File src/security/tpm/tss.h:
https://review.coreboot.org/c/coreboot/+/69160/comment/4c0031dd_43fe391f PS4, Line 26: * tlcl_lib_init(). I think you should use tlcl1_ and tlcl2_ as prefixes... mixing tlcl12_ and tlcl2_ is just confusing.
https://review.coreboot.org/c/coreboot/+/69160/comment/32e28046_2580604d PS4, Line 30: * TPM1.2-specific I think this would be a good opportunity to factor this out into more files, since this one is getting quite long... I would make one for 1.2-only functions, one for 2.0-only functions, and one for the shared wrappers.
File src/security/tpm/tss/tss.c:
https://review.coreboot.org/c/coreboot/+/69160/comment/89a24733_90c31bf3 PS4, Line 32: #if CONFIG(TPM1) These don't need to be preprocessor #if, you can just write `if (CONFIG(TPM1) && tpm_family == 1)`. Linker garbage collection will ensure that the code for compiled-out versions doesn't get pulled into the final binary.
https://review.coreboot.org/c/coreboot/+/69160/comment/4647751c_ce998039 PS4, Line 35: return VB2_SUCCESS; Why don't you just make tis_sendrecv an exported global in this file that can be directly accessed from the other files, rather than passing this pointer back and forth between so many files?
https://review.coreboot.org/c/coreboot/+/69160/comment/1f49a065_236180aa PS4, Line 62: die("%s: can't be used with TPM %d\n", __func__, tpm_family); nit: not really sure you need these since it shouldn't be possible to get to this place (tlcl_lib_init() would have already resulted in an error).
-
Julius Werner (Code Review)