[U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot

Vadim Bendebury vbendeb at chromium.org
Sun Oct 16 03:04:33 CEST 2011


On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote:
>> Dear Marek Vasut,
>>
>> thank you for your comments, please see below:
>>
>> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
>> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote:
>> >> TPM (Trusted Platform Module) is an integrated circuit and
>> >> software platform that provides computer manufacturers with the
>> >> core components of a subsystem used to assure authenticity,
>> >> integrity and confidentiality.
>> >
>> > [...]
>> >
>> > Quick points:
>> > * The license
>>
>> Please suggest the appropriate file header text.
>
> Uh ... you should know the license !!!

removed the BSD part

>>
[..]
>> >> +
>> >> +struct lpc_tpm {
>> >> +     struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
>> >> +};
>> >
>> > Do you need such envelope ?
>>
>> I think I do - this accurately describes the structure of the chip.
>
> There's just one member ... it's weird?
>

I think it is appropriate in this case to encapsulate the entire chip
description in a structure. Among other things makes it easier to pass
a pointer to the chip description around.

[..]
>> >
>> > Dot missing at the end.
>>
>> ok.
>
> Please fix globally.
>

done

>>
>> >> +#define TPM_DRIVER_ERR               (-1)
>> >> +
>> >> + /* 1 second is plenty for anything TPM does.*/
>> >> +#define MAX_DELAY_US (1000 * 1000)
>> >> +
>> >> +/* Retrieve burst count value out of the status register contents. */
>> >> +#define BURST_COUNT(status) ((u16)(((status) >>
>> >> TIS_STS_BURST_COUNT_SHIFT) & \ +
>> >>  TIS_STS_BURST_COUNT_MASK))
>> >
>> > Do you need the cast ?
>>
>> I think it demonstrates the intentional truncation of the value, it
>> gets assigned to u16 values down the road, prevents compiler warnings
>> about assigning incompatible values in some cases.
>
> Make it an inline function then, this will do the typechecking for you.
>

I am not sure what is wrong with a short macro in this case - is this
against the coding style?

>>
>> >> +
>> >> +/*
>> >> + * Structures defined below allow creating descriptions of TPM
>> >> vendor/device + * ID information for run time discovery. The only device
>> >> the system knows + * about at this time is Infineon slb9635
>> >> + */
>> >> +struct device_name {
>> >> +     u16 dev_id;
>> >> +     const char * const dev_name;
>> >> +};
>> >> +
>> >> +struct vendor_name {
>> >> +     u16 vendor_id;
>> >> +     const char *vendor_name;
>> >> +     const struct device_name *dev_names;
>> >> +};
>> >> +
>> >> +static const struct device_name infineon_devices[] = {
>> >> +     {0xb, "SLB9635 TT 1.2"},
>> >> +     {0}
>> >> +};
>> >> +
>> >> +static const struct vendor_name vendor_names[] = {
>> >> +     {0x15d1, "Infineon", infineon_devices},
>> >> +};
>> >> +
>> >> +/*
>> >> + * Cached vendor/device ID pair to indicate that the device has been
>> >> already + * discovered
>> >> + */
>> >> +static u32 vendor_dev_id;
>> >> +
>> >> +/* TPM access going through macros to make tracing easier. */
>> >> +#define tpm_read(ptr) ({ \
>> >> +     u32  __ret; \
>> >> +     __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
>> >> +      debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
>> >> +            (u32)ptr - (u32)lpc_tpm_dev, __ret); \
>> >> +                                      __ret; })
>> >> +
>> >
>> > Make this inline function
>> >
>> >> +#define tpm_write(value, ptr) ({                \
>> >> +     u32 __v = value;        \
>> >> +     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> >> +            (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> >> +     if (sizeof(*ptr) == 1) \
>> >> +             writeb(__v, ptr); \
>> >> +     else \
>> >> +             writel(__v, ptr); })
>> >> +
>> >
>> > DTTO
>>
>> Are you sure these will work as inline functions?
>
> Why not ? Also, why do you introduce the __v ?

macro vs function: need to be able to tell the pointed object size at run time.

__v is needed to avoid side effects when invoking the macro.

