[SeaBIOS] [PATCH v2 4/4] tpm: add TPM CRB device support

[Stefan Berger]

On 02/23/2018 12:05 AM, Kevin O'Connor wrote:
> On Tue, Feb 13, 2018 at 11:08:07AM -0500, Stefan Berger wrote:
>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>
>> The CRB device was introduced with TPM 2.0 to be physical-bus agnostic
>> and defined in TCG PC Client Platform TPM Profile (PTP) Specification
>> Family “2.0” Level 00 Revision 01.03 v22
>>
>> It seems to be required with Windows 10. It is also a simpler device
>> than FIFO/TIS.
>>
>> This patch only support locality 0 since also the CRB device in QEMU
>> only supports this locality.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
>> Reviewed-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>> Tested-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
>> ---
>>   src/hw/tpm_drivers.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   src/hw/tpm_drivers.h |  26 +++++++
>>   2 files changed, 223 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
>> index 5cee9d8..24ce9b7 100644
>> --- a/src/hw/tpm_drivers.c
>> +++ b/src/hw/tpm_drivers.c
>> @@ -17,6 +17,8 @@
>>   #include "util.h" // timer_calc_usec
>>   #include "x86.h" // readl
>>   
>> +#define MIN(x,y) ((x) < (y) ? (x) : (y))
> I'd prefer not to introduce adhoc MIN/MAX macros - they tend to be
> confusing as sometimes they're evaluation safe and sometimes not.  I'm
> okay with a separate patch that adds min/max globally to seabios, but
> otherwise I suggest just implementing this directly in its one call
> site.

Then I just implement it 'in its one call site' here.

>
> [...]
>> +/* if device is not there, return '0', '1' otherwise */
>> +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 */
>> +            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 > __UINT32_MAX__)
> Where does the __UINT32_MAX__ constant come from?  We don't normally
> depend on any system libraries.  Again, probably best to just use
> 0xffffffff.

Will be fixed in  v2.

    Stefan

>
> Otherwise the series looks good to me.
>
> Thanks,
> -Kevin
>