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

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