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

[Paul Moore <pmoore@redhat.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<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.
> 
> 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