[PATCH 00/10] (v5) virtio: console: Fixes, new way of discovering ports

[PATCH 00/10] (v5) virtio: console: Fixes, new way of discovering ports

[Amit Shah]

Hello,

This patchset is still based on Linus' tree but I'll rebase when Rusty
sends out the disable multiport patches.

This series reworks the ABI to allow port discovery (only) via the
control queue.

In addition, it adds support for non-blocking write() support, which
means no spinning. This works fine with the recent patches that are on
qemu-devel.

Also included is removal of hvc_remove() as removing one such console
port causes other console ports (registered with hvc) to stall. This
has to be debugged in the hvc_console.c file, I'll do that later, but
we have a nice workaround for this: returning -EPIPE on any hvc
operations will make the hvc console core perform any cleanups for the
removed ports. Looks like we don't lose much by removing hvc_remove().

New in this version:
- Complete support for non-blocking write()
- Don't call hvc_remove()
- Return -EPIPE for hvc operations if a console port was
  hot-unplugged.

Amit Shah (10):
  virtio: console: Add a __send_control_msg() that can send messages
    without a valid port
  virtio: console: Let host know of port or device add failures
  virtio: console: Return -EPIPE to hvc_console if we lost the
    connection
  virtio: console: Don't call hvc_remove() on unplugging console ports
  virtio: console: Remove config work handler
  virtio: console: Move code around for future patches
  virtio: console: Use a control message to add ports
  virtio: console: Don't always create a port 0 if using multiport
  virtio: console: Rename wait_is_over() to will_read_block()
  virtio: console: Add support for nonblocking write()s

 drivers/char/virtio_console.c  |  533 +++++++++++++++++++++-------------------
 include/linux/virtio_console.h |   18 +-
 2 files changed, 288 insertions(+), 263 deletions(-)

[PATCH 01/10] virtio: console: Add a __send_control_msg() that can send messages without a valid port

[Amit Shah]

We will introduce control messages that operate on the device as a whole
rather than just ports. Make send_control_msg() a wrapper around
__send_control_msg() which does not need a valid port.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8f665bc..5d24015 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -373,22 +373,22 @@ out:
 	return ret;
 }
 
-static ssize_t send_control_msg(struct port *port, unsigned int event,
-				unsigned int value)
+static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
+				  unsigned int event, unsigned int value)
 {
 	struct scatterlist sg[1];
 	struct virtio_console_control cpkt;
 	struct virtqueue *vq;
 	unsigned int len;
 
-	if (!use_multiport(port->portdev))
+	if (!use_multiport(portdev))
 		return 0;
 
-	cpkt.id = port->id;
+	cpkt.id = port_id;
 	cpkt.event = event;
 	cpkt.value = value;
 
-	vq = port->portdev->c_ovq;
+	vq = portdev->c_ovq;
 
 	sg_init_one(sg, &cpkt, sizeof(cpkt));
 	if (vq->vq_ops->add_buf(vq, sg, 1, 0, &cpkt) >= 0) {
@@ -399,6 +399,12 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	return 0;
 }
 
+static ssize_t send_control_msg(struct port *port, unsigned int event,
+				unsigned int value)
+{
+	return __send_control_msg(port->portdev, port->id, event, value);
+}
+
 static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
 {
 	struct scatterlist sg[1];
-- 
1.6.2.5

[PATCH 02/10] virtio: console: Let host know of port or device add failures

[Amit Shah]

The host may want to know and let management apps notify of port or
device add failures. Send a control message saying the device or port is
not ready in this case.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c  |    5 +++++
 include/linux/virtio_console.h |    3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5d24015..1ae11f4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1217,6 +1217,8 @@ free_cdev:
 free_port:
 	kfree(port);
 fail:
+	/* The host might want to notify management sw about port add failure */
+	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
 	return err;
 }
 
@@ -1495,6 +1497,9 @@ free_chrdev:
 free:
 	kfree(portdev);
 fail:
+	/* The host might want to notify mgmt sw about device add failure */
+	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+			   VIRTIO_CONSOLE_DEVICE_READY, 0);
 	return err;
 }
 
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ae4f039..0157361 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -14,6 +14,8 @@
 #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
 #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
 
+#define VIRTIO_CONSOLE_BAD_ID		(~(u32)0)
+
 struct virtio_console_config {
 	/* colums of the screens */
 	__u16 cols;
@@ -42,6 +44,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
 #define VIRTIO_CONSOLE_PORT_REMOVE	5
+#define VIRTIO_CONSOLE_DEVICE_READY	6
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

[PATCH 03/10] virtio: console: Return -EPIPE to hvc_console if we lost the connection

[Amit Shah]

hvc_console handles -EPIPE properly when the connection to the host is
lost.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1ae11f4..733276d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -660,7 +660,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 
 	port = find_port_by_vtermno(vtermno);
 	if (!port)
-		return 0;
+		return -EPIPE;
 
 	return send_buf(port, (void *)buf, count);
 }
