Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1705387 > unrolled thread

[PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

Back to article view | Back to linux.kernel

Contents

  [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount Nayna Jain <nayna@linux.vnet.ibm.com> - 2017-08-07 13:50 +0200
    Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount Peter Huewe <peterhuewe@gmx.de> - 2017-08-07 14:00 +0200
      Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring  burstcount Nayna <nayna@linux.vnet.ibm.com> - 2017-08-07 16:30 +0200
        Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring  burstcount Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-09 00:00 +0200
      Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring  burstcount Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-08 21:20 +0200
        Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by  ignoring burstcount Ken Goldman <kgold@linux.vnet.ibm.com> - 2017-08-09 22:30 +0200
          Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount "Peter Huewe" <PeterHuewe@gmx.de> - 2017-08-09 22:50 +0200
            Re: Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount Ken Goldman <kgold@linux.vnet.ibm.com> - 2017-08-12 00:00 +0200
        Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by  ignoring burstcount Ken Goldman <kgold@linux.vnet.ibm.com> - 2017-08-09 22:30 +0200
          Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount "Peter Huewe" <PeterHuewe@gmx.de> - 2017-08-09 23:10 +0200
            Re: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-11 13:20 +0200
              Re: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-08-11 17:40 +0200
            Re: Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send()  performance by ignoring burstcount Ken Goldman <kgold@linux.vnet.ibm.com> - 2017-08-11 23:40 +0200
          Re: [Linux-ima-devel] [tpmdd-devel] [PATCH] tpm: improve tpm_tis  send() performance by ignoring burstcount Ken Goldman <kgold@linux.vnet.ibm.com> - 2017-08-11 23:50 +0200
    Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring  burstcount Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-08-08 21:10 +0200

#1705387 — [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

The TPM burstcount status indicates the number of bytes that can
be sent to the TPM without causing bus wait states.  Effectively,
it is the number of empty bytes in the command FIFO. Further,
some TPMs have a static burstcount, when the value remains zero
until the entire FIFO is empty.

This patch ignores burstcount, permitting wait states, and thus
writes the command as fast as the TPM can accept the bytes.
The performance of a 34 byte extend on a TPM 1.2 improved from
52 msec to 11 msec.

Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_tis_core.c | 45 ++---------------------------------------
 1 file changed, 2 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index b617b2eeb080..478cbc0f61c3 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 {
 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
-	int rc, status, burstcnt;
-	size_t count = 0;
-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
+	int rc, status;
 
 	status = tpm_tis_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 		}
 	}
 
-	while (count < len - 1) {
-		burstcnt = get_burstcount(chip);
-		if (burstcnt < 0) {
-			dev_err(&chip->dev, "Unable to read burstcount\n");
-			rc = burstcnt;
-			goto out_err;
-		}
-		burstcnt = min_t(int, burstcnt, len - count - 1);
-		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
-					 burstcnt, buf + count);
-		if (rc < 0)
-			goto out_err;
-
-		count += burstcnt;
-
-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-					&priv->int_queue, false) < 0) {
-			rc = -ETIME;
-			goto out_err;
-		}
-		status = tpm_tis_status(chip);
-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
-			rc = -EIO;
-			goto out_err;
-		}
-	}
-
-	/* write last byte */
-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
+	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, buf);
 	if (rc < 0)
 		goto out_err;
 
-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
-				&priv->int_queue, false) < 0) {
-		rc = -ETIME;
-		goto out_err;
-	}
-	status = tpm_tis_status(chip);
-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
-		rc = -EIO;
-		goto out_err;
-	}
-
 	return 0;
 
 out_err:
-- 
2.13.3

[toc] | [next] | [standalone]

#1705397

Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>:
>The TPM burstcount status indicates the number of bytes that can
>be sent to the TPM without causing bus wait states.  Effectively,
>it is the number of empty bytes in the command FIFO. Further,
>some TPMs have a static burstcount, when the value remains zero
>until the entire FIFO is empty.
>
>This patch ignores burstcount, permitting wait states, and thus
>writes the command as fast as the TPM can accept the bytes.
>The performance of a 34 byte extend on a TPM 1.2 improved from
>52 msec to 11 msec.
>
>Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
>conjunction with the TPM Device Driver work group.
>Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

Are you sure this is a good idea?
On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).

