Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1535471 > unrolled thread

[PATCH v2 0/3] Locality support for tpm_crb

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] Locality support for tpm_crb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-03 19:00 +0100
    [PATCH v2 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-03 19:00 +0100
      Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-03 19:50 +0100
        RE: [tpmdd-devel] [PATCH v2 3/3] tpm_crb: request and relinquish         locality 0 "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-12-05 23:00 +0100
          Re: [tpmdd-devel] [PATCH v2 3/3] tpm_crb: request and relinquish  locality 0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-06 19:50 +0100
      Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0 Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-12-05 17:40 +0100
        Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-06 19:50 +0100
    Re: [PATCH v2 0/3] Locality support for tpm_crb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-12-03 19:50 +0100

#1535471 — [PATCH v2 0/3] Locality support for tpm_crb

The tpm_crb driver should follow the policy of reserving and
relinquishing the locality it uses when multiple localities are used,
like when TXT is another locality.

Jarkko Sakkinen (3):
  tpm_crb: map locality registers
  tpm_crb: encapsulate crb_wait_for_reg_32
  tpm_crb: request and relinquish locality 0

 drivers/char/tpm/tpm_crb.c | 159 ++++++++++++++++++++++++++++++---------------
 1 file changed, 106 insertions(+), 53 deletions(-)

-- 
v2: added sanity check for start address of the control area
2.9.3

[toc] | [next] | [standalone]

#1535472 — [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

Request and relinquish locality for the driver use in order to be
a better citizen in a multi locality environment like TXT. The
locality is requested and relinquished as part of going into and
waking up from idle.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index f986d02..f6e0beb 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,6 +34,15 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
+enum crb_loc_ctrl {
+	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
+	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
+};
+
+enum crb_loc_state {
+	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
+};
+
 enum crb_ctrl_req {
 	CRB_CTRL_REQ_CMD_READY	= BIT(0),
 	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
@@ -101,12 +110,8 @@ struct crb_priv {
  * @dev:  crb device
  * @priv: crb private data
  *
- * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
- * The device should respond within TIMEOUT_C by clearing the bit.
- * Anyhow, we do not wait here as a consequent CMD_READY request
- * will be handled correctly even if idle was not completed.
- *
- * The function does nothing for devices with ACPI-start method.
+ * Put device to the idle state and relinquish locality. The function does
+ * nothing for devices with the ACPI-start method.
  *
  * Return: 0 always
  */
@@ -115,6 +120,7 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 	if (priv->flags & CRB_FL_ACPI_START)
 		return 0;
 
+	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
 	/* we don't really care when this settles */
 
@@ -146,11 +152,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
  * @dev:  crb device
  * @priv: crb private data
  *
- * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
- * and poll till the device acknowledge it by clearing the bit.
- * The device should respond within TIMEOUT_C.
- *
- * The function does nothing for devices with ACPI-start method
+ * Try to wake up the device and request locality. The function does nothing
+ * for devices with the ACPI-start method.
  *
  * Return: 0 on success -ETIME on timeout;
  */
@@ -165,7 +168,16 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 				 CRB_CTRL_REQ_CMD_READY /* mask */,
 				 0, /* value */
 				 TPM2_TIMEOUT_C)) {
-		dev_warn(dev, "cmdReady timed out\n");
+		dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed out\n");
+		return -ETIME;
+	}
+
+	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
+				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
+				 TPM2_TIMEOUT_C)) {
+		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
 		return -ETIME;
 	}
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1535500 — Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

CC linux-security-module

On Sat, Dec 03, 2016 at 07:52:13PM +0200, Jarkko Sakkinen wrote:
> Request and relinquish locality for the driver use in order to be
> a better citizen in a multi locality environment like TXT. The
> locality is requested and relinquished as part of going into and
> waking up from idle.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index f986d02..f6e0beb 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,6 +34,15 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> +enum crb_loc_ctrl {
> +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> +};
> +
> +enum crb_loc_state {
> +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> +};
> +
>  enum crb_ctrl_req {
>  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
>  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> @@ -101,12 +110,8 @@ struct crb_priv {
>   * @dev:  crb device
>   * @priv: crb private data
>   *
> - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> - * The device should respond within TIMEOUT_C by clearing the bit.
> - * Anyhow, we do not wait here as a consequent CMD_READY request
> - * will be handled correctly even if idle was not completed.
> - *
> - * The function does nothing for devices with ACPI-start method.
> + * Put device to the idle state and relinquish locality. The function does
> + * nothing for devices with the ACPI-start method.
>   *
>   * Return: 0 always
>   */
> @@ -115,6 +120,7 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
>  	if (priv->flags & CRB_FL_ACPI_START)
>  		return 0;
>  
> +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
>  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>  	/* we don't really care when this settles */
>  
> @@ -146,11 +152,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>   * @dev:  crb device
>   * @priv: crb private data
>   *
> - * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> - * and poll till the device acknowledge it by clearing the bit.
> - * The device should respond within TIMEOUT_C.
> - *
> - * The function does nothing for devices with ACPI-start method
> + * Try to wake up the device and request locality. The function does nothing
> + * for devices with the ACPI-start method.
>   *
>   * Return: 0 on success -ETIME on timeout;
>   */
> @@ -165,7 +168,16 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  				 CRB_CTRL_REQ_CMD_READY /* mask */,
>  				 0, /* value */
>  				 TPM2_TIMEOUT_C)) {
> -		dev_warn(dev, "cmdReady timed out\n");
> +		dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed out\n");
> +		return -ETIME;
> +	}
> +
> +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
> +				 TPM2_TIMEOUT_C)) {
> +		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
>  		return -ETIME;
>  	}
>  
> -- 
> 2.9.3
> 

