This series of patches attempts to fix the probing of the CRB interface for real hardware.
Stephen Douthit tested the changes on real hardware and it seems to fix an existing problem.
Stefan
v1->v2: - test LOC_STATE register for set tpmRegValidSts flag and unset locAssigned flag without writing to any other registers
Stefan Berger (3): tpm: Wait for tpmRegValidSts flag on CRQ interface before probing tpm: revert return values for successful/failed CRB probing tpm: when CRB is active, select, lock it, and check addresses
src/hw/tpm_drivers.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
Real hardware seems to set the tpmRegValidSts flag without for example requesting access to a locality.
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com --- src/hw/tpm_drivers.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index ed58bf5..7e6a96a 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -374,12 +374,22 @@ static u32 tis_waitrespready(enum tpmDurationType to_t) return rc; }
+#define CRB_STATE_VALID_STS 0b10000000 +#define CRB_STATE_LOC_ASSIGNED 0x00000010 +#define CRB_STATE_READY_MASK (CRB_STATE_VALID_STS | CRB_STATE_LOC_ASSIGNED) + /* if device is not there, return '0', '1' otherwise */ static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
+ /* Wait for the interface to report it's ready */ + u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D, + CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); + if (rc) + return 0; + u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) {
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Real hardware seems to set the tpmRegValidSts flag without for example requesting access to a locality.
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com
src/hw/tpm_drivers.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index ed58bf5..7e6a96a 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -374,12 +374,22 @@ static u32 tis_waitrespready(enum tpmDurationType to_t) return rc; }
+#define CRB_STATE_VALID_STS 0b10000000 +#define CRB_STATE_LOC_ASSIGNED 0x00000010 +#define CRB_STATE_READY_MASK (CRB_STATE_VALID_STS | CRB_STATE_LOC_ASSIGNED)
/* if device is not there, return '0', '1' otherwise */ static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
/* Wait for the interface to report it's ready */
u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D,
CRB_STATE_READY_MASK, CRB_STATE_VALID_STS);if (rc)
return 0;u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID)); if ((ifaceid & 0xf) != 0xf) {
On 03/19/2018 12:23 PM, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
gee... CRQ is a ppc64 thing.
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Thanks.
Stefan
Real hardware seems to set the tpmRegValidSts flag without for example requesting access to a locality.
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com
src/hw/tpm_drivers.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index ed58bf5..7e6a96a 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -374,12 +374,22 @@ static u32 tis_waitrespready(enum tpmDurationType to_t) return rc; } +#define CRB_STATE_VALID_STS 0b10000000 +#define CRB_STATE_LOC_ASSIGNED 0x00000010 +#define CRB_STATE_READY_MASK (CRB_STATE_VALID_STS | CRB_STATE_LOC_ASSIGNED)
- /* if device is not there, return '0', '1' otherwise */ static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0;
- /* Wait for the interface to report it's ready */
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D,
CRB_STATE_READY_MASK, CRB_STATE_VALID_STS);- if (rc)
return 0;u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID)); if ((ifaceid & 0xf) != 0xf) {
Dear Stephen,
On 03/19/18 17:23, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Do you have a setup, where you can measure the times? What times do you see?
Kind regards,
Paul
On 03/19/2018 12:36 PM, Paul Menzel wrote:
Dear Stephen,
On 03/19/18 17:23, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Do you have a setup, where you can measure the times? What times do you see?
Even in the timeout case it's shorter than the timer interrupt tick interval on my board. 'ticks' reads 0 for both the present and absent case on my board.
diff --git a/src/tcgbios.c b/src/tcgbios.c index 40b3028..056c412 100644 --- a/src/tcgbios.c +++ b/src/tcgbios.c @@ -968,7 +968,10 @@ tpm_setup(void) if (!CONFIG_TCGBIOS) return;
+ u32 start = irqtimer_calc(0); TPM_version = tpmhw_probe(); + u32 ticks = irqtimer_calc(0) - start; + dprintf(1, "TCGBIOS: tpmhw_probe completed in %d ticks (%d ms)\n", ticks, ticks_to_ms(ticks)); if (TPM_version == TPM_VERSION_NONE) return;
On Mon, Mar 19, 2018 at 12:23:10PM -0400, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Thanks. I committed this series.
Does this resolve the pause at startup for non-TPM users?
-Kevin
On 03/21/2018 10:38 AM, Kevin O'Connor wrote:
On Mon, Mar 19, 2018 at 12:23:10PM -0400, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Thanks. I committed this series.
Does this resolve the pause at startup for non-TPM users?
It should. I removed that write() statement you commented on and that was necessary to run before the read() due to an implementation mistake in QEMU. Maybe Paul can confirm with the latest tip.
Stefan
-Kevin
Dear Kevin,
On 03/21/18 15:38, Kevin O'Connor wrote:
On Mon, Mar 19, 2018 at 12:23:10PM -0400, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Thanks. I committed this series.
Does this resolve the pause at startup for non-TPM users?
I booted the latest master branch this morning on the ASRock E350M1, and a possible delay wasn’t noticeable by me. But looking at the log, there is still one time-out messages. Maybe I’ll have time to measure it next week.
``` BS: BS_PAYLOAD_LOAD times (us): entry 0 run 32666 exit 0 Jumping to boot code at 000fec22(c7d77000) CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack used: 2388 bytes SeaBIOS (version rel-1.11.0-28-g4922d6c) 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 0x000e1e40 to 0xc7cf3300 (size 52320) Found CBFS header at 0xffc00238 multiboot: eax=c7eea9a0, ebx=c7eea954 Found 24 PCI devices (max PCI bus is 03) Copying SMBIOS entry point from 0xc7d40000 to 0x000f6620 Copying ACPI RSDP from 0xc7d51000 to 0x000f65f0 Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f63f0 Copying PIR from 0xc7d76000 to 0x000f63c0 Using pmtimer, ioport 0x808 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-28-g4922d6c) 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 Searching bootorder for: /pci@i0cf8/*@11/drive@0/disk@0 AHCI/0: Set transfer mode to UDMA-6 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 AHCI/0: registering: "AHCI/0: SanDisk SDSSDP064G ATA-9 Hard-Disk (61057 MiBytes)" USB mouse initialized PS2 keyboard initialized All threads complete. Scan for option roms
Press ESC for boot menu. ```
Maybe the time-out warning should be also rephrased to be better understandable, and a context given, that no TPM is found or set up.
Kind regards,
Paul
On 03/22/2018 06:57 AM, Paul Menzel wrote:
Dear Kevin,
On 03/21/18 15:38, Kevin O'Connor wrote:
On Mon, Mar 19, 2018 at 12:23:10PM -0400, Stephen Douthit wrote:
On 03/19/2018 12:00 PM, Stefan Berger wrote:
Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register to be set; we espect the locAssigned flag to not be set.
s/espect/expect/ and in the subject line s/CRQ/CRB
Just retested the v2 series on my board so: Tested-by: Stephen Douthit stephend@silicom-usa.com
Thanks. I committed this series.
Does this resolve the pause at startup for non-TPM users?
I booted the latest master branch this morning on the ASRock E350M1, and a possible delay wasn’t noticeable by me. But looking at the log, there is still one time-out messages. Maybe I’ll have time to measure it next week.
BS: BS_PAYLOAD_LOAD times (us): entry 0 run 32666 exit 0 Jumping to boot code at 000fec22(c7d77000) CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack used: 2388 bytes SeaBIOS (version rel-1.11.0-28-g4922d6c) 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 0x000e1e40 to 0xc7cf3300 (size 52320) Found CBFS header at 0xffc00238 multiboot: eax=c7eea9a0, ebx=c7eea954 Found 24 PCI devices (max PCI bus is 03) Copying SMBIOS entry point from 0xc7d40000 to 0x000f6620 Copying ACPI RSDP from 0xc7d51000 to 0x000f65f0 Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f63f0 Copying PIR from 0xc7d76000 to 0x000f63c0 Using pmtimer, ioport 0x808 WARNING - Timeout at wait_reg8:81!
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Stefan
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
-Kevin
--- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -78,7 +78,8 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) break; } if (timer_check(end)) { - warn_timeout(); + if (time) + warn_timeout(); break; } yield(); @@ -108,7 +109,7 @@ static u32 tis_probe(void) return 0;
/* Wait for the interface to report it's ready */ - u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A, + u32 rc = tis_wait_access(0, 0, TIS_ACCESS_TPM_REG_VALID_STS, TIS_ACCESS_TPM_REG_VALID_STS); if (rc) @@ -385,7 +386,7 @@ static u32 crb_probe(void) return 0;
/* Wait for the interface to report it's ready */ - u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D, + u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, 0, CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); if (rc) return 0;
On 03/25/2018 11:45 AM, Kevin O'Connor wrote:
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
I had thought about something like that also. Do we have the time in milliseconds since power-on or reset? We could subtract the timeout values you are discarding below from it and wait for the time difference, ignoring negative values of course. TIS_DEFAULT_TIMEOUT_A is 750ms, so more critical than TIS2_DEFAULT_TIMEOUT_D with 30ms.
-Kevin
--- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -78,7 +78,8 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) break; } if (timer_check(end)) {
warn_timeout();
if (time)warn_timeout(); break; } yield();@@ -108,7 +109,7 @@ static u32 tis_probe(void) return 0;
/* Wait for the interface to report it's ready */
- u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
- u32 rc = tis_wait_access(0, 0, TIS_ACCESS_TPM_REG_VALID_STS, TIS_ACCESS_TPM_REG_VALID_STS); if (rc)
@@ -385,7 +386,7 @@ static u32 crb_probe(void) return 0;
/* Wait for the interface to report it's ready */
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D,
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, 0, CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); if (rc) return 0;
On Sun, Mar 25, 2018 at 07:17:33PM -0400, Stefan Berger wrote:
On 03/25/2018 11:45 AM, Kevin O'Connor wrote:
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
I had thought about something like that also. Do we have the time in milliseconds since power-on or reset? We could subtract the timeout values you are discarding below from it and wait for the time difference, ignoring negative values of course. TIS_DEFAULT_TIMEOUT_A is 750ms, so more critical than TIS2_DEFAULT_TIMEOUT_D with 30ms.
Unfortunately, we don't have that info. It's not easy to get it because the clock detection code doesn't run until we're nearly at the tpmhw_probe() code anyway.
That said, for tis_probe() is there any harm in moving the didvid check up? I understand it theoretically isn't setup yet, but I'd imagine in practice it always would be. Just as, in practice, I suspect the tpm would always be ready by the time we got to the tpmhw_probe() code. See below.
-Kevin
--- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -78,7 +78,8 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) break; } if (timer_check(end)) { - warn_timeout(); + if (time) + warn_timeout(); break; } yield(); @@ -107,6 +108,10 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
+ u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID)); + if (didvid == 0 || didvid == 0xffffffff) + 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, @@ -114,11 +119,6 @@ static u32 tis_probe(void) 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));
@@ -137,7 +137,7 @@ static u32 tis_probe(void) writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); }
- return rc; + return 1; }
static TPMVersion tis_get_tpm_version(void) @@ -385,7 +385,7 @@ static u32 crb_probe(void) return 0;
/* Wait for the interface to report it's ready */ - u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D, + u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, 0, CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); if (rc) return 0;
On 03/25/2018 07:46 PM, Kevin O'Connor wrote:
On Sun, Mar 25, 2018 at 07:17:33PM -0400, Stefan Berger wrote:
On 03/25/2018 11:45 AM, Kevin O'Connor wrote:
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
I had thought about something like that also. Do we have the time in milliseconds since power-on or reset? We could subtract the timeout values you are discarding below from it and wait for the time difference, ignoring negative values of course. TIS_DEFAULT_TIMEOUT_A is 750ms, so more critical than TIS2_DEFAULT_TIMEOUT_D with 30ms.
Unfortunately, we don't have that info. It's not easy to get it because the clock detection code doesn't run until we're nearly at the tpmhw_probe() code anyway.
That said, for tis_probe() is there any harm in moving the didvid check up? I understand it theoretically isn't setup yet, but I'd imagine in practice it always would be. Just as, in practice, I suspect the tpm would always be ready by the time we got to the tpmhw_probe() code. See below.
It's hard to say without testing this with real hardware. When I had the Acer, it was working quite reliably without waiting for the flag to be set. Though the Acer only used one certain manufacturer's TPM. Other TPMs may behave differently. Maybe Stephen can give it a try?
Stefan
-Kevin
--- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -78,7 +78,8 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) break; } if (timer_check(end)) {
warn_timeout();
if (time)warn_timeout(); break; } yield();@@ -107,6 +108,10 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
- if (didvid == 0 || didvid == 0xffffffff)
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,@@ -114,11 +119,6 @@ static u32 tis_probe(void) 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));@@ -137,7 +137,7 @@ static u32 tis_probe(void) writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); }
- return rc;
return 1; }
static TPMVersion tis_get_tpm_version(void)
@@ -385,7 +385,7 @@ static u32 crb_probe(void) return 0;
/* Wait for the interface to report it's ready */
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D,
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, 0, CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); if (rc) return 0;
Dear Stefan,
On 03/26/18 12:44, Stefan Berger wrote:
On 03/25/2018 07:46 PM, Kevin O'Connor wrote:
On Sun, Mar 25, 2018 at 07:17:33PM -0400, Stefan Berger wrote:
On 03/25/2018 11:45 AM, Kevin O'Connor wrote:
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
I had thought about something like that also. Do we have the time in milliseconds since power-on or reset? We could subtract the timeout values you are discarding below from it and wait for the time difference, ignoring negative values of course. TIS_DEFAULT_TIMEOUT_A is 750ms, so more critical than TIS2_DEFAULT_TIMEOUT_D with 30ms.
Unfortunately, we don't have that info. It's not easy to get it because the clock detection code doesn't run until we're nearly at the tpmhw_probe() code anyway.
That said, for tis_probe() is there any harm in moving the didvid check up? I understand it theoretically isn't setup yet, but I'd imagine in practice it always would be. Just as, in practice, I suspect the tpm would always be ready by the time we got to the tpmhw_probe() code. See below.
It's hard to say without testing this with real hardware. When I had the Acer, it was working quite reliably without waiting for the flag to be set. Though the Acer only used one certain manufacturer's TPM. Other TPMs may behave differently. Maybe Stephen can give it a try?
Can’t you get some hardware to test this? For example an old laptop with a TPM? If you work in this area, that might be a good idea.
Kind regards,
Paul
On 03/25/2018 07:46 PM, Kevin O'Connor wrote:
On Sun, Mar 25, 2018 at 07:17:33PM -0400, Stefan Berger wrote:
On 03/25/2018 11:45 AM, Kevin O'Connor wrote:
On Thu, Mar 22, 2018 at 08:19:09AM -0400, Stefan Berger wrote:
The timeout to wait for the register change is 30ms. We yield() while waiting, so we don't block everything entirely... Is the error message misleading and we should print out that a device was not detected or print out if it is detected instead?
Unfortunately, although the TPM code calls yield(), there isn't anything else running at that point, so the delay still directly impacts the total boot time. It's not easy to push back the TPM initialization so that other "threads" are running in parallel, because the TPM code wants to be initialized prior to running option roms and other devices.
Could we do something like the below (completely untested)? I don't think we have to wait for the TPM device to report ready, because in a real world scenario it would take an x86 cpu hundreds of milliseconds from power on to get to this point of the code anyway.
I had thought about something like that also. Do we have the time in milliseconds since power-on or reset? We could subtract the timeout values you are discarding below from it and wait for the time difference, ignoring negative values of course. TIS_DEFAULT_TIMEOUT_A is 750ms, so more critical than TIS2_DEFAULT_TIMEOUT_D with 30ms.
Unfortunately, we don't have that info. It's not easy to get it because the clock detection code doesn't run until we're nearly at the tpmhw_probe() code anyway.
That said, for tis_probe() is there any harm in moving the didvid check up? I understand it theoretically isn't setup yet, but I'd imagine in practice it always would be. Just as, in practice, I suspect the tpm would always be ready by the time we got to the tpmhw_probe() code. See below.
-Kevin
--- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -78,7 +78,8 @@ static u32 wait_reg8(u8* reg, u32 time, u8 mask, u8 expect) break; } if (timer_check(end)) {
warn_timeout();
if (time)warn_timeout(); break; } yield();@@ -107,6 +108,10 @@ static u32 tis_probe(void) if (!CONFIG_TCGBIOS) return 0;
- u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
- if (didvid == 0 || didvid == 0xffffffff)
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,@@ -114,11 +119,6 @@ static u32 tis_probe(void) if (rc) return 0;
- u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
- if ((didvid != 0) && (didvid != 0xffffffff))
rc = 1;
The didvid worked well for TPM 1.2. I think we should duplicate the reading of it, once before the tis_wait_access and keep it after.
Stefan
/* TPM 2 has an interface register */ u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));@@ -137,7 +137,7 @@ static u32 tis_probe(void) writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19)); }
- return rc;
return 1; }
static TPMVersion tis_get_tpm_version(void)
@@ -385,7 +385,7 @@ static u32 crb_probe(void) return 0;
/* Wait for the interface to report it's ready */
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D,
- u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, 0, CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); if (rc) return 0;
The return values for successful/failed CRB probing were reverted. Fix it.
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com --- src/hw/tpm_drivers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 7e6a96a..271f8d3 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -409,13 +409,13 @@ static u32 crb_probe(void)
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) - return 1; + return 0;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) - return 1; + return 0;
- return 0; + return 1; }
static TPMVersion crb_get_tpm_version(void)
Do not just indicate that the probing for the CRB interface was successful if we find it active. Instead, select it, lock it, and test the addresses for whether they can be used (must be 32 bit).
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com --- src/hw/tpm_drivers.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index 271f8d3..bd971f7 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -395,9 +395,7 @@ static u32 crb_probe(void) if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */ - return 1; - } - if ((ifaceid & (1 << 14)) == 0) { + } else if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; }
-
Kevin O'Connor -
Paul Menzel -
Stefan Berger -
Stephen Douthit