2017-03-15 05:57:58

by Jarkko Sakkinen

Subject: [PATCH v3] tpm_crb: request and relinquish locality 0

From: Jarkko Sakkinen <[email protected]>

This commit adds support for requesting and relinquishing locality 0 in
tpm_crb for the course of command transmission.

In order to achieve this, two new callbacks are added to struct
tpm_class_ops:

- request_locality
- relinquish_locality

These are called before sending and receiving data from the TPM. We
update also tpm_tis_core to use these callbacks. A small modification to
request_locality() is done so that it returns -EBUSY instead of locality
number when check_locality() fails in order to return something
meaningful to the user space.

With CRB interface you first set either requestAccess or relinquish bit
from TPM_LOC_CTRL_x register and then wait for locAssigned and
tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.

The reason why were are doing this is to make sure that the driver
will work properly with Intel TXT that uses locality 2. There's no
explicit guarantee that it would relinquish this locality. In more
general sense this commit enables tpm_crb to be a well behaving
citizen in a multi locality environment.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v2:
- TPM driver level calllbacks
v3:
- Call ops->relinquish_locality only if ops->request_locality has been
successful.
- Do not reserve locality in nested tpm_transmit calls.
- Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
stable.
drivers/char/tpm/tpm-interface.c | 10 ++++++++++
drivers/char/tpm/tpm.h | 3 ++-
drivers/char/tpm/tpm2-space.c | 15 +++++++++------
drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
include/linux/tpm.h | 3 ++-
6 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 95c6f98..4cd216f 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
ssize_t len = 0;
u32 count, ordinal;
unsigned long stop;
+ bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);

if (!tpm_validate_command(chip, buf, bufsiz))
return -EINVAL;
@@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
if (chip->dev.parent)
pm_runtime_get_sync(chip->dev.parent);

+ if (no_locality && chip->ops->request_locality) {
+ rc = chip->ops->request_locality(chip, 0);
+ if (rc)
+ goto out_no_locality;
+ }
+
rc = tpm2_prepare_space(chip, space, ordinal, buf);
if (rc)
goto out;
@@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
rc = tpm2_commit_space(chip, space, ordinal, buf, &len);

out:
+ if (no_locality && chip->ops->relinquish_locality)
+ chip->ops->relinquish_locality(chip, 0, false);
+out_no_locality:
if (chip->dev.parent)
pm_runtime_put_sync(chip->dev.parent);

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 5eacb3f..ba2c00d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
extern struct idr dev_nums_idr;

enum tpm_transmit_flags {
- TPM_TRANSMIT_UNLOCKED = BIT(0),
+ TPM_TRANSMIT_UNLOCKED = BIT(0),
+ TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
};

ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index d36d81e..43d05ab 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -19,6 +19,9 @@
#include <asm/unaligned.h>
#include "tpm.h"

+#define TPM_TRANSMIT_NESTED \
+ (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
+
enum tpm2_handle_types {
TPM2_HT_HMAC_SESSION = 0x02000000,
TPM2_HT_POLICY_SESSION = 0x03000000,
@@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
if (space->session_tbl[i])
tpm2_flush_context_cmd(chip, space->session_tbl[i],
- TPM_TRANSMIT_UNLOCKED);
+ TPM_TRANSMIT_NESTED);
}
}

@@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
tpm_buf_append(&tbuf, &buf[*offset], body_size);

rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
- TPM_TRANSMIT_UNLOCKED, NULL);
+ TPM_TRANSMIT_NESTED, NULL);
if (rc < 0) {
dev_warn(&chip->dev, "%s: failed with a system error %d\n",
__func__, rc);
@@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
tpm_buf_append_u32(&tbuf, handle);

rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
- TPM_TRANSMIT_UNLOCKED, NULL);
+ TPM_TRANSMIT_NESTED, NULL);
if (rc < 0) {
dev_warn(&chip->dev, "%s: failed with a system error %d\n",
__func__, rc);
@@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
if (space->context_tbl[i] && ~space->context_tbl[i])
tpm2_flush_context_cmd(chip, space->context_tbl[i],
- TPM_TRANSMIT_UNLOCKED);
+ TPM_TRANSMIT_NESTED);

tpm2_flush_sessions(chip, space);
}
@@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,

return 0;
out_no_slots:
- tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
+ tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
phandle);
return -ENOMEM;
@@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
return rc;

tpm2_flush_context_cmd(chip, space->context_tbl[i],
- TPM_TRANSMIT_UNLOCKED);
+ TPM_TRANSMIT_NESTED);
space->context_tbl[i] = ~0;
}

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1dfc37e..eb2fcf1 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,6 +34,16 @@ 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),
+ CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
+};
+
enum crb_ctrl_req {
CRB_CTRL_REQ_CMD_READY = BIT(0),
CRB_CTRL_REQ_GO_IDLE = BIT(1),
@@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
return 0;
}

+static int crb_request_locality(struct tpm_chip *chip, int loc)
+{
+ struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+ u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
+ CRB_LOC_STATE_TPM_REG_VALID_STS;
+
+ if (!priv->regs_h)
+ return 0;
+
+ iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
+ if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
+ TPM2_TIMEOUT_C)) {
+ dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
+ return -ETIME;
+ }
+
+ return 0;
+}
+
+static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
+{
+ struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+ if (!priv->regs_h)
+ return;
+
+ iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
+}
+
static u8 crb_status(struct tpm_chip *chip)
{
struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
.send = crb_send,
.cancel = crb_cancel,
.req_canceled = crb_req_canceled,
+ .request_locality = crb_request_locality,
+ .relinquish_locality = crb_relinquish_locality,
.req_complete_mask = CRB_DRV_STS_COMPLETE,
.req_complete_val = CRB_DRV_STS_COMPLETE,
};
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index fc0e9a2..c3cbe24 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
return -1;
}

-static void release_locality(struct tpm_chip *chip, int l, int force)
+static void release_locality(struct tpm_chip *chip, int l, bool force)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc;
@@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
long rc;

if (check_locality(chip, l) >= 0)
- return l;
+ return -EBUSY;

rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
if (rc < 0)
@@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)

out:
tpm_tis_ready(chip);
- release_locality(chip, priv->locality, 0);
return size;
}

@@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
size_t count = 0;
bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;

- if (request_locality(chip, 0) < 0)
- return -EBUSY;
-
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
tpm_tis_ready(chip);
@@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)

out_err:
tpm_tis_ready(chip);
- release_locality(chip, priv->locality, 0);
return rc;
}

@@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
return len;
out_err:
tpm_tis_ready(chip);
- release_locality(chip, priv->locality, 0);
return rc;
}

