[PATCH RFC 0/3] Locality support for the CRB driver

[PATCH RFC 0/3] Locality support for the CRB driver

[Jarkko Sakkinen]

Request and relinquish locality 0 so that the CRB driver is a good
citizen in a multi locality environment like TXT.

Jarkko Sakkinen (3):
  tpm_crb: expand struct crb_control_area to struct crb_regs
  tpm_crb: encapsulate crb_wait_for_reg_32
  tpm_crb: request and relinquish locality 0

 drivers/char/tpm/tpm_crb.c | 148 ++++++++++++++++++++++++++++-----------------
 1 file changed, 92 insertions(+), 56 deletions(-)

-- 
2.7.4

[PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

In order to allow to use locality 0, expand the data structure to expose
all of the CRB registers. The address is calculated from the control
area address in order to retain backwards compatibility to ACPI start
based hardware (pre-Skylake).

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

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 65040d7..3d021e6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -52,18 +52,26 @@ enum crb_cancel {
 	CRB_CANCEL_INVOKE	= BIT(0),
 };
 
-struct crb_control_area {
-	u32 req;
-	u32 sts;
-	u32 cancel;
-	u32 start;
-	u32 int_enable;
-	u32 int_sts;
-	u32 cmd_size;
-	u32 cmd_pa_low;
-	u32 cmd_pa_high;
-	u32 rsp_size;
-	u64 rsp_pa;
+struct crb_regs {
+	u32 loc_state;
+	u32 reserved1;
+	u32 loc_ctrl;
+	u32 loc_sts;
+	u8 reserved2[32];
+	u64 intf_id;
+	u64 ctrl_ext;
+	/* the control area */
+	u32 ctrl_req;
+	u32 ctrl_sts;
+	u32 ctrl_cancel;
+	u32 ctrl_start;
+	u32 ctrl_int_enable;
+	u32 ctrl_int_sts;
+	u32 ctrl_cmd_size;
+	u32 ctrl_cmd_pa_low;
+	u32 ctrl_cmd_pa_high;
+	u32 ctrl_rsp_size;
+	u64 ctrl_rsp_pa;
 } __packed;
 
 enum crb_status {
@@ -78,7 +86,7 @@ enum crb_flags {
 struct crb_priv {
 	unsigned int flags;
 	void __iomem *iobase;
-	struct crb_control_area __iomem *cca;
+	struct crb_regs __iomem *regs;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
 	u32 cmd_size;
@@ -104,7 +112,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_CTRL_REQ_GO_IDLE, &priv->cca->req);
+	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs->ctrl_req);
 	/* we don't really care when this settles */
 
 	return 0;
@@ -128,16 +136,18 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 					struct crb_priv *priv)
 {
 	ktime_t stop, start;
+	u32 req;
 
 	if (priv->flags & CRB_FL_ACPI_START)
 		return 0;
 
-	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req);
+	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs->ctrl_req);
 
 	start = ktime_get();
 	stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C));
 	do {
-		if (!(ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY)) {
+		req = ioread32(&priv->regs->ctrl_req);
+		if (!(req & CRB_CTRL_REQ_CMD_READY)) {
 			dev_dbg(dev, "cmdReady in %lld usecs\n",
 				ktime_to_us(ktime_sub(ktime_get(), start)));
 			return 0;
@@ -145,7 +155,7 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 		usleep_range(50, 100);
 	} while (ktime_before(ktime_get(), stop));
 
-	if (ioread32(&priv->cca->req) & CRB_CTRL_REQ_CMD_READY) {
+	if (ioread32(&priv->regs->ctrl_req) & CRB_CTRL_REQ_CMD_READY) {
 		dev_warn(dev, "cmdReady timed out\n");
 		return -ETIME;
 	}
@@ -158,7 +168,7 @@ static u8 crb_status(struct tpm_chip *chip)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	u8 sts = 0;
 
-	if ((ioread32(&priv->cca->start) & CRB_START_INVOKE) !=
+	if ((ioread32(&priv->regs->ctrl_start) & CRB_START_INVOKE) !=
 	    CRB_START_INVOKE)
 		sts |= CRB_DRV_STS_COMPLETE;
 
@@ -174,7 +184,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	if (count < 6)
 		return -EIO;
 
-	if (ioread32(&priv->cca->sts) & CRB_CTRL_STS_ERROR)
+	if (ioread32(&priv->regs->ctrl_sts) & CRB_CTRL_STS_ERROR)
 		return -EIO;
 
 	memcpy_fromio(buf, priv->rsp, 6);
@@ -213,7 +223,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	/* Zero the cancel register so that the next command will not get
 	 * canceled.
 	 */
-	iowrite32(0, &priv->cca->cancel);
+	iowrite32(0, &priv->regs->ctrl_cancel);
 
 	if (len > priv->cmd_size) {
 		dev_err(&chip->dev, "invalid command count value %zd %d\n",
@@ -227,7 +237,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	wmb();
 
 	if (priv->flags & CRB_FL_CRB_START)
-		iowrite32(CRB_START_INVOKE, &priv->cca->start);
+		iowrite32(CRB_START_INVOKE, &priv->regs->ctrl_start);
 
 	if (priv->flags & CRB_FL_ACPI_START)
 		rc = crb_do_acpi_start(chip);
@@ -239,7 +249,7 @@ static void crb_cancel(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 
-	iowrite32(CRB_CANCEL_INVOKE, &priv->cca->cancel);
+	iowrite32(CRB_CANCEL_INVOKE, &priv->regs->ctrl_cancel);
 
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
@@ -248,7 +258,7 @@ static void crb_cancel(struct tpm_chip *chip)
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
-	u32 cancel = ioread32(&priv->cca->cancel);
+	u32 cancel = ioread32(&priv->regs->ctrl_cancel);
 
 	return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
 }
@@ -302,6 +312,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
+	void *ctrl;
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
@@ -325,10 +336,17 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (IS_ERR(priv->iobase))
 		return PTR_ERR(priv->iobase);
 
-	priv->cca = crb_map_res(dev, priv, &io_res, buf->control_address,
-				sizeof(struct crb_control_area));
-	if (IS_ERR(priv->cca))
-		return PTR_ERR(priv->cca);
+	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
+			   sizeof(struct crb_regs) -
+			   offsetof(struct crb_regs, ctrl_req));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	/* The control area always overrlaps IO memory mapped from the ACPI
+	 * object with CRB start only devices. Thus, this is perfectly safe.
+	 */
+	priv->regs = (void *)((unsigned long)ctrl -
+		offsetof(struct crb_regs, ctrl_req));
 
 	/*
 	 * PTT HW bug w/a: wake up the device to access
@@ -338,10 +356,10 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (ret)
 		return ret;
 
-	pa_high = ioread32(&priv->cca->cmd_pa_high);
-	pa_low  = ioread32(&priv->cca->cmd_pa_low);
+	pa_high = ioread32(&priv->regs->ctrl_cmd_pa_high);
+	pa_low  = ioread32(&priv->regs->ctrl_cmd_pa_low);
 	cmd_pa = ((u64)pa_high << 32) | pa_low;
-	cmd_size = ioread32(&priv->cca->cmd_size);
+	cmd_size = ioread32(&priv->regs->ctrl_cmd_size);
 
 	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
 		pa_high, pa_low, cmd_size);
@@ -352,9 +370,9 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		goto out;
 	}
 
-	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
+	memcpy_fromio(&rsp_pa, &priv->regs->ctrl_rsp_pa, 8);
 	rsp_pa = le64_to_cpu(rsp_pa);
-	rsp_size = ioread32(&priv->cca->rsp_size);
+	rsp_size = ioread32(&priv->regs->ctrl_rsp_size);
 
 	if (cmd_pa != rsp_pa) {
 		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
-- 
2.7.4

[PATCH RFC 2/3] tpm_crb: encapsulate crb_wait_for_reg_32

[Jarkko Sakkinen]

Encapsulated crb_wait_for_reg32() so that state changes in other CRB
registers than CTRL_REQ_X can be waited.

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

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3d021e6..ffd3a6c 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -118,6 +118,25 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 	return 0;
 }
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+				unsigned long timeout)
+{
+	ktime_t start;
+	ktime_t stop;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(timeout));
+
+	do {
+		if ((ioread32(reg) & mask) == value)
+			return true;
+
+		usleep_range(50, 100);
+	} while (ktime_before(ktime_get(), stop));
+
+	return false;
+}
+
 /**
  * crb_cmd_ready - request tpm crb device to enter ready state
  *
@@ -135,27 +154,14 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)
 static int __maybe_unused crb_cmd_ready(struct device *dev,
 					struct crb_priv *priv)
 {
-	ktime_t stop, start;
-	u32 req;
-
 	if (priv->flags & CRB_FL_ACPI_START)
 		return 0;
 
 	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs->ctrl_req);
-
-	start = ktime_get();
-	stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C));
-	do {
-		req = ioread32(&priv->regs->ctrl_req);
-		if (!(req & CRB_CTRL_REQ_CMD_READY)) {
-			dev_dbg(dev, "cmdReady in %lld usecs\n",
-				ktime_to_us(ktime_sub(ktime_get(), start)));
-			return 0;
-		}
-		usleep_range(50, 100);
-	} while (ktime_before(ktime_get(), stop));
-
-	if (ioread32(&priv->regs->ctrl_req) & CRB_CTRL_REQ_CMD_READY) {
+	if (!crb_wait_for_reg_32(&priv->regs->ctrl_req,
+				 CRB_CTRL_REQ_CMD_READY /* mask */,
+				 0, /* value */
+				 TPM2_TIMEOUT_C)) {
 		dev_warn(dev, "cmdReady timed out\n");
 		return -ETIME;
 	}
-- 
2.7.4

[PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Jarkko Sakkinen]

Request and relinquish locality for the driver use in order to be
a better citizen in a multi locality environment like with TXT as
it uses locality 2.

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 ffd3a6c..9e07cf3 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),
@@ -98,12 +107,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
  */
@@ -112,6 +117,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->loc_ctrl);
 	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs->ctrl_req);
 	/* we don't really care when this settles */
 