@@ -676,9 +676,13 @@ static int get_chars(u32 vtermno, char *buf, int count)
 {
 	struct port *port;
 
+	/* If we've not set up the port yet, we have no input to give. */
+	if (unlikely(early_put_chars))
+		return 0;
+
 	port = find_port_by_vtermno(vtermno);
 	if (!port)
-		return 0;
+		return -EPIPE;
 
 	/* If we don't have an input queue yet, we can't get input. */
 	BUG_ON(!port->in_vq);
-- 
1.6.2.5

[PATCH 04/10] virtio: console: Don't call hvc_remove() on unplugging console ports

[Amit Shah]

hvc_remove() has some bug which freezes other active hvc ports when one
port is removed.

So disable calling of hvc_remove() which deregisters a port with the
hvc_console.

If the hvc_console code calls into our get_chars() routine as a result
of a poll operation, we will return -EPIPE and the hvc_console code will
then do the necessary cleanup.

This call will be restored when the bug in hvc_remove() is found and
fixed.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 733276d..b40b112 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -876,7 +876,18 @@ static int remove_port(struct port *port)
 		spin_lock_irq(&pdrvdata_lock);
 		list_del(&port->cons.list);
 		spin_unlock_irq(&pdrvdata_lock);
+#if 0
+		/*
+		 * hvc_remove() not called as removing one hvc port
+		 * results in other hvc ports getting frozen.
+		 *
+		 * Once this is resolved in hvc, this functionality
+		 * will be enabled.  Till that is done, the -EPIPE
+		 * return from get_chars() above will help
+		 * hvc_console.c to clean up on ports we remove here.
+		 */
 		hvc_remove(port->cons.hvc);
+#endif
 	}
 	if (port->guest_connected)
 		send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
-- 
1.6.2.5

[PATCH 05/10] virtio: console: Remove config work handler

[Amit Shah]

