[U-Boot] [PATCH 3/6] tpm: allow TPM v1 and v2 to be compiled at the same time

[Miquel Raynal]

Hi Simon,

Simon Glass <sjg at chromium.org> wrote on Wed, 18 Jul 2018 19:31:41 -0600:

> 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.

Of course.

> 
> >
> >         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)

Actually, in the present state, get_tpmx_commands symbol is only
available if the file if TPM_Vx is selected in Kconfig.

I created two dummy functions in tpm-common.h which return NULL for
each symbol if their respective TPM version is not compiled. This way it
removes any conditionals in the code.

> 
> > +               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;
> > +       }

[...]

> >
> > +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?

When I first wrote this I faced some issues. As I did not remembered
why it would fail I tried again and indeed, moving this
selection in the probe works.

> 
> > +
> > +       return 0;
> > +}
> > +

[...]

> > @@ -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.

Actually there is no need for this hook or any helper anymore, I
removed them all as the version selection is done in the driver
->probe().

I'll let other people that would need to change the version at runtime
adding support for that if they want, it should not be too tricky
anyway.

Thanks,
Miquèl