| Message ID | 1475972112-2819-2-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> -----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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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);
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(-)