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

[Stephen Douthit]

On 03/13/2018 07:39 AM, Stefan Berger wrote:
> On 03/13/2018 07:31 AM, Stefan Berger wrote:
>> On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
>>> On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
>>>> I've got a board modded so I can jumper the TPM in and out.
>>>>
>>>> What I found in the no-TPM case was that both tis_probe() and
>>>> crb_probe() incorrectly return 1 for device present if all Fs are read.
>>>>
>>>> For tis_probe() that was because rc wasn't updated to 0 if didvid was
>>>> 0xffffffff.  For crb_probe() the last three return statements are
>>
>> static u32 tis_probe(void)
>> {
>>     if (!CONFIG_TCGBIOS)
>>         return 0;
>>
>>     /* Wait for the interface to report it's ready */
>>     u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
>>                              TIS_ACCESS_TPM_REG_VALID_STS,
>>                              TIS_ACCESS_TPM_REG_VALID_STS);
>>     if (rc)
>>         return 0;
> 
> Can we do without this tis_wait_access on real hardware? Presumably it only affects the STS register. If so...

The reset timing chapter of the spec implies that you need to check this
bit to know when the TPM has completed it's initialization after reset
is released.

>>
>>     u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
>>
>>     if ((didvid != 0) && (didvid != 0xffffffff))
>>         rc = 1;
> 
> ... then we could have an else branch here and 'return 0'.
> 
>>
>>     /* TPM 2 has an interface register */
>>     u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>>
>>     if ((ifaceid & 0xf) != 0xf) {
>>         if ((ifaceid & 0xf) == 1) {
>>             /* CRB is active; no TIS */
>>             return 0;
>>         }
>>         if ((ifaceid & (1 << 13)) == 0) {
>>             /* TIS cannot be selected */
>>             return 0;
>>         }
>>         /* write of 0 to bits 17-18 selects TIS */
>>         writel(TIS_REG(0, TIS_REG_IFACE_ID), 0);
>>         /* since we only support TIS, we lock it */
>>         writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19));
>>     }
>>
>>     return rc;
>> }
>>
>> When tis_probe() returns '1', it means the interface was detected.
>> If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
>>
>> static u32 crb_probe(void)
>> {
>>     if (!CONFIG_TCGBIOS)
>>         return 0;
>>
>>     u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>
>>     if ((ifaceid & 0xf) != 0xf) {
>>         if ((ifaceid & 0xf) == 1) {
>>             /* CRB is active */
>>             return 1;
>>         }
>>         if ((ifaceid & (1 << 14)) == 0) {
>>             /* CRB cannot be selected */
>>             return 0;
>>         }
>>         /* write of 1 to bits 17-18 selects CRB */
>>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>         /* lock it */
>>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>     }
>>
>>     /* no support for 64 bit addressing yet */
>>     if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>         return 1;
>>
>>     u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>     if (addr > 0xffffffff)
>>         return 1;
>>
>>     return 0;
>> }
>>
>> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
>>
>>
>>
>>>> inverted from what they should be, and the first 64bit address check
>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>> timeout for me when the TPM was disconnected.
>>
>> I see a problem with the crb_probe function but not the tis_probe.
>>
>>>>
>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
>>>> check that instead of didvid in tis_probe to handle the aborted read all
>>>> 0s/Fs case.
>>
>> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
>>
>>>>
>>>> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
>>>> what's in tis_probe() to avoid potential races on real hardware.
>>
>> That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
>>
>>>> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
>>>> use as a sanity check against the no device all Fs case.
>>>>
>>>> Let me know if that sounds like a better way to catch the no device
>>>> case, or if there's is some other check that would be better.
>>> Thanks for looking at this.  It is common on x86 for invalid memory
>>> accesses to return 0xff.  I don't know enough about the TPM hardware
>>> to make a judgement call on the best way to test for presence. I'd
>>> like to hear what Stefan's thoughts are on this.
>>>
>>> -Kevin
>>>
>>
>>
>> _______________________________________________
>> SeaBIOS mailing list
>> SeaBIOS at seabios.org
>> https://mail.coreboot.org/mailman/listinfo/seabios
>>
>