1. Changes and additions for autotest; 2. Two-level write-free locking test.

1. Changes and additions for autotest; 2. Two-level write-free locking test.

[Luigi Semenzato, 2010-04-01 17:28:31]

Hi Gaurav, I will appreciate your feedback on this.  Thanks.

[gauravsh, 2010-04-01 20:33:46]

http://codereview.chromium.org/1558015/diff/1/2
File src/platform/tpm_lite/src/include/tlcl.h (right):

http://codereview.chromium.org/1558015/diff/1/2#newcode58
src/platform/tpm_lite/src/include/tlcl.h:58: /* Log to stdout unless -q is
passed to TlclLibInit.  Arguments like printf.
-q is for quiet? -v seems more like that standard command line flag for
specifying log verbosity.

http://codereview.chromium.org/1558015/diff/1/3
File src/platform/tpm_lite/src/testsuite/Makefile (right):

http://codereview.chromium.org/1558015/diff/1/3#newcode29
src/platform/tpm_lite/src/testsuite/Makefile:29: tpmtest_lock \
time to organize in alpha order?

http://codereview.chromium.org/1558015/diff/1/4
File src/platform/tpm_lite/src/testsuite/clear.c (right):

http://codereview.chromium.org/1558015/diff/1/4#newcode10
src/platform/tpm_lite/src/testsuite/clear.c:10: #include <stdint.h>
nit: alpha order

http://codereview.chromium.org/1558015/diff/1/5
File src/platform/tpm_lite/src/testsuite/enable.c (right):

http://codereview.chromium.org/1558015/diff/1/5#newcode10
src/platform/tpm_lite/src/testsuite/enable.c:10: #include <stdint.h>
nit: alpha order

http://codereview.chromium.org/1558015/diff/1/6
File src/platform/tpm_lite/src/testsuite/globallock.c (right):

http://codereview.chromium.org/1558015/diff/1/6#newcode49
src/platform/tpm_lite/src/testsuite/globallock.c:49: result =
TlclWrite(TPM_NV_INDEX0, (uint8_t*) &x, 0);
The global lock is set via a write of 0 bytes to TPM_NV_INDEX0? Should this or
other indices be renamed - it's not clear what the different between INDEX0 and
TPM_NV_INDEX0.

I think it would also be better to have an API call that sets the global lock.

http://codereview.chromium.org/1558015/diff/1/9
File src/platform/tpm_lite/src/testsuite/writelimit.c (right):

http://codereview.chromium.org/1558015/diff/1/9#newcode24
src/platform/tpm_lite/src/testsuite/writelimit.c:24: TlclLibInit(argc, argv);
see my comment about tlcllibinit(.

If you need argument processing for all the tests, create a separate function
which sets up tlcl_quiet and put it in a common file used by all the tests. Or,
another way is to make tlcl_quiet based on a compile time macro.

http://codereview.chromium.org/1558015/diff/1/11
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):

http://codereview.chromium.org/1558015/diff/1/11#newcode47
src/platform/tpm_lite/src/tlcl/tlcl.c:47: if (! tlcl_quiet) {
nit: space between ! and tlcl_quiet

http://codereview.chromium.org/1558015/diff/1/11#newcode204
src/platform/tpm_lite/src/tlcl/tlcl.c:204: void TlclLibInit(int argc, char**
argv) {
Why conflate command line processing with TlclLibInit()? Since this is a library
that will be used by other code, this should be done by the calling program, not
the library.

[Luigi Semenzato, 2010-04-02 01:10:33]

Thank you for the feedback.  I agree with everything except I am arguing that it
may be better to leave the verbose output on by default, but I'll change it if
you argue back :-)

http://codereview.chromium.org/1558015/diff/1/2
File src/platform/tpm_lite/src/include/tlcl.h (right):

http://codereview.chromium.org/1558015/diff/1/2#newcode58
src/platform/tpm_lite/src/include/tlcl.h:58: /* Log to stdout unless -q is
passed to TlclLibInit.  Arguments like printf.
On 2010/04/01 20:33:46, gauravsh wrote:
> -q is for quiet? -v seems more like that standard command line flag for
> specifying log verbosity.

Yes---I started out thinking that way, then I concluded that for an autotest, it
is better to be verbose as the default, so there is more information if
something goes wrong.  In fact I don't use -q anywhere.  Shall I just take it
out?  If I change it to -v, I will need to change all the autotest invocations
to use -v.  What do you think?

http://codereview.chromium.org/1558015/diff/1/3
File src/platform/tpm_lite/src/testsuite/Makefile (right):

http://codereview.chromium.org/1558015/diff/1/3#newcode29
src/platform/tpm_lite/src/testsuite/Makefile:29: tpmtest_lock \
On 2010/04/01 20:33:46, gauravsh wrote:
> time to organize in alpha order?

Indeed.  Thanks.

http://codereview.chromium.org/1558015/diff/1/4
File src/platform/tpm_lite/src/testsuite/clear.c (right):

http://codereview.chromium.org/1558015/diff/1/4#newcode10
src/platform/tpm_lite/src/testsuite/clear.c:10: #include <stdint.h>
On 2010/04/01 20:33:46, gauravsh wrote:
> nit: alpha order

Thanks.  They actually all need to go away.

http://codereview.chromium.org/1558015/diff/1/6
File src/platform/tpm_lite/src/testsuite/globallock.c (right):

http://codereview.chromium.org/1558015/diff/1/6#newcode49
src/platform/tpm_lite/src/testsuite/globallock.c:49: result =
TlclWrite(TPM_NV_INDEX0, (uint8_t*) &x, 0);
On 2010/04/01 20:33:46, gauravsh wrote:
> The global lock is set via a write of 0 bytes to TPM_NV_INDEX0? Should this or
> other indices be renamed - it's not clear what the different between INDEX0
and
> TPM_NV_INDEX0.
> 
> I think it would also be better to have an API call that sets the global lock.


Excellent points.  I am adding the separate API.  I hope this removes the INDEX0
confusion concern.

http://codereview.chromium.org/1558015/diff/1/9
File src/platform/tpm_lite/src/testsuite/writelimit.c (right):

http://codereview.chromium.org/1558015/diff/1/9#newcode24
src/platform/tpm_lite/src/testsuite/writelimit.c:24: TlclLibInit(argc, argv);
On 2010/04/01 20:33:46, gauravsh wrote:
> see my comment about tlcllibinit(.
> 
> If you need argument processing for all the tests, create a separate function
> which sets up tlcl_quiet and put it in a common file used by all the tests.
Or,
> another way is to make tlcl_quiet based on a compile time macro.

Thanks.  I added TlclSetLogLevel().

[gauravsh, 2010-04-02 01:22:58]

LGTM

verbose by default is good.

http://codereview.chromium.org/1558015/diff/1/2
File src/platform/tpm_lite/src/include/tlcl.h (right):

http://codereview.chromium.org/1558015/diff/1/2#newcode58
src/platform/tpm_lite/src/include/tlcl.h:58: /* Log to stdout unless -q is
passed to TlclLibInit.  Arguments like printf.
On 2010/04/02 01:10:33, Luigi Semenzato wrote:
> On 2010/04/01 20:33:46, gauravsh wrote:
> > -q is for quiet? -v seems more like that standard command line flag for
> > specifying log verbosity.
> 
> Yes---I started out thinking that way, then I concluded that for an autotest,
it
> is better to be verbose as the default, so there is more information if
> something goes wrong.  In fact I don't use -q anywhere.  Shall I just take it
> out?  If I change it to -v, I will need to change all the autotest invocations
> to use -v.  What do you think?
> 
> 
Since TlclLog will usually be invoked explicitly by tests, it's fine to be
verbose by default. So removing the -q option seems ok to me.

http://codereview.chromium.org/1558015/diff/11001/12010
File src/platform/tpm_lite/src/tlcl/tlcl.c (right):

http://codereview.chromium.org/1558015/diff/11001/12010#newcode349
src/platform/tpm_lite/src/tlcl/tlcl.c:349: uint32_t result =
TlclWrite(TPM_NV_INDEX0, (uint8_t*) &x, 0);
Where is TPM_NV_INDEX0 defined? I can't seem to find it anywhere.

[Luigi Semenzato, 2010-04-02 04:52:55]

On Thu, Apr 1, 2010 at 6:22 PM,  <gauravsh@chromium.org> wrote:

> http://codereview.chromium.org/1558015/diff/11001/12010#newcode349
> src/platform/tpm_lite/src/tlcl/tlcl.c:349: uint32_t result =
> TlclWrite(TPM_NV_INDEX0, (uint8_t*) &x, 0);
> Where is TPM_NV_INDEX0 defined? I can't seem to find it anywhere.
>
> http://codereview.chromium.org/1558015
>

It's in /usr/include/tss/tpm.h.  It's defined by the TCG. :-)

[Luigi Semenzato, 2010-04-02 15:42:00]

Would you mind taking a quick look at the Makefile?  I had to make a small
change to make it compile under ARM.  The problem is that generator.c is
compiled locally and the rest is cross-compiled.  Thanks!

[gauravsh, 2010-04-02 20:57:46]

lgtm