[U-Boot] [PATCH v2 10/19] tpm: add TPM2_SelfTest command support

[Miquel Raynal]

Hi Simon,

On Thu, 26 Apr 2018 08:40:24 -0600, Simon Glass <sjg at chromium.org>
wrote:

> Hi Miquel,
> 
> On 24 April 2018 at 06:53, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> > Hi Simon,
> >
> > I am back on that topic, let me answer some of your questions before
> > addressing them in a next version.
> >
> > On Fri, 30 Mar 2018 06:42:25 +0800, Simon Glass
> > <sjg at chromium.org> wrote:
> >  
> >> Hi Miquel,
> >>
> >> On 29 March 2018 at 15:43, Miquel Raynal <miquel.raynal at bootlin.com> wrote:  
> >> > Add support for the TPM2_Selftest command.
> >> >
> >> > Change the command file and the help accordingly.
> >> >
> >> > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> >> > ---
> >> >  include/tpm.h |  9 +++++++--
> >> >  lib/tpm.c     | 36 ++++++++++++++++++++++++++++--------
> >> >  2 files changed, 35 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/include/tpm.h b/include/tpm.h
> >> > index ba71bac885..38d7cb899d 100644
> >> > --- a/include/tpm.h
> >> > +++ b/include/tpm.h
> >> > @@ -31,6 +31,11 @@ enum tpm2_structures {
> >> >         TPM2_ST_SESSIONS        = 0x8002,
> >> >  };
> >> >
> >> > +enum tpm2_yes_no {
> >> > +       TPMI_YES                = 1,
> >> > +       TPMI_NO                 = 0,
> >> > +};
> >> > +
> >> >  enum tpm_startup_type {
> >> >         TPM_ST_CLEAR            = 0x0001,
> >> >         TPM_ST_STATE            = 0x0002,
> >> > @@ -476,14 +481,14 @@ int tpm_startup(enum tpm_startup_type mode);
> >> >   *
> >> >   * @return return code of the operation
> >> >   */
> >> > -uint32_t tpm_self_test_full(void);
> >> > +int tpm_self_test_full(void);
> >> >
> >> >  /**
> >> >   * Issue a TPM_ContinueSelfTest command.
> >> >   *
> >> >   * @return return code of the operation
> >> >   */
> >> > -uint32_t tpm_continue_self_test(void);
> >> > +int tpm_continue_self_test(void);
> >> >
> >> >  /**
> >> >   * Issue a TPM_NV_DefineSpace command.  The implementation is limited
> >> > diff --git a/lib/tpm.c b/lib/tpm.c
> >> > index 0c57ef8dc7..3cc2888267 100644
> >> > --- a/lib/tpm.c
> >> > +++ b/lib/tpm.c
> >> > @@ -341,20 +341,40 @@ int tpm_startup(enum tpm_startup_type mode)
> >> >         return 0;
> >> >  }
> >> >
> >> > -uint32_t tpm_self_test_full(void)
> >> > +int tpm_self_test_full(void)
> >> >  {
> >> > -       const uint8_t command[10] = {
> >> > -               0x0, 0xc1, 0x0, 0x0, 0x0, 0xa, 0x0, 0x0, 0x0, 0x50,
> >> > +       const u8 command_v1[10] = {
> >> > +               U16_TO_ARRAY(0xc1),  
> >>
> >> Here I can see some benefit to your macros because the data is better
> >> structured. But why not use the pack_byte_string() function?  
> >
> > TPM buffers (1.0 and 2.0) are, most of the time, filled with data of
> > known length (1, 2 or 4 bytes). You can see that most of the time,
> > TPMv1 simple commands were just filling a buffer of bytes. I don't like
> > very much the fact that the user should split the data in byte array so
> > I introduced these macros that do it for me in a cleaner way. When you
> > get an hexadecimal value (like it was the case before) it is easy to
> > split a "0xabcd" in "0xab, 0xcd", but I prefer to use variables or
> > simply defines sometimes and writing "TPM2_ST_NO_SESSIONS" as
> > "TPM2_ST_NO_SESSIONS >> 8, TPM2_ST_NO_SESSIONS & 0xFF" is not readable
> > at all.
> >
> > Now why did I not use the existing pack_byte_string() function. Well,
> > at first I did and realized it was a pain to have a decent
> > incrementation of the offsets, mostly when commands tend to be longer
> > and longer. Having such lists of offset/variable/length while you could
> > define them statically on the stack -which also saves some computing
> > time- is quickly annoying and hard to review. From my point of view this
> > function is a real asset when it comes to 'unknown'/'big' variable
> > length (like a password, an hmac, an user input, etc) but should be
> > avoided when not necessary: typically to fill a buffer with defined
> > values.
> >
> > I am of course very open on the topic, this is my feeling but I don't
> > have that much experience and would carefully listen to yours.  
> 
> I don't think this is an unreasonable point of view. Either works.
> 
> But if you are changing the approach to use static data, should you
> convert the existing code to the new scheme?

I don't think this is relevant anymore as files have been split. You'll
tell me what you think with the next version (need to do some testing
and I'll be done with it).

Regards,
Miquèl

> 
> Regards,
> Simon



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com