From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3Rs1-0000Un-Ho for qemu-devel@nongnu.org; Tue, 13 Sep 2011 08:13:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R3Rrv-0002ho-P9 for qemu-devel@nongnu.org; Tue, 13 Sep 2011 08:13:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53592) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R3Rrv-0002hR-I2 for qemu-devel@nongnu.org; Tue, 13 Sep 2011 08:13:39 -0400 From: Paul Moore Date: Tue, 13 Sep 2011 08:13:06 -0400 Message-ID: <3897760.kOCyxHAs0a@sifl> In-Reply-To: <4E6E97B5.2090303@linux.vnet.ibm.com> References: <20110831143551.127339744@linux.vnet.ibm.com> <2432308.1lh7Ha8ipr@sifl> <4E6E97B5.2090303@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 Monday, September 12, 2011 07:37:25 PM Stefan Berger wrote: > 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. Great, thanks. -- paul moore virtualization @ redhat