This series of patches attempts to fix the probing of the CRB interface for real hardware.
Stephen Douthit should test this on real hardware.
Regards, Stefan
Stefan Berger (3): tpm: Wait for tpmRegValidSts flags on CRQ interface before probing tpm: revert return values for successful/failed CRB probing tpm: when CRB is active, select, lock it, and check addresses
src/hw/tpm_drivers.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
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 stefanb@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); + + /* Wait for the interface to report it's ready */ + u32 rc = crb_wait_reg(0, CRB_REG_LOC_STATE, TIS2_DEFAULT_TIMEOUT_A, + CRB_STATE_VALID_STS, CRB_STATE_VALID_STS); + if (rc) + return 0; + u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
if ((ifaceid & 0xf) != 0xf) {
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 stefanb@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.
-Kevin
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 stefanb@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.
Good point. See my other email to the mailing list regarding the valid bit, the specs and the implementation in QEMU. Maybe we have to adapt the QEMU implementation and set the bit to valid upon initialization of the device, I am not sure about this (see the comment before the write). What Stephen suggested was to read this flag and check whether it indicates valid contents of this and other registers before reading them.
Stefan
-Kevin
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 stefanb@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.
-Kevin
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?
Stefan
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 stefanb@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.
-Steve
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 stefanb@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
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com --- src/hw/tpm_drivers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index ad97f67..b208a37 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -410,13 +410,13 @@ static u32 crb_probe(void)
/* no support for 64 bit addressing yet */ if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR))) - return 1; + return 0;
u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR)); if (addr > 0xffffffff) - return 1; + return 0;
- return 0; + return 1; }
static TPMVersion crb_get_tpm_version(void)
Do not just indicate that the probing for the CRB interface was successful if we find it active. Instead, select it, lock it, and test the addresses for whether they can be used (must be 32 bit).
Signed-off-by: Stefan Berger stefanb@linux.vnet.ibm.com --- src/hw/tpm_drivers.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c index b208a37..4278f20 100644 --- a/src/hw/tpm_drivers.c +++ b/src/hw/tpm_drivers.c @@ -396,9 +396,7 @@ static u32 crb_probe(void) if ((ifaceid & 0xf) != 0xf) { if ((ifaceid & 0xf) == 1) { /* CRB is active */ - return 1; - } - if ((ifaceid & (1 << 14)) == 0) { + } else if ((ifaceid & (1 << 14)) == 0) { /* CRB cannot be selected */ return 0; }
On 03/14/2018 01:42 PM, Stefan Berger wrote:
This series of patches attempts to fix the probing of the CRB interface for real hardware.
Stephen Douthit should test this on real hardware.
Thanks for putting this together.
The modded unit is tied up in other development work that I need to complete this week. It will probably be a few days before I can get back to this.
Regards, Steve
Dear Stefan,
On 03/14/18 18:42, Stefan Berger wrote:
This series of patches attempts to fix the probing of the CRB interface for real hardware.
Stephen Douthit should test this on real hardware.
I tested this series on the Lenovo X60, where coreboot doesn’t set up the TPM, and the long delay is indeed gone. I didn’t do any measurements though, if there are small delays.
So, the bug fix commits fix real problems.
Tested-by: Paul Menzel pmenzel@molgen.mpg.de
Kind regards,
Paul
On 03/19/2018 09:02 AM, Paul Menzel wrote:
Dear Stefan,
On 03/14/18 18:42, Stefan Berger wrote:
This series of patches attempts to fix the probing of the CRB interface for real hardware.
Stephen Douthit should test this on real hardware.
I tested this series on the Lenovo X60, where coreboot doesn’t set up the TPM, and the long delay is indeed gone. I didn’t do any measurements though, if there are small delays.
So, the bug fix commits fix real problems.
Tested-by: Paul Menzel pmenzel@molgen.mpg.de
Thank you!
Stefan
Kind regards,
Paul
-
Kevin O'Connor -
Paul Menzel -
Stefan Berger -
Stephen Douthit