Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: Enable long cr50 ready pulses for Tigerlake systems ......................................................................
Patch Set 7:
(8 comments)
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/2//COMMIT_MSG@15 PS2, Line 15: to FSP. For Volteer (and future Tigerlake boards) we enable mode S0i3.4
Regarding Julius' alternative proposal, of moving all the ramstage cr50 communication to be earlier […]
The version is always loaded the first time the TPM interface is initialized in each stage, so if you don't load it in early ramstage the cr50 update check will just load it later. You're not saving any time by passing things through CBMEM (but you're wasting memory that will be reserved from the OS forever).
BS_PRE_DEVICE is the bootstage where the callback runs (which is previously BS_PAYLOAD_LOAD for the cr50 update). BS_ON_ENTRY/BS_ON_EXIT decides whether to run before or after that stage. From my reading of the FSP-S loading code it is currently loaded in the pre-device stage, so I think if you combine BS_PRE_DEVICE and BS_ON_ENTRY it should run before FSP-S loading.
I agree with Furquan though that it would be better to root cause and eliminate the SPI hardware problem rather than trying to play around with timings to sidestep it.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@3... PS7, Line 357: int cr50_read_board_cfg(uint32_t *status) Maybe check the version number right in here and return a -1 or something if we know the board doesn't support board_cfg?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@5... PS7, Line 571: set_cr50_board_cfg(); Twice?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 600: static Why is this static?
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 650: index = 3; This is super complicated. Can't we just do something like
char version[REASONABLE_MAXIMUM_LETS_SAY_MAYBE_256]; do { read chunks of 50 into version } version[last_read_index + 1] = '\0'; printk(BIOS_INFO, "Firmware version: %s\n", version); /* Next part should probably be encapsulated in separate CONFIG(TPM_CR50)-only function */ char *number = strstr("RW_A:", version); if (!number) number = strstr("RW_B:", version); if (!number) { ...complain... } else { cr50_version_major = skip_atoi(number); if (*number++ != '.') ...error out... cr50_version_minor = skip_atoi(number); if (*number++ != '.') ...error out... cr50_version_rev = skip_atoi(number); }
Looks like coreboot doesn't have a strstr() yet so it's probably time to add one. Just strcmp() in a while() loop, should almost be a one-liner...
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 661: #if CONFIG(TPM_CR50) && CONFIG_TPM_BOARD_CFG Please always use
if (CONFIG(...))
instead of
#if CONFIG(...)
where possible.
https://review.coreboot.org/c/coreboot/+/43741/7/src/drivers/spi/tpm/tpm.c@6... PS7, Line 662: ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK || !CONFIG(VBOOT) Let's factor this check out into a separate function (e.g. if (first_access_this_boot()) )
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c File src/security/vboot/cr50.c:
https://review.coreboot.org/c/coreboot/+/43741/2/src/security/vboot/cr50.c@2... PS2, Line 26: return;
I asked Namyoon this question, and he told me that prior versions of the cr50 firmware will give ran […]
... I really wish I was still surprised by that at this point...