On which systems have you tested this?
Spi/Lpc? Architecture?

This might not be noticable for small transfers, but think about much larger transfers....

Imho: NACK from my side.

Thanks,
Peter

>---
>drivers/char/tpm/tpm_tis_core.c | 45
>++---------------------------------------
> 1 file changed, 2 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>index b617b2eeb080..478cbc0f61c3 100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>*buf, size_t count)
>static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>len)
> {
> 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>-	int rc, status, burstcnt;
>-	size_t count = 0;
>-	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>+	int rc, status;
> 
> 	status = tpm_tis_status(chip);
> 	if ((status & TPM_STS_COMMAND_READY) == 0) {
>@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
> 		}
> 	}
> 
>-	while (count < len - 1) {
>-		burstcnt = get_burstcount(chip);
>-		if (burstcnt < 0) {
>-			dev_err(&chip->dev, "Unable to read burstcount\n");
>-			rc = burstcnt;
>-			goto out_err;
>-		}
>-		burstcnt = min_t(int, burstcnt, len - count - 1);
>-		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>-					 burstcnt, buf + count);
>-		if (rc < 0)
>-			goto out_err;
>-
>-		count += burstcnt;
>-
>-		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-					&priv->int_queue, false) < 0) {
>-			rc = -ETIME;
>-			goto out_err;
>-		}
>-		status = tpm_tis_status(chip);
>-		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>-			rc = -EIO;
>-			goto out_err;
>-		}
>-	}
>-
>-	/* write last byte */
>-	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>+	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>buf);
> 	if (rc < 0)
> 		goto out_err;
> 
>-	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-				&priv->int_queue, false) < 0) {
>-		rc = -ETIME;
>-		goto out_err;
>-	}
>-	status = tpm_tis_status(chip);
>-	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>-		rc = -EIO;
>-		goto out_err;
>-	}
>-
> 	return 0;
> 
> out_err:

-- 
Sent from my mobile

[toc] | [prev] | [next] | [standalone]

#1705572 — Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On 08/07/2017 05:22 PM, Peter Huewe wrote:
>
>
> Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states.  Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>>
>> This patch ignores burstcount, permitting wait states, and thus
>> writes the command as fast as the TPM can accept the bytes.
>> The performance of a 34 byte extend on a TPM 1.2 improved from
>> 52 msec to 11 msec.
>>
>> Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>
> Are you sure this is a good idea?
> On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).

Thanks Peter for quick response.

I actually meant to post this patch as RFC. Sorry, missed that.
It was meant to be a starting place for the discussion related to 
burst_count.

>
> On which systems have you tested this?
> Spi/Lpc? Architecture?

Tested it with LPC on x86.

>
> This might not be noticable for small transfers, but think about much larger transfers....

I did the following testing:

* Ran a script with 1000 extends. This was to test continuous extends
which are generally in large numbers when IMA is enabled.

* Ran a command to ask TPM to hash big size file like 1MB. This was to
test the long command.

In both of the above cases, I didn't face any tpm specific errors.

Is there any test-script or test-cases which I can try to test the
scenario(stalling the bus, including keyboard/mouse) with the patch ?

Thanks & Regards,
    - Nayna


>
> Imho: NACK from my side.
>
> Thanks,
> Peter
>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 45
>> ++---------------------------------------
>> 1 file changed, 2 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> b/drivers/char/tpm/tpm_tis_core.c
>> index b617b2eeb080..478cbc0f61c3 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>> *buf, size_t count)
>> static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>> len)
>> {
>> 	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -	int rc, status, burstcnt;
>> -	size_t count = 0;
>> -	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>> +	int rc, status;
>>
>> 	status = tpm_tis_status(chip);
>> 	if ((status & TPM_STS_COMMAND_READY) == 0) {
>> @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>> *chip, u8 *buf, size_t len)
>> 		}
>> 	}
>>
>> -	while (count < len - 1) {
>> -		burstcnt = get_burstcount(chip);
>> -		if (burstcnt < 0) {
>> -			dev_err(&chip->dev, "Unable to read burstcount\n");
>> -			rc = burstcnt;
>> -			goto out_err;
>> -		}
>> -		burstcnt = min_t(int, burstcnt, len - count - 1);
>> -		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> -					 burstcnt, buf + count);
>> -		if (rc < 0)
>> -			goto out_err;
>> -
>> -		count += burstcnt;
>> -
>> -		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> -					&priv->int_queue, false) < 0) {
>> -			rc = -ETIME;
>> -			goto out_err;
>> -		}
>> -		status = tpm_tis_status(chip);
>> -		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>> -			rc = -EIO;
>> -			goto out_err;
>> -		}
>> -	}
>> -
>> -	/* write last byte */
>> -	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>> +	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>> buf);
>> 	if (rc < 0)
>> 		goto out_err;
>>
>> -	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>> -				&priv->int_queue, false) < 0) {
>> -		rc = -ETIME;
>> -		goto out_err;
>> -	}
>> -	status = tpm_tis_status(chip);
>> -	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>> -		rc = -EIO;
>> -		goto out_err;
>> -	}
>> -
>> 	return 0;
>>
>> out_err:
>