@@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
.send = tpm_tis_send,
.cancel = tpm_tis_ready,
.update_timeouts = tpm_tis_update_timeouts,
+ .request_locality = request_locality,
+ .relinquish_locality = release_locality,
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..65e05f9 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -48,7 +48,8 @@ struct tpm_class_ops {
u8 (*status) (struct tpm_chip *chip);
bool (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
-
+ int (*request_locality)(struct tpm_chip *chip, int loc);
+ void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
};

#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
--
2.9.3

2017-03-16 02:38:46

by Jerry Snitselaar

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

Jarkko Sakkinen @ 2017-03-15 05:57 GMT:

> From: Jarkko Sakkinen <[email protected]>
>
> This commit adds support for requesting and relinquishing locality 0 in
> tpm_crb for the course of command transmission.
>
> In order to achieve this, two new callbacks are added to struct
> tpm_class_ops:
>
> - request_locality
> - relinquish_locality
>
> These are called before sending and receiving data from the TPM. We
> update also tpm_tis_core to use these callbacks. A small modification to
> request_locality() is done so that it returns -EBUSY instead of locality
> number when check_locality() fails in order to return something
> meaningful to the user space.
>
> With CRB interface you first set either requestAccess or relinquish bit
> from TPM_LOC_CTRL_x register and then wait for locAssigned and
> tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.
>
> The reason why were are doing this is to make sure that the driver
> will work properly with Intel TXT that uses locality 2. There's no
> explicit guarantee that it would relinquish this locality. In more
> general sense this commit enables tpm_crb to be a well behaving
> citizen in a multi locality environment.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> v2:
> - TPM driver level calllbacks
> v3:
> - Call ops->relinquish_locality only if ops->request_locality has been
> successful.
> - Do not reserve locality in nested tpm_transmit calls.
> - Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
> stable.
> drivers/char/tpm/tpm-interface.c | 10 ++++++++++
> drivers/char/tpm/tpm.h | 3 ++-
> drivers/char/tpm/tpm2-space.c | 15 +++++++++------
> drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
> drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
> include/linux/tpm.h | 3 ++-
> 6 files changed, 68 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 95c6f98..4cd216f 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> ssize_t len = 0;
> u32 count, ordinal;
> unsigned long stop;
> + bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);
>
> if (!tpm_validate_command(chip, buf, bufsiz))
> return -EINVAL;
> @@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> if (chip->dev.parent)
> pm_runtime_get_sync(chip->dev.parent);
>
> + if (no_locality && chip->ops->request_locality) {
> + rc = chip->ops->request_locality(chip, 0);
> + if (rc)
> + goto out_no_locality;
> + }
> +
> rc = tpm2_prepare_space(chip, space, ordinal, buf);
> if (rc)
> goto out;
> @@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>
> out:
> + if (no_locality && chip->ops->relinquish_locality)
> + chip->ops->relinquish_locality(chip, 0, false);
> +out_no_locality:
> if (chip->dev.parent)
> pm_runtime_put_sync(chip->dev.parent);
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 5eacb3f..ba2c00d 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
> extern struct idr dev_nums_idr;
>
> enum tpm_transmit_flags {
> - TPM_TRANSMIT_UNLOCKED = BIT(0),
> + TPM_TRANSMIT_UNLOCKED = BIT(0),
> + TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
> };
>
> ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index d36d81e..43d05ab 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -19,6 +19,9 @@
> #include <asm/unaligned.h>
> #include "tpm.h"
>
> +#define TPM_TRANSMIT_NESTED \
> + (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
> +
> enum tpm2_handle_types {
> TPM2_HT_HMAC_SESSION = 0x02000000,
> TPM2_HT_POLICY_SESSION = 0x03000000,
> @@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
> for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> if (space->session_tbl[i])
> tpm2_flush_context_cmd(chip, space->session_tbl[i],
> - TPM_TRANSMIT_UNLOCKED);
> + TPM_TRANSMIT_NESTED);
> }
> }
>
> @@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> tpm_buf_append(&tbuf, &buf[*offset], body_size);
>
> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> - TPM_TRANSMIT_UNLOCKED, NULL);
> + TPM_TRANSMIT_NESTED, NULL);
> if (rc < 0) {
> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> __func__, rc);
> @@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
> tpm_buf_append_u32(&tbuf, handle);
>
> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
> - TPM_TRANSMIT_UNLOCKED, NULL);
> + TPM_TRANSMIT_NESTED, NULL);
> if (rc < 0) {
> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> __func__, rc);
> @@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
> if (space->context_tbl[i] && ~space->context_tbl[i])
> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> - TPM_TRANSMIT_UNLOCKED);
> + TPM_TRANSMIT_NESTED);
>
> tpm2_flush_sessions(chip, space);
> }
> @@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
>
> return 0;
> out_no_slots:
> - tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
> + tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
> dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
> phandle);
> return -ENOMEM;
> @@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
> return rc;
>
> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> - TPM_TRANSMIT_UNLOCKED);
> + TPM_TRANSMIT_NESTED);
> space->context_tbl[i] = ~0;
> }
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1dfc37e..eb2fcf1 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,6 +34,16 @@ 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),
> + CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
> +};
> +
> enum crb_ctrl_req {
> CRB_CTRL_REQ_CMD_READY = BIT(0),
> CRB_CTRL_REQ_GO_IDLE = BIT(1),
> @@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
> return 0;
> }
>
> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> +{
> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> + u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> + CRB_LOC_STATE_TPM_REG_VALID_STS;
> +
> + if (!priv->regs_h)
> + return 0;
> +
> + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> + if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
> + TPM2_TIMEOUT_C)) {
> + dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> + return -ETIME;
> + }
> +
> + return 0;
> +}
> +
> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> +{
> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> + if (!priv->regs_h)
> + return;
> +
> + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> +}
> +
> static u8 crb_status(struct tpm_chip *chip)
> {
> struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
> .send = crb_send,
> .cancel = crb_cancel,
> .req_canceled = crb_req_canceled,
> + .request_locality = crb_request_locality,
> + .relinquish_locality = crb_relinquish_locality,
> .req_complete_mask = CRB_DRV_STS_COMPLETE,
> .req_complete_val = CRB_DRV_STS_COMPLETE,
> };
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index fc0e9a2..c3cbe24 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
> return -1;
> }
>
> -static void release_locality(struct tpm_chip *chip, int l, int force)
> +static void release_locality(struct tpm_chip *chip, int l, bool force)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> int rc;
> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
> long rc;
>
> if (check_locality(chip, l) >= 0)
> - return l;
> + return -EBUSY;
>
> rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
> if (rc < 0)
> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>
> out:
> tpm_tis_ready(chip);
> - release_locality(chip, priv->locality, 0);
> return size;
> }
>
> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> size_t count = 0;
> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>
> - if (request_locality(chip, 0) < 0)
> - return -EBUSY;
> -
> status = tpm_tis_status(chip);
> if ((status & TPM_STS_COMMAND_READY) == 0) {
> tpm_tis_ready(chip);
> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>
> out_err:
> tpm_tis_ready(chip);
> - release_locality(chip, priv->locality, 0);
> return rc;
> }
>
> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
> return len;
> out_err:
> tpm_tis_ready(chip);
> - release_locality(chip, priv->locality, 0);
> return rc;
> }
>
> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
> .send = tpm_tis_send,
> .cancel = tpm_tis_ready,
> .update_timeouts = tpm_tis_update_timeouts,
> + .request_locality = request_locality,
> + .relinquish_locality = release_locality,
> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> .req_canceled = tpm_tis_req_canceled,
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index da158f0..65e05f9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -48,7 +48,8 @@ struct tpm_class_ops {
> u8 (*status) (struct tpm_chip *chip);
> bool (*update_timeouts)(struct tpm_chip *chip,
> unsigned long *timeout_cap);
> -
> + int (*request_locality)(struct tpm_chip *chip, int loc);
> + void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
> };
>
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)

This is broken for me on tpm_tis at the moment. release_locality only
actually follows through with releasing if you've set force or there is
a pending request for the TPM from another locality. So first time
through it requests and gets the locality, does its work, goes
to release it and doesn't because there is no pending request. Then
next time through, request_locality calls check_locality and returns
-EBUSY causing it to bail out of tpm_transmit.

2017-03-16 03:28:00

by Jerry Snitselaar

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

Jerry Snitselaar @ 2017-03-16 02:38 GMT:

> Jarkko Sakkinen @ 2017-03-15 05:57 GMT:
>
>> From: Jarkko Sakkinen <[email protected]>
>>
>> This commit adds support for requesting and relinquishing locality 0 in
>> tpm_crb for the course of command transmission.
>>
>> In order to achieve this, two new callbacks are added to struct
>> tpm_class_ops:
>>
>> - request_locality
>> - relinquish_locality
>>
>> These are called before sending and receiving data from the TPM. We
>> update also tpm_tis_core to use these callbacks. A small modification to
>> request_locality() is done so that it returns -EBUSY instead of locality
>> number when check_locality() fails in order to return something
>> meaningful to the user space.
>>
>> With CRB interface you first set either requestAccess or relinquish bit
>> from TPM_LOC_CTRL_x register and then wait for locAssigned and
>> tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.
>>
>> The reason why were are doing this is to make sure that the driver
>> will work properly with Intel TXT that uses locality 2. There's no
>> explicit guarantee that it would relinquish this locality. In more
>> general sense this commit enables tpm_crb to be a well behaving
>> citizen in a multi locality environment.
>>
>> Signed-off-by: Jarkko Sakkinen <[email protected]>
>> ---
>> v2:
>> - TPM driver level calllbacks
>> v3:
>> - Call ops->relinquish_locality only if ops->request_locality has been
>> successful.
>> - Do not reserve locality in nested tpm_transmit calls.
>> - Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
>> stable.
>> drivers/char/tpm/tpm-interface.c | 10 ++++++++++
>> drivers/char/tpm/tpm.h | 3 ++-
>> drivers/char/tpm/tpm2-space.c | 15 +++++++++------
>> drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
>> drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
>> include/linux/tpm.h | 3 ++-
>> 6 files changed, 68 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 95c6f98..4cd216f 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> ssize_t len = 0;
>> u32 count, ordinal;
>> unsigned long stop;
>> + bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);
>>
>> if (!tpm_validate_command(chip, buf, bufsiz))
>> return -EINVAL;
>> @@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> if (chip->dev.parent)
>> pm_runtime_get_sync(chip->dev.parent);
>>
>> + if (no_locality && chip->ops->request_locality) {
>> + rc = chip->ops->request_locality(chip, 0);
>> + if (rc)
>> + goto out_no_locality;
>> + }
>> +
>> rc = tpm2_prepare_space(chip, space, ordinal, buf);
>> if (rc)
>> goto out;
>> @@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>>
>> out:
>> + if (no_locality && chip->ops->relinquish_locality)
>> + chip->ops->relinquish_locality(chip, 0, false);
>> +out_no_locality:
>> if (chip->dev.parent)
>> pm_runtime_put_sync(chip->dev.parent);
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 5eacb3f..ba2c00d 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
>> extern struct idr dev_nums_idr;
>>
>> enum tpm_transmit_flags {
>> - TPM_TRANSMIT_UNLOCKED = BIT(0),
>> + TPM_TRANSMIT_UNLOCKED = BIT(0),
>> + TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
>> };
>>
>> ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>> index d36d81e..43d05ab 100644
>> --- a/drivers/char/tpm/tpm2-space.c
>> +++ b/drivers/char/tpm/tpm2-space.c
>> @@ -19,6 +19,9 @@
>> #include <asm/unaligned.h>
>> #include "tpm.h"
>>
>> +#define TPM_TRANSMIT_NESTED \
>> + (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
>> +
>> enum tpm2_handle_types {
>> TPM2_HT_HMAC_SESSION = 0x02000000,
>> TPM2_HT_POLICY_SESSION = 0x03000000,
>> @@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
>> for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
>> if (space->session_tbl[i])
>> tpm2_flush_context_cmd(chip, space->session_tbl[i],
>> - TPM_TRANSMIT_UNLOCKED);
>> + TPM_TRANSMIT_NESTED);
>> }
>> }
>>
>> @@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>> tpm_buf_append(&tbuf, &buf[*offset], body_size);
>>
>> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
>> - TPM_TRANSMIT_UNLOCKED, NULL);
>> + TPM_TRANSMIT_NESTED, NULL);
>> if (rc < 0) {
>> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>> __func__, rc);
>> @@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>> tpm_buf_append_u32(&tbuf, handle);
>>
>> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
>> - TPM_TRANSMIT_UNLOCKED, NULL);
>> + TPM_TRANSMIT_NESTED, NULL);
>> if (rc < 0) {
>> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>> __func__, rc);
>> @@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
>> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
>> if (space->context_tbl[i] && ~space->context_tbl[i])
>> tpm2_flush_context_cmd(chip, space->context_tbl[i],
>> - TPM_TRANSMIT_UNLOCKED);
>> + TPM_TRANSMIT_NESTED);
>>
>> tpm2_flush_sessions(chip, space);
>> }
>> @@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
>>
>> return 0;
>> out_no_slots:
>> - tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
>> + tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
>> dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
>> phandle);
>> return -ENOMEM;
>> @@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
>> return rc;
>>
>> tpm2_flush_context_cmd(chip, space->context_tbl[i],
>> - TPM_TRANSMIT_UNLOCKED);
>> + TPM_TRANSMIT_NESTED);
>> space->context_tbl[i] = ~0;
>> }
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index 1dfc37e..eb2fcf1 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -34,6 +34,16 @@ 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),
>> + CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
>> +};
>> +
>> enum crb_ctrl_req {
>> CRB_CTRL_REQ_CMD_READY = BIT(0),
>> CRB_CTRL_REQ_GO_IDLE = BIT(1),
>> @@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>> return 0;
>> }
>>
>> +static int crb_request_locality(struct tpm_chip *chip, int loc)
>> +{
>> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> + u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
>> + CRB_LOC_STATE_TPM_REG_VALID_STS;
>> +
>> + if (!priv->regs_h)
>> + return 0;
>> +
>> + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
>> + if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
>> + TPM2_TIMEOUT_C)) {
>> + dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
>> + return -ETIME;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
>> +{
>> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> +
>> + if (!priv->regs_h)
>> + return;
>> +
>> + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
>> +}
>> +
>> static u8 crb_status(struct tpm_chip *chip)
>> {
>> struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> @@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
>> .send = crb_send,
>> .cancel = crb_cancel,
>> .req_canceled = crb_req_canceled,
>> + .request_locality = crb_request_locality,
>> + .relinquish_locality = crb_relinquish_locality,
>> .req_complete_mask = CRB_DRV_STS_COMPLETE,
>> .req_complete_val = CRB_DRV_STS_COMPLETE,
>> };
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index fc0e9a2..c3cbe24 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
>> return -1;
>> }
>>
>> -static void release_locality(struct tpm_chip *chip, int l, int force)
>> +static void release_locality(struct tpm_chip *chip, int l, bool force)
>> {
>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> int rc;
>> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
>> long rc;
>>
>> if (check_locality(chip, l) >= 0)
>> - return l;
>> + return -EBUSY;
>>
>> rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
>> if (rc < 0)
>> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>>
>> out:
>> tpm_tis_ready(chip);
>> - release_locality(chip, priv->locality, 0);
>> return size;
>> }
>>
>> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> size_t count = 0;
>> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>
>> - if (request_locality(chip, 0) < 0)
>> - return -EBUSY;
>> -
>> status = tpm_tis_status(chip);
>> if ((status & TPM_STS_COMMAND_READY) == 0) {
>> tpm_tis_ready(chip);
>> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>>
>> out_err:
>> tpm_tis_ready(chip);
>> - release_locality(chip, priv->locality, 0);
>> return rc;
>> }
>>
>> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
>> return len;
>> out_err:
>> tpm_tis_ready(chip);
>> - release_locality(chip, priv->locality, 0);
>> return rc;
>> }
>>
>> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
>> .send = tpm_tis_send,
>> .cancel = tpm_tis_ready,
>> .update_timeouts = tpm_tis_update_timeouts,
>> + .request_locality = request_locality,
>> + .relinquish_locality = release_locality,
>> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> .req_canceled = tpm_tis_req_canceled,
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index da158f0..65e05f9 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -48,7 +48,8 @@ struct tpm_class_ops {
>> u8 (*status) (struct tpm_chip *chip);
>> bool (*update_timeouts)(struct tpm_chip *chip,
>> unsigned long *timeout_cap);
>> -
>> + int (*request_locality)(struct tpm_chip *chip, int loc);
>> + void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
>> };
>>
>> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
>
> This is broken for me on tpm_tis at the moment. release_locality only
> actually follows through with releasing if you've set force or there is
> a pending request for the TPM from another locality. So first time
> through it requests and gets the locality, does its work, goes
> to release it and doesn't because there is no pending request. Then
> next time through, request_locality calls check_locality and returns
> -EBUSY causing it to bail out of tpm_transmit.

Actually I should amend that, I'd been debugging a bit. With the
patch itself, tpm_tis_core_init() calls tpm2_probe() which fails
because tpm_transmit_cmd -> tpm_transmit -> request_locality returns
-EBUSY. There is a request_locality() call right before the
tpm2_probe(). I'd dropped that as part of my debugging which allowed
it to get further along, but then run into the problem mentioned above.

2017-03-16 11:55:51

by Jarkko Sakkinen

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

On Wed, Mar 15, 2017 at 08:27:54PM -0700, Jerry Snitselaar wrote:
>
> Jerry Snitselaar @ 2017-03-16 02:38 GMT:
>
> > Jarkko Sakkinen @ 2017-03-15 05:57 GMT:
> >
> >> From: Jarkko Sakkinen <[email protected]>
> >>
> >> This commit adds support for requesting and relinquishing locality 0 in
> >> tpm_crb for the course of command transmission.
> >>
> >> In order to achieve this, two new callbacks are added to struct
> >> tpm_class_ops:
> >>
> >> - request_locality
> >> - relinquish_locality
> >>
> >> These are called before sending and receiving data from the TPM. We
> >> update also tpm_tis_core to use these callbacks. A small modification to
> >> request_locality() is done so that it returns -EBUSY instead of locality
> >> number when check_locality() fails in order to return something
> >> meaningful to the user space.
> >>
> >> With CRB interface you first set either requestAccess or relinquish bit
> >> from TPM_LOC_CTRL_x register and then wait for locAssigned and
> >> tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.
> >>
> >> The reason why were are doing this is to make sure that the driver
> >> will work properly with Intel TXT that uses locality 2. There's no
> >> explicit guarantee that it would relinquish this locality. In more
> >> general sense this commit enables tpm_crb to be a well behaving
> >> citizen in a multi locality environment.
> >>
> >> Signed-off-by: Jarkko Sakkinen <[email protected]>
> >> ---
> >> v2:
> >> - TPM driver level calllbacks
> >> v3:
> >> - Call ops->relinquish_locality only if ops->request_locality has been
> >> successful.
> >> - Do not reserve locality in nested tpm_transmit calls.
> >> - Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
> >> stable.
> >> drivers/char/tpm/tpm-interface.c | 10 ++++++++++
> >> drivers/char/tpm/tpm.h | 3 ++-
> >> drivers/char/tpm/tpm2-space.c | 15 +++++++++------
> >> drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
> >> drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
> >> include/linux/tpm.h | 3 ++-
> >> 6 files changed, 68 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> index 95c6f98..4cd216f 100644
> >> --- a/drivers/char/tpm/tpm-interface.c
> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> @@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> ssize_t len = 0;
> >> u32 count, ordinal;
> >> unsigned long stop;
> >> + bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);
> >>
> >> if (!tpm_validate_command(chip, buf, bufsiz))
> >> return -EINVAL;
> >> @@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> if (chip->dev.parent)
> >> pm_runtime_get_sync(chip->dev.parent);
> >>
> >> + if (no_locality && chip->ops->request_locality) {
> >> + rc = chip->ops->request_locality(chip, 0);
> >> + if (rc)
> >> + goto out_no_locality;
> >> + }
> >> +
> >> rc = tpm2_prepare_space(chip, space, ordinal, buf);
> >> if (rc)
> >> goto out;
> >> @@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> >>
> >> out:
> >> + if (no_locality && chip->ops->relinquish_locality)
> >> + chip->ops->relinquish_locality(chip, 0, false);
> >> +out_no_locality:
> >> if (chip->dev.parent)
> >> pm_runtime_put_sync(chip->dev.parent);
> >>
> >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> >> index 5eacb3f..ba2c00d 100644
> >> --- a/drivers/char/tpm/tpm.h
> >> +++ b/drivers/char/tpm/tpm.h
> >> @@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
> >> extern struct idr dev_nums_idr;
> >>
> >> enum tpm_transmit_flags {
> >> - TPM_TRANSMIT_UNLOCKED = BIT(0),
> >> + TPM_TRANSMIT_UNLOCKED = BIT(0),
> >> + TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
> >> };
> >>
> >> ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> >> index d36d81e..43d05ab 100644
> >> --- a/drivers/char/tpm/tpm2-space.c
> >> +++ b/drivers/char/tpm/tpm2-space.c
> >> @@ -19,6 +19,9 @@
> >> #include <asm/unaligned.h>
> >> #include "tpm.h"
> >>
> >> +#define TPM_TRANSMIT_NESTED \
> >> + (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
> >> +
> >> enum tpm2_handle_types {
> >> TPM2_HT_HMAC_SESSION = 0x02000000,
> >> TPM2_HT_POLICY_SESSION = 0x03000000,
> >> @@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
> >> for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> >> if (space->session_tbl[i])
> >> tpm2_flush_context_cmd(chip, space->session_tbl[i],
> >> - TPM_TRANSMIT_UNLOCKED);
> >> + TPM_TRANSMIT_NESTED);
> >> }
> >> }
> >>
> >> @@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> >> tpm_buf_append(&tbuf, &buf[*offset], body_size);
> >>
> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> >> - TPM_TRANSMIT_UNLOCKED, NULL);
> >> + TPM_TRANSMIT_NESTED, NULL);
> >> if (rc < 0) {
> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> >> __func__, rc);
> >> @@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
> >> tpm_buf_append_u32(&tbuf, handle);
> >>
> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
> >> - TPM_TRANSMIT_UNLOCKED, NULL);
> >> + TPM_TRANSMIT_NESTED, NULL);
> >> if (rc < 0) {
> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> >> __func__, rc);
> >> @@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
> >> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
> >> if (space->context_tbl[i] && ~space->context_tbl[i])
> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> >> - TPM_TRANSMIT_UNLOCKED);
> >> + TPM_TRANSMIT_NESTED);
> >>
> >> tpm2_flush_sessions(chip, space);
> >> }
> >> @@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
> >>
> >> return 0;
> >> out_no_slots:
> >> - tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
> >> + tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
> >> dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
> >> phandle);
> >> return -ENOMEM;
> >> @@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
> >> return rc;
> >>
> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> >> - TPM_TRANSMIT_UNLOCKED);
> >> + TPM_TRANSMIT_NESTED);
> >> space->context_tbl[i] = ~0;
> >> }
> >>
> >> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> >> index 1dfc37e..eb2fcf1 100644
> >> --- a/drivers/char/tpm/tpm_crb.c
> >> +++ b/drivers/char/tpm/tpm_crb.c
> >> @@ -34,6 +34,16 @@ 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),
> >> + CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
> >> +};
> >> +
> >> enum crb_ctrl_req {
> >> CRB_CTRL_REQ_CMD_READY = BIT(0),
> >> CRB_CTRL_REQ_GO_IDLE = BIT(1),
> >> @@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
> >> return 0;
> >> }
> >>
> >> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> >> +{
> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> + u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> >> + CRB_LOC_STATE_TPM_REG_VALID_STS;
> >> +
> >> + if (!priv->regs_h)
> >> + return 0;
> >> +
> >> + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> >> + if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
> >> + TPM2_TIMEOUT_C)) {
> >> + dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> >> + return -ETIME;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> >> +{
> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> +
> >> + if (!priv->regs_h)
> >> + return;
> >> +
> >> + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> >> +}
> >> +
> >> static u8 crb_status(struct tpm_chip *chip)
> >> {
> >> struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> @@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
> >> .send = crb_send,
> >> .cancel = crb_cancel,
> >> .req_canceled = crb_req_canceled,
> >> + .request_locality = crb_request_locality,
> >> + .relinquish_locality = crb_relinquish_locality,
> >> .req_complete_mask = CRB_DRV_STS_COMPLETE,
> >> .req_complete_val = CRB_DRV_STS_COMPLETE,
> >> };
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index fc0e9a2..c3cbe24 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
> >> return -1;
> >> }
> >>
> >> -static void release_locality(struct tpm_chip *chip, int l, int force)
> >> +static void release_locality(struct tpm_chip *chip, int l, bool force)
> >> {
> >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> int rc;
> >> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
> >> long rc;
> >>
> >> if (check_locality(chip, l) >= 0)
> >> - return l;
> >> + return -EBUSY;
> >>
> >> rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
> >> if (rc < 0)
> >> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> >>
> >> out:
> >> tpm_tis_ready(chip);
> >> - release_locality(chip, priv->locality, 0);
> >> return size;
> >> }
> >>
> >> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> >> size_t count = 0;
> >> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> >>
> >> - if (request_locality(chip, 0) < 0)
> >> - return -EBUSY;
> >> -
> >> status = tpm_tis_status(chip);
> >> if ((status & TPM_STS_COMMAND_READY) == 0) {
> >> tpm_tis_ready(chip);
> >> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> >>
> >> out_err:
> >> tpm_tis_ready(chip);
> >> - release_locality(chip, priv->locality, 0);
> >> return rc;
> >> }
> >>
> >> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
> >> return len;
> >> out_err:
> >> tpm_tis_ready(chip);
> >> - release_locality(chip, priv->locality, 0);
> >> return rc;
> >> }
> >>
> >> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
> >> .send = tpm_tis_send,
> >> .cancel = tpm_tis_ready,
> >> .update_timeouts = tpm_tis_update_timeouts,
> >> + .request_locality = request_locality,
> >> + .relinquish_locality = release_locality,
> >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >> .req_canceled = tpm_tis_req_canceled,
> >> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> >> index da158f0..65e05f9 100644
> >> --- a/include/linux/tpm.h
> >> +++ b/include/linux/tpm.h
> >> @@ -48,7 +48,8 @@ struct tpm_class_ops {
> >> u8 (*status) (struct tpm_chip *chip);
> >> bool (*update_timeouts)(struct tpm_chip *chip,
> >> unsigned long *timeout_cap);
> >> -
> >> + int (*request_locality)(struct tpm_chip *chip, int loc);
> >> + void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
> >> };
> >>
> >> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> >
> > This is broken for me on tpm_tis at the moment. release_locality only
> > actually follows through with releasing if you've set force or there is
> > a pending request for the TPM from another locality. So first time
> > through it requests and gets the locality, does its work, goes
> > to release it and doesn't because there is no pending request. Then
> > next time through, request_locality calls check_locality and returns
> > -EBUSY causing it to bail out of tpm_transmit.
>
> Actually I should amend that, I'd been debugging a bit. With the
> patch itself, tpm_tis_core_init() calls tpm2_probe() which fails
> because tpm_transmit_cmd -> tpm_transmit -> request_locality returns
> -EBUSY. There is a request_locality() call right before the
> tpm2_probe(). I'd dropped that as part of my debugging which allowed
> it to get further along, but then run into the problem mentioned above.

I see.

The request_locality call in tpm_tis_core_init should works with this
change because it does only whether the return value is zero so that
can be safely ignored.

The send data case totally breaks.

Changing the return value to -EBUSY was a stupid mistake from my side.

I'll try revise this a bit in a way that the API will allow positive
value for stating that the given locality has been already taking.

Thank you for catching this up.

[1] http://lxr.free-electrons.com/source/drivers/char/tpm/tpm_tis_core.c

/Jarkko

2017-03-17 17:13:39

by Jason Gunthorpe

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

On Fri, Mar 17, 2017 at 10:00:41AM -0700, Jerry Snitselaar wrote:

> > Changing the return value to -EBUSY was a stupid mistake from my side.
> >
> > I'll try revise this a bit in a way that the API will allow positive
> > value for stating that the given locality has been already taking.
>
> Is there a big performance hit with requesting and releasing locality?
> If instead it just released it when release_locality is called I think
> the changes are pretty minor.

If you can measure please let us know :)

This is all very old it may not actually make any sense..

.. and as I said earlier if we want to 'cache' the locality for
performance then the core code should do it.

I kinda thought the point of releasing the locality was to allow other
platform things to access the TPM, so I'm confused why TIS wouldn't
always release it as well..

Jason

2017-03-17 17:26:08

by Jerry Snitselaar

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

Jarkko Sakkinen @ 2017-03-16 11:55 GMT:

> On Wed, Mar 15, 2017 at 08:27:54PM -0700, Jerry Snitselaar wrote:
>>
>> Jerry Snitselaar @ 2017-03-16 02:38 GMT:
>>
>> > Jarkko Sakkinen @ 2017-03-15 05:57 GMT:
>> >
>> >> From: Jarkko Sakkinen <[email protected]>
>> >>
>> >> This commit adds support for requesting and relinquishing locality 0 in
>> >> tpm_crb for the course of command transmission.
>> >>
>> >> In order to achieve this, two new callbacks are added to struct
>> >> tpm_class_ops:
>> >>
>> >> - request_locality
>> >> - relinquish_locality
>> >>
>> >> These are called before sending and receiving data from the TPM. We
>> >> update also tpm_tis_core to use these callbacks. A small modification to
>> >> request_locality() is done so that it returns -EBUSY instead of locality
>> >> number when check_locality() fails in order to return something
>> >> meaningful to the user space.
>> >>
>> >> With CRB interface you first set either requestAccess or relinquish bit
>> >> from TPM_LOC_CTRL_x register and then wait for locAssigned and
>> >> tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.
>> >>
>> >> The reason why were are doing this is to make sure that the driver
>> >> will work properly with Intel TXT that uses locality 2. There's no
>> >> explicit guarantee that it would relinquish this locality. In more
>> >> general sense this commit enables tpm_crb to be a well behaving
>> >> citizen in a multi locality environment.
>> >>
>> >> Signed-off-by: Jarkko Sakkinen <[email protected]>
>> >> ---
>> >> v2:
>> >> - TPM driver level calllbacks
>> >> v3:
>> >> - Call ops->relinquish_locality only if ops->request_locality has been
>> >> successful.
>> >> - Do not reserve locality in nested tpm_transmit calls.
>> >> - Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
>> >> stable.
>> >> drivers/char/tpm/tpm-interface.c | 10 ++++++++++
>> >> drivers/char/tpm/tpm.h | 3 ++-
>> >> drivers/char/tpm/tpm2-space.c | 15 +++++++++------
>> >> drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
>> >> drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
>> >> include/linux/tpm.h | 3 ++-
>> >> 6 files changed, 68 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> >> index 95c6f98..4cd216f 100644
>> >> --- a/drivers/char/tpm/tpm-interface.c
>> >> +++ b/drivers/char/tpm/tpm-interface.c
>> >> @@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> >> ssize_t len = 0;
>> >> u32 count, ordinal;
>> >> unsigned long stop;
>> >> + bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);
>> >>
>> >> if (!tpm_validate_command(chip, buf, bufsiz))
>> >> return -EINVAL;
>> >> @@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> >> if (chip->dev.parent)
>> >> pm_runtime_get_sync(chip->dev.parent);
>> >>
>> >> + if (no_locality && chip->ops->request_locality) {
>> >> + rc = chip->ops->request_locality(chip, 0);
>> >> + if (rc)
>> >> + goto out_no_locality;
>> >> + }
>> >> +
>> >> rc = tpm2_prepare_space(chip, space, ordinal, buf);
>> >> if (rc)
>> >> goto out;
>> >> @@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> >> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
>> >>
>> >> out:
>> >> + if (no_locality && chip->ops->relinquish_locality)
>> >> + chip->ops->relinquish_locality(chip, 0, false);
>> >> +out_no_locality:
>> >> if (chip->dev.parent)
>> >> pm_runtime_put_sync(chip->dev.parent);
>> >>
>> >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> >> index 5eacb3f..ba2c00d 100644
>> >> --- a/drivers/char/tpm/tpm.h
>> >> +++ b/drivers/char/tpm/tpm.h
>> >> @@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
>> >> extern struct idr dev_nums_idr;
>> >>
>> >> enum tpm_transmit_flags {
>> >> - TPM_TRANSMIT_UNLOCKED = BIT(0),
>> >> + TPM_TRANSMIT_UNLOCKED = BIT(0),
>> >> + TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
>> >> };
>> >>
>> >> ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
>> >> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>> >> index d36d81e..43d05ab 100644
>> >> --- a/drivers/char/tpm/tpm2-space.c
>> >> +++ b/drivers/char/tpm/tpm2-space.c
>> >> @@ -19,6 +19,9 @@
>> >> #include <asm/unaligned.h>
>> >> #include "tpm.h"
>> >>
>> >> +#define TPM_TRANSMIT_NESTED \
>> >> + (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
>> >> +
>> >> enum tpm2_handle_types {
>> >> TPM2_HT_HMAC_SESSION = 0x02000000,
>> >> TPM2_HT_POLICY_SESSION = 0x03000000,
>> >> @@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
>> >> for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
>> >> if (space->session_tbl[i])
>> >> tpm2_flush_context_cmd(chip, space->session_tbl[i],
>> >> - TPM_TRANSMIT_UNLOCKED);
>> >> + TPM_TRANSMIT_NESTED);
>> >> }
>> >> }
>> >>
>> >> @@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>> >> tpm_buf_append(&tbuf, &buf[*offset], body_size);
>> >>
>> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
>> >> - TPM_TRANSMIT_UNLOCKED, NULL);
>> >> + TPM_TRANSMIT_NESTED, NULL);
>> >> if (rc < 0) {
>> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>> >> __func__, rc);
>> >> @@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>> >> tpm_buf_append_u32(&tbuf, handle);
>> >>
>> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
>> >> - TPM_TRANSMIT_UNLOCKED, NULL);
>> >> + TPM_TRANSMIT_NESTED, NULL);
>> >> if (rc < 0) {
>> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>> >> __func__, rc);
>> >> @@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
>> >> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
>> >> if (space->context_tbl[i] && ~space->context_tbl[i])
>> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
>> >> - TPM_TRANSMIT_UNLOCKED);
>> >> + TPM_TRANSMIT_NESTED);
>> >>
>> >> tpm2_flush_sessions(chip, space);
>> >> }
>> >> @@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
>> >>
>> >> return 0;
>> >> out_no_slots:
>> >> - tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
>> >> + tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
>> >> dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
>> >> phandle);
>> >> return -ENOMEM;
>> >> @@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
>> >> return rc;
>> >>
>> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
>> >> - TPM_TRANSMIT_UNLOCKED);
>> >> + TPM_TRANSMIT_NESTED);
>> >> space->context_tbl[i] = ~0;
>> >> }
>> >>
>> >> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> >> index 1dfc37e..eb2fcf1 100644
>> >> --- a/drivers/char/tpm/tpm_crb.c
>> >> +++ b/drivers/char/tpm/tpm_crb.c
>> >> @@ -34,6 +34,16 @@ 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),
>> >> + CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
>> >> +};
>> >> +
>> >> enum crb_ctrl_req {
>> >> CRB_CTRL_REQ_CMD_READY = BIT(0),
>> >> CRB_CTRL_REQ_GO_IDLE = BIT(1),
>> >> @@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>> >> return 0;
>> >> }
>> >>
>> >> +static int crb_request_locality(struct tpm_chip *chip, int loc)
>> >> +{
>> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> >> + u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
>> >> + CRB_LOC_STATE_TPM_REG_VALID_STS;
>> >> +
>> >> + if (!priv->regs_h)
>> >> + return 0;
>> >> +
>> >> + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
>> >> + if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
>> >> + TPM2_TIMEOUT_C)) {
>> >> + dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
>> >> + return -ETIME;
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
>> >> +{
>> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> >> +
>> >> + if (!priv->regs_h)
>> >> + return;
>> >> +
>> >> + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
>> >> +}
>> >> +
>> >> static u8 crb_status(struct tpm_chip *chip)
>> >> {
>> >> struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> >> @@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
>> >> .send = crb_send,
>> >> .cancel = crb_cancel,
>> >> .req_canceled = crb_req_canceled,
>> >> + .request_locality = crb_request_locality,
>> >> + .relinquish_locality = crb_relinquish_locality,
>> >> .req_complete_mask = CRB_DRV_STS_COMPLETE,
>> >> .req_complete_val = CRB_DRV_STS_COMPLETE,
>> >> };
>> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> >> index fc0e9a2..c3cbe24 100644
>> >> --- a/drivers/char/tpm/tpm_tis_core.c
>> >> +++ b/drivers/char/tpm/tpm_tis_core.c
>> >> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
>> >> return -1;
>> >> }
>> >>
>> >> -static void release_locality(struct tpm_chip *chip, int l, int force)
>> >> +static void release_locality(struct tpm_chip *chip, int l, bool force)
>> >> {
>> >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> >> int rc;
>> >> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
>> >> long rc;
>> >>
>> >> if (check_locality(chip, l) >= 0)
>> >> - return l;
>> >> + return -EBUSY;
>> >>
>> >> rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
>> >> if (rc < 0)
>> >> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>> >>
>> >> out:
>> >> tpm_tis_ready(chip);
>> >> - release_locality(chip, priv->locality, 0);
>> >> return size;
>> >> }
>> >>
>> >> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> >> size_t count = 0;
>> >> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>> >>
>> >> - if (request_locality(chip, 0) < 0)
>> >> - return -EBUSY;
>> >> -
>> >> status = tpm_tis_status(chip);
>> >> if ((status & TPM_STS_COMMAND_READY) == 0) {
>> >> tpm_tis_ready(chip);
>> >> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
>> >>
>> >> out_err:
>> >> tpm_tis_ready(chip);
>> >> - release_locality(chip, priv->locality, 0);
>> >> return rc;
>> >> }
>> >>
>> >> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
>> >> return len;
>> >> out_err:
>> >> tpm_tis_ready(chip);
>> >> - release_locality(chip, priv->locality, 0);
>> >> return rc;
>> >> }
>> >>
>> >> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
>> >> .send = tpm_tis_send,
>> >> .cancel = tpm_tis_ready,
>> >> .update_timeouts = tpm_tis_update_timeouts,
>> >> + .request_locality = request_locality,
>> >> + .relinquish_locality = release_locality,
>> >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> >> .req_canceled = tpm_tis_req_canceled,
>> >> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> >> index da158f0..65e05f9 100644
>> >> --- a/include/linux/tpm.h
>> >> +++ b/include/linux/tpm.h
>> >> @@ -48,7 +48,8 @@ struct tpm_class_ops {
>> >> u8 (*status) (struct tpm_chip *chip);
>> >> bool (*update_timeouts)(struct tpm_chip *chip,
>> >> unsigned long *timeout_cap);
>> >> -
>> >> + int (*request_locality)(struct tpm_chip *chip, int loc);
>> >> + void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
>> >> };
>> >>
>> >> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
>> >
>> > This is broken for me on tpm_tis at the moment. release_locality only
>> > actually follows through with releasing if you've set force or there is
>> > a pending request for the TPM from another locality. So first time
>> > through it requests and gets the locality, does its work, goes
>> > to release it and doesn't because there is no pending request. Then
>> > next time through, request_locality calls check_locality and returns
>> > -EBUSY causing it to bail out of tpm_transmit.
>>
>> Actually I should amend that, I'd been debugging a bit. With the
>> patch itself, tpm_tis_core_init() calls tpm2_probe() which fails
>> because tpm_transmit_cmd -> tpm_transmit -> request_locality returns
>> -EBUSY. There is a request_locality() call right before the
>> tpm2_probe(). I'd dropped that as part of my debugging which allowed
>> it to get further along, but then run into the problem mentioned above.
>
> I see.
>
> The request_locality call in tpm_tis_core_init should works with this
> change because it does only whether the return value is zero so that
> can be safely ignored.
>

Yes, the request_locality call works there, I just wasn't sure it was
necessary since tpm2_probe will end up calling it (and returning
-EBUSY). I guess it would be needed prior to the probe_itpm call since
nothing in there calls request_locality now. It seems a bit odd
that probe_iptm calls release_locality twice though.

> The send data case totally breaks.
>
> Changing the return value to -EBUSY was a stupid mistake from my side.
>
> I'll try revise this a bit in a way that the API will allow positive
> value for stating that the given locality has been already taking.
>

Is there a big performance hit with requesting and releasing locality?
If instead it just released it when release_locality is called I think
the changes are pretty minor.

> Thank you for catching this up.
>
> [1] http://lxr.free-electrons.com/source/drivers/char/tpm/tpm_tis_core.c
>
> /Jarkko

2017-03-17 20:35:35

by Jarkko Sakkinen

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

