Thanks, I think this looks good now from my side (other than a few obvious one line fixes I commented). I'll leave the final review and +2 to Philipp since this is all his code.
Patch set 54:Code-Review +1
7 comments:
File src/drivers/pc80/tpm/Makefile.inc:
Patch Set #54, Line 5: bootblock-$(CONFIG_LPC_TPM) += tis.c
No longer necessary.
Patch Set #54, Line 24: #include <security/tpm/tspi/crtm.h>
No longer necessary.
File src/security/tpm/tspi/crtm.c:
Patch Set #54, Line 52: static int tcpa_log_initialized;
This whole thing with a global variable and several functions seems a bit heavy-handed to me... I think you could've achieved the same by just flipping the lines in my suggestion:
if (ENV_BOOTBLOCK) {
static bool initialized = 0;
if (!initialized) {
initialized = 1;
tspi_init_crtm();
}
}
But I'm fine with this too if you think it makes the code clearer.
Patch Set #54, Line 55: if (ENV_DECOMPRESSOR)
nit: not wrong, but the decompressor can never include CBFS code anyway so also not really necessary
Patch Set #54, Line 73: printk(BIOS_INFO, "TSPI: CRTM already initialized!\n");
I don't see why you need this? This should never be printed, right? (Or maybe it does during that recursion thing you mentioned, but then I'm not sure why you'd always want this message to be printed.)
Patch Set #54, Line 129: "Initializing CRTM failed!");
Should probably return 0 here?
File src/security/vboot/symbols.h:
Patch Set #54, Line 23: DECLARE_REGION(tpm_tcpa_log)
nit: move this to <symbols.h>? (Not really sure why this separate file still exists anyway, we've been throwing all kinds of optional feature or arch specific stuff into <symbols.h> lately.)
To view, visit change 35077. To unsubscribe, or for help writing mail filters, visit settings.