* [PATCH RFC] tpm_crb: excluding locality registers for ARM64 @ 2017-07-10 19:52 Jiandi An [not found] ` <8e465692-c445-0f59-5764-9a5ffb4f56f5-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 0 siblings, 1 reply; 3+ messages in thread From: Jiandi An @ 2017-07-10 19:52 UTC (permalink / raw) To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f Cc: harba-sgV2jX0FEOL9JmXXK+q4OQ, anjiandi-sgV2jX0FEOL9JmXXK+q4OQ Hi Guys, In this patch https://patchwork.kernel.org/patch/9562247/ Are the locality registers in crb_regs_head specific to x86? On ARM64, we don't have locality registers before control area in ACPI IO region. We are hitting this check and getting the FW_BUG "Bad ACPI memory layout" log. + /* The ACPI IO region starts at the head area and continues to include + * 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 (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"); + } The quick fix that would make it work for ARM64 is to also exclude CRB_FL_CRB_SMC_START. - if (!(priv->flags & CRB_FL_ACPI_START)) { + if (!(priv->flags & CRB_FL_ACPI_START || + priv->flags & CRB_FL_CRB_SMC_START)) { But would like to understand more why the check was only excluding CRB_FL_ACPI_START and getting opinions on this fix. We don't see locality registers defined in TCG ACPI spec nor how a platform indicates the locality registers are present before control area. I've done similar workaround to bypass x86 specific workaround in common code path. For example, in crb_map_io(), there is a specific PTT HW bug workaround for x86. /* * PTT HW bug w/a: wake up the device to access * possibly not retained registers. */ ret = crb_cmd_ready(dev, priv); if (ret) return ret; crb_cmd_ready() does nothing for devices with ACPI-start method as it does not support goIdle and cmdReady bits and idle state management is not exposed to the host SW. So I've done similar workaround to bypass by additionally excluding CRB_FL_CRB_SMC_START. 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; -- 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. ------------------------------------------------------------------------------ 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] 3+ messages in thread
[parent not found: <8e465692-c445-0f59-5764-9a5ffb4f56f5-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH RFC] tpm_crb: excluding locality registers for ARM64 [not found] ` <8e465692-c445-0f59-5764-9a5ffb4f56f5-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2017-07-10 20:29 ` Jason Gunthorpe [not found] ` <20170710202957.GA19948-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 0 siblings, 1 reply; 3+ messages in thread From: Jason Gunthorpe @ 2017-07-10 20:29 UTC (permalink / raw) To: Jiandi An Cc: harba-sgV2jX0FEOL9JmXXK+q4OQ, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f On Mon, Jul 10, 2017 at 02:52:34PM -0500, Jiandi An wrote: > > Hi Guys, > > In this patch https://patchwork.kernel.org/patch/9562247/ > Are the locality registers in crb_regs_head specific to x86? IMHO, the ACPI specification for TPM2 has been a disaster. The spec is too vauge and weird - this buisness with storing addresses inside a buffer inside a memory map is insane. This is allowing implementations to do all manner of crazy things that make no sense at all. > The quick fix that would make it work for ARM64 is to also exclude > CRB_FL_CRB_SMC_START. Well, when the ACPI extension for ACPI_TPM2_COMMAND_BUFFER_WITH_SMC was defined, it needs to specify how to handle locality. If it is done via the registers in crb_regs_head then the ACPI spec needs to define how to locate those registers. I suspect the test you pointed out is just poorly constructed: if (!(priv->flags & CRB_FL_ACPI_START)) { It should be if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED) As those are the only two modes to define the register layout in this way. But, you will still need to implement locality support for ACPI_TPM2_COMMAND_BUFFER_WITH_SMC. > For example, in crb_map_io(), there is a specific PTT HW bug workaround for > x86. > > /* > * PTT HW bug w/a: wake up the device to access > * possibly not retained registers. > */ > ret = crb_cmd_ready(dev, priv); > if (ret) > return ret; > > crb_cmd_ready() does nothing for devices with ACPI-start method as it does > not support goIdle and cmdReady bits and idle state management is not > exposed to the host SW. So I've done similar workaround to bypass by > additionally excluding CRB_FL_CRB_SMC_START. Again, I think these tests are backwards, a work around like this should be a while list, not a black list.. So it should refer to exactly the sm values the impacted chipsets would use. 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] 3+ messages in thread
[parent not found: <20170710202957.GA19948-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]
* Re: [PATCH RFC] tpm_crb: excluding locality registers for ARM64 [not found] ` <20170710202957.GA19948-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> @ 2017-07-16 11:16 ` Jarkko Sakkinen 0 siblings, 0 replies; 3+ messages in thread From: Jarkko Sakkinen @ 2017-07-16 11:16 UTC (permalink / raw) To: Jason Gunthorpe, Jiandi An Cc: harba-sgV2jX0FEOL9JmXXK+q4OQ, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f On Mon, 2017-07-10 at 14:29 -0600, Jason Gunthorpe wrote: > On Mon, Jul 10, 2017 at 02:52:34PM -0500, Jiandi An wrote: > > > > Hi Guys, > > > > In this patch https://patchwork.kernel.org/patch/9562247/ > > Are the locality registers in crb_regs_head specific to x86? > > IMHO, the ACPI specification for TPM2 has been a disaster. The spec > is > too vauge and weird - this buisness with storing addresses inside a > buffer inside a memory map is insane. > > This is allowing implementations to do all manner of crazy things > that > make no sense at all. > > > The quick fix that would make it work for ARM64 is to also exclude > > CRB_FL_CRB_SMC_START. > > Well, when the ACPI extension for ACPI_TPM2_COMMAND_BUFFER_WITH_SMC > was defined, it needs to specify how to handle locality. If it is > done > via the registers in crb_regs_head then the ACPI spec needs to define > how to locate those registers. > > I suspect the test you pointed out is just poorly constructed: > > if (!(priv->flags & CRB_FL_ACPI_START)) { > > It should be > > if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == > ACPI_TPM2_MEMORY_MAPPED) > > As those are the only two modes to define the register layout in > this way. > > But, you will still need to implement locality support for > ACPI_TPM2_COMMAND_BUFFER_WITH_SMC. > > > For example, in crb_map_io(), there is a specific PTT HW bug > > workaround for > > x86. > > > > /* > > * PTT HW bug w/a: wake up the device to access > > * possibly not retained registers. > > */ > > ret = crb_cmd_ready(dev, priv); > > if (ret) > > return ret; > > > > crb_cmd_ready() does nothing for devices with ACPI-start method as > > it does > > not support goIdle and cmdReady bits and idle state management is > > not > > exposed to the host SW. So I've done similar workaround to bypass > > by > > additionally excluding CRB_FL_CRB_SMC_START. > > Again, I think these tests are backwards, a work around like this > should be a while list, not a black list.. So it should refer to > exactly the sm values the impacted chipsets would use. > > Jason I happen have a NUC that reports ACPI_TPM2_START_METHOD but still requires that bit to be set always before invoking the ACPI: https://lkml.org/lkml/2016/10/20/417 /Jarkko ------------------------------------------------------------------------------ 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] 3+ messages in thread
end of thread, other threads:[~2017-07-16 11:16 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-10 19:52 [PATCH RFC] tpm_crb: excluding locality registers for ARM64 Jiandi An [not found] ` <8e465692-c445-0f59-5764-9a5ffb4f56f5-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-07-10 20:29 ` Jason Gunthorpe [not found] ` <20170710202957.GA19948-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2017-07-16 11:16 ` 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).