[toc] | [prev] | [next] | [standalone]

#1706899 — Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On Mon, Aug 07, 2017 at 07:55:49PM +0530, Nayna wrote:
> 
> 
> On 08/07/2017 05:22 PM, Peter Huewe wrote:
> > 
> > 
> > Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>:
> > > The TPM burstcount status indicates the number of bytes that can
> > > be sent to the TPM without causing bus wait states.  Effectively,
> > > it is the number of empty bytes in the command FIFO. Further,
> > > some TPMs have a static burstcount, when the value remains zero
> > > until the entire FIFO is empty.
> > > 
> > > This patch ignores burstcount, permitting wait states, and thus
> > > writes the command as fast as the TPM can accept the bytes.
> > > The performance of a 34 byte extend on a TPM 1.2 improved from
> > > 52 msec to 11 msec.
> > > 
> > > Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
> > > conjunction with the TPM Device Driver work group.
> > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > 
> > Are you sure this is a good idea?
> > On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).
> 
> Thanks Peter for quick response.
> 
> I actually meant to post this patch as RFC. Sorry, missed that.
> It was meant to be a starting place for the discussion related to
> burst_count.
> 
> > 
> > On which systems have you tested this?
> > Spi/Lpc? Architecture?
> 
> Tested it with LPC on x86.
> 
> > 
> > This might not be noticable for small transfers, but think about much larger transfers....
> 
> I did the following testing:
> 
> * Ran a script with 1000 extends. This was to test continuous extends
> which are generally in large numbers when IMA is enabled.
> 
> * Ran a command to ask TPM to hash big size file like 1MB. This was to
> test the long command.
> 
> In both of the above cases, I didn't face any tpm specific errors.
> 
> Is there any test-script or test-cases which I can try to test the
> scenario(stalling the bus, including keyboard/mouse) with the patch ?
> 
> Thanks & Regards,
>    - Nayna

My stand here is that if you want to such patch included there should
be outstanding evidence that:

- Burst count could be always safely ignored.
- There's no hardware platform including TPM that assumes that driver
  takes the burst count into account.

My main concern is stability and only if the stability is not at risk
we can consider this. There's no test script to take care all of this.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1706839 — Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
> 
> 
> Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>:
> >The TPM burstcount status indicates the number of bytes that can
> >be sent to the TPM without causing bus wait states.  Effectively,
> >it is the number of empty bytes in the command FIFO. Further,
> >some TPMs have a static burstcount, when the value remains zero
> >until the entire FIFO is empty.
> >
> >This patch ignores burstcount, permitting wait states, and thus
> >writes the command as fast as the TPM can accept the bytes.
> >The performance of a 34 byte extend on a TPM 1.2 improved from
> >52 msec to 11 msec.
> >
> >Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
> >conjunction with the TPM Device Driver work group.
> >Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> >Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> Are you sure this is a good idea?
> On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc).
> 
> On which systems have you tested this?
> Spi/Lpc? Architecture?
> 
> This might not be noticable for small transfers, but think about much larger transfers....
> 
> Imho: NACK from my side.
> 
> Thanks,
> Peter

Thanks Peter, a great insight. TPM could share the bus with other
devices. Even if this optimizes the performace for TPM it might cause
performance issues elsewhere.

