* [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method @ 2017-08-18 4:15 Jiandi An 2017-08-19 17:05 ` Jarkko Sakkinen 2017-08-22 17:39 ` Jarkko Sakkinen 0 siblings, 2 replies; 13+ messages in thread From: Jiandi An @ 2017-08-18 4:15 UTC (permalink / raw) To: peterhuewe, tpmdd, jarkko.sakkinen, jgunthorpe Cc: tpmdd-devel, linux-kernel, 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-18 4:15 [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method Jiandi An @ 2017-08-19 17:05 ` Jarkko Sakkinen 2017-08-21 3:41 ` Jiandi An 2017-08-22 17:39 ` Jarkko Sakkinen 1 sibling, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-19 17:05 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-19 17:05 ` Jarkko Sakkinen @ 2017-08-21 3:41 ` Jiandi An 2017-08-22 17:36 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jiandi An @ 2017-08-21 3:41 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-21 3:41 ` Jiandi An @ 2017-08-22 17:36 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-22 17:36 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-18 4:15 [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method Jiandi An 2017-08-19 17:05 ` Jarkko Sakkinen @ 2017-08-22 17:39 ` Jarkko Sakkinen 2017-08-22 21:28 ` Jiandi An 1 sibling, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-22 17:39 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-22 17:39 ` Jarkko Sakkinen @ 2017-08-22 21:28 ` Jiandi An [not found] ` <d88e255c-f8c9-b6fc-64bd-8cf56153fcce-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-08-24 12:26 ` Jarkko Sakkinen 0 siblings, 2 replies; 13+ messages in thread From: Jiandi An @ 2017-08-22 21:28 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <d88e255c-f8c9-b6fc-64bd-8cf56153fcce-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method [not found] ` <d88e255c-f8c9-b6fc-64bd-8cf56153fcce-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2017-08-23 2:25 ` Jason Gunthorpe 0 siblings, 0 replies; 13+ messages in thread From: Jason Gunthorpe @ 2017-08-23 2:25 UTC (permalink / raw) To: Jiandi An Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jarkko Sakkinen 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-22 21:28 ` Jiandi An [not found] ` <d88e255c-f8c9-b6fc-64bd-8cf56153fcce-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2017-08-24 12:26 ` Jarkko Sakkinen 2017-08-24 17:20 ` Jiandi An 1 sibling, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-24 12:26 UTC (permalink / raw) To: Jiandi An; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-24 12:26 ` Jarkko Sakkinen @ 2017-08-24 17:20 ` Jiandi An 2017-08-25 16:21 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jiandi An @ 2017-08-24 17:20 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-24 17:20 ` Jiandi An @ 2017-08-25 16:21 ` Jarkko Sakkinen 2017-08-25 16:53 ` Jason Gunthorpe 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-25 16:21 UTC (permalink / raw) To: Jiandi An Cc: peterhuewe, tpmdd, jgunthorpe, tpmdd-devel, linux-kernel, linux-security-module 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-25 16:21 ` Jarkko Sakkinen @ 2017-08-25 16:53 ` Jason Gunthorpe 2017-08-25 17:28 ` Jiandi An 0 siblings, 1 reply; 13+ messages in thread From: Jason Gunthorpe @ 2017-08-25 16:53 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Jiandi An, peterhuewe, tpmdd, tpmdd-devel, linux-kernel, linux-security-module 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-25 16:53 ` Jason Gunthorpe @ 2017-08-25 17:28 ` Jiandi An 2017-08-25 17:35 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jiandi An @ 2017-08-25 17:28 UTC (permalink / raw) To: Jason Gunthorpe, Jarkko Sakkinen Cc: peterhuewe, tpmdd, tpmdd-devel, linux-kernel, linux-security-module 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method 2017-08-25 17:28 ` Jiandi An @ 2017-08-25 17:35 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2017-08-25 17:35 UTC (permalink / raw) To: Jiandi An Cc: Jason Gunthorpe, peterhuewe, tpmdd, tpmdd-devel, linux-kernel, linux-security-module 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-08-25 17:35 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-18 4:15 [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method Jiandi An
2017-08-19 17:05 ` Jarkko Sakkinen
2017-08-21 3:41 ` Jiandi An
2017-08-22 17:36 ` Jarkko Sakkinen
2017-08-22 17:39 ` Jarkko Sakkinen
2017-08-22 21:28 ` Jiandi An
[not found] ` <d88e255c-f8c9-b6fc-64bd-8cf56153fcce-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2017-08-23 2:25 ` Jason Gunthorpe
2017-08-24 12:26 ` Jarkko Sakkinen
2017-08-24 17:20 ` Jiandi An
2017-08-25 16:21 ` Jarkko Sakkinen
2017-08-25 16:53 ` Jason Gunthorpe
2017-08-25 17:28 ` Jiandi An
2017-08-25 17:35 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).