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.
--
paul moore
virtualization @ redhat
