test/py: always use autostart on tpm2 selftests

commit 789ed2784256 ("test/py: replace 'tpm2 init, startup, selftest' sequences")
changed some of the tpm2 init sequences to 'tpm2 autostart' instead of
calling 'tpm init', 'tpm startup TPM2_SU_CLEAR', 'tpm2 self_test full'.

The autostart command calls the afforementioned sequence and on top of
that deals with the 'tpm2 init' return codes if the tpm is already
started. Since we initialize the tpm from various subsystems now,
replace the last remaining instances of 'tpm2 init' with 'tpm2
autostart'.  Since the latter calls 'tpm2 init' anyway we will still be
implicitly testing the validity of that command

It's worth noting that since 'tpm2 autostart' performs the startup and
self tests sequences of the tpm we could drop
'test_tpm2_sandbox_self_test_full' and 'test_tpm2_startup, but let's
keep the since they test tpm commands and options

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 test/py/tests/test_tpm2.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Ilias,

On Wed, 25 Oct 2023 at 07:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> commit 789ed2784256 ("test/py: replace 'tpm2 init, startup, selftest' sequences")
> changed some of the tpm2 init sequences to 'tpm2 autostart' instead of
> calling 'tpm init', 'tpm startup TPM2_SU_CLEAR', 'tpm2 self_test full'.
>
> The autostart command calls the afforementioned sequence and on top of
> that deals with the 'tpm2 init' return codes if the tpm is already
> started. Since we initialize the tpm from various subsystems now,
> replace the last remaining instances of 'tpm2 init' with 'tpm2
> autostart'.  Since the latter calls 'tpm2 init' anyway we will still be
> implicitly testing the validity of that command
>
> It's worth noting that since 'tpm2 autostart' performs the startup and
> self tests sequences of the tpm we could drop
> 'test_tpm2_sandbox_self_test_full' and 'test_tpm2_startup, but let's
> keep the since they test tpm commands and options
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  test/py/tests/test_tpm2.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py
> index 47392b87a98f..1d654cd4a23b 100644
> --- a/test/py/tests/test_tpm2.py
> +++ b/test/py/tests/test_tpm2.py
> @@ -61,7 +61,7 @@ def test_tpm2_init(u_boot_console):
>      skip_test = u_boot_console.config.env.get('env__tpm_device_test_skip', False)
>      if skip_test:
>          pytest.skip('skip TPM device test')
> -    u_boot_console.run_command('tpm2 init')
> +    u_boot_console.run_command('tpm2 autostart')
>      output = u_boot_console.run_command('echo $?')
>      assert output.endswith('0')
>
> @@ -100,7 +100,7 @@ def test_tpm2_sandbox_self_test_full(u_boot_console):
>      """
>      if is_sandbox(u_boot_console):
>          u_boot_console.restart_uboot()

We should not need this. Can you please take a look at what reset is
needed in the TPM code to get the TPM back into its original state? We
have dm_test_pre_run() which could call a tpm_reset_for_test()
function, or similar.

> -        u_boot_console.run_command('tpm2 init')
> +        u_boot_console.run_command('tpm2 autostart')
>          output = u_boot_console.run_command('echo $?')
>          assert output.endswith('0')
>
> --
> 2.37.2
>

Regards,
Simon

Hi Simon,

[...]

> > ---
> >  test/py/tests/test_tpm2.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py
> > index 47392b87a98f..1d654cd4a23b 100644
> > --- a/test/py/tests/test_tpm2.py
> > +++ b/test/py/tests/test_tpm2.py
> > @@ -61,7 +61,7 @@ def test_tpm2_init(u_boot_console):
> >      skip_test = u_boot_console.config.env.get('env__tpm_device_test_skip', False)
> >      if skip_test:
> >          pytest.skip('skip TPM device test')
> > -    u_boot_console.run_command('tpm2 init')
> > +    u_boot_console.run_command('tpm2 autostart')
> >      output = u_boot_console.run_command('echo $?')
> >      assert output.endswith('0')
> >
> > @@ -100,7 +100,7 @@ def test_tpm2_sandbox_self_test_full(u_boot_console):
> >      """
> >      if is_sandbox(u_boot_console):
> >          u_boot_console.restart_uboot()
>
> We should not need this. Can you please take a look at what reset is
> needed in the TPM code to get the TPM back into its original state? We
> have dm_test_pre_run() which could call a tpm_reset_for_test()
> function, or similar.