We're going to switch to using control messages for port hot-plug and
initial port discovery. Remove the config work handler which handled
port hot-plug so far.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   64 +----------------------------------------
 1 files changed, 1 insertions(+), 63 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b40b112..0473221 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -109,7 +109,6 @@ struct ports_device {
 	 * notification
 	 */
 	struct work_struct control_work;
-	struct work_struct config_work;
 
 	struct list_head ports;
 
@@ -1091,10 +1090,7 @@ static void config_intr(struct virtio_device *vdev)
 	struct ports_device *portdev;
 
 	portdev = vdev->priv;
-	if (use_multiport(portdev)) {
-		/* Handle port hot-add */
-		schedule_work(&portdev->config_work);
-	}
+
 	/*
 	 * We'll use this way of resizing only for legacy support.
 	 * For newer userspace (VIRTIO_CONSOLE_F_MULTPORT+), use
@@ -1237,62 +1233,6 @@ fail:
 	return err;
 }
 
-/*
- * The workhandler for config-space updates.
- *
- * This is called when ports are hot-added.
- */
-static void config_work_handler(struct work_struct *work)
-{
-	struct virtio_console_config virtconconf;
-	struct ports_device *portdev;
-	struct virtio_device *vdev;
-	int err;
-
-	portdev = container_of(work, struct ports_device, config_work);
-
-	vdev = portdev->vdev;
-	vdev->config->get(vdev,
-			  offsetof(struct virtio_console_config, nr_ports),
-			  &virtconconf.nr_ports,
-			  sizeof(virtconconf.nr_ports));
-
-	if (portdev->config.nr_ports == virtconconf.nr_ports) {
-		/*
-		 * Port 0 got hot-added.  Since we already did all the
-		 * other initialisation for it, just tell the Host
-		 * that the port is ready if we find the port.  In
-		 * case the port was hot-removed earlier, we call
-		 * add_port to add the port.
-		 */
-		struct port *port;
-
-		port = find_port_by_id(portdev, 0);
-		if (!port)
-			add_port(portdev, 0);
-		else
-			send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
-		return;
-	}
-	if (virtconconf.nr_ports > portdev->config.max_nr_ports) {
-		dev_warn(&vdev->dev,
-			 "More ports specified (%u) than allowed (%u)",
-			 portdev->config.nr_ports + 1,
-			 portdev->config.max_nr_ports);
-		return;
-	}
-	if (virtconconf.nr_ports < portdev->config.nr_ports)
-		return;
-
-	/* Hot-add ports */
-	while (virtconconf.nr_ports - portdev->config.nr_ports) {
-		err = add_port(portdev, portdev->config.nr_ports);
-		if (err)
-			break;
-		portdev->config.nr_ports++;
-	}
-}
-
 static int init_vqs(struct ports_device *portdev)
 {
 	vq_callback_t **io_callbacks;
@@ -1485,7 +1425,6 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 		spin_lock_init(&portdev->cvq_lock);
 		INIT_WORK(&portdev->control_work, &control_work_handler);
-		INIT_WORK(&portdev->config_work, &config_work_handler);
 
 		nr_added_bufs = fill_queue(portdev->c_ivq, &portdev->cvq_lock);
 		if (!nr_added_bufs) {
@@ -1528,7 +1467,6 @@ static void virtcons_remove(struct virtio_device *vdev)
 	portdev = vdev->priv;
 
 	cancel_work_sync(&portdev->control_work);
-	cancel_work_sync(&portdev->config_work);
 
 	list_for_each_entry_safe(port, port2, &portdev->ports, list)
 		remove_port(port);
-- 
1.6.2.5

[PATCH 06/10] virtio: console: Move code around for future patches

[Amit Shah]

We're going to use add_port() from handle_control_message() in the next
patch.

Move the add_port() and fill_queue(), which depends on it, above
handle_control_message() to avoid forward declarations.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |  264 ++++++++++++++++++++---------------------
 1 files changed, 131 insertions(+), 133 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0473221..fd98861 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -862,6 +862,137 @@ static const struct file_operations port_debugfs_ops = {
 	.read  = debugfs_read,
 };
 
+static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
+{
+	struct port_buffer *buf;
+	unsigned int nr_added_bufs;
+	int ret;
+
+	nr_added_bufs = 0;
+	do {
+		buf = alloc_buf(PAGE_SIZE);
+		if (!buf)
+			break;
+
+		spin_lock_irq(lock);
+		ret = add_inbuf(vq, buf);
+		if (ret < 0) {
+			spin_unlock_irq(lock);
+			free_buf(buf);
+			break;
+		}
+		nr_added_bufs++;
+		spin_unlock_irq(lock);
+	} while (ret > 0);
+
+	return nr_added_bufs;
+}
+
+static int add_port(struct ports_device *portdev, u32 id)
+{
+	char debugfs_name[16];
+	struct port *port;
+	struct port_buffer *buf;
+	dev_t devt;
+	unsigned int nr_added_bufs;
+	int err;
+
+	port = kmalloc(sizeof(*port), GFP_KERNEL);
+	if (!port) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	port->portdev = portdev;
+	port->id = id;
+
+	port->name = NULL;
+	port->inbuf = NULL;
+	port->cons.hvc = NULL;
+
+	port->host_connected = port->guest_connected = false;
+
+	port->in_vq = portdev->in_vqs[port->id];
+	port->out_vq = portdev->out_vqs[port->id];
+
+	cdev_init(&port->cdev, &port_fops);
+
+	devt = MKDEV(portdev->chr_major, id);
+	err = cdev_add(&port->cdev, devt, 1);
+	if (err < 0) {
+		dev_err(&port->portdev->vdev->dev,
+			"Error %d adding cdev for port %u\n", err, id);
+		goto free_port;
+	}
+	port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
+				  devt, port, "vport%up%u",
+				  port->portdev->drv_index, id);
+	if (IS_ERR(port->dev)) {
+		err = PTR_ERR(port->dev);
+		dev_err(&port->portdev->vdev->dev,
+			"Error %d creating device for port %u\n",
+			err, id);
+		goto free_cdev;
+	}
+
+	spin_lock_init(&port->inbuf_lock);
+	init_waitqueue_head(&port->waitqueue);
+
+	/* 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) {
+		dev_err(port->dev, "Error allocating inbufs\n");
+		err = -ENOMEM;
+		goto free_device;
+	}
+
+	/*
+	 * If we're not using multiport support, this has to be a console port
+	 */
+	if (!use_multiport(port->portdev)) {
+		err = init_port_console(port);
+		if (err)
+			goto free_inbufs;
+	}
+
+	spin_lock_irq(&portdev->ports_lock);
+	list_add_tail(&port->list, &port->portdev->ports);
+	spin_unlock_irq(&portdev->ports_lock);
+
+	/*
+	 * Tell the Host we're set so that it can send us various
+	 * configuration parameters for this port (eg, port name,
+	 * caching, whether this is a console port, etc.)
+	 */
+	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+
+	if (pdrvdata.debugfs_dir) {
+		/*
+		 * Finally, create the debugfs file that we can use to
+		 * inspect a port's state at any time
+		 */
+		sprintf(debugfs_name, "vport%up%u",
+			port->portdev->drv_index, id);
+		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
+							 pdrvdata.debugfs_dir,
+							 port,
+							 &port_debugfs_ops);
+	}
+	return 0;
+
+free_inbufs:
+	while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
+		free_buf(buf);
+free_device:
+	device_destroy(pdrvdata.class, port->dev->devt);
+free_cdev:
+	cdev_del(&port->cdev);
+free_port:
+	kfree(port);
+fail:
+	return err;
+}
+
 /* Remove all port-specific data. */
 static int remove_port(struct port *port)
 {
@@ -1100,139 +1231,6 @@ static void config_intr(struct virtio_device *vdev)
 	resize_console(find_port_by_id(portdev, 0));
 }
 
-static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
-{
-	struct port_buffer *buf;
-	unsigned int nr_added_bufs;
-	int ret;
-
-	nr_added_bufs = 0;
-	do {
-		buf = alloc_buf(PAGE_SIZE);
-		if (!buf)
-			break;
-
-		spin_lock_irq(lock);
-		ret = add_inbuf(vq, buf);
-		if (ret < 0) {
-			spin_unlock_irq(lock);
-			free_buf(buf);
-			break;
-		}
-		nr_added_bufs++;
-		spin_unlock_irq(lock);
-	} while (ret > 0);
-
-	return nr_added_bufs;
-}
-
-static int add_port(struct ports_device *portdev, u32 id)
-{
-	char debugfs_name[16];
-	struct port *port;
-	struct port_buffer *buf;
-	dev_t devt;
-	unsigned int nr_added_bufs;
-	int err;
-
-	port = kmalloc(sizeof(*port), GFP_KERNEL);
-	if (!port) {
-		err = -ENOMEM;
-		goto fail;
-	}
-
-	port->portdev = portdev;
-	port->id = id;
-
-	port->name = NULL;
-	port->inbuf = NULL;
-	port->cons.hvc = NULL;
-
-	port->host_connected = port->guest_connected = false;
-
-	port->in_vq = portdev->in_vqs[port->id];
-	port->out_vq = portdev->out_vqs[port->id];
-
-	cdev_init(&port->cdev, &port_fops);
-
-	devt = MKDEV(portdev->chr_major, id);
-	err = cdev_add(&port->cdev, devt, 1);
-	if (err < 0) {
-		dev_err(&port->portdev->vdev->dev,
-			"Error %d adding cdev for port %u\n", err, id);
-		goto free_port;
-	}
-	port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
-				  devt, port, "vport%up%u",
-				  port->portdev->drv_index, id);
-	if (IS_ERR(port->dev)) {
-		err = PTR_ERR(port->dev);
-		dev_err(&port->portdev->vdev->dev,
-			"Error %d creating device for port %u\n",
-			err, id);
-		goto free_cdev;
-	}
-
-	spin_lock_init(&port->inbuf_lock);
-	init_waitqueue_head(&port->waitqueue);
-
-	/* 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) {
-		dev_err(port->dev, "Error allocating inbufs\n");
-		err = -ENOMEM;
-		goto free_device;
-	}
-
-	/*
-	 * If we're not using multiport support, this has to be a console port
-	 */
-	if (!use_multiport(port->portdev)) {
-		err = init_port_console(port);
-		if (err)
-			goto free_inbufs;
-	}
-
-	spin_lock_irq(&portdev->ports_lock);
-	list_add_tail(&port->list, &port->portdev->ports);
-	spin_unlock_irq(&portdev->ports_lock);
-
-	/*
-	 * Tell the Host we're set so that it can send us various
-	 * configuration parameters for this port (eg, port name,
-	 * caching, whether this is a console port, etc.)
-	 */
-	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
-
-	if (pdrvdata.debugfs_dir) {
-		/*
-		 * Finally, create the debugfs file that we can use to
-		 * inspect a port's state at any time
-		 */
-		sprintf(debugfs_name, "vport%up%u",
-			port->portdev->drv_index, id);
-		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
-							 pdrvdata.debugfs_dir,
-							 port,
-							 &port_debugfs_ops);
-	}
-	return 0;
-
-free_inbufs:
-	while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
-		free_buf(buf);
-free_device:
-	device_destroy(pdrvdata.class, port->dev->devt);
-free_cdev:
-	cdev_del(&port->cdev);
-free_port:
-	kfree(port);
-fail:
-	/* The host might want to notify management sw about port add failure */
-	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
-	return err;
-}
-
 static int init_vqs(struct ports_device *portdev)
 {
 	vq_callback_t **io_callbacks;
-- 
1.6.2.5

[PATCH 07/10] virtio: console: Use a control message to add ports

[Amit Shah]

Instead of the host and guest independently enumerating ports, switch to
a control message to add ports where the host supplies the port number
so there's no ambiguity or a possibility of a race between the host and
the guest port numbers.

We now no longer need the 'nr_ports' config value. Since no kernel has
been released with the MULTIPORT changes yet, we have a chance to fiddle
with the config space without adding compatibility features.

This is beneficial for management software, which would now be able to
instantiate ports at known locations and avoid problems that arise with
implicit numbering in the host and the guest. This removes the 'guessing
game' part of it, and management software can now actually indicate
which id to spawn a particular port on.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c  |   78 +++++++++++++++++----------------------
 include/linux/virtio_console.h |   17 ++++-----
 2 files changed, 42 insertions(+), 53 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fd98861..7b2dcdd 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1053,7 +1053,7 @@ static void handle_control_message(struct ports_device *portdev,
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
 
 	port = find_port_by_id(portdev, cpkt->id);
-	if (!port) {
+	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
 		/* No valid header at start of buffer.  Drop it. */
 		dev_dbg(&portdev->vdev->dev,
 			"Invalid index %u in control packet\n", cpkt->id);
@@ -1061,6 +1061,30 @@ static void handle_control_message(struct ports_device *portdev,
 	}
 
 	switch (cpkt->event) {
+	case VIRTIO_CONSOLE_PORT_ADD:
+		if (port) {
+			/*
+			 * This can happen for port 0: we have to
+			 * create a console port during probe() as was
+			 * the behaviour before the MULTIPORT feature.
+			 * On a newer host, when the host tells us
+			 * that a port 0 is available, we should just
+			 * say we have the port all set up.
+			 */
+			send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+			break;
+		}
+		if (cpkt->id >= portdev->config.max_nr_ports) {
+			dev_warn(&portdev->vdev->dev,
+				"Request for adding port with out-of-bound id %u, max. supported id: %u\n",
+				cpkt->id, portdev->config.max_nr_ports - 1);
+			break;
+		}
+		add_port(portdev, cpkt->id);
+		break;
+	case VIRTIO_CONSOLE_PORT_REMOVE:
+		remove_port(port);
+		break;
 	case VIRTIO_CONSOLE_CONSOLE_PORT:
 		if (!cpkt->value)
 			break;
@@ -1119,32 +1143,6 @@ static void handle_control_message(struct ports_device *portdev,
 			kobject_uevent(&port->dev->kobj, KOBJ_CHANGE);
 		}
 		break;
-	case VIRTIO_CONSOLE_PORT_REMOVE:
-		/*
-		 * Hot unplug the port.  We don't decrement nr_ports
-		 * since we don't want to deal with extra complexities
-		 * of using the lowest-available port id: We can just
-		 * pick up the nr_ports number as the id and not have
-		 * userspace send it to us.  This helps us in two
-		 * ways:
-		 *
-		 * - We don't need to have a 'port_id' field in the
-		 *   config space when a port is hot-added.  This is a
-		 *   good thing as we might queue up multiple hotplug
-		 *   requests issued in our workqueue.
-		 *
-		 * - Another way to deal with this would have been to
-		 *   use a bitmap of the active ports and select the
-		 *   lowest non-active port from that map.  That
-		 *   bloats the already tight config space and we
-		 *   would end up artificially limiting the
-		 *   max. number of ports to sizeof(bitmap).  Right
-		 *   now we can support 2^32 ports (as the port id is
-		 *   stored in a u32 type).
-		 *
-		 */
-		remove_port(port);
-		break;
 	}
 }
 
@@ -1352,7 +1350,6 @@ static const struct file_operations portdev_fops = {
 static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
 	struct ports_device *portdev;
-	u32 i;
 	int err;
 	bool multiport;
 
@@ -1381,29 +1378,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	}
 
 	multiport = false;
-	portdev->config.nr_ports = 1;
 	portdev->config.max_nr_ports = 1;
 	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
 		multiport = true;
 		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
 
 		vdev->config->get(vdev, offsetof(struct virtio_console_config,
-						 nr_ports),
-				  &portdev->config.nr_ports,
-				  sizeof(portdev->config.nr_ports));
-		vdev->config->get(vdev, offsetof(struct virtio_console_config,
 						 max_nr_ports),
 				  &portdev->config.max_nr_ports,
 				  sizeof(portdev->config.max_nr_ports));
-		if (portdev->config.nr_ports > portdev->config.max_nr_ports) {
-			dev_warn(&vdev->dev,
-				 "More ports (%u) specified than allowed (%u). Will init %u ports.",
-				 portdev->config.nr_ports,
-				 portdev->config.max_nr_ports,
-				 portdev->config.max_nr_ports);
-
-			portdev->config.nr_ports = portdev->config.max_nr_ports;
-		}
 	}
 
 	/* Let the Host know we support multiple ports.*/
@@ -1431,13 +1414,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 			err = -ENOMEM;
 			goto free_vqs;
 		}
