[1/2,v3] tpm: add a function that performs selftest + startup

As described in [0] if a command requires use of an untested algorithm
or functional module, the TPM performs the test and then completes the
command actions.

Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
the TPM in that case) and even if we would, it would complicate our TPM
code for no apparent reason,  add a wrapper function that performs both
the selftest and the startup sequence of the TPM.

It's worth noting that this is implemented on TPMv2.0.  The code for
1.2 would look similar,  but I don't have a device available to test.

[0]
https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
§12.3 Self-test modes

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since v2:
- add tpm_init() to auto start

Changes since v1:
- Remove a superfluous if statement
- Move function comments to the header file
 include/tpm-v2.h  | 19 +++++++++++++++++++
 include/tpm_api.h |  8 ++++++++
 lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
 lib/tpm_api.c     |  8 ++++++++
 4 files changed, 59 insertions(+)

--
2.38.1

Hi Ilias,

On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> As described in [0] if a command requires use of an untested algorithm
> or functional module, the TPM performs the test and then completes the
> command actions.
>
> Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> the TPM in that case) and even if we would, it would complicate our TPM
> code for no apparent reason,  add a wrapper function that performs both
> the selftest and the startup sequence of the TPM.
>
> It's worth noting that this is implemented on TPMv2.0.  The code for
> 1.2 would look similar,  but I don't have a device available to test.
>
> [0]
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> §12.3 Self-test modes
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v2:
> - add tpm_init() to auto start
>
> Changes since v1:
> - Remove a superfluous if statement
> - Move function comments to the header file
>  include/tpm-v2.h  | 19 +++++++++++++++++++
>  include/tpm_api.h |  8 ++++++++
>  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
>  lib/tpm_api.c     |  8 ++++++++
>  4 files changed, 59 insertions(+)

I think this is a good idea, but it should be implemented at the API
level. Please see below.

>
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 737e57551d73..1c644f0048f6 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -688,4 +688,23 @@ u32 tpm2_report_state(struct udevice *dev, uint vendor_cmd, uint vendor_subcmd,
>  u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd,
>                           uint vendor_subcmd);
>
> +/**
> + * tpm2_auto_start() - start up the TPM and perform selftests.
> + *                     If a testable function has not been tested and is
> + *                     requested the TPM2  will return TPM_RC_NEEDS_TEST.
> + *
> + *
> + *

drop extra lines

> + * @param dev          TPM device
> + * Return: TPM2_RC_TESTING, if TPM2 self-test has been received and the tests are

80 cols

> + *         not complete.
> + *         TPM2_RC_SUCCESS, if testing of all functions is complete without
> + *         functional failures.
> + *         TPM2_RC_FAILURE, if any test failed.
> + *         TPM2_RC_INITIALIZE, if the TPM has not gone through the Startup
> + *         sequence
> +
> + */
> +u32 tpm2_auto_start(struct udevice *dev);
> +
>  #endif /* __TPM_V2_H */
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 8979d9d6df7e..022a8bbaeca6 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -331,4 +331,12 @@ static inline bool tpm_is_v2(struct udevice *dev)
>         return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
>  }
>
> +/**
> + * tpm_auto_start() - start up the TPM and perform selftests
> + *
> + * @param dev          TPM device
> + * Return: return code of the operation (0 = success)
> + */
> +u32 tpm_auto_start(struct udevice *dev);
> +
>  #endif /* __TPM_API_H */
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 697b982e079f..2141d58632ff 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -44,6 +44,30 @@ u32 tpm2_self_test(struct udevice *dev, enum tpm2_yes_no full_test)
>         return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
>  }
>
> +u32 tpm2_auto_start(struct udevice *dev)
> +{
> +       u32 rc;
> +
> +       /*
> +        * the tpm_init() will return -EBUSY if the init has already happened
> +        * The selftest and startup code can run multiple times with no side effects

80 cols

> +        */
> +       rc = tpm_init(dev);
> +       if (rc && rc != -EBUSY)

Does that work, with rc being unsigned?

> +               return rc;
> +       rc = tpm2_self_test(dev, TPMI_YES);

Can we call tpm_self_test_full() ? If not, please update the API as needed.

> +
> +       if (rc == TPM2_RC_INITIALIZE) {
> +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);

Should call tpm_startup()

> +               if (rc)
> +                       return rc;
> +
> +               rc = tpm2_self_test(dev, TPMI_YES);

Again, tpm_self_test_full(). We are trying to provide a TPM API that
covers v1 and v2, to the extent possible.

> +       }
> +
> +       return rc;
> +}
> +