We do need this once I land Eddie's patches [0].  The problem is not
related to reset sequences or what's needed on the TPM. Before this
patchset, the sandbox TPM lacked a bunch of capabilities and we
couldn't test UEFI measured boot.

Eddie added those missing capabilities and since the EFI subsystem
comes up, it also initializes the TPM.  As a result 'tpm2 init' will
see the device in open state and return -EBUSY, but that's *not* an
actual error.

So instead of playing around with ordering of tests and failing if
someone initialized the TPM early or whatever we should just use
tpm_auto_start which
- Does the right thing and gracefully handles that -EBUSY
- Still allows you to run the remaining sequence of a TPM
initialization -- tpm2 startup TPM2_SU_CLEAR && tpm2 self_test full

[0] https://lore.kernel.org/u-boot/20231024154354.197524-3-eajames@linux.ibm.com/

For reference here's the sandbox output, compiled with
sandbox64_defconfig with Eddie's patches applied

U-Boot 2024.01-rc1-00043-g3ecf75343640-dirty (Oct 26 2023 - 10:57:54 +0300)

Reset Status: WARM Reset Status: COLD
Model: sandbox
DRAM:  256 MiB
using memory 0x1b513000-0x1f515000 for malloc()
[nvmxip-qspi1@08000000]: the block device blk#2 ready for use
[nvmxip-qspi2@08200000]: the block device blk#2 ready for use
Core:  260 devices, 92 uclasses, devicetree: board
WDT:   Not starting wdt-gpio-toggle
WDT:   Not starting wdt-gpio-level
WDT:   Not starting wdt@0
MMC:   Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
mmc1 - probe failed: -1
mmc2: 2 (SD)Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
, mmc0: 0 (SD)
Loading Environment from nowhere... OK
In:    serial,cros-ec-keyb,usbkbd
Out:   serial,vidconsole
Err:   serial,vidconsole
Model: sandbox
Net:   eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6:
eth@10004000, eth8: phy-test-eth, eth4: dsa-test-eth, eth2: lan0,
eth7: lan1
Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
No EFI system partition    <-------- UEFI subsystem init
No EFI system partition
Failed to persist EFI variables
EFI using ACPI tables at b746000
Hit any key to stop autoboot:  0

Thanks
/Ilias
>
> > -        u_boot_console.run_command('tpm2 init')
> > +        u_boot_console.run_command('tpm2 autostart')
> >          output = u_boot_console.run_command('echo $?')
> >          assert output.endswith('0')
> >
> > --
> > 2.37.2
> >
>
> Regards,
> Simon

Hi Ilias,

On Wed, 25 Oct 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 25 Oct 2023 at 07:25, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > commit 789ed2784256 ("test/py: replace 'tpm2 init, startup, selftest' sequences")
> > changed some of the tpm2 init sequences to 'tpm2 autostart' instead of
> > calling 'tpm init', 'tpm startup TPM2_SU_CLEAR', 'tpm2 self_test full'.
> >
> > The autostart command calls the afforementioned sequence and on top of
> > that deals with the 'tpm2 init' return codes if the tpm is already
> > started. Since we initialize the tpm from various subsystems now,
> > replace the last remaining instances of 'tpm2 init' with 'tpm2
> > autostart'.  Since the latter calls 'tpm2 init' anyway we will still be
> > implicitly testing the validity of that command
> >
> > It's worth noting that since 'tpm2 autostart' performs the startup and
> > self tests sequences of the tpm we could drop
> > 'test_tpm2_sandbox_self_test_full' and 'test_tpm2_startup, but let's
> > keep the since they test tpm commands and options
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  test/py/tests/test_tpm2.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py
> > index 47392b87a98f..1d654cd4a23b 100644
> > --- a/test/py/tests/test_tpm2.py
> > +++ b/test/py/tests/test_tpm2.py
> > @@ -61,7 +61,7 @@ def test_tpm2_init(u_boot_console):
> >      skip_test = u_boot_console.config.env.get('env__tpm_device_test_skip', False)
> >      if skip_test:
> >          pytest.skip('skip TPM device test')
> > -    u_boot_console.run_command('tpm2 init')
> > +    u_boot_console.run_command('tpm2 autostart')
> >      output = u_boot_console.run_command('echo $?')
> >      assert output.endswith('0')
> >
> > @@ -100,7 +100,7 @@ def test_tpm2_sandbox_self_test_full(u_boot_console):
> >      """
> >      if is_sandbox(u_boot_console):
> >          u_boot_console.restart_uboot()
>
> We should not need this. Can you please take a look at what reset is
> needed in the TPM code to get the TPM back into its original state? We
> have dm_test_pre_run() which could call a tpm_reset_for_test()
> function, or similar.
>
> > -        u_boot_console.run_command('tpm2 init')
> > +        u_boot_console.run_command('tpm2 autostart')
> >          output = u_boot_console.run_command('echo $?')
> >          assert output.endswith('0')

Somehow I didn't see your reply come through in my email.

There are two problems as I see it:

tpm2_init() needs to get the 'tpm2 init' command. If that test can
only pass on sandbox, then we should mark it as such, perhaps creating
a different test for tpm_autostart

The second chunk is definitely for sandbox only, so we should fix the
reset_for_test stuff so that the TPM is inited before every test. That
is what we do for the rest of the sandbox tests. If you are not sure
or need me to dig into how to do that, let me know. But we should not
paper over this...it has been a problem for long enough now, causing
us to add hacks like restarting U-Boot, etc.

Regards,
Simon

Hi Simon




On Fri, Oct 27, 2023, 22:15 Simon Glass <sjg@chromium.org> wrote:

> Hi Ilias,
>
> On Wed, 25 Oct 2023 at 18:23, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 25 Oct 2023 at 07:25, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > commit 789ed2784256 ("test/py: replace 'tpm2 init, startup, selftest'
> sequences")
> > > changed some of the tpm2 init sequences to 'tpm2 autostart' instead of
> > > calling 'tpm init', 'tpm startup TPM2_SU_CLEAR', 'tpm2 self_test full'.
> > >
> > > The autostart command calls the afforementioned sequence and on top of
> > > that deals with the 'tpm2 init' return codes if the tpm is already
> > > started. Since we initialize the tpm from various subsystems now,
> > > replace the last remaining instances of 'tpm2 init' with 'tpm2
> > > autostart'.  Since the latter calls 'tpm2 init' anyway we will still be
> > > implicitly testing the validity of that command
> > >
> > > It's worth noting that since 'tpm2 autostart' performs the startup and
> > > self tests sequences of the tpm we could drop
> > > 'test_tpm2_sandbox_self_test_full' and 'test_tpm2_startup, but let's
> > > keep the since they test tpm commands and options
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  test/py/tests/test_tpm2.py | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py
> > > index 47392b87a98f..1d654cd4a23b 100644
> > > --- a/test/py/tests/test_tpm2.py
> > > +++ b/test/py/tests/test_tpm2.py
> > > @@ -61,7 +61,7 @@ def test_tpm2_init(u_boot_console):
> > >      skip_test =
> u_boot_console.config.env.get('env__tpm_device_test_skip', False)
> > >      if skip_test:
> > >          pytest.skip('skip TPM device test')
> > > -    u_boot_console.run_command('tpm2 init')
> > > +    u_boot_console.run_command('tpm2 autostart')
> > >      output = u_boot_console.run_command('echo $?')
> > >      assert output.endswith('0')
> > >
> > > @@ -100,7 +100,7 @@ def
> test_tpm2_sandbox_self_test_full(u_boot_console):
> > >      """
> > >      if is_sandbox(u_boot_console):
> > >          u_boot_console.restart_uboot()
> >
> > We should not need this. Can you please take a look at what reset is
> > needed in the TPM code to get the TPM back into its original state? We
> > have dm_test_pre_run() which could call a tpm_reset_for_test()
> > function, or similar.
> >
> > > -        u_boot_console.run_command('tpm2 init')
> > > +        u_boot_console.run_command('tpm2 autostart')
> > >          output = u_boot_console.run_command('echo $?')
> > >          assert output.endswith('0')
>
> Somehow I didn't see your reply come through in my email.
>
> There are two problems as I see it:
>
> tpm2_init() needs to get the 'tpm2 init' command. If that test can
> only pass on sandbox, then we should mark it as such, perhaps creating
> a different test for tpm_autostart
>

Ok. Please note that Tom pulled this along with the rest of the tpm series.
I'll revert it Monday and send a fix.


> The second chunk is definitely for sandbox only, so we should fix the
> reset_for_test stuff so that the TPM is inited before every test. That
> is what we do for the rest of the sandbox tests. If you are not sure
> or need me to dig into how to do that, let me know. But we should not
> paper over this...it has been a problem for long enough now, causing
> us to add hacks like restarting U-Boot, etc.
>

To do that we need to add a tpm_close. Should we do that or simply not fail
on EBUSY, which isn't actually an error?

Thanks
Ilias

>
> Regards,
> Simon
>