Re: [Qemu-devel] qemu segfauls with spiceport chardev and isa-serial

[Martin Kletzander]

On Tue, Feb 04, 2014 at 07:05:24AM +0100, Martin Kletzander wrote:
> On Tue, Feb 04, 2014 at 11:40:41AM +1000, Peter Crosthwaite wrote:
> > On Tue, Feb 4, 2014 at 4:45 AM, Dr. David Alan Gilbert
> > <address@hidden> wrote:
> > > (cc'ing in Peter Crosthwaite and Michael Tokarev due to a serial fifo 
> > > change
> > > - see below!)
> > >
> > > * Martin Kletzander (address@hidden) wrote:
> > >> Hello,
> > >
> > > Hi Martin,
> > >    I don't know about your spice warnings that triggered this but looking
> > > down the backtrace I can see something odd:
> > >
> > >> current HEAD (2f61120c10da9128357510debc8e66880cd2bfdc) segfaults when
> > >> I'm trying to do the following:
> > >>
> > >> I add this to qemu's command-line:
> > >>
> > >>  -chardev spiceport,id=charserial0,name=org.qemu.console.serial.0 \
> > >>  -device isa-serial,chardev=charserial0,id=serial0
> > >>
> > >> and then use spicy to connect to that machine.  That spits out the
> > >> following error:
> > >>
> > >>  GSpice-Message: main channel: opened
> > >>  port 0x7f74182366e0 org.qemu.console.serial.0: opened
> > >>
> > >>  (spicy:32386): GSpice-WARNING **: incomplete link header (-104/16)
> > >>
> > >>  (spicy:32386): GSpice-WARNING **: incomplete link header (-104/16)
> > >>  GSpice-Message: main channel: closed
> > >>
> > >> I can see that the console works when the window flashes, so there was
> > >> some communication done (Im running the kernel inside with
> > >> "console=tty0 console=ttyS0,115200n8" as suggested here:
> > >>
> > >> http://lists.freedesktop.org/archives/spice-devel/2014-January/015919.html
> > >>
> > >> The full command-line with backtrace of all the threads (with
> > >> abort()-ing thread being thread #1 follows.  Let me know if I can help
> > >> anyhow.
> > >>
> > >> Thanks,
> > >> Martin
> > >>
> > >> Command-line:
> > >>
> > >> qemu-system-x86_64 -name rhel7 -S -machine \
> > >> pc-i440fx-1.7,accel=kvm,usb=off,dump-guest-core=off -cpu SandyBridge \
> > >> -m 4101 -realtime mlock=off -smp 1,sockets=1,cores=1,threads=1 -uuid \
> > >> f49fa544-f21d-4267-8958-d82570644f39 -no-user-config -nodefaults \
> > >> -chardev \
> > >> socket,id=charmonitor,path=/var/lib/libvirt/qemu/rhel7.monitor,server,nowait
> > >>  \
> > >> -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc \
> > >> -no-shutdown -boot strict=on -device \
> > >> piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device \
> > >> virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 -drive \
> > >> if=none,id=drive-ide0-0-0,readonly=on,format=raw -device \
> > >> ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive \
> > >> file=/home/nert/.config/libvirt/images/rhel7.img,if=none,id=drive-virtio-disk0,format=qcow2
> > >>  \
> > >> -device \
> > >> virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
> > >>  \
> > >> -netdev tap,fd=20,id=hostnet0,vhost=on,vhostfd=21 -device \
> > >> virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:42:be:45,bus=pci.0,addr=0x3
> > >>  \
> > >> -chardev spiceport,id=charserial0,name=org.qemu.console.serial.0 \
> > >> -device isa-serial,chardev=charserial0,id=serial0 -device \
> > >> usb-tablet,id=input0 -vnc 127.0.0.1:0 -spice \
> > >> port=5901,tls-port=5902,addr=127.0.0.1,disable-ticketing,x509-dir=/etc/pki/libvirt-spice,seamless-migration=on
> > >>  \
> > >> -device \
> > >> qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x2
> > >>  \
> > >> -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> > >>
> > >> Backtrace:
> > >>
> > >
> > > <snipped boring threads in poll>
> > >
> > >> Thread 1 (Thread 0x7fee3da66980 (LWP 32022)):
> > >> #0  0x00007fee344f1f4e in __GI_raise (address@hidden) at 
> > >> ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> > >> #1  0x00007fee344f369f in __GI_abort () at abort.c:89
> > >> #2  0x00007fee3de72baa in fifo8_pop (address@hidden) at util/fifo8.c:45
> > >
> > > fifo8_pop is aborting because the fifo is empty:
> > >     if (fifo->num == 0) {
> > >         abort();
> > >     }
> > >
> > > which seems fair enough
> > >
> > >> #3  0x00007fee3dc0c110 in serial_xmit (chan=<optimized out>, 
> > >> cond=<optimized out>, opaque=0x7fee3fc286a0)
> > >>     at hw/char/serial.c:228
> > >
> > >             s->tsr = fifo8_is_full(&s->xmit_fifo) ?
> > >                         0 : fifo8_pop(&s->xmit_fifo);
> > >
> > > Hmm, now I don't know anything about the tsr stuff; but that calls
> > > fifo8_pop whenever the fifo isn't *full* - i.e. it still gets called if 
> > > empty.
> > >
> > > I think the change here comes from Peter's 8e8638fa87ff04 'char/serial: 
> > > Use generic Fifo8'
> > > changeset from June which did:
> > >
> > > -            s->tsr = fifo_get(s,XMIT_FIFO);
> > > -            if (!s->xmit_fifo.count) {
> > > +            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
> > > +                        0 : fifo8_pop(&s->xmit_fifo);
> > > +            if (!s->xmit_fifo.num) {
> > >
> > > which makes me think (without having looked at the old data structure
> > > properly) if that should be   fifo8_is_empty ?
> > > (The old serial fifo_get routine returned 0 if empty rather than 
> > > aborting).
> > >
> >
> > Hi Dave,
> >
> > Yes that does looks suss. My bad. Can you confirm your theory by
> > making the proposed change? does it fix the bug?
> >
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -225,7 +225,7 @@ static gboolean serial_xmit(GIOChannel *chan,
> > GIOCondition cond, void
> >
> >      if (s->tsr_retry <= 0) {
> >          if (s->fcr & UART_FCR_FE) {
> > -            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
> > +            s->tsr = fifo8_is_empty(&s->xmit_fifo) ?
> >                          0 : fifo8_pop(&s->xmit_fifo);
> >              if (!s->xmit_fifo.num) {
> >                  s->lsr |= UART_LSR_THRE;
> >
>
> I wish I went one line up the stack, this makes sense.  I changed this
> line to what you suggested and indeed this fixes the crash.
>
> I'm still unable to use the console (spicy shows a window which it
> shouldn't AFAIK), but that's a whole another story unrelated to this
> problem.
>

Trying a bit more I've found out this might not be the whole fix which
is needed.  When I'm trying to write to it in the guest, the process
in the guest freezes, I'm trying to isolate the issue at the moment
and will keep you posted if I'll find anything more.

Martin

> Thanks for finding that out,
> Martin
>
> > Regards,
> > Peter
> >
> > > Dave
> > >
> > >> #4  0x00007fee3d1a8957 in g_main_dispatch (context=0x7fee3fa49470)
> > >>     at 
> > >> /var/tmp/portage/dev-libs/glib-2.38.2/work/glib-2.38.2/glib/gmain.c:3066
> > >> #5  g_main_context_dispatch (address@hidden)
> > >>     at 
> > >> /var/tmp/portage/dev-libs/glib-2.38.2/work/glib-2.38.2/glib/gmain.c:3642
> > >> #6  0x00007fee3dccdde7 in glib_pollfds_poll () at main-loop.c:189
> > >> #7  os_host_main_loop_wait (timeout=<optimized out>) at main-loop.c:234
> > >> #8  main_loop_wait (nonblocking=<optimized out>) at main-loop.c:483
> > >> #9  0x00007fee3db61501 in main_loop () at vl.c:2018
> > >> #10 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized 
> > >> out>) at vl.c:4410
> > >
> > >
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > >

signature.asc
Description: Digital signature