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

[Stefan Berger]

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 at 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
>