On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:> virtio spec requires drivers to set DRIVER_OK before using VQs. > This is set automatically after probe returns, virtio console violated this > rule by adding inbufs, which causes the VQ to be used directly within > probe. > > To fix, call virtio_device_ready before using VQs. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/char/virtio_console.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index b585b47..6ebe8f6 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) > spin_lock_init(&port->outvq_lock); > init_waitqueue_head(&port->waitqueue); > > + virtio_device_ready(portdev->vdev); > + > /* Fill the in_vq with buffers so the host can send us data. */ > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > if (!nr_added_bufs) {Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK 1.839658] kernel BUG at include/linux/virtio_config.h:125! [ 1.839995] invalid opcode: 0000 [#1] SMP [ 1.840169] Modules linked in: serio_raw virtio_balloon pcspkr virtio_net virtio_console soundcore parport_pc floppy pvpanic parport i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc qxl drm_kms_helper ttm drm virtio_blk i2c_core virtio_pci ata_generic virtio_ring virtio pata_acpi [ 1.840169] CPU: 2 PID: 266 Comm: kworker/2:2 Not tainted 3.17.0+ #1 [ 1.840169] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 1.840169] Workqueue: events control_work_handler [virtio_console] [ 1.840169] task: ffff8800364bc840 ti: ffff880078490000 task.ti: ffff880078490000 [ 1.840169] RIP: 0010:[<ffffffffa01d92c6>] [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console] [ 1.840169] RSP: 0018:ffff880078493c78 EFLAGS: 00010202 [ 1.840169] RAX: 0000000000000007 RBX: ffff880036406200 RCX: 0000000000006e39 [ 1.840169] RDX: 000000000000c0b2 RSI: 0000000000000000 RDI: 000000000001c0b2 [ 1.840169] RBP: ffff880078493c78 R08: ffffffff81d2c6f8 R09: 0000000000000000 [ 1.840169] R10: 0000000000000001 R11: ffff8800364bd000 R12: ffff880036618400 [ 1.840169] R13: 0000000000000001 R14: ffff8800368c6800 R15: ffff880036406220 [ 1.840169] FS: 0000000000000000(0000) GS:ffff88007fd00000(0000) knlGS:0000000000000000 [ 1.840169] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b [ 1.840169] CR2: 00007f1c31c90000 CR3: 0000000001c14000 CR4: 00000000000006e0 [ 1.840169] Stack: [ 1.840169] ffff880078493ce8 ffffffffa01d886a ffff880000000001 ffffffff810e20cd [ 1.840169] ffff880078493cb8 0000000000000282 0000000000000000 0000000087f90194 [ 1.840169] ffff880078493ce8 ffff88007ab1d4e0 ffff880036618498 ffff880036618410 [ 1.840169] Call Trace: [ 1.840169] [<ffffffffa01d886a>] add_port+0x40a/0x440 [virtio_console] [ 1.840169] [<ffffffff810e20cd>] ? trace_hardirqs_on+0xd/0x10 [ 1.840169] [<ffffffffa01d8c6a>] control_work_handler+0x3ca/0x420 [virtio_console] [ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530 [ 1.840169] [<ffffffff810b0ef4>] process_one_work+0x1d4/0x530 [ 1.840169] [<ffffffff810b0e7b>] ? process_one_work+0x15b/0x530 [ 1.840169] [<ffffffff810b136b>] worker_thread+0x11b/0x490 [ 1.840169] [<ffffffff810b1250>] ? process_one_work+0x530/0x530 [ 1.840169] [<ffffffff810b67c3>] kthread+0xf3/0x110 [ 1.840169] [<ffffffff81788f00>] ? _raw_spin_unlock_irq+0x30/0x50 [ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240 [ 1.840169] [<ffffffff81789a7c>] ret_from_fork+0x7c/0xb0 [ 1.840169] [<ffffffff810b66d0>] ? kthread_create_on_node+0x240/0x240 [ 1.840169] Code: ff 49 89 c4 4d 85 e4 0f 8f 25 ff ff ff eb ad 48 c7 c0 f4 ff ff ff e9 17 ff ff ff e8 f5 cd eb e0 90 55 48 89 e5 0f 0b 55 48 89 e5 <0f> 0b 55 48 89 e5 0f 0b 55 48 89 e5 e8 99 e2 ff ff 48 c7 c7 c0 [ 1.840169] RIP [<ffffffffa01d92c6>] virtio_device_ready.part.12+0x4/0x6 [virtio_console] [ 1.840169] RSP <ffff880078493c78>
On 10/20/14 at 02:42pm, Cornelia Huck wrote:> On Mon, 20 Oct 2014 13:07:50 +0100 > Thomas Graf <tgraf at suug.ch> wrote: > > > On 10/13/14 at 10:50am, Michael S. Tsirkin wrote: > > > virtio spec requires drivers to set DRIVER_OK before using VQs. > > > This is set automatically after probe returns, virtio console violated this > > > rule by adding inbufs, which causes the VQ to be used directly within > > > probe. > > > > > > To fix, call virtio_device_ready before using VQs. > > > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > --- > > > drivers/char/virtio_console.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > > index b585b47..6ebe8f6 100644 > > > --- a/drivers/char/virtio_console.c > > > +++ b/drivers/char/virtio_console.c > > > @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) > > > spin_lock_init(&port->outvq_lock); > > > init_waitqueue_head(&port->waitqueue); > > > > > > + virtio_device_ready(portdev->vdev); > > > + > > > /* Fill the in_vq with buffers so the host can send us data. */ > > > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > > > if (!nr_added_bufs) { > > > > Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK > > I think we need to set this in the probe function instead, otherwise we > fail for multiqueue (which also wants to use the control queue early). > > Completely untested patch below; I can send this with proper s-o-b if > it helps.virtio_dev_probe() already sets DRIVER_OK if ->probe() returned without an error. I assume Michael added it to add_port() because probing doesn't always occur first. Can we just remove the BUG_ON()?
On 10/20/14 at 04:10pm, Michael S. Tsirkin wrote:> On Mon, Oct 20, 2014 at 01:07:50PM +0100, Thomas Graf wrote: > > On 10/13/14 at 10:50am, Michael S. Tsirkin wrote: > > > virtio spec requires drivers to set DRIVER_OK before using VQs. > > > This is set automatically after probe returns, virtio console violated this > > > rule by adding inbufs, which causes the VQ to be used directly within > > > probe. > > > > > > To fix, call virtio_device_ready before using VQs. > > > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > --- > > > drivers/char/virtio_console.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > > index b585b47..6ebe8f6 100644 > > > --- a/drivers/char/virtio_console.c > > > +++ b/drivers/char/virtio_console.c > > > @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) > > > spin_lock_init(&port->outvq_lock); > > > init_waitqueue_head(&port->waitqueue); > > > > > > + virtio_device_ready(portdev->vdev); > > > + > > > /* Fill the in_vq with buffers so the host can send us data. */ > > > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > > > if (!nr_added_bufs) { > > I see Cornelia sent a patch already. > I'd like to reproduce this though - could you send me > the command line please?1. Invoke qemu: /usr/bin/qemu-system-x86_64 -machine accel=kvm -name f20-2 -S -machine pc-i440fx-1.4,accel=kvm,usb=off -m 2048 -realtime mlock=off -smp 4,sockets=4,cores=1,threads=1 -uuid dd45ec4c-7c26-4b73-9b6b-81f2912c5235 -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/f20-2.monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/virt/f20n2-clone,if=none,id=drive-virtio-disk0,format=raw -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 -netdev tap,fd=24,id=hostnet0,vhost=on,vhostfd=25 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ef:72:4e,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev spicevmc,id=charchannel0,name=vdagent -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0 -device usb-tablet,id=input0 -spice port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x2 -device intel-hda,id=sound0,bus=pci.0,addr=0x4 -device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 2. Attach console right away: virsh console f20-2
Michael S. Tsirkin
2014-Oct-20 13:14 UTC
[PATCH v4 13/25] virtio_console: enable VQs early
On Mon, Oct 20, 2014 at 04:10:16PM +0300, Michael S. Tsirkin wrote:> On Mon, Oct 20, 2014 at 01:07:50PM +0100, Thomas Graf wrote: > > On 10/13/14 at 10:50am, Michael S. Tsirkin wrote: > > > virtio spec requires drivers to set DRIVER_OK before using VQs. > > > This is set automatically after probe returns, virtio console violated this > > > rule by adding inbufs, which causes the VQ to be used directly within > > > probe. > > > > > > To fix, call virtio_device_ready before using VQs. > > > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > --- > > > drivers/char/virtio_console.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > > index b585b47..6ebe8f6 100644 > > > --- a/drivers/char/virtio_console.c > > > +++ b/drivers/char/virtio_console.c > > > @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) > > > spin_lock_init(&port->outvq_lock); > > > init_waitqueue_head(&port->waitqueue); > > > > > > + virtio_device_ready(portdev->vdev); > > > + > > > /* Fill the in_vq with buffers so the host can send us data. */ > > > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > > > if (!nr_added_bufs) { > > I see Cornelia sent a patch already. > I'd like to reproduce this though - could you send me > the command line please?Nevermind, the trick is to add a port it seems: -device virtio-serial -chardev socket,path=/tmp/c1,server,nowait,id=foo -device virtserialport,chardev=foo,name=org.fedoraproject.port.0 works fine without -device virtserialport. -- MST
Michael S. Tsirkin
2014-Oct-20 13:35 UTC
[PATCH v4 13/25] virtio_console: enable VQs early
On Mon, Oct 20, 2014 at 02:42:23PM +0200, Cornelia Huck wrote:> On Mon, 20 Oct 2014 13:07:50 +0100 > Thomas Graf <tgraf at suug.ch> wrote: > > > On 10/13/14 at 10:50am, Michael S. Tsirkin wrote: > > > virtio spec requires drivers to set DRIVER_OK before using VQs. > > > This is set automatically after probe returns, virtio console violated this > > > rule by adding inbufs, which causes the VQ to be used directly within > > > probe. > > > > > > To fix, call virtio_device_ready before using VQs. > > > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > > --- > > > drivers/char/virtio_console.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > > index b585b47..6ebe8f6 100644 > > > --- a/drivers/char/virtio_console.c > > > +++ b/drivers/char/virtio_console.c > > > @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id) > > > spin_lock_init(&port->outvq_lock); > > > init_waitqueue_head(&port->waitqueue); > > > > > > + virtio_device_ready(portdev->vdev); > > > + > > > /* Fill the in_vq with buffers so the host can send us data. */ > > > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > > > if (!nr_added_bufs) { > > > > Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK > > I think we need to set this in the probe function instead, otherwise we > fail for multiqueue (which also wants to use the control queue early). > > Completely untested patch below; I can send this with proper s-o-b if > it helps. > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index bfa6400..cf7a561 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 id) > spin_lock_init(&port->outvq_lock); > init_waitqueue_head(&port->waitqueue); > > - virtio_device_ready(portdev->vdev); > - > /* Fill the in_vq with buffers so the host can send us data. */ > nr_added_bufs = fill_queue(port->in_vq, &port->inbuf_lock); > if (!nr_added_bufs) { > @@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev) > spin_lock_init(&portdev->ports_lock); > INIT_LIST_HEAD(&portdev->ports); > > + virtio_device_ready(portdev->vdev); > + > if (multiport) { > unsigned int nr_added_bufs; >I wanted to set DRIVER_OK as late as possible, to both reduce the chance it can fail after DRIVER_OK and to reduce the risk of introducing a regression since old qemu might only start sending interrupts after DRIVER_OK is set. So I wanted everything completely initialized before DRIVER_OK. You patch makes sense to me since nothing can fail, and we won't get interrupts before we add inbufs. Reviewed-by: Michael S. Tsirkin <mst at redhat.com> Testig will report shortly, pls send with sob line. -- MST