[PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

For ARM64, the locality is handled by Trust Zone in FW.
The layout does not have crb_regs_head.  It is hitting
the following line.
dev_warn(dev, FW_BUG "Bad ACPI memory layout");

Current code excludes CRB_FL_ACPI_START and when
CRB_FL_CRB_SMC_START is added around the same time
locality support is added, it should also be excluded.

For goIdle and cmdReady where code was excluding
CRB_FL_ACPI_START only (do nothing for ACPI start method),
CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
method does not have TPM_CRB_CTRL_REQ.
Change if confition to white list instead of black list.

Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
---
 drivers/char/tpm/tpm_crb.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8f0a98d..cbfdbdde 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -128,18 +128,16 @@ struct tpm2_crb_smc {
  * Anyhow, we do not wait here as a consequent CMD_READY request
  * will be handled correctly even if idle was not completed.
  *
- * The function does nothing for devices with ACPI-start method.
+ * The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
  *
  * Return: 0 always
  */
 static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
-	if ((priv->flags & CRB_FL_ACPI_START) ||
-	    (priv->flags & CRB_FL_CRB_SMC_START))
-		return 0;
-
-	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
-	/* we don't really care when this settles */
+	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
+		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
+		/* we don't really care when this settles */
 
 	return 0;
 }
@@ -174,23 +172,22 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  * The device should respond within TIMEOUT_C.
  *
  * The function does nothing for devices with ACPI-start method
+ * or SMC-start method.
  *
  * Return: 0 on success -ETIME on timeout;
  */
 static int __maybe_unused crb_cmd_ready(struct device *dev,
 					struct crb_priv *priv)
 {
-	if ((priv->flags & CRB_FL_ACPI_START) ||
-	    (priv->flags & CRB_FL_CRB_SMC_START))
-		return 0;
-
-	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
-	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
-				 CRB_CTRL_REQ_CMD_READY /* mask */,
-				 0, /* value */
-				 TPM2_TIMEOUT_C)) {
-		dev_warn(dev, "cmdReady timed out\n");
-		return -ETIME;
+	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
+		iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
+		if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
+					 CRB_CTRL_REQ_CMD_READY /* mask */,
+					 0, /* value */
+					 TPM2_TIMEOUT_C)) {
+			dev_warn(dev, "cmdReady timed out\n");
+			return -ETIME;
+		}
 	}
 
 	return 0;
@@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 * the control area, as one nice sane region except for some older
 	 * stuff that puts the control area outside the ACPI IO region.
 	 */
-	if (!(priv->flags & CRB_FL_ACPI_START)) {
+	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
 		if (buf->control_address == io_res.start +
 		    sizeof(*priv->regs_h))
 			priv->regs_h = priv->iobase;
-- 
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.

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
> For ARM64, the locality is handled by Trust Zone in FW.
> The layout does not have crb_regs_head.  It is hitting
> the following line.
> dev_warn(dev, FW_BUG "Bad ACPI memory layout");
> 
> Current code excludes CRB_FL_ACPI_START and when
> CRB_FL_CRB_SMC_START is added around the same time
> locality support is added, it should also be excluded.
> 
> For goIdle and cmdReady where code was excluding
> CRB_FL_ACPI_START only (do nothing for ACPI start method),
> CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
> method does not have TPM_CRB_CTRL_REQ.
> Change if confition to white list instead of black list.
> 
> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>

Is this v2? If so, where is the change log?

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8f0a98d..cbfdbdde 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -128,18 +128,16 @@ struct tpm2_crb_smc {
>   * Anyhow, we do not wait here as a consequent CMD_READY request
>   * will be handled correctly even if idle was not completed.
>   *
> - * The function does nothing for devices with ACPI-start method.
> + * The function does nothing for devices with ACPI-start method
> + * or SMC-start method.
>   *
>   * Return: 0 always
>   */
>  static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>  {
> -	if ((priv->flags & CRB_FL_ACPI_START) ||
> -	    (priv->flags & CRB_FL_CRB_SMC_START))
> -		return 0;
> -
> -	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> -	/* we don't really care when this settles */
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
> +		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> +		/* we don't really care when this settles */
>  
>  	return 0;
>  }
> @@ -174,23 +172,22 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>   * The device should respond within TIMEOUT_C.
>   *
>   * The function does nothing for devices with ACPI-start method
> + * or SMC-start method.
>   *
>   * Return: 0 on success -ETIME on timeout;
>   */
>  static int __maybe_unused crb_cmd_ready(struct device *dev,
>  					struct crb_priv *priv)
>  {
> -	if ((priv->flags & CRB_FL_ACPI_START) ||
> -	    (priv->flags & CRB_FL_CRB_SMC_START))
> -		return 0;
> -
> -	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
> -	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> -				 CRB_CTRL_REQ_CMD_READY /* mask */,
> -				 0, /* value */
> -				 TPM2_TIMEOUT_C)) {
> -		dev_warn(dev, "cmdReady timed out\n");
> -		return -ETIME;
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
> +		iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
> +		if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> +					 CRB_CTRL_REQ_CMD_READY /* mask */,
> +					 0, /* value */
> +					 TPM2_TIMEOUT_C)) {
> +			dev_warn(dev, "cmdReady timed out\n");
> +			return -ETIME;
> +		}
>  	}
>  
>  	return 0;
> @@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	 * the control area, as one nice sane region except for some older
>  	 * stuff that puts the control area outside the ACPI IO region.
>  	 */
> -	if (!(priv->flags & CRB_FL_ACPI_START)) {
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>  		if (buf->control_address == io_res.start +
>  		    sizeof(*priv->regs_h))
>  			priv->regs_h = priv->iobase;
> -- 
> 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.
> 

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

On 08/19/2017 12:05 PM, Jarkko Sakkinen wrote:
> On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
>> For ARM64, the locality is handled by Trust Zone in FW.
>> The layout does not have crb_regs_head.  It is hitting
>> the following line.
>> dev_warn(dev, FW_BUG "Bad ACPI memory layout");
>>
>> Current code excludes CRB_FL_ACPI_START and when
>> CRB_FL_CRB_SMC_START is added around the same time
>> locality support is added, it should also be excluded.
>>
>> For goIdle and cmdReady where code was excluding
>> CRB_FL_ACPI_START only (do nothing for ACPI start method),
>> CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
>> method does not have TPM_CRB_CTRL_REQ.
>> Change if confition to white list instead of black list.
>>
>> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
>
> Is this v2? If so, where is the change log?
Based on the previous comments, I now understand that
because of Intel PTT bug workaround, it is not appropriate
for the patch to have title/subject "Access locality for
only CRB_START method"

So the more descriptive patch title is "Access locality for
only non-ACPI and non-SMC start method".  Because the patch
is changed, I thought I start a new series.  Would you like
me to tag this v3 and put change log even though patch title
has changed?

Thanks
- Jiandi

>
> /Jarkko
>

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

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Sun, Aug 20, 2017 at 10:41:38PM -0500, Jiandi An wrote:
> 
> 
> On 08/19/2017 12:05 PM, Jarkko Sakkinen wrote:
> > On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
> > > For ARM64, the locality is handled by Trust Zone in FW.
> > > The layout does not have crb_regs_head.  It is hitting
> > > the following line.
> > > dev_warn(dev, FW_BUG "Bad ACPI memory layout");
> > > 
> > > Current code excludes CRB_FL_ACPI_START and when
> > > CRB_FL_CRB_SMC_START is added around the same time
> > > locality support is added, it should also be excluded.
> > > 
> > > For goIdle and cmdReady where code was excluding
> > > CRB_FL_ACPI_START only (do nothing for ACPI start method),
> > > CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
> > > method does not have TPM_CRB_CTRL_REQ.
> > > Change if confition to white list instead of black list.
> > > 
> > > Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
> > 
> > Is this v2? If so, where is the change log?
> Based on the previous comments, I now understand that
> because of Intel PTT bug workaround, it is not appropriate
> for the patch to have title/subject "Access locality for
> only CRB_START method"
> 
> So the more descriptive patch title is "Access locality for
> only non-ACPI and non-SMC start method".  Because the patch
> is changed, I thought I start a new series.  Would you like
> me to tag this v3 and put change log even though patch title
> has changed?
> 
> Thanks
> - Jiandi

I see. Thanks for explaining.

/Jarkko

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
> For ARM64, the locality is handled by Trust Zone in FW.
> The layout does not have crb_regs_head.  It is hitting
> the following line.
> dev_warn(dev, FW_BUG "Bad ACPI memory layout");
> 
> Current code excludes CRB_FL_ACPI_START and when
> CRB_FL_CRB_SMC_START is added around the same time
> locality support is added, it should also be excluded.
> 
> For goIdle and cmdReady where code was excluding
> CRB_FL_ACPI_START only (do nothing for ACPI start method),
> CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
> method does not have TPM_CRB_CTRL_REQ.
> Change if confition to white list instead of black list.
> 
> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
> ---
>  drivers/char/tpm/tpm_crb.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8f0a98d..cbfdbdde 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -128,18 +128,16 @@ struct tpm2_crb_smc {
>   * Anyhow, we do not wait here as a consequent CMD_READY request
>   * will be handled correctly even if idle was not completed.
>   *
> - * The function does nothing for devices with ACPI-start method.
> + * The function does nothing for devices with ACPI-start method
> + * or SMC-start method.
>   *
>   * Return: 0 always
>   */
>  static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>  {
> -	if ((priv->flags & CRB_FL_ACPI_START) ||
> -	    (priv->flags & CRB_FL_CRB_SMC_START))
> -		return 0;
> -
> -	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> -	/* we don't really care when this settles */
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
> +		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> +		/* we don't really care when this settles */

It's *exactly* the same condition expessed in different form.


>  
>  	return 0;
>  }
> @@ -174,23 +172,22 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>   * The device should respond within TIMEOUT_C.
>   *
>   * The function does nothing for devices with ACPI-start method
> + * or SMC-start method.
>   *
>   * Return: 0 on success -ETIME on timeout;
>   */
>  static int __maybe_unused crb_cmd_ready(struct device *dev,
>  					struct crb_priv *priv)
>  {
> -	if ((priv->flags & CRB_FL_ACPI_START) ||
> -	    (priv->flags & CRB_FL_CRB_SMC_START))
> -		return 0;
> -
> -	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
> -	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> -				 CRB_CTRL_REQ_CMD_READY /* mask */,
> -				 0, /* value */
> -				 TPM2_TIMEOUT_C)) {
> -		dev_warn(dev, "cmdReady timed out\n");
> -		return -ETIME;
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
> +		iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
> +		if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
> +					 CRB_CTRL_REQ_CMD_READY /* mask */,
> +					 0, /* value */
> +					 TPM2_TIMEOUT_C)) {
> +			dev_warn(dev, "cmdReady timed out\n");
> +			return -ETIME;
> +		}
>  	}
>  
>  	return 0;
> @@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>  	 * the control area, as one nice sane region except for some older
>  	 * stuff that puts the control area outside the ACPI IO region.
>  	 */
> -	if (!(priv->flags & CRB_FL_ACPI_START)) {
> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>  		if (buf->control_address == io_res.start +
>  		    sizeof(*priv->regs_h))
>  			priv->regs_h = priv->iobase;
> -- 
> 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.
> 

NAK

/Jarkko

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

On 08/22/2017 12:39 PM, Jarkko Sakkinen wrote:
> On Thu, Aug 17, 2017 at 11:15:36PM -0500, Jiandi An wrote:
>> For ARM64, the locality is handled by Trust Zone in FW.
>> The layout does not have crb_regs_head.  It is hitting
>> the following line.
>> dev_warn(dev, FW_BUG "Bad ACPI memory layout");
>>
>> Current code excludes CRB_FL_ACPI_START and when
>> CRB_FL_CRB_SMC_START is added around the same time
>> locality support is added, it should also be excluded.
>>
>> For goIdle and cmdReady where code was excluding
>> CRB_FL_ACPI_START only (do nothing for ACPI start method),
>> CRB_FL_CRB_SMC_START was also excluded as ARM64 SMC start
>> method does not have TPM_CRB_CTRL_REQ.
>> Change if confition to white list instead of black list.
>>
>> Signed-off-by: Jiandi An <anjiandi@codeaurora.org>
>> ---
>>  drivers/char/tpm/tpm_crb.c | 35 ++++++++++++++++-------------------
>>  1 file changed, 16 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index 8f0a98d..cbfdbdde 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -128,18 +128,16 @@ struct tpm2_crb_smc {
>>   * Anyhow, we do not wait here as a consequent CMD_READY request
>>   * will be handled correctly even if idle was not completed.
>>   *
>> - * The function does nothing for devices with ACPI-start method.
>> + * The function does nothing for devices with ACPI-start method
>> + * or SMC-start method.
>>   *
>>   * Return: 0 always
>>   */
>>  static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>>  {
>> -	if ((priv->flags & CRB_FL_ACPI_START) ||
>> -	    (priv->flags & CRB_FL_CRB_SMC_START))
>> -		return 0;
>> -
>> -	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>> -	/* we don't really care when this settles */
>> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
>> +		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>> +		/* we don't really care when this settles */
>
> It's *exactly* the same condition expessed in different form.
>

I'm sorry perhaps I didn't fully understand the workaround specific to 
Intel PPT.  In previous patch thread, you mentioned the following where
a platform could report to require start method 2 (ACPI start) which is
sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.

But you listed the following code logic which for either sm = 2 or
sm = 8, CRB_FL_ACPI_START flag is set.

if (sm == ACPI_TPM2_START_METHOD ||
     sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
	priv->flags |= CRB_FL_ACPI_START;

So I'm a little confused about the PPT workaround you mentioned

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

I took it as some platform sm = 2 or sm = 8 which CRB_FL_ACPI_START
flag is set, additionally, if HID = MSFT0101, CRB_FL_CRB_START flag
is set.

So I took it as the PPT workaround you mentioned is an instance where
both CRB_FL_ACPI_START and CRB_FL_CRB_START are set.

The original code logic for crb_go_idle() and crb_cmd_ready() only
exclude CRB_FL_ACPI_START, saying crb_go_idle() and crb_cmd_ready()
do nothing if CRB_FL_ACPI_START is set.

static int __maybe_unused crb_go_idle(struct device *dev,
                                       struct crb_priv *priv)
{
      if (priv->flags & CRB_FL_ACPI_START)
          return 0;

So even in the case when both CRB_FL_ACPI_START and CRB_FL_CRB_START
are set, crb_go_idle() and crb_cmd_ready() will not do anything because
CRB_FL_ACPI_START is set.

And when SMC start method was enabled for ARM64, I further excluded
CRB_FL_CRB_SMC_START

static int __maybe_unused crb_go_idle(struct device *dev,
                                       struct crb_priv *priv)
{
      if ((priv->flags & CRB_FL_ACPI_START) ||
          (prive->flags & CRB_FL_CRB_SMC_START))
          return 0;

And Jason's comment was telling me to have these list the cases where
crb_go_idle() and crb_cmd_ready() are known to be required.  Less
brittle that way.  And he gave example...

if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_START)) == 0)
        return 0


The following is your comment and Jason's comment in the other patch
thread discussion here
https://sourceforge.net/p/tpmdd/mailman/message/35984747/

*****************************
What about platforms with firmware bug that they report only requiring
ACPI start but actually also require CRB start.

if (sm == ACPI_TPM2_START_METHOD ||
     sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
	priv->flags |= CRB_FL_ACPI_START;

I've encountered a platform that reports to require start method 2 (ACPI
start) while it actually requires start method 8.

That's why we have this nasty workaround:

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

Also, since start method 8 exist in the spec, it is a legit config.

/Jarkko


I think it would be better to have these list the cases where go_idle
is known to be required. Less brittle that way..

   if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_START)) == 0)
        return 0

