Hi Miquel, On 14 July 2018 at 06:16, Miquel Raynal <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
