TPM Lite: Add a "firmware" target for building TPM Lite

TPM Lite: Add a "firmware" target for building TPM Lite

This CL adds a firmware target for standalone compilation of TPM lite for use in the firmware. In particular, it removes dependencies on external Trouser TSS headers and uses the snapshot version of structures.h which define TPM command structures.

I also removed the include/ directory and moved the tlcl.h header to src/tlcl/ since I think that having a separate directory for just a single header file seems overkill.

[Luigi Semenzato, 2010-04-27 04:52:31]

Looks good---just a couple of comments, mostly in the Makefile.

Going to bed now---next review tomorrow at 8:30a :-)

http://codereview.chromium.org/1787004/diff/3001/4003
File src/platform/tpm_lite/src/tlcl/Makefile (right):

http://codereview.chromium.org/1787004/diff/3001/4003#newcode35
src/platform/tpm_lite/src/tlcl/Makefile:35: $(CC) -c -Wall -Werror -ansi
-DFIRMWARE tlcl.c
Not completely sure about this, for two reasons.  1. It seems that the result of
the compilation should still be tlcl.o---it's just one more target, but all the
others produce that same file.  The dependency change (structures.h.snapshot vs.
structures.h) can be handled by a variable.  (Also note that maybe
structures.h.snapshot was not a great choice for the file name).  2. I don't
know for sure that this doesn't work, but generally if I use #if X then I use
-DX=0 or -DX=1, versus #ifdef Y and -DY.

So maybe saved-structures.h?  I some .h ending may be better.  You could create
it in the Makefile.

http://codereview.chromium.org/1787004/diff/3001/4003#newcode38
src/platform/tpm_lite/src/tlcl/Makefile:38: rm -f generator *.o *.a structures.h
tpm_firmware.o *~
*.o includes tpm_firmware.o

http://codereview.chromium.org/1787004/diff/3001/4004
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):

http://codereview.chromium.org/1787004/diff/3001/4004#newcode152
src/platform/tpm_lite/src/tlcl/tlcl.c:152: * command and a pointer to the output
buffer for putting in the results..
Two periods.

http://codereview.chromium.org/1787004/diff/3001/4007
File src/platform/tpm_lite/src/tlcl/tss_constants.h (right):

http://codereview.chromium.org/1787004/diff/3001/4007#newcode7
src/platform/tpm_lite/src/tlcl/tss_constants.h:7: *
Remove extra line?

[gauravsh, 2010-04-27 18:03:58]

http://codereview.chromium.org/1787004/diff/3001/4003
File src/platform/tpm_lite/src/tlcl/Makefile (right):

http://codereview.chromium.org/1787004/diff/3001/4003#newcode35
src/platform/tpm_lite/src/tlcl/Makefile:35: $(CC) -c -Wall -Werror -ansi
-DFIRMWARE tlcl.c
On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> Not completely sure about this, for two reasons.  1. It seems that the result
of
> the compilation should still be tlcl.o---it's just one more target, but all
the

Oh yes, I missed that -o arg. Fixed.

BTW, I think it is better to have a separately named target than trying to do
everything using one rule and conditional variables. This way, I simply have to
do a "make firmware" to build it for the firmware. Also, it makes it clearer as
to what is needed to port this code to firmware. What do you think?


> others produce that same file.  The dependency change (structures.h.snapshot
vs.
> structures.h) can be handled by a variable.  (Also note that maybe
> structures.h.snapshot was not a great choice for the file name).  

> 2. I don't
> know for sure that this doesn't work, but generally if I use #if X then I use
> -DX=0 or -DX=1, versus #ifdef Y and -DY.
This works. -DVAR is equivalent to doing a #define VAR

> 
> So maybe saved-structures.h?  I some .h ending may be better.  You could
create
> it in the Makefile.

I think it is a bad idea to clutter the Makefile with too much rarely used
functionality. So how about I just rename the .snapshot to saved-structures.h
for now?

http://codereview.chromium.org/1787004/diff/3001/4003#newcode38
src/platform/tpm_lite/src/tlcl/Makefile:38: rm -f generator *.o *.a structures.h
tpm_firmware.o *~
On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> *.o includes tpm_firmware.o

Fixed.

http://codereview.chromium.org/1787004/diff/3001/4004
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):

