Change in ...coreboot[master]: security/tpm: Fix TCPA log feature

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29563 )

Change subject: security/tpm: Fix TCPA log feature
......................................................................

Patch Set 36:

(3 comments)

https://review.coreboot.org/#/c/29563/36/src/commonlib/include/commonlib/tcp... 
File src/commonlib/include/commonlib/tcpa_log_serialized.h:

https://review.coreboot.org/#/c/29563/36/src/commonlib/include/commonlib/tcp... 
PS36, Line 25: // Assumption of 2K Eventlog size reserved for CAR/SRAM
nit: TCPA log, not eventlog. (Also, comments please /* C89 */ instead of // C99 )

https://review.coreboot.org/#/c/29563/36/src/security/tpm/tspi/log.c 
File src/security/tpm/tspi/log.c:

https://review.coreboot.org/#/c/29563/36/src/security/tpm/tspi/log.c@41 
PS36, Line 41: 				tclt = (struct tcpa_table *)_vboot2_tpm_log;
This looks really messy... can't you just let this fall through to use the same code as the else-case below? Like this:

if (cbmem_possibly_online()) {
   tclt = cbmem_find(...)
   if (tclt)
     return tclt;
 }

tclt = (...)_vboot2_tpm_log;
 ...

I would just do the cbmem_add() and the initialization in recovery_tcpa_log(), then you don't need to work it in here.

(I guess you may have done this to try to fix the symbol linking issues you have? But this doesn't solve that either. We happen to have a similar problem while reworking the vboot handoff in CB:31329 right now, Joel is going to introduce a preram_symbols_accessible() helper that could also help you here.)

https://review.coreboot.org/#/c/29563/36/src/security/tpm/tspi/log.c@59 
PS36, Line 59: strncmp(TCPA_LOG_HEADER
Be careful that not all platforms necessarily wipe their SRAM on reset. So if you want this to work on non-x86 systems, you should define some point where it will definitely get overwritten. I think since your measurements always start in verstage anyway, you could just do

tclt = (...)_vboot2_tpm_log;
 if (ENV_VERSTAGE || ENV_BOOTBLOCK) {
    tclt->max_entries = MAX...;
    tclt->num_entries = 0;
 }

(I don't think a header is really necessary, CAR/SRAM doesn't just get magically corrupted between stages.)