Re: [Qemu-devel] [PATCH V8 03/14] Add persistent state handling to TPM TIS frontend driver

[Paul Moore <pmoore@redhat.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<stefanb@us.ibm.com>
> >>    *         David Safford<safford@us.ibm.com>
> >>    *
> >> 
> >> + * Xen 4 support: Andrease Niederl<andreas.niederl@iaik.tugraz.at>
> >> + *
> >> 
> >>    * 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