[PATCH RFC] tpm_crb: excluding locality registers for ARM64

[PATCH RFC] tpm_crb: excluding locality registers for ARM64

[Jiandi An]

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

Re: [PATCH RFC] tpm_crb: excluding locality registers for ARM64

[Jason Gunthorpe]

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

Re: [PATCH RFC] tpm_crb: excluding locality registers for ARM64

[Jarkko Sakkinen]

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