Change in coreboot[master]: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset
Hello Karthikeyan Ramasubramanian,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44653
to review the following change.
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
drivers/spi/tpm: Add helper to identify how cr50 sees AP reset
Introduce a helper to identify if Cr50 uses SYS_RESET# signal to observer the AP reset. This helper is used to trigger an appropriate AP reset when CSE Lite jumps from RO to RW.
BUG=None TEST=Ensure that Drawcia board boots to OS. Ensure that global reset is triggered when cr50 is running firmware versions newer than 0.0.22. On cr50 versions 0.0.22 or older, EC triggers cold reset of AP.
Change-Id: Id84b152993f253878a6c133cc433a0da2c990cf2 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h 2 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44653/1
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index bc40e852..64969cb 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -849,3 +849,16 @@
return payload_size; } + +bool cr50_sees_ap_reset_on_sys_reset(void) +{ + /* Cr50 observes AP Reset using SYS_RESET# signal on or before version 0.0.22 */ + if (cr50_firmware_version.epoch == 0 && cr50_firmware_version.major == 0 && + cr50_firmware_version.minor <= 22) + return true; + + printk(BIOS_INFO, "Cr50 firmware does not use SYS_RESET#, version: %d.%d.%d\n", + cr50_firmware_version.epoch, cr50_firmware_version.major, + cr50_firmware_version.minor); + return false; +} diff --git a/src/drivers/spi/tpm/tpm.h b/src/drivers/spi/tpm/tpm.h index b3e3f45..eb15160 100644 --- a/src/drivers/spi/tpm/tpm.h +++ b/src/drivers/spi/tpm/tpm.h @@ -44,4 +44,7 @@ /* Indicates whether Cr50 ready pulses are guaranteed to be at least 100us. */ bool cr50_is_long_interrupt_pulse_enabled(void);
+/* Indicates whether Cr50 observes AP reset using SYS_RESET# signal. */ +bool cr50_sees_ap_reset_on_sys_reset(void); + #endif /* ! __COREBOOT_SRC_DRIVERS_SPI_TPM_TPM_H */
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 1: Code-Review+2
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 1: Code-Review+2
Hello Sam McNally, build bot (Jenkins), Namyoon Woo, Furquan Shaikh, Christian Walter, Edward O'Callaghan, Nick Vaccaro, Tim Wawrzynczak, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44653
to look at the new patch set (#2).
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
drivers/spi/tpm: Add helper to identify how cr50 sees AP reset
Introduce a helper to identify if Cr50 uses SYS_RESET# signal to observe the AP reset. This helper is used to trigger an appropriate AP reset when CSE Lite jumps from RO to RW.
BUG=None TEST=Ensure that Drawcia board boots to OS. Ensure that global reset is triggered when cr50 is running firmware versions newer than 0.0.22. On cr50 versions 0.0.22 or older, EC triggers cold reset of AP.
Change-Id: Id84b152993f253878a6c133cc433a0da2c990cf2 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h 2 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44653/2
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 47: Indicates whether Cr50 observes AP reset using SYS_RESET# signal. If we want to keep this function as a common helper routine, then we should add a comment explaining under what conditions this is applicable. As I understand this, currently this is applicable only to boards that are using strap cfg 0xe where the old firmware defaults to assuming that PLTRST# is connected to a pin which is actually SYS_RESET# signal. For boards using all other strap configs this function is not really applicable.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 48: cr50_sees_ap_reset_on_sys_reset Some thoughts just to ensure that this does not get used incorrectly for any board in the future. I am wondering if we should:
1. Add a Kconfig "CR50_STRAP_REQUIRES_UPDATED_FIRMWARE" or "CR50_STRAP_CHECK_FIRMWARE_SUPPORT" so that it can be set by mainboards that actually use a new strap that old cr50 firmware does not know about.
2. Set this Kconfig in boards that are actually affected - Puff, Dedede, Volteer
3. Rename function to cr50_firmware_pltrst_on_wrong_pin()
4. This function will: a. Return false if CR50_STRAP_REQUIRES_UPDATED_FIRMWARE/CR50_STRAP_CHECK_FIRMWARE_SUPPORT is not selected. b. Else, return false if firmware version is newer than the minimum version required for the new strap. c. Else, return true.
The other option is to simply add a helper function to read cr50 firmware version i.e. cr50_get_version() and implement the logic to decide whether EC-driven SYSRST# is required directly in csme_board_reset() since it is implemented only by boards that require this workaround. Thoughts?
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 856: cr50_firmware_version.epoch == 0 && cr50_firmware_version.major == 0 && : cr50_firmware_version.minor <= 22 Can you please check with Mary or Namyoon to get a confirmation on what firmware version really supports the new strap 0xe. I think we might need a check for more than just 0.0.22. i.e. firmware versions newer than 0.0.22 as well.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 860: printk(BIOS_INFO, "Cr50 firmware does not use SYS_RESET#, version: %d.%d.%d\n", : cr50_firmware_version.epoch, cr50_firmware_version.major, : cr50_firmware_version.minor); Is this print really required? It is not really accurate. cr50 firmware that understands strap 0xe uses SYS_RESET# signal to reset the AP.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 48: cr50_sees_ap_reset_on_sys_reset
Some thoughts just to ensure that this does not get used incorrectly for any board in the future. […]
I prefer option 1.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to identify how cr50 sees AP reset ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 48: cr50_sees_ap_reset_on_sys_reset
I prefer option 1.
On a second thought, passing firmware version seems more generic than the cr50_firmware_pltrst_on_wrong_pin. May be more use-cases in future might benefit out of it than this one-off helper function. I will go ahead and implement a getter function to get the firmware version.
Hello Sam McNally, build bot (Jenkins), Daniel Kurtz, Furquan Shaikh, Namyoon Woo, Christian Walter, Edward O'Callaghan, Nick Vaccaro, Tim Wawrzynczak, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44653
to look at the new patch set (#3).
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
drivers/spi/tpm: Add helper to get cr50 firmware version
Introduce a helper to get the cached cr50 firmware version. This information is in turn used to identify the strap configuration supported by Cr50.
BUG=None TEST=Ensure that Drawcia board boots to OS. Ensure that the version cached cr50 firmware version is returned.
Change-Id: Id84b152993f253878a6c133cc433a0da2c990cf2 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h 2 files changed, 17 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44653/3
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h File src/drivers/spi/tpm/tpm.h:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 47: Indicates whether Cr50 observes AP reset using SYS_RESET# signal.
If we want to keep this function as a common helper routine, then we should add a comment explaining […]
Add a more generic function to return the cr50 firmware version.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.h@4... PS1, Line 48: cr50_sees_ap_reset_on_sys_reset
On a second thought, passing firmware version seems more generic than the cr50_firmware_pltrst_on_wr […]
Done
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 856: cr50_firmware_version.epoch == 0 && cr50_firmware_version.major == 0 && : cr50_firmware_version.minor <= 22
Can you please check with Mary or Namyoon to get a confirmation on what firmware version really supp […]
Added a more generic function to return Cr50 firmware version.
https://review.coreboot.org/c/coreboot/+/44653/1/src/drivers/spi/tpm/tpm.c@8... PS1, Line 860: printk(BIOS_INFO, "Cr50 firmware does not use SYS_RESET#, version: %d.%d.%d\n", : cr50_firmware_version.epoch, cr50_firmware_version.major, : cr50_firmware_version.minor);
Is this print really required? It is not really accurate. […]
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44653/3/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44653/3/src/drivers/spi/tpm/tpm.c@8... PS3, Line 849: { nit: memcpy should work fine as well.
Hello Sam McNally, build bot (Jenkins), Daniel Kurtz, Furquan Shaikh, Namyoon Woo, Christian Walter, Edward O'Callaghan, Nick Vaccaro, Tim Wawrzynczak, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44653
to look at the new patch set (#4).
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
drivers/spi/tpm: Add helper to get cr50 firmware version
Introduce a helper to get the cached cr50 firmware version. This information is in turn used to identify the strap configuration supported by Cr50.
BUG=None TEST=Ensure that Drawcia board boots to OS. Ensure that the version cached cr50 firmware version is returned.
Change-Id: Id84b152993f253878a6c133cc433a0da2c990cf2 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h 2 files changed, 15 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44653/4
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44653/3/src/drivers/spi/tpm/tpm.c File src/drivers/spi/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/44653/3/src/drivers/spi/tpm/tpm.c@8... PS3, Line 849: {
nit: memcpy should work fine as well.
Done
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 4: Code-Review+2
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 4: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
drivers/spi/tpm: Add helper to get cr50 firmware version
Introduce a helper to get the cached cr50 firmware version. This information is in turn used to identify the strap configuration supported by Cr50.
BUG=None TEST=Ensure that Drawcia board boots to OS. Ensure that the version cached cr50 firmware version is returned.
Change-Id: Id84b152993f253878a6c133cc433a0da2c990cf2 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44653 Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Sam McNally sammc@google.com Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/spi/tpm/tpm.c M src/drivers/spi/tpm/tpm.h 2 files changed, 15 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Sam McNally: Looks good to me, approved
diff --git a/src/drivers/spi/tpm/tpm.c b/src/drivers/spi/tpm/tpm.c index bc40e852..66db671 100644 --- a/src/drivers/spi/tpm/tpm.c +++ b/src/drivers/spi/tpm/tpm.c @@ -48,11 +48,6 @@
/* Cached TPM device identification. */ static struct tpm2_info tpm_info; -struct cr50_firmware_version { - int epoch; - int major; - int minor; -}; static struct cr50_firmware_version cr50_firmware_version;
/* @@ -849,3 +844,8 @@
return payload_size; } + +void cr50_get_firmware_version(struct cr50_firmware_version *version) +{ + memcpy(version, &cr50_firmware_version, sizeof(*version)); +} diff --git a/src/drivers/spi/tpm/tpm.h b/src/drivers/spi/tpm/tpm.h index b3e3f45..39d54e7 100644 --- a/src/drivers/spi/tpm/tpm.h +++ b/src/drivers/spi/tpm/tpm.h @@ -16,6 +16,13 @@ uint16_t revision; };
+/* Structure describing the elements of Cr50 firmware version. */ +struct cr50_firmware_version { + int epoch; + int major; + int minor; +}; + /* * Initialize a TPM2 device: read its id, claim locality of zero, verify that * this indeed is a TPM2 device. Use the passed in handle to access the right @@ -44,4 +51,7 @@ /* Indicates whether Cr50 ready pulses are guaranteed to be at least 100us. */ bool cr50_is_long_interrupt_pulse_enabled(void);
+/* Get the cr50 firmware version information. */ +void cr50_get_firmware_version(struct cr50_firmware_version *version); + #endif /* ! __COREBOOT_SRC_DRIVERS_SPI_TPM_TPM_H */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44653 )
Change subject: drivers/spi/tpm: Add helper to get cr50 firmware version ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 6/1/7 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16153 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16152 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/16151 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16150 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/16149 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16155 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16154
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Daniel Kurtz (Code Review)
-
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Sam McNally (Code Review)