Also please add a test for this to test/dm/tpm.c

>  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
>                const ssize_t pw_sz)
>  {
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 7e8df8795ef3..5b2c11a277cc 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>         }
>  }
>
> +u32 tpm_auto_start(struct udevice *dev)
> +{
> +       if (tpm_is_v2(dev))
> +               return tpm2_auto_start(dev);

Hopefully all the code from above can move here.

> +
> +       return -ENOSYS;
> +}
> +
>  u32 tpm_resume(struct udevice *dev)
>  {
>         if (tpm_is_v1(dev))
> --
> 2.38.1
>

Regards,
Simon

Hi Simon, 
On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > As described in [0] if a command requires use of an untested algorithm
> > or functional module, the TPM performs the test and then completes the
> > command actions.
> >
> > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> > the TPM in that case) and even if we would, it would complicate our TPM
> > code for no apparent reason,  add a wrapper function that performs both
> > the selftest and the startup sequence of the TPM.
> >
> > It's worth noting that this is implemented on TPMv2.0.  The code for
> > 1.2 would look similar,  but I don't have a device available to test.
> >
> > [0]
> > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> > §12.3 Self-test modes
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since v2:
> > - add tpm_init() to auto start
> >
> > Changes since v1:
> > - Remove a superfluous if statement
> > - Move function comments to the header file
> >  include/tpm-v2.h  | 19 +++++++++++++++++++
> >  include/tpm_api.h |  8 ++++++++
> >  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
> >  lib/tpm_api.c     |  8 ++++++++
> >  4 files changed, 59 insertions(+)
> 
> I think this is a good idea, but it should be implemented at the API
> level. Please see below.

It is implemented at the API level.  I could try testing this in QEMU using
swtpm, but the driver I added for mmio is TPMv2 only.  So I don't really feel
comfortable adding support for a device I can't test.  If someone has a
tpm1.2 and is willing to test it, I can modify the patch accordingly.

> 
> >
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index 737e57551d73..1c644f0048f6 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -688,4 +688,23 @@ u32 tpm2_report_state(struct udevice *dev, uint vendor_cmd, uint vendor_subcmd,
> >  u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd,
> >                           uint vendor_subcmd);
> >
> > +/**
> > + * tpm2_auto_start() - start up the TPM and perform selftests.
> > + *                     If a testable function has not been tested and is
> > + *                     requested the TPM2  will return TPM_RC_NEEDS_TEST.
> > + *
> > + *
> > + *
> 
> drop extra lines

Sure

> 
> > + * @param dev          TPM device
> > + * Return: TPM2_RC_TESTING, if TPM2 self-test has been received and the tests are
> 
> 80 cols

Sure

> 
> > + *         not complete.
> > + *         TPM2_RC_SUCCESS, if testing of all functions is complete without
> > + *         functional failures.

[...]

> > +        */
> > +       rc = tpm_init(dev);
> > +       if (rc && rc != -EBUSY)
> 
> Does that work, with rc being unsigned?


Yes integer promotion is fine here.  As long as we don't try to do anything
silly e.g start doing math on the result or compare it against bigger types
this is fine.  
There was a patch from Sughosh in the past that you rejected and it was
converting enough of the TPM API return calls to int.  I think the reason
the API works with u32, is that the spec return codes from the device
itself are described as u32.  But that shouldn't affect the internal U-Boot
API.  I can send a patch afterwards moving the U-Boot TPM API functions and
return int.

> 
> > +               return rc;
> > +       rc = tpm2_self_test(dev, TPMI_YES);
> 
> Can we call tpm_self_test_full() ? If not, please update the API as needed.
> 
> > +
> > +       if (rc == TPM2_RC_INITIALIZE) {
> > +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);
> 
> Should call tpm_startup()


Same logic applies to both of these.  Yes that makes sense to use the top
level API but only if we add support for 1.2 as well.  Otherwise the only
thing this is going to create is circular dependencies between the v2 code
and the API library.

