Thread (40 messages) 40 messages, 6 authors, 2014-11-11

Re: [PATCH v4 13/25] virtio_console: enable VQs early

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2014-10-20 13:33:27
Also in: kvm, linux-s390, linux-scsi, lkml, virtualization

On Mon, Oct 20, 2014 at 02:42:23PM +0200, Cornelia Huck wrote:
quoted hunk ↗ jump to hunk
On Mon, 20 Oct 2014 13:07:50 +0100
Thomas Graf [off-list ref] wrote:
quoted
On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
quoted
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@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@redhat.com>

Testig will report shortly, pls send with sob line.

-- 
MST
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help