Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
@ 2020-03-18 23:49 George Wilson
  2020-03-19 19:50 ` Jarkko Sakkinen
  0 siblings, 1 reply; 5+ messages in thread
From: George Wilson @ 2020-03-18 23:49 UTC (permalink / raw)
  To: linux-integrity
  Cc: Alexey Kardashevskiy, Stefan Berger, Jarkko Sakkinen, Nayna Jain,
	Jason Gunthorpe, linux-kernel, George Wilson, Linh Pham

tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
“partner partition suspended” transport event disables the associated CRQ
such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
retries the ibmvtpm_send_crq() once.

Reported-by: Linh Pham <phaml@us.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
Tested-by: Linh Pham <phaml@us.ibm.com>
Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
---
 drivers/char/tpm/tpm_ibmvtpm.c | 136 ++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 78cc52690177..d097d47e0efa 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (C) 2012 IBM Corporation
+ * Copyright (C) 2012-2020 IBM Corporation
  *
  * Author: Ashley Lai <ashleydlai@gmail.com>
  *
@@ -133,6 +133,64 @@ static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	return len;
 }
 
+/**
+ * ibmvtpm_crq_send_init - Send a CRQ initialize message
+ * @ibmvtpm:	vtpm device struct
+ *
+ * Return:
+ *	0 on success.
+ *	Non-zero on failure.
+ */
+static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
+{
+	int rc;
+
+	rc = ibmvtpm_send_crq_word(ibmvtpm->vdev, INIT_CRQ_CMD);
+	if (rc != H_SUCCESS)
+		dev_err(ibmvtpm->dev,
+			"ibmvtpm_crq_send_init failed rc=%d\n", rc);
+
+	return rc;
+}
+
+/**
+ * tpm_ibmvtpm_resume - Resume from suspend
+ *
+ * @dev:	device struct
+ *
+ * Return: Always 0.
+ */
+static int tpm_ibmvtpm_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	int rc = 0;
+
+	do {
+		if (rc)
+			msleep(100);
+		rc = plpar_hcall_norets(H_ENABLE_CRQ,
+					ibmvtpm->vdev->unit_address);
+	} while (rc == H_IN_PROGRESS || rc == H_BUSY || H_IS_LONG_BUSY(rc));
+
+	if (rc) {
+		dev_err(dev, "Error enabling ibmvtpm rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = vio_enable_interrupts(ibmvtpm->vdev);
+	if (rc) {
+		dev_err(dev, "Error vio_enable_interrupts rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = ibmvtpm_crq_send_init(ibmvtpm);
+	if (rc)
+		dev_err(dev, "Error send_init rc=%d\n", rc);
+
+	return rc;
+}
+
 /**
  * tpm_ibmvtpm_send() - Send a TPM command
  * @chip:	tpm chip struct
@@ -146,6 +204,7 @@ static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	bool retry = true;
 	int rc, sig;
 
 	if (!ibmvtpm->rtce_buf) {
@@ -179,18 +238,27 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 	 */
 	ibmvtpm->tpm_processing_cmd = true;
 
+again:
 	rc = ibmvtpm_send_crq(ibmvtpm->vdev,
 			IBMVTPM_VALID_CMD, VTPM_TPM_COMMAND,
 			count, ibmvtpm->rtce_dma_handle);
 	if (rc != H_SUCCESS) {
+		/*
+		 * H_CLOSED can be returned after LPM resume.  Call
+		 * tpm_ibmvtpm_resume() to re-enable the CRQ then retry
+		 * ibmvtpm_send_crq() once before failing.
+		 */
+		if (rc == H_CLOSED && retry) {
+			tpm_ibmvtpm_resume(ibmvtpm->dev);
+			retry = false;
+			goto again;
+		}
 		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed rc=%d\n", rc);
-		rc = 0;
 		ibmvtpm->tpm_processing_cmd = false;
-	} else
-		rc = 0;
+	}
 
 	spin_unlock(&ibmvtpm->rtce_lock);
-	return rc;
+	return 0;
 }
 
 static void tpm_ibmvtpm_cancel(struct tpm_chip *chip)
@@ -268,26 +336,6 @@ static int ibmvtpm_crq_send_init_complete(struct ibmvtpm_dev *ibmvtpm)
 	return rc;
 }
 
-/**
- * ibmvtpm_crq_send_init - Send a CRQ initialize message
- * @ibmvtpm:	vtpm device struct
- *
- * Return:
- *	0 on success.
- *	Non-zero on failure.
- */
-static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
-{
-	int rc;
-
-	rc = ibmvtpm_send_crq_word(ibmvtpm->vdev, INIT_CRQ_CMD);
-	if (rc != H_SUCCESS)
-		dev_err(ibmvtpm->dev,
-			"ibmvtpm_crq_send_init failed rc=%d\n", rc);
-
-	return rc;
-}
-
 /**
  * tpm_ibmvtpm_remove - ibm vtpm remove entry point
  * @vdev:	vio device struct
@@ -400,44 +448,6 @@ static int ibmvtpm_reset_crq(struct ibmvtpm_dev *ibmvtpm)
 				  ibmvtpm->crq_dma_handle, CRQ_RES_BUF_SIZE);
 }
 
-/**
- * tpm_ibmvtpm_resume - Resume from suspend
- *
- * @dev:	device struct
- *
- * Return: Always 0.
- */
-static int tpm_ibmvtpm_resume(struct device *dev)
-{
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
-	int rc = 0;
-
-	do {
-		if (rc)
-			msleep(100);
-		rc = plpar_hcall_norets(H_ENABLE_CRQ,
-					ibmvtpm->vdev->unit_address);
-	} while (rc == H_IN_PROGRESS || rc == H_BUSY || H_IS_LONG_BUSY(rc));
-
-	if (rc) {
-		dev_err(dev, "Error enabling ibmvtpm rc=%d\n", rc);
-		return rc;
-	}
-
-	rc = vio_enable_interrupts(ibmvtpm->vdev);
-	if (rc) {
-		dev_err(dev, "Error vio_enable_interrupts rc=%d\n", rc);
-		return rc;
-	}
-
-	rc = ibmvtpm_crq_send_init(ibmvtpm);
-	if (rc)
-		dev_err(dev, "Error send_init rc=%d\n", rc);
-
-	return rc;
-}
-
 static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	return (status == 0);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
  2020-03-18 23:49 [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() George Wilson
@ 2020-03-19 19:50 ` Jarkko Sakkinen
  2020-03-19 19:55   ` Jarkko Sakkinen
  0 siblings, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2020-03-19 19:50 UTC (permalink / raw)
  To: George Wilson
  Cc: linux-integrity, Alexey Kardashevskiy, Stefan Berger, Nayna Jain,
	Jason Gunthorpe, linux-kernel, Linh Pham

On Wed, Mar 18, 2020 at 07:49:27PM -0400, George Wilson wrote:
> tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
> with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
> “partner partition suspended” transport event disables the associated CRQ
> such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
> until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
> This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
> ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
> retries the ibmvtpm_send_crq() once.
> 
> Reported-by: Linh Pham <phaml@us.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
> Tested-by: Linh Pham <phaml@us.ibm.com>
> Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Thank you.

/Jarkko

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
  2020-03-19 19:50 ` Jarkko Sakkinen