+
 	}
 
-	for (i = 0; i < portdev->config.nr_ports; i++)
-		add_port(portdev, i);
+	/*
+	 * For backward compatibility: if we're running on an older
+	 * host, we always want to create a console port.
+	 */
+	add_port(portdev, 0);
 
 	/* Start using the new console output. */
 	early_put_chars = NULL;
+
+	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+			   VIRTIO_CONSOLE_DEVICE_READY, 1);
 	return 0;
 
 free_vqs:
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 0157361..a85064d 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -23,8 +23,6 @@ struct virtio_console_config {
 	__u16 rows;
 	/* max. number of ports this device can hold */
 	__u32 max_nr_ports;
-	/* number of ports added so far */
-	__u32 nr_ports;
 } __attribute__((packed));
 
 /*
@@ -38,13 +36,14 @@ struct virtio_console_control {
 };
 
 /* Some events for control messages */
-#define VIRTIO_CONSOLE_PORT_READY	0
-#define VIRTIO_CONSOLE_CONSOLE_PORT	1
-#define VIRTIO_CONSOLE_RESIZE		2
-#define VIRTIO_CONSOLE_PORT_OPEN	3
-#define VIRTIO_CONSOLE_PORT_NAME	4
-#define VIRTIO_CONSOLE_PORT_REMOVE	5
-#define VIRTIO_CONSOLE_DEVICE_READY	6
+#define VIRTIO_CONSOLE_DEVICE_READY	0
+#define VIRTIO_CONSOLE_PORT_ADD		1
+#define VIRTIO_CONSOLE_PORT_REMOVE	2
+#define VIRTIO_CONSOLE_PORT_READY	3
+#define VIRTIO_CONSOLE_CONSOLE_PORT	4
+#define VIRTIO_CONSOLE_RESIZE		5
+#define VIRTIO_CONSOLE_PORT_OPEN	6
+#define VIRTIO_CONSOLE_PORT_NAME	7
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

