Re: [PATCH] tpm/tpm_crb: Access locality for non-ACPI and non-SMC start method

[Jiandi An]

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.