> 
> > +               if (rc)
> > +                       return rc;
> > +
> > +               rc = tpm2_self_test(dev, TPMI_YES);
> 
> Again, tpm_self_test_full(). We are trying to provide a TPM API that
> covers v1 and v2, to the extent possible.

See above.

> 
> > +       }
> > +
> > +       return rc;
> > +}
> > +
> 
> Also please add a test for this to test/dm/tpm.c

sure

> 
> >  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> >                const ssize_t pw_sz)
> >  {
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index 7e8df8795ef3..5b2c11a277cc 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> >         }
> >  }
> >
> > +u32 tpm_auto_start(struct udevice *dev)
> > +{
> > +       if (tpm_is_v2(dev))
> > +               return tpm2_auto_start(dev);
> 
> Hopefully all the code from above can move here.

Repeating myself here, but I don't want to add untested code.  So I really
prefer the patch as is, until someone verifies the 1.2 init sequence for
me.

Regards
/Ilias

Hi Ilias,

On Mon, 6 Feb 2023 at 06:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
> On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > As described in [0] if a command requires use of an untested algorithm
> > > or functional module, the TPM performs the test and then completes the
> > > command actions.
> > >
> > > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> > > the TPM in that case) and even if we would, it would complicate our TPM
> > > code for no apparent reason,  add a wrapper function that performs both
> > > the selftest and the startup sequence of the TPM.
> > >
> > > It's worth noting that this is implemented on TPMv2.0.  The code for
> > > 1.2 would look similar,  but I don't have a device available to test.
> > >
> > > [0]
> > > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> > > §12.3 Self-test modes
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v2:
> > > - add tpm_init() to auto start
> > >
> > > Changes since v1:
> > > - Remove a superfluous if statement
> > > - Move function comments to the header file
> > >  include/tpm-v2.h  | 19 +++++++++++++++++++
> > >  include/tpm_api.h |  8 ++++++++
> > >  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
> > >  lib/tpm_api.c     |  8 ++++++++
> > >  4 files changed, 59 insertions(+)
> >
> > I think this is a good idea, but it should be implemented at the API
> > level. Please see below.
>
> It is implemented at the API level.  I could try testing this in QEMU using
> swtpm, but the driver I added for mmio is TPMv2 only.  So I don't really feel
> comfortable adding support for a device I can't test.  If someone has a
> tpm1.2 and is willing to test it, I can modify the patch accordingly.

swtpm is a bit of a pain, as we discussed and as you are showing by
this comment. IMO it is good enough to use the sandbox testing for
this function. See below.

>
> >
> > >
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 737e57551d73..1c644f0048f6 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -688,4 +688,23 @@ u32 tpm2_report_state(struct udevice *dev, uint vendor_cmd, uint vendor_subcmd,
> > >  u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd,
> > >                           uint vendor_subcmd);
> > >
> > > +/**
> > > + * tpm2_auto_start() - start up the TPM and perform selftests.
> > > + *                     If a testable function has not been tested and is
> > > + *                     requested the TPM2  will return TPM_RC_NEEDS_TEST.
> > > + *
> > > + *
> > > + *
> >
> > drop extra lines
>
> Sure
>
> >
> > > + * @param dev          TPM device
> > > + * Return: TPM2_RC_TESTING, if TPM2 self-test has been received and the tests are
> >
> > 80 cols
>
> Sure
>
> >
> > > + *         not complete.
> > > + *         TPM2_RC_SUCCESS, if testing of all functions is complete without
> > > + *         functional failures.
>
> [...]
>
> > > +        */
> > > +       rc = tpm_init(dev);
> > > +       if (rc && rc != -EBUSY)
> >
> > Does that work, with rc being unsigned?
>
>
> Yes integer promotion is fine here.  As long as we don't try to do anything
> silly e.g start doing math on the result or compare it against bigger types
> this is fine.

OK

> There was a patch from Sughosh in the past that you rejected and it was
> converting enough of the TPM API return calls to int.  I think the reason
> the API works with u32, is that the spec return codes from the device
> itself are described as u32.  But that shouldn't affect the internal U-Boot
> API.  I can send a patch afterwards moving the U-Boot TPM API functions and
> return int.