[PATCH 08/10] virtio: console: Don't always create a port 0 if using multiport

[Amit Shah]

If we're using multiport, there's no point in always creating a console
port. Create the console port only if the host doesn't support
multiport.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7b2dcdd..3d70729 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -791,6 +791,13 @@ int init_port_console(struct port *port)
 	spin_unlock_irq(&pdrvdata_lock);
 	port->guest_connected = true;
 
+	/*
+	 * Start using the new console output if this is the first
+	 * console to come up.
+	 */
+	if (early_put_chars)
+		early_put_chars = NULL;
+
 	/* Notify host of port being opened */
 	send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
@@ -1063,14 +1070,8 @@ static void handle_control_message(struct ports_device *portdev,
 	switch (cpkt->event) {
 	case VIRTIO_CONSOLE_PORT_ADD:
 		if (port) {
-			/*
-			 * This can happen for port 0: we have to
-			 * create a console port during probe() as was
-			 * the behaviour before the MULTIPORT feature.
-			 * On a newer host, when the host tells us
-			 * that a port 0 is available, we should just
-			 * say we have the port all set up.
-			 */
+			dev_dbg(&portdev->vdev->dev,
+				"Port %u already added\n", port->id);
 			send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
 			break;
 		}
@@ -1414,18 +1415,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 			err = -ENOMEM;
 			goto free_vqs;
 		}
-
+	} else {
+		/*
+		 * If we're running on an older host, we always want
+		 * to create a console port.
+		 */
+		add_port(portdev, 0);
 	}
 