@@ -143,11 +149,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;
  */
@@ -162,7 +165,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->loc_ctrl);
+	if (!crb_wait_for_reg_32(&priv->regs->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.7.4

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> +			   sizeof(struct crb_regs) -
> +			   offsetof(struct crb_regs, ctrl_req));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	/* The control area always overrlaps IO memory mapped from the ACPI
> +	 * object with CRB start only devices. Thus, this is perfectly safe.
> +	 */
> +	priv->regs = (void *)((unsigned long)ctrl -
> +		offsetof(struct crb_regs, ctrl_req));

Hum. No, this makes bad assumptions about the structure of iomapping.

The map itself needs to be done with the adjustment:

	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
		offsetof(struct crb_regs, ctrl_req),
	   	sizeof(struct crb_regs));

.. and nothing actually proves that control_address follows anything
in the driver, so this seems like a terrifying blind assumption, but
everything about the iomap in this ACPI binding seems totally bonkers
so that is in good company I guess.

.. and the comment says this only holds for 'crb start only' devices,
but the code doesn't actually act differently based on what sort of
device we have..

Your commit message also seems to imply the new registers are only on
newer hardware, but nothing seems to check for that before acessing
them?  Confusing.

Jason

RE: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Winkler, Tomas]

> 
> Request and relinquish locality for the driver use in order to be a better citizen
> in a multi locality environment like with TXT as it uses locality 2.	