Well, the question is, what is the return value? If it is the actual
TPM return code, then u32 is (unfortunately) correct. If it is an
errno then it should be int. The challenge is that some code wants to
know about the TPM internals, special error codes, etc. so we cannot
easily move to errno. Is that right?

>
> >
> > > +               return rc;
> > > +       rc = tpm2_self_test(dev, TPMI_YES);
> >
> > Can we call tpm_self_test_full() ? If not, please update the API as needed.
> >
> > > +
> > > +       if (rc == TPM2_RC_INITIALIZE) {
> > > +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);
> >
> > Should call tpm_startup()
>
>
> Same logic applies to both of these.  Yes that makes sense to use the top
> level API but only if we add support for 1.2 as well.  Otherwise the only
> thing this is going to create is circular dependencies between the v2 code
> and the API library.

Then just add 1.2 support.

>
> >
> > > +               if (rc)
> > > +                       return rc;
> > > +
> > > +               rc = tpm2_self_test(dev, TPMI_YES);
> >
> > Again, tpm_self_test_full(). We are trying to provide a TPM API that
> > covers v1 and v2, to the extent possible.
>
> See above.
>
> >
> > > +       }
> > > +
> > > +       return rc;
> > > +}
> > > +
> >
> > Also please add a test for this to test/dm/tpm.c
>
> sure
>
> >
> > >  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> > >                const ssize_t pw_sz)
> > >  {
> > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > index 7e8df8795ef3..5b2c11a277cc 100644
> > > --- a/lib/tpm_api.c
> > > +++ b/lib/tpm_api.c
> > > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> > >         }
> > >  }
> > >
> > > +u32 tpm_auto_start(struct udevice *dev)
> > > +{
> > > +       if (tpm_is_v2(dev))
> > > +               return tpm2_auto_start(dev);
> >
> > Hopefully all the code from above can move here.
>
> Repeating myself here, but I don't want to add untested code.  So I really
> prefer the patch as is, until someone verifies the 1.2 init sequence for
> me.

I don't see any tests in this patch anyway. We do have some very basic
tests for the TPM in test/dm/tpm.c and I'm sure you can add your
function there. That should be enough for now. It really doesn't make
sense to work around the existing API just as a way of not
implementing it for something you cannot manually test. It just adds
confusion when some functions use the API and some don't.

Regards,
Simon

Hi Simon,

Apologies for the late reply.

On Tue, Feb 07, 2023 at 06:38:54AM -0700, Simon Glass wrote:
> Hi Ilias,
>
> On Mon, 6 Feb 2023 at 06:25, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> > On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > As described in [0] if a command requires use of an untested algorithm
> > > > or functional module, the TPM performs the test and then completes the
> > > > command actions.
> > > >
> > > > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> > > > the TPM in that case) and even if we would, it would complicate our TPM
> > > > code for no apparent reason,  add a wrapper function that performs both
> > > > the selftest and the startup sequence of the TPM.
> > > >
> > > > It's worth noting that this is implemented on TPMv2.0.  The code for
> > > > 1.2 would look similar,  but I don't have a device available to test.
> > > >
> > > > [0]
> > > > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> > > > §12.3 Self-test modes
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > > Changes since v2:
> > > > - add tpm_init() to auto start
> > > >
> > > > Changes since v1:
> > > > - Remove a superfluous if statement
> > > > - Move function comments to the header file
> > > >  include/tpm-v2.h  | 19 +++++++++++++++++++
> > > >  include/tpm_api.h |  8 ++++++++
> > > >  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
> > > >  lib/tpm_api.c     |  8 ++++++++
> > > >  4 files changed, 59 insertions(+)
> > >
> > > I think this is a good idea, but it should be implemented at the API
> > > level. Please see below.
> >
> > It is implemented at the API level.  I could try testing this in QEMU using
> > swtpm, but the driver I added for mmio is TPMv2 only.  So I don't really feel
> > comfortable adding support for a device I can't test.  If someone has a
> > tpm1.2 and is willing to test it, I can modify the patch accordingly.
>
> swtpm is a bit of a pain, as we discussed and as you are showing by
> this comment. IMO it is good enough to use the sandbox testing for
> this function. See below.
>