http://codereview.chromium.org/1787004/diff/3001/4004#newcode152
src/platform/tpm_lite/src/tlcl/tlcl.c:152: * command and a pointer to the output
buffer for putting in the results..
On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> Two periods.
Fixed.

http://codereview.chromium.org/1787004/diff/3001/4007
File src/platform/tpm_lite/src/tlcl/tss_constants.h (right):

http://codereview.chromium.org/1787004/diff/3001/4007#newcode7
src/platform/tpm_lite/src/tlcl/tss_constants.h:7: *
On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> Remove extra line?
Done.

[Luigi Semenzato, 2010-04-27 18:13:25]

On Tue, Apr 27, 2010 at 11:03 AM,  <gauravsh@chromium.org> wrote:
>
> http://codereview.chromium.org/1787004/diff/3001/4003
> File src/platform/tpm_lite/src/tlcl/Makefile (right):
>
> http://codereview.chromium.org/1787004/diff/3001/4003#newcode35
> src/platform/tpm_lite/src/tlcl/Makefile:35: $(CC) -c -Wall -Werror -ansi
> -DFIRMWARE tlcl.c
> On 2010/04/27 04:52:31, Luigi Semenzato wrote:
>>
>> Not completely sure about this, for two reasons.  1. It seems that the
>
> result of
>>
>> the compilation should still be tlcl.o---it's just one more target,
>
> but all the
>
> Oh yes, I missed that -o arg. Fixed.

Yes, but why not call it tlcl.o?  Since you're compiling tlcl.c, that
would be less surprising.  Also more consistent, since the other two
targets (local and cross) also name it tlcl.o.

> BTW, I think it is better to have a separately named target than trying
> to do everything using one rule and conditional variables. This way, I
> simply have to do a "make firmware" to build it for the firmware. Also,
> it makes it clearer as to what is needed to port this code to firmware.
> What do you think?

Yes---I think it's good to structure it that way.

>> others produce that same file.  The dependency change
>
> (structures.h.snapshot vs.
>>
>> structures.h) can be handled by a variable.  (Also note that maybe
>> structures.h.snapshot was not a great choice for the file name).
>
>> 2. I don't
>> know for sure that this doesn't work, but generally if I use #if X
>
> then I use
>>
>> -DX=0 or -DX=1, versus #ifdef Y and -DY.
>
> This works. -DVAR is equivalent to doing a #define VAR

I know that, but the question is whether #if VAR works when VAR
defines to nothing.  I don't think it does, and if it does it's
probably not good style and one should use #ifdef.  Did you compile
this code?

I chose #if TPM_EMULATOR instead of #ifdef to make sure it was defined
one way or the other and not just forgotten.

>
>
>> So maybe saved-structures.h?  I some .h ending may be better.  You
>
> could create
>>
>> it in the Makefile.
>
> I think it is a bad idea to clutter the Makefile with too much rarely
> used functionality. So how about I just rename the .snapshot to
> saved-structures.h for now?

Yes, thanks.  Mine was a bad name choice.

>
> http://codereview.chromium.org/1787004/diff/3001/4003#newcode38
> src/platform/tpm_lite/src/tlcl/Makefile:38: rm -f generator *.o *.a
> structures.h tpm_firmware.o *~
> On 2010/04/27 04:52:31, Luigi Semenzato wrote:
>>
>> *.o includes tpm_firmware.o
>
> Fixed.
>
> http://codereview.chromium.org/1787004/diff/3001/4004
> File src/platform/tpm_lite/src/tlcl/tlcl.c (right):
>
> http://codereview.chromium.org/1787004/diff/3001/4004#newcode152
> src/platform/tpm_lite/src/tlcl/tlcl.c:152: * command and a pointer to
> the output buffer for putting in the results..
> On 2010/04/27 04:52:31, Luigi Semenzato wrote:
>>
>> Two periods.
>
> Fixed.
>
> http://codereview.chromium.org/1787004/diff/3001/4007
> File src/platform/tpm_lite/src/tlcl/tss_constants.h (right):
>
> http://codereview.chromium.org/1787004/diff/3001/4007#newcode7
> src/platform/tpm_lite/src/tlcl/tss_constants.h:7: *
> On 2010/04/27 04:52:31, Luigi Semenzato wrote:
>>
>> Remove extra line?
>
> Done.
>
> http://codereview.chromium.org/1787004/show
>

