[PATCH] cmd: tpm: add a subcommand device

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Philippe,

Thanks for the patch!

Philippe Reynes <philippe.reynes@softathome.com> wrote on Wed,  8 Jan
2020 19:59:06 +0100:

> The command tpm (and tpm2) search the tpm and use it.
> On sandbox, there are two tpm (tpm 1.x and tpm 2.0).
> So the command tpm and tpm2 are always executed with
> the first tpm (tpm 1.x), and the command tpm2 always
> fails.

That's odd as I got the explicit request to make this possible and
at that time the tpm2 sandbox tests were working fine IIRC.

However, even if we wanted to be able to compile both tpmv1 and v2
stacks in the same build for sandboxing purposes, we all agreed that it
was not a real use case because there is (apparently) no sense in
having more than one TPM in a design for now.

Anyway, the patch looks fine (even if I don't like the idea of having a
global pointer but I don't see a better/simple idea), you'll find a few
minor comments below!
 
> 
> This add a subcommand device to command tpm and
> command tpm2. Then the command tpm and tpm2 use
> the device selected with the subcommand device.
> 
> To be compatible with previous behaviour, if the
> subcommand device is not used before a tpm (or tpm2)
> command, the device 0 is selected.
> 
> Signed-off-by: Philippe Reynes <philippe.reynes@softathome.com>
> ---
>  cmd/tpm-common.c     | 75 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  cmd/tpm-user-utils.h |  1 +
>  cmd/tpm-v1.c         |  3 +++
>  cmd/tpm-v2.c         |  3 +++
>  4 files changed, 78 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c
> index 38900fb..a99d438 100644
> --- a/cmd/tpm-common.c
> +++ b/cmd/tpm-common.c
> @@ -12,6 +12,8 @@
>  #include <tpm-common.h>
>  #include "tpm-user-utils.h"
>  
> +static struct udevice *tpm_dev;
> +
>  /**
>   * Print a byte string in hexdecimal format, 16-bytes per line.
>   *
> @@ -231,19 +233,84 @@ int type_string_write_vars(const char *type_str, u8 *data,
>  	return 0;
>  }
>  
> +static int tpm_show_device(void)
> +{
> +	struct udevice *dev;
> +	char buf[80];
> +	int n = 0, rc;
> +
> +	uclass_first_device(UCLASS_TPM, &dev);
> +	while (dev) {
> +		rc = tpm_get_desc(dev, buf, sizeof(buf));
> +		if (rc < 0)
> +			printf("device %d: can't get info\n", n);
> +		else
> +			printf("device %d: %s\n", n, buf);
> +
> +		uclass_next_device(&dev);

I wonder if there is a helper for that loop. Something like

        for_each_tpm_device(dev) {
                ...
        }

> +		n++;
> +	};
> +
> +	return 0;
> +}
> +
> +static int tpm_set_device(unsigned long num)
> +{
> +	struct udevice *dev;
> +	unsigned long n = 0;
> +	int rc = CMD_RET_FAILURE;
> +
> +	uclass_first_device(UCLASS_TPM, &dev);
> +	while (dev) {
> +		if (n == num) {
> +			tpm_dev = dev;

I would separate the research logic and the assignation logic.

        loop_for_research () {
                if (cond == found) {
                        rc = 0;
                        break;
                }
        }

	if (rc)
		return;

	tpm_dev = ...;

> +			rc = 0;
> +			break;
> +		}
> +
> +		uclass_next_device(&dev);
> +		n++;
> +	};
> +
> +	return rc;
> +}
> +
>  int get_tpm(struct udevice **devp)
>  {
>  	int rc;
>  
> -	rc = uclass_first_device_err(UCLASS_TPM, devp);
> -	if (rc) {
> -		printf("Could not find TPM (ret=%d)\n", rc);
> -		return CMD_RET_FAILURE;
> +	if (!tpm_dev) {
> +		rc = tpm_set_device(0);

Maybe a comment to state that this is for backward compatibility and 0
is a good enough default?

> +		if (rc < 0) {
> +			printf("Couldn't set TPM 0 (%d)\n", rc);
> +			return CMD_RET_FAILURE;
> +		}
>  	}
>  
> +	if (devp)
> +		*devp = tpm_dev;
> +
>  	return 0;
>  }
>  
> +int do_tpm_device(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	unsigned long num;
> +	int rc;
> +
> +	if (argc == 2) {
> +		num = simple_strtoul(argv[1], NULL, 10);
> +
> +		rc = tpm_set_device(num);
> +		if (rc < 0)
> +			printf("Couldn't set TPM %lu (%d)\n", num, rc);
> +	} else {
> +		rc = tpm_show_device();
> +	}
> +
> +	return rc;
> +}
> +
>  int do_tpm_info(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	struct udevice *dev;
> diff --git a/cmd/tpm-user-utils.h b/cmd/tpm-user-utils.h
> index 8ce9861..a851d9c 100644
> --- a/cmd/tpm-user-utils.h
> +++ b/cmd/tpm-user-utils.h
> @@ -17,6 +17,7 @@ int type_string_pack(const char *type_str, char * const values[], u8 *data);
>  int type_string_write_vars(const char *type_str, u8 *data, char * const vars[]);
>  int get_tpm(struct udevice **devp);
>  
> +int do_tpm_device(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]);
>  int do_tpm_init(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]);
>  int do_tpm_info(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]);
>  int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c
> index 2807331..bc34e06 100644
> --- a/cmd/tpm-v1.c
> +++ b/cmd/tpm-v1.c
> @@ -645,6 +645,7 @@ TPM_COMMAND_NO_ARG(tpm_physical_enable)
>  TPM_COMMAND_NO_ARG(tpm_physical_disable)
>  
>  static cmd_tbl_t tpm1_commands[] = {
> +	U_BOOT_CMD_MKENT(device, 0, 1, do_tpm_device, "", ""),
>  	U_BOOT_CMD_MKENT(info, 0, 1, do_tpm_info, "", ""),
>  	U_BOOT_CMD_MKENT(init, 0, 1, do_tpm_init, "", ""),
>  	U_BOOT_CMD_MKENT(startup, 0, 1,
> @@ -721,6 +722,8 @@ U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm,
>  "cmd args...\n"
>  "    - Issue TPM command <cmd> with arguments <args...>.\n"
>  "Admin Startup and State Commands:\n"
> +"  device [num device]\n"
> +"    - Show all devices or set the specified device\n"
>  "  info - Show information about the TPM\n"
>  "  init\n"
>  "    - Put TPM into a state where it waits for 'startup' command.\n"
> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> index 459a955..0cd3982 100644
> --- a/cmd/tpm-v2.c
> +++ b/cmd/tpm-v2.c
> @@ -354,6 +354,7 @@ static int do_tpm_pcr_setauthvalue(cmd_tbl_t *cmdtp, int flag,
>  }
>  
>  static cmd_tbl_t tpm2_commands[] = {
> +	U_BOOT_CMD_MKENT(device, 0, 1, do_tpm_device, "", ""),
>  	U_BOOT_CMD_MKENT(info, 0, 1, do_tpm_info, "", ""),
>  	U_BOOT_CMD_MKENT(init, 0, 1, do_tpm_init, "", ""),
>  	U_BOOT_CMD_MKENT(startup, 0, 1, do_tpm2_startup, "", ""),
> @@ -381,6 +382,8 @@ cmd_tbl_t *get_tpm2_commands(unsigned int *size)
>  U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
>  "<command> [<arguments>]\n"
>  "\n"
> +"device [num device]\n"
> +"    Show all devices or set the specified device\n"
>  "info\n"
>  "    Show information about the TPM.\n"
>  "init\n"

Thanks,
Miquèl