The comment says the exact opposite.  It never hints on any swtpm
limitations or problems.

fwiw swtpm is fine.  It even offers a socket we could hook against instead of
re-inventing the wheel with the sandbox tpm (which is missing 95% of the
tpm functionality anyway).  The reason we can't use it is that the u-boot
driver only implements 2.0.

[...]

> > > > +       rc = tpm_init(dev);
> > > > +       if (rc && rc != -EBUSY)
> > >
> > > Does that work, with rc being unsigned?
> >
> >
> > Yes integer promotion is fine here.  As long as we don't try to do anything
> > silly e.g start doing math on the result or compare it against bigger types
> > this is fine.
>
> OK
>
> > There was a patch from Sughosh in the past that you rejected and it was
> > converting enough of the TPM API return calls to int.  I think the reason
> > the API works with u32, is that the spec return codes from the device
> > itself are described as u32.  But that shouldn't affect the internal U-Boot
> > API.  I can send a patch afterwards moving the U-Boot TPM API functions and
> > return int.
>
> Well, the question is, what is the return value? If it is the actual
> TPM return code, then u32 is (unfortunately) correct. If it is an
> errno then it should be int. The challenge is that some code wants to
> know about the TPM internals, special error codes, etc. so we cannot
> easily move to errno. Is that right?

Maybe, I'll have to take a closer look, but in principle I don't see why
the internal API should return device error codes verbatim.  If we care
about the tpm error that much we can pass a u32 * and still return an int.
In any case as we discussed integer promotion is fine here.

>
> >
> > >
> > > > +               return rc;
> > > > +       rc = tpm2_self_test(dev, TPMI_YES);
> > >
> > > Can we call tpm_self_test_full() ? If not, please update the API as needed.
> > >
> > > > +
> > > > +       if (rc == TPM2_RC_INITIALIZE) {
> > > > +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);
> > >
> > > Should call tpm_startup()
> >
> >
> > Same logic applies to both of these.  Yes that makes sense to use the top
> > level API but only if we add support for 1.2 as well.  Otherwise the only
> > thing this is going to create is circular dependencies between the v2 code
> > and the API library.
>
> Then just add 1.2 support.

That doesn't make any sense and I personally don't agree with the comment.
The patch improves and fixes problems we have in TPM 2.0 which is what
(hopefully) all devices use the last couple of years.

Unfortunately I don't have the time to fix 1.2 and I don't see why this
should hinder our efforts for decent TPM2.0 support.