>>
>> > [...]
>> >
>> >> +static u32 tis_senddata(const u8 * const data, u32 len)
>> >> +{
>> >> +     u32 offset = 0;
>> >> +     u16 burst = 0;
>> >> +     u32 max_cycles = 0;
>> >> +     u8 locality = 0;
>> >> +     u32 value;
>> >> +
>> >> +     value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
>> >> +                          TIS_STS_COMMAND_READY,
>> >> TIS_STS_COMMAND_READY); +     if (value == TPM_TIMEOUT_ERR) {
>> >> +             printf("%s:%d - failed to get 'command_ready' status\n",
>> >> +                    __FILE__, __LINE__);
>> >> +             return TPM_DRIVER_ERR;
>> >> +     }
>> >> +     burst = BURST_COUNT(value);
>> >> +
>> >> +     while (1) {
>> >
>> > No endless loops please.
>>
>> You are the third person looking at this, but the first one
>> uncomfortable with this. Obviously this is not an endless loop, there
>> are three points where the loop terminates, two on timeout errors and
>> one on success.
>
> So there's no case where this can loop endlessly ? fine.
>
>>
>> >> +             unsigned count;
>> >> +
>> >> +             /* Wait till the device is ready to accept more data. */
>> >> +             while (!burst) {
>> >> +                     if (max_cycles++ == MAX_DELAY_US) {
>> >> +                             printf("%s:%d failed to feed %d bytes of
>> >> %d\n", +                                    __FILE__, __LINE__, len -
>> >> offset, len); +                             return TPM_DRIVER_ERR;
>> >> +                     }
>> >> +                     udelay(1);
>> >> +                     burst =
>> >> BURST_COUNT(tpm_read(&lpc_tpm_dev->locality +
>> >>                        [locality].tpm_status)); +             }
>> >> +
>> >> +             max_cycles = 0;
>> >> +
>> >> +             /*
>> >> +              * Calculate number of bytes the TPM is ready to accept in
>> >> one +              * shot.
>> >> +              *
>> >> +              * We want to send the last byte outside of the loop
>> >> (hence +              * the -1 below) to make sure that the 'expected'
>> >> status bit +              * changes to zero exactly after the last byte
>> >> is fed into the +              * FIFO.
>> >> +              */
>> >> +             count = min(burst, len - offset - 1);
>> >> +             while (count--)
>> >> +                     tpm_write(data[offset++],
>> >> +                               &lpc_tpm_dev->locality[locality].data);
>> >> +
>> >> +             value = tis_wait_reg(&lpc_tpm_dev->locality
>> >> +                                  [locality].tpm_status,
>> >> +                                  TIS_STS_VALID, TIS_STS_VALID);
>> >> +
>> >> +             if ((value == TPM_TIMEOUT_ERR) || !(value &
>> >> TIS_STS_EXPECT)) { +                     printf("%s:%d TPM command feed
>> >> overflow\n", +                            __FILE__, __LINE__);
>> >> +                     return TPM_DRIVER_ERR;
>> >> +             }
>> >> +
>> >> +             burst = BURST_COUNT(value);
>> >> +             if ((offset == (len - 1)) && burst)
>> >> +                     /*
>> >> +                      * We need to be able to send the last byte to the
>> >> +                      * device, so burst size must be nonzero before we
>> >> +                      * break out.
>> >> +                      */
>> >> +                     break;
>> >> +     }
>> >> +
>> >> +     /* Send the last byte. */
>> >> +     tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
>> >> +     /*
>> >> +      * Verify that TPM does not expect any more data as part of this
>> >> +      * command.
>> >> +      */
>> >> +     value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
>> >> +                          TIS_STS_VALID, TIS_STS_VALID);
>> >> +     if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
>> >> +             printf("%s:%d unexpected TPM status 0x%x\n",
>> >> +                    __FILE__, __LINE__, value);
>> >> +             return TPM_DRIVER_ERR;
>> >> +     }
>> >> +
>> >> +     /* OK, sitting pretty, let's start the command execution. */
>> >> +     tpm_write(TIS_STS_TPM_GO,
>> >> &lpc_tpm_dev->locality[locality].tpm_status); +     return 0;
>> >> +}
>> >> +
>> >> +/*
>> >> + * tis_readresponse()
>> >> + *
>> >> + * read the TPM device response after a command was issued.
>> >> + *
>> >> + * @buffer - address where to read the response, byte by byte.
>> >> + * @len - pointer to the size of buffer
>> >> + *
>> >> + * On success stores the number of received bytes to len and returns 0.
>> >> On + * errors (misformatted TPM data or synchronization problems)
>> >> returns + * TPM_DRIVER_ERR.
>> >> + */
>> >> +static u32 tis_readresponse(u8 *buffer, u32 *len)
>> >> +{
>> >> +     u16 burst_count;
>> >> +     u32 status;
>> >> +     u32 offset = 0;
>> >> +     u8 locality = 0;
>> >> +     const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
>> >> +     u32 expected_count = *len;
>> >> +     int max_cycles = 0;
>> >> +
>> >> +     /* Wait for the TPM to process the command */
>> >> +     status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
>> >> +                           has_data, has_data);
>> >> +     if (status == TPM_TIMEOUT_ERR) {
>> >
>> > Just make it return non-zero for timeout and check for non-zero. And
>> > unify the variable names please.
>>
>> What variable names are you referring to, can you please elaborate.
>
> "value" in the previous function, "status" in this one. Why the inconsistence ?
>

done

>>
>> >> +             printf("%s:%d failed processing command\n",
>> >> +                    __FILE__, __LINE__);
>> >> +             return TPM_DRIVER_ERR;
>> >> +     }
>> >> +
>> >> +     do {
>> >> +             while ((burst_count = BURST_COUNT(status)) == 0) {
>> >> +                     if (max_cycles++ == MAX_DELAY_US) {
>> >> +                             printf("%s:%d TPM stuck on read\n",
>> >> +                                    __FILE__, __LINE__);
>> >> +                             return TPM_DRIVER_ERR;
>> >> +                     }
>> >> +                     udelay(1);
>> >> +                     status = tpm_read(&lpc_tpm_dev->locality
>> >> +                                       [locality].tpm_status);
>> >> +             }
>> >> +
>> >> +             max_cycles = 0;
>> >> +
>> >> +             while (burst_count-- && (offset < expected_count)) {
>> >> +                     buffer[offset++] = (u8)
>> >> tpm_read(&lpc_tpm_dev->locality +
>> >>                [locality].data); +
>> >> +                     if (offset == 6) {
>> >> +                             /*
>> >> +                              * We got the first six bytes of the
>> >> reply, +                              * let's figure out how many bytes
>> >> to expect +                              * total - it is stored as a 4
>> >> byte number in +                              * network order, starting
>> >> with offset 2 into +                              * the body of the
>> >> reply.
>> >> +                              */
>> >> +                             u32 real_length;
>> >> +                             memcpy(&real_length,
>> >> +                                    buffer + 2,
>> >> +                                    sizeof(real_length));
>> >> +                             expected_count = be32_to_cpu(real_length);
>> >> +
>> >> +                             if ((expected_count < offset) ||
>> >> +                                 (expected_count > *len)) {
>> >> +                                     printf("%s:%d bad response size
>> >> %d\n", +                                            __FILE__, __LINE__,
>> >> +                                            expected_count);
>> >> +                                     return TPM_DRIVER_ERR;
>> >> +                             }
>> >> +                     }
>> >> +             }
>> >> +
>> >> +             /* Wait for the next portion */
>> >> +             status = tis_wait_reg(&lpc_tpm_dev->locality
>> >> +                                   [locality].tpm_status,
>> >> +                                   TIS_STS_VALID, TIS_STS_VALID);
>> >> +             if (status == TPM_TIMEOUT_ERR) {
>> >> +                     printf("%s:%d failed to read response\n",
>> >> +                            __FILE__, __LINE__);
>> >> +                     return TPM_DRIVER_ERR;
>> >> +             }
>> >> +
>> >> +             if (offset == expected_count)
>> >> +                     break;  /* We got all we need */
>> >> +
>> >> +     } while ((status & has_data) == has_data);
>> >
>> > No endless loop please.
>>
>> I am not sure why you call this endless - it terminates on a few
>> events. The thing is that it is not even known in advance how many
>> cycles the loop will have to run, but it has necessary stop gaps to
>> prevent it from running forever.
>
> See above, is it possible for this to hang ? If not, ok.
>
>>
>> >> +
>> >> +     /*
>> >> +      * Make sure we indeed read all there was. The TIS_STS_VALID bit
>> >> is +      * known to be set.
>> >> +      */
>> >> +     if (status & TIS_STS_DATA_AVAILABLE) {
>> >> +             printf("%s:%d wrong receive status %x\n",
>> >> +                    __FILE__, __LINE__, status);
>> >> +             return TPM_DRIVER_ERR;
>> >> +     }
>> >> +
>> >> +     /* Tell the TPM that we are done. */
>> >> +     tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
>> >> +               [locality].tpm_status);
>> >> +     *len = offset;
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +int tis_init(void)
>> >> +{
>> >> +     return tis_probe();
>> >> +}
>> >
>> > Make tis_probe into tis_init and be done with it ?
>>
>> ok
>>
>> >> +
>> >> +int tis_open(void)
>> >> +{
>> >> +     u8 locality = 0; /* we use locality zero for everything */
>> >> +
>> >> +     if (tis_close())
>> >> +             return TPM_DRIVER_ERR;
>> >> +
>> >> +     /* now request access to locality */
>> >> +     tpm_write(TIS_ACCESS_REQUEST_USE,
>> >> +               &lpc_tpm_dev->locality[locality].access);
>> >> +
>> >> +
>> >> +     /* did we get a lock? */
>> >> +     if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
>> >> +                      TIS_ACCESS_ACTIVE_LOCALITY,
>> >> +                      TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
>> >> +             printf("%s:%d - failed to lock locality %d\n",
>> >> +                    __FILE__, __LINE__, locality);
>> >> +             return TPM_DRIVER_ERR;
>> >> +     }
>> >> +
>> >> +     tpm_write(TIS_STS_COMMAND_READY,
>> >> +               &lpc_tpm_dev->locality[locality].tpm_status);
>> >> +     return 0;
>> >> +}
>> >
>> > [...]
>> >
>> > Cheers
>>
>> cheers,
>> /vb
>
> Cheers
>

cheers,
/vb


More information about the U-Boot mailing list