| Message ID | 1489139889-14376-4-git-send-email-anjiandi@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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
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. >>
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
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;
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(-)