[toc] | [prev] | [next] | [standalone]

#1536466 — RE: [tpmdd-devel] [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

> 
> CC linux-security-module
> 
> On Sat, Dec 03, 2016 at 07:52:13PM +0200, Jarkko Sakkinen wrote:
> > Request and relinquish locality for the driver use in order to be a
> > better citizen in a multi locality environment like TXT. The locality
> > is requested and relinquished as part of going into and waking up from
> > idle.
> 
This has nothing to do with power management, please note, that this is run via runtime_pm handlers, 
which can be disabled  via sysfs and actually on server platforms runtime pm is not relevant, 
while you cannot disable locality acquire/relinquish flow.

NACK
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index f986d02..f6e0beb 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -34,6 +34,15 @@ enum crb_defaults {
> >  	CRB_ACPI_START_INDEX = 1,
> >  };
> >
> > +enum crb_loc_ctrl {
> > +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> > +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> > +};
> > +
> > +enum crb_loc_state {
> > +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> > +};
> > +
> >  enum crb_ctrl_req {
> >  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
> >  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> > @@ -101,12 +110,8 @@ struct crb_priv {
> >   * @dev:  crb device
> >   * @priv: crb private data
> >   *
> > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > - * The device should respond within TIMEOUT_C by clearing the bit.
> > - * Anyhow, we do not wait here as a consequent CMD_READY request
> > - * will be handled correctly even if idle was not completed.
> > - *
> > - * The function does nothing for devices with ACPI-start method.
> > + * Put device to the idle state and relinquish locality. The function
> > + does
> > + * nothing for devices with the ACPI-start method.
> >   *
> >   * Return: 0 always
> >   */
> > @@ -115,6 +120,7 @@ static int __maybe_unused crb_go_idle(struct device
> *dev, struct crb_priv *priv)
> >  	if (priv->flags & CRB_FL_ACPI_START)
> >  		return 0;
> >
> > +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);	
This is misleading this has nothing to do with go idle,  what is the 
> >  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> >  	/* we don't really care when this settles */
> >
> > @@ -146,11 +152,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg,
> u32 mask, u32 value,
> >   * @dev:  crb device
> >   * @priv: crb private data
> >   *
> > - * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > - * and poll till the device acknowledge it by clearing the bit.
> > - * The device should respond within TIMEOUT_C.
> > - *
> > - * The function does nothing for devices with ACPI-start method
> > + * Try to wake up the device and request locality. The function does
> > + nothing
> > + * for devices with the ACPI-start method.
> >   *
> >   * Return: 0 on success -ETIME on timeout;
> >   */
> > @@ -165,7 +168,16 @@ static int __maybe_unused crb_cmd_ready(struct
> device *dev,
> >  				 CRB_CTRL_REQ_CMD_READY /* mask */,
> >  				 0, /* value */
> >  				 TPM2_TIMEOUT_C)) {
> > -		dev_warn(dev, "cmdReady timed out\n");
> > +		dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed
> out\n");
We are always in locality 0 here, right?

> > +		return -ETIME;
> > +	}
> > +
> > +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h-
> >loc_ctrl);
> > +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
> > +				 TPM2_TIMEOUT_C)) {
> > +		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed
> out\n");


> >  		return -ETIME;
> >  	}
 

[toc] | [prev] | [next] | [standalone]

#1537207 — Re: [tpmdd-devel] [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

On Mon, Dec 05, 2016 at 09:51:42PM +0000, Winkler, Tomas wrote:
> > 
> > CC linux-security-module
> > 
> > On Sat, Dec 03, 2016 at 07:52:13PM +0200, Jarkko Sakkinen wrote:
> > > Request and relinquish locality for the driver use in order to be a
> > > better citizen in a multi locality environment like TXT. The locality
> > > is requested and relinquished as part of going into and waking up from
> > > idle.
> > 
> This has nothing to do with power management, please note, that this is run via runtime_pm handlers, 
> which can be disabled  via sysfs and actually on server platforms runtime pm is not relevant, 
> while you cannot disable locality acquire/relinquish flow.
> 
> NACK