> 
> 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
> ffd3a6c..9e07cf3 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),
> @@ -98,12 +107,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
>   */
> @@ -112,6 +117,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->loc_ctrl);


Please don't mix different functionalities in one function 
Also those functions are called from runtime pm, this has nothing to do with the power management 

>  	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs->ctrl_req);
>  	/* we don't really care when this settles */
> 
> @@ -143,11 +149,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;
>   */
> @@ -162,7 +165,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->loc_ctrl);
> +	if (!crb_wait_for_reg_32(&priv->regs->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");
Same here

>  		return -ETIME;
>  	}
> 
> --

Re: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 06:35:28AM +0000, Winkler, Tomas wrote:
> 
> 
> > 
> > Request and relinquish locality for the driver use in order to be a better citizen
> > in a multi locality environment like with TXT as it uses locality 2.	
> 
> > 
> > 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
> > ffd3a6c..9e07cf3 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),
> > @@ -98,12 +107,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
> >   */
> > @@ -112,6 +117,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->loc_ctrl);
> 
> 
> Please don't mix different functionalities in one function 

??

> Also those functions are called from runtime pm, this has nothing to
> do with the power management 

It all depends on granularity. If you want to make an argument, could
you propose a better granularity? Do you think it'd be better to do it
for each transmission?

