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 <[email protected]> in
conjunction with the TPM Device Driver work group.
Signed-off-by: Nayna Jain <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
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
Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <[email protected]>:
>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 <[email protected]> in
>conjunction with the TPM Device Driver work group.
>Signed-off-by: Nayna Jain <[email protected]>
>Acked-by: Mimi Zohar <[email protected]>
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
On 08/07/2017 05:22 PM, Peter Huewe wrote:
>
>
> Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <[email protected]>:
>> 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 <[email protected]> in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain <[email protected]>
>> Acked-by: Mimi Zohar <[email protected]>
>
> 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:
>
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 <[email protected]> in
> conjunction with the TPM Device Driver work group.
> Signed-off-by: Nayna Jain <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>
> ---
> 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
On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
>
>
> Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <[email protected]>:
> >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 <[email protected]> in
> >conjunction with the TPM Device Driver work group.
> >Signed-off-by: Nayna Jain <[email protected]>
> >Acked-by: Mimi Zohar <[email protected]>
>
> 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
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 <[email protected]>:
> > > 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 <[email protected]> in
> > > conjunction with the TPM Device Driver work group.
> > > Signed-off-by: Nayna Jain <[email protected]>
> > > Acked-by: Mimi Zohar <[email protected]>
> >
> > 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
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?
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.
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
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
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
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
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.
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.
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.
Hello,
On Fri, 11 Aug 2017 17:32:12 -0400
Ken Goldman <[email protected]> wrote:
> 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.
Where are the PS/2 ports attached today?
About 500 out of 700 mainboards sold today has a PS/2 port which is
probably due to prevalence of legacy devices and usbhid limitations.
Similarily many boards have serial and parallel hardware ports.
In all diagrams detailed enough to show these ports I have seen them
attached to the LPC bus.
Thanks
Michal
On Fri, Aug 11, 2017 at 11:30:19AM -0400, Mimi Zohar wrote:
> 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().
I think it would sense to have a function that takes msecs because msecs
are mostly used everywhere in the subsystem. This way we don't have to
change any of the existing constants.
> > 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.
I wasn't trying to point the blame to you at all. I didn't bring this to
table back then myself. I agree what you are saying.
I was mainly trying to explain why I think it should be done this way
now while I didn't suggest it back then :-)
> > 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
I guess that could make sense but I would like to hear feedback first.
/Jarkko
On Mon, Aug 14, 2017 at 01:51:30PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 11, 2017 at 11:30:19AM -0400, Mimi Zohar wrote:
> > 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().
>
> I think it would sense to have a function that takes msecs because msecs
> are mostly used everywhere in the subsystem. This way we don't have to
> change any of the existing constants.
>
> > > 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.
>
> I wasn't trying to point the blame to you at all. I didn't bring this to
> table back then myself. I agree what you are saying.
>
> I was mainly trying to explain why I think it should be done this way
> now while I didn't suggest it back then :-)
>
> > > 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
>
> I guess that could make sense but I would like to hear feedback first.
>
> /Jarkko
And I'm worried would that it'd be left for many years to come as an
option. I do not have any metrics what portion of hardware in the field
would break if this is turned on.
It would slow down kernel testing as I would have to run tests for the
driver with that option turned on and off because it is a major shift
from how driver functions. And I have zero idea how long I would go on
doing this.
One maybe a little bit better option would be to have a sysfs attribute
for this functionality (disable_burst_count). What do you think about
that?
/Jarkko
On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote:
> > > > 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().
> >
> > I think it would sense to have a function that takes msecs because msecs
> > are mostly used everywhere in the subsystem. This way we don't have to
> > change any of the existing constants.
For now converting from msleep() to tpm_msleep() will be straight
forward. Internally we would just use usleep_range().
Going forward, my concern is that even 1 msec might be too long for
some of these sleeps.
Mimi
On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote:
> > > 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
> >
> > I guess that could make sense but I would like to hear feedback first.
> >
> > /Jarkko
>
> And I'm worried would that it'd be left for many years to come as an
> option. I do not have any metrics what portion of hardware in the field
> would break if this is turned on.
>
> It would slow down kernel testing as I would have to run tests for the
> driver with that option turned on and off because it is a major shift
> from how driver functions. And I have zero idea how long I would go on
> doing this.
>
> One maybe a little bit better option would be to have a sysfs attribute
> for this functionality (disable_burst_count). What do you think about
> that?
That works! So we'll define a module_param named disable_burst_count,
which can be specified on the boot command line.
Mimi
On Mon, Aug 14, 2017 at 08:03:05AM -0400, Mimi Zohar wrote:
> On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote:
>
> > > > > 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().
> > >
> > > I think it would sense to have a function that takes msecs because msecs
> > > are mostly used everywhere in the subsystem. This way we don't have to
> > > change any of the existing constants.
>
> For now converting from msleep() to tpm_msleep() will be straight
> forward. ?Internally we would just use usleep_range().
>
> Going forward, my concern is that even 1 msec might be too long for
> some of these sleeps.
>
> Mimi
We can revisit this. I would take the simple route right now.
/Jarkko
On Mon, Aug 14, 2017 at 08:12:53AM -0400, Mimi Zohar wrote:
> On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote:
>
> > > > 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
> > >
> > > I guess that could make sense but I would like to hear feedback first.
> > >
> > > /Jarkko
> >
> > And I'm worried would that it'd be left for many years to come as an
> > option. I do not have any metrics what portion of hardware in the field
> > would break if this is turned on.
> >
> > It would slow down kernel testing as I would have to run tests for the
> > driver with that option turned on and off because it is a major shift
> > from how driver functions. And I have zero idea how long I would go on
> > doing this.
> >
> > One maybe a little bit better option would be to have a sysfs attribute
> > for this functionality (disable_burst_count). What do you think about
> > that?
>
> That works! So we'll define a module_param named disable_burst_count,
> which can be specified on the boot command line.
>
> Mimi
That would work for me.
/Jarkko
On 8/13/2017 7:53 PM, msuchanek wrote:
> About 500 out of 700 mainboards sold today has a PS/2 port which is
> probably due to prevalence of legacy devices and usbhid limitations.
>
> Similarily many boards have serial and parallel hardware ports.
>
> In all diagrams detailed enough to show these ports I have seen them
> attached to the LPC bus.
Do these boards have a TPM? Remember that the TPM requires special LPC
bus cycles.
Even if so, the TPM LPC bus wait states are less than a usec. My
thought is that it's unlikely that any device (serial port, mouse,
keyboard, printer) will be adversely affected.
On Tue, 15 Aug 2017 18:02:57 -0400
Ken Goldman <[email protected]> wrote:
> On 8/13/2017 7:53 PM, msuchanek wrote:
> > About 500 out of 700 mainboards sold today has a PS/2 port which is
> > probably due to prevalence of legacy devices and usbhid limitations.
> >
> > Similarily many boards have serial and parallel hardware ports.
> >
> > In all diagrams detailed enough to show these ports I have seen them
> > attached to the LPC bus.
>
> Do these boards have a TPM? Remember that the TPM requires special
> LPC bus cycles.
Out of nearly 700 boards over 500 have PS/2 connector and over 400
have TPM slot (which is subset of the PS/2 enabled boards). Some more
possibly have on-board TPM chip.
>
> Even if so, the TPM LPC bus wait states are less than a usec. My
> thought is that it's unlikely that any device (serial port, mouse,
> keyboard, printer) will be adversely affected.
Yes, in theory this is negligible. So unless there is a possibility
these wait states chain or the device otherwise takes over the bus for
extended period of time this should be fine.
Thanks
Michal
On 8/14/2017 6:10 AM, Jarkko Sakkinen wrote:
>> 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.
>
> Does this break anything lets say in a decade time frame? If it does, I
> will not even consider this. Can you give a definitive answer for that?
My attempt at predicting the future ...
1 - Clock speed should get faster, not slower, so the 1 usec wait state
should get shorter.
2 - TPM buffers should get larger. I'm already reading of 256 byte
buffers. So there should be fewer wait states.
3 - There is a trend toward using the CRB, making the FIFO less applicable.
4 - There is a trend away from LPC connected serial port devices,
printers, and PS/2 mouse and keyboard, and toward USB connected devices.
I assume this will continue, making the already insignificant wait
states irrelevant.