[gauravsh, 2010-04-27 18:20:38]

On 2010/04/27 18:13:25, Luigi Semenzato wrote:
> On Tue, Apr 27, 2010 at 11:03 AM,  <mailto:gauravsh@chromium.org> wrote:
> >
> > http://codereview.chromium.org/1787004/diff/3001/4003
> > File src/platform/tpm_lite/src/tlcl/Makefile (right):
> >
> > http://codereview.chromium.org/1787004/diff/3001/4003#newcode35
> > src/platform/tpm_lite/src/tlcl/Makefile:35: $(CC) -c -Wall -Werror -ansi
> > -DFIRMWARE tlcl.c
> > On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> >>
> >> Not completely sure about this, for two reasons. &nbsp;1. It seems that the
> >
> > result of
> >>
> >> the compilation should still be tlcl.o---it's just one more target,
> >
> > but all the
> >
> > Oh yes, I missed that -o arg. Fixed.
> 
> Yes, but why not call it tlcl.o?  Since you're compiling tlcl.c, that
> would be less surprising.  Also more consistent, since the other two
> targets (local and cross) also name it tlcl.o.

Ok, makes sense. Done.

> 
> > BTW, I think it is better to have a separately named target than trying
> > to do everything using one rule and conditional variables. This way, I
> > simply have to do a "make firmware" to build it for the firmware. Also,
> > it makes it clearer as to what is needed to port this code to firmware.
> > What do you think?
> 
> Yes---I think it's good to structure it that way.
> 
> >> others produce that same file. &nbsp;The dependency change
> >
> > (structures.h.snapshot vs.
> >>
> >> structures.h) can be handled by a variable. &nbsp;(Also note that maybe
> >> structures.h.snapshot was not a great choice for the file name).
> >
> >> 2. I don't
> >> know for sure that this doesn't work, but generally if I use #if X
> >
> > then I use
> >>
> >> -DX=0 or -DX=1, versus #ifdef Y and -DY.
> >
> > This works. -DVAR is equivalent to doing a #define VAR
> 
> I know that, but the question is whether #if VAR works when VAR
> defines to nothing.  I don't think it does, and if it does it's
> probably not good style and one should use #ifdef.  Did you compile
> this code?
> 
Yes, and it works. In any case, I have changed FIRMWARE blocks to #ifdefs.

> I chose #if TPM_EMULATOR instead of #ifdef to make sure it was defined
> one way or the other and not just forgotten.
> 
> >
> >
> >> So maybe saved-structures.h? &nbsp;I some .h ending may be better.
&nbsp;You
> >
> > could create
> >>
> >> it in the Makefile.
> >
> > I think it is a bad idea to clutter the Makefile with too much rarely
> > used functionality. So how about I just rename the .snapshot to
> > saved-structures.h for now?
> 
> Yes, thanks.  Mine was a bad name choice.
> 
> >
> > http://codereview.chromium.org/1787004/diff/3001/4003#newcode38
> > src/platform/tpm_lite/src/tlcl/Makefile:38: rm -f generator *.o *.a
> > structures.h tpm_firmware.o *~
> > On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> >>
> >> *.o includes tpm_firmware.o
> >
> > Fixed.
> >
> > http://codereview.chromium.org/1787004/diff/3001/4004
> > File src/platform/tpm_lite/src/tlcl/tlcl.c (right):
> >
> > http://codereview.chromium.org/1787004/diff/3001/4004#newcode152
> > src/platform/tpm_lite/src/tlcl/tlcl.c:152: * command and a pointer to
> > the output buffer for putting in the results..
> > On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> >>
> >> Two periods.
> >
> > Fixed.
> >
> > http://codereview.chromium.org/1787004/diff/3001/4007
> > File src/platform/tpm_lite/src/tlcl/tss_constants.h (right):
> >
> > http://codereview.chromium.org/1787004/diff/3001/4007#newcode7
> > src/platform/tpm_lite/src/tlcl/tss_constants.h:7: *
> > On 2010/04/27 04:52:31, Luigi Semenzato wrote:
> >>
> >> Remove extra line?
> >
> > Done.
> >
> > http://codereview.chromium.org/1787004/show
> >
>

[Luigi Semenzato, 2010-04-27 20:06:31]

LGTM
optionally consider the Makefile issues