One more viewpoint: TCG must added the burst count for a reason (might
be very well related what Peter said). Is ignoring it something that TCG
recommends? Not following standard exactly in the driver code sometimes
makes sense on *small details* but I would not say that this a small
detail...

After these viewpoints definitive NACK from my side too...

/Jarkko

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1707843 — Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On 8/8/2017 3:11 PM, Jarkko Sakkinen wrote:
> On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
>> Imho: NACK from my side.
> After these viewpoints definitive NACK from my side too...


I responded to the thread comments separately.  However, assuming NACK 
is the final response, I have a question.

The problem is the 5 msec sleep between polls of burst count.  In the 
case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of 
these sleeps.

Would another solution be to reduce the burst count poll and sleep to, 
e.g., 100 usec or even 10 usec?  This would probably help greatly, but 
still not incur the wait states that triggered the NACK.

My worry is that the scheduler would not be able to context switch that 
fast, and so we wouldn't actually see usec speed polling.

Can a kernel expert offer an opinion?

[toc] | [prev] | [next] | [standalone]

#1707895 — Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

Hi Ken,

(speaking on behalf of myself here, not my employer :) )
> On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
>> Imho: NACK from my side.
> After these viewpoints definitive NACK from my side too...


> I responded to the thread comments separately. However, assuming NACK
> is the final response, I have a question.

Nothing is ever final :)

> The problem is the 5 msec sleep between polls of burst count. In the
> case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of
> these sleeps.

Yes that's bad, especially with current msleep(5) is actually msleep(20).
However, please also keep in mind SPI tpms show a much higher burst count value,  (255)
our I2C TPM SLB9645 even shows something in the range of 1k. :)


> Would another solution be to reduce the burst count poll and sleep to,
> e.g., 100 usec or even 10 usec? This would probably help greatly, but
> till not incur the wait states that triggered the NACK.

Imho the 5ms were an arbitrary chosen value for old old tpms.
Back then also only msleep was available which was msleep(20) anyway.


> My worry is that the scheduler would not be able to context switch that
> fast, and so we wouldn't actually see usec speed polling.
> Can a kernel expert offer an opinion?
If you use sleep it's not guaranteed that you wakeup after exactly xx specified amount of time.
Just that you sleep at least xx amount of time. Otherwise you would have to do delay/busywaiting.

https://www.kernel.org/doc/Documentation/timers/timers-howto.txt


Imho the best option is to
figure out whether any vendor can determine the "FIFO flush time" i.e. how much time does it take to empty the fifo and fillup the burstcount and use this on the lower bound of an usleep range.
I don't think tpms are in the range of < 10 us...

@Ken: Maybe can you check in DDWG?

What we often see/saw during some performance optimization tests is something like 
burstcount 7, burstcount 1, burstcount 7, burstcount 1...
which is the oposite of what you want to achieve.

You'd like to have something like
burstcount 8, burstcount 8, burstcount 8....

Unfortunately TPMS don't report their max burstcount (afaik),
otherwise it would be easy to write a calibration routine for the poll times.


Thanks,
Peter

[toc] | [prev] | [next] | [standalone]

#1709992 — Re: Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On 8/9/2017 4:43 PM, Peter Huewe wrote:
> 
> Yes that's bad, especially with current msleep(5) is actually
> msleep(20). However, please also keep in mind SPI tpms show a much
> higher burst count value,  (255) our I2C TPM SLB9645 even shows
> something in the range of 1k. :)

One of our platforms has a TPM 1.2 with an 8 byte FIFO and a static 
burst count.  This is where we're debugging.

>> Would another solution be to reduce the burst count poll and sleep
>> to, e.g., 100 usec or even 10 usec? This would probably help
>> greatly, but till not incur the wait states that triggered the
>> NACK.
>
> If you use sleep it's not guaranteed that you wakeup after exactly xx
> specified amount of time. Just that you sleep at least xx amount of
> time. Otherwise you would have to do delay/busywaiting.

Understood.  However, if the DD maintainers will not accept ignoring 
burstCount, would a short sleep (e.g., 10 usec) be acceptable.

If so, we can benchmark it.

> Imho the best option is to figure out whether any vendor can
> determine the "FIFO flush time" i.e. how much time does it take to
> empty the fifo and fillup the burstcount and use this on the lower
> bound of an usleep range. I don't think tpms are in the range of < 10
> us...
> 
> @Ken: Maybe can you check in DDWG?

