[Qemu-devel] Re: kvm crashes with spice while loading qxl

[Marcelo Tosatti]

On Sun, Mar 06, 2011 at 12:30:59PM +0200, Alon Levy wrote:
> On Sat, Mar 05, 2011 at 01:35:58PM -0300, Marcelo Tosatti wrote:
> > On Sat, Feb 26, 2011 at 01:29:01PM +0100, Jan Kiszka wrote:
> > > >     at 
> > > > /var/tmp/portage/app-emulation/qemu-kvm-0.14.0/work/qemu-kvm-0.14.0/qemu-kvm.c:1466
> > > > #12 0x00007ffff77bb944 in start_thread () from /lib/libpthread.so.0
> > > > #13 0x00007ffff5e491dd in clone () from /lib/libc.so.6
> > > > (gdb)
> > > 
> > > That's a spice bug. In fact, there are a lot of
> > > qemu_mutex_lock/unlock_iothread in that subsystem. I bet at least a few
> > > of them can cause even more subtle problems.
> > > 
> > > Two general issues with dropping the global mutex like this:
> > >  - The caller of mutex_unlock is responsible for maintaining
> > >    cpu_single_env across the unlocked phase (that's related to the
> > >    abort above).
> > >  - Dropping the lock in the middle of a callback is risky. That may
> > >    enable re-entrances of code sections that weren't designed for this
> > >    (I'm skeptic about the side effects of
> > >    qemu_spice_vm_change_state_handler - why dropping the lock here?).
> > > 
> > > Spice requires a careful review regarding such issues. Or it should
> > > pioneer with introducing its own lock so that we can handle at least
> > > related I/O activities over the VCPUs without holding the global mutex
> > > (but I bet it's not the simplest candidate for such a new scheme).
> > > 
> > > Jan
> > > 
> > 
> > Agree with the concern regarding spice.
> > 
> 
> What are the pros and cons of (re)introducing a spice specific lock?
>  + simplicity. Only spice touches the spice lock.
>  - ? what were the original reasons for Gerd dropping the spice lock?
> 
> I have no problem reintroducing this lock, I'm just concerned that it's
> wasted effort because after I send that patch someone will jump and remind
> me why it was removed in the first place.

Well, can't comment on why it was done or a proper way to fix it.
Point is that dropping the global lock requires careful review to verify
safety, as Jan mentioned.
For example, a potential problem would be:

  vcpu context                  iothread context
  qxl pio write 
  drop lock 
                                acquire lock
                                1) change state
                                drop lock
  acquire lock

1) could be device hotunplug, system reset, etc.