You are saying that this is all bad without saying really backing up
your statements by any means.

/Jarkko

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > +			   sizeof(struct crb_regs) -
> > +			   offsetof(struct crb_regs, ctrl_req));
> > +	if (IS_ERR(ctrl))
> > +		return PTR_ERR(ctrl);
> > +
> > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > +	 */
> > +	priv->regs = (void *)((unsigned long)ctrl -
> > +		offsetof(struct crb_regs, ctrl_req));
> 
> Hum. No, this makes bad assumptions about the structure of iomapping.
> 
> The map itself needs to be done with the adjustment:
> 
> 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> 		offsetof(struct crb_regs, ctrl_req),
> 	   	sizeof(struct crb_regs));

That would be wrong address for the control area as it does not start
from the beginning of CRB registers.

> .. and nothing actually proves that control_address follows anything
> in the driver, so this seems like a terrifying blind assumption, but
> everything about the iomap in this ACPI binding seems totally bonkers
> so that is in good company I guess.

There are basically two kinds of CRB devices in the wild:

1. ACPI start devices that use DMA
2. CRB MMIO devices

For 1 you always iomap control area. For 2 the ACPI object given buffer
covers the control area.

I think the crb_map_io and crb_map_res are too generic. Better way to do
things would be to validate that assumptions for these two cases hold.

> .. and the comment says this only holds for 'crb start only' devices,
> but the code doesn't actually act differently based on what sort of
> device we have..
> 
> Your commit message also seems to imply the new registers are only on
> newer hardware, but nothing seems to check for that before acessing
> them?  Confusing.

That's why there's this thing called RFC :)

> Jason

/Jarkko

RE: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Winkler, Tomas]

> >
> > >
> > > Request and relinquish locality for the driver use in order to be a better
> citizen
> > > in a multi locality environment like with TXT as it uses locality 2.
> >
> > >
> > > 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
> > > ffd3a6c..9e07cf3 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),
> > > @@ -98,12 +107,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
> > >   */
> > > @@ -112,6 +117,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->loc_ctrl);
> >
> >
> > Please don't mix different functionalities in one function
> 
> ??
> 
> > Also those functions are called from runtime pm, this has nothing to
> > do with the power management
> 
> It all depends on granularity. If you want to make an argument, could you
> propose a better granularity? Do you think it'd be better to do it for each
> transmission?

Not sure, I don't believe we closed the design here with all the parties, you are jumping ahead. 

> You are saying that this is all bad without saying really backing up your
> statements by any means.

You are right,  I assumed it's pretty obvious, I'm taking this to my attention not do it again.

So my point is if you have a function which is named go_idle  it probably does go idle flow and no other things like relinquish functionality that's for code readability and style.
Also you lost degree  of freedom even now you may want perform each of these operation for each tpm request, that might not be true in general, we would like to witch to runtime auto suspend it won't work anymore, for example. Also as you pointed right now we are not clear on the granularity of the locality access. 

Thanks
Tomas

Re: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 09:43:59AM +0000, Winkler, Tomas wrote:
> 
> > >
> > > >
> > > > Request and relinquish locality for the driver use in order to be a better
> > citizen
> > > > in a multi locality environment like with TXT as it uses locality 2.
> > >
> > > >
> > > > 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
> > > > ffd3a6c..9e07cf3 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),
> > > > @@ -98,12 +107,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
> > > >   */
> > > > @@ -112,6 +117,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->loc_ctrl);
> > >
> > >
> > > Please don't mix different functionalities in one function
> > 
> > ??
> > 
> > > Also those functions are called from runtime pm, this has nothing to
> > > do with the power management
> > 
> > It all depends on granularity. If you want to make an argument, could you
> > propose a better granularity? Do you think it'd be better to do it for each
> > transmission?
> 
> Not sure, I don't believe we closed the design here with all the
> parties, you are jumping ahead. 

