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