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) {
--
2.5.5
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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) {
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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) {
>>
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
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;
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
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
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
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
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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;
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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;
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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;
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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;
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
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;
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
© 2016 - 2025 Red Hat, Inc.