Hi Miquel, On 27 April 2018 at 07:39, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > Hi Simon, > > On Thu, 26 Apr 2018 08:40:27 -0600, Simon Glass <s...@chromium.org> > wrote: > >> Hi Miquel, >> >> On 24 April 2018 at 07:17, Miquel Raynal <miquel.ray...@bootlin.com> wrote: >> > Hi Simon, >> > >> > On Fri, 30 Mar 2018 06:42:32 +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_Clear command. >> >> > >> >> > Change the command file and the help accordingly. >> >> > >> >> > Signed-off-by: Miquel Raynal <miquel.ray...@bootlin.com> >> >> > --- >> >> > cmd/tpm.c | 29 ++++++++++++++++++++++++++--- >> >> > cmd/tpm_test.c | 6 +++--- >> >> > include/tpm.h | 21 +++++++++++++++++---- >> >> > lib/tpm.c | 42 ++++++++++++++++++++++++++++++++++++++---- >> >> > 4 files changed, 84 insertions(+), 14 deletions(-) >> >> > >> >> > diff --git a/cmd/tpm.c b/cmd/tpm.c >> >> > index fc9ef9d4a3..32921e1a70 100644 >> >> > --- a/cmd/tpm.c >> >> > +++ b/cmd/tpm.c >> >> > @@ -454,6 +454,29 @@ static int do_tpm_init(cmd_tbl_t *cmdtp, int flag, >> >> > return report_return_code(tpm_init()); >> >> > } >> >> > >> >> > +static int do_tpm_force_clear(cmd_tbl_t *cmdtp, int flag, >> >> > + int argc, char * const argv[]) >> >> > +{ >> >> > + u32 handle = 0; >> >> > + const char *pw = (argc < 3) ? NULL : argv[2]; >> >> > + const ssize_t pw_sz = pw ? strlen(pw) : 0; >> >> > + >> >> > + if (argc < 2 || argc > 3) >> >> > + return CMD_RET_USAGE; >> >> > + >> >> > + if (pw_sz > TPM2_DIGEST_LENGTH) >> >> > + return -EINVAL; >> >> > + >> >> > + if (!strcasecmp("TPM2_RH_LOCKOUT", argv[1])) >> >> > + handle = TPM2_RH_LOCKOUT; >> >> > + else if (!strcasecmp("TPM2_RH_PLATFORM", argv[1])) >> >> > + handle = TPM2_RH_PLATFORM; >> >> > + else >> >> > + return CMD_RET_USAGE; >> >> > + >> >> > + return report_return_code(tpm_force_clear(handle, pw, pw_sz)); >> >> > +} >> >> > + >> >> > #define TPM_COMMAND_NO_ARG(cmd) \ >> >> > static int do_##cmd(cmd_tbl_t *cmdtp, int flag, \ >> >> > int argc, char * const argv[]) \ >> >> > @@ -465,7 +488,6 @@ static int do_##cmd(cmd_tbl_t *cmdtp, int flag, >> >> > \ >> >> > >> >> > TPM_COMMAND_NO_ARG(tpm_self_test_full) >> >> > TPM_COMMAND_NO_ARG(tpm_continue_self_test) >> >> > -TPM_COMMAND_NO_ARG(tpm_force_clear) >> >> > TPM_COMMAND_NO_ARG(tpm_physical_enable) >> >> > TPM_COMMAND_NO_ARG(tpm_physical_disable) >> >> > >> >> > @@ -951,8 +973,9 @@ U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm, >> >> > " physical_set_deactivated 0|1\n" >> >> > " - Set deactivated flag.\n" >> >> > "Admin Ownership Commands:\n" >> >> > -" force_clear\n" >> >> > -" - Issue TPM_ForceClear command.\n" >> >> > +" force_clear [<type>]\n" >> >> > +" - Issue TPM_[Force]Clear command, with <type> one of (TPMv2 >> >> > only):\n" >> >> > +" * TPM2_RH_LOCKOUT, TPM2_RH_PLATFORM.\n" >> >> > " tsc_physical_presence flags\n" >> >> > " - Set TPM device's Physical Presence flags to <flags>.\n" >> >> > "The Capability Commands:\n" >> >> > diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c >> >> > index 37ad2ff33d..da40dbc423 100644 >> >> > --- a/cmd/tpm_test.c >> >> > +++ b/cmd/tpm_test.c >> >> > @@ -176,7 +176,7 @@ static int test_fast_enable(void) >> >> > TPM_CHECK(tpm_get_flags(&disable, &deactivated, NULL)); >> >> > printf("\tdisable is %d, deactivated is %d\n", disable, >> >> > deactivated); >> >> > for (i = 0; i < 2; i++) { >> >> > - TPM_CHECK(tpm_force_clear()); >> >> > + TPM_CHECK(tpm_force_clear(0, NULL, 0)); >> >> > TPM_CHECK(tpm_get_flags(&disable, &deactivated, NULL)); >> >> > printf("\tdisable is %d, deactivated is %d\n", disable, >> >> > deactivated); >> >> > @@ -458,7 +458,7 @@ static int test_write_limit(void) >> >> > TPM_CHECK(TlclStartupIfNeeded()); >> >> > TPM_CHECK(tpm_self_test_full()); >> >> > TPM_CHECK(tpm_tsc_physical_presence(PRESENCE)); >> >> > - TPM_CHECK(tpm_force_clear()); >> >> > + TPM_CHECK(tpm_force_clear(0, NULL, 0)); >> >> > TPM_CHECK(tpm_physical_enable()); >> >> > TPM_CHECK(tpm_physical_set_deactivated(0)); >> >> > >> >> > @@ -477,7 +477,7 @@ static int test_write_limit(void) >> >> > } >> >> > >> >> > /* Reset write count */ >> >> > - TPM_CHECK(tpm_force_clear()); >> >> > + TPM_CHECK(tpm_force_clear(0, NULL, 0)); >> >> > TPM_CHECK(tpm_physical_enable()); >> >> > TPM_CHECK(tpm_physical_set_deactivated(0)); >> >> > >> >> > diff --git a/include/tpm.h b/include/tpm.h >> >> > index 38d7cb899d..2f17166662 100644 >> >> > --- a/include/tpm.h >> >> > +++ b/include/tpm.h >> >> > @@ -43,13 +43,23 @@ enum tpm_startup_type { >> >> > }; >> >> > >> >> > enum tpm2_startup_types { >> >> > - TPM2_SU_CLEAR = 0x0000, >> >> > - TPM2_SU_STATE = 0x0001, >> >> > + TPM2_SU_CLEAR = 0x0000, >> >> > + TPM2_SU_STATE = 0x0001, >> >> > +}; >> >> > + >> >> > +enum tpm2_handles { >> >> >> >> Please add comment to enum >> > >> > Fine, I will document all of them. Just for you to know, these are >> > values extracted from the (publicly available) specification, I did >> > not change any of them. >> > >> >> >> >> > + TPM2_RH_OWNER = 0x40000001, >> >> > + TPM2_RS_PW = 0x40000009, >> >> > + TPM2_RH_LOCKOUT = 0x4000000A, >> >> > + TPM2_RH_ENDORSEMENT = 0x4000000B, >> >> > + TPM2_RH_PLATFORM = 0x4000000C, >> >> > }; >> >> > >> >> > enum tpm2_command_codes { >> >> > TPM2_CC_STARTUP = 0x0144, >> >> > TPM2_CC_SELF_TEST = 0x0143, >> >> > + TPM2_CC_CLEAR = 0x0126, >> >> > + TPM2_CC_CLEARCONTROL = 0x0127, >> >> > TPM2_CC_GET_CAPABILITY = 0x017A, >> >> > TPM2_CC_PCR_READ = 0x017E, >> >> > TPM2_CC_PCR_EXTEND = 0x0182, >> >> > @@ -567,11 +577,14 @@ uint32_t tpm_tsc_physical_presence(uint16_t >> >> > presence); >> >> > uint32_t tpm_read_pubek(void *data, size_t count); >> >> > >> >> > /** >> >> > - * Issue a TPM_ForceClear command. >> >> > + * Issue a TPM_ForceClear or a TPM2_Clear command. >> >> > * >> >> > + * @param handle Handle >> >> > + * @param pw Password >> >> > + * @param pw_sz Length of the password >> >> > * @return return code of the operation >> >> > */ >> >> > -uint32_t tpm_force_clear(void); >> >> > +int tpm_force_clear(u32 handle, const char *pw, const ssize_t pw_sz); >> >> > >> >> > /** >> >> > * Issue a TPM_PhysicalEnable command. >> >> > diff --git a/lib/tpm.c b/lib/tpm.c >> >> > index 3cc2888267..9a46ac09e6 100644 >> >> > --- a/lib/tpm.c >> >> > +++ b/lib/tpm.c >> >> > @@ -608,13 +608,47 @@ uint32_t tpm_read_pubek(void *data, size_t count) >> >> > return 0; >> >> > } >> >> > >> >> > -uint32_t tpm_force_clear(void) >> >> > +int tpm_force_clear(u32 handle, const char *pw, const ssize_t pw_sz) >> >> > { >> >> > - const uint8_t command[10] = { >> >> > - 0x0, 0xc1, 0x0, 0x0, 0x0, 0xa, 0x0, 0x0, 0x0, 0x5d, >> >> > + const u8 command_v1[10] = { >> >> > + U16_TO_ARRAY(0xc1), >> >> > + U32_TO_ARRAY(10), >> >> > + U32_TO_ARRAY(0x5d), >> >> > }; >> >> > + u8 command_v2[COMMAND_BUFFER_SIZE] = { >> >> > + U16_TO_ARRAY(TPM2_ST_SESSIONS), /* TAG */ >> >> > + U32_TO_ARRAY(27 + pw_sz), /* Length */ >> >> > + U32_TO_ARRAY(TPM2_CC_CLEAR), /* Command code */ >> >> >> >> Here we have both v1 and v2 commands. Perhaps we should have a Kconfig >> >> option to enable v2 support? Or do you think it is not a big addition >> >> in terms of code side? >> > >> > It is a big addition in terms of code side. >> > >> >> >> >> One way to do this would be to have an inline function which can tell >> >> if you are using v2: >> >> >> >> static inline bool tpm_v2(void) { >> >> return IS_ENABLED(CONFIG_TPM_V2) && further things... >> >> } >> >> >> >> static inline bool tpm_v1(void) { >> >> return IS_ENABLED(CONFIG_TPM_V1) && further things... >> >> } >> > >> > I like this way of choosing one specification or the other, I could >> > make them mutually exclusive. It would prevent the need for a global >> > variable (or an additional field in uclass). >> > >> > Would it be fine to actually rename the file (with a 'tpm1' suffix) and >> > create a new one specific for TPMv2 commands ? Only one file would be >> > compiled/linked depending on the configuration, avoiding the possibility >> > to call a v1 command from a v2 chip and vice versa. >> > >> > At first I thought a lot of code would be shared but I don't think we >> > would add so much by having one function per revision now. >> >> Well you could have two separate files, if there really is no sharing >> of commands. But if there are common commands, you should have a >> 'common' file to implement them, rather than duplicating code. >> >> With the above static inline functions we can support: >> >> - v1 only (no code-size increment) >> - v2 only (minimal code size for what will be a common case) >> - v1 + v2 (e.g. for sandbox testing) > > I am changing the whole architecture as you suggested. > > Now v1 and v2 are chosen with a Kconfig menu, common code is in a > tpm-common.c file, and commands/library functions for each > specification is in tpm-v1.c and tpm-v2.c (with all the needed headers. > This way TPMv1 code is absolutely untouched while it allows TPMv2 > support to be introduced independently. > > You'll tell me how you find this alternative, I will soon send a v3. > > Otherwise, as suggested in a first answer, I changed U32_TO_array
I think it works OK as you have it now, with the rename. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot