| Message ID | 20230510074359.2837818-9-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] tpm: Fix spelling for tpmu_ha union | expand |
Hi Ilias, On Wed, 10 May 2023 at 01:44, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > For a TPM device to be operational we need to initialize it and > perform its startup sequence. The 'tpm init' command currently calls > tpm_init() which ends up calling the ->open() per-device callback and > performs the initial hardware configuration as well as requesting > locality 0 for the caller. We recently added tpm_auto_start() though, > which automates the initialization process -- On top of that calling > tpm_init() on selftests is a bit problematic, since calling it twice > will return -EBUSY the second time although there is no actual problem > with the TPM or the software stack. > > So let's wire up the 'tpm init' command and call tpm_auto_start() which > leaves the device in an operational state. > > It's worth noting that calling tpm_init() only, doesn't allow a someone > to use the TPM since the startup sequence is mandatory. We keep > repeating the pattern of calling > - tpm_init > - tpm_startup > - tpm_self_test_full or tpm_continue_self_test > > So we don't expect any regression or boot delays with the current > change. > > While at it fix the identation of test_tpm_autostart() comments as well > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/tpm-common.c | 3 ++- > test/dm/tpm.c | 9 +++++---- > 2 files changed, 7 insertions(+), 5 deletions(-) We've been through this before. I do understand that EFI just does everything in U-Boot proper, but it is better for previous phases to set up the TPM, e.g. VPL, as we discussed on irc. In that case we cannot init the TPM twice. I think what you want is a new 'tpm autostart' command, or something like that? You already have the tpm_auto_start() function so you can call that as needed. Regards, Simon
On Wed, 10 May 2023 at 17:32, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 10 May 2023 at 01:44, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > For a TPM device to be operational we need to initialize it and > > perform its startup sequence. The 'tpm init' command currently calls > > tpm_init() which ends up calling the ->open() per-device callback and > > performs the initial hardware configuration as well as requesting > > locality 0 for the caller. We recently added tpm_auto_start() though, > > which automates the initialization process -- On top of that calling > > tpm_init() on selftests is a bit problematic, since calling it twice > > will return -EBUSY the second time although there is no actual problem > > with the TPM or the software stack. > > > > So let's wire up the 'tpm init' command and call tpm_auto_start() which > > leaves the device in an operational state. > > > > It's worth noting that calling tpm_init() only, doesn't allow a someone > > to use the TPM since the startup sequence is mandatory. We keep > > repeating the pattern of calling > > - tpm_init > > - tpm_startup > > - tpm_self_test_full or tpm_continue_self_test > > > > So we don't expect any regression or boot delays with the current > > change. > > > > While at it fix the identation of test_tpm_autostart() comments as well > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > cmd/tpm-common.c | 3 ++- > > test/dm/tpm.c | 9 +++++---- > > 2 files changed, 7 insertions(+), 5 deletions(-) > > We've been through this before. I do understand that EFI just does > everything in U-Boot proper, but it is better for previous phases to > set up the TPM, e.g. VPL, as we discussed on irc. In that case we > cannot init the TPM twice. Why can't we? Nothing bad happens to the device and the auto start function takes that into account and doesn't run tpm2_startup() twice if it's already initialized. > > I think what you want is a new 'tpm autostart' command, or something > like that? You already have the tpm_auto_start() function so you can > call that as needed. I don't like having many confusing ways of starting the TPM. To me 'init' means, initialize the device so I can use it. Our code right now needs 4 extra commands to happen which is confusing at best. Do you have any measurements that running auto start twice adds substantial overhead? Not to mention that tpm_init() returns 2 different error codes even if no errors are there. Half oof our code just ignores the return code of tpm_init due to that. So my plan is to get rid of it eventually and only have one sane way of starting the device Thanks /Ilias > > Regards, > Simon
On Wed, 10 May 2023 at 18:32, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 10 May 2023 at 17:32, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 10 May 2023 at 01:44, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > For a TPM device to be operational we need to initialize it and > > > perform its startup sequence. The 'tpm init' command currently calls > > > tpm_init() which ends up calling the ->open() per-device callback and > > > performs the initial hardware configuration as well as requesting > > > locality 0 for the caller. We recently added tpm_auto_start() though, > > > which automates the initialization process -- On top of that calling > > > tpm_init() on selftests is a bit problematic, since calling it twice > > > will return -EBUSY the second time although there is no actual problem > > > with the TPM or the software stack. > > > > > > So let's wire up the 'tpm init' command and call tpm_auto_start() which > > > leaves the device in an operational state. > > > > > > It's worth noting that calling tpm_init() only, doesn't allow a someone > > > to use the TPM since the startup sequence is mandatory. We keep > > > repeating the pattern of calling > > > - tpm_init > > > - tpm_startup > > > - tpm_self_test_full or tpm_continue_self_test > > > > > > So we don't expect any regression or boot delays with the current > > > change. > > > > > > While at it fix the identation of test_tpm_autostart() comments as well > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > cmd/tpm-common.c | 3 ++- > > > test/dm/tpm.c | 9 +++++---- > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > We've been through this before. I do understand that EFI just does > > everything in U-Boot proper, but it is better for previous phases to > > set up the TPM, e.g. VPL, as we discussed on irc. In that case we > > cannot init the TPM twice. I also forgot to answer this. EFI does *not* need anything in u-boot proper. In fact you rejected a patch which was initializing the TPM earlier in the past with the 'it's slow' argument. You also argued we should just init it in the efi subsystem when we need it (which we do). So if you ever want to initialize the device early, you *must* use the tpm_auto_start anyway and I can remove the ad-hoc init in the EFI subsystem. But even if we leave it there, it will make no difference. Thanks /Ilias > > Why can't we? Nothing bad happens to the device and the auto start > function takes that into account and doesn't run tpm2_startup() twice > if it's already initialized. > > > > > I think what you want is a new 'tpm autostart' command, or something > > like that? You already have the tpm_auto_start() function so you can > > call that as needed. > > I don't like having many confusing ways of starting the TPM. To me > 'init' means, initialize the device so I can use it. Our code right > now needs 4 extra commands to happen which is confusing at best. Do > you have any measurements that running auto start twice adds > substantial overhead? Not to mention that tpm_init() returns 2 > different error codes even if no errors are there. Half oof our code > just ignores the return code of tpm_init due to that. So my plan is > to get rid of it eventually and only have one sane way of starting the > device > > Thanks > /Ilias > > > > Regards, > > Simon
diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c index d0c63cadf413..9b1ad0b371df 100644 --- a/cmd/tpm-common.c +++ b/cmd/tpm-common.c @@ -11,6 +11,7 @@ #include <asm/unaligned.h> #include <linux/string.h> #include <tpm-common.h> +#include <tpm_api.h> #include "tpm-user-utils.h" static struct udevice *tpm_dev; @@ -364,7 +365,7 @@ int do_tpm_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (rc) return rc; - return report_return_code(tpm_init(dev)); + return report_return_code(tpm_auto_start(dev)); } int do_tpm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) diff --git a/test/dm/tpm.c b/test/dm/tpm.c index 3defb3c3da1f..cde933ab2848 100644 --- a/test/dm/tpm.c +++ b/test/dm/tpm.c @@ -98,10 +98,11 @@ static int test_tpm_autostart(struct unit_test_state *uts, if (reinit) ut_assertok(tpm_init(dev)); - /* - * tpm_auto_start will rerun tpm_init() if reinit, but handles the - * -EBUSY return code internally. - */ + + /* + * tpm_auto_start will rerun tpm_init() if reinit, but handles the + * -EBUSY return code internally. + */ ut_assertok(tpm_auto_start(dev)); return 0;
For a TPM device to be operational we need to initialize it and perform its startup sequence. The 'tpm init' command currently calls tpm_init() which ends up calling the ->open() per-device callback and performs the initial hardware configuration as well as requesting locality 0 for the caller. We recently added tpm_auto_start() though, which automates the initialization process -- On top of that calling tpm_init() on selftests is a bit problematic, since calling it twice will return -EBUSY the second time although there is no actual problem with the TPM or the software stack. So let's wire up the 'tpm init' command and call tpm_auto_start() which leaves the device in an operational state. It's worth noting that calling tpm_init() only, doesn't allow a someone to use the TPM since the startup sequence is mandatory. We keep repeating the pattern of calling - tpm_init - tpm_startup - tpm_self_test_full or tpm_continue_self_test So we don't expect any regression or boot delays with the current change. While at it fix the identation of test_tpm_autostart() comments as well Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/tpm-common.c | 3 ++- test/dm/tpm.c | 9 +++++---- 2 files changed, 7 insertions(+), 5 deletions(-)