[PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

Because all the existing hardware have HID MSFT0101 we end up always
setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
Even if ACPI start is used, the driver will always issue also CRB start.

This commit makes the invocation of CRB start unconditional.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 65040d7..5928ec8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -72,7 +72,6 @@ enum crb_status {
 
 enum crb_flags {
 	CRB_FL_ACPI_START	= BIT(0),
-	CRB_FL_CRB_START	= BIT(1),
 };
 
 struct crb_priv {
@@ -226,8 +225,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	/* Make sure that cmd is populated before issuing start. */
 	wmb();
 
-	if (priv->flags & CRB_FL_CRB_START)
-		iowrite32(CRB_START_INVOKE, &priv->cca->start);
+
+	/* At least some of the 4th Gen Core CPUs that report only needing ACPI
+	 * start require also CRB start so we always set it just in case.
+	 */
+	iowrite32(CRB_START_INVOKE, &priv->cca->start);
 
 	if (priv->flags & CRB_FL_ACPI_START)
 		rc = crb_do_acpi_start(chip);
@@ -407,14 +409,6 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
-	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
-	 * report only ACPI start but in practice seems to require both
-	 * ACPI start and CRB start.
-	 */
-	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
-	    !strcmp(acpi_device_hid(device), "MSFT0101"))
-		priv->flags |= CRB_FL_CRB_START;
-
 	if (sm == ACPI_TPM2_START_METHOD ||
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
-- 
2.9.3

Re: [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Mon, Oct 17, 2016 at 11:42:24PM +0300, Jarkko Sakkinen wrote:
> Because all the existing hardware have HID MSFT0101 we end up always
> setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
> Even if ACPI start is used, the driver will always issue also CRB start.
> 
> This commit makes the invocation of CRB start unconditional.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)

I will include this to the next version of locale series if this is
accepted. Just wanted to make sure that this is OK before I make the
next version of the series.

/Jarkko

> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 65040d7..5928ec8 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -72,7 +72,6 @@ enum crb_status {
>  
>  enum crb_flags {
>  	CRB_FL_ACPI_START	= BIT(0),
> -	CRB_FL_CRB_START	= BIT(1),
>  };
>  
>  struct crb_priv {
> @@ -226,8 +225,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	/* Make sure that cmd is populated before issuing start. */
>  	wmb();
>  
> -	if (priv->flags & CRB_FL_CRB_START)
> -		iowrite32(CRB_START_INVOKE, &priv->cca->start);
> +
> +	/* At least some of the 4th Gen Core CPUs that report only needing ACPI
> +	 * start require also CRB start so we always set it just in case.
> +	 */
> +	iowrite32(CRB_START_INVOKE, &priv->cca->start);
>  
>  	if (priv->flags & CRB_FL_ACPI_START)
>  		rc = crb_do_acpi_start(chip);
> @@ -407,14 +409,6 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
> -	 * report only ACPI start but in practice seems to require both
> -	 * ACPI start and CRB start.
> -	 */
> -	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
> -	    !strcmp(acpi_device_hid(device), "MSFT0101"))
> -		priv->flags |= CRB_FL_CRB_START;
> -
>  	if (sm == ACPI_TPM2_START_METHOD ||
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
> -- 
> 2.9.3
> 

RE: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Winkler, Tomas]

> 
> On Mon, Oct 17, 2016 at 11:42:24PM +0300, Jarkko Sakkinen wrote:
> > Because all the existing hardware have HID MSFT0101 we end up always
> > setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
> > Even if ACPI start is used, the driver will always issue also CRB start.

Do you have some more historical data about this fix, I was wondering about this quirk before, when restructuring the start method parsing. 
The description is ' in practice seems to require both'  sounds not certain about the root cause of this.


> > This commit makes the invocation of CRB start unconditional.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 16 +++++-----------
> >  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> I will include this to the next version of locale series if this is accepted. Just
> wanted to make sure that this is OK before I make the next version of the
> series.
> 
> /Jarkko
> 
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 65040d7..5928ec8 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -72,7 +72,6 @@ enum crb_status {
> >
> >  enum crb_flags {
> >  	CRB_FL_ACPI_START	= BIT(0),
> > -	CRB_FL_CRB_START	= BIT(1),
> >  };
> >
> >  struct crb_priv {
> > @@ -226,8 +225,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf,
> size_t len)
> >  	/* Make sure that cmd is populated before issuing start. */
> >  	wmb();
> >
> > -	if (priv->flags & CRB_FL_CRB_START)
> > -		iowrite32(CRB_START_INVOKE, &priv->cca->start);
> > +
> > +	/* At least some of the 4th Gen Core CPUs that report only needing
> ACPI
> > +	 * start require also CRB start so we always set it just in case.
> > +	 */
> > +	iowrite32(CRB_START_INVOKE, &priv->cca->start);
> >
> >  	if (priv->flags & CRB_FL_ACPI_START)
> >  		rc = crb_do_acpi_start(chip);
> > @@ -407,14 +409,6 @@ static int crb_acpi_add(struct acpi_device *device)
> >  	if (!priv)
> >  		return -ENOMEM;
> >
> > -	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
> > -	 * report only ACPI start but in practice seems to require both
> > -	 * ACPI start and CRB start.
> > -	 */
> > -	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm ==
> ACPI_TPM2_MEMORY_MAPPED ||
> > -	    !strcmp(acpi_device_hid(device), "MSFT0101"))
> > -		priv->flags |= CRB_FL_CRB_START;
> > -
> >  	if (sm == ACPI_TPM2_START_METHOD ||
> >  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> >  		priv->flags |= CRB_FL_ACPI_START;
> > --
> > 2.9.3
> >
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Wed, Oct 19, 2016 at 10:28:29AM +0000, Winkler, Tomas wrote:
> 
> 
> > 
> > On Mon, Oct 17, 2016 at 11:42:24PM +0300, Jarkko Sakkinen wrote:
> > > Because all the existing hardware have HID MSFT0101 we end up always
> > > setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
> > > Even if ACPI start is used, the driver will always issue also CRB start.
> 
> Do you have some more historical data about this fix, I was wondering
> about this quirk before, when restructuring the start method parsing.
> The description is ' in practice seems to require both'  sounds not
> certain about the root cause of this.

I have a 4th Gen Core NUC where I experienced this issue. It reported
requiring only ACPI start but actually required ACPI + CRB start. The
comment could have been better.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Wed, Oct 19, 2016 at 07:09:28PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 19, 2016 at 10:28:29AM +0000, Winkler, Tomas wrote:
> > 
> > 
> > > 
> > > On Mon, Oct 17, 2016 at 11:42:24PM +0300, Jarkko Sakkinen wrote:
> > > > Because all the existing hardware have HID MSFT0101 we end up always
> > > > setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
> > > > Even if ACPI start is used, the driver will always issue also CRB start.
> > 
> > Do you have some more historical data about this fix, I was wondering
> > about this quirk before, when restructuring the start method parsing.
> > The description is ' in practice seems to require both'  sounds not
> > certain about the root cause of this.
> 
> I have a 4th Gen Core NUC where I experienced this issue. It reported
> requiring only ACPI start but actually required ACPI + CRB start. The
> comment could have been better.

With the latest master branch if I remove the workaround:

[  395.161155] tpm_crb: module verification failed: signature and/or required key missing - tainting kernel
[  480.087136] tpm tpm0: A TPM error (323) occurred continue selftest
[  480.087141] tpm tpm0: TPM self test failed

jsakkine at jsakkine-tpm1 in ~/devel/linux-tpmdd (master●●) 
$ git --no-pager diff
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 65040d7..5b186e0 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -407,14 +407,6 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
-	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
-	 * report only ACPI start but in practice seems to require both
-	 * ACPI start and CRB start.
-	 */
-	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
-	    !strcmp(acpi_device_hid(device), "MSFT0101"))
-		priv->flags |= CRB_FL_CRB_START;
-
 	if (sm == ACPI_TPM2_START_METHOD ||
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;

jsakkine at jsakkine-tpm1 in ~/devel/linux-tpmdd (master●●) 
$ sudo dmidecode -t bios -q
BIOS Information
	Vendor: Intel Corp.
	Version: WYLPT10H.86A.0033.2014.1201.0940
	Release Date: 12/01/2014
	Address: 0xF0000
	Runtime Size: 64 kB
	ROM Size: 6656 kB
	Characteristics:
		PCI is supported
		BIOS is upgradeable
		BIOS shadowing is allowed
		Boot from CD is supported
		Selectable boot is supported
		BIOS ROM is socketed
		EDD is supported
		5.25"/1.2 MB floppy services are supported (int 13h)
		3.5"/720 kB floppy services are supported (int 13h)
		3.5"/2.88 MB floppy services are supported (int 13h)
		Print screen service is supported (int 5h)
		Serial services are supported (int 14h)
		Printer services are supported (int 17h)
		ACPI is supported
		USB legacy is supported
		BIOS boot specification is supported
		Targeted content distribution is supported
		UEFI is supported
	BIOS Revision: 4.6

BIOS Language Information
	Language Description Format: Long
	Installable Languages: 1
		en|US|iso8859-1
	Currently Installed Language: en|US|iso8859-1

jsakkine at jsakkine-tpm1 in ~/tmp (master) 
$ cat ~/tmp/tpm2.dsl 
/*
 * Intel ACPI Component Architecture
 * AML Disassembler version 20140214-64 [Mar 29 2014]
 * Copyright (c) 2000 - 2014 Intel Corporation
 * 
 * Disassembly of tpm2.dat, Wed Jun  1 16:26:49 2016
 *
 * ACPI Data Table [TPM2]
 *
 * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
 */

[000h 0000   4]                    Signature : "TPM2"    [Trusted Platform Module hardware interface table]
[004h 0004   4]                 Table Length : 00000034
[008h 0008   1]                     Revision : 03
[009h 0009   1]                     Checksum : 31
[00Ah 0010   6]                       Oem ID : "INTEL "
[010h 0016   8]                 Oem Table ID : "D34010WY"
[018h 0024   4]                 Oem Revision : 00000021
[01Ch 0028   4]              Asl Compiler ID : ""
[020h 0032   4]        Asl Compiler Revision : 00000000

[024h 0036   4]                        Flags : 00000000
[028h 0040   8]              Control Address : 00000000DBFFF000
[030h 0048   4]                 Start Method : 00000002

Raw Table Data: Length 52 (0x34)

  0000: 54 50 4D 32 34 00 00 00 03 31 49 4E 54 45 4C 20  TPM24....1INTEL 
  0010: 44 33 34 30 31 30 57 59 21 00 00 00 00 00 00 00  D34010WY!.......
  0020: 00 00 00 00 00 00 00 00 00 F0 FF DB 00 00 00 00  ................
  0030: 02 00 00 00                                      ....

Obviously I'm going to keep the work around because I don't want to risk
breaking machines in the field. Because as a side effect for any machine
the driver always invokes CRB start I will definitely want to simplify
the state of the driver.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Thu, Oct 20, 2016 at 04:59:06PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 19, 2016 at 07:09:28PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 19, 2016 at 10:28:29AM +0000, Winkler, Tomas wrote:
> > > 
> > > 
> > > > 
> > > > On Mon, Oct 17, 2016 at 11:42:24PM +0300, Jarkko Sakkinen wrote:
> > > > > Because all the existing hardware have HID MSFT0101 we end up always
> > > > > setting CRB_FL_CRB_START flag as a workaround for 4th Gen Core CPUs.
> > > > > Even if ACPI start is used, the driver will always issue also CRB start.
> > > 
> > > Do you have some more historical data about this fix, I was wondering
> > > about this quirk before, when restructuring the start method parsing.
> > > The description is ' in practice seems to require both'  sounds not
> > > certain about the root cause of this.
> > 
> > I have a 4th Gen Core NUC where I experienced this issue. It reported
> > requiring only ACPI start but actually required ACPI + CRB start. The
> > comment could have been better.
> 
> With the latest master branch if I remove the workaround:
> 
> [  395.161155] tpm_crb: module verification failed: signature and/or required key missing - tainting kernel
> [  480.087136] tpm tpm0: A TPM error (323) occurred continue selftest
> [  480.087141] tpm tpm0: TPM self test failed
> 
> jsakkine at jsakkine-tpm1 in ~/devel/linux-tpmdd (master●●) 
> $ git --no-pager diff
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 65040d7..5b186e0 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -407,14 +407,6 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
> -	 * report only ACPI start but in practice seems to require both
> -	 * ACPI start and CRB start.
> -	 */
> -	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
> -	    !strcmp(acpi_device_hid(device), "MSFT0101"))
> -		priv->flags |= CRB_FL_CRB_START;
> -
>  	if (sm == ACPI_TPM2_START_METHOD ||
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
> 
> jsakkine at jsakkine-tpm1 in ~/devel/linux-tpmdd (master●●) 
> $ sudo dmidecode -t bios -q
> BIOS Information
> 	Vendor: Intel Corp.
> 	Version: WYLPT10H.86A.0033.2014.1201.0940
> 	Release Date: 12/01/2014
> 	Address: 0xF0000
> 	Runtime Size: 64 kB
> 	ROM Size: 6656 kB
> 	Characteristics:
> 		PCI is supported
> 		BIOS is upgradeable
> 		BIOS shadowing is allowed
> 		Boot from CD is supported
> 		Selectable boot is supported
> 		BIOS ROM is socketed
> 		EDD is supported
> 		5.25"/1.2 MB floppy services are supported (int 13h)
> 		3.5"/720 kB floppy services are supported (int 13h)
> 		3.5"/2.88 MB floppy services are supported (int 13h)
> 		Print screen service is supported (int 5h)
> 		Serial services are supported (int 14h)
> 		Printer services are supported (int 17h)
> 		ACPI is supported
> 		USB legacy is supported
> 		BIOS boot specification is supported
> 		Targeted content distribution is supported
> 		UEFI is supported
> 	BIOS Revision: 4.6
> 
> BIOS Language Information
> 	Language Description Format: Long
> 	Installable Languages: 1
> 		en|US|iso8859-1
> 	Currently Installed Language: en|US|iso8859-1
> 
> jsakkine at jsakkine-tpm1 in ~/tmp (master) 
> $ cat ~/tmp/tpm2.dsl 
> /*
>  * Intel ACPI Component Architecture
>  * AML Disassembler version 20140214-64 [Mar 29 2014]
>  * Copyright (c) 2000 - 2014 Intel Corporation
>  * 
>  * Disassembly of tpm2.dat, Wed Jun  1 16:26:49 2016
>  *
>  * ACPI Data Table [TPM2]
>  *
>  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
>  */
> 
> [000h 0000   4]                    Signature : "TPM2"    [Trusted Platform Module hardware interface table]
> [004h 0004   4]                 Table Length : 00000034
> [008h 0008   1]                     Revision : 03
> [009h 0009   1]                     Checksum : 31
> [00Ah 0010   6]                       Oem ID : "INTEL "
> [010h 0016   8]                 Oem Table ID : "D34010WY"
> [018h 0024   4]                 Oem Revision : 00000021
> [01Ch 0028   4]              Asl Compiler ID : ""
> [020h 0032   4]        Asl Compiler Revision : 00000000
> 
> [024h 0036   4]                        Flags : 00000000
> [028h 0040   8]              Control Address : 00000000DBFFF000
> [030h 0048   4]                 Start Method : 00000002
> 
> Raw Table Data: Length 52 (0x34)
> 
>   0000: 54 50 4D 32 34 00 00 00 03 31 49 4E 54 45 4C 20  TPM24....1INTEL 
>   0010: 44 33 34 30 31 30 57 59 21 00 00 00 00 00 00 00  D34010WY!.......
>   0020: 00 00 00 00 00 00 00 00 00 F0 FF DB 00 00 00 00  ................
>   0030: 02 00 00 00                                      ....
> 
> Obviously I'm going to keep the work around because I don't want to risk
> breaking machines in the field. Because as a side effect for any machine
> the driver always invokes CRB start I will definitely want to simplify
> the state of the driver.

Oops. I forgot to mension the device: D34010WYK

http://www.intel.com/content/www/us/en/nuc/nuc-kit-d34010wyk-board-d34010wyb.html

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jason Gunthorpe]

On Thu, Oct 20, 2016 at 05:00:11PM +0300, Jarkko Sakkinen wrote:

> > > I have a 4th Gen Core NUC where I experienced this issue. It reported
> > > requiring only ACPI start but actually required ACPI + CRB start. The
> > > comment could have been better.

Shouldn't bios work arounds be keyed on something? What happens if a
system rolls around that cannot do ACPI + CRB start? How does this
system work in windows?

Jason

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Thu, Oct 20, 2016 at 02:24:21PM -0600, Jason Gunthorpe wrote:
> On Thu, Oct 20, 2016 at 05:00:11PM +0300, Jarkko Sakkinen wrote:
> 
> > > > I have a 4th Gen Core NUC where I experienced this issue. It reported
> > > > requiring only ACPI start but actually required ACPI + CRB start. The
> > > > comment could have been better.
> 
> Shouldn't bios work arounds be keyed on something? What happens if a
> system rolls around that cannot do ACPI + CRB start? How does this
> system work in windows?

I didn't find anything better to key it on at the time and it has been
working for two years now without any problems.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm, tpm_crb: remove redundant CRB_FL_CRB_START flag

[Jarkko Sakkinen]

On Fri, Oct 21, 2016 at 06:11:41PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 20, 2016 at 02:24:21PM -0600, Jason Gunthorpe wrote:
> > On Thu, Oct 20, 2016 at 05:00:11PM +0300, Jarkko Sakkinen wrote:
> > 
> > > > > I have a 4th Gen Core NUC where I experienced this issue. It reported
> > > > > requiring only ACPI start but actually required ACPI + CRB start. The
> > > > > comment could have been better.
> > 
> > Shouldn't bios work arounds be keyed on something? What happens if a
> > system rolls around that cannot do ACPI + CRB start? How does this
> > system work in windows?
> 
> I didn't find anything better to key it on at the time and it has been
> working for two years now without any problems.

I've though about this and came to conclusion that maybe I won't apply
this patch because it is good to keep this robustness in for now. I
think your point is valid.

/Jarkko