[Luigi Semenzato, 2010-04-27 20:52:21]

Sorry, forgot to publish + mail.

http://codereview.chromium.org/1787004/diff/17001/18003
File src/platform/tpm_lite/src/tlcl/Makefile (right):

http://codereview.chromium.org/1787004/diff/17001/18003#newcode33
src/platform/tpm_lite/src/tlcl/Makefile:33: $(CC) -Wall -Werror -ansi -DFIRMWARE
-c tlcl.c -o tlcl.o
-o tlcl.o is redundant.
-Wall -Werror should be $(CFLAGS), no?

Well OK.  I didn't give good feedback.  I thought the additional target
(firmware) would be in the top-level Makefile (in ..).  Right now the other
compilations are started there---this Makefile was not meant to be invoked
directly.  If you do that, the Makefile structure becomes more uniform.  It
would be nice to use the same rule (the implicit rule) for all targets.

I am thinking of something like this:

ifeq ($(FIRMWARE), 1)
CFLAGS += -DFIRMWARE -ansi
STRUCTURES_H = saved-structures.h
else
STRUCTURES_H = structures.h
endif

tlcl.o: tlcl.c tlcl_internal.h tlcl.h $(STRUCTURES_H)

and no special firmware rule.

If you prefer to have the directory be self-contained, maybe it's cleaner to
have a completely separate Makefile.firmware.  Right now the only thing you
share is $(CC)---might as well.

Otherwise I'd keep it the way you had it earlier, with

firmware: tlcl.o

tlcl.o: tlcl.c etc.
       etc.

http://codereview.chromium.org/1787004/diff/17001/18005
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):

http://codereview.chromium.org/1787004/diff/17001/18005#newcode27
src/platform/tpm_lite/src/tlcl/tlcl.c:27: #ifndef FIRMWARE
Complete nit, and it started this way, but since there is both a then and an
else part, maybe switch them around and use #ifdef instead of #ifndef? 
Completely optional.

[gauravsh, 2010-04-27 21:00:50]

On 2010/04/27 20:52:21, Luigi Semenzato wrote:
> Sorry, forgot to publish + mail.
> 
> http://codereview.chromium.org/1787004/diff/17001/18003
> File src/platform/tpm_lite/src/tlcl/Makefile (right):
> 
> http://codereview.chromium.org/1787004/diff/17001/18003#newcode33
> src/platform/tpm_lite/src/tlcl/Makefile:33: $(CC) -Wall -Werror -ansi
-DFIRMWARE
> -c tlcl.c -o tlcl.o
> -o tlcl.o is redundant.
> -Wall -Werror should be $(CFLAGS), no?
> 
> Well OK.  I didn't give good feedback.  I thought the additional target
> (firmware) would be in the top-level Makefile (in ..).  Right now the other
> compilations are started there---this Makefile was not meant to be invoked
> directly.  If you do that, the Makefile structure becomes more uniform.  It
> would be nice to use the same rule (the implicit rule) for all targets.
> 
> I am thinking of something like this:
> 
> ifeq ($(FIRMWARE), 1)
> CFLAGS += -DFIRMWARE -ansi
> STRUCTURES_H = saved-structures.h
> else
> STRUCTURES_H = structures.h
> endif
> 
> tlcl.o: tlcl.c tlcl_internal.h tlcl.h $(STRUCTURES_H)
> 
> and no special firmware rule.
> 
> If you prefer to have the directory be self-contained, maybe it's cleaner to
> have a completely separate Makefile.firmware.  Right now the only thing you
> share is $(CC)---might as well.

I like this idea. It makes it clear what is needed to be done differently for
firmware compilation. Done.

> 
> Otherwise I'd keep it the way you had it earlier, with
> 
> firmware: tlcl.o
> 
> tlcl.o: tlcl.c etc.
>        etc.
> 
> http://codereview.chromium.org/1787004/diff/17001/18005
> File src/platform/tpm_lite/src/tlcl/tlcl.c (right):
> 
> http://codereview.chromium.org/1787004/diff/17001/18005#newcode27
> src/platform/tpm_lite/src/tlcl/tlcl.c:27: #ifndef FIRMWARE
> Complete nit, and it started this way, but since there is both a then and an
> else part, maybe switch them around and use #ifdef instead of #ifndef? 
> Completely optional.

Done.