I asked this week.

Nuvoton, ST Micro, and Infineon confirmed that the TPM empties a byte 
from the FIFO in under 1 usec.  So, even with a static burst count, the 
entire FIFO would empty in under 10 usec.

[toc] | [prev] | [next] | [standalone]

#1707847 — Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On 8/8/2017 3:11 PM, Jarkko Sakkinen wrote:
 > On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:

 >> Are you sure this is a good idea?
 >> On lpc systems this more or less stalls the bus, including 
keyboard/mouse (if connected via superio lpc).
 >>
 >> On which systems have you tested this?
 >> Spi/Lpc? Architecture?
 >>
 >> This might not be noticable for small transfers, but think about 
much larger transfers....
 >>
 >> Imho: NACK from my side.
 >>
 >> Thanks,
 >> Peter
 >
 > Thanks Peter, a great insight. TPM could share the bus with other
 > devices. Even if this optimizes the performance for TPM it might cause
 > performance issues elsewhere.

Does anyone know of platforms where this occurs?

I suspect (but not sure) that the days of SuperIO connecting floppy 
drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and 
such legacy systems will not have a TPM. Would SuperIO even support the 
special TPM LPC bus cycles?

Even then, the wait states of a mhz speed LPC are likely to be usec,
not noticeable for even a mouse.

Is this a reasonable assumption?

If so, to we affect TPM performance to the point where it's unusable to 
help a case that is unlikely to appear in current platforms?

 >
 > One more viewpoint: TCG must added the burst count for a reason (might
 > be very well related what Peter said). Is ignoring it something that TCG
 > recommends? Not following standard exactly in the driver code sometimes
 > makes sense on *small details* but I would not say that this a small
 > detail...

I checked with the TCG's device driver work group (DDWG).  Both the spec 
editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that 
ignoring burst count may incur wait states but nothing more.  Operations 
will still be successful.

[toc] | [prev] | [next] | [standalone]

#1707957 — Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

Hi Ken,
(again speaking only on my behalf, not my employer)

> Does anyone know of platforms where this occurs?
> I suspect (but not sure) that the days of SuperIO connecting floppy
> drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and
> such legacy systems will not have a TPM. Would SuperIO even support the
> special TPM LPC bus cycles?

Since we are the linux kernel, we do have to care for legacy devices.
And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon.

And heck, we even have support for 1.1b TPM devices....


>> One more viewpoint: TCG must added the burst count for a reason (might
>> be very well related what Peter said). Is ignoring it something that TCG
>> recommends? Not following standard exactly in the driver code sometimes
>> makes sense on *small details* but I would not say that this a small
>> detail...

> I checked with the TCG's device driver work group (DDWG). Both the spec
> editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that
> ignoring burst count may incur wait states but nothing more. Operations
> will still be successful.