-	/*
-	 * For backward compatibility: if we're running on an older
-	 * host, we always want to create a console port.
-	 */
-	add_port(portdev, 0);
-
-	/* Start using the new console output. */
-	early_put_chars = NULL;
-
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 1);
 	return 0;
-- 
1.6.2.5

[PATCH 09/10] virtio: console: Rename wait_is_over() to will_read_block()

[Amit Shah]

We'll introduce a function that checks if write will block.  Have
function names that are similar for the two cases.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3d70729..680ea25 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -486,9 +486,9 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
 }
 
 /* The condition that must be true for polling to end */
-static bool wait_is_over(struct port *port)
+static bool will_read_block(struct port *port)
 {
-	return port_has_data(port) || !port->host_connected;
+	return !port_has_data(port) && port->host_connected;
 }
 
 static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
@@ -511,7 +511,7 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
 			return -EAGAIN;
 
 		ret = wait_event_interruptible(port->waitqueue,
-					       wait_is_over(port));
+					       !will_read_block(port));
 		if (ret < 0)
 			return ret;
 	}
-- 
1.6.2.5

[PATCH 10/10] virtio: console: Add support for nonblocking write()s

[Amit Shah]

If the host port is not open, a write() should either just return if the
file is opened in non-blocking mode, or block till the host port is
opened.

Also, don't spin till host consumes data for nonblocking ports. For
non-blocking ports, we can do away with the spinning and reclaim the
buffers consumed by the host on the next write call or on the condition
that'll make poll return.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   82 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 680ea25..d70efda 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -183,6 +183,8 @@ struct port {
 	/* The 'id' to identify the port with the Host */
 	u32 id;
 
+	bool outvq_full;
+
 	/* Is the host device open */
 	bool host_connected;
 
