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@chromium.org wrote:
Hi Miquel,
On 29 March 2018 at 15:43, Miquel Raynal miquel.raynal@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@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);}