Hi Miquel,
On 24 April 2018 at 06:53, Miquel Raynal miquel.raynal@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@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.
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?
Regards, Simon