From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Tue, 24 Apr 2018 14:53:58 +0200 Subject: [U-Boot] [PATCH v2 10/19] tpm: add TPM2_SelfTest command support In-Reply-To: References: <20180329074401.8691-1-miquel.raynal@bootlin.com> <20180329074401.8691-11-miquel.raynal@bootlin.com> Message-ID: <20180424145358.5f621d26@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de 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 wrote: > Hi Miquel, >=20 > On 29 March 2018 at 15:43, Miquel Raynal wrot= e: > > Add support for the TPM2_Selftest command. > > > > Change the command file and the help accordingly. > > > > Signed-off-by: Miquel Raynal > > --- > > 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 =3D 0x8002, > > }; > > > > +enum tpm2_yes_no { > > + TPMI_YES =3D 1, > > + TPMI_NO =3D 0, > > +}; > > + > > enum tpm_startup_type { > > TPM_ST_CLEAR =3D 0x0001, > > TPM_ST_STATE =3D 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] =3D { > > - 0x0, 0xc1, 0x0, 0x0, 0x0, 0xa, 0x0, 0x0, 0x0, 0x50, > > + const u8 command_v1[10] =3D { > > + U16_TO_ARRAY(0xc1), =20 >=20 > 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=C3=A8l >=20 > > + U32_TO_ARRAY(10), > > + U32_TO_ARRAY(0x50), > > }; > > - return tpm_sendrecv_command(command, NULL, NULL); > > + const u8 command_v2[12] =3D { > > + 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); > > } > > --=20 Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com