Re: [Qemu-devel] [PATCH V14 2/7] Add TPM (frontend) hardware interface (TPM TIS) to Qemu

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

On 03/04/2012 05:59 PM, Michael S. Tsirkin wrote:
> On Fri, Mar 02, 2012 at 07:02:21AM -0500, Stefan Berger wrote:
>> Having instrumented the code with qemu_mutex_trylock() and a counter
>> for failed attempts with subsequent qemu_mutex_lock() practically
>> shows no lock contention at all for either polling or IRQ mode being
>> used by the Linux driver.
>>
>> IRQ mode: 4 failed lock attempts out of 1726208 locks ->  0.00%
>> polling mode: 2 failed lock attempts out of 1545216 locks ->  0.00%
>>
>> I used the libtpms based backend with and ran IMA and a test suite
>> in the VM.
>>
>>
>>      Stefan
> so maybe you can just do qemu_mutex_lock_iothread similar
> to what kvm does.
>

I have tried that now. I ended up having to remove the locking from the 
TIS except for one place where the result is delivered from the backend 
to the frontend via the thread. The reason why I had to remove the lock 
is because the pthread library detects a deadlock, likely due to 
attempted double-locking of the global lock that is not allowed to be 
locked more than once. That by itself is not the scary part but here's 
the stacktrace while I still had the lock in there:

#0  0x00007ffff37e2215 in __GI_raise (sig=6)
     at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ffff37e3b2b in __GI_abort () at abort.c:93
#2  0x00005555555c1069 in error_exit (err=<optimized out>, msg=
     0x55555583f350 "qemu_mutex_lock") at qemu-thread-posix.c:25
#3  0x00005555556daf10 in qemu_mutex_lock (mutex=<optimized out>)
     at qemu-thread-posix.c:56
#4  0x00005555557a6d43 in tpm_tis_mmio_read (opaque=0x555556e06a40, 
addr=3840,
     size=<optimized out>) at 
/home/stefanb/qemu/qemu-git.pt/hw/tpm_tis.c:448
#5  0x000055555575e157 in memory_region_read_accessor (opaque=<optimized 
out>,
     addr=<optimized out>, value=0x7fffef596c60, size=<optimized out>, 
shift=0,
     mask=4294967295) at /home/stefanb/qemu/qemu-git.pt/memory.c:259
#6  0x000055555575e270 in access_with_adjusted_size (addr=3840, value=
     0x7fffef596c60, size=4, access_size_min=<optimized out>,
     access_size_max=<optimized out>, access=
     0x55555575e120 <memory_region_read_accessor>, opaque=0x555556e06af0)
     at /home/stefanb/qemu/qemu-git.pt/memory.c:304
#7  0x0000555555762b80 in memory_region_dispatch_read1 (size=4, 
addr=3840, mr=
     0x555556e06af0) at /home/stefanb/qemu/qemu-git.pt/memory.c:928
#8  memory_region_dispatch_read (size=4, addr=3840, mr=0x555556e06af0)
     at /home/stefanb/qemu/qemu-git.pt/memory.c:960
#9  io_mem_read (io_index=<optimized out>, addr=3840, size=4)
     at /home/stefanb/qemu/qemu-git.pt/memory.c:1558
#10 0x000055555573a876 in cpu_physical_memory_rw (addr=4275310336, buf=
     0x7ffff7ff4028 "", len=4, is_write=0)
     at /home/stefanb/qemu/qemu-git.pt/exec.c:3620
#11 0x0000555555755225 in kvm_cpu_exec (env=0x555556561100)
     at /home/stefanb/qemu/qemu-git.pt/kvm-all.c:1173
#12 0x0000555555731201 in qemu_kvm_cpu_thread_fn (arg=0x555556561100)
     at /home/stefanb/qemu/qemu-git.pt/cpus.c:732
#13 0x00007ffff64a0b41 in start_thread (arg=0x7fffef597700)
     at pthread_create.c:305
#14 0x00007ffff388c49d in clone ()
     at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115

It may say qemu_mutex_lock but it is really calling 
qemu_mutex_lock_iothread.
The initial lock happends in kvm_cpu_exec (#11). The subsequent lock 
then occurs in tpm_tis_mmio_read (#4), so far away from the first lock. 
Well, if something in the code there changes we may end up having a race 
in the TPM TIS driver if we end up factoring the lock out. So personally 
I would prefer local locking where we can withstand changes in other 
parts of the code unless one can be sure that that global lock is never 
going to go away and that all code paths reaching the TIS driver 
function that need the locking have that global lock held.

     Stefan