@@ -404,7 +406,19 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	return __send_control_msg(port->portdev, port->id, event, value);
 }
 
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
+static void reclaim_consumed_buffers(struct port *port)
+{
+	void *buf;
+	unsigned int len;
+
+	while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
+		kfree(buf);
+		port->outvq_full = false;
+	}
+}
+
+static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
+			bool nonblock)
 {
 	struct scatterlist sg[1];
 	struct virtqueue *out_vq;
@@ -413,6 +427,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
 
 	out_vq = port->out_vq;
 
+	reclaim_consumed_buffers(port);
+
 	sg_init_one(sg, in_buf, in_count);
 	ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
 
@@ -424,6 +440,12 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count)
 		goto fail;
 	}
 
+	if (ret == 0)
+		port->outvq_full = true;
+
+	if (nonblock)
+		return in_count;
+
 	/*
 	 * Wait till the host acknowledges it pushed out the data we
 	 * sent.  This is done for ports in blocking mode or for data
@@ -491,6 +513,20 @@ static bool will_read_block(struct port *port)
 	return !port_has_data(port) && port->host_connected;
 }
 
+static bool will_write_block(struct port *port)
+{
+	if (!port->host_connected)
+		return true;
+
+	/*
+	 * Check if the Host has consumed any buffers since we last
+	 * sent data (this is only applicable for nonblocking ports).
+	 */
+	reclaim_consumed_buffers(port);
+
+	return port->outvq_full;
+}
+
 static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
 			      size_t count, loff_t *offp)
 {
@@ -537,9 +573,22 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	struct port *port;
 	char *buf;
 	ssize_t ret;
+	bool nonblock;
 
 	port = filp->private_data;
 
+	nonblock = filp->f_flags & O_NONBLOCK;
+
+	if (will_write_block(port)) {
+		if (nonblock)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(port->waitqueue,
+					       !will_write_block(port));
+		if (ret < 0)
+			return ret;
+	}
+
 	count = min((size_t)(32 * 1024), count);
 
 	buf = kmalloc(count, GFP_KERNEL);
@@ -552,9 +601,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto free_buf;
 	}
 
-	ret = send_buf(port, buf, count);
+	ret = send_buf(port, buf, count, nonblock);
+
+	if (nonblock && ret > 0)
+		goto out;
+
 free_buf:
 	kfree(buf);
+out:
 	return ret;
 }
 
@@ -569,7 +623,7 @@ static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
 	ret = 0;
 	if (port->inbuf)
 		ret |= POLLIN | POLLRDNORM;
-	if (port->host_connected)
+	if (!will_write_block(port))
 		ret |= POLLOUT;
 	if (!port->host_connected)
 		ret |= POLLHUP;
@@ -592,6 +646,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	discard_port_data(port);
 
 	spin_unlock_irq(&port->inbuf_lock);
+	reclaim_consumed_buffers(port);
 
 	return 0;
 }
@@ -621,6 +676,13 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	port->guest_connected = true;
 	spin_unlock_irq(&port->inbuf_lock);
 
+	/*
+	 * There might be a chance that we missed reclaiming a few
+	 * buffers in the window of the port getting previously closed
+	 * and opening now.
+	 */
+	reclaim_consumed_buffers(port);
+
 	/* Notify host of port being opened */
 	send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
@@ -661,7 +723,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (!port)
 		return -EPIPE;
 
-	return send_buf(port, (void *)buf, count);
+	return send_buf(port, (void *)buf, count, false);
 }
 
 /*
@@ -853,6 +915,8 @@ static ssize_t debugfs_read(struct file *filp, char __user *ubuf,
 	out_offset += snprintf(buf + out_offset, out_count - out_offset,
 			       "host_connected: %d\n", port->host_connected);
 	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "outvq_full: %d\n", port->outvq_full);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
 			       "is_console: %s\n",
 			       is_console_port(port) ? "yes" : "no");
 	out_offset += snprintf(buf + out_offset, out_count - out_offset,
@@ -919,6 +983,8 @@ static int add_port(struct ports_device *portdev, u32 id)
 
 	port->host_connected = port->guest_connected = false;
 
+	port->outvq_full = false;
+
 	port->in_vq = portdev->in_vqs[port->id];
 	port->out_vq = portdev->out_vqs[port->id];
 
@@ -1036,6 +1102,8 @@ static int remove_port(struct port *port)
 	/* Remove unused data this port might have received. */
 	discard_port_data(port);
 
+	reclaim_consumed_buffers(port);
+
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = port->in_vq->vq_ops->detach_unused_buf(port->in_vq)))
 		free_buf(buf);
