[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
>> 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;

     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));

     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
>