@ 2020-03-19 19:55   ` Jarkko Sakkinen
  2020-03-19 23:15     ` George Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2020-03-19 19:55 UTC (permalink / raw)
  To: George Wilson
  Cc: linux-integrity, Alexey Kardashevskiy, Stefan Berger, Nayna Jain,
	Jason Gunthorpe, linux-kernel, Linh Pham

On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 18, 2020 at 07:49:27PM -0400, George Wilson wrote:
> > tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
> > with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
> > “partner partition suspended” transport event disables the associated CRQ
> > such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
> > until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
> > This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
> > ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
> > retries the ibmvtpm_send_crq() once.
> > 
> > Reported-by: Linh Pham <phaml@us.ibm.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
> > Tested-by: Linh Pham <phaml@us.ibm.com>
> > Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Unfortunately have to take that back because it has checkpatch
errors:

$ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#11:
“partner partition suspended” transport event disables the associated CRQ

WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string
#61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152:
+			"ibmvtpm_crq_send_init failed rc=%d\n", rc);

Also the fixes tag is incorrect. Should be:

Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM")

/Jarkko

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
  2020-03-19 19:55   ` Jarkko Sakkinen
@ 2020-03-19 23:15     ` George Wilson
  2020-03-20  2:00       ` Jarkko Sakkinen
  0 siblings, 1 reply; 5+ messages in thread
From: George Wilson @ 2020-03-19 23:15 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, Alexey Kardashevskiy, Stefan Berger, Nayna Jain,
	Jason Gunthorpe, linux-kernel, Linh Pham

On Thu, Mar 19, 2020 at 09:55:03PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Mar 18, 2020 at 07:49:27PM -0400, George Wilson wrote:
> > > tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
> > > with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
> > > “partner partition suspended” transport event disables the associated CRQ
> > > such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
> > > until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
> > > This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
> > > ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
> > > retries the ibmvtpm_send_crq() once.
> > > 
> > > Reported-by: Linh Pham <phaml@us.ibm.com>
> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
> > > Tested-by: Linh Pham <phaml@us.ibm.com>
> > > Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Unfortunately have to take that back because it has checkpatch
> errors:
> 
> $ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #11:
> “partner partition suspended” transport event disables the associated CRQ

I'd noticed that but it appears to be a spurious checkpatch warning.
The line is 73 chars long, the same as the first line of the commit
description.  Maybe the quotes throw it off?

> 
> WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string
> #61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152:
> +			"ibmvtpm_crq_send_init failed rc=%d\n", rc);

I didn't change that error string because it's in an unmodified existing
function that I moved above the caller so a declaration wasn't required.
All other examples in the file are the same.  I'm of course happy to
change it in this function if you think it's appropriate to do so.

> 
> Also the fixes tag is incorrect. Should be:
> 
> Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM")

I see it done different ways, mostly without the path, even for the TPM
drivers.  For example, there's no path in Stefan's "[PATCH v7 2/3] tpm:
ibmvtpm: Wait for buffer to be set before proceeding."  I'm certainly
happy to change it, however, and it's good to know that's the preferred
style going forward.

Separate topic:  Since this fixes a migration hang, do you think it
should also be cc'd to stable?

> 
> /Jarkko

-- 
George Wilson
IBM Linux Technology Center
Security Development

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
  2020-03-19 23:15     ` George Wilson