I also added RFC tag.

I have had this done for a while but it has had a dependency for runtime
PM so I decided to make it available.

> > You are saying that this is all bad without saying really backing up your
> > statements by any means.
> 
> You are right,  I assumed it's pretty obvious, I'm taking this to my
> attention not do it again.
> 
> So my point is if you have a function which is named go_idle  it
> probably does go idle flow and no other things like relinquish
> functionality that's for code readability and style.  Also you lost

When you go to idle you certain tasks before going to sleep. If the
granularity matches, relinquishing locality is one of those tasks.

> degree  of freedom even now you may want perform each of these
> operation for each tpm request, that might not be true in general, we
> would like to witch to runtime auto suspend it won't work anymore, for
> example. Also as you pointed right now we are not clear on the
> granularity of the locality access. 

That's not true. You don't loose any freedoms. You can start with
something simple like once per transmission or once per idle/ready.

If that does not scale, then it must be adjusted. I think this is
really business as usual...

> Thanks
> Tomas

/Jarkko

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote:
> On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> > On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > > +			   sizeof(struct crb_regs) -
> > > +			   offsetof(struct crb_regs, ctrl_req));
> > > +	if (IS_ERR(ctrl))
> > > +		return PTR_ERR(ctrl);
> > > +
> > > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > > +	 */
> > > +	priv->regs = (void *)((unsigned long)ctrl -
> > > +		offsetof(struct crb_regs, ctrl_req));
> > 
> > Hum. No, this makes bad assumptions about the structure of iomapping.
> > 
> > The map itself needs to be done with the adjustment:
> > 
> > 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> > 		offsetof(struct crb_regs, ctrl_req),
> > 	   	sizeof(struct crb_regs));
> 
> That would be wrong address for the control area as it does not start
> from the beginning of CRB registers.

Of course, I just pointed out what the map call should look like

Something like this

 	priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address -
 		offsetof(struct crb_regs, ctrl_req),
 	   	sizeof(struct crb_regs));
        ctrl = &priv->regs.ctrl_req;

> I think the crb_map_io and crb_map_res are too generic. Better way to do
> things would be to validate that assumptions for these two cases hold.

If the driver is going to be using a negative offset like this, then
it very much should validate the assumptions before doing it.

and not even map these regsiters if they are not supported by
hardware.

Jason

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 10:49:05AM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote:
> > On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> > > On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > > > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > > > +			   sizeof(struct crb_regs) -
> > > > +			   offsetof(struct crb_regs, ctrl_req));
> > > > +	if (IS_ERR(ctrl))
> > > > +		return PTR_ERR(ctrl);
> > > > +
> > > > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > > > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > > > +	 */
> > > > +	priv->regs = (void *)((unsigned long)ctrl -
> > > > +		offsetof(struct crb_regs, ctrl_req));
> > > 
> > > Hum. No, this makes bad assumptions about the structure of iomapping.
> > > 
> > > The map itself needs to be done with the adjustment:
> > > 
> > > 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> > > 		offsetof(struct crb_regs, ctrl_req),
> > > 	   	sizeof(struct crb_regs));
> > 
> > That would be wrong address for the control area as it does not start
> > from the beginning of CRB registers.
> 
> Of course, I just pointed out what the map call should look like
> 
> Something like this
> 
>  	priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address -
>  		offsetof(struct crb_regs, ctrl_req),
>  	   	sizeof(struct crb_regs));
>         ctrl = &priv->regs.ctrl_req;
> 
> > I think the crb_map_io and crb_map_res are too generic. Better way to do
> > things would be to validate that assumptions for these two cases hold.
> 
> If the driver is going to be using a negative offset like this, then
> it very much should validate the assumptions before doing it.
> 
> and not even map these regsiters if they are not supported by
> hardware.

Yes, that's what the point I tried to make in my response in the part
that is not quoted here.

