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

[Vadim Bendebury]

On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote:
>> On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>> > On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote:
>> >> 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
>> >
>> > Are you sure you're not relicensing code you don't own ? I'm just
>> > curious, what's the origin of the code ? I'd prefer to avoid legal crap.
>>
>> I am sure.
>
> Would you mind answering my second question please ?
>

I wrote this from scratch.


>>
>> >> [..]
>> >>
>> >> >> >> +
>> >> >> >> +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.
>> >
>> > can't you pass the locality array ?
>>
>> no, because it would not be clear how big the array is.
>
> TPM_TOTAL_LOCALITIES big ?
>

I believe it is clearer when this information is included in a
structure describing the chip (as opposed to the array size being a
separate #define)

>>
>> >> [..]
>> >>
>> >> >> > 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?
>> >
>> > It doesn't do typechecking.
>>
>> but the code around it does, doesn't it?
>>
>> Sorry, as I said, I am new here: how does this work on this project -
>> does the submitter have to agree to all reviewer's comments? Can I ask
>> somebody else to confirm that using a macro in this case in
>> inappropriate?
>>

converted BURST_COUNT into a function.

>> >> >> >> +
>> >> >> >> +/*
>> >> >> >> + * 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.
>> >
>> > This seems wrong like hell.
>>
>> You are entitled to your opinion, but you should not be suggesting to
>> change this code to inline functions, because it would break it.
>
> Then write it so it won't break please.
>

it is not broken as of now and IMO this is a good case for using macros.

There is at least one other custodian who supports this, and Wolfgang
Denk does not seem to mind.

>>
>> >> __v is needed to avoid side effects when invoking the macro.
>> >
>> > Side effects ? What side effects ?
>>
>> https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid
>> +side-effects+in+arguments+to+unsafe+macros
>
> I still don't see it. You use the variable in printf() and writeX(), neither of
> which change the variable ... so where's the sideeffect ?
>

The side effect comes from the calling site.

When data[count++] is used as a macro argument, if there is no
intermediate variable defined in the macro declaration, macro
expansion inserts data[count++] in the code several times (as many
times as the parameter is used in the macro declaration), and in this
particular case gets executed twice, resulting in `count' advancing by
2 and wrong `data' values used.

> Cheers
>

cheers,
/vb