Changes from v0 -> v1
Patch 1/3 *Rename sts to value
Patch 2/3 *No Change
Patch 3/3 *No longer sending command to the TPM. Instead check the InterfaceVersion field in the TPM_INTF_CAPABILITY register if necessary. *Removed "Tested-by:" since I wasn't able to find 1.2 hardware for regression test.
Stephen Douthit (3): tpm: Refactor duplicated wait code in tis_wait_sts() & crb_wait_reg() tpm: Wait for interface startup when probing tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
src/hw/tpm_drivers.c | 113 +++++++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 53 deletions(-)
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com --- src/hw/tpm_drivers.c | 80 ++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 47 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 1fd05e1..13ad821 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -63,6 +63,39 @@ static void *crb_cmd; static u32 crb_resp_size; static void *crb_resp;
+static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) +{ + if (!CONFIG_TCGBIOS) + return 0; + + u32 rc = 1; + u32 end = timer_calc_usec(time); + + for (;;) { + u8 value = readl(reg); + if ((value & mask) == expect) { + rc = 0; + break; + } + if (timer_check(end)) { + warn_timeout(); + break; + } + yield(); + } + return rc; +} + +static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) +{ + return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect); +} + +static u32 crb_wait_reg(u8 locty, u16 reg, u32 time, u8 mask, u8 expect) +{ + return wait_reg8(CRB_REG(locty, reg), time, mask, expect); +} + /* if device is not there, return '0', '1' otherwise */ static u32 tis_probe(void) { @@ -152,30 +185,6 @@ static void set_timeouts(u32 timeouts[4], u32 durations[3]) memcpy(dus, durations, 3 * sizeof(u32)); }
- -static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) -{ - if (!CONFIG_TCGBIOS) - return 0; - - u32 rc = 1; - u32 end = timer_calc_usec(time); - - for (;;) { - u8 sts = readb(TIS_REG(locty, TIS_REG_STS)); - if ((sts & mask) == expect) { - rc = 0; - break; - } - if (timer_check(end)) { - warn_timeout(); - break; - } - yield(); - } - return rc; -} - static u32 tis_activate(u8 locty) { if (!CONFIG_TCGBIOS) @@ -399,29 +408,6 @@ static u32 crb_init(void) return 0; }
-static u32 crb_wait_reg(u8 locty, u8 reg, u32 time, u8 mask, u8 expect) -{ - if (!CONFIG_TCGBIOS) - return 0; - - u32 rc = 1; - u32 end = timer_calc_usec(time); - - for (;;) { - u8 sts = readl(CRB_REG(locty, reg)); - if ((sts & mask) == expect) { - rc = 0; - break; - } - if (timer_check(end)) { - warn_timeout(); - break; - } - yield(); - } - return rc; -} - static u32 crb_activate(u8 locty) { if (!CONFIG_TCGBIOS)
On 02/27/2018 02:17 PM, Stephen Douthit wrote:
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com
Reviewed-by: Stefan Berger stefanb@linux.vnet.ibm.com
src/hw/tpm_drivers.c | 80 ++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 47 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 1fd05e1..13ad821 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -63,6 +63,39 @@ static void *crb_cmd; static u32 crb_resp_size; static void *crb_resp;
+static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) +{
- if (!CONFIG_TCGBIOS)
return 0;- u32 rc = 1;
- u32 end = timer_calc_usec(time);
- for (;;) {
u8 value = readl(reg);if ((value & mask) == expect) {rc = 0;break;}if (timer_check(end)) {warn_timeout();break;}yield();- }
- return rc;
+}
+static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect);
+}
+static u32 crb_wait_reg(u8 locty, u16 reg, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(CRB_REG(locty, reg), time, mask, expect);
+}
- /* if device is not there, return '0', '1' otherwise */ static u32 tis_probe(void) {
@@ -152,30 +185,6 @@ static void set_timeouts(u32 timeouts[4], u32 durations[3]) memcpy(dus, durations, 3 * sizeof(u32)); }
-static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) -{
- if (!CONFIG_TCGBIOS)
return 0;- u32 rc = 1;
- u32 end = timer_calc_usec(time);
- for (;;) {
u8 sts = readb(TIS_REG(locty, TIS_REG_STS));if ((sts & mask) == expect) {rc = 0;break;}if (timer_check(end)) {warn_timeout();break;}yield();- }
- return rc;
-}
- static u32 tis_activate(u8 locty) { if (!CONFIG_TCGBIOS)
@@ -399,29 +408,6 @@ static u32 crb_init(void) return 0; }
-static u32 crb_wait_reg(u8 locty, u8 reg, u32 time, u8 mask, u8 expect) -{
- if (!CONFIG_TCGBIOS)
return 0;- u32 rc = 1;
- u32 end = timer_calc_usec(time);
- for (;;) {
u8 sts = readl(CRB_REG(locty, reg));if ((sts & mask) == expect) {rc = 0;break;}if (timer_check(end)) {warn_timeout();break;}yield();- }
- return rc;
-}
- static u32 crb_activate(u8 locty) { if (!CONFIG_TCGBIOS)
This is based on wait_startup() from the Linux tpm_tis driver.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com --- src/hw/tpm_drivers.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 13ad821..da8bb63 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -86,6 +86,11 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) return rc; }
+static u32 tis_wait_access(u8 locty, u32 time, u8 mask, u8 expect) +{ + return wait_reg8(TIS_REG(locty, TIS_REG_ACCESS), time, mask, expect); +} + static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) { return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect); @@ -102,7 +107,13 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 rc = 0; + /* Wait for the interface to report it's ready */ + u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A, + TIS_ACCESS_TPM_REG_VALID_STS, + TIS_ACCESS_TPM_REG_VALID_STS); + if (rc) + return 0; + u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
if ((didvid != 0) && (didvid != 0xffffffff))
On 02/27/2018 02:17 PM, Stephen Douthit wrote:
This is based on wait_startup() from the Linux tpm_tis driver.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com
Reviewed-by: Stefan Berger stefanb@linux.vnet.ibm.com
src/hw/tpm_drivers.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 13ad821..da8bb63 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -86,6 +86,11 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) return rc; }
+static u32 tis_wait_access(u8 locty, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(TIS_REG(locty, TIS_REG_ACCESS), time, mask, expect);
+}
- static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) { return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect);
@@ -102,7 +107,13 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 rc = 0;
/* Wait for the interface to report it's ready */
u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
TIS_ACCESS_TPM_REG_VALID_STS,TIS_ACCESS_TPM_REG_VALID_STS);if (rc)
return 0;u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID)); if ((didvid != 0) && (didvid != 0xffffffff))
On Tue, Feb 27, 2018 at 02:17:10PM -0500, Stephen Douthit wrote:
This is based on wait_startup() from the Linux tpm_tis driver.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com
src/hw/tpm_drivers.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 13ad821..da8bb63 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -86,6 +86,11 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) return rc; }
+static u32 tis_wait_access(u8 locty, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(TIS_REG(locty, TIS_REG_ACCESS), time, mask, expect);
+}
static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) { return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect); @@ -102,7 +107,13 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 rc = 0;
- /* Wait for the interface to report it's ready */
- u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
TIS_ACCESS_TPM_REG_VALID_STS,TIS_ACCESS_TPM_REG_VALID_STS);- if (rc)
return 0;
Thanks. I was about to commit this when I noticed the above. In the case where tis_wait_access() fails due to a timeout, shouldn't tis_probe() return an error indicator instead of success?
-Kevin
On 03/02/2018 11:05 AM, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:10PM -0500, Stephen Douthit wrote:
This is based on wait_startup() from the Linux tpm_tis driver.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com
src/hw/tpm_drivers.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 13ad821..da8bb63 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -86,6 +86,11 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) return rc; }
+static u32 tis_wait_access(u8 locty, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(TIS_REG(locty, TIS_REG_ACCESS), time, mask, expect);
+}
- static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) { return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect);
@@ -102,7 +107,13 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 rc = 0;
- /* Wait for the interface to report it's ready */
- u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
TIS_ACCESS_TPM_REG_VALID_STS,TIS_ACCESS_TPM_REG_VALID_STS);- if (rc)
return 0;Thanks. I was about to commit this when I noticed the above. In the case where tis_wait_access() fails due to a timeout, shouldn't tis_probe() return an error indicator instead of success?
tis_probe() returns 0 for no device, '1' for device present.
Take a look at the usage in tpmhw_probe(), or the comment on tis_probe() itself.
Thanks, Steve
On Fri, Mar 02, 2018 at 11:27:46AM -0500, Stephen Douthit wrote:
On 03/02/2018 11:05 AM, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:10PM -0500, Stephen Douthit wrote:
This is based on wait_startup() from the Linux tpm_tis driver.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com Tested-by: Stephen Douthit stephend@silicom-usa.com
src/hw/tpm_drivers.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 13ad821..da8bb63 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -86,6 +86,11 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) return rc; } +static u32 tis_wait_access(u8 locty, u32 time, u8 mask, u8 expect) +{
- return wait_reg8(TIS_REG(locty, TIS_REG_ACCESS), time, mask, expect);
+}
- static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect) { return wait_reg8(TIS_REG(locty, TIS_REG_STS), time, mask, expect);
@@ -102,7 +107,13 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 rc = 0;
- /* Wait for the interface to report it's ready */
- u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
TIS_ACCESS_TPM_REG_VALID_STS,TIS_ACCESS_TPM_REG_VALID_STS);- if (rc)
return 0;Thanks. I was about to commit this when I noticed the above. In the case where tis_wait_access() fails due to a timeout, shouldn't tis_probe() return an error indicator instead of success?
tis_probe() returns 0 for no device, '1' for device present.
Take a look at the usage in tpmhw_probe(), or the comment on tis_probe() itself.
Okay, thanks for clarifying.
-Kevin
If a device reports 0xf in the InterfaceType field of the TPM_INTERFACE_ID, then the rest of the fields are invalid, and the InterfaceVersion field of the TPM_INTF_CAPABILITY register must be checked instead.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com --- src/hw/tpm_drivers.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index da8bb63..ed58bf5 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -142,13 +142,23 @@ static u32 tis_probe(void)
static TPMVersion tis_get_tpm_version(void) { - /* TPM 2 has an interface register */ - u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID)); - - if ((ifaceid & 0xf) == 0) { - /* TPM 2 */ + u32 reg = readl(TIS_REG(0, TIS_REG_IFACE_ID)); + + /* + * FIFO interface as defined in TIS1.3 is active + * Interface capabilities are defined in TIS_REG_INTF_CAPABILITY + */ + if ((reg & 0xf) == 0xf) { + reg = readl(TIS_REG(0, TIS_REG_INTF_CAPABILITY)); + /* Interface 1.3 for TPM 2.0 */ + if (((reg >> 28) & 0x7) == 3) + return TPM_VERSION_2; + } + /* FIFO interface as defined in PTP for TPM 2.0 is active */ + else if ((reg & 0xf) == 0) { return TPM_VERSION_2; } + return TPM_VERSION_1_2; }
On 02/27/2018 02:17 PM, Stephen Douthit wrote:
If a device reports 0xf in the InterfaceType field of the TPM_INTERFACE_ID, then the rest of the fields are invalid, and the InterfaceVersion field of the TPM_INTF_CAPABILITY register must be checked instead.
Signed-off-by: Stephen Douthit stephend@silicom-usa.com
src/hw/tpm_drivers.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index da8bb63..ed58bf5 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -142,13 +142,23 @@ static u32 tis_probe(void)
static TPMVersion tis_get_tpm_version(void) {
- /* TPM 2 has an interface register */
- u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
- if ((ifaceid & 0xf) == 0) {
/* TPM 2 */
- u32 reg = readl(TIS_REG(0, TIS_REG_IFACE_ID));
- /*
* FIFO interface as defined in TIS1.3 is active* Interface capabilities are defined in TIS_REG_INTF_CAPABILITY*/- if ((reg & 0xf) == 0xf) {
reg = readl(TIS_REG(0, TIS_REG_INTF_CAPABILITY));/* Interface 1.3 for TPM 2.0 */if (((reg >> 28) & 0x7) == 3)return TPM_VERSION_2;- }
- /* FIFO interface as defined in PTP for TPM 2.0 is active */
- else if ((reg & 0xf) == 0) { return TPM_VERSION_2; }
return TPM_VERSION_1_2;
Tested it with QEMU TPM 2 and TPM 1.2:
Tested-by: Stefan Berger stefanb@linux.vnet.ibm.com
Reviewed-by: Stefan Berger stefanb@linux.vnet.ibm.com
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
Changes from v0 -> v1
Patch 1/3 *Rename sts to value
Patch 2/3 *No Change
Patch 3/3 *No longer sending command to the TPM. Instead check the InterfaceVersion field in the TPM_INTF_CAPABILITY register if necessary. *Removed "Tested-by:" since I wasn't able to find 1.2 hardware for regression test.
Thanks. I committed this series.
-Kevin
Dear Stephen, dear Kevin,
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
``` […] CBFS: Locating 'fallback/payload' CBFS: Found @ offset 75840 size 10bfc Loading segment from ROM address 0xffc75a78 code (compression=1) New segment dstaddr 0xe0200 memsize 0x1fe00 srcaddr 0xffc75ab0 filesize 0x10bc4 Loading segment from ROM address 0xffc75a94 Entry Point 0x000fd23d Loading Segment: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4 lb: [0x00000000c7eb3000, 0x00000000c7fb5730) Post relocation: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4 using LZMA [ 0x000e0200, 00100000, 0x00100000) <- ffc75ab0 dest 000e0200, end 00100000, bouncebuffer ffffffff Loaded segments BS: BS_PAYLOAD_LOAD times (us): entry 0 run 95671 exit 0 Jumping to boot code at 000fd23d(c7d77000) CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack used: 2388 bytes SeaBIOS (version rel-1.11.0-25-g5adc8bd) BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1 SeaBIOS (version rel-1.11.0-25-g5adc8bd) BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1 Found coreboot cbmem console @ c7fde000 Found mainboard ASROCK E350M1 Relocating init from 0x000e1840 to 0xc7cf32e0 (size 52352) Found CBFS header at 0xffc00238 multiboot: eax=c7eeab60, ebx=c7eeab14 Found 24 PCI devices (max PCI bus is 03) Copying SMBIOS entry point from 0xc7d40000 to 0x000f6120 Copying ACPI RSDP from 0xc7d51000 to 0x000f60f0 Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f5ef0 Copying PIR from 0xc7d76000 to 0x000f5ec0 Using pmtimer, ioport 0x808 WARNING - Timeout at wait_reg8:81! WARNING - Timeout at wait_reg8:81! WARNING - Timeout at wait_reg8:81! Scan for VGA option rom Running option rom at c000:0003 Turning on vga text mode console SeaBIOS (version rel-1.11.0-25-g5adc8bd) EHCI init on dev 00:12.2 (regs=0xf014c020) EHCI init on dev 00:13.2 (regs=0xf014d020) OHCI init on dev 00:12.0 (regs=0xf0148000) OHCI init on dev 00:13.0 (regs=0xf0149000) OHCI init on dev 00:14.5 (regs=0xf014a000) AHCI controller at 00:11.0, iobase 0xf014b000, irq 0 Found 0 lpt ports Found 1 serial ports Searching bootorder for: /rom@img/memtest Searching bootorder for: /rom@img/tint Searching bootorder for: /rom@img/nvramcui Searching bootorder for: /rom@img/coreinfo USB mouse initialized PS2 keyboard initialized All threads complete. Scan for option roms
Press ESC for boot menu.
WARNING - Timeout at wait_reg8:81! ```
Please find the config file attached.
Is it possible to add better error handling? Or should this option better be disable by default for people running `make olddefconfig`.
Kind regards,
Paul
On 03/06/2018 11:04 AM, Paul Menzel wrote:
Dear Stephen, dear Kevin,
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Thanks, Steve
[…] CBFS: Locating 'fallback/payload' CBFS: Found @ offset 75840 size 10bfc Loading segment from ROM address 0xffc75a78 code (compression=1) New segment dstaddr 0xe0200 memsize 0x1fe00 srcaddr 0xffc75ab0 filesize 0x10bc4 Loading segment from ROM address 0xffc75a94 Entry Point 0x000fd23d Loading Segment: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4 lb: [0x00000000c7eb3000, 0x00000000c7fb5730) Post relocation: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4 using LZMA [ 0x000e0200, 00100000, 0x00100000) <- ffc75ab0 dest 000e0200, end 00100000, bouncebuffer ffffffff Loaded segments BS: BS_PAYLOAD_LOAD times (us): entry 0 run 95671 exit 0 Jumping to boot code at 000fd23d(c7d77000) CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack used: 2388 bytes SeaBIOS (version rel-1.11.0-25-g5adc8bd) BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1 SeaBIOS (version rel-1.11.0-25-g5adc8bd) BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1 Found coreboot cbmem console @ c7fde000 Found mainboard ASROCK E350M1 Relocating init from 0x000e1840 to 0xc7cf32e0 (size 52352) Found CBFS header at 0xffc00238 multiboot: eax=c7eeab60, ebx=c7eeab14 Found 24 PCI devices (max PCI bus is 03) Copying SMBIOS entry point from 0xc7d40000 to 0x000f6120 Copying ACPI RSDP from 0xc7d51000 to 0x000f60f0 Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f5ef0 Copying PIR from 0xc7d76000 to 0x000f5ec0 Using pmtimer, ioport 0x808 WARNING - Timeout at wait_reg8:81! WARNING - Timeout at wait_reg8:81! WARNING - Timeout at wait_reg8:81! Scan for VGA option rom Running option rom at c000:0003 Turning on vga text mode console SeaBIOS (version rel-1.11.0-25-g5adc8bd) EHCI init on dev 00:12.2 (regs=0xf014c020) EHCI init on dev 00:13.2 (regs=0xf014d020) OHCI init on dev 00:12.0 (regs=0xf0148000) OHCI init on dev 00:13.0 (regs=0xf0149000) OHCI init on dev 00:14.5 (regs=0xf014a000) AHCI controller at 00:11.0, iobase 0xf014b000, irq 0 Found 0 lpt ports Found 1 serial ports Searching bootorder for: /rom@img/memtest Searching bootorder for: /rom@img/tint Searching bootorder for: /rom@img/nvramcui Searching bootorder for: /rom@img/coreinfo USB mouse initialized PS2 keyboard initialized All threads complete. Scan for option roms Press ESC for boot menu. WARNING - Timeout at wait_reg8:81!Please find the config file attached.
Is it possible to add better error handling? Or should this option better be disable by default for people running `make olddefconfig`.
Kind regards,
Paul
Dear Stephen,
Thank you for your quick response.
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote:
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
``` $ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version() ```
And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Kind regards,
Paul
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Dear Stephen,
Thank you for your quick response.
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote:
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
Please try the attached patch instead of the 'tpm: Save tis_probe...' and see if that works better.
Thanks, Steve
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Dear Stephen,
Thank you for your quick response.
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote:
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
FYI, even adding 5ms to the boot time is unacceptable. Is there anyway to verify the hardware exists before waiting for it to be ready?
-Kevin
On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Dear Stephen,
Thank you for your quick response.
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote:
On 03/02/18 17:31, Kevin O'Connor wrote:
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
[…]
Thanks. I committed this series.
The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
FYI, even adding 5ms to the boot time is unacceptable. Is there anyway to verify the hardware exists before waiting for it to be ready?
The only way I know of would be to check if we have TCPA or TPM2 ACPI tables, and only attempt to probe for a TPM if those are present.
Attached patch should do that, and it's probably a good idea independent of any of my other patches.
Dear Stephen,
On 03/07/2018 07:24 PM, Stephen Douthit wrote:
On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote:
On 03/02/18 17:31, Kevin O'Connor wrote: > > On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote: […]
> > Thanks. I committed this series. The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
FYI, even adding 5ms to the boot time is unacceptable. Is there anyway to verify the hardware exists before waiting for it to be ready?
The only way I know of would be to check if we have TCPA or TPM2 ACPI tables, and only attempt to probe for a TPM if those are present.
Attached patch should do that, and it's probably a good idea independent of any of my other patches.
I applied both the latest commits, and quickly testing that, I believe the long delay is still there. I won’t be able to get to until next week, and make the ACPI tables available. Maybe there is a way to test this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.
Kind regards,
Paul
On 03/09/2018 06:05 AM, Paul Menzel wrote:
Dear Stephen,
On 03/07/2018 07:24 PM, Stephen Douthit wrote:
On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
On 03/06/2018 11:04 AM, Paul Menzel wrote: > On 03/02/18 17:31, Kevin O'Connor wrote: >> >> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote: > […] > >> >> Thanks. I committed this series. > The second commit introduced a regression with coreboot on the > ASRock E350M1. There are a bunch of time-outs, causing the startup > to be really slow. With no serial console, the user thinks, it’s > not working and start to debug.
Looking through the the user manual for that board I don't see that it has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
Multiple 750ms timeouts does seem pretty painful though. I hadn't considered that tis_probe() would be called multiple times if no TPM was present.
What's the preferred way to have a probe function run and bail before rerunning the timeout? Just put a static flag in tis_probe()? The attached patch takes that approach. Please let me know if that fixes the issue for you, or if there's some other preferred pattern I should use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
FYI, even adding 5ms to the boot time is unacceptable. Is there anyway to verify the hardware exists before waiting for it to be ready?
The only way I know of would be to check if we have TCPA or TPM2 ACPI tables, and only attempt to probe for a TPM if those are present.
Attached patch should do that, and it's probably a good idea independent of any of my other patches.
I applied both the latest commits, and quickly testing that, I believe the long delay is still there. I won’t be able to get to until next week, and make the ACPI tables available. Maybe there is a way to test this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.
Thanks for the continued testing. I don't have a good theory for what's going on at the moment.
It looks like there's a series resistor I can depop to isolate the TPM reset on the board I was testing on. I should be able to jumper that so I can test the TPM and no-TPM cases on the same hardware. Hopefully I can reproduce the timeout that way.
Kevin - Do you want to revert the second patch for now? I'll get a board modded today, and hopefully get a patch out early next week assuming this reproduces on my system.
Apologies for the thrash here.
On 03/09/2018 09:49 AM, Stephen Douthit wrote:
[This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
On 03/09/2018 06:05 AM, Paul Menzel wrote:
Dear Stephen,
On 03/07/2018 07:24 PM, Stephen Douthit wrote:
On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
On 03/07/2018 10:33 AM, Paul Menzel wrote:
Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit: > On 03/06/2018 11:04 AM, Paul Menzel wrote: >> On 03/02/18 17:31, Kevin O'Connor wrote: >>> >>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote: >> […] >> >>> >>> Thanks. I committed this series. >> The second commit introduced a regression with coreboot on the >> ASRock E350M1. There are a bunch of time-outs, causing the startup >> to be really slow. With no serial console, the user thinks, it’s >> not working and start to debug. > > Looking through the the user manual for that board I don't see that it > has a TPM, or even the header for one, so a timeout seems correct.
Indeed, no TPM is present.
Thanks for confirming.
> Multiple 750ms timeouts does seem pretty painful though. I hadn't > considered that tis_probe() would be called multiple times if no TPM > was present. > > What's the preferred way to have a probe function run and bail before > rerunning the timeout? Just put a static flag in tis_probe()? The > attached patch takes that approach. Please let me know if that fixes > the issue for you, or if there's some other preferred pattern I should > use here.
Unfortunately, that didn’t help.
$ git log --oneline -2 fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()And the time-outs seem to be around 20 seconds or more. Please find the log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
Yikes, 20 seconds is the medium duration timeout, not the default A timeout of 750ms. I was poking the wrong area with the last patch. It looks like tis_probe() is propagating the return from tis_wait_access() in the no device present case.
FYI, even adding 5ms to the boot time is unacceptable. Is there anyway to verify the hardware exists before waiting for it to be ready?
The only way I know of would be to check if we have TCPA or TPM2 ACPI tables, and only attempt to probe for a TPM if those are present.
Attached patch should do that, and it's probably a good idea independent of any of my other patches.
I applied both the latest commits, and quickly testing that, I believe the long delay is still there. I won’t be able to get to until next week, and make the ACPI tables available. Maybe there is a way to test this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.
Thanks for the continued testing. I don't have a good theory for what's going on at the moment.
It looks like there's a series resistor I can depop to isolate the TPM reset on the board I was testing on. I should be able to jumper that so I can test the TPM and no-TPM cases on the same hardware. Hopefully I can reproduce the timeout that way.
I've got a board modded so I can jumper the TPM in and out.
What I found in the no-TPM case was that both tis_probe() and crb_probe() incorrectly return 1 for device present if all Fs are read.
For tis_probe() that was because rc wasn't updated to 0 if didvid was 0xffffffff. For crb_probe() the last three return statements are inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware. There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks, Steve
On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
I've got a board modded so I can jumper the TPM in and out.
What I found in the no-TPM case was that both tis_probe() and crb_probe() incorrectly return 1 for device present if all Fs are read.
For tis_probe() that was because rc wasn't updated to 0 if didvid was 0xffffffff. For crb_probe() the last three return statements are inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware. There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks for looking at this. It is common on x86 for invalid memory accesses to return 0xff. I don't know enough about the TPM hardware to make a judgement call on the best way to test for presence. I'd like to hear what Stefan's thoughts are on this.
-Kevin
On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
I've got a board modded so I can jumper the TPM in and out.
What I found in the no-TPM case was that both tis_probe() and crb_probe() incorrectly return 1 for device present if all Fs are read.
For tis_probe() that was because rc wasn't updated to 0 if didvid was 0xffffffff. For crb_probe() the last three return statements are
static u32 tis_probe(void) { if (!CONFIG_TCGBIOS) return 0;
/* Wait for the interface to report it's ready */ u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A, TIS_ACCESS_TPM_REG_VALID_STS, TIS_ACCESS_TPM_REG_VALID_STS); if (rc) return 0;
u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
if ((didvid != 0) && (didvid != 0xffffffff)) rc = 1;
/* TPM 2 has an interface register */ u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active; no TIS */ return 0; } if ((ifaceid & (1 << 13)) == 0) { /* TIS cannot be selected */ return 0; } /* write of 0 to bits 17-18 selects TIS */ writel(TIS_REG(0, TIS_REG_IFACE_ID), 0); /* since we only support TIS, we lock it */ writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); }
return rc; }
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */ return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); }
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1;
return 0; }
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware.
That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks for looking at this. It is common on x86 for invalid memory accesses to return 0xff. I don't know enough about the TPM hardware to make a judgement call on the best way to test for presence. I'd like to hear what Stefan's thoughts are on this.
-Kevin
On 03/13/2018 07:31 AM, Stefan Berger wrote:
On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
I've got a board modded so I can jumper the TPM in and out.
What I found in the no-TPM case was that both tis_probe() and crb_probe() incorrectly return 1 for device present if all Fs are read.
For tis_probe() that was because rc wasn't updated to 0 if didvid was 0xffffffff. For crb_probe() the last three return statements are
static u32 tis_probe(void) { if (!CONFIG_TCGBIOS) return 0;
/* Wait for the interface to report it's ready */ u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A, TIS_ACCESS_TPM_REG_VALID_STS, TIS_ACCESS_TPM_REG_VALID_STS); if (rc) return 0;
Can we do without this tis_wait_access on real hardware? Presumably it only affects the STS register. If so...
u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID)); if ((didvid != 0) && (didvid != 0xffffffff)) rc = 1;
... then we could have an else branch here and 'return 0'.
/* TPM 2 has an interface register */ u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID)); if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active; no TIS */ return 0; } if ((ifaceid & (1 << 13)) == 0) { /* TIS cannot be selected */ return 0; } /* write of 0 to bits 17-18 selects TIS */ writel(TIS_REG(0, TIS_REG_IFACE_ID), 0); /* since we only support TIS, we lock it */ writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); } return rc;}
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID)); if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */ return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); } /* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1; u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1; return 0;}
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware.
That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks for looking at this. It is common on x86 for invalid memory accesses to return 0xff. I don't know enough about the TPM hardware to make a judgement call on the best way to test for presence. I'd like to hear what Stefan's thoughts are on this.
-Kevin
SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
On 03/13/2018 07:39 AM, Stefan Berger wrote:
On 03/13/2018 07:31 AM, Stefan Berger wrote:
On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
I've got a board modded so I can jumper the TPM in and out.
What I found in the no-TPM case was that both tis_probe() and crb_probe() incorrectly return 1 for device present if all Fs are read.
For tis_probe() that was because rc wasn't updated to 0 if didvid was 0xffffffff. For crb_probe() the last three return statements are
static u32 tis_probe(void) { if (!CONFIG_TCGBIOS) return 0;
/* Wait for the interface to report it's ready */ u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A, TIS_ACCESS_TPM_REG_VALID_STS, TIS_ACCESS_TPM_REG_VALID_STS); if (rc) return 0;
Can we do without this tis_wait_access on real hardware? Presumably it only affects the STS register. If so...
The reset timing chapter of the spec implies that you need to check this bit to know when the TPM has completed it's initialization after reset is released.
u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
if ((didvid != 0) && (didvid != 0xffffffff)) rc = 1;
... then we could have an else branch here and 'return 0'.
/* TPM 2 has an interface register */ u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active; no TIS */ return 0; } if ((ifaceid & (1 << 13)) == 0) { /* TIS cannot be selected */ return 0; } /* write of 0 to bits 17-18 selects TIS */ writel(TIS_REG(0, TIS_REG_IFACE_ID), 0); /* since we only support TIS, we lock it */ writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); }
return rc; }
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */ return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); }
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1;
return 0; }
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware.
That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks for looking at this. It is common on x86 for invalid memory accesses to return 0xff. I don't know enough about the TPM hardware to make a judgement call on the best way to test for presence. I'd like to hear what Stefan's thoughts are on this.
-Kevin
SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
Sounds good, I assumed I had screwed this up based on Paul's original bug report, and was still carrying the patch for my 'fix' to this function when I was doing my testing.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */
We also return here without doing the 64 bit address check.
return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); }
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1;
return 0; }
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
The old parallel PCI bus referred to the termination of any access to a bad address where no device responded as 'Master Abort'. That's where I got the aborted read term from. As Kevin mentioned, on x86 this case normally results in a read of all Fs.
The reason I want to move to Seize (or something like it) instead of didvid is that the spec actually says that _must_ always return '0', I don't see any explicit restrictions on didvid.
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware.
That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
For x86 if there's no device then there is no wait. The valid flag is still set in what I was calling the abort case. In this case it's not returned by a device, but by the bus read logic.
With the ACPI check reorder we only have to wait in the following scenario.
1) CONFIG_TPM is set 2) There's a bogus TCPA/TPM2 table for a device that doesn't exist. 3) There's no TPM 4) We're on a platform that returns 0 for aborted reads, so we poll for a tpmRegValidSts that will never be set.
There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could use as a sanity check against the no device all Fs case.
Let me know if that sounds like a better way to catch the no device case, or if there's is some other check that would be better.
Thanks for looking at this. It is common on x86 for invalid memory accesses to return 0xff. I don't know enough about the TPM hardware to make a judgement call on the best way to test for presence. I'd like to hear what Stefan's thoughts are on this.
-Kevin
On 03/13/2018 10:15 AM, Stephen Douthit wrote:
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
Sounds good, I assumed I had screwed this up based on Paul's original bug report, and was still carrying the patch for my 'fix' to this function when I was doing my testing.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID)); if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */We also return here without doing the 64 bit address check.
Right...
return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); } /* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1; u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1; return 0;}
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
The old parallel PCI bus referred to the termination of any access to a bad address where no device responded as 'Master Abort'. That's where I got the aborted read term from. As Kevin mentioned, on x86 this case normally results in a read of all Fs.
The reason I want to move to Seize (or something like it) instead of didvid is that the spec actually says that _must_ always return '0', I don't see any explicit restrictions on didvid.
We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Regis...
So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
I think that should be a first test, maybe also for CRB.
Stefan
On 03/13/2018 10:40 AM, Stefan Berger wrote:
On 03/13/2018 10:15 AM, Stephen Douthit wrote:
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
Sounds good, I assumed I had screwed this up based on Paul's original bug report, and was still carrying the patch for my 'fix' to this function when I was doing my testing.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */
We also return here without doing the 64 bit address check.
Right...
return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); }
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1;
return 0; }
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
The old parallel PCI bus referred to the termination of any access to a bad address where no device responded as 'Master Abort'. That's where I got the aborted read term from. As Kevin mentioned, on x86 this case normally results in a read of all Fs.
The reason I want to move to Seize (or something like it) instead of didvid is that the spec actually says that _must_ always return '0', I don't see any explicit restrictions on didvid.
We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Regis...
So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
It's fine with the caveat that no vendor in the future is ever assigned 0xffff or 0x0000. It's a low risk, but not a no risk scenario.
I think that should be a first test, maybe also for CRB.
I don't think we can make that the first test. If we don't wait for tpmRegValidSts (qualified by some known zero bit), then we can't tell the difference between no-TPM, and reading before the device is ready.
Note 2 in section 6.6 of the TIS 1.3 spec:
2. Within 30 milliseconds of the completion of TPM_Init: a. All fields within the access register and all other registers MUST return with the state of all their fields valid (i.e. TPM_ACCESS_x.tpmRegValidSts is set to ‘1’). b. The TPM MUST be ready to receive a command
Maybe the best thing to do is try to skip probing entirely based if we don't see a related ACPI table. If we do probe, and have to wait in the corrected wait code where we qualify tpmRegValidSts against vendor ID or Seize, and we still find no device, we should probably print a message to the user.
I don't see a way around some potential wait if we want to support real hardware devices reliably.
On 03/13/2018 10:40 AM, Stefan Berger wrote:
On 03/13/2018 10:15 AM, Stephen Douthit wrote:
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
Sounds good, I assumed I had screwed this up based on Paul's original bug report, and was still carrying the patch for my 'fix' to this function when I was doing my testing.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */
We also return here without doing the 64 bit address check.
Right...
return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); }
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1;
return 0; }
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
The old parallel PCI bus referred to the termination of any access to a bad address where no device responded as 'Master Abort'. That's where I got the aborted read term from. As Kevin mentioned, on x86 this case normally results in a read of all Fs.
The reason I want to move to Seize (or something like it) instead of didvid is that the spec actually says that _must_ always return '0', I don't see any explicit restrictions on didvid.
We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Regis...
So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
It's fine with the caveat that no vendor in the future is ever assigned 0xffff or 0x0000. It's a low risk, but not a no risk scenario.
I think that should be a first test, maybe also for CRB.
I don't think we can make that the first test. If we don't wait for tpmRegValidSts (qualified by some known zero bit), then we can't tell the difference between no-TPM, and reading before the device is ready.
Note 2 in section 6.6 of the TIS 1.3 spec:
- Within 30 milliseconds of the completion of TPM_Init:
a. All fields within the access register and all other registers MUST return with the state of all their fields valid (i.e. TPM_ACCESS_x.tpmRegValidSts is set to ‘1’). b. The TPM MUST be ready to receive a command
Maybe the best thing to do is try to skip probing entirely based if we don't see a related ACPI table. If we do probe, and have to wait in the corrected wait code where we qualify tpmRegValidSts against vendor ID or Seize, and we still find no device, we should probably print a message to the user.
I don't see a way around some potential wait if we want to support real hardware devices reliably.
Or maybe we could skip polling on QEMU based on CONFIG_QEMU?
On Tue, Mar 13, 2018 at 11:36:25AM -0400, Stephen Douthit wrote:
On 03/13/2018 10:40 AM, Stefan Berger wrote:
I think that should be a first test, maybe also for CRB.
I don't think we can make that the first test. If we don't wait for tpmRegValidSts (qualified by some known zero bit), then we can't tell the difference between no-TPM, and reading before the device is ready.
Note 2 in section 6.6 of the TIS 1.3 spec:
- Within 30 milliseconds of the completion of TPM_Init:
a. All fields within the access register and all other registers MUST return with the state of all their fields valid (i.e. TPM_ACCESS_x.tpmRegValidSts is set to ‘1’). b. The TPM MUST be ready to receive a command
I'm not sure of the specifics with the TPM. But, as a general rule of thumb, the SeaBIOS code can assume it's been over 30ms since power was turned on to the machine.
-Kevin
On 03/13/2018 11:36 AM, Stephen Douthit wrote:
On 03/13/2018 10:40 AM, Stefan Berger wrote:
On 03/13/2018 10:15 AM, Stephen Douthit wrote:
When tis_probe() returns '1', it means the interface was detected. If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
Sounds good, I assumed I had screwed this up based on Paul's original bug report, and was still carrying the patch for my 'fix' to this function when I was doing my testing.
static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID)); if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */We also return here without doing the 64 bit address check.
Right...
return 1; } if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; } /* write of 1 to bits 17-18 selects CRB */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17)); /* lock it */ writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19)); } /* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) return 1; u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) return 1; return 0;}
if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
inverted from what they should be, and the first 64bit address check returned the wrong value. Fixing both probe functions got rid of the timeout for me when the TPM was disconnected.
I see a problem with the crb_probe function but not the tis_probe.
It looks like there's a bit in the ACCESS register called Seize that must always read '0' for the version 1.2/1.3 interfaces. I'd like to check that instead of didvid in tis_probe to handle the aborted read all 0s/Fs case.
Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
The old parallel PCI bus referred to the termination of any access to a bad address where no device responded as 'Master Abort'. That's where I got the aborted read term from. As Kevin mentioned, on x86 this case normally results in a read of all Fs.
The reason I want to move to Seize (or something like it) instead of didvid is that the spec actually says that _must_ always return '0', I don't see any explicit restrictions on didvid.
We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Regis...
So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
It's fine with the caveat that no vendor in the future is ever assigned 0xffff or 0x0000. It's a low risk, but not a no risk scenario.
I think that should be a first test, maybe also for CRB.
I don't think we can make that the first test. If we don't wait for tpmRegValidSts (qualified by some known zero bit), then we can't tell the difference between no-TPM, and reading before the device is ready.
Fine, then we add the wait for valid STS register check before that.
Note 2 in section 6.6 of the TIS 1.3 spec:
- Within 30 milliseconds of the completion of TPM_Init:
a. All fields within the access register and all other registers MUST return with the state of all their fields valid (i.e. TPM_ACCESS_x.tpmRegValidSts is set to ‘1’). b. The TPM MUST be ready to receive a command
Maybe the best thing to do is try to skip probing entirely based if we don't see a related ACPI table. If we do probe, and have to wait in the corrected wait code where we qualify tpmRegValidSts against vendor ID or Seize, and we still find no device, we should probably print a message to the user.
We cannot get rid of the probing entirely since we do have failure handling in QEMU that disables the interface (by returning 0 on all registers).
I don't see a way around some potential wait if we want to support real hardware devices reliably.
On 03/12/2018 01:38 PM, Stephen Douthit wrote:
I'd like to add a poll for tpmRegValidSts to crb_probe() similar to what's in tis_probe() to avoid potential races on real hardware.
The QEMU CRB currently does not set this bit until access is requested by writing bit 0 to TPM_LOC_CTRL_x.
The spec is a bit ambiguous about this bit and says 'The TPM SHALL NOT set TPM_LOC_STATE_x.tpmRegValidSts to 1 unless all other fields are valid' (description near Table 24 of TCG PC Client Platform TPM Profile (PTP) Specification). What makes all the other fields valid? Do you know whether your hardware has this bit set to '1' at this point ? I can add an initialization to QEMU that sets this bit to '1' as well, but if your hardware doesn't have it to '1' I'd rather not do it but do the following:
- request access by writing '1' to TPM_LOC_CTRL - checking whether this bit is now '1'
The code may be a bit confusing and driven by our QEMU implementation.
Stefan
-
Kevin O'Connor -
Paul Menzel -
Stefan Berger -
Stephen Douthit