Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stefan Berger]

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 0x 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 > 0x)
 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 0x nor a 0x vendor -- 
assuming the list is complete.


https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf 



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
0x or 0x.  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:

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.


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.




___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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 0x 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 > 0x)
 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 0x nor a 0x vendor -- assuming the list is 
complete.

https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf

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
0x or 0x.  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.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

When tis_probe() returns '1', it means the interface was detected.
If all registers return 0x 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 > 0x)
     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






___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stefan Berger]

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
0x.  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 != 0x))
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 0x 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 > 0x)
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

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stefan Berger]

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
0x.  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 != 0x))
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 0x 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 > 0x)
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

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Kevin O'Connor]

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
> 0x.  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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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
0x.  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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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.

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Paul Menzel]

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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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.
>From f02f25e56915727e761dc7b8b0f12321441e8537 Mon Sep 17 00:00:00 2001
From: Stephen Douthit 
Date: Wed, 7 Mar 2018 13:17:36 -0500
Subject: [PATCH] tpm: Check for TPM related ACPI tables before attempting hw
 probe

Signed-off-by: Stephen Douthit 
---
 src/tcgbios.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/tcgbios.c b/src/tcgbios.c
index 40b3028..24846d3 100644
--- a/src/tcgbios.c
+++ b/src/tcgbios.c
@@ -968,6 +968,13 @@ tpm_setup(void)
 if (!CONFIG_TCGBIOS)
 return;
 
+int ret = tpm_tpm2_probe();
+if (ret) {
+ret = tpm_tcpa_probe();
+if (ret)
+return;
+}
+
 TPM_version = tpmhw_probe();
 if (TPM_version == TPM_VERSION_NONE)
 return;
@@ -976,13 +983,6 @@ tpm_setup(void)
 "TCGBIOS: Detected a TPM %s.\n",
  (TPM_version == TPM_VERSION_1_2) ? "1.2" : "2");
 
-int ret = tpm_tpm2_probe();
-if (ret) {
-ret = tpm_tcpa_probe();
-if (ret)
-return;
-}
-
 TPM_working = 1;
 
 if (runningOnXen())
-- 
2.14.3

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Kevin O'Connor]

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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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


>From dea29b2d87b7b740d3e5abad0ee72c3a01ae5b2e Mon Sep 17 00:00:00 2001
From: Stephen Douthit 
Date: Wed, 7 Mar 2018 12:26:21 -0500
Subject: [PATCH] tpm: Don't propagate return from tis_wait_access() in
 tis_probe() if no TPM

Signed-off-by: Stephen Douthit 
---
 src/hw/tpm_drivers.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
index ed58bf5..9a52558 100644
--- a/src/hw/tpm_drivers.c
+++ b/src/hw/tpm_drivers.c
@@ -118,6 +118,8 @@ static u32 tis_probe(void)
 
 if ((didvid != 0) && (didvid != 0x))
 rc = 1;
+else
+rc = 0;
 
 /* TPM 2 has an interface register */
 u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
-- 
2.14.3

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios