* [PATCH RFC 0/3] Locality support for the CRB driver
@ 2016-10-09 0:15 Jarkko Sakkinen
2016-10-09 0:15 ` [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Jarkko Sakkinen
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Jarkko Sakkinen @ 2016-10-09 0:15 UTC (permalink / raw)
To: Peter Huewe
Cc: Jarkko Sakkinen, Jason Gunthorpe, open list,
moderated list:TPM DEVICE DRIVER
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 0:15 [PATCH RFC 0/3] Locality support for the CRB driver Jarkko Sakkinen @ 2016-10-09 0:15 ` Jarkko Sakkinen 2016-10-09 1:42 ` Jason Gunthorpe 2016-10-09 0:15 ` [PATCH RFC 2/3] tpm_crb: encapsulate crb_wait_for_reg_32 Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen 2 siblings, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 0:15 UTC (permalink / raw) To: Peter Huewe Cc: Jarkko Sakkinen, Marcel Selhorst, Jason Gunthorpe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 0:15 ` [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Jarkko Sakkinen @ 2016-10-09 1:42 ` Jason Gunthorpe 2016-10-09 9:38 ` Jarkko Sakkinen 0 siblings, 1 reply; 19+ messages in thread From: Jason Gunthorpe @ 2016-10-09 1:42 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 1:42 ` Jason Gunthorpe @ 2016-10-09 9:38 ` Jarkko Sakkinen 2016-10-09 16:49 ` Jason Gunthorpe 0 siblings, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 9:38 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 9:38 ` Jarkko Sakkinen @ 2016-10-09 16:49 ` Jason Gunthorpe 2016-10-09 18:06 ` Jarkko Sakkinen 2016-10-09 18:32 ` Jarkko Sakkinen 0 siblings, 2 replies; 19+ messages in thread From: Jason Gunthorpe @ 2016-10-09 16:49 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 16:49 ` Jason Gunthorpe @ 2016-10-09 18:06 ` Jarkko Sakkinen 2016-10-09 18:32 ` Jarkko Sakkinen 1 sibling, 0 replies; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 18:06 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 16:49 ` Jason Gunthorpe 2016-10-09 18:06 ` Jarkko Sakkinen @ 2016-10-09 18:32 ` Jarkko Sakkinen 2016-10-09 18:33 ` Jarkko Sakkinen 1 sibling, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 18:32 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 18:32 ` Jarkko Sakkinen @ 2016-10-09 18:33 ` Jarkko Sakkinen 2016-10-09 23:07 ` Jason Gunthorpe 0 siblings, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 18:33 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 18:33 ` Jarkko Sakkinen @ 2016-10-09 23:07 ` Jason Gunthorpe 2016-10-10 0:25 ` [tpmdd-devel] " Winkler, Tomas 2016-10-10 4:45 ` Jarkko Sakkinen 0 siblings, 2 replies; 19+ messages in thread From: Jason Gunthorpe @ 2016-10-09 23:07 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 23:07 ` Jason Gunthorpe @ 2016-10-10 0:25 ` Winkler, Tomas 2016-10-10 3:26 ` Jason Gunthorpe 2016-10-10 4:59 ` Jarkko Sakkinen 2016-10-10 4:45 ` Jarkko Sakkinen 1 sibling, 2 replies; 19+ messages in thread From: Winkler, Tomas @ 2016-10-10 0:25 UTC (permalink / raw) To: Jason Gunthorpe, Jarkko Sakkinen Cc: moderated list:TPM DEVICE DRIVER, open list > -----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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-10 0:25 ` [tpmdd-devel] " Winkler, Tomas @ 2016-10-10 3:26 ` Jason Gunthorpe 2016-10-10 4:59 ` Jarkko Sakkinen 1 sibling, 0 replies; 19+ messages in thread From: Jason Gunthorpe @ 2016-10-10 3:26 UTC (permalink / raw) To: Winkler, Tomas Cc: Jarkko Sakkinen, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [tpmdd-devel] [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-10 0:25 ` [tpmdd-devel] " Winkler, Tomas 2016-10-10 3:26 ` Jason Gunthorpe @ 2016-10-10 4:59 ` Jarkko Sakkinen 1 sibling, 0 replies; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-10 4:59 UTC (permalink / raw) To: Winkler, Tomas Cc: Jason Gunthorpe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs 2016-10-09 23:07 ` Jason Gunthorpe 2016-10-10 0:25 ` [tpmdd-devel] " Winkler, Tomas @ 2016-10-10 4:45 ` Jarkko Sakkinen 1 sibling, 0 replies; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-10 4:45 UTC (permalink / raw) To: Jason Gunthorpe Cc: Peter Huewe, Marcel Selhorst, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 2/3] tpm_crb: encapsulate crb_wait_for_reg_32 2016-10-09 0:15 [PATCH RFC 0/3] Locality support for the CRB driver Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Jarkko Sakkinen @ 2016-10-09 0:15 ` Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen 2 siblings, 0 replies; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 0:15 UTC (permalink / raw) To: Peter Huewe Cc: Jarkko Sakkinen, Marcel Selhorst, Jason Gunthorpe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 2016-10-09 0:15 [PATCH RFC 0/3] Locality support for the CRB driver Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 2/3] tpm_crb: encapsulate crb_wait_for_reg_32 Jarkko Sakkinen @ 2016-10-09 0:15 ` Jarkko Sakkinen 2016-10-09 6:35 ` [tpmdd-devel] " Winkler, Tomas 2 siblings, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 0:15 UTC (permalink / raw) To: Peter Huewe Cc: Jarkko Sakkinen, Marcel Selhorst, Jason Gunthorpe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 2016-10-09 0:15 ` [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen @ 2016-10-09 6:35 ` Winkler, Tomas 2016-10-09 9:25 ` Jarkko Sakkinen 0 siblings, 1 reply; 19+ messages in thread From: Winkler, Tomas @ 2016-10-09 6:35 UTC (permalink / raw) To: Jarkko Sakkinen, Peter Huewe; +Cc: moderated list:TPM DEVICE DRIVER, open list > > 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; > } > > -- ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 2016-10-09 6:35 ` [tpmdd-devel] " Winkler, Tomas @ 2016-10-09 9:25 ` Jarkko Sakkinen 2016-10-09 9:43 ` Winkler, Tomas 0 siblings, 1 reply; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 9:25 UTC (permalink / raw) To: Winkler, Tomas; +Cc: Peter Huewe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 2016-10-09 9:25 ` Jarkko Sakkinen @ 2016-10-09 9:43 ` Winkler, Tomas 2016-10-09 10:47 ` Jarkko Sakkinen 0 siblings, 1 reply; 19+ messages in thread From: Winkler, Tomas @ 2016-10-09 9:43 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Peter Huewe, moderated list:TPM DEVICE DRIVER, open list > > > > > > > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 2016-10-09 9:43 ` Winkler, Tomas @ 2016-10-09 10:47 ` Jarkko Sakkinen 0 siblings, 0 replies; 19+ messages in thread From: Jarkko Sakkinen @ 2016-10-09 10:47 UTC (permalink / raw) To: Winkler, Tomas; +Cc: Peter Huewe, moderated list:TPM DEVICE DRIVER, open list 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-10-10 4:59 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-09 0:15 [PATCH RFC 0/3] Locality support for the CRB driver Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs Jarkko Sakkinen 2016-10-09 1:42 ` Jason Gunthorpe 2016-10-09 9:38 ` Jarkko Sakkinen 2016-10-09 16:49 ` Jason Gunthorpe 2016-10-09 18:06 ` Jarkko Sakkinen 2016-10-09 18:32 ` Jarkko Sakkinen 2016-10-09 18:33 ` Jarkko Sakkinen 2016-10-09 23:07 ` Jason Gunthorpe 2016-10-10 0:25 ` [tpmdd-devel] " Winkler, Tomas 2016-10-10 3:26 ` Jason Gunthorpe 2016-10-10 4:59 ` Jarkko Sakkinen 2016-10-10 4:45 ` Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 2/3] tpm_crb: encapsulate crb_wait_for_reg_32 Jarkko Sakkinen 2016-10-09 0:15 ` [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0 Jarkko Sakkinen 2016-10-09 6:35 ` [tpmdd-devel] " Winkler, Tomas 2016-10-09 9:25 ` Jarkko Sakkinen 2016-10-09 9:43 ` Winkler, Tomas 2016-10-09 10:47 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).