Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43741 )
Change subject: drivers/spi/tpm: Enable long cr50 ready pulses for Tiger Lake systems ......................................................................
Patch Set 20:
(12 comments)
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43741/19//COMMIT_MSG@20 PS19, Line 20: Bug: b:154333137
This needs to go before the Signed-off-by line. […]
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.h@... PS19, Line 45: get_cr50_ready_pulse_length_us
Do we really need to pass a length back? I think this function can simply return a bool indicating w […]
You may be right that I am over-engineering, for cases where a future family/architecture requires e.g. 250us pulses, and we could have the cr50 emitting either 4us, 100us or 250us, and had in response to disable power savings to various degrees in response.
We can add such functionality when we need it, for now I have changed this method to returning bool.
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/18/src/drivers/spi/tpm/tpm.c@... PS18, Line 638: padded
I do believe that Cr50 will truly zero-pad the sequence of bytes transmitted over SPI, even though t […]
Actually, the code both before and after my change looks only at the very last of the 50 bytes read, to determine if more chunks should be read or not. That works only if it can rely on the string from cr50 being zero-padded. (Strictly speaking, if the remaning string is three or more bytes shorter than the buffer, the "middle" part of the padding need not be zero'ed, for the previous code to work, onle the first and last byte of the padding. However, I cannot imagine the cr50 code honoring that, without also filling the intervening padding with zeroes.)
I have reverted the comment to stating that the chunk will be zero padded.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 451: constant value.
nit: This probably fits on the previous line within the 96-column limit?
Ack
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 484: board_cfg_value, CR50_BOARD_CFG_VALUE);
Should there be an early return if board_cfg_value == CR50_BOARD_CFG_VALUE?
The orignal plan was that Cr50 made the register read-only after it being written once. That has been revised such that AP jumping to RW also locks the register. Given that revision, it is safe to not explicitly set it here, if the value matches already.
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 510: if (get_cr50_board_cfg() & CR50_BOARD_CFG_100US_READY_PULSE) In a dependent CL, Furquan suggested that this method should call tlcl_lib_init(), in order to ensure that the library is initialized before calling get_cr50_board_cfg().
So far I have resisted, because I feel that this is a low level library, and that it is not appropriate for it to call the higher-level tlcl_lib. For instance the tpm2_get_info() function elsewhere in this file also does not call tlcl_lib_init(), but instead assumes that the higher level library has already been initialized, calling tpm2_init() in the process.
We could consider creating a wrapper at a higher place, which could call first tlcl_lib_init() and then this function. Furquan, do you have suggestions for an appropriate place?
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 628: char
static?
Is stack space tight? We are not using this version beyond the invocation of this method, so what is the benefit of declaring it static?
(An earlier version of this CL kept only 50 byte in memory at a time, and performed the parsing incrementally. That code was hideous.)
https://review.coreboot.org/c/coreboot/+/43741/19/src/drivers/spi/tpm/tpm.c@... PS19, Line 645: sizeof(version_str)
Shouldn't this be sizeof(version_str) - 1 to keep the logic same as before? Or was the change intent […]
You are right, thanks for paying attention.
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/18/src/security/tpm/tss/vendo... PS18, Line 19: "Whether to request longer pulses using Cr50 BOARD_CFG register"
Would probably be good to clarify that just selecting this option doesn't guarantee you're gonna get […]
In time, the factory image of Cr50 will include the feature, and from that point, new designs can drop the logic around testing whether the Cr50 supports the feature. I do not want to name the setting itself to fit the transition period, but I have elaborated in the help description. Later on, we can edit the description, to not confuse future engineers with legacy concerns.
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... File src/security/tpm/tss/vendor/cr50/Kconfig:
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: "
" not required. Add '.' instead.
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: "
" not required
Done
https://review.coreboot.org/c/coreboot/+/43741/19/src/security/tpm/tss/vendo... PS19, Line 19: pulses
interrupt pulses.
Done