Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30826 )
Change subject: security/tpm/tss/tcg-2.0: Add function to return TPM2 capabilities ......................................................................
Patch Set 4: Code-Review-1
(6 comments)
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss.h File src/security/tpm/tss.h:
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss.h@125 PS4, Line 125: #if IS_ENABLED(CONFIG_TPM2) Please add the TPM2-only stuff to the existing CONFIG(TPM2) block above. (Also, IS_ENABLED() is deprecated, please use CONFIG().)
Eventually, we should try to split the version-specific stuff here out into separate headers.
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss.h@136 PS4, Line 136: void *tpm_process_command_ex(TPM_CC command, void *command_body, Please don't just create a new function for the same thing. This is not an interface that needs to be kept stable, we can update old callers. You should enhance the existing tpm_process_command() (e.g. make it keep the old functionality if response_size is NULL and marshal is false, and update callers to pass the correct command_size for the old behavior).
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss.h@144 PS4, Line 144: uint32_t tlcl_extend_ex(int pcr_num, const TPML_DIGEST_VALUES *in_digests); Same here, please update the existing tlcl_extend() to be more flexible, we don't need two extend functions.
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss/tcg-2.0/tss.c File src/security/tpm/tss/tcg-2.0/tss.c:
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss/tcg-2.0/tss.c@2... PS4, Line 237: if (in_digests->digests[0].hashAlg == TPM_ALG_ERROR) { Please use && for this sort of stuff, not 4 levels of nesting.
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss/tcg-2.0/tss.c@2... PS4, Line 290: printk(BIOS_SPEW, "%s: tlcl_init_done is %d\n", __func__, done); Do we need to permanently merge these? They seem excessive even for SPEW...
https://review.coreboot.org/#/c/30826/4/src/security/tpm/tss/tcg-2.0/tss.c@5... PS4, Line 531: for (index = 0; index < ARRAY_SIZE(mHashInfo); index++) nit: maybe that's just me, but wouldn't a switch() statement look better than parsing an array from somewhere else?