Interesting - let me check with Georg tomorrow.
Unfortunately I do not have access to my tcg mails from home (since I'm not working :), 
but did you _explicitly_ talk about LPC and the system?
I'm sure the TPM does not care about the waitstates...

If my memory does not betray me, 
it is actually possible to "freeze up" a system completly by flooding the lpc bus.
Let me double check tomorrow...


In anycase - I really would like to see a much more performant tpm subsystem - 
however it will be quite an effort with a lot of legacy testing.
(which I unfortunately cannot spend on my private time ... and also of course lacking test systems).

Thanks,
Peter

[toc] | [prev] | [next] | [standalone]

#1709467 — Re: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote:
> Hi Ken,
> (again speaking only on my behalf, not my employer)
> 
> > Does anyone know of platforms where this occurs?
> > I suspect (but not sure) that the days of SuperIO connecting floppy
> > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and
> > such legacy systems will not have a TPM. Would SuperIO even support the
> > special TPM LPC bus cycles?
> 
> Since we are the linux kernel, we do have to care for legacy devices.
> And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon.
> 
> And heck, we even have support for 1.1b TPM devices....
> 
> 
> >> One more viewpoint: TCG must added the burst count for a reason (might
> >> be very well related what Peter said). Is ignoring it something that TCG
> >> recommends? Not following standard exactly in the driver code sometimes
> >> makes sense on *small details* but I would not say that this a small
> >> detail...
> 
> > I checked with the TCG's device driver work group (DDWG). Both the spec
> > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that
> > ignoring burst count may incur wait states but nothing more. Operations
> > will still be successful.
> 
> Interesting - let me check with Georg tomorrow.
> Unfortunately I do not have access to my tcg mails from home (since I'm not working :), 
> but did you _explicitly_ talk about LPC and the system?
> I'm sure the TPM does not care about the waitstates...
> 
> If my memory does not betray me, 
> it is actually possible to "freeze up" a system completly by flooding the lpc bus.
> Let me double check tomorrow...
> 
> 
> In anycase - I really would like to see a much more performant tpm subsystem - 
> however it will be quite an effort with a lot of legacy testing.
> (which I unfortunately cannot spend on my private time ... and also of course lacking test systems).
> 
> Thanks,
> Peter

I would like to see tpm_msleep() wrapper to replace current msleep()
usage across the subsystem before considering this. I.e. wrapper that
internally uses usleep_range(). This way we can mechanically convert
everything to a more low latency option.

This should have been done already for patch that Mini and Nayna
provided instead of open coding stuff.

That change is something that can be applied right now. On the other
hand, this is a very controversial change.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1709751 — Re: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On Fri, 2017-08-11 at 14:14 +0300, Jarkko Sakkinen wrote:
> On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote:
> > Hi Ken,
> > (again speaking only on my behalf, not my employer)
> > 
> > > Does anyone know of platforms where this occurs?
> > > I suspect (but not sure) that the days of SuperIO connecting floppy
> > > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and
> > > such legacy systems will not have a TPM. Would SuperIO even support the
> > > special TPM LPC bus cycles?
> > 
> > Since we are the linux kernel, we do have to care for legacy devices.
> > And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon.
> > 
> > And heck, we even have support for 1.1b TPM devices....
> > 
> > 
> > >> One more viewpoint: TCG must added the burst count for a reason (might
> > >> be very well related what Peter said). Is ignoring it something that TCG
> > >> recommends? Not following standard exactly in the driver code sometimes
> > >> makes sense on *small details* but I would not say that this a small
> > >> detail...
> > 
> > > I checked with the TCG's device driver work group (DDWG). Both the spec
> > > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that
> > > ignoring burst count may incur wait states but nothing more. Operations
> > > will still be successful.
> > 
> > Interesting - let me check with Georg tomorrow.
> > Unfortunately I do not have access to my tcg mails from home (since I'm not working :), 
> > but did you _explicitly_ talk about LPC and the system?
> > I'm sure the TPM does not care about the waitstates...
> > 
> > If my memory does not betray me, 
> > it is actually possible to "freeze up" a system completly by flooding the lpc bus.
> > Let me double check tomorrow...
> > 
> > 
> > In anycase - I really would like to see a much more performant tpm subsystem - 
> > however it will be quite an effort with a lot of legacy testing.
> > (which I unfortunately cannot spend on my private time ... and also of course lacking test systems).
> > 
> > Thanks,
> > Peter
> 
> I would like to see tpm_msleep() wrapper to replace current msleep()
> usage across the subsystem before considering this. I.e. wrapper that
> internally uses usleep_range(). This way we can mechanically convert
> everything to a more low latency option.

Fine.  I assume you meant tpm_sleep(), not tpm_msleep().
   
> This should have been done already for patch that Mini and Nayna
> provided instead of open coding stuff.

At that time, we had no idea what caused the major change in TPM
performance.  We only knew that the change occurred somewhere between
linux-4.7 and linux-4.8.  Even after figuring out it was the change to
msleep(), we were hoping that msleep() would be fixed.  So your
comment, that we should have done it differently back then, is
unwarranted.

> That change is something that can be applied right now. On the other
> hand, this is a very controversial change.

Since the main concern about this change is breaking old systems that
might potentially have other peripherals hanging off the LPC bus, can
we define a new Kconfig option, with the default as 'N'?

Mimi

[toc] | [prev] | [next] | [standalone]

#1709983 — Re: Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On 8/9/2017 5:00 PM, Peter Huewe wrote:
> 
> Since we are the linux kernel, we do have to care for legacy
> devices. And a system with LPC, PS2Mouse on SuperIO and a TPM are not
> that uncommon.
> 
> And heck, we even have support for 1.1b TPM devices....

Understood.  However, remember that SuperIO is a 1980's device that
predates the TPM.  Since the TPM requires special LPC bus cycles, it's
even less likely that an old chipset has an attached TPM.

> Interesting - let me check with Georg tomorrow. Unfortunately I do
> not have access to my tcg mails from home (since I'm not working :), 
> but did you _explicitly_ talk about LPC and the system? I'm sure the
> TPM does not care about the waitstates...

Yes, Infineon was one of the 3 TPM vendors who confirmed this.

[toc] | [prev] | [next] | [standalone]

#1709985 — Re: [Linux-ima-devel] [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

Following up on this thread based on this week's TCG call ...

1 - burstCount can safely be ignored on writes.  This is explicit in
most places in the TCG spec.  In places where it is not explicit, it was 
simply an editorial omission.  We are going through the spec and adding 
"without incurring wait states."

TCG is willing to publish an errata if that makes developers more 
comfortable.

2 - These are multi-mhz buses.  The TPM vendors conformed that wait 
states, even if incurred, will be sub-usec.  I.e., less that a microsecond.

Essentially, the DD is loading the FIFO, and the TPM is unloading the 
FIFO at processor speeds.

Thus, even if one were worried about an odd system new enough to have a 
TPM, but old enough to have an LPC attached printer, keyboard, mouse or 
floppy, the delay in printing or typing will be insignificant.

3 - I asked several platform vendors with long TCG experience, and they 
said that they know of no motherboards that share the LPC bus with a TPM 
plus another device.

[toc] | [prev] | [next] | [standalone]

#1706831 — Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

On Mon, Aug 07, 2017 at 07:46:32AM -0400, Nayna Jain wrote:
> The TPM burstcount status indicates the number of bytes that can
> be sent to the TPM without causing bus wait states.  Effectively,
> it is the number of empty bytes in the command FIFO. Further,
> some TPMs have a static burstcount, when the value remains zero
> until the entire FIFO is empty.
> 
> This patch ignores burstcount, permitting wait states, and thus
> writes the command as fast as the TPM can accept the bytes.
> The performance of a 34 byte extend on a TPM 1.2 improved from
> 52 msec to 11 msec.
> 
> Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in
> conjunction with the TPM Device Driver work group.
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 45 ++---------------------------------------
>  1 file changed, 2 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index b617b2eeb080..478cbc0f61c3 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int rc, status, burstcnt;
> -	size_t count = 0;
> -	bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> +	int rc, status;

As you anyway edit that line you could turn this as:

int rc;
int status;

>  	status = tpm_tis_status(chip);
>  	if ((status & TPM_STS_COMMAND_READY) == 0) {
> @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>  		}
>  	}
>  
> -	while (count < len - 1) {
> -		burstcnt = get_burstcount(chip);
> -		if (burstcnt < 0) {
> -			dev_err(&chip->dev, "Unable to read burstcount\n");
> -			rc = burstcnt;
> -			goto out_err;
> -		}
> -		burstcnt = min_t(int, burstcnt, len - count - 1);
> -		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
> -					 burstcnt, buf + count);
> -		if (rc < 0)
> -			goto out_err;
> -
> -		count += burstcnt;
> -
> -		if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> -					&priv->int_queue, false) < 0) {
> -			rc = -ETIME;
> -			goto out_err;
> -		}
> -		status = tpm_tis_status(chip);
> -		if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
> -			rc = -EIO;
> -			goto out_err;
> -		}
> -	}
> -
> -	/* write last byte */
> -	rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
> +	rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, buf);
>  	if (rc < 0)
>  		goto out_err;
>  
> -	if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
> -				&priv->int_queue, false) < 0) {
> -		rc = -ETIME;
> -		goto out_err;
> -	}
> -	status = tpm_tis_status(chip);
> -	if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
> -		rc = -EIO;
> -		goto out_err;
> -	}
> -
>  	return 0;
>  
>  out_err:
> -- 
> 2.13.3

Here's an open question that I do not know the answer: can ignoring
burst count cause hardware issues in the field? The commit message
does not sort it out so I don't really feel safe merging this commit.

/Jarkko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web