[U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot
Marek Vasut
marek.vasut at gmail.com
Sat Oct 15 23:09:55 CEST 2011
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 !!!
>
> > * Use debug() when fitting
>
> It seems to me debug/puts/printf are used where appropriate - do you
> have some cases in mind which need to be changed?
Ok
>
> >> diff --git a/drivers/tpm/generic_lpc_tpm.c
> >> b/drivers/tpm/generic_lpc_tpm.c new file mode 100644
> >> index 0000000..8319286
> >> --- /dev/null
> >> +++ b/drivers/tpm/generic_lpc_tpm.c
> >
> > [...]
> >
> >> +
> >> +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?
>
> >> +
> >> +static struct lpc_tpm *lpc_tpm_dev =
> >> + (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
> >> +
> >> +/* Some registers' bit field definitions */
> >> +#define TIS_STS_VALID (1 << 7) /* 0x80 */
> >> +#define TIS_STS_COMMAND_READY (1 << 6) /* 0x40 */
> >> +#define TIS_STS_TPM_GO (1 << 5) /* 0x20 */
> >> +#define TIS_STS_DATA_AVAILABLE (1 << 4) /* 0x10 */
> >> +#define TIS_STS_EXPECT (1 << 3) /* 0x08 */
> >> +#define TIS_STS_RESPONSE_RETRY (1 << 1) /* 0x02 */
> >> +
> >> +#define TIS_ACCESS_TPM_REG_VALID_STS (1 << 7) /* 0x80 */
> >> +#define TIS_ACCESS_ACTIVE_LOCALITY (1 << 5) /* 0x20 */
> >> +#define TIS_ACCESS_BEEN_SEIZED (1 << 4) /* 0x10 */
> >> +#define TIS_ACCESS_SEIZE (1 << 3) /* 0x08 */
> >> +#define TIS_ACCESS_PENDING_REQUEST (1 << 2) /* 0x04 */
> >> +#define TIS_ACCESS_REQUEST_USE (1 << 1) /* 0x02 */
> >> +#define TIS_ACCESS_TPM_ESTABLISHMENT (1 << 0) /* 0x01 */
> >> +
> >> +#define TIS_STS_BURST_COUNT_MASK (0xffff)
> >> +#define TIS_STS_BURST_COUNT_SHIFT (8)
> >> +
> >> +/*
> >> + * Error value returned if a tpm register does not enter the expected
> >> state + * after continuous polling. No actual TPM register reading ever
> >> returns ~0, + * so this value is a safe error indication to be mixed
> >> with possible status + * register values.
> >> + */
> >> +#define TPM_TIMEOUT_ERR (~0)
> >> +
> >> +/* Error value returned on various TPM driver errors */
> >
> > Dot missing at the end.
>
> ok.
Please fix globally.
>
> >> +#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.
>
> >> +
> >> +/*
> >> + * 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 ?
>
> > [...]
> >
> >> +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 ?
>
> >> + 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
More information about the U-Boot
mailing list