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<address@hidden>
* David Safford<address@hidden>
*
+ * Xen 4 support: Andrease Niederl<address@hidden>
+ *
* 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.