@ 2020-03-20  2:00       ` Jarkko Sakkinen
  0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2020-03-20  2:00 UTC (permalink / raw)
  To: George Wilson
  Cc: linux-integrity, Alexey Kardashevskiy, Stefan Berger, Nayna Jain,
	Jason Gunthorpe, linux-kernel, Linh Pham

On Thu, Mar 19, 2020 at 06:15:52PM -0500, George Wilson wrote:
> On Thu, Mar 19, 2020 at 09:55:03PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote:
> > > On Wed, Mar 18, 2020 at 07:49:27PM -0400, George Wilson wrote:
> > > > tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
> > > > with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
> > > > “partner partition suspended” transport event disables the associated CRQ
> > > > such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
> > > > until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
> > > > This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
> > > > ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
> > > > retries the ibmvtpm_send_crq() once.
> > > > 
> > > > Reported-by: Linh Pham <phaml@us.ibm.com>
> > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
> > > > Tested-by: Linh Pham <phaml@us.ibm.com>
> > > > Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Unfortunately have to take that back because it has checkpatch
> > errors:
> > 
> > $ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch
> > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> > #11:
> > “partner partition suspended” transport event disables the associated CRQ
> 
> I'd noticed that but it appears to be a spurious checkpatch warning.
> The line is 73 chars long, the same as the first line of the commit
> description.  Maybe the quotes throw it off?

Lets just ignore this warning.

> > 
> > WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string
> > #61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152:
> > +			"ibmvtpm_crq_send_init failed rc=%d\n", rc);
> 
> I didn't change that error string because it's in an unmodified existing
> function that I moved above the caller so a declaration wasn't required.
> All other examples in the file are the same.  I'm of course happy to
> change it in this function if you think it's appropriate to do so.

What you are saying makes sense to me but given that it is rather
minuscule change I'd just sweep it away.

> > 
> > Also the fixes tag is incorrect. Should be:
> > 
> > Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM")
> 
> I see it done different ways, mostly without the path, even for the TPM
> drivers.  For example, there's no path in Stefan's "[PATCH v7 2/3] tpm:
> ibmvtpm: Wait for buffer to be set before proceeding."  I'm certainly
> happy to change it, however, and it's good to know that's the preferred
> style going forward.

"If your patch fixes a bug in a specific commit, e.g. you found an issue
using git bisect, please use the ‘Fixes:’ tag with the first 12
characters of the SHA-1 ID, and the one line summary."

https://www.kernel.org/doc/html/v5.5/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

> Separate topic:  Since this fixes a migration hang, do you think it
> should also be cc'd to stable?

Sure, it would make sense.

/Jarkko

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 23:49 [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() George Wilson
2020-03-19 19:50 ` Jarkko Sakkinen
2020-03-19 19:55   ` Jarkko Sakkinen
2020-03-19 23:15     ` George Wilson
2020-03-20  2:00       ` Jarkko Sakkinen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git