Hi Miquel,
On 29 March 2018 at 15:43, Miquel Raynal <miquel.ray...@bootlin.com> wrote:
> Later choice between v1 and v2 compliant functions will be handled by a
> global value in lib/tpm.c that will be accessible through set/get
> accessors from lib/cmd.c.
>
> This global value is set during the initialization phase if the TPM2
> handle is given to the init command.
>
> Signed-off-by: Miquel Raynal <miquel.ray...@bootlin.com>
> ---
> cmd/tpm.c | 37 +++++++++++++++++++++-----
> include/tpm.h | 20 ++++++++++++++
> lib/tpm.c | 83
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 131 insertions(+), 9 deletions(-)
>
> diff --git a/cmd/tpm.c b/cmd/tpm.c
> index f456396d75..1d32028b64 100644
> --- a/cmd/tpm.c
> +++ b/cmd/tpm.c
> @@ -89,12 +89,16 @@ static void *parse_byte_string(char *bytes, uint8_t
> *data, size_t *count_ptr)
> */
> static int report_return_code(int return_code)
> {
> - if (return_code) {
> - printf("Error: %d\n", return_code);
> - return CMD_RET_FAILURE;
> - } else {
> + if (!return_code)
> return CMD_RET_SUCCESS;
> - }
> +
> + if (return_code == -EOPNOTSUPP)
> + printf("TPMv%d error: unavailable command with this spec\n",
> + tpm_get_specification());
> + else
> + printf("TPM error: %d\n", return_code);
> +
> + return CMD_RET_FAILURE;
> }
>
> /**
> @@ -427,6 +431,24 @@ static int do_tpm_get_capability(cmd_tbl_t *cmdtp, int
> flag,
> return report_return_code(rc);
> }
>
> +static int do_tpm_init(cmd_tbl_t *cmdtp, int flag,
> + int argc, char * const argv[])
> +{
> + if (argc > 2)
> + return CMD_RET_USAGE;
> +
> + if (argc == 2) {
> + if (!strcasecmp("TPM1", argv[1]))
> + tpm_set_specification(1);
> + else if (!strcasecmp("TPM2", argv[1]))
> + tpm_set_specification(2);
> + else
> + return CMD_RET_USAGE;
> + }
> +
I don't like the idea of setting a global before calling tpm_init().
Can we instead make it an arg to tpm_init()?
Also, instead of 1 and 2, can you create an enum?
> + return report_return_code(tpm_init());
> +}
> +
> #define TPM_COMMAND_NO_ARG(cmd) \
> static int do_##cmd(cmd_tbl_t *cmdtp, int flag, \
> int argc, char * const argv[]) \
> @@ -436,7 +458,6 @@ static int do_##cmd(cmd_tbl_t *cmdtp, int flag,
> \
> return report_return_code(cmd()); \
> }
>
> -TPM_COMMAND_NO_ARG(tpm_init)
> TPM_COMMAND_NO_ARG(tpm_self_test_full)
> TPM_COMMAND_NO_ARG(tpm_continue_self_test)
> TPM_COMMAND_NO_ARG(tpm_force_clear)
> @@ -902,8 +923,10 @@ U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm,
> " - Issue TPM command <cmd> with arguments <args...>.\n"
> "Admin Startup and State Commands:\n"
> " info - Show information about the TPM\n"
> -" init\n"
> +" init [<type>]\n"
> " - Put TPM into a state where it waits for 'startup' command.\n"
> +" <type> is one of TPM1 (default) or TPM2. This choice impacts the
> way\n"
> +" other functions will behave.\n"
> " startup mode\n"
> " - Issue TPM_Starup command. <mode> is one of TPM_ST_CLEAR,\n"
> " TPM_ST_STATE, and TPM_ST_DEACTIVATED.\n"
> diff --git a/include/tpm.h b/include/tpm.h
> index 760d94865c..0ec3428ea4 100644
> --- a/include/tpm.h
> +++ b/include/tpm.h
> @@ -30,6 +30,11 @@ enum tpm_startup_type {
> TPM_ST_DEACTIVATED = 0x0003,
> };
>
> +enum tpm2_startup_types {
Please add a comment as to what this is for.
> + TPM2_SU_CLEAR = 0x0000,
> + TPM2_SU_STATE = 0x0001,
> +};
> +
> enum tpm_physical_presence {
> TPM_PHYSICAL_PRESENCE_HW_DISABLE = 0x0200,
> TPM_PHYSICAL_PRESENCE_CMD_DISABLE = 0x0100,
> @@ -417,6 +422,21 @@ int tpm_xfer(struct udevice *dev, const uint8_t
> *sendbuf, size_t send_size,
> */
> int tpm_init(void);
>
> +/**
> + * Assign a value to the global is_nfcv2 boolean to discriminate in the lib
> + * between the available specifications.
> + *
> + * @version: 1 or 2, depending on the supported specification
> + */
> +int tpm_set_specification(int version);
> +
> +/**
> + * Return the current value of the specification.
> + *
> + * @return: 1 or 2, depending on the supported specification
> + */
> +int tpm_get_specification(void);
> +
> /**
> * Issue a TPM_Startup command.
> *
> diff --git a/lib/tpm.c b/lib/tpm.c
> index 99556b1cf6..38b76b4961 100644
> --- a/lib/tpm.c
> +++ b/lib/tpm.c
> @@ -15,6 +15,9 @@
> /* Internal error of TPM command library */
> #define TPM_LIB_ERROR ((uint32_t)~0u)
>
> +/* Global boolean to discriminate TPMv2.x from TPMv1.x functions */
> +static bool is_tpmv2;
Can this go in the TPM uclass as uclass-private data? See struct
uclass member 'priv'.
> +
> /* Useful constants */
> enum {
> COMMAND_BUFFER_SIZE = 256,
> @@ -262,6 +265,26 @@ static uint32_t tpm_sendrecv_command(const void *command,
> return tpm_return_code(response);
> }
>
> +int tpm_set_specification(int version)
> +{
> + if (version == 1) {
> + debug("TPM complies to the v1.x specification\n");
> + is_tpmv2 = false;
> + } else if (version == 2) {
> + debug("TPM complies to the v2.x specification\n");
> + is_tpmv2 = true;
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int tpm_get_specification(void)
> +{
> + return is_tpmv2 ? 2 : 1;
> +}
> +
> int tpm_init(void)
> {
> int err;
> @@ -338,6 +361,9 @@ uint32_t tpm_nv_define_space(uint32_t index, uint32_t
> perm, uint32_t size)
> const size_t size_offset = 77;
> uint8_t buf[COMMAND_BUFFER_SIZE];
>
> + if (is_tpmv2)
> + return -EOPNOTSUPP;
> +
> if (pack_byte_string(buf, sizeof(buf), "sddd",
> 0, command, sizeof(command),
> index_offset, index,
> @@ -362,6 +388,9 @@ uint32_t tpm_nv_read_value(uint32_t index, void *data,
> uint32_t count)
> uint32_t data_size;
> uint32_t err;
>
> + if (is_tpmv2)
> + return -EOPNOTSUPP;
This return code should be mentioned in the header file for these functions.
[...]
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