/Jarkko

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 10:49:05AM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote:
> > On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> > > On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > > > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > > > +			   sizeof(struct crb_regs) -
> > > > +			   offsetof(struct crb_regs, ctrl_req));
> > > > +	if (IS_ERR(ctrl))
> > > > +		return PTR_ERR(ctrl);
> > > > +
> > > > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > > > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > > > +	 */
> > > > +	priv->regs = (void *)((unsigned long)ctrl -
> > > > +		offsetof(struct crb_regs, ctrl_req));
> > > 
> > > Hum. No, this makes bad assumptions about the structure of iomapping.
> > > 
> > > The map itself needs to be done with the adjustment:
> > > 
> > > 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> > > 		offsetof(struct crb_regs, ctrl_req),
> > > 	   	sizeof(struct crb_regs));
> > 
> > That would be wrong address for the control area as it does not start
> > from the beginning of CRB registers.
> 
> Of course, I just pointed out what the map call should look like
> 
> Something like this
> 
>  	priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address -
>  		offsetof(struct crb_regs, ctrl_req),
>  	   	sizeof(struct crb_regs));
>         ctrl = &priv->regs.ctrl_req;

Sorry I missed this part.

Here are the constraints for existing hardware:

1. All the existing CRB start only hardware has the iomem covering the
   control area and registers for multiple localities.
2. All the existing ACPI start hardware has only the control area.

If you assume that SSDT does not have malicous behavior caused by either
a BIOS bug or maybe a rootkit, then the current patch works for all the
existing hardware.

To counter-measure for unexpected behavior in non-existing hardware and
buggy or malicious firmware it probably make sense to use crb_map_res to
validate the part of the CRB registers that is not part of the control
area.

Doing it in the way you proposed does not work for ACPI start devices.

For them it should be done in the same way as I'm doing in the existing
patch as for ACPI start devices the address below the control area are
never accessed. Having a separate crb_map_res for CRB start only devices
is sane thing to do for validation.

/Jarkko

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 09:32:32PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 09, 2016 at 10:49:05AM -0600, Jason Gunthorpe wrote:
> > On Sun, Oct 09, 2016 at 12:38:18PM +0300, Jarkko Sakkinen wrote:
> > > On Sat, Oct 08, 2016 at 07:42:56PM -0600, Jason Gunthorpe wrote:
> > > > On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> > > > > +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> > > > > +			   sizeof(struct crb_regs) -
> > > > > +			   offsetof(struct crb_regs, ctrl_req));
> > > > > +	if (IS_ERR(ctrl))
> > > > > +		return PTR_ERR(ctrl);
> > > > > +
> > > > > +	/* The control area always overrlaps IO memory mapped from the ACPI
> > > > > +	 * object with CRB start only devices. Thus, this is perfectly safe.
> > > > > +	 */
> > > > > +	priv->regs = (void *)((unsigned long)ctrl -
> > > > > +		offsetof(struct crb_regs, ctrl_req));
> > > > 
> > > > Hum. No, this makes bad assumptions about the structure of iomapping.
> > > > 
> > > > The map itself needs to be done with the adjustment:
> > > > 
> > > > 	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
> > > > 		offsetof(struct crb_regs, ctrl_req),
> > > > 	   	sizeof(struct crb_regs));
> > > 
> > > That would be wrong address for the control area as it does not start
> > > from the beginning of CRB registers.
> > 
> > Of course, I just pointed out what the map call should look like
> > 
> > Something like this
> > 
> >  	priv->regs = crb_map_res(dev, priv, &io_res, buf->control_address -
> >  		offsetof(struct crb_regs, ctrl_req),
> >  	   	sizeof(struct crb_regs));
> >         ctrl = &priv->regs.ctrl_req;
> 
> Sorry I missed this part.
> 
> Here are the constraints for existing hardware:
> 
> 1. All the existing CRB start only hardware has the iomem covering the
>    control area and registers for multiple localities.
> 2. All the existing ACPI start hardware has only the control area.
> 
> If you assume that SSDT does not have malicous behavior caused by either
> a BIOS bug or maybe a rootkit, then the current patch works for all the
> existing hardware.
> 
> To counter-measure for unexpected behavior in non-existing hardware and
> buggy or malicious firmware it probably make sense to use crb_map_res to
> validate the part of the CRB registers that is not part of the control
> area.
> 
> Doing it in the way you proposed does not work for ACPI start devices.
> 
> For them it should be done in the same way as I'm doing in the existing
> patch as for ACPI start devices the address below the control area are
> never accessed. Having a separate crb_map_res for CRB start only devices
> is sane thing to do for validation.

