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<stef...@us.ibm.com>
> >>>> * David Safford<saff...@us.ibm.com>
> >>>> *
> >>>>
> >>>> + * Xen 4 support: Andrease
> >>>> Niederl<andreas.nied...@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
