[U-Boot] [PATCH 3/6] tpm: allow TPM v1 and v2 to be compiled at the same time
Simon Glass
sjg at chromium.org
Thu Jul 19 01:31:41 UTC 2018
Hi Miquel,
On 14 July 2018 at 06:16, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> While there is probably no reason to do so in a real life situation, it
> will allow to compile test both stacks with the same sandbox defconfig.
>
> As we cannot define two 'tpm' commands at the same time, the command for
> TPM v1 is still called 'tpm' and the one for TPM v2 'tpm2'. While this
> is the exact command name that must be written into eg. test files, any
> user already using the TPM v2 stack can continue using just 'tpm' as
> command as long as TPM v1 support is not compiled (because U-Boot prompt
> will search for the closest command named after 'tpm'.b)
>
> The command set can also be changed at runtime (not supported yet, but
> ready to be), but as one can compile only either one stack or the other,
> there is still one spot in the code where conditionals are used: to
> retrieve the v1 or v2 command set.
>
> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> ---
> cmd/tpm-common.c | 24 +++++++++++++++++++++++-
> cmd/tpm-v1.c | 2 +-
> cmd/tpm-v2.c | 4 ++--
> drivers/tpm/Kconfig | 7 ++-----
> drivers/tpm/tpm-uclass.c | 13 ++++++++++---
> drivers/tpm/tpm2_tis_sandbox.c | 11 +++++++++++
> drivers/tpm/tpm2_tis_spi.c | 15 +++++++++++++++
> include/tpm-common.h | 36 ++++++++++++++++++++++++++++++------
> lib/tpm-common.c | 4 ++++
> lib/tpm-utils.h | 9 +++++++++
> 10 files changed, 107 insertions(+), 18 deletions(-)
>
Reviewed-by: Simon Glass <sjg at chromium.org>
Some comments below.
> diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c
> index 6cf9fcc9ac..69cc02b599 100644
> --- a/cmd/tpm-common.c
> +++ b/cmd/tpm-common.c
> @@ -273,12 +273,34 @@ int do_tpm_init(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[])
> {
> cmd_tbl_t *tpm_commands, *cmd;
> + struct tpm_chip_priv *priv;
> + struct udevice *dev;
> unsigned int size;
> + int rc;
Can we use 'ret'? That is more common.
>
> if (argc < 2)
> return CMD_RET_USAGE;
>
> - tpm_commands = get_tpm_commands(&size);
> + rc = get_tpm(&dev);
> + if (rc)
> + return rc;
> +
> + priv = dev_get_uclass_priv(dev);
> +
> + switch (priv->version) {
> +#if defined(CONFIG_TPM_V1)
> + case TPM_V1:
if (IS_ENABLED(CONFIG_TPM_V1))
(I think #ifdef is worse)
> + tpm_commands = get_tpm1_commands(&size);
> + break;
> +#endif
> +#if defined(CONFIG_TPM_V2)
> + case TPM_V2:
> + tpm_commands = get_tpm2_commands(&size);
> + break;
> +#endif
> + default:
> + return CMD_RET_USAGE;
> + }
>
> cmd = find_cmd_tbl(argv[1], tpm_commands, size);
> if (!cmd)
> diff --git a/cmd/tpm-v1.c b/cmd/tpm-v1.c
> index 0874c4d7ba..69870002d4 100644
> --- a/cmd/tpm-v1.c
> +++ b/cmd/tpm-v1.c
> @@ -608,7 +608,7 @@ static cmd_tbl_t tpm1_commands[] = {
> #endif /* CONFIG_TPM_LIST_RESOURCES */
> };
>
> -cmd_tbl_t *get_tpm_commands(unsigned int *size)
> +cmd_tbl_t *get_tpm1_commands(unsigned int *size)
> {
> *size = ARRAY_SIZE(tpm1_commands);
>
> diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
> index 38add4f462..ffbf35a75c 100644
> --- a/cmd/tpm-v2.c
> +++ b/cmd/tpm-v2.c
> @@ -319,14 +319,14 @@ static cmd_tbl_t tpm2_commands[] = {
> do_tpm_pcr_setauthvalue, "", ""),
> };
>
> -cmd_tbl_t *get_tpm_commands(unsigned int *size)
> +cmd_tbl_t *get_tpm2_commands(unsigned int *size)
> {
> *size = ARRAY_SIZE(tpm2_commands);
>
> return tpm2_commands;
> }
>
> -U_BOOT_CMD(tpm, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
> +U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tpm, "Issue a TPMv2.x command",
> "<command> [<arguments>]\n"
> "\n"
> "info\n"
> diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> index 5e3fb3267f..704ab81e2d 100644
> --- a/drivers/tpm/Kconfig
> +++ b/drivers/tpm/Kconfig
> @@ -4,9 +4,6 @@
>
> menu "TPM support"
>
> -comment "Please select only one TPM revision"
> - depends on TPM_V1 && TPM_V2
> -
> config TPM_V1
> bool "TPMv1.x support"
> depends on TPM
> @@ -15,7 +12,7 @@ config TPM_V1
> Major TPM versions are not compatible at all, choose either
> one or the other. This option enables TPMv1.x drivers/commands.
>
> -if TPM_V1 && !TPM_V2
> +if TPM_V1
>
> config TPM_TIS_SANDBOX
> bool "Enable sandbox TPM driver"
> @@ -128,7 +125,7 @@ config TPM_V2
> Major TPM versions are not compatible at all, choose either
> one or the other. This option enables TPMv2.x drivers/commands.
>
> -if TPM_V2 && !TPM_V1
> +if TPM_V2
>
> config TPM2_TIS_SANDBOX
> bool "Enable sandbox TPMv2.x driver"
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index 412697eedc..a80f3df78b 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -7,13 +7,20 @@
> #include <common.h>
> #include <dm.h>
> #include <linux/unaligned/be_byteshift.h>
> -#if defined(CONFIG_TPM_V1)
> #include <tpm-v1.h>
> -#elif defined(CONFIG_TPM_V2)
> #include <tpm-v2.h>
> -#endif
> #include "tpm_internal.h"
>
> +int tpm_set_version(struct udevice *dev)
> +{
> + struct tpm_ops *ops = tpm_get_ops(dev);
> +
> + if (ops->set_version)
> + return ops->set_version(dev);
> +
> + return 0;
> +}
> +
> int tpm_open(struct udevice *dev)
> {
> struct tpm_ops *ops = tpm_get_ops(dev);
> diff --git a/drivers/tpm/tpm2_tis_sandbox.c b/drivers/tpm/tpm2_tis_sandbox.c
> index 3240cc5dba..0b8baad056 100644
> --- a/drivers/tpm/tpm2_tis_sandbox.c
> +++ b/drivers/tpm/tpm2_tis_sandbox.c
> @@ -573,6 +573,16 @@ static int sandbox_tpm2_get_desc(struct udevice *dev, char *buf, int size)
> return snprintf(buf, size, "Sandbox TPM2.x");
> }
>
> +static int sandbox_tpm2_set_version(struct udevice *dev)
> +{
> + struct tpm_chip_priv *priv = dev_get_uclass_priv(dev);
> +
> + /* Use the TPM v2 stack */
> + priv->version = TPM_V2;
Don't you think this could be done in the probe() method?
> +
> + return 0;
> +}
> +
> static int sandbox_tpm2_open(struct udevice *dev)
> {
> struct sandbox_tpm2 *tpm = dev_get_priv(dev);
> @@ -604,6 +614,7 @@ static int sandbox_tpm2_close(struct udevice *dev)
> }
>
> static const struct tpm_ops sandbox_tpm2_ops = {
> + .set_version = sandbox_tpm2_set_version,
> .open = sandbox_tpm2_open,
> .close = sandbox_tpm2_close,
> .get_desc = sandbox_tpm2_get_desc,
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index c5d17a679d..3577f3501c 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -507,9 +507,23 @@ static int tpm_tis_spi_cleanup(struct udevice *dev)
> return 0;
> }
>
> +static int tpm_tis_spi_set_version(struct udevice *dev)
> +{
> + struct tpm_chip_priv *priv = dev_get_uclass_priv(dev);
> +
> + /* Use the TPM v2 stack */
> + priv->version = TPM_V2;
> +
> + return 0;
> +}
> +
> static int tpm_tis_spi_open(struct udevice *dev)
> {
> struct tpm_chip *chip = dev_get_priv(dev);
> + struct tpm_chip_priv *priv = dev_get_uclass_priv(dev);
> +
> + /* Use the TPM v2 stack */
> + priv->version = TPM_V2;
>
> if (chip->is_open)
> return -EBUSY;
> @@ -646,6 +660,7 @@ static int tpm_tis_spi_remove(struct udevice *dev)
> }
>
> static const struct tpm_ops tpm_tis_spi_ops = {
> + .set_version = tpm_tis_spi_set_version,
> .open = tpm_tis_spi_open,
> .close = tpm_tis_spi_close,
> .get_desc = tpm_tis_get_desc,
> diff --git a/include/tpm-common.h b/include/tpm-common.h
> index 68bf8fd627..f39b58cd6c 100644
> --- a/include/tpm-common.h
> +++ b/include/tpm-common.h
> @@ -26,6 +26,16 @@ enum tpm_duration {
> /* Max buffer size supported by our tpm */
> #define TPM_DEV_BUFSIZE 1260
>
> +/**
> + * enum tpm_version - The version of the TPM stack to be used
> + * @TPM_V1: Use TPM v1.x stack
> + * @TPM_V2: Use TPM v2.x stack
> + */
> +enum tpm_version {
> + TPM_V1 = 0,
> + TPM_V2,
> +};
> +
> /**
> * struct tpm_chip_priv - Information about a TPM, stored by the uclass
> *
> @@ -33,20 +43,23 @@ enum tpm_duration {
> * communcation is attempted. If the device has an xfer() method, this is
> * not needed. There is no need to set up @buf.
> *
> + * @version: TPM stack to be used
> * @duration_ms: Length of each duration type in milliseconds
> * @retry_time_ms: Time to wait before retrying receive
> + * @buf: Buffer used during the exchanges with the chip
> * @pcr_count: Number of PCR per bank
> * @pcr_select_min: Minimum size in bytes of the pcrSelect array
> - * @buf: Buffer used during the exchanges with the chip
> */
> struct tpm_chip_priv {
> + enum tpm_version version;
> +
> uint duration_ms[TPM_DURATION_COUNT];
> uint retry_time_ms;
> -#if defined(CONFIG_TPM_V2)
> + u8 buf[TPM_DEV_BUFSIZE + sizeof(u8)]; /* Max buffer size + addr */
> +
> + /* TPM v2 specific data */
> uint pcr_count;
> uint pcr_select_min;
> -#endif
> - u8 buf[TPM_DEV_BUFSIZE + sizeof(u8)]; /* Max buffer size + addr */
> };
>
> /**
> @@ -65,6 +78,16 @@ struct tpm_chip_priv {
> * to bytes which are sent and received.
> */
> struct tpm_ops {
> + /**
> + * set_version() - Set the right TPM stack version to use
> + *
> + * Default is TPM_V1. TPM of newer versions can implement this
> + * optional hook to set another value.
> + *
> + * @dev: TPM device
> + */
> + int (*set_version)(struct udevice *dev);
I think this could just be a helper function provided by the uclass,
rather than a device method.
> +
> /**
> * open() - Request access to locality 0 for the caller
> *
> @@ -208,10 +231,11 @@ int tpm_xfer(struct udevice *dev, const u8 *sendbuf, size_t send_size,
> int tpm_init(void);
>
> /**
> - * Retrieve the array containing all the commands.
> + * Retrieve the array containing all the v1 (resp. v2) commands.
> *
> * @return a cmd_tbl_t array.
> */
> -cmd_tbl_t *get_tpm_commands(unsigned int *size);
> +cmd_tbl_t *get_tpm1_commands(unsigned int *size);
> +cmd_tbl_t *get_tpm2_commands(unsigned int *size);
>
> #endif /* __TPM_COMMON_H */
> diff --git a/lib/tpm-common.c b/lib/tpm-common.c
> index 43b530865a..b228aad0aa 100644
> --- a/lib/tpm-common.c
> +++ b/lib/tpm-common.c
> @@ -193,5 +193,9 @@ int tpm_init(void)
> if (err)
> return err;
>
> + err = tpm_set_version(dev);
> + if (err)
> + return err;
> +
> return tpm_open(dev);
> }
> diff --git a/lib/tpm-utils.h b/lib/tpm-utils.h
> index a9cb7dc7ee..10daffb423 100644
> --- a/lib/tpm-utils.h
> +++ b/lib/tpm-utils.h
> @@ -18,6 +18,15 @@
> #define tpm_u16(x) __MSB(x), __LSB(x)
> #define tpm_u32(x) tpm_u16((x) >> 16), tpm_u16((x) & 0xFFFF)
>
> +/**
> + * tpm_open() - Request the driver to set the TPM version it uses
> + *
> + * This must be done first. By default, TPM v1 stack is used.
> + *
> + * Returns 0 on success, a negative error otherwise.
> + */
> +int tpm_set_version(struct udevice *dev);
> +
> /**
> * tpm_open() - Request access to locality 0 for the caller
> *
> --
> 2.14.1
>
Regards,
Simon
More information about the U-Boot
mailing list