Alternative is to do two structures crb_regs_head and crb_regs_tail,
which might be cleaner. I'm fine with going either route.


/Jarkko

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jason Gunthorpe]

On Sun, Oct 09, 2016 at 09:33:58PM +0300, Jarkko Sakkinen wrote:

> > Sorry I missed this part.
> > 
> > Here are the constraints for existing hardware:
> > 
> > 1. All the existing CRB start only hardware has the iomem covering the
> >    control area and registers for multiple localities.
> > 2. All the existing ACPI start hardware has only the control area.
> > 
> > If you assume that SSDT does not have malicous behavior caused by either
> > a BIOS bug or maybe a rootkit, then the current patch works for all the
> > existing hardware.
> > 
> > To counter-measure for unexpected behavior in non-existing hardware and
> > buggy or malicious firmware it probably make sense to use crb_map_res to
> > validate the part of the CRB registers that is not part of the control
> > area.

I don't know how much I'd assume BIOS authors do what you think - the
spec I saw for this seems very vauge.

Certainly checking that locality region falls within the acpi mapping
seems essential.

> > Doing it in the way you proposed does not work for ACPI start devices.
> > 
> > For them it should be done in the same way as I'm doing in the existing
> > patch as for ACPI start devices the address below the control area are
> > never accessed. Having a separate crb_map_res for CRB start only devices
> > is sane thing to do for validation.
> 
> Alternative is to do two structures crb_regs_head and crb_regs_tail,
> which might be cleaner. I'm fine with going either route.

Since the iomem doesn't actually exist for a configuration having two
pointers is the better choice. Make sure one is null for the
configuration that does not support it.

The negative offset thing is way too subtle.

Jason

RE: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Winkler, Tomas]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com]
> Sent: Monday, October 10, 2016 02:08
> To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: moderated list:TPM DEVICE DRIVER <tpmdd-devel@lists.sourceforge.net>;
> open list <linux-kernel@vger.kernel.org>
> Subject: Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct
> crb_control_area to struct crb_regs
> 
> On Sun, Oct 09, 2016 at 09:33:58PM +0300, Jarkko Sakkinen wrote:
> 
> > > Sorry I missed this part.
> > >
> > > Here are the constraints for existing hardware:
> > >
> > > 1. All the existing CRB start only hardware has the iomem covering the
> > >    control area and registers for multiple localities.
> > > 2. All the existing ACPI start hardware has only the control area.
> > >
> > > If you assume that SSDT does not have malicous behavior caused by
> > > either a BIOS bug or maybe a rootkit, then the current patch works
> > > for all the existing hardware.
> > >
> > > To counter-measure for unexpected behavior in non-existing hardware
> > > and buggy or malicious firmware it probably make sense to use
> > > crb_map_res to validate the part of the CRB registers that is not
> > > part of the control area.
> 
> I don't know how much I'd assume BIOS authors do what you think - the spec I
> saw for this seems very vauge.
> 
> Certainly checking that locality region falls within the acpi mapping seems
> essential.
> 
> > > Doing it in the way you proposed does not work for ACPI start devices.
> > >
> > > For them it should be done in the same way as I'm doing in the
> > > existing patch as for ACPI start devices the address below the
> > > control area are never accessed. Having a separate crb_map_res for
> > > CRB start only devices is sane thing to do for validation.
> >
> > Alternative is to do two structures crb_regs_head and crb_regs_tail,
> > which might be cleaner. I'm fine with going either route.
> 
> Since the iomem doesn't actually exist for a configuration having two pointers
> is the better choice. Make sure one is null for the configuration that does not
> support it.
> 
> The negative offset thing is way too subtle.

I addition I believe it should be always on offset FED4_0xxxh by the Spec, so all this arithmetic is a bit of overkill.
Thanks
Tomas

Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jason Gunthorpe]

On Mon, Oct 10, 2016 at 12:25:11AM +0000, Winkler, Tomas wrote:

> I addition I believe it should be always on offset FED4_0xxxh by the
> Spec, so all this arithmetic is a bit of overkill.

We don't have any hard coded addresses in the drivers - that seems
very inappropriate these days..

