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

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