>
> >
> > >
> > > > +               if (rc)
> > > > +                       return rc;
> > > > +
> > > > +               rc = tpm2_self_test(dev, TPMI_YES);
> > >
> > > Again, tpm_self_test_full(). We are trying to provide a TPM API that
> > > covers v1 and v2, to the extent possible.
> >
> > See above.
> >
> > >
> > > > +       }
> > > > +
> > > > +       return rc;
> > > > +}
> > > > +
> > >
> > > Also please add a test for this to test/dm/tpm.c
> >
> > sure
> >
> > >
> > > >  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> > > >                const ssize_t pw_sz)
> > > >  {
> > > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > > index 7e8df8795ef3..5b2c11a277cc 100644
> > > > --- a/lib/tpm_api.c
> > > > +++ b/lib/tpm_api.c
> > > > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> > > >         }
> > > >  }
> > > >
> > > > +u32 tpm_auto_start(struct udevice *dev)
> > > > +{
> > > > +       if (tpm_is_v2(dev))
> > > > +               return tpm2_auto_start(dev);
> > >
> > > Hopefully all the code from above can move here.
> >
> > Repeating myself here, but I don't want to add untested code.  So I really
> > prefer the patch as is, until someone verifies the 1.2 init sequence for
> > me.
>
> I don't see any tests in this patch anyway. We do have some very basic
> tests for the TPM in test/dm/tpm.c and I'm sure you can add your
> function there. That should be enough for now. It really doesn't make
> sense to work around the existing API just as a way of not
> implementing it for something you cannot manually test. It just adds
> confusion when some functions use the API and some don't.

I've already added the tests and having selftests in test/dm/tpm.c makes
sense.  What doesn't is requesting to fix 1.2 as well.  I'll send a v2 with
the tests included, but I can't change the API side of things.
I can send an RFC that fixes tpm1.2 if someone can test that on
real hardware.  But I don't have time to spend adding 1.2 support in sandbox.

Regards
/Ilias

>
> Regards,
> Simon

Hi Ilias,

On Thu, 16 Feb 2023 at 14:10, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> Apologies for the late reply.
>
> On Tue, Feb 07, 2023 at 06:38:54AM -0700, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Mon, 6 Feb 2023 at 06:25, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > > On Sat, Jan 28, 2023 at 03:01:22PM -0700, Simon Glass wrote:
> > > > Hi Ilias,
> > > >
> > > > On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > As described in [0] if a command requires use of an untested algorithm
> > > > > or functional module, the TPM performs the test and then completes the
> > > > > command actions.
> > > > >
> > > > > Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> > > > > the TPM in that case) and even if we would, it would complicate our TPM
> > > > > code for no apparent reason,  add a wrapper function that performs both
> > > > > the selftest and the startup sequence of the TPM.
> > > > >
> > > > > It's worth noting that this is implemented on TPMv2.0.  The code for
> > > > > 1.2 would look similar,  but I don't have a device available to test.
> > > > >
> > > > > [0]
> > > > > https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> > > > > §12.3 Self-test modes
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > > Changes since v2:
> > > > > - add tpm_init() to auto start
> > > > >
> > > > > Changes since v1:
> > > > > - Remove a superfluous if statement
> > > > > - Move function comments to the header file
> > > > >  include/tpm-v2.h  | 19 +++++++++++++++++++
> > > > >  include/tpm_api.h |  8 ++++++++
> > > > >  lib/tpm-v2.c      | 24 ++++++++++++++++++++++++
> > > > >  lib/tpm_api.c     |  8 ++++++++
> > > > >  4 files changed, 59 insertions(+)
> > > >
> > > > I think this is a good idea, but it should be implemented at the API
> > > > level. Please see below.
> > >
> > > It is implemented at the API level.  I could try testing this in QEMU using
> > > swtpm, but the driver I added for mmio is TPMv2 only.  So I don't really feel
> > > comfortable adding support for a device I can't test.  If someone has a
> > > tpm1.2 and is willing to test it, I can modify the patch accordingly.
> >
> > swtpm is a bit of a pain, as we discussed and as you are showing by
> > this comment. IMO it is good enough to use the sandbox testing for
> > this function. See below.
> >
>
> The comment says the exact opposite.  It never hints on any swtpm
> limitations or problems.

Manual testing is fine but it doesn't last. It is better to invest in
automated tests.

>
> fwiw swtpm is fine.  It even offers a socket we could hook against instead of
> re-inventing the wheel with the sandbox tpm (which is missing 95% of the
> tpm functionality anyway).  The reason we can't use it is that the u-boot
> driver only implements 2.0.

I wish I could convince you that emulators are not reinventing the
wheel. They are designed for testing. They run quickly, are easy to
debug and can test failure conditions, not just the happy path.

In Chrome OS we recently took our Zephyr code base from ~35% to 90%
test coverage using emulators, without a QEMU in sight[1]. Emulators
are a long-established software technique, one that has taken U-Boot a
long way.

>
> [...]
>
> > > > > +       rc = tpm_init(dev);
> > > > > +       if (rc && rc != -EBUSY)
> > > >
> > > > Does that work, with rc being unsigned?
> > >
> > >
> > > Yes integer promotion is fine here.  As long as we don't try to do anything
> > > silly e.g start doing math on the result or compare it against bigger types
> > > this is fine.
> >
> > OK
> >
> > > There was a patch from Sughosh in the past that you rejected and it was
> > > converting enough of the TPM API return calls to int.  I think the reason
> > > the API works with u32, is that the spec return codes from the device
> > > itself are described as u32.  But that shouldn't affect the internal U-Boot
> > > API.  I can send a patch afterwards moving the U-Boot TPM API functions and
> > > return int.
> >
> > Well, the question is, what is the return value? If it is the actual
> > TPM return code, then u32 is (unfortunately) correct. If it is an
> > errno then it should be int. The challenge is that some code wants to
> > know about the TPM internals, special error codes, etc. so we cannot
> > easily move to errno. Is that right?
>
> Maybe, I'll have to take a closer look, but in principle I don't see why
> the internal API should return device error codes verbatim.  If we care
> about the tpm error that much we can pass a u32 * and still return an int.
> In any case as we discussed integer promotion is fine here.

