Re: [PATCH v2 13/13] vtpm: Correct timeout units and command duration

From: "Daniel P. Smith" <dpsmith.dev@gmail.com>
To: Jason Andryuk <jandryuk@gmail.com>, xen-devel@lists.xenproject.org
Cc: Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Samuel Thibault <samuel.thibault@ens-lyon.org>
Subject: Re: [PATCH v2 13/13] vtpm: Correct timeout units and command duration
Date: Mon, 10 May 2021 09:40:24 -0400	[thread overview]
Message-ID: <e0658f24-5646-c145-c232-dbccd86cb064@gmail.com> (raw)
In-Reply-To: <20210506135923.161427-14-jandryuk@gmail.com>

On 5/6/21 9:59 AM, Jason Andryuk wrote:
> Add two patches:
> vtpm-microsecond-duration.patch fixes the units for timeouts and command
> durations.
> vtpm-command-duration.patch increases the timeout linux uses to allow
> commands to succeed.
> 
> Linux works around low timeouts, but not low durations.  The second
> patch allows commands to complete that often timeout with the lower
> command durations.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

>  stubdom/Makefile                        |  2 +
>  stubdom/vtpm-command-duration.patch     | 52 +++++++++++++++++++++++++
>  stubdom/vtpm-microsecond-duration.patch | 52 +++++++++++++++++++++++++
>  3 files changed, 106 insertions(+)
>  create mode 100644 stubdom/vtpm-command-duration.patch
>  create mode 100644 stubdom/vtpm-microsecond-duration.patch
> 
> diff --git a/stubdom/Makefile b/stubdom/Makefile
> index c6de5f68ae..06aa69d8bc 100644
> --- a/stubdom/Makefile
> +++ b/stubdom/Makefile
> @@ -239,6 +239,8 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz
>  	patch -d $@ -p1 < vtpm-implicit-fallthrough.patch
>  	patch -d $@ -p1 < vtpm_TPM_ChangeAuthAsymFinish.patch
>  	patch -d $@ -p1 < vtpm_extern.patch
> +	patch -d $@ -p1 < vtpm-microsecond-duration.patch
> +	patch -d $@ -p1 < vtpm-command-duration.patch
>  	mkdir $@/build
>  	cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement"
>  	touch $@
> diff --git a/stubdom/vtpm-command-duration.patch b/stubdom/vtpm-command-duration.patch
> new file mode 100644
> index 0000000000..6fdf2fc9be
> --- /dev/null
> +++ b/stubdom/vtpm-command-duration.patch
> @@ -0,0 +1,52 @@
> +From e7c976b5864e7d2649292d90ea60d5aea091a990 Mon Sep 17 00:00:00 2001
> +From: Jason Andryuk <jandryuk@gmail.com>
> +Date: Sun, 14 Mar 2021 12:46:34 -0400
> +Subject: [PATCH 2/2] Increase command durations
> +
> +Wth Linux 5.4 xen-tpmfront and a Xen vtpm-stubdom, xen-tpmfront was
> +failing commands with -ETIME:
> +tpm tpm0: tpm_try_transmit: send(): error-62
> +
> +The vtpm was returning the data, but it was after the duration timeout
> +in vtpm_send.  Linux may have started being more stringent about timing?
> +
> +The vtpm-stubdom has a little delay since it writes its disk before
> +returning the response.
> +
> +Anyway, the durations are rather low.  When they were 1/10/1000 before
> +converting to microseconds, Linux showed all three durations rounded to
> +10000.  Update them with values from a physical TPM1.2.  These were
> +taken from a WEC which was software downgraded from a TPM2 to a TPM1.2.
> +They might be excessive, but I'd rather have a command succeed than
> +return -ETIME.
> +
> +An IFX physical TPM1.2 uses:
> +1000000
> +1500000
> +150000000
> +
> +Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> +---
> + tpm/tpm_data.c | 6 +++---
> + 1 file changed, 3 insertions(+), 3 deletions(-)
> +
> +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c
> +index bebaf10..844afca 100644
> +--- a/tpm/tpm_data.c
> ++++ b/tpm/tpm_data.c
> +@@ -71,9 +71,9 @@ static void init_timeouts(void)
> +   tpmData.permanent.data.tis_timeouts[1] = 2000000;
> +   tpmData.permanent.data.tis_timeouts[2] = 750000;
> +   tpmData.permanent.data.tis_timeouts[3] = 750000;
> +-  tpmData.permanent.data.cmd_durations[0] = 1000;
> +-  tpmData.permanent.data.cmd_durations[1] = 10000;
> +-  tpmData.permanent.data.cmd_durations[2] = 1000000;
> ++  tpmData.permanent.data.cmd_durations[0] = 3000000;
> ++  tpmData.permanent.data.cmd_durations[1] = 3000000;
> ++  tpmData.permanent.data.cmd_durations[2] = 600000000;
> + }
> + 
> + void tpm_init_data(void)
> +-- 
> +2.30.2
> +
> diff --git a/stubdom/vtpm-microsecond-duration.patch b/stubdom/vtpm-microsecond-duration.patch
> new file mode 100644
> index 0000000000..7a906e72c5
> --- /dev/null
> +++ b/stubdom/vtpm-microsecond-duration.patch
> @@ -0,0 +1,52 @@
> +From 5a510e0afd7c288e3f0fb3523ec749ba1366ad61 Mon Sep 17 00:00:00 2001
> +From: Jason Andryuk <jandryuk@gmail.com>
> +Date: Sun, 14 Mar 2021 12:42:10 -0400
> +Subject: [PATCH 1/2] Use microseconds for timeouts and durations
> +
> +The timeout and duration fields should be in microseconds according to
> +the spec.
> +
> +TPM_CAP_PROP_TIS_TIMEOUT:
> +A 4 element array of UINT32 values each denoting the timeout value in
> +microseconds for the following in this order:
> +
> +TPM_CAP_PROP_DURATION:
> +A 3 element array of UINT32 values each denoting the duration value in
> +microseconds of the duration of the three classes of commands:
> +
> +Linux will scale the timeouts up by 1000, but not the durations.  Change
> +the units for both sets as appropriate.
> +
> +Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> +---
> + tpm/tpm_data.c | 14 +++++++-------
> + 1 file changed, 7 insertions(+), 7 deletions(-)
> +
> +diff --git a/tpm/tpm_data.c b/tpm/tpm_data.c
> +index a3a79ef..bebaf10 100644
> +--- a/tpm/tpm_data.c
> ++++ b/tpm/tpm_data.c
> +@@ -67,13 +67,13 @@ static void init_nv_storage(void)
> + static void init_timeouts(void)
> + {
> +   /* for the timeouts we use the PC platform defaults */
> +-  tpmData.permanent.data.tis_timeouts[0] = 750;
> +-  tpmData.permanent.data.tis_timeouts[1] = 2000;
> +-  tpmData.permanent.data.tis_timeouts[2] = 750;
> +-  tpmData.permanent.data.tis_timeouts[3] = 750;
> +-  tpmData.permanent.data.cmd_durations[0] = 1;
> +-  tpmData.permanent.data.cmd_durations[1] = 10;
> +-  tpmData.permanent.data.cmd_durations[2] = 1000;
> ++  tpmData.permanent.data.tis_timeouts[0] = 750000;
> ++  tpmData.permanent.data.tis_timeouts[1] = 2000000;
> ++  tpmData.permanent.data.tis_timeouts[2] = 750000;
> ++  tpmData.permanent.data.tis_timeouts[3] = 750000;
> ++  tpmData.permanent.data.cmd_durations[0] = 1000;
> ++  tpmData.permanent.data.cmd_durations[1] = 10000;
> ++  tpmData.permanent.data.cmd_durations[2] = 1000000;
> + }
> + 
> + void tpm_init_data(void)
> +-- 
> +2.30.2
> +
> 