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@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
