On 03/19/2018 10:48 AM, Stephen Douthit wrote:
On 03/19/2018 08:55 AM, Stefan Berger wrote:On 03/14/2018 03:42 PM, Kevin O'Connor wrote:On Wed, Mar 14, 2018 at 01:42:41PM -0400, Stefan Berger wrote:Wait for the tpmRegValidSts flag on the TPM_LOC_STATE_x register.Since it's not quite clear when this flag may become valid, we request access to the interace on locality 0, which must then make it valid. Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com> --- src/hw/tpm_drivers.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index ed58bf5..ad97f67 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c@@ -374,12 +374,23 @@ static u32 tis_waitrespready(enum tpmDurationType to_t)return rc; } +#define CRB_STATE_VALID_STS 0b10000000 + /* if device is not there, return '0', '1' otherwise */ static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0; + /* request access -- this must cause a valid STS flag */ + writeb(CRB_REG(0, CRB_REG_LOC_CTRL), 1);Would it be possible to do a sanity check with read operations before doing a write operation? The fear is that the device is not present and the write operation corrupts some other device, causes a bus error, or something else undesirable.We don't need to force use of locality 0 for this polling. LOC_STATE is a single register aliased to all localities. We can just start polling on LOC_STATE immediately without the write. There's also a note in the spec that locAssigned and activeLocality will be set to 0 as their initial state after power on.If this one is controversal, the other two patches fix real bugs and we should consider applying them. I think for testing Stephen's feedback is primarily important on 1/3. Stephen, can you comment?I think we can change this patch to something like this: +#define CRB_STATE_VALID_STS 0b10000000 +#define CRB_STATE_LOC_ASSIGNED 0b00000010+#define CRB_STATE_READY_MASK (CRB_STATE_VALID_STS | CRB_STATE_LOC_ASSIGNED)+ /* if device is not there, return '0', '1' otherwise */ static u32 crb_probe(void) { if (!CONFIG_TCGBIOS) return 0; + /* Wait for the interface to report it's ready */ + u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_D, + CRB_STATE_READY_MASK, CRB_STATE_VALID_STS); + if (rc) + return 0; + I tested this on my system (2.0 TPM using FIFO interface) in the present and absent and it works as expected. This testing misses the CRB device present case though.
Ok. In that case we'll have to set that tpmRegValidSts field also in QEMU upon reset. I'll take your changes for a v2 series and post again.
Stefan
-Steve
_______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