Is that address really not discoverable via ACPI somehow?

Jason

Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 05:07:37PM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 09:33:58PM +0300, Jarkko Sakkinen wrote:
> 
> > > Sorry I missed this part.
> > > 
> > > Here are the constraints for existing hardware:
> > > 
> > > 1. All the existing CRB start only hardware has the iomem covering the
> > >    control area and registers for multiple localities.
> > > 2. All the existing ACPI start hardware has only the control area.
> > > 
> > > If you assume that SSDT does not have malicous behavior caused by either
> > > a BIOS bug or maybe a rootkit, then the current patch works for all the
> > > existing hardware.
> > > 
> > > To counter-measure for unexpected behavior in non-existing hardware and
> > > buggy or malicious firmware it probably make sense to use crb_map_res to
> > > validate the part of the CRB registers that is not part of the control
> > > area.
> 
> I don't know how much I'd assume BIOS authors do what you think - the
> spec I saw for this seems very vauge.
> 
> Certainly checking that locality region falls within the acpi mapping
> seems essential.
> 
> > > Doing it in the way you proposed does not work for ACPI start devices.
> > > 
> > > For them it should be done in the same way as I'm doing in the existing
> > > patch as for ACPI start devices the address below the control area are
> > > never accessed. Having a separate crb_map_res for CRB start only devices
> > > is sane thing to do for validation.
> > 
> > Alternative is to do two structures crb_regs_head and crb_regs_tail,
> > which might be cleaner. I'm fine with going either route.
> 
> Since the iomem doesn't actually exist for a configuration having two
> pointers is the better choice. Make sure one is null for the
> configuration that does not support it.
> 
> The negative offset thing is way too subtle.

Yeah, I do agree with you on this. Even if it was functionalliy correct,
it is hard to understand if you don't proactively work on the driver.

> Jason

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jarkko Sakkinen]

On Mon, Oct 10, 2016 at 12:25:11AM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com]
> > Sent: Monday, October 10, 2016 02:08
> > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Cc: moderated list:TPM DEVICE DRIVER <tpmdd-devel@lists.sourceforge.net>;
> > open list <linux-kernel@vger.kernel.org>
> > Subject: Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct
> > crb_control_area to struct crb_regs
> > 
> > On Sun, Oct 09, 2016 at 09:33:58PM +0300, Jarkko Sakkinen wrote:
> > 
> > > > Sorry I missed this part.
> > > >
> > > > Here are the constraints for existing hardware:
> > > >
> > > > 1. All the existing CRB start only hardware has the iomem covering the
> > > >    control area and registers for multiple localities.
> > > > 2. All the existing ACPI start hardware has only the control area.
> > > >
> > > > If you assume that SSDT does not have malicous behavior caused by
> > > > either a BIOS bug or maybe a rootkit, then the current patch works
> > > > for all the existing hardware.
> > > >
> > > > To counter-measure for unexpected behavior in non-existing hardware
> > > > and buggy or malicious firmware it probably make sense to use
> > > > crb_map_res to validate the part of the CRB registers that is not
> > > > part of the control area.
> > 
> > I don't know how much I'd assume BIOS authors do what you think - the spec I
> > saw for this seems very vauge.
> > 
> > Certainly checking that locality region falls within the acpi mapping seems
> > essential.
> > 
> > > > Doing it in the way you proposed does not work for ACPI start devices.
> > > >
> > > > For them it should be done in the same way as I'm doing in the
> > > > existing patch as for ACPI start devices the address below the
> > > > control area are never accessed. Having a separate crb_map_res for
> > > > CRB start only devices is sane thing to do for validation.
> > >
> > > Alternative is to do two structures crb_regs_head and crb_regs_tail,
> > > which might be cleaner. I'm fine with going either route.
> > 
> > Since the iomem doesn't actually exist for a configuration having two pointers
> > is the better choice. Make sure one is null for the configuration that does not
> > support it.
> > 
> > The negative offset thing is way too subtle.
> 
> I addition I believe it should be always on offset FED4_0xxxh by the
> Spec, so all this arithmetic is a bit of overkill.

It's all done to workaround ACPI start. Even if CRB start devices would
always be in that offset it still would be needed.

> Thanks
> Tomas

/Jarkko