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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

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.

Regards,
     Stefan