From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40289) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3G4D-0000hZ-IH for qemu-devel@nongnu.org; Mon, 12 Sep 2011 19:37:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R3G4C-0007I7-3q for qemu-devel@nongnu.org; Mon, 12 Sep 2011 19:37:33 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:53368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3G4B-0007Hj-Ur for qemu-devel@nongnu.org; Mon, 12 Sep 2011 19:37:32 -0400 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by e37.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p8CNYE8Z008427 for ; Mon, 12 Sep 2011 17:34:14 -0600 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p8CNbTQN207754 for ; Mon, 12 Sep 2011 17:37:29 -0600 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p8CNbSdp015783 for ; Mon, 12 Sep 2011 17:37:28 -0600 Message-ID: <4E6E97B5.2090303@linux.vnet.ibm.com> Date: Mon, 12 Sep 2011 19:37:25 -0400 From: Stefan Berger MIME-Version: 1.0 References: <20110831143551.127339744@linux.vnet.ibm.com> <1989656.evrl8IrjSB@sifl> <4E6CE591.9080801@linux.vnet.ibm.com> <2432308.1lh7Ha8ipr@sifl> In-Reply-To: <2432308.1lh7Ha8ipr@sifl> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Paul Moore 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 09/12/2011 05:16 PM, Paul Moore wrote: > 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. > Let's give the consistency argument the favor and extend the locking over those parts that usually also get locked. Regards, Stefan