1. Patchew
  2. Seabios
  3. [SeaBIOS] [PATCH 0/3] Fix CRB probing to work with real hardware
  4. View patch

[SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing

Stefan Berger posted 3 patches 7 years, 1 month ago
There is a newer version of this series
[SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Stefan Berger 7 years, 1 month ago 
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) {
-- 
2.5.5


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Kevin O'Connor 7 years, 1 month ago 
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

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Stefan Berger 7 years, 1 month ago 
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
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Stefan Berger 7 years, 1 month ago 
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
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Stephen Douthit 7 years, 1 month ago 
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

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH 1/3] tpm: Wait for tpmRegValidSts flags on CRQ interface before probing
Posted by Stefan Berger 7 years, 1 month ago 
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
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.