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-Regis...
So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
I think that should be a first test, maybe also for CRB.
Stefan