@@ -1107,6 +1175,12 @@ static void handle_control_message(struct ports_device *portdev,
 	case VIRTIO_CONSOLE_PORT_OPEN:
 		port->host_connected = cpkt->value;
 		wake_up_interruptible(&port->waitqueue);
+		/*
+		 * If the host port got closed and the host had any
+		 * unconsumed buffers, we'll be able to reclaim them
+		 * now.
+		 */
+		reclaim_unconsumed_buffers(port);
 		break;
 	case VIRTIO_CONSOLE_PORT_NAME:
 		/*
-- 
1.6.2.5

Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s

[Rusty Russell]

On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> If the host port is not open, a write() should either just return if the
> file is opened in non-blocking mode, or block till the host port is
> opened.
> 
> Also, don't spin till host consumes data for nonblocking ports. For
> non-blocking ports, we can do away with the spinning and reclaim the
> buffers consumed by the host on the next write call or on the condition
> that'll make poll return.

I'm only reading the patch so I might have missed it, but what's the
locking going on here?

Can we race thinking we're full or not full incorrectly?

But the basic approach of this series seems good!

Thanks,
Rusty.

Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s

[Amit Shah]

On (Tue) Apr 06 2010 [12:42:58], Rusty Russell wrote:
> On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> > If the host port is not open, a write() should either just return if the
> > file is opened in non-blocking mode, or block till the host port is
> > opened.
> > 
> > Also, don't spin till host consumes data for nonblocking ports. For
> > non-blocking ports, we can do away with the spinning and reclaim the
> > buffers consumed by the host on the next write call or on the condition
> > that'll make poll return.
> 
> I'm only reading the patch so I might have missed it, but what's the
> locking going on here?
> 
> Can we race thinking we're full or not full incorrectly?

Yes, we can. I forgot to mention that in the changelog, I'm working on
adding (and testing) the locking.

Basically I wanted to send out the series in this stage for a review --
only the locking remains to be done as of now.

> But the basic approach of this series seems good!

Thanks!

		Amit

Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s

[Amit Shah]

On (Tue) Apr 06 2010 [12:42:58], Rusty Russell wrote:
> On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> > If the host port is not open, a write() should either just return if the
> > file is opened in non-blocking mode, or block till the host port is
> > opened.
> > 
> > Also, don't spin till host consumes data for nonblocking ports. For
> > non-blocking ports, we can do away with the spinning and reclaim the
> > buffers consumed by the host on the next write call or on the condition
> > that'll make poll return.
> 
> I'm only reading the patch so I might have missed it, but what's the
> locking going on here?
> 
> Can we race thinking we're full or not full incorrectly?

Turns out just this much on top of this patch should be sufficient.


diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0c632ee..6541f76 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -158,6 +158,9 @@ struct port {
 	 */
 	spinlock_t inbuf_lock;
 
+	/* Protect the operations on the out_vq. */
+	spinlock_t outbuf_lock;
+
 	/* The IO vqs for this port */
 	struct virtqueue *in_vq, *out_vq;
 
@@ -409,12 +412,15 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 static void reclaim_consumed_buffers(struct port *port)
 {
 	void *buf;
+	unsigned long flags;
 	unsigned int len;
 
+	spin_lock_irqsave(&port->outbuf_lock, flags);
 	while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
 		kfree(buf);
 		port->outvq_full = false;
 	}
+	spin_unlock_irqrestore(&port->outbuf_lock, flags);
 }
 
 static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
@@ -423,6 +429,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	struct scatterlist sg[1];
 	struct virtqueue *out_vq;
 	ssize_t ret;
+	unsigned long flags;
 	unsigned int len;
 
 	out_vq = port->out_vq;
@@ -430,6 +437,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	reclaim_consumed_buffers(port);
 
 	sg_init_one(sg, in_buf, in_count);
+
+	spin_lock_irqsave(&port->outbuf_lock, flags);
+
 	ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
 
 	/* Tell Host to go! */
@@ -437,14 +447,14 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 
 	if (ret < 0) {
 		in_count = 0;
-		goto fail;
+		goto done;
 	}
 
 	if (ret == 0)
 		port->outvq_full = true;
 
 	if (nonblock)
-		return in_count;
+		goto done;
 
 	/*
 	 * Wait till the host acknowledges it pushed out the data we
@@ -454,7 +464,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	 */
 	while (!out_vq->vq_ops->get_buf(out_vq, &len))
 		cpu_relax();
-fail:
+done:
+	spin_unlock_irqrestore(&port->outbuf_lock, flags);
 	/*
 	 * We're expected to return the amount of data we wrote -- all
 	 * of it
@@ -1009,6 +1020,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	}
 
 	spin_lock_init(&port->inbuf_lock);
+	spin_lock_init(&port->outbuf_lock);
 	init_waitqueue_head(&port->waitqueue);
 
 	/* Fill the in_vq with buffers so the host can send us data. */

Re: [PATCH 07/10] virtio: console: Use a control message to add ports

[Rusty Russell]

On Mon, 5 Apr 2010 11:24:11 pm Amit Shah wrote:
> @@ -1431,13 +1414,20 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
>  			err = -ENOMEM;
>  			goto free_vqs;
>  		}
> +
>  	}

Minor nit: gratuitous whitespace addition.

Cheers,
Rusty.