If we don't return the 'real' error codes then I worry that it will be
impossible to implement certain things. If we plan to map TPM errors
to errno, and can document all the cases, perhaps that would work, but
it seems like a lot of work.

>
> >
> > >
> > > >
> > > > > +               return rc;
> > > > > +       rc = tpm2_self_test(dev, TPMI_YES);
> > > >
> > > > Can we call tpm_self_test_full() ? If not, please update the API as needed.
> > > >
> > > > > +
> > > > > +       if (rc == TPM2_RC_INITIALIZE) {
> > > > > +               rc = tpm2_startup(dev, TPM2_SU_CLEAR);
> > > >
> > > > Should call tpm_startup()
> > >
> > >
> > > Same logic applies to both of these.  Yes that makes sense to use the top
> > > level API but only if we add support for 1.2 as well.  Otherwise the only
> > > thing this is going to create is circular dependencies between the v2 code
> > > and the API library.
> >
> > Then just add 1.2 support.
>
> That doesn't make any sense and I personally don't agree with the comment.
> The patch improves and fixes problems we have in TPM 2.0 which is what
> (hopefully) all devices use the last couple of years.
>
> Unfortunately I don't have the time to fix 1.2 and I don't see why this
> should hinder our efforts for decent TPM2.0 support.

The patch would actually be easier to write if you didn't limit it to
TPM2. To repeat myself, you are going around the TPM API we created,
because you don't want to use the sandbox emulator, which probably has
the functionality you need, or could do with 10 lines of code.

It really isn't saving time. Whose time is it saving?

>
> >
> > >
> > > >
> > > > > +               if (rc)
> > > > > +                       return rc;
> > > > > +
> > > > > +               rc = tpm2_self_test(dev, TPMI_YES);
> > > >
> > > > Again, tpm_self_test_full(). We are trying to provide a TPM API that
> > > > covers v1 and v2, to the extent possible.
> > >
> > > See above.
> > >
> > > >
> > > > > +       }
> > > > > +
> > > > > +       return rc;
> > > > > +}
> > > > > +
> > > >
> > > > Also please add a test for this to test/dm/tpm.c
> > >
> > > sure
> > >
> > > >
> > > > >  u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> > > > >                const ssize_t pw_sz)
> > > > >  {
> > > > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > > > index 7e8df8795ef3..5b2c11a277cc 100644
> > > > > --- a/lib/tpm_api.c
> > > > > +++ b/lib/tpm_api.c
> > > > > @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> > > > >         }
> > > > >  }
> > > > >
> > > > > +u32 tpm_auto_start(struct udevice *dev)
> > > > > +{
> > > > > +       if (tpm_is_v2(dev))
> > > > > +               return tpm2_auto_start(dev);
> > > >
> > > > Hopefully all the code from above can move here.
> > >
> > > Repeating myself here, but I don't want to add untested code.  So I really
> > > prefer the patch as is, until someone verifies the 1.2 init sequence for
> > > me.
> >
> > I don't see any tests in this patch anyway. We do have some very basic
> > tests for the TPM in test/dm/tpm.c and I'm sure you can add your
> > function there. That should be enough for now. It really doesn't make
> > sense to work around the existing API just as a way of not
> > implementing it for something you cannot manually test. It just adds
> > confusion when some functions use the API and some don't.
>
> I've already added the tests and having selftests in test/dm/tpm.c makes
> sense.  What doesn't is requesting to fix 1.2 as well.  I'll send a v2 with
> the tests included, but I can't change the API side of things.
> I can send an RFC that fixes tpm1.2 if someone can test that on
> real hardware.  But I don't have time to spend adding 1.2 support in sandbox.

OK, well I will take the time to do this properly in the API. I will
keep track of how long I spend on it and let you know.

Regards,
Simon

[1] https://www.youtube.com/watch?v=WmAgXYTj4Fg