Jason
*****************************

The intent of this patch is to additionally exclude CRB_FL_CRB_SMC_START
from crb_go_idle(), crb_cmd_ready(), and the check for "Bad ACPI memory
layout" in crb_map_io(), on top of current conditions.

The original code just has as long as CRB_FL_ACPI_START is not set,
don't do crb_go_idle(), crb_cmd_ready() and the additional check for
"Bad ACPI memory layout" in crb_map_io().

Could you share some insight on what exact conditions are for 
crb_go_idle(), crb_cmd_ready(), and the check "BAD ACPI memory layout" 
in crb_map_io()?

Is it as long as !CRB_FL_ACPI_START? (before SMC was added, that just
means CRB_FL_CRB_START is set).
So with the PTT workaround condition, are you suggesting the following
conditions are when crb_go_idle(), crb_cmd_ready(), and check for "Bad 
ACPI memory layout" in crb_map_io() should run?

1. CRB_FL_CRB_START is set.
2. CRB_FL_CRB_START and CRB_FL_ACPI_START are set.

Thanks.
- Jiandi

>
>>
>>  	return 0;
>>  }
>> @@ -174,23 +172,22 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>>   * The device should respond within TIMEOUT_C.
>>   *
>>   * The function does nothing for devices with ACPI-start method
>> + * or SMC-start method.
>>   *
>>   * Return: 0 on success -ETIME on timeout;
>>   */
>>  static int __maybe_unused crb_cmd_ready(struct device *dev,
>>  					struct crb_priv *priv)
>>  {
>> -	if ((priv->flags & CRB_FL_ACPI_START) ||
>> -	    (priv->flags & CRB_FL_CRB_SMC_START))
>> -		return 0;
>> -
>> -	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
>> -	if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
>> -				 CRB_CTRL_REQ_CMD_READY /* mask */,
>> -				 0, /* value */
>> -				 TPM2_TIMEOUT_C)) {
>> -		dev_warn(dev, "cmdReady timed out\n");
>> -		return -ETIME;
>> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>> +		iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
>> +		if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req,
>> +					 CRB_CTRL_REQ_CMD_READY /* mask */,
>> +					 0, /* value */
>> +					 TPM2_TIMEOUT_C)) {
>> +			dev_warn(dev, "cmdReady timed out\n");
>> +			return -ETIME;
>> +		}
>>  	}
>>
>>  	return 0;
>> @@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>>  	 * the control area, as one nice sane region except for some older
>>  	 * stuff that puts the control area outside the ACPI IO region.
>>  	 */
>> -	if (!(priv->flags & CRB_FL_ACPI_START)) {
>> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>>  		if (buf->control_address == io_res.start +
>>  		    sizeof(*priv->regs_h))
>>  			priv->regs_h = priv->iobase;
>> --
>> 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.
>>
>
> NAK
>
> /Jarkko
>

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

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jason Gunthorpe]