On Fri, Mar 17, 2017 at 10:00:41AM -0700, Jerry Snitselaar wrote:
>
> Jarkko Sakkinen @ 2017-03-16 11:55 GMT:
>
> > On Wed, Mar 15, 2017 at 08:27:54PM -0700, Jerry Snitselaar wrote:
> >>
> >> Jerry Snitselaar @ 2017-03-16 02:38 GMT:
> >>
> >> > Jarkko Sakkinen @ 2017-03-15 05:57 GMT:
> >> >
> >> >> From: Jarkko Sakkinen <[email protected]>
> >> >>
> >> >> This commit adds support for requesting and relinquishing locality 0 in
> >> >> tpm_crb for the course of command transmission.
> >> >>
> >> >> In order to achieve this, two new callbacks are added to struct
> >> >> tpm_class_ops:
> >> >>
> >> >> - request_locality
> >> >> - relinquish_locality
> >> >>
> >> >> These are called before sending and receiving data from the TPM. We
> >> >> update also tpm_tis_core to use these callbacks. A small modification to
> >> >> request_locality() is done so that it returns -EBUSY instead of locality
> >> >> number when check_locality() fails in order to return something
> >> >> meaningful to the user space.
> >> >>
> >> >> With CRB interface you first set either requestAccess or relinquish bit
> >> >> from TPM_LOC_CTRL_x register and then wait for locAssigned and
> >> >> tpmRegValidSts bits to be set in the TPM_LOC_STATE_x register.
> >> >>
> >> >> The reason why were are doing this is to make sure that the driver
> >> >> will work properly with Intel TXT that uses locality 2. There's no
> >> >> explicit guarantee that it would relinquish this locality. In more
> >> >> general sense this commit enables tpm_crb to be a well behaving
> >> >> citizen in a multi locality environment.
> >> >>
> >> >> Signed-off-by: Jarkko Sakkinen <[email protected]>
> >> >> ---
> >> >> v2:
> >> >> - TPM driver level calllbacks
> >> >> v3:
> >> >> - Call ops->relinquish_locality only if ops->request_locality has been
> >> >> successful.
> >> >> - Do not reserve locality in nested tpm_transmit calls.
> >> >> - Check for tpmRegValidSts to make sure that the value in TPM_LOC_STATE_x is
> >> >> stable.
> >> >> drivers/char/tpm/tpm-interface.c | 10 ++++++++++
> >> >> drivers/char/tpm/tpm.h | 3 ++-
> >> >> drivers/char/tpm/tpm2-space.c | 15 +++++++++------
> >> >> drivers/char/tpm/tpm_crb.c | 41 ++++++++++++++++++++++++++++++++++++++++
> >> >> drivers/char/tpm/tpm_tis_core.c | 12 ++++--------
> >> >> include/linux/tpm.h | 3 ++-
> >> >> 6 files changed, 68 insertions(+), 16 deletions(-)
> >> >>
> >> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> >> index 95c6f98..4cd216f 100644
> >> >> --- a/drivers/char/tpm/tpm-interface.c
> >> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> >> @@ -384,6 +384,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> >> ssize_t len = 0;
> >> >> u32 count, ordinal;
> >> >> unsigned long stop;
> >> >> + bool no_locality = !(flags & TPM_TRANSMIT_HAS_LOCALITY);
> >> >>
> >> >> if (!tpm_validate_command(chip, buf, bufsiz))
> >> >> return -EINVAL;
> >> >> @@ -407,6 +408,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> >> if (chip->dev.parent)
> >> >> pm_runtime_get_sync(chip->dev.parent);
> >> >>
> >> >> + if (no_locality && chip->ops->request_locality) {
> >> >> + rc = chip->ops->request_locality(chip, 0);
> >> >> + if (rc)
> >> >> + goto out_no_locality;
> >> >> + }
> >> >> +
> >> >> rc = tpm2_prepare_space(chip, space, ordinal, buf);
> >> >> if (rc)
> >> >> goto out;
> >> >> @@ -466,6 +473,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> >> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> >> >>
> >> >> out:
> >> >> + if (no_locality && chip->ops->relinquish_locality)
> >> >> + chip->ops->relinquish_locality(chip, 0, false);
> >> >> +out_no_locality:
> >> >> if (chip->dev.parent)
> >> >> pm_runtime_put_sync(chip->dev.parent);
> >> >>
> >> >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> >> >> index 5eacb3f..ba2c00d 100644
> >> >> --- a/drivers/char/tpm/tpm.h
> >> >> +++ b/drivers/char/tpm/tpm.h
> >> >> @@ -521,7 +521,8 @@ extern const struct file_operations tpmrm_fops;
> >> >> extern struct idr dev_nums_idr;
> >> >>
> >> >> enum tpm_transmit_flags {
> >> >> - TPM_TRANSMIT_UNLOCKED = BIT(0),
> >> >> + TPM_TRANSMIT_UNLOCKED = BIT(0),
> >> >> + TPM_TRANSMIT_HAS_LOCALITY = BIT(1),
> >> >> };
> >> >>
> >> >> ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> >> >> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> >> >> index d36d81e..43d05ab 100644
> >> >> --- a/drivers/char/tpm/tpm2-space.c
> >> >> +++ b/drivers/char/tpm/tpm2-space.c
> >> >> @@ -19,6 +19,9 @@
> >> >> #include <asm/unaligned.h>
> >> >> #include "tpm.h"
> >> >>
> >> >> +#define TPM_TRANSMIT_NESTED \
> >> >> + (TPM_TRANSMIT_UNLOCKED | TPM_TRANSMIT_HAS_LOCALITY)
> >> >> +
> >> >> enum tpm2_handle_types {
> >> >> TPM2_HT_HMAC_SESSION = 0x02000000,
> >> >> TPM2_HT_POLICY_SESSION = 0x03000000,
> >> >> @@ -39,7 +42,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
> >> >> for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> >> >> if (space->session_tbl[i])
> >> >> tpm2_flush_context_cmd(chip, space->session_tbl[i],
> >> >> - TPM_TRANSMIT_UNLOCKED);
> >> >> + TPM_TRANSMIT_NESTED);
> >> >> }
> >> >> }
> >> >>
> >> >> @@ -84,7 +87,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> >> >> tpm_buf_append(&tbuf, &buf[*offset], body_size);
> >> >>
> >> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> >> >> - TPM_TRANSMIT_UNLOCKED, NULL);
> >> >> + TPM_TRANSMIT_NESTED, NULL);
> >> >> if (rc < 0) {
> >> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> >> >> __func__, rc);
> >> >> @@ -132,7 +135,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
> >> >> tpm_buf_append_u32(&tbuf, handle);
> >> >>
> >> >> rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
> >> >> - TPM_TRANSMIT_UNLOCKED, NULL);
> >> >> + TPM_TRANSMIT_NESTED, NULL);
> >> >> if (rc < 0) {
> >> >> dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> >> >> __func__, rc);
> >> >> @@ -169,7 +172,7 @@ static void tpm2_flush_space(struct tpm_chip *chip)
> >> >> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
> >> >> if (space->context_tbl[i] && ~space->context_tbl[i])
> >> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> >> >> - TPM_TRANSMIT_UNLOCKED);
> >> >> + TPM_TRANSMIT_NESTED);
> >> >>
> >> >> tpm2_flush_sessions(chip, space);
> >> >> }
> >> >> @@ -376,7 +379,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
> >> >>
> >> >> return 0;
> >> >> out_no_slots:
> >> >> - tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
> >> >> + tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
> >> >> dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
> >> >> phandle);
> >> >> return -ENOMEM;
> >> >> @@ -468,7 +471,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
> >> >> return rc;
> >> >>
> >> >> tpm2_flush_context_cmd(chip, space->context_tbl[i],
> >> >> - TPM_TRANSMIT_UNLOCKED);
> >> >> + TPM_TRANSMIT_NESTED);
> >> >> space->context_tbl[i] = ~0;
> >> >> }
> >> >>
> >> >> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> >> >> index 1dfc37e..eb2fcf1 100644
> >> >> --- a/drivers/char/tpm/tpm_crb.c
> >> >> +++ b/drivers/char/tpm/tpm_crb.c
> >> >> @@ -34,6 +34,16 @@ 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),
> >> >> + CRB_LOC_STATE_TPM_REG_VALID_STS = BIT(7),
> >> >> +};
> >> >> +
> >> >> enum crb_ctrl_req {
> >> >> CRB_CTRL_REQ_CMD_READY = BIT(0),
> >> >> CRB_CTRL_REQ_GO_IDLE = BIT(1),
> >> >> @@ -172,6 +182,35 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
> >> >> return 0;
> >> >> }
> >> >>
> >> >> +static int crb_request_locality(struct tpm_chip *chip, int loc)
> >> >> +{
> >> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> >> + u32 value = CRB_LOC_STATE_LOC_ASSIGNED |
> >> >> + CRB_LOC_STATE_TPM_REG_VALID_STS;
> >> >> +
> >> >> + if (!priv->regs_h)
> >> >> + return 0;
> >> >> +
> >> >> + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs_h->loc_ctrl);
> >> >> + if (!crb_wait_for_reg_32(&priv->regs_h->loc_state, value, value,
> >> >> + TPM2_TIMEOUT_C)) {
> >> >> + dev_warn(&chip->dev, "TPM_LOC_STATE_x.requestAccess timed out\n");
> >> >> + return -ETIME;
> >> >> + }
> >> >> +
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> +static void crb_relinquish_locality(struct tpm_chip *chip, int loc, bool force)
> >> >> +{
> >> >> + struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> >> +
> >> >> + if (!priv->regs_h)
> >> >> + return;
> >> >> +
> >> >> + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs_h->loc_ctrl);
> >> >> +}
> >> >> +
> >> >> static u8 crb_status(struct tpm_chip *chip)
> >> >> {
> >> >> struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> >> @@ -278,6 +317,8 @@ static const struct tpm_class_ops tpm_crb = {
> >> >> .send = crb_send,
> >> >> .cancel = crb_cancel,
> >> >> .req_canceled = crb_req_canceled,
> >> >> + .request_locality = crb_request_locality,
> >> >> + .relinquish_locality = crb_relinquish_locality,
> >> >> .req_complete_mask = CRB_DRV_STS_COMPLETE,
> >> >> .req_complete_val = CRB_DRV_STS_COMPLETE,
> >> >> };
> >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> >> index fc0e9a2..c3cbe24 100644
> >> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> >> @@ -73,7 +73,7 @@ static int check_locality(struct tpm_chip *chip, int l)
> >> >> return -1;
> >> >> }
> >> >>
> >> >> -static void release_locality(struct tpm_chip *chip, int l, int force)
> >> >> +static void release_locality(struct tpm_chip *chip, int l, bool force)
> >> >> {
> >> >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >> >> int rc;
> >> >> @@ -97,7 +97,7 @@ static int request_locality(struct tpm_chip *chip, int l)
> >> >> long rc;
> >> >>
> >> >> if (check_locality(chip, l) >= 0)
> >> >> - return l;
> >> >> + return -EBUSY;
> >> >>
> >> >> rc = tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_REQUEST_USE);
> >> >> if (rc < 0)
> >> >> @@ -252,7 +252,6 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> >> >>
> >> >> out:
> >> >> tpm_tis_ready(chip);
> >> >> - release_locality(chip, priv->locality, 0);
> >> >> return size;
> >> >> }
> >> >>
> >> >> @@ -268,9 +267,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> >> >> size_t count = 0;
> >> >> bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
> >> >>
> >> >> - if (request_locality(chip, 0) < 0)
> >> >> - return -EBUSY;
> >> >> -
> >> >> status = tpm_tis_status(chip);
> >> >> if ((status & TPM_STS_COMMAND_READY) == 0) {
> >> >> tpm_tis_ready(chip);
> >> >> @@ -329,7 +325,6 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
> >> >>
> >> >> out_err:
> >> >> tpm_tis_ready(chip);
> >> >> - release_locality(chip, priv->locality, 0);
> >> >> return rc;
> >> >> }
> >> >>
> >> >> @@ -390,7 +385,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, u8 *buf, size_t len)
> >> >> return len;
> >> >> out_err:
> >> >> tpm_tis_ready(chip);
> >> >> - release_locality(chip, priv->locality, 0);
> >> >> return rc;
> >> >> }
> >> >>
> >> >> @@ -681,6 +675,8 @@ static const struct tpm_class_ops tpm_tis = {
> >> >> .send = tpm_tis_send,
> >> >> .cancel = tpm_tis_ready,
> >> >> .update_timeouts = tpm_tis_update_timeouts,
> >> >> + .request_locality = request_locality,
> >> >> + .relinquish_locality = release_locality,
> >> >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >> >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> >> >> .req_canceled = tpm_tis_req_canceled,
> >> >> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> >> >> index da158f0..65e05f9 100644
> >> >> --- a/include/linux/tpm.h
> >> >> +++ b/include/linux/tpm.h
> >> >> @@ -48,7 +48,8 @@ struct tpm_class_ops {
> >> >> u8 (*status) (struct tpm_chip *chip);
> >> >> bool (*update_timeouts)(struct tpm_chip *chip,
> >> >> unsigned long *timeout_cap);
> >> >> -
> >> >> + int (*request_locality)(struct tpm_chip *chip, int loc);
> >> >> + void (*relinquish_locality)(struct tpm_chip *chip, int loc, bool force);
> >> >> };
> >> >>
> >> >> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> >> >
> >> > This is broken for me on tpm_tis at the moment. release_locality only
> >> > actually follows through with releasing if you've set force or there is
> >> > a pending request for the TPM from another locality. So first time
> >> > through it requests and gets the locality, does its work, goes
> >> > to release it and doesn't because there is no pending request. Then
> >> > next time through, request_locality calls check_locality and returns
> >> > -EBUSY causing it to bail out of tpm_transmit.
> >>
> >> Actually I should amend that, I'd been debugging a bit. With the
> >> patch itself, tpm_tis_core_init() calls tpm2_probe() which fails
> >> because tpm_transmit_cmd -> tpm_transmit -> request_locality returns
> >> -EBUSY. There is a request_locality() call right before the
> >> tpm2_probe(). I'd dropped that as part of my debugging which allowed
> >> it to get further along, but then run into the problem mentioned above.
> >
> > I see.
> >
> > The request_locality call in tpm_tis_core_init should works with this
> > change because it does only whether the return value is zero so that
> > can be safely ignored.
> >
>
> Yes, the request_locality call works there, I just wasn't sure it was
> necessary since tpm2_probe will end up calling it (and returning
> -EBUSY). I guess it would be needed prior to the probe_itpm call since
> nothing in there calls request_locality now. It seems a bit odd
> that probe_iptm calls release_locality twice though.
>
> > The send data case totally breaks.
> >
> > Changing the return value to -EBUSY was a stupid mistake from my side.
> >
> > I'll try revise this a bit in a way that the API will allow positive
> > value for stating that the given locality has been already taking.
> >
>
> Is there a big performance hit with requesting and releasing locality?
> If instead it just released it when release_locality is called I think
> the changes are pretty minor.

I wouldn't think so with modern TPMs. I could go with this.

/Jarkko

2017-03-17 20:39:25

by Jarkko Sakkinen

Subject: Re: [PATCH v3] tpm_crb: request and relinquish locality 0

On Fri, Mar 17, 2017 at 11:11:16AM -0600, Jason Gunthorpe wrote:
> On Fri, Mar 17, 2017 at 10:00:41AM -0700, Jerry Snitselaar wrote:
>
> > > Changing the return value to -EBUSY was a stupid mistake from my side.
> > >
> > > I'll try revise this a bit in a way that the API will allow positive
> > > value for stating that the given locality has been already taking.
> >
> > Is there a big performance hit with requesting and releasing locality?
> > If instead it just released it when release_locality is called I think
> > the changes are pretty minor.
>
> If you can measure please let us know :)
>
> This is all very old it may not actually make any sense..
>
> .. and as I said earlier if we want to 'cache' the locality for
> performance then the core code should do it.
>
> I kinda thought the point of releasing the locality was to allow other
> platform things to access the TPM, so I'm confused why TIS wouldn't
> always release it as well..
>
> Jason

I would propose to make tpm_tis_core to work like that.

/Jarkko