Subject: Re: [PATCH] vTPM: fix missing error handling for suspend operation
From: "Hon Ching(Vicky) Lo" <honclo@linux.vnet.ibm.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: tpmdd-devel@lists.sourceforge.net, Peter Huewe <PeterHuewe@gmx.de>,
        Ashley Lai <ashley@ashleylai.com>, Vicky Lo <honclo2014@gmail.com>,
        linux-kernel@vger.kernel.org
In-Reply-To: <20160304165537.GA13204@intel.com>
References: <1456899827-14365-1-git-send-email-honclo@linux.vnet.ibm.com>
	 <20160304165537.GA13204@intel.com>
Content-Type: text/plain; charset="UTF-8"
Date: Tue, 08 Mar 2016 15:44:15 -0500
Message-ID: <1457469855.30806.14.camel@vtpm2014.ibm.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4353
Lines: 122

On Fri, 2016-03-04 at 18:55 +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 02, 2016 at 01:23:47AM -0500, Hon Ching(Vicky) Lo wrote:
> > ibmvtpm_send_crq in tpm_ibmvtpm_suspend returns errors in a more
> > granular level than what the existing code does.  This patch adds
> > the missing CRQ transport event code checks to ensure appropriate
> > action taken, in the case that ibmvtpm_send_crq returns H_CLOSED.
> > 
> > Signed-off-by: Hon Ching(Vicky) Lo <honclo@linux.vnet.ibm.com>
> > ---
> >  drivers/char/tpm/tpm_ibmvtpm.c |   58 +++++++++++++++++++++++++++++++++++++---
> >  drivers/char/tpm/tpm_ibmvtpm.h |    9 ++++++
> >  2 files changed, 63 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> > index 3e6a226..5d984af 100644
> > --- a/drivers/char/tpm/tpm_ibmvtpm.c
> > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> > @@ -335,17 +335,61 @@ static int tpm_ibmvtpm_suspend(struct device *dev)
> >  	struct ibmvtpm_crq crq;
> >  	u64 *buf = (u64 *) &crq;
> >  	int rc = 0;
> > +	int sig;
> >  
> > -	crq.valid = (u8)IBMVTPM_VALID_CMD;
> > -	crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
> > +	crq_initialized = 0;
> > +	crq.valid = (u8) IBMVTPM_VALID_CMD;
> > +	crq.msg = (u8) VTPM_PREPARE_TO_SUSPEND;
> >  
> >  	rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
> >  			      cpu_to_be64(buf[1]));
> > +
> > +	if ((rc == H_CLOSED) && (crq.valid == (u8) VALID_TRANSPORT_EVENT)) {
> 
> What if rc == H_CLOSED and crq.valid != VALID_TRANSPORT_EVENT?

If that's the case, the function will return rc as the execution will
skip this if block.  

> 
> > +		if (crq.msg == (u8) PARTNER_PARTITION_SUSPENDED) {
> > +			/* 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 CRQ has been explicitly
> > +			 * enabled using the H_ENABLED_CRQ hcall.
> > +			 */
> > +			return H_SUCCESS;
> 
> I'm having trouble to understand when the suspend happens through this
> route and when you just get H_SUCCESS from ibmvtpm_send_crq(). It
> seems that there are two ways how suspend can happen.
> 
> I don't understand the big picture.

You're right.  This is not a valid case.  As I revisited it, I realized
that "partner partition suspended" transport event was handled in the 
rtas calls; vtpm doesn't have to take that into account.  I'll get rid
of this if-block.

So, upon receiving H_CLOSED only the following two events are expected
to be handled: 1) vtpm has terminated fatally. 2) partner partition
preregistered CRQ.


> > +		} else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
> > +			dev_err(ibmvtpm->dev,
> > +				"vtpm has terminated fatally; reboot to reinstate a trusted state.\n");
> > +		} else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
> > +			/* The vtpm is in the process of being reloaded by
> > +			 * firmware and has de-registered CRQ.  The client
> > +			 * must wait for the CRQ INITIALIZATION message and
> > +			 * respond and must resubmit suspend message.
> > +			 */
> > +			sig =
> > +			    wait_event_interruptible(ibmvtpm->wq,
> > +						     crq_initialized == 1);
> > +			if (sig)
> > +				return -EINTR;
> > +
> > +			if (suspend_again_count < 1) {
> > +				suspend_again_count++;
> > +				goto suspendagain;
> > +			}
> > +		} else
> > +			;
> > +	}
> > +
> >  	if (rc != H_SUCCESS)
> > -		dev_err(ibmvtpm->dev,
> > -			"tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > +		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> >  
> >  	return rc;
> > +
> > +suspendagain:
> > +	rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
> > +	suspend_again_count = 0;
> > +
> > +	if (rc != H_SUCCESS)
> > +		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > +
> > +	return rc;
> > +
> 
> Get rid of this horrible looking tail recursion thing.
> 
> What the heck is suspend_again_count and why it can be module scope
> variable? You could use a local variable instead if you would iterate
> with a loop.
> 

The reason for the 'goto' statement and the suspend_again_count was to
prevent the suspend function recurse again.  In the case if vtpm is in
the process of being reloaded by firmware, we want to wait for the CRQ
INITIALIZATION and resubmit suspend message i.e. recurse only once.


> /Jarkko
>