On Tue, Aug 22, 2017 at 04:28:54PM -0500, Jiandi An wrote:

> I'm sorry perhaps I didn't fully understand the workaround specific to Intel
> PPT.  In previous patch thread, you mentioned the following where
> a platform could report to require start method 2 (ACPI start) which is
> sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
> is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.

I'm also not sure.

To be clear, my desire to see a test that triggers only for the Intel
chips with the problem, and is written in a way that matches exactly
the ACPI data from the broken chip - so things like !CRB are not what
I want to see..

In that light the example I gave was probably not well thought out,
but I also do not understand the exact conditions needed for the Intel
work around either. Hopefully Jarkko can clarify.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Tue, Aug 22, 2017 at 04:28:54PM -0500, Jiandi An wrote:
>  > >  {
> > > -	if ((priv->flags & CRB_FL_ACPI_START) ||
> > > -	    (priv->flags & CRB_FL_CRB_SMC_START))
> > > -		return 0;
> > > -
> > > -	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> > > -	/* we don't really care when this settles */
> > > +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
> > > +		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> > > +		/* we don't really care when this settles */
> > 
> > It's *exactly* the same condition expessed in different form.
> > 
> 
> I'm sorry perhaps I didn't fully understand the workaround specific to Intel
> PPT.  In previous patch thread, you mentioned the following where
> a platform could report to require start method 2 (ACPI start) which is
> sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
> is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.

What this has to do with the above code change? Those two versions
compile most likely to almost if not exactly same machine code.

Both the original code and your updated blacklist cases where either
of those flags is set.

> But you listed the following code logic which for either sm = 2 or
> sm = 8, CRB_FL_ACPI_START flag is set.
> 
> if (sm == ACPI_TPM2_START_METHOD ||
>     sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
> 	priv->flags |= CRB_FL_ACPI_START;
> 
> So I'm a little confused about the PPT workaround you mentioned
> 
> /* 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;
> 
> I took it as some platform sm = 2 or sm = 8 which CRB_FL_ACPI_START
> flag is set, additionally, if HID = MSFT0101, CRB_FL_CRB_START flag
> is set.

Yes.

I'm starting to think that the code might be easier to follow if we
removed 'flags' and store sm to the priv struct and put conditionals in
places where we need them based on sm.

I think the 'flags' field was not a good idea in the first place.

/Jarkko

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

On 08/24/2017 07:26 AM, Jarkko Sakkinen wrote:
> On Tue, Aug 22, 2017 at 04:28:54PM -0500, Jiandi An wrote:
>>  > >  {
>>>> -	if ((priv->flags & CRB_FL_ACPI_START) ||
>>>> -	    (priv->flags & CRB_FL_CRB_SMC_START))
>>>> -		return 0;
>>>> -
>>>> -	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>>>> -	/* we don't really care when this settles */
>>>> +	if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0)
>>>> +		iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>>>> +		/* we don't really care when this settles */
>>>
>>> It's *exactly* the same condition expessed in different form.
>>>
>>
>> I'm sorry perhaps I didn't fully understand the workaround specific to Intel
>> PPT.  In previous patch thread, you mentioned the following where
>> a platform could report to require start method 2 (ACPI start) which is
>> sm = ACPI_TPM2_START_METHOD, and actually requires start method 8, which
>> is sm = ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD.
>
> What this has to do with the above code change? Those two versions
> compile most likely to almost if not exactly same machine code.
>
> Both the original code and your updated blacklist cases where either
> of those flags is set.

I know they don't change the logic.  This was to address comment from 
Jason.  He wanted to express the exact condition which crb_go_idle(),
crb_cmd_ready(), and the check for "Bad ACPI memory layout" in
crb_map_io() should run, instead of the if not the condition, return.
Since you want it as is, I'll change it back. It's already excluding
CRB_FL_CRB_SMC_START in addition to CRB_FL_ACPI_START which is what's
intended.

Like I said the goal for this patch is to really further exclude
CRB_FL_CRB_SMC_START from the check for "Bad ACPI memory layout"
in crb_map_io() in the code below.

@@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, 
struct crb_priv *priv,
   * the control area, as one nice sane region except for some older
   * stuff that puts the control area outside the ACPI IO region.
   */
  -if (!(priv->flags & CRB_FL_ACPI_START)) {
  +if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
   		if (buf->control_address == io_res.start +
   		    sizeof(*priv->regs_h))
   			priv->regs_h = priv->iobase;
   		else
   		    dev_warn(dev, FW_BUG "Bad ACPI memory layout");
   }

I'll submit v2 with only this change.  Are you okay which this change?

Thanks
- Jiandi


>
>> But you listed the following code logic which for either sm = 2 or
>> sm = 8, CRB_FL_ACPI_START flag is set.
>>
>> if (sm == ACPI_TPM2_START_METHOD ||
>>     sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>> 	priv->flags |= CRB_FL_ACPI_START;
>>
>> So I'm a little confused about the PPT workaround you mentioned
>>
>> /* 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;
>>
>> I took it as some platform sm = 2 or sm = 8 which CRB_FL_ACPI_START
>> flag is set, additionally, if HID = MSFT0101, CRB_FL_CRB_START flag
>> is set.
>
> Yes.
>
> I'm starting to think that the code might be easier to follow if we
> removed 'flags' and store sm to the priv struct and put conditionals in
> places where we need them based on sm.
>
> I think the 'flags' field was not a good idea in the first place.
>
> /Jarkko
>


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

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 12:20:35PM -0500, Jiandi An wrote:
> I know they don't change the logic.  This was to address comment from Jason.
> He wanted to express the exact condition which crb_go_idle(),
> crb_cmd_ready(), and the check for "Bad ACPI memory layout" in
> crb_map_io() should run, instead of the if not the condition, return.
> Since you want it as is, I'll change it back. It's already excluding
> CRB_FL_CRB_SMC_START in addition to CRB_FL_ACPI_START which is what's
> intended.

I think this very in simple terms. It does not change *anything*.

> Like I said the goal for this patch is to really further exclude
> CRB_FL_CRB_SMC_START from the check for "Bad ACPI memory layout"
> in crb_map_io() in the code below.
> 
> @@ -458,7 +455,7 @@ static int crb_map_io(struct acpi_device *device, struct
> crb_priv *priv,
>   * the control area, as one nice sane region except for some older
>   * stuff that puts the control area outside the ACPI IO region.
>   */
>  -if (!(priv->flags & CRB_FL_ACPI_START)) {
>  +if ((priv->flags & (CRB_FL_ACPI_START | CRB_FL_CRB_SMC_START)) == 0) {
>   		if (buf->control_address == io_res.start +
>   		    sizeof(*priv->regs_h))
>   			priv->regs_h = priv->iobase;
>   		else
>   		    dev_warn(dev, FW_BUG "Bad ACPI memory layout");
>   }
> 
> I'll submit v2 with only this change.  Are you okay which this change?

I'm not OK with those parts that do nothing except shuffle the code.

As I said before it would make much more sense to make code always deal
with sm and remove flags completely. That would help maintaining code
easier as new logic for TZ is introduced.

> Thanks
> - Jiandi

/Jarkko

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jason Gunthorpe]

On Fri, Aug 25, 2017 at 07:21:39PM +0300, Jarkko Sakkinen wrote:
> As I said before it would make much more sense to make code always deal
> with sm and remove flags completely. That would help maintaining code
> easier as new logic for TZ is introduced.

Yes please

Jason

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

On 08/25/2017 11:53 AM, Jason Gunthorpe wrote:
> On Fri, Aug 25, 2017 at 07:21:39PM +0300, Jarkko Sakkinen wrote:
>> As I said before it would make much more sense to make code always deal
>> with sm and remove flags completely. That would help maintaining code
>> easier as new logic for TZ is introduced.
>
> Yes please
>
> Jason
>
Okay, I'll work on moving sm to priv and getting rid of flags from
priv.  Will submit a new patch with appropriate patch name and not
a v2 of this patch since the scope of changes have changed.

Thanks for your comments and feedback.

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

Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jarkko Sakkinen]

On Fri, Aug 25, 2017 at 12:28:21PM -0500, Jiandi An wrote:
> 
> 
> On 08/25/2017 11:53 AM, Jason Gunthorpe wrote:
> > On Fri, Aug 25, 2017 at 07:21:39PM +0300, Jarkko Sakkinen wrote:
> > > As I said before it would make much more sense to make code always deal
> > > with sm and remove flags completely. That would help maintaining code
> > > easier as new logic for TZ is introduced.
> > 
> > Yes please
> > 
> > Jason
> > 
> Okay, I'll work on moving sm to priv and getting rid of flags from
> priv.  Will submit a new patch with appropriate patch name and not
> a v2 of this patch since the scope of changes have changed.
> 
> Thanks for your comments and feedback.

Great

/Jarkko