[tpmdd-devel,3/3] tpm/tpm_crb: Enable TPM CRB interface for ARM64

Submitted by Jiandi An on March 10, 2017, 9:58 a.m.

Details

Message ID 1489139889-14376-4-git-send-email-anjiandi@codeaurora.org
State New
Headers show

Commit Message

Jiandi An March 10, 2017, 9:58 a.m.
This enables TPM Command Response Buffer interface driver for
ARM64 and implements an ARM specific TPM CRB start method that
invokes a Secure Monitor Call to request the Firmware to execute
or cancel a TPM 2.0 command.

Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
---
 drivers/char/tpm/Kconfig   |  2 +-
 drivers/char/tpm/tpm_crb.c | 24 ++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

Comments

Jason Gunthorpe March 10, 2017, 5:01 p.m.
On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:

> +	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
> +		if ((buf->header.length - default_len) !=
> +		    sizeof(struct tpm2_crb_smc)) {
> +			dev_err(dev, "TPM2 ACPI table has wrong size %u for start method type %d\n",
> +				buf->header.length,

This should be:

	dev_err(dev, FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",

Jason

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
Jason Gunthorpe March 10, 2017, 5:02 p.m.
On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:

>  	tristate "TPM 2.0 CRB Interface"
> -	depends on X86 && ACPI
> +	depends on (X86 || ARM64) && ACPI

Oh, and why is the X86 even here?

Surely it should just be 'depends on ACPI'? I don't remember anything
x86 specific in CRB?

Jason

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
Jiandi An March 10, 2017, 7:50 p.m.
On 03/10/17 11:02, Jason Gunthorpe wrote:
> On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
>
>>   	tristate "TPM 2.0 CRB Interface"
>> -	depends on X86 && ACPI
>> +	depends on (X86 || ARM64) && ACPI
>
> Oh, and why is the X86 even here?
>
> Surely it should just be 'depends on ACPI'? I don't remember anything
> x86 specific in CRB?
>
> Jason
>

During testing of my patches on ARM64, I hit crb_cmd_ready() and
crb_go_idle() that fails.  Looking at the comment and commit history,
it was added for fixing Intel PTT hw bug in Skylake and Broxton.
The functions themselves do nothing if flag is CRB_FL_ACPI_START.
I added CRB_FL_CRB_SMC_START which is the new ARM64 start method for
those two functions to also do nothing.  So crb_cmd_ready() and
crb_go_idle() are only for when CRB_FL_ACPI_START.

And in crb_acpi_add(), there is also comments on what drives
CRB_FL_CRB_START to be set.  ACPI_TPM2_COMMAND_BUFFER or
ACPI_TPM2_MEMORY_MAPPED

/* 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.
  */

I didn't have too much background on the CRB driver for X86.  I
thought to be safe just or in ARM64.  But if you see no issues,
I'll remove (X86 || ARM64) and just make it depend on ACPI.

Thanks.
- Jiandi
Jiandi An March 11, 2017, 12:45 a.m.
On 03/10/17 11:01, Jason Gunthorpe wrote:
> On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
>
>> +	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
>> +		if ((buf->header.length - default_len) !=
>> +		    sizeof(struct tpm2_crb_smc)) {
>> +			dev_err(dev, "TPM2 ACPI table has wrong size %u
> for start method type %d\n",
>> +				buf->header.length,
>
> This should be:
>
> 	dev_err(dev, FW_BUG "TPM2 ACPI table has wrong size %u for start
> method type %d\n",
>
> Jason

I will fix this in next version.  Waiting to see if there are
more comments from others.
Thanks
- Jiandi

>
> --------------------------------------------------------------------------
> ----
> Announcing the Oxford Dictionaries API! The API offers world-renowned
> dictionary content that is easy and intuitive to access. Sign up for an
> account today to start using our lexical data to power your apps and
> projects. Get started today and enter our developer competition.
> http://sdm.link/oxford
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
Jarkko Sakkinen March 11, 2017, 8:42 a.m.
On Fri, Mar 10, 2017 at 10:02:16AM -0700, Jason Gunthorpe wrote:
> On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
> 
> >  	tristate "TPM 2.0 CRB Interface"
> > -	depends on X86 && ACPI
> > +	depends on (X86 || ARM64) && ACPI
> 
> Oh, and why is the X86 even here?
> 
> Surely it should just be 'depends on ACPI'? I don't remember anything
> x86 specific in CRB?
> 
> Jason

Not sure, maybe I though originally (back in 2014) the byte order
but if all ACPI platforms are little endian it is not needed.

/Jarkko

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
Jarkko Sakkinen March 11, 2017, 8:42 a.m.
On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
> This enables TPM Command Response Buffer interface driver for
> ARM64 and implements an ARM specific TPM CRB start method that
> invokes a Secure Monitor Call to request the Firmware to execute
> or cancel a TPM 2.0 command.
> 
> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>

Does look good to me. I might consider to squash this with the
two inline functions.

/Jarkko

> ---
>  drivers/char/tpm/Kconfig   |  2 +-
>  drivers/char/tpm/tpm_crb.c | 24 ++++++++++++++++++++++--
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index d520ac5..9659f40 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -136,7 +136,7 @@ config TCG_XEN
>  
>  config TCG_CRB
>  	tristate "TPM 2.0 CRB Interface"
> -	depends on X86 && ACPI
> +	depends on (X86 || ARM64) && ACPI
>  	---help---
>  	  If you have a TPM security chip that is compliant with the
>  	  TCG CRB 2.0 TPM specification say Yes and it will be accessible
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 089fcf8..d29a84a 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -73,6 +73,7 @@ enum crb_status {
>  enum crb_flags {
>  	CRB_FL_ACPI_START	= BIT(0),
>  	CRB_FL_CRB_START	= BIT(1),
> +	CRB_FL_CRB_SMC_START	= BIT(2),
>  };
>  
>  struct crb_priv {
> @@ -82,6 +83,7 @@ struct crb_priv {
>  	u8 __iomem *cmd;
>  	u8 __iomem *rsp;
>  	u32 cmd_size;
> +	u32 smc_func_id;
>  };
>  
>  /**
> @@ -101,7 +103,8 @@ struct crb_priv {
>   */
>  static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>  {
> -	if (priv->flags & CRB_FL_ACPI_START)
> +	if ((priv->flags & CRB_FL_ACPI_START) ||
> +	    (priv->flags & CRB_FL_CRB_SMC_START))
>  		return 0;
>  
>  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req);
> @@ -129,7 +132,8 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  {
>  	ktime_t stop, start;
>  
> -	if (priv->flags & CRB_FL_ACPI_START)
> +	if ((priv->flags & CRB_FL_ACPI_START) ||
> +	    (priv->flags & CRB_FL_CRB_SMC_START))
>  		return 0;
>  
>  	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req);
> @@ -229,6 +233,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	if (priv->flags & CRB_FL_ACPI_START)
>  		rc = crb_do_acpi_start(chip);
>  
> +	if (priv->flags & CRB_FL_CRB_SMC_START) {
> +		iowrite32(CRB_START_INVOKE, &priv->cca->start);
> +		rc = tpm_crb_smc_start(priv->smc_func_id);
> +	}
> +
>  	return rc;
>  }
>  
> @@ -445,6 +454,17 @@ static int crb_acpi_add(struct acpi_device *device)
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
> +	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
> +		if ((buf->header.length - default_len) !=
> +		    sizeof(struct tpm2_crb_smc)) {
> +			dev_err(dev, "TPM2 ACPI table has wrong size %u for start method type %d\n",
> +				buf->header.length, ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
> +			return -EINVAL;
> +		}
> +		priv->flags |= CRB_FL_CRB_SMC_START;
> +		priv->smc_func_id = buf->platform_data.smc_params.smc_func_id;
> +	}
> +
>  	rc = crb_map_io(device, priv, buf);
>  	if (rc)
>  		return rc;
> -- 
> Jiandi An
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
Jiandi An March 11, 2017, 8:40 p.m.
On 03/11/17 02:42, Jarkko Sakkinen wrote:
> On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
>> This enables TPM Command Response Buffer interface driver for
>> ARM64 and implements an ARM specific TPM CRB start method that
>> invokes a Secure Monitor Call to request the Firmware to execute
>> or cancel a TPM 2.0 command.
>>
>> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
>
> Does look good to me. I might consider to squash this with the
> two inline functions.
>
> /Jarkko

I'll squash the SMC inline function patch with this in next version
to address this.

Thanks.
- Jiandi

>
>> ---
>>   drivers/char/tpm/Kconfig   |  2 +-
>>   drivers/char/tpm/tpm_crb.c | 24 ++++++++++++++++++++++--
>>   2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>> index d520ac5..9659f40 100644
>> --- a/drivers/char/tpm/Kconfig
>> +++ b/drivers/char/tpm/Kconfig
>> @@ -136,7 +136,7 @@ config TCG_XEN
>>
>>   config TCG_CRB
>>   	tristate "TPM 2.0 CRB Interface"
>> -	depends on X86 && ACPI
>> +	depends on (X86 || ARM64) && ACPI
>>   	---help---
>>   	  If you have a TPM security chip that is compliant with the
>>   	  TCG CRB 2.0 TPM specification say Yes and it will be accessible
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index 089fcf8..d29a84a 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -73,6 +73,7 @@ enum crb_status {
>>   enum crb_flags {
>>   	CRB_FL_ACPI_START	= BIT(0),
>>   	CRB_FL_CRB_START	= BIT(1),
>> +	CRB_FL_CRB_SMC_START	= BIT(2),
>>   };
>>
>>   struct crb_priv {
>> @@ -82,6 +83,7 @@ struct crb_priv {
>>   	u8 __iomem *cmd;
>>   	u8 __iomem *rsp;
>>   	u32 cmd_size;
>> +	u32 smc_func_id;
>>   };
>>
>>   /**
>> @@ -101,7 +103,8 @@ struct crb_priv {
>>    */
>>   static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>>   {
>> -	if (priv->flags & CRB_FL_ACPI_START)
>> +	if ((priv->flags & CRB_FL_ACPI_START) ||
>> +	    (priv->flags & CRB_FL_CRB_SMC_START))
>>   		return 0;
>>
>>   	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req);
>> @@ -129,7 +132,8 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>>   {
>>   	ktime_t stop, start;
>>
>> -	if (priv->flags & CRB_FL_ACPI_START)
>> +	if ((priv->flags & CRB_FL_ACPI_START) ||
>> +	    (priv->flags & CRB_FL_CRB_SMC_START))
>>   		return 0;
>>
>>   	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req);
>> @@ -229,6 +233,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>   	if (priv->flags & CRB_FL_ACPI_START)
>>   		rc = crb_do_acpi_start(chip);
>>
>> +	if (priv->flags & CRB_FL_CRB_SMC_START) {
>> +		iowrite32(CRB_START_INVOKE, &priv->cca->start);
>> +		rc = tpm_crb_smc_start(priv->smc_func_id);
>> +	}
>> +
>>   	return rc;
>>   }
>>
>> @@ -445,6 +454,17 @@ static int crb_acpi_add(struct acpi_device *device)
>>   	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>>   		priv->flags |= CRB_FL_ACPI_START;
>>
>> +	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
>> +		if ((buf->header.length - default_len) !=
>> +		    sizeof(struct tpm2_crb_smc)) {
>> +			dev_err(dev, "TPM2 ACPI table has wrong size %u for start method type %d\n",
>> +				buf->header.length, ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
>> +			return -EINVAL;
>> +		}
>> +		priv->flags |= CRB_FL_CRB_SMC_START;
>> +		priv->smc_func_id = buf->platform_data.smc_params.smc_func_id;
>> +	}
>> +
>>   	rc = crb_map_io(device, priv, buf);
>>   	if (rc)
>>   		return rc;
>> --
>> Jiandi An
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
Jarkko Sakkinen March 13, 2017, 11:42 a.m.
On Sat, Mar 11, 2017 at 02:40:30PM -0600, Jiandi An wrote:
> On 03/11/17 02:42, Jarkko Sakkinen wrote:
> > On Fri, Mar 10, 2017 at 03:58:09AM -0600, Jiandi An wrote:
> > > This enables TPM Command Response Buffer interface driver for
> > > ARM64 and implements an ARM specific TPM CRB start method that
> > > invokes a Secure Monitor Call to request the Firmware to execute
> > > or cancel a TPM 2.0 command.
> > > 
> > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
> > 
> > Does look good to me. I might consider to squash this with the
> > two inline functions.
> > 
> > /Jarkko
> 
> I'll squash the SMC inline function patch with this in next version
> to address this.
> 
> Thanks.
> - Jiandi

Thanks!

/Jarkko

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford

Patch hide | download patch | download mbox

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index d520ac5..9659f40 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -136,7 +136,7 @@  config TCG_XEN
 
 config TCG_CRB
 	tristate "TPM 2.0 CRB Interface"
-	depends on X86 && ACPI
+	depends on (X86 || ARM64) && ACPI
 	---help---
 	  If you have a TPM security chip that is compliant with the
 	  TCG CRB 2.0 TPM specification say Yes and it will be accessible
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 089fcf8..d29a84a 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -73,6 +73,7 @@  enum crb_status {
 enum crb_flags {
 	CRB_FL_ACPI_START	= BIT(0),
 	CRB_FL_CRB_START	= BIT(1),
+	CRB_FL_CRB_SMC_START	= BIT(2),
 };
 
 struct crb_priv {
@@ -82,6 +83,7 @@  struct crb_priv {
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
 	u32 cmd_size;
+	u32 smc_func_id;
 };
 
 /**
@@ -101,7 +103,8 @@  struct crb_priv {
  */
 static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
-	if (priv->flags & CRB_FL_ACPI_START)
+	if ((priv->flags & CRB_FL_ACPI_START) ||
+	    (priv->flags & CRB_FL_CRB_SMC_START))
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->cca->req);
@@ -129,7 +132,8 @@  static int __maybe_unused crb_cmd_ready(struct device *dev,
 {
 	ktime_t stop, start;
 
-	if (priv->flags & CRB_FL_ACPI_START)
+	if ((priv->flags & CRB_FL_ACPI_START) ||
+	    (priv->flags & CRB_FL_CRB_SMC_START))
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req);
@@ -229,6 +233,11 @@  static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	if (priv->flags & CRB_FL_ACPI_START)
 		rc = crb_do_acpi_start(chip);
 
+	if (priv->flags & CRB_FL_CRB_SMC_START) {
+		iowrite32(CRB_START_INVOKE, &priv->cca->start);
+		rc = tpm_crb_smc_start(priv->smc_func_id);
+	}
+
 	return rc;
 }
 
@@ -445,6 +454,17 @@  static int crb_acpi_add(struct acpi_device *device)
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
 
+	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
+		if ((buf->header.length - default_len) !=
+		    sizeof(struct tpm2_crb_smc)) {
+			dev_err(dev, "TPM2 ACPI table has wrong size %u for start method type %d\n",
+				buf->header.length, ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
+			return -EINVAL;
+		}
+		priv->flags |= CRB_FL_CRB_SMC_START;
+		priv->smc_func_id = buf->platform_data.smc_params.smc_func_id;
+	}
+
 	rc = crb_map_io(device, priv, buf);
 	if (rc)
 		return rc;