From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3DsC-0000ps-3P for qemu-devel@nongnu.org; Mon, 12 Sep 2011 17:17:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R3DsA-0008A0-JQ for qemu-devel@nongnu.org; Mon, 12 Sep 2011 17:16:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41787) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3DsA-00082O-CH for qemu-devel@nongnu.org; Mon, 12 Sep 2011 17:16:58 -0400 From: Paul Moore Date: Mon, 12 Sep 2011 17:16:24 -0400 Message-ID: <2432308.1lh7Ha8ipr@sifl> In-Reply-To: <4E6CE591.9080801@linux.vnet.ibm.com> References: <20110831143551.127339744@linux.vnet.ibm.com> <1989656.evrl8IrjSB@sifl> <4E6CE591.9080801@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [Qemu-devel] [PATCH V8 03/14] Add persistent state handling to TPM TIS frontend driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: chrisw@redhat.com, anbang.ruan@cs.ox.ac.uk, qemu-devel@nongnu.org, andreas.niederl@iaik.tugraz.at, alevy@redhat.com, rrelyea@redhat.com, serge@hallyn.com On Sunday, September 11, 2011 12:45:05 PM Stefan Berger wrote: > On 09/09/2011 05:13 PM, Paul Moore wrote: > > On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote: > >> Index: qemu-git/hw/tpm_tis.c > >> =================================================================== > >> --- qemu-git.orig/hw/tpm_tis.c > >> +++ qemu-git/hw/tpm_tis.c > >> @@ -6,6 +6,8 @@ > >> > >> * Author: Stefan Berger > >> * David Safford > >> * > >> > >> + * Xen 4 support: Andrease Niederl > >> + * > >> > >> * This program is free software; you can redistribute it and/or > >> * modify it under the terms of the GNU General Public License as > >> * published by the Free Software Foundation, version 2 of the > >> > >> @@ -839,3 +841,167 @@ static int tis_init(ISADevice *dev) > >> > >> err_exit: > >> return -1; > >> > >> } > >> > >> + > >> +/* persistent state handling */ > >> + > >> +static void tis_pre_save(void *opaque) > >> +{ > >> + TPMState *s = opaque; > >> + uint8_t locty = s->active_locty; > > > > Is it safe to read s->active_locty without the state_lock? I'm not sure > > at this point but I saw it being protected by the lock elsewhere ... > It cannot change anymore since no vCPU is in the TPM TIS emulation layer > anymore but all we're doing is wait for the last outstanding command to > be returned to use from the TPM thread. > I don't mind putting this reading into the critical section, though, > just to have it be consistent. [Dropping the rest of the comments since they all cover the same issue] I'm a big fan of consistency, especially when it comes to locking; inconsistent lock usage can lead to confusion and that is almost never good. If we need a lock here because there is the potential for an outstanding TPM command, then I vote for locking in this function just as you would in any other. However, if we really don't need locks here because the outstanding TPM command will have _no_ effect on the TPMState or any related structure, then just do away with the lock completely and make of note of it in the function explaining why. -- paul moore virtualization @ redhat