[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 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
>>
>> Sounds good, I assumed I had screwed this up based on Paul's original
>> bug report, and was still carrying the patch for my 'fix' to this
>> function when I was doing my testing.
>>
>>> static u32 crb_probe(void)
>>> {
>>>      if (!CONFIG_TCGBIOS)
>>>          return 0;
>>>
>>>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>>
>>>      if ((ifaceid & 0xf) != 0xf) {
>>>          if ((ifaceid & 0xf) == 1) {
>>>              /* CRB is active */
>>
>> We also return here without doing the 64 bit address check.
> 
> Right...
> 
>>
>>>              return 1;
>>>          }
>>>          if ((ifaceid & (1 << 14)) == 0) {
>>>              /* CRB cannot be selected */
>>>              return 0;
>>>          }
>>>          /* write of 1 to bits 17-18 selects CRB */
>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>>          /* lock it */
>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>>      }
>>>
>>>      /* no support for 64 bit addressing yet */
>>>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>>          return 1;
>>>
>>>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>>      if (addr > 0xffffffff)
>>>          return 1;
>>>
>>>      return 0;
>>> }
>>>
>>> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
>>>
>>>
>>>
>>>>> inverted from what they should be, and the first 64bit address check
>>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>>> timeout for me when the TPM was disconnected.
>>>
>>> I see a problem with the crb_probe function but not the tis_probe.
>>>
>>>>>
>>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>>> must always read '0' for the version 1.2/1.3 interfaces. I'd like to
>>>>> check that instead of didvid in tis_probe to handle the aborted read all
>>>>> 0s/Fs case.
>>>
>>> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
>>
>> The old parallel PCI bus referred to the termination of any access to a
>> bad address where no device responded as 'Master Abort'.  That's where I
>> got the aborted read term from.  As Kevin mentioned, on x86 this case
>> normally results in a read of all Fs.
>>
>> The reason I want to move to Seize (or something like it) instead of
>> didvid is that the spec actually says that _must_ always return '0', I
>> don't see any explicit restrictions on didvid.
> 
> We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
> 
> https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-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
0xffff or 0x0000.  It's a low risk, but not a no risk scenario.

> I think that should be a first test, maybe also for CRB.

I don't think we can make that the first test.  If we don't wait for
tpmRegValidSts (qualified by some known zero bit), then we can't tell
the difference between no-TPM, and reading before the device is ready.

Note 2 in section 6.6 of the TIS 1.3 spec:

2. Within 30 milliseconds of the completion of TPM_Init:
   a. All fields within the access register and all other registers MUST
      return with the state of all their fields valid (i.e.
      TPM_ACCESS_x.tpmRegValidSts is set to ‘1’).
   b. The TPM MUST be ready to receive a command

Maybe the best thing to do is try to skip probing entirely based if we
don't see a related ACPI table.  If we do probe, and have to wait in the
corrected wait code where we qualify tpmRegValidSts against vendor ID or
Seize, and we still find no device, we should probably print a message
to the user.

I don't see a way around some potential wait if we want to support real
hardware devices reliably.