diff mbox series

[v2,08/10] tpm: Add the RNG child device

Message ID 20220228120638.678137-9-sughosh.ganu@linaro.org
State New
Headers show
Series tpm: rng: Move TPM RNG functionality to driver model | expand

Commit Message

Sughosh Ganu Feb. 28, 2022, 12:06 p.m. UTC
The TPM device comes with the random number generator(RNG)
functionality which is built into the TPM device. Add logic to add the
RNG child device in the TPM uclass post probe callback.

The RNG device can then be used to pass a set of random bytes to the
linux kernel, need for address space randomisation through the
EFI_RNG_PROTOCOL interface.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1: None

 drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

Comments

Simon Glass March 1, 2022, 2:58 p.m. UTC | #1
Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device comes with the random number generator(RNG)
> functionality which is built into the TPM device. Add logic to add the
> RNG child device in the TPM uclass post probe callback.
>
> The RNG device can then be used to pass a set of random bytes to the
> linux kernel, need for address space randomisation through the
> EFI_RNG_PROTOCOL interface.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V1: None
>
>  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index 8619da89d8..383cc7bc48 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -16,6 +16,11 @@
>  #include <tpm-v2.h>
>  #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> +
>  bool is_tpm1(struct udevice *dev)
>  {
>         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>         return 0;
>  }
>
> +#if IS_ENABLED(CONFIG_TPM)

This should be in the Makefile so that we only build this file if TPM
is enabled.

> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +       int ret;
> +       const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> +       struct udevice *child;
> +
> +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +       if (ret == -ENOENT) {
> +               log_err("No driver configured for tpm-rng device\n");
> +               return 0;
> +       }
> +
> +       if (ret) {
> +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> +{
> +       int ret;
> +
> +       ret = tpm_open(dev->parent);
> +       if (ret == -EBUSY) {
> +               log_info("TPM device already opened\n");
> +       } else if (ret) {
> +               log_err("Unable to open TPM device\n");
> +               return ret;
> +       }
> +
> +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> +       if (ret)
> +               log_err("Unable to start TPM device\n");
> +
> +       return ret;
> +}
> +#endif /* CONFIG_TPM */
> +
>  UCLASS_DRIVER(tpm) = {
> -       .id             = UCLASS_TPM,
> -       .name           = "tpm",
> -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> +       .id                     = UCLASS_TPM,
> +       .name                   = "tpm",
> +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
>  #if CONFIG_IS_ENABLED(OF_REAL)
> -       .post_bind      = dm_scan_fdt_dev,
> +       .post_bind              = dm_scan_fdt_dev,
> +#endif
> +#if IS_ENABLED(CONFIG_TPM)
> +       .post_probe             = tpm_uclass_post_probe,
> +       .child_pre_probe        = tpm_uclass_child_pre_probe,
>  #endif
>         .per_device_auto        = sizeof(struct tpm_chip_priv),
>  };
> --
> 2.25.1
>

Regards,
Simon
Sughosh Ganu March 2, 2022, 4:52 a.m. UTC | #2
hi Simon,

On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM device comes with the random number generator(RNG)
> > functionality which is built into the TPM device. Add logic to add the
> > RNG child device in the TPM uclass post probe callback.
> >
> > The RNG device can then be used to pass a set of random bytes to the
> > linux kernel, need for address space randomisation through the
> > EFI_RNG_PROTOCOL interface.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V1: None
> >
> >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > index 8619da89d8..383cc7bc48 100644
> > --- a/drivers/tpm/tpm-uclass.c
> > +++ b/drivers/tpm/tpm-uclass.c
> > @@ -16,6 +16,11 @@
> >  #include <tpm-v2.h>
> >  #include "tpm_internal.h"
> >
> > +#include <dm/lists.h>
> > +
> > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > +
> >  bool is_tpm1(struct udevice *dev)
> >  {
> >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >         return 0;
> >  }
> >
> > +#if IS_ENABLED(CONFIG_TPM)
>
> This should be in the Makefile so that we only build this file if TPM
> is enabled.

The Makefile allows for the tpm uclass driver to be built for SPL and
TPL stages as well. The addition of the RNG device is to be done only
in the u-boot proper stage, since we enable RNG support only in u-boot
proper. Thanks.

-sughosh

>
> > +static int tpm_uclass_post_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> > +       struct udevice *child;
> > +
> > +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> > +       if (ret == -ENOENT) {
> > +               log_err("No driver configured for tpm-rng device\n");
> > +               return 0;
> > +       }
> > +
> > +       if (ret) {
> > +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = tpm_open(dev->parent);
> > +       if (ret == -EBUSY) {
> > +               log_info("TPM device already opened\n");
> > +       } else if (ret) {
> > +               log_err("Unable to open TPM device\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> > +       if (ret)
> > +               log_err("Unable to start TPM device\n");
> > +
> > +       return ret;
> > +}
> > +#endif /* CONFIG_TPM */
> > +
> >  UCLASS_DRIVER(tpm) = {
> > -       .id             = UCLASS_TPM,
> > -       .name           = "tpm",
> > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > +       .id                     = UCLASS_TPM,
> > +       .name                   = "tpm",
> > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >  #if CONFIG_IS_ENABLED(OF_REAL)
> > -       .post_bind      = dm_scan_fdt_dev,
> > +       .post_bind              = dm_scan_fdt_dev,
> > +#endif
> > +#if IS_ENABLED(CONFIG_TPM)
> > +       .post_probe             = tpm_uclass_post_probe,
> > +       .child_pre_probe        = tpm_uclass_child_pre_probe,
> >  #endif
> >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> >  };
> > --
> > 2.25.1
> >
>
> Regards,
> Simon
Simon Glass March 3, 2022, 3:47 a.m. UTC | #3
Hi Sughosh,

On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The TPM device comes with the random number generator(RNG)
> > > functionality which is built into the TPM device. Add logic to add the
> > > RNG child device in the TPM uclass post probe callback.
> > >
> > > The RNG device can then be used to pass a set of random bytes to the
> > > linux kernel, need for address space randomisation through the
> > > EFI_RNG_PROTOCOL interface.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V1: None
> > >
> > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > index 8619da89d8..383cc7bc48 100644
> > > --- a/drivers/tpm/tpm-uclass.c
> > > +++ b/drivers/tpm/tpm-uclass.c
> > > @@ -16,6 +16,11 @@
> > >  #include <tpm-v2.h>
> > >  #include "tpm_internal.h"
> > >
> > > +#include <dm/lists.h>
> > > +
> > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > +
> > >  bool is_tpm1(struct udevice *dev)
> > >  {
> > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > >         return 0;
> > >  }
> > >
> > > +#if IS_ENABLED(CONFIG_TPM)
> >
> > This should be in the Makefile so that we only build this file if TPM
> > is enabled.
>
> The Makefile allows for the tpm uclass driver to be built for SPL and
> TPL stages as well. The addition of the RNG device is to be done only
> in the u-boot proper stage, since we enable RNG support only in u-boot
> proper. Thanks.

Well in that case, create a new SPL_TPM_RAND or similar to control
enabling it in SPL. It should be explicit.

Regards,
Simon
Sughosh Ganu March 3, 2022, 12:11 p.m. UTC | #4
hi Simon,

On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The TPM device comes with the random number generator(RNG)
> > > > functionality which is built into the TPM device. Add logic to add the
> > > > RNG child device in the TPM uclass post probe callback.
> > > >
> > > > The RNG device can then be used to pass a set of random bytes to the
> > > > linux kernel, need for address space randomisation through the
> > > > EFI_RNG_PROTOCOL interface.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V1: None
> > > >
> > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > index 8619da89d8..383cc7bc48 100644
> > > > --- a/drivers/tpm/tpm-uclass.c
> > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > @@ -16,6 +16,11 @@
> > > >  #include <tpm-v2.h>
> > > >  #include "tpm_internal.h"
> > > >
> > > > +#include <dm/lists.h>
> > > > +
> > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > +
> > > >  bool is_tpm1(struct udevice *dev)
> > > >  {
> > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > >         return 0;
> > > >  }
> > > >
> > > > +#if IS_ENABLED(CONFIG_TPM)
> > >
> > > This should be in the Makefile so that we only build this file if TPM
> > > is enabled.
> >
> > The Makefile allows for the tpm uclass driver to be built for SPL and
> > TPL stages as well. The addition of the RNG device is to be done only
> > in the u-boot proper stage, since we enable RNG support only in u-boot
> > proper. Thanks.
>
> Well in that case, create a new SPL_TPM_RAND or similar to control
> enabling it in SPL. It should be explicit.

I think it is easier to just protect the child addition functions
under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
don't have any requirement for generating random numbers in the SPL
and TPL stages. I feel that creating new symbols just for the sake of
not putting a check for CONFIG_TPM is a bit of an overkill, especially
since we do not have any requirement for RNG devices in the SPL/TPL
stages.

-sughosh

>
> Regards,
> Simon
Simon Glass March 4, 2022, 2:37 a.m. UTC | #5
Hi Sughosh,

On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > The TPM device comes with the random number generator(RNG)
> > > > > functionality which is built into the TPM device. Add logic to add the
> > > > > RNG child device in the TPM uclass post probe callback.
> > > > >
> > > > > The RNG device can then be used to pass a set of random bytes to the
> > > > > linux kernel, need for address space randomisation through the
> > > > > EFI_RNG_PROTOCOL interface.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes since V1: None
> > > > >
> > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > index 8619da89d8..383cc7bc48 100644
> > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > @@ -16,6 +16,11 @@
> > > > >  #include <tpm-v2.h>
> > > > >  #include "tpm_internal.h"
> > > > >
> > > > > +#include <dm/lists.h>
> > > > > +
> > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > +
> > > > >  bool is_tpm1(struct udevice *dev)
> > > > >  {
> > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > >
> > > > This should be in the Makefile so that we only build this file if TPM
> > > > is enabled.
> > >
> > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > TPL stages as well. The addition of the RNG device is to be done only
> > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > proper. Thanks.
> >
> > Well in that case, create a new SPL_TPM_RAND or similar to control
> > enabling it in SPL. It should be explicit.
>
> I think it is easier to just protect the child addition functions
> under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> don't have any requirement for generating random numbers in the SPL
> and TPL stages. I feel that creating new symbols just for the sake of
> not putting a check for CONFIG_TPM is a bit of an overkill, especially
> since we do not have any requirement for RNG devices in the SPL/TPL
> stages.

What does checking for CONFIG_TPM have to do with SPL and TPL? If that
option is enabled, the feature will be active in SPL and TPL too.

Also I see another problem, on further examination. You cannot start
up the TPM in the pre_probe() function. That needs to be done under
board control. E.g. for coral it happens in the TPL (or soon VPL)
phase so cannot be done again in U-Boot proper.

So perhaps we need to remember the state of the TPM (using SPL handoff
perhaps). Also you probably need to move the startup stuff to the RNG
itself.

Perhaps we could add a new function to handle this, which can be
called from your rand driver.

int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)

Regards,
Simon
Sughosh Ganu March 4, 2022, 1:43 p.m. UTC | #6
hi Simon,

On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > The TPM device comes with the random number generator(RNG)
> > > > > > functionality which is built into the TPM device. Add logic to add the
> > > > > > RNG child device in the TPM uclass post probe callback.
> > > > > >
> > > > > > The RNG device can then be used to pass a set of random bytes to the
> > > > > > linux kernel, need for address space randomisation through the
> > > > > > EFI_RNG_PROTOCOL interface.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > Changes since V1: None
> > > > > >
> > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > @@ -16,6 +16,11 @@
> > > > > >  #include <tpm-v2.h>
> > > > > >  #include "tpm_internal.h"
> > > > > >
> > > > > > +#include <dm/lists.h>
> > > > > > +
> > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > +
> > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > >  {
> > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > >
> > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > is enabled.
> > > >
> > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > proper. Thanks.
> > >
> > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > enabling it in SPL. It should be explicit.
> >
> > I think it is easier to just protect the child addition functions
> > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > don't have any requirement for generating random numbers in the SPL
> > and TPL stages. I feel that creating new symbols just for the sake of
> > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > since we do not have any requirement for RNG devices in the SPL/TPL
> > stages.
>
> What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> option is enabled, the feature will be active in SPL and TPL too.

Maybe I am not explaining it properly. We need the addition of the RNG
child device only in the u-boot proper stage, not in the SPL and TPL
stages. The TPM uclass driver can indeed be built for the SPL and TPL
stages, while the RNG uclass is needed only for u-boot proper. So, the
addition of the RNG child device done in the TPM uclass driver should
only happen in u-boot proper, and not in SPL/TPL stages. Which is the
reason for the CONFIG_TPM check.

>
> Also I see another problem, on further examination. You cannot start
> up the TPM in the pre_probe() function. That needs to be done under
> board control. E.g. for coral it happens in the TPL (or soon VPL)
> phase so cannot be done again in U-Boot proper.

I tested running the RNG command after the TPM device has already been
probed and tpm_startup has been called. Even if I call the tpm_startup
again, I do not see any issues. Does the TPM spec prohibit calling the
initialisation function multiple times. I believe that the TPM device
should be able to handle this scenario right?

>
> So perhaps we need to remember the state of the TPM (using SPL handoff
> perhaps). Also you probably need to move the startup stuff to the RNG
> itself.

I can move the call to tpm_startup to the RNG driver's probe function.
But I thought it is better that the parent device(TPM) is initialised
before calling the probe of the child device.

>
> Perhaps we could add a new function to handle this, which can be
> called from your rand driver.
>
> int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)

Okay, I can add this, but the question is, does calling the
tpm_startup function cause issues? If not, maybe this is not needed?

-sughosh

>
> Regards,
> Simon
Ilias Apalodimas March 4, 2022, 2:15 p.m. UTC | #7
Hi,

[...]

> > >
> > > I think it is easier to just protect the child addition functions
> > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > don't have any requirement for generating random numbers in the SPL
> > > and TPL stages. I feel that creating new symbols just for the sake of
> > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > stages.
> >
> > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > option is enabled, the feature will be active in SPL and TPL too.
>
> Maybe I am not explaining it properly. We need the addition of the RNG
> child device only in the u-boot proper stage, not in the SPL and TPL
> stages. The TPM uclass driver can indeed be built for the SPL and TPL
> stages, while the RNG uclass is needed only for u-boot proper. So, the
> addition of the RNG child device done in the TPM uclass driver should
> only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> reason for the CONFIG_TPM check.
>
> >
> > Also I see another problem, on further examination. You cannot start
> > up the TPM in the pre_probe() function. That needs to be done under
> > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > phase so cannot be done again in U-Boot proper.
>
> I tested running the RNG command after the TPM device has already been
> probed and tpm_startup has been called. Even if I call the tpm_startup
> again, I do not see any issues. Does the TPM spec prohibit calling the
> initialisation function multiple times. I believe that the TPM device
> should be able to handle this scenario right?
[...]

We've already discussed this in [1].  Issuing multiple startup
commands will just make the TPM respond with TPM2_RC_INITIALIZE.  At
least that's my reading of [2]

[1] https://lore.kernel.org/all/CAC_iWjLQmsGLLYfnDFq17-SD3RC7-Da-M41Qc5DjWp3XX9ZHTA@mail.gmail.com/
[2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38.pdf
section 9.3.1

Regards
/Ilias
Simon Glass March 6, 2022, 3:07 a.m. UTC | #8
Hi Sughosh,

On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > The TPM device comes with the random number generator(RNG)
> > > > > > > functionality which is built into the TPM device. Add logic to add the
> > > > > > > RNG child device in the TPM uclass post probe callback.
> > > > > > >
> > > > > > > The RNG device can then be used to pass a set of random bytes to the
> > > > > > > linux kernel, need for address space randomisation through the
> > > > > > > EFI_RNG_PROTOCOL interface.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since V1: None
> > > > > > >
> > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > @@ -16,6 +16,11 @@
> > > > > > >  #include <tpm-v2.h>
> > > > > > >  #include "tpm_internal.h"
> > > > > > >
> > > > > > > +#include <dm/lists.h>
> > > > > > > +
> > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > +
> > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > >  {
> > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > >
> > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > is enabled.
> > > > >
> > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > proper. Thanks.
> > > >
> > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > enabling it in SPL. It should be explicit.
> > >
> > > I think it is easier to just protect the child addition functions
> > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > don't have any requirement for generating random numbers in the SPL
> > > and TPL stages. I feel that creating new symbols just for the sake of
> > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > stages.
> >
> > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > option is enabled, the feature will be active in SPL and TPL too.
>
> Maybe I am not explaining it properly. We need the addition of the RNG
> child device only in the u-boot proper stage, not in the SPL and TPL
> stages. The TPM uclass driver can indeed be built for the SPL and TPL
> stages, while the RNG uclass is needed only for u-boot proper. So, the
> addition of the RNG child device done in the TPM uclass driver should
> only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> reason for the CONFIG_TPM check.

Let me try one more time. If this doesn't work, please chat with a colleague.

The way we do that is with Kconfig options. We don't use ad-hoc
methods to enable things in U-Boot proper but not SPL. Boards that
have CONFIG_TPM set have it set everywhere, including in SPL. So
checking for CONFIG_TPM is going to return true in both U-Boot proper
and SPL. If you want different behaviour in SPL, you need
CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
enable your driver in SPL, you need a CONFIG_SPL_...

Also, you should use devicetree to provide the random number
generator, just a device_bind(). Add the node as a subnode of the TPM.
Otherwise it cannot work with OF_PLATDATA.

>
> >
> > Also I see another problem, on further examination. You cannot start
> > up the TPM in the pre_probe() function. That needs to be done under
> > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > phase so cannot be done again in U-Boot proper.
>
> I tested running the RNG command after the TPM device has already been
> probed and tpm_startup has been called. Even if I call the tpm_startup
> again, I do not see any issues. Does the TPM spec prohibit calling the
> initialisation function multiple times. I believe that the TPM device
> should be able to handle this scenario right?

I hangs on coral, for example.

>
> >
> > So perhaps we need to remember the state of the TPM (using SPL handoff
> > perhaps). Also you probably need to move the startup stuff to the RNG
> > itself.
>
> I can move the call to tpm_startup to the RNG driver's probe function.
> But I thought it is better that the parent device(TPM) is initialised
> before calling the probe of the child device.

We use lazy init in U-Boot, so it should be possible to probe the TPM
without automatically starting it. Otherwise you are going to break
some of the tpm commands.

>
> >
> > Perhaps we could add a new function to handle this, which can be
> > called from your rand driver.
> >
> > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
>
> Okay, I can add this, but the question is, does calling the
> tpm_startup function cause issues? If not, maybe this is not needed?

It does on coral. I don't know whether other TPMs are tolerant of it,
but in any case it is not a good idea.

Regards,
Simon
Sughosh Ganu March 6, 2022, 7:03 a.m. UTC | #9
hi Simon,

On Sun, 6 Mar 2022 at 08:37, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > The TPM device comes with the random number generator(RNG)
> > > > > > > > functionality which is built into the TPM device. Add logic to add the
> > > > > > > > RNG child device in the TPM uclass post probe callback.
> > > > > > > >
> > > > > > > > The RNG device can then be used to pass a set of random bytes to the
> > > > > > > > linux kernel, need for address space randomisation through the
> > > > > > > > EFI_RNG_PROTOCOL interface.
> > > > > > > >
> > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes since V1: None
> > > > > > > >
> > > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > > @@ -16,6 +16,11 @@
> > > > > > > >  #include <tpm-v2.h>
> > > > > > > >  #include "tpm_internal.h"
> > > > > > > >
> > > > > > > > +#include <dm/lists.h>
> > > > > > > > +
> > > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > > +
> > > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > > >  {
> > > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > > >
> > > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > > is enabled.
> > > > > >
> > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > > proper. Thanks.
> > > > >
> > > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > > enabling it in SPL. It should be explicit.
> > > >
> > > > I think it is easier to just protect the child addition functions
> > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > > don't have any requirement for generating random numbers in the SPL
> > > > and TPL stages. I feel that creating new symbols just for the sake of
> > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > > stages.
> > >
> > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > > option is enabled, the feature will be active in SPL and TPL too.
> >
> > Maybe I am not explaining it properly. We need the addition of the RNG
> > child device only in the u-boot proper stage, not in the SPL and TPL
> > stages. The TPM uclass driver can indeed be built for the SPL and TPL
> > stages, while the RNG uclass is needed only for u-boot proper. So, the
> > addition of the RNG child device done in the TPM uclass driver should
> > only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> > reason for the CONFIG_TPM check.
>
> Let me try one more time. If this doesn't work, please chat with a colleague.
>
> The way we do that is with Kconfig options. We don't use ad-hoc
> methods to enable things in U-Boot proper but not SPL. Boards that
> have CONFIG_TPM set have it set everywhere, including in SPL. So
> checking for CONFIG_TPM is going to return true in both U-Boot proper
> and SPL. If you want different behaviour in SPL, you need
> CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
> enable your driver in SPL, you need a CONFIG_SPL_...

Okay. I now get your point. So this is because these config symbols
are getting included in all the stages, so I cannot use the symbol
name to identify the spl/tpl/u-boot stages. I believe earlier, the
identification used to be done based on whether a symbol is CONFIG_xxx
as against CONFIG_SPL_xxx. Now it is being done with
CONFIG_{SPL,TPL}_BUILD symbols for differentiating between stages. I
will use these instead to have the code enabled only for the u-boot
proper stage. Thing is, I don't want to define these functions for the
spl and tpl stages since they are adding to the size with no benefits.
I do understand that currently no platform is enabling tpm drivers in
the spl/tpl stage, but the driver can be enabled in those stages.

>
> Also, you should use devicetree to provide the random number
> generator, just a device_bind(). Add the node as a subnode of the TPM.
> Otherwise it cannot work with OF_PLATDATA.

As per the documentation in of-plat.rst, the OF_PLATDATA method is to
be used only for the SPL/TPL stages where there might be a size
constraint. And the tpm rng drivers are to be enabled only in the
u-boot proper stage. So I think we would not need to put a subnode in
the devicetree. Basically, the presence of the rng functionality in
the TPM device is not optional as per my understanding, so the rng
child node can just be added in the tpm uclass driver.

>
> >
> > >
> > > Also I see another problem, on further examination. You cannot start
> > > up the TPM in the pre_probe() function. That needs to be done under
> > > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > > phase so cannot be done again in U-Boot proper.
> >
> > I tested running the RNG command after the TPM device has already been
> > probed and tpm_startup has been called. Even if I call the tpm_startup
> > again, I do not see any issues. Does the TPM spec prohibit calling the
> > initialisation function multiple times. I believe that the TPM device
> > should be able to handle this scenario right?
>
> I hangs on coral, for example.
>
> >
> > >
> > > So perhaps we need to remember the state of the TPM (using SPL handoff
> > > perhaps). Also you probably need to move the startup stuff to the RNG
> > > itself.
> >
> > I can move the call to tpm_startup to the RNG driver's probe function.
> > But I thought it is better that the parent device(TPM) is initialised
> > before calling the probe of the child device.
>
> We use lazy init in U-Boot, so it should be possible to probe the TPM
> without automatically starting it. Otherwise you are going to break
> some of the tpm commands.

Yes I understand. With the patch, the tpm device is being started only
when a child device is being probed. But I can remove the startup of
the tpm device from the child_pre_probe function. It will have to be
done explicitly before the rng device is to be used.

Btw, can you please comment on patch 3 of this series[1].

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2022-March/476830.html

>
> >
> > >
> > > Perhaps we could add a new function to handle this, which can be
> > > called from your rand driver.
> > >
> > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
> >
> > Okay, I can add this, but the question is, does calling the
> > tpm_startup function cause issues? If not, maybe this is not needed?
>
> It does on coral. I don't know whether other TPMs are tolerant of it,
> but in any case it is not a good idea.
>
> Regards,
> Simon
Simon Glass March 12, 2022, 2:43 a.m. UTC | #10
Hi Sughosh,

On Sun, 6 Mar 2022 at 00:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Mar 2022 at 08:37, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > The TPM device comes with the random number generator(RNG)
> > > > > > > > > functionality which is built into the TPM device. Add logic to add the
> > > > > > > > > RNG child device in the TPM uclass post probe callback.
> > > > > > > > >
> > > > > > > > > The RNG device can then be used to pass a set of random bytes to the
> > > > > > > > > linux kernel, need for address space randomisation through the
> > > > > > > > > EFI_RNG_PROTOCOL interface.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes since V1: None
> > > > > > > > >
> > > > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > > > @@ -16,6 +16,11 @@
> > > > > > > > >  #include <tpm-v2.h>
> > > > > > > > >  #include "tpm_internal.h"
> > > > > > > > >
> > > > > > > > > +#include <dm/lists.h>
> > > > > > > > > +
> > > > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > > > +
> > > > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > > > >  {
> > > > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > > > >
> > > > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > > > is enabled.
> > > > > > >
> > > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > > > proper. Thanks.
> > > > > >
> > > > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > > > enabling it in SPL. It should be explicit.
> > > > >
> > > > > I think it is easier to just protect the child addition functions
> > > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > > > don't have any requirement for generating random numbers in the SPL
> > > > > and TPL stages. I feel that creating new symbols just for the sake of
> > > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > > > stages.
> > > >
> > > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > > > option is enabled, the feature will be active in SPL and TPL too.
> > >
> > > Maybe I am not explaining it properly. We need the addition of the RNG
> > > child device only in the u-boot proper stage, not in the SPL and TPL
> > > stages. The TPM uclass driver can indeed be built for the SPL and TPL
> > > stages, while the RNG uclass is needed only for u-boot proper. So, the
> > > addition of the RNG child device done in the TPM uclass driver should
> > > only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> > > reason for the CONFIG_TPM check.
> >
> > Let me try one more time. If this doesn't work, please chat with a colleague.
> >
> > The way we do that is with Kconfig options. We don't use ad-hoc
> > methods to enable things in U-Boot proper but not SPL. Boards that
> > have CONFIG_TPM set have it set everywhere, including in SPL. So
> > checking for CONFIG_TPM is going to return true in both U-Boot proper
> > and SPL. If you want different behaviour in SPL, you need
> > CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
> > enable your driver in SPL, you need a CONFIG_SPL_...
>
> Okay. I now get your point. So this is because these config symbols
> are getting included in all the stages, so I cannot use the symbol
> name to identify the spl/tpl/u-boot stages. I believe earlier, the
> identification used to be done based on whether a symbol is CONFIG_xxx
> as against CONFIG_SPL_xxx. Now it is being done with
> CONFIG_{SPL,TPL}_BUILD symbols for differentiating between stages. I
> will use these instead to have the code enabled only for the u-boot
> proper stage. Thing is, I don't want to define these functions for the
> spl and tpl stages since they are adding to the size with no benefits.
> I do understand that currently no platform is enabling tpm drivers in
> the spl/tpl stage, but the driver can be enabled in those stages.

coral uses TPM in the VPL stage. In general, the TPM needs to be used
earlier than U-Boot proper, since verified boot has to be complete by
then.

>
> >
> > Also, you should use devicetree to provide the random number
> > generator, just a device_bind(). Add the node as a subnode of the TPM.
> > Otherwise it cannot work with OF_PLATDATA.
>
> As per the documentation in of-plat.rst, the OF_PLATDATA method is to
> be used only for the SPL/TPL stages where there might be a size
> constraint. And the tpm rng drivers are to be enabled only in the
> u-boot proper stage. So I think we would not need to put a subnode in
> the devicetree. Basically, the presence of the rng functionality in
> the TPM device is not optional as per my understanding, so the rng
> child node can just be added in the tpm uclass driver.

We don't need the subnode but it is normal practice to have one. If
you want to use a manual bind, that is OK, at least for U-Boot proper.

>
> >
> > >
> > > >
> > > > Also I see another problem, on further examination. You cannot start
> > > > up the TPM in the pre_probe() function. That needs to be done under
> > > > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > > > phase so cannot be done again in U-Boot proper.
> > >
> > > I tested running the RNG command after the TPM device has already been
> > > probed and tpm_startup has been called. Even if I call the tpm_startup
> > > again, I do not see any issues. Does the TPM spec prohibit calling the
> > > initialisation function multiple times. I believe that the TPM device
> > > should be able to handle this scenario right?
> >
> > I hangs on coral, for example.
> >
> > >
> > > >
> > > > So perhaps we need to remember the state of the TPM (using SPL handoff
> > > > perhaps). Also you probably need to move the startup stuff to the RNG
> > > > itself.
> > >
> > > I can move the call to tpm_startup to the RNG driver's probe function.
> > > But I thought it is better that the parent device(TPM) is initialised
> > > before calling the probe of the child device.
> >
> > We use lazy init in U-Boot, so it should be possible to probe the TPM
> > without automatically starting it. Otherwise you are going to break
> > some of the tpm commands.
>
> Yes I understand. With the patch, the tpm device is being started only
> when a child device is being probed. But I can remove the startup of
> the tpm device from the child_pre_probe function. It will have to be
> done explicitly before the rng device is to be used.

Yes, that's how it should work, for now.

>
> Btw, can you please comment on patch 3 of this series[1].

I suspect it is in my queue but I have not had time yet.

Regards,
Simon



>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2022-March/476830.html
>
> >
> > >
> > > >
> > > > Perhaps we could add a new function to handle this, which can be
> > > > called from your rand driver.
> > > >
> > > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
> > >
> > > Okay, I can add this, but the question is, does calling the
> > > tpm_startup function cause issues? If not, maybe this is not needed?
> >
> > It does on coral. I don't know whether other TPMs are tolerant of it,
> > but in any case it is not a good idea.
> >
> > Regards,
> > Simon
diff mbox series

Patch

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index 8619da89d8..383cc7bc48 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -16,6 +16,11 @@ 
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG1_DRV_NAME	"tpm1-rng"
+#define TPM_RNG2_DRV_NAME	"tpm2-rng"
+
 bool is_tpm1(struct udevice *dev)
 {
 	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
@@ -147,12 +152,57 @@  int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_TPM)
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
+	struct udevice *child;
+
+	ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+	if (ret == -ENOENT) {
+		log_err("No driver configured for tpm-rng device\n");
+		return 0;
+	}
+
+	if (ret) {
+		log_err("Unable to bind rng driver with the tpm-rng device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tpm_uclass_child_pre_probe(struct udevice *dev)
+{
+	int ret;
+
+	ret = tpm_open(dev->parent);
+	if (ret == -EBUSY) {
+		log_info("TPM device already opened\n");
+	} else if (ret) {
+		log_err("Unable to open TPM device\n");
+		return ret;
+	}
+
+	ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
+	if (ret)
+		log_err("Unable to start TPM device\n");
+
+	return ret;
+}
+#endif /* CONFIG_TPM */
+
 UCLASS_DRIVER(tpm) = {
-	.id		= UCLASS_TPM,
-	.name		= "tpm",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.id			= UCLASS_TPM,
+	.name			= "tpm",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 #if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
+	.post_bind		= dm_scan_fdt_dev,
+#endif
+#if IS_ENABLED(CONFIG_TPM)
+	.post_probe		= tpm_uclass_post_probe,
+	.child_pre_probe	= tpm_uclass_child_pre_probe,
 #endif
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };