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 <s...@chromium.org> wrote: > Hi Miquel, > > On 29 March 2018 at 15:43, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > > Add support for the TPM2_Selftest command. > > > > Change the command file and the help accordingly. > > > > Signed-off-by: Miquel Raynal <miquel.ray...@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. Thank you, Miquèl > > > + U32_TO_ARRAY(10), > > + U32_TO_ARRAY(0x50), > > }; > > - return tpm_sendrecv_command(command, NULL, NULL); > > + const u8 command_v2[12] = { > > + U16_TO_ARRAY(TPM2_ST_NO_SESSIONS), > > + U32_TO_ARRAY(11), > > + U32_TO_ARRAY(TPM2_CC_SELF_TEST), > > + TPMI_YES, > > + }; > > + > > + return tpm_sendrecv_command(is_tpmv2 ? command_v2 : command_v1, > > NULL, > > + NULL); > > } > > -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot