Hi Julia, On Wed, 2 Aug 2023 at 02:28, Julia Daxenberger <julia.daxenber...@infineon.com> wrote: > > Hi Simon, > > On Thu, 6 Jul 2023 at 09:57, Simon Glass <s...@chromium.org> wrote: > > > > Hi Julia, > > > > On Wed, 5 Jul 2023 at 00:55, Julia Daxenberger > > <julia.daxenber...@infineon.com> wrote: > > > > > > Hi Simon, > > > > > > On Mon, 3 Jul 2023 at 15:31, Simon Glass <s...@chromium.org> wrote: > > > > > > > > Hi Julia, > > > > > > > > On Mon, 3 Jul 2023 at 14:03, Julia Daxenberger > > > > <julia.daxenber...@infineon.com> wrote: > > > > > > > > > > Add TPM2_GetTestResult command support and change the command file > > > > > and the > > > > > help accordingly. Add Python tests and sandbox driver functionality. > > > > > > > > > > The TPM2_GetTestResult command is performed after the TPM2_SelfTest > > > > > command > > > > > and returns manufacturer-specific information regarding the results > > > > > of the > > > > > self-test and an indication of the test status. > > > > > > > > > > Signed-off-by: Julia Daxenberger <julia.daxenber...@infineon.com> > > > > > --- > > > > > cmd/tpm-v2.c | 60 +++++++++++++++++++++++++ > > > > > drivers/tpm/tpm2_tis_sandbox.c | 47 ++++++++++++++++++- > > > > > include/tpm-v2.h | 23 ++++++++++ > > > > > lib/tpm-v2.c | 82 > > > > > ++++++++++++++++++++++++++++++++++ > > > > > test/py/tests/test_tpm2.py | 50 +++++++++++++++++++++ > > > > > 5 files changed, 261 insertions(+), 1 deletion(-) > > > > > > > > > > > > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > > > > > > > > > See below > > > > > > > > [..] > > > > > > > > > diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py > > > > > index d2ad6f9e73..aad1d7a55b 100644 > > > > > --- a/test/py/tests/test_tpm2.py > > > > > +++ b/test/py/tests/test_tpm2.py > > > > > @@ -1,6 +1,7 @@ > > > > > # SPDX-License-Identifier: GPL-2.0+ > > > > > # Copyright (c) 2018, Bootlin > > > > > # Author: Miquel Raynal <miquel.ray...@bootlin.com> > > > > > +# Copyright (C) 2023 Infineon Technologies AG > > > > > > > > > > import os.path > > > > > import pytest > > > > > @@ -316,3 +317,52 @@ def test_tpm2_cleanup(u_boot_console): > > > > > """Ensure the TPM is cleared from password or test related > > > > > configuration.""" > > > > > > > > > > force_init(u_boot_console, True) > > > > > + > > > > > +@pytest.mark.buildconfigspec('cmd_tpm_v2') > > > > > +def test_tpm2_get_test_result(u_boot_console): > > > > > + """Execute a TPM_GetTestResult command. > > > > > + > > > > > + Ask the TPM to get the test result of the self test. > > > > > + Display the Test Result and Test Result Data. > > > > > + > > > > > + Expected default return value of tpm2_get_test_result, if the > > > > > TPM has not been initialized: > > > > > + - TPM2_RC_INITIALIZE = TPM2_RC_VER1 + 0x0000 = 0x00000100. > > > > > + > > > > > + Expected default value for test_result: > > > > > + - TPM_RC_NEEDS_TEST = 0x00000153, if tpm2 self_test has not been > > > > > executed. > > > > > + - TPM_RC_SUCCESS = 0x00000000, if testing is complete without > > > > > functional failures. > > > > > + > > > > > + There is no expected default value for the test result data > > > > > because it would depend on the chip > > > > > + used. The test result data is therefore not tested. > > > > > + """ > > > > > + if is_sandbox(u_boot_console): > > > > > + u_boot_console.restart_uboot() > > > > > > > > We should get rid of this somehow. We don't want sandbox rebooting > > > > inthe middle of a test. It makes debugging painful, apart from > > > > anything else. What TPM state needs to be reset? > > > > > > > > Looking at tpm2_tis_sandbox.c it is probably the s_state variable. The > > > > TPM state can be preserved across runs and is stored in the state > > > > file. > > > > > > > > But if the state file is not being used (no -s argument) then the TPM > > > > should be reset each time DM is brought back up, i.e. between every > > > > test. > > > > > > > > So, do we even need this reset? > > > > > > In my understanding, shutting down and restarting U-Boot with > > > restart_uboot() at the beginning of a test is beneficial, as it puts > > > sandbox back into a known state for the test. This allows all tests to > > > run in parallel, since no test depends on another. > > > > Actually with sandbox, we just restart the driver model each time. You > > can see this logic in test-main.c > > > > > > > > In the test_tpm2_get_test_result test specifically, the reset is needed > > > to test the behaviour of the do_tpm2_get_test_result() function, when > > > the tpm has not already been initialized and the TPM self_test has not > > > been executed. Performing restart_uboot() is essential to test for error > > > code output, as sandbox always returns "TPM2_RC_SUCCESS" when the > > > restart is not performed, making the test fail. > > > > > > restart_uboot() is performed at the beginning of all tests in > > > test_tpm2.py, either on its own, or as part of the tpm2_sandbox_init() > > > command. > > > > Yes, we should fix this. No other subsystem has this problem. I am > > wondering what is actually going on? The tpm must be keeping state > > outside of driver model. Perhaps we just need to call a TPM function > > to clear that state, in state_reset_for_test(). > > After looking at the code again, I agree that rebooting sandbox at the > beginning of every test is not optimal. Currently, I do not have an > in-depth understanding of the needed changes and their full impact. > Taking into account the current changes to tpm functions, such as the > new startup sequence, I would prefer to retain the restart for now. > > I will send a new version of the patch shortly, where I will move the > tests relying on the restart into an additional patch, to be merged > independently.
OK that sounds good, thank you. With that final patch, I should be able to figure out what is going wrong, so it is not needed. Regards, Simon