Good point. I'll move it as part of tpm_transmit()

/Jarkko

> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index f986d02..f6e0beb 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -34,6 +34,15 @@ enum crb_defaults {
> > >  	CRB_ACPI_START_INDEX = 1,
> > >  };
> > >
> > > +enum crb_loc_ctrl {
> > > +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> > > +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> > > +};
> > > +
> > > +enum crb_loc_state {
> > > +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> > > +};
> > > +
> > >  enum crb_ctrl_req {
> > >  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
> > >  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> > > @@ -101,12 +110,8 @@ struct crb_priv {
> > >   * @dev:  crb device
> > >   * @priv: crb private data
> > >   *
> > > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > - * The device should respond within TIMEOUT_C by clearing the bit.
> > > - * Anyhow, we do not wait here as a consequent CMD_READY request
> > > - * will be handled correctly even if idle was not completed.
> > > - *
> > > - * The function does nothing for devices with ACPI-start method.
> > > + * Put device to the idle state and relinquish locality. The function
> > > + does
> > > + * nothing for devices with the ACPI-start method.
> > >   *
> > >   * Return: 0 always
> > >   */
> > > @@ -115,6 +120,7 @@ static int __maybe_unused crb_go_idle(struct device
> > *dev, struct crb_priv *priv)
> > >  	if (priv->flags & CRB_FL_ACPI_START)
> > >  		return 0;
> > >
> > > +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);	
> This is misleading this has nothing to do with go idle,  what is the 
> > >  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
> > >  	/* we don't really care when this settles */
> > >
> > > @@ -146,11 +152,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg,
> > u32 mask, u32 value,
> > >   * @dev:  crb device
> > >   * @priv: crb private data
> > >   *
> > > - * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > - * and poll till the device acknowledge it by clearing the bit.
> > > - * The device should respond within TIMEOUT_C.
> > > - *
> > > - * The function does nothing for devices with ACPI-start method
> > > + * Try to wake up the device and request locality. The function does
> > > + nothing
> > > + * for devices with the ACPI-start method.
> > >   *
> > >   * Return: 0 on success -ETIME on timeout;
> > >   */
> > > @@ -165,7 +168,16 @@ static int __maybe_unused crb_cmd_ready(struct
> > device *dev,
> > >  				 CRB_CTRL_REQ_CMD_READY /* mask */,
> > >  				 0, /* value */
> > >  				 TPM2_TIMEOUT_C)) {
> > > -		dev_warn(dev, "cmdReady timed out\n");
> > > +		dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed
> > out\n");
> We are always in locality 0 here, right?
> 
> > > +		return -ETIME;
> > > +	}
> > > +
> > > +	iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h-
> > >loc_ctrl);
> > > +	if (!crb_wait_for_reg_32(&priv->regs_h->loc_state,
> > > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* mask */
> > > +				 CRB_LOC_STATE_LOC_ASSIGNED, /* value */
> > > +				 TPM2_TIMEOUT_C)) {
> > > +		dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed
> > out\n");
> 
> 
> > >  		return -ETIME;
> > >  	}
>  
> 

[toc] | [prev] | [next] | [standalone]

#1536233 — Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

On Sat, Dec 03, 2016 at 07:52:13PM +0200, Jarkko Sakkinen wrote:
> +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);

Since regs_h can be null shouldn't there be some guards ?

Jason

[toc] | [prev] | [next] | [standalone]

#1537205 — Re: [PATCH v2 3/3] tpm_crb: request and relinquish locality 0

On Mon, Dec 05, 2016 at 09:32:35AM -0700, Jason Gunthorpe wrote:
> On Sat, Dec 03, 2016 at 07:52:13PM +0200, Jarkko Sakkinen wrote:
> > +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> 
> Since regs_h can be null shouldn't there be some guards ?

Yes, there should. Thanks for pointing this out!

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1535498

CC linux-security-module

On Sat, Dec 03, 2016 at 07:52:10PM +0200, Jarkko Sakkinen wrote:
> The tpm_crb driver should follow the policy of reserving and
> relinquishing the locality it uses when multiple localities are used,
> like when TXT is another locality.
> 
> Jarkko Sakkinen (3):
>   tpm_crb: map locality registers
>   tpm_crb: encapsulate crb_wait_for_reg_32
>   tpm_crb: request and relinquish locality 0
> 
>  drivers/char/tpm/tpm_crb.c | 159 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 106 insertions(+), 53 deletions(-)
> 
> -- 
> v2: added sanity check for start address of the control area
> 2.9.3
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web