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

[Sughosh Ganu]

hi Simon,

On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu at 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 at 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