[Qemu-devel] Extending virtio_console to support multiple ports

[Qemu-devel] Extending virtio_console to support multiple ports

[Amit Shah]

Hello all,

Here is a new iteration of the patch series that implements a
transport for guest and host communications.

The code has been updated to reuse the virtio-console device instead
of creating a new virtio-serial device.

I've tested for compatibility (old qemu & new kernel, new qemu & old
kernel, new qemu & new kernel) and it all works fine.

There are a few items on my todo list but this works well.

New since last send:
- connect/disconnect notifications on connections to ports

TODO:
- Look at converting to a tty interface instead of the current char
interface
- Migration: save the extra state that's necessary
- Convert all config writes to little endian in qemu / convert from
little endian to host endian in guest
- Address a few FIXMEs spread in the code
- Introduce a watermark to stop a rogue host process flooding guest
with data
- Make connect/disconnect work for guest

Please review.
	Amit

[Qemu-devel] [PATCH] virtio_console: Add interface for guest and host communication

[Amit Shah]

Expose multiple char devices ("ports") for simple communication
between the host userspace and guest.

Sample offline usages can be: poking around in a guest to find out
the file systems used, applications installed, etc. Online usages
can be sharing of clipboard data between the guest and the host,
sending information about logged-in users to the host, locking the
screen or session when a vnc session is closed, and so on.

The interface presented to guest userspace is of a simple char
device, so it can be used like this:

    fd = open("/dev/vcon2", O_RDWR);
    ret = read(fd, buf, 100);
    ret = write(fd, string, strlen(string));

Each port is to be assigned a unique function, for example, the
first 4 ports may be reserved for libvirt usage, the next 4 for
generic streaming data and so on. This port-function mapping
isn't finalised yet.

For requirements, use-cases and some history see

    http://www.linux-kvm.org/page/VMchannel_Requirements

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/Kconfig           |    4 +-
 drivers/char/virtio_console.c  |  871 +++++++++++++++++++++++++++++++++++-----
 include/linux/virtio_console.h |   27 ++
 3 files changed, 791 insertions(+), 111 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 6a06913..fe76627 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -678,7 +678,9 @@ config VIRTIO_CONSOLE
 	select HVC_DRIVER
 	help
 	  Virtio console for use with lguest and other hypervisors.
-
+	  Also serves as a general-purpose serial device for data transfer
+	  between the guest and host. Character devices at /dev/vconNN will
+	  be created when corresponding ports are found.
 
 config HVCS
 	tristate "IBM Hypervisor Virtual Console Server support"
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c74dacf..3548bf6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -13,6 +13,7 @@
  * Host can send more.  Buffering in the Host could alleviate this, but it is a
  * difficult problem in general. :*/
 /* Copyright (C) 2006, 2007 Rusty Russell, IBM Corporation
+ * Copyright (C) 2009, Amit Shah, Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -28,100 +29,374 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+
+#include <linux/cdev.h>
+#include <linux/device.h>
 #include <linux/err.h>
+#include <linux/fs.h>
 #include <linux/init.h>
+#include <linux/poll.h>
 #include <linux/virtio.h>
 #include <linux/virtio_console.h>
+#include <linux/workqueue.h>
 #include "hvc_console.h"
 
-/*D:340 These represent our input and output console queues, and the virtio
+/*D:340 FIXME These represent our input and output console queues, and the virtio
  * operations for them. */
-static struct virtqueue *in_vq, *out_vq;
-static struct virtio_device *vdev;
+struct virtio_console_struct {
+	struct work_struct rx_work;
+	struct work_struct tx_work;
+	struct work_struct config_work;
+
+	struct list_head port_head;
+	struct list_head unused_read_head;
+
+	struct virtio_device *vdev;
+	struct class *class;
+	struct virtqueue *in_vq, *out_vq;
+
+	struct virtio_console_config *config;
+};
+
+/* This struct holds individual buffers received for each port */
+struct virtio_console_port_buffer {
+	struct list_head next;
 
-/* This is our input buffer, and how much data is left in it. */
-static unsigned int in_len;
-static char *in, *inbuf;
+	char *buf;
+
+	size_t len; /* length of the buffer */
+	size_t offset; /* offset in the buf from which to consume data */
+};
+
+struct virtio_console_port {
+	/* Next port in the list */
+	struct list_head next;
+
+	/* Buffer management */
+	struct virtio_console_port_buffer read_buf;
+	struct list_head readbuf_head;
+	wait_queue_head_t waitqueue;
+
+	/* Each port associates with a separate char device */
+	struct cdev cdev;
+	struct device *dev;
+
+	bool host_connected;  /* Is the host device open */
+};
+
+static struct virtio_console_struct virtconsole;
+
+static int major = 60; /* from the experimental range */
 
 /* The operations for our console. */
+/* FIXME: this should go into the per-port struct */
 static struct hv_ops virtio_cons;
 
 /* The hvc device */
 static struct hvc_struct *hvc;
 
-/*D:310 The put_chars() callback is pretty straightforward.
+static struct virtio_console_port *get_port_from_id(u32 id)
+{
+	struct virtio_console_port *port;
+
+	list_for_each_entry(port, &virtconsole.port_head, next) {
+		if (MINOR(port->dev->devt) == id)
+			return port;
+	}
+	return NULL;
+}
+
+static int get_id_from_port(struct virtio_console_port *port)
+{
+	return MINOR(port->dev->devt);
+}
+
+static inline bool use_multiport(void)
+{
+	return virtconsole.vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+static ssize_t fill_readbuf(struct virtio_console_port *port,
+			    char *out_buf, size_t out_count, bool to_user)
+{
+	struct virtio_console_port_buffer *buf, *buf2;
+	ssize_t out_offset, ret;
+
+	out_offset = 0;
+	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, next) {
+		size_t copy_size;
+
+		copy_size = out_count;
+		if (copy_size > buf->len - buf->offset)
+			copy_size = buf->len - buf->offset;
+
+		if (to_user) {
+			ret = copy_to_user(out_buf + out_offset,
+					   buf->buf + buf->offset,
+					   copy_size);
+			/* FIXME: Deal with ret != 0 */
+		} else {
+			memcpy(out_buf + out_offset,
+			       buf->buf + buf->offset,
+			       copy_size);
+			ret = 0; /* Emulate copy_to_user behaviour */
+		}
+
+		/* Return the number of bytes actually copied */
+		ret = copy_size - ret;
+		buf->offset += ret;
+		out_offset += ret;
+		out_count -= ret;
+
+		if (buf->len - buf->offset == 0) {
+			list_del(&buf->next);
+			kfree(buf->buf);
+			kfree(buf);
+		}
+		if (!out_count)
+			break;
+	}
+	return out_offset;
+}
+
+static ssize_t virtconsole_read(struct file *filp, char __user *ubuf,
+			       size_t count, loff_t *offp)
+{
+	struct virtio_console_port *port;
+	ssize_t ret;
+
+	port = filp->private_data;
+
+	ret = 0;
+	if (list_empty(&port->readbuf_head)) {
+		if (filp->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(port->waitqueue,
+					       !list_empty(&port->readbuf_head));
+	}
+	if (ret < 0)
+		return ret;
+
+	return fill_readbuf(port, ubuf, count, true);
+}
+
+/* For data that exceeds PAGE_SIZE in size we should send it all in
+ * one sg to not unnecessarily split up the data. Also some (all?)
+ * vnc clients don't consume split data.
  *
- * We turn the characters into a scatter-gather list, add it to the output
- * queue and then kick the Host.  Then we sit here waiting for it to finish:
- * inefficient in theory, but in practice implementations will do it
- * immediately (lguest's Launcher does). */
-static int put_chars(u32 vtermno, const char *buf, int count)
+ * If we are to keep PAGE_SIZE sized buffers, we then have to stack
+ * multiple of those in one virtio request. virtio-ring returns to us
+ * just one pointer for all the buffers. So use this struct to
+ * allocate the bufs in so that freeing this up later is easier.
+ */
+struct vbuf {
+	char **bufs;
+	struct scatterlist *sg;
+	unsigned int nent;
+};
+
+static ssize_t send_buf(struct virtio_console_port *port,
+			const char *in_buf, size_t in_count,
+			bool from_user, bool internal)
 {
-	struct scatterlist sg[1];
-	unsigned int len;
-
-	/* This is a convenient routine to initialize a single-elem sg list */
-	sg_init_one(sg, buf, count);
-
-	/* add_buf wants a token to identify this buffer: we hand it any
-	 * non-NULL pointer, since there's only ever one buffer. */
-	if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, (void *)1) == 0) {
-		/* Tell Host to go! */
-		out_vq->vq_ops->kick(out_vq);
-		/* Chill out until it's done with the buffer. */
-		while (!out_vq->vq_ops->get_buf(out_vq, &len))
-			cpu_relax();
+	struct virtqueue *out_vq;
+	struct virtio_console_header header;
+	struct vbuf *vbuf;
+	size_t in_offset, copy_size;
+	ssize_t ret;
+	unsigned int i, header_len;
+
+	if (!in_count)
+		return 0;
+
+	out_vq = virtconsole.out_vq;
+
+	/* We can't send internal messages to a host that won't understand it */
+	if (!use_multiport() && internal) {
+		return 0;
+	}
+	header_len = 0;
+	if (use_multiport()) {
+		header.id = get_id_from_port(port);
+		header.internal_message = internal;
+		header_len = sizeof(header);
+	}
+	ret = -ENOMEM;
+	vbuf = kzalloc(sizeof(struct vbuf), GFP_KERNEL);
+	if (!vbuf)
+		return ret;
+
+	/* Max. number of buffers clubbed together in one message */
+	vbuf->nent = (in_count + header_len + PAGE_SIZE - 1) / PAGE_SIZE;
+
+	vbuf->bufs = kzalloc(sizeof(char *) * vbuf->nent, GFP_KERNEL);
+	if (!vbuf->bufs)
+		goto free_vbuf;
+
+	vbuf->sg = kzalloc(sizeof(struct scatterlist) * vbuf->nent, GFP_KERNEL);
+	if (!vbuf->sg)
+		goto free_bufs;
+	sg_init_table(vbuf->sg, vbuf->nent);
+
+	i = 0; /* vbuf->bufs[i] */
+	in_offset = 0; /* offset in the user buffer */
+	while (in_count - in_offset) {
+		copy_size = min(in_count - in_offset + header_len, PAGE_SIZE);
+		vbuf->bufs[i] = kzalloc(copy_size, GFP_KERNEL);
+		if (!vbuf->bufs[i]) {
+			ret = -ENOMEM;
+			if (!i)
+				goto free_sg;
+			goto free_buffers;
+		}
+		if (header_len) {
+			memcpy(vbuf->bufs[i], &header, header_len);
+			copy_size -= header_len;
+		}
+		if (from_user)
+			ret = copy_from_user(vbuf->bufs[i] + header_len,
+					     in_buf + in_offset, copy_size);
+		else {
+			/* Since we're not sure when the host will actually
+			 * consume the data and tell us about it, we have
+			 * to copy the data here in case the caller
+			 * frees the in_buf
+			 */
+			memcpy(vbuf->bufs[i] + header_len,
+			       in_buf + in_offset, copy_size);
+			ret = 0; /* Emulate copy_from_user */
+		}
+		in_offset += copy_size - ret;
+
+		sg_set_buf(&vbuf->sg[i], vbuf->bufs[i],
+			   copy_size - ret + header_len);
+		header_len = 0; /* Pass the header only in the first buffer */
+		i++;
+	}
+	if (out_vq->vq_ops->add_buf(out_vq, vbuf->sg, i, 0, vbuf)) {
+		ret = -EIO;
+		goto free_buffers;
 	}
+	/* Tell Host to go! */
+	out_vq->vq_ops->kick(out_vq);
+
+	/* We're expected to return the amount of data we wrote */
+	return in_offset;
+free_buffers:
+	while (--i >= 0)
+		kfree(vbuf->bufs[i]);
+free_sg:
+	kfree(vbuf->sg);
+free_bufs:
+	kfree(vbuf->bufs);
+free_vbuf:
+	kfree(vbuf);
+	return ret;
+}
+
+static ssize_t virtconsole_write(struct file *filp, const char __user *ubuf,
+				 size_t count, loff_t *offp)
+{
+	struct virtio_console_port *port;
 
-	/* We're expected to return the amount of data we wrote: all of it. */
-	return count;
+	port = filp->private_data;
+
+	return send_buf(port, ubuf, count, 1, 0);
 }
 
-/* Create a scatter-gather list representing our input buffer and put it in the
- * queue. */
-static void add_inbuf(void)
+static unsigned int virtconsole_poll(struct file *filp, poll_table *wait)
 {
-	struct scatterlist sg[1];
-	sg_init_one(sg, inbuf, PAGE_SIZE);
+	struct virtio_console_port *port;
+	unsigned int ret;
+
+	port = filp->private_data;
+	poll_wait(filp, &port->waitqueue, wait);
 
-	/* We should always be able to add one buffer to an empty queue. */
-	if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, inbuf) != 0)
-		BUG();
-	in_vq->vq_ops->kick(in_vq);
+	ret = 0;
+	if (!list_empty(&port->readbuf_head))
+		ret |= POLLIN | POLLRDNORM;
+
+	return ret;
 }
 
-/*D:350 get_chars() is the callback from the hvc_console infrastructure when
- * an interrupt is received.
+static int virtconsole_release(struct inode *inode, struct file *filp)
+{
+	struct virtio_console_control cpkt;
+
+	/* Notify host of port being closed */
+	cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+	cpkt.value = 0;
+	send_buf(filp->private_data, (char *)&cpkt, sizeof(cpkt), false, true);
+
+	return 0;
+}
+
+static int virtconsole_open(struct inode *inode, struct file *filp)
+{
+	struct cdev *cdev = inode->i_cdev;
+	struct virtio_console_port *port;
+	struct virtio_console_control cpkt;
+
+	port = container_of(cdev, struct virtio_console_port, cdev);
+	filp->private_data = port;
+
+	/* Notify host of port being opened */
+	cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+	cpkt.value = 1;
+	send_buf(filp->private_data, (char *)&cpkt, sizeof(cpkt), false, true);
+
+	return 0;
+}
+
+static const struct file_operations virtconsole_fops = {
+	.owner = THIS_MODULE,
+	.open  = virtconsole_open,
+	.read  = virtconsole_read,
+	.write = virtconsole_write,
+	.poll  = virtconsole_poll,
+	.release = virtconsole_release,
+};
+
+/*D:310 The cons_put_chars() callback is pretty straightforward.
+ *
+ * We turn the characters into a scatter-gather list, add it to the output
+ * queue and then kick the Host.  Then we sit here waiting for it to finish:
+ * inefficient in theory, but in practice implementations will do it
+ * immediately (lguest's Launcher does). */
+static int cons_put_chars(u32 vtermno, const char *buf, int count)
+{
+	struct virtio_console_port *port;
+
+	port = get_port_from_id(vtermno);
+	if (!port)
+		return 0;
+
+	return send_buf(port, buf, count, false, false);
+}
+
+/*D:350 cons_get_chars() is the callback from the hvc_console
+ * infrastructure when an interrupt is received.
  *
  * Most of the code deals with the fact that the hvc_console() infrastructure
  * only asks us for 16 bytes at a time.  We keep in_offset and in_used fields
  * for partially-filled buffers. */
-static int get_chars(u32 vtermno, char *buf, int count)
+static int cons_get_chars(u32 vtermno, char *buf, int count)
 {
-	/* If we don't have an input queue yet, we can't get input. */
-	BUG_ON(!in_vq);
+	struct virtio_console_port *port;
 
-	/* No buffer?  Try to get one. */
-	if (!in_len) {
-		in = in_vq->vq_ops->get_buf(in_vq, &in_len);
-		if (!in)
-			return 0;
-	}
+	/* If we don't have an input queue yet, we can't get input. */
+	BUG_ON(!virtconsole.in_vq);
 
-	/* You want more than we have to give?  Well, try wanting less! */
-	if (in_len < count)
-		count = in_len;
+	port = get_port_from_id(vtermno);
+	if (!port)
+		return 0;
 
-	/* Copy across to their buffer and increment offset. */
-	memcpy(buf, in, count);
-	in += count;
-	in_len -= count;
+	if (list_empty(&port->readbuf_head))
+		return 0;
 
-	/* Finished?  Re-register buffer so Host will use it again. */
-	if (in_len == 0)
-		add_inbuf();
+	return fill_readbuf(port, buf, count, false);
 
-	return count;
 }
 /*:*/
 
@@ -160,23 +435,317 @@ static void virtcons_apply_config(struct virtio_device *dev)
  * we support only one console, the hvc struct is a global var
  * We set the configuration at this point, since we now have a tty
  */
-static int notifier_add_vio(struct hvc_struct *hp, int data)
+static int cons_notifier_add_vio(struct hvc_struct *hp, int data)
 {
 	hp->irq_requested = 1;
-	virtcons_apply_config(vdev);
+	virtcons_apply_config(virtconsole.vdev);
 
 	return 0;
 }
 
-static void notifier_del_vio(struct hvc_struct *hp, int data)
+static void cons_notifier_del_vio(struct hvc_struct *hp, int data)
 {
 	hp->irq_requested = 0;
 }
 
-static void hvc_handle_input(struct virtqueue *vq)
+static void fill_queue(struct virtqueue *vq, size_t buf_size,
+		       struct list_head *unused_head)
 {
-	if (hvc_poll(hvc))
+	struct scatterlist sg[1];
+	struct virtio_console_port_buffer *buf;
+
+	while (1) {
+		/* We have to keep track of the unused buffers
+		 * so that they can be freed when the module
+		 * is being removed
+		 */
+		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+		if (!buf)
+			break;
+		buf->buf = kzalloc(buf_size, GFP_KERNEL);
+		if (!buf->buf) {
+			kfree(buf);
+			break;
+		}
+		sg_init_one(sg, buf->buf, buf_size);
+
+		if (vq->vq_ops->add_buf(vq, sg, 0, 1, buf->buf) < 0) {
+			kfree(buf->buf);
+			kfree(buf);
+			break;
+		}
+		list_add_tail(&buf->next, unused_head);
+	}
+	vq->vq_ops->kick(vq);
+}
+
+static void fill_receive_queue(void)
+{
+	fill_queue(virtconsole.in_vq, PAGE_SIZE, &virtconsole.unused_read_head);
+}
+
+static void handle_control_message(struct virtio_console_port *port,
+				   struct virtio_console_control *cpkt)
+{
+	switch (cpkt->event) {
+	case VIRTIO_CONSOLE_PORT_OPEN:
+		port->host_connected = cpkt->value;
+		break;
+	}
+}
+
+static void virtio_console_rx_work_handler(struct work_struct *work)
+{
+	struct virtio_console_port *port = NULL;
+	struct virtio_console_port_buffer *buf;
+	struct virtio_console_header header;
+	struct virtqueue *vq;
+	char *tmpbuf;
+	unsigned int tmplen, header_len;
+
+	header_len = use_multiport() ? sizeof(header) : 0;
+
+	vq = virtconsole.in_vq;
+	while ((tmpbuf = vq->vq_ops->get_buf(vq, &tmplen))) {
+		list_for_each_entry(buf, &virtconsole.unused_read_head, next) {
+			if (tmpbuf == buf->buf)
+				break;
+		}
+		BUG_ON(!buf);
+		/* The buffer is no longer unused */
+		list_del(&buf->next);
+
+		if (use_multiport()) {
+			memcpy(&header, buf->buf, header_len);
+			port = get_port_from_id(header.id);
+		} else
+			port = get_port_from_id(VIRTIO_CONSOLE_CONSOLE_PORT);
+		if (!port) {
+			/* No valid index at start of
+			 * buffer. Drop it.
+			 */
+			pr_debug("%s: invalid index in buffer, %c %d\n",
+				 __func__, buf->buf[0], buf->buf[0]);
+			kfree(buf->buf);
+			kfree(buf);
+			break;
+		}
+		if (use_multiport() && header.internal_message) {
+			handle_control_message(port,
+					       (struct virtio_console_control *)
+					       (tmpbuf + header_len));
+
+			/* FIXME: This buffer can be added to the
+			 * unused list to avoid free/alloc
+			 */
+			kfree(buf->buf);
+			kfree(buf);
+		} else {
+			buf->len = tmplen;
+			buf->offset = header_len;
+			list_add_tail(&buf->next, &port->readbuf_head);
+		}
+		wake_up_interruptible(&port->waitqueue);
+	}
+
+	if (get_id_from_port(port) == VIRTIO_CONSOLE_CONSOLE_PORT && hvc_poll(hvc))
 		hvc_kick();
+
+	/* Allocate buffers for all the ones that got used up */
+	fill_receive_queue();
+}
+
+static void virtio_console_tx_work_handler(struct work_struct *work)
+{
+	struct virtqueue *vq;
+	struct vbuf *vbuf;
+	unsigned int tmplen;
+	unsigned int i;
+
+	vq = virtconsole.out_vq;
+	while ((vbuf = vq->vq_ops->get_buf(vq, &tmplen))) {
+		for (i = 0; i < vbuf->nent; i++) {
+			kfree(vbuf->bufs[i]);
+		}
+		kfree(vbuf->bufs);
+		kfree(vbuf->sg);
+		kfree(vbuf);
+	}
+}
+
+static void rx_intr(struct virtqueue *vq)
+{
+	schedule_work(&virtconsole.rx_work);
+}
+
+static void tx_intr(struct virtqueue *vq)
+{
+	schedule_work(&virtconsole.tx_work);
+}
+
+static void config_intr(struct virtio_device *vdev)
+{
+	/* Handle port hot-add */
+	schedule_work(&virtconsole.config_work);
+
+	/* Handle console size changes */
+	virtcons_apply_config(vdev);
+}
+
+static u32 virtconsole_get_hot_add_port(struct virtio_console_config *config)
+{
+	u32 i;
+	u32 port_nr;
+
+	for (i = 0; i < virtconsole.config->max_nr_ports / 32; i++) {
+		port_nr = ffs(config->ports_map[i] ^ virtconsole.config->ports_map[i]);
+		if (port_nr)
+			break;
+	}
+	if (unlikely(!port_nr))
+		return VIRTIO_CONSOLE_BAD_ID;
+
+	/* We used ffs above */
+	port_nr--;
+
+	/* FIXME: Do this only when add_port is successful */
+	virtconsole.config->ports_map[i] |= 1U << port_nr;
+
+	port_nr += i * 32;
+	return port_nr;
+}
+
+static u32 virtconsole_find_next_port(u32 *map, int *map_i)
+{
+	u32 port_nr;
+
+	while (1) {
+		port_nr = ffs(*map);
+		if (port_nr)
+			break;
+
+		if (unlikely(*map_i >= virtconsole.config->max_nr_ports / 32))
+			return VIRTIO_CONSOLE_BAD_ID;
+		++*map_i;
+		*map = virtconsole.config->ports_map[*map_i];
+	}
+	/* We used ffs above */
+	port_nr--;
+
+	/* FIXME: Do this only when add_port is successful / reset bit
+	 * in config space if add_port was unsuccessful
+	 */
+	*map &= ~(1U << port_nr);
+
+	port_nr += *map_i * 32;
+	return port_nr;
+}
+
+static int virtconsole_add_port(u32 port_nr)
+{
+	struct virtio_console_port *port;
+	dev_t devt;
+	int ret;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	devt = MKDEV(major, port_nr);
+	cdev_init(&port->cdev, &virtconsole_fops);
+
+	ret = register_chrdev_region(devt, 1, "virtio-console");
+	if (ret < 0) {
+		pr_err("%s: error registering chrdev region, ret = %d\n",
+		       __func__, ret);
+		goto free_cdev;
+	}
+	ret = cdev_add(&port->cdev, devt, 1);
+	if (ret < 0) {
+		pr_err("%s: error adding cdev, ret = %d\n", __func__, ret);
+		goto free_cdev;
+	}
+	port->dev = device_create(virtconsole.class, NULL, devt, NULL,
+				  "vcon%u", port_nr);
+	if (IS_ERR(port->dev)) {
+		ret = PTR_ERR(port->dev);
+		pr_err("%s: Error creating device, ret = %d\n", __func__, ret);
+		goto free_cdev;
+	}
+	INIT_LIST_HEAD(&port->readbuf_head);
+	init_waitqueue_head(&port->waitqueue);
+
+	list_add_tail(&port->next, &virtconsole.port_head);
+
+	if (port_nr == VIRTIO_CONSOLE_CONSOLE_PORT) {
+		/* Start using the new console output. */
+		virtio_cons.get_chars = cons_get_chars;
+		virtio_cons.put_chars = cons_put_chars;
+		virtio_cons.notifier_add = cons_notifier_add_vio;
+		virtio_cons.notifier_del = cons_notifier_del_vio;
+		virtio_cons.notifier_hangup = cons_notifier_del_vio;
+
+		/* The first argument of hvc_alloc() is the virtual
+		 * console number, so we use zero.  The second
+		 * argument is the parameter for the notification
+		 * mechanism (like irq number). We currently leave
+		 * this as zero, virtqueues have implicit
+		 * notifications.
+		 *
+		 * The third argument is a "struct hv_ops" containing
+		 * the put_chars() get_chars(), notifier_add() and
+		 * notifier_del() pointers.  The final argument is the
+		 * output buffer size: we can do any size, so we put
+		 * PAGE_SIZE here. */
+		hvc = hvc_alloc(0, 0, &virtio_cons, PAGE_SIZE);
+		if (IS_ERR(hvc)) {
+			ret = PTR_ERR(hvc);
+			goto free_cdev;
+		}
+	}
+	pr_info("virtio-console port found at id %u\n", port_nr);
+
+	return 0;
+free_cdev:
+	unregister_chrdev(major, "virtio-console");
+	return ret;
+}
+
+static u32 get_ports_map_size(u32 max_ports)
+{
+	return sizeof(u32) * ((max_ports + 31) / 32);
+}
+
+static void virtio_console_config_work_handler(struct work_struct *work)
+{
+	struct virtio_console_config *virtconconf;
+	struct virtio_device *vdev = virtconsole.vdev;
+	u32 i, port_nr;
+	int ret;
+
+	virtconconf = kzalloc(sizeof(*virtconconf) +
+			      get_ports_map_size(virtconsole.config->max_nr_ports),
+			      GFP_KERNEL);
+	vdev->config->get(vdev,
+			  offsetof(struct virtio_console_config, nr_active_ports),
+			  &virtconconf->nr_active_ports,
+			  sizeof(virtconconf->nr_active_ports));
+	vdev->config->get(vdev,
+			  offsetof(struct virtio_console_config, ports_map),
+			  virtconconf->ports_map,
+			  get_ports_map_size(virtconsole.config->max_nr_ports));
+
+	/* Hot-add ports */
+	for (i = virtconsole.config->nr_active_ports;
+	     i < virtconconf->nr_active_ports; i++) {
+		port_nr = virtconsole_get_hot_add_port(virtconconf);
+		if (port_nr == VIRTIO_CONSOLE_BAD_ID)
+			continue;
+		ret = virtconsole_add_port(port_nr);
+		if (!ret)
+			virtconsole.config->nr_active_ports++;
+	}
+	kfree(virtconconf);
 }
 
 /*D:370 Once we're further in boot, we get probed like any other virtio device.
@@ -186,64 +755,132 @@ static void hvc_handle_input(struct virtqueue *vq)
  * never remove the console device we never need this pointer again.
  *
  * Finally we put our input buffer in the input queue, ready to receive. */
-static int __devinit virtcons_probe(struct virtio_device *dev)
+static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
-	vq_callback_t *callbacks[] = { hvc_handle_input, NULL};
+	vq_callback_t *callbacks[] = { rx_intr, tx_intr };
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
-	int err;
-
-	vdev = dev;
-
-	/* This is the scratch page we use to receive console input */
-	inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!inbuf) {
-		err = -ENOMEM;
-		goto fail;
+	u32 i, map;
+	int ret, map_i;
+	u32 max_nr_ports;
+	bool multiport;
+
+	virtconsole.vdev = vdev;
+
+	multiport = false;
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+		multiport = true;
+		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
+		vdev->config->finalize_features(vdev);
+
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_console_config,
+					   max_nr_ports),
+				  &max_nr_ports,
+				  sizeof(max_nr_ports));
+		virtconsole.config = kzalloc(sizeof(struct virtio_console_config)
+					     + get_ports_map_size(max_nr_ports),
+					     GFP_KERNEL);
+		if (!virtconsole.config)
+			return -ENOMEM;
+		virtconsole.config->max_nr_ports = max_nr_ports;
+
+		vdev->config->get(vdev, offsetof(struct virtio_console_config,
+						 nr_active_ports),
+				  &virtconsole.config->nr_active_ports,
+				  sizeof(virtconsole.config->nr_active_ports));
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_console_config,
+					   ports_map),
+				  virtconsole.config->ports_map,
+				  get_ports_map_size(max_nr_ports));
 	}
 
 	/* Find the queues. */
 	/* FIXME: This is why we want to wean off hvc: we do nothing
 	 * when input comes in. */
-	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
-	if (err)
-		goto free;
+	ret = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
+	if (ret)
+		goto fail;
 
-	in_vq = vqs[0];
-	out_vq = vqs[1];
+	virtconsole.in_vq = vqs[0];
+	virtconsole.out_vq = vqs[1];
 
-	/* Start using the new console output. */
-	virtio_cons.get_chars = get_chars;
-	virtio_cons.put_chars = put_chars;
-	virtio_cons.notifier_add = notifier_add_vio;
-	virtio_cons.notifier_del = notifier_del_vio;
-	virtio_cons.notifier_hangup = notifier_del_vio;
-
-	/* The first argument of hvc_alloc() is the virtual console number, so
-	 * we use zero.  The second argument is the parameter for the
-	 * notification mechanism (like irq number). We currently leave this
-	 * as zero, virtqueues have implicit notifications.
-	 *
-	 * The third argument is a "struct hv_ops" containing the put_chars()
-	 * get_chars(), notifier_add() and notifier_del() pointers.
-	 * The final argument is the output buffer size: we can do any size,
-	 * so we put PAGE_SIZE here. */
-	hvc = hvc_alloc(0, 0, &virtio_cons, PAGE_SIZE);
-	if (IS_ERR(hvc)) {
-		err = PTR_ERR(hvc);
-		goto free_vqs;
-	}
+	INIT_LIST_HEAD(&virtconsole.port_head);
+	INIT_LIST_HEAD(&virtconsole.unused_read_head);
+
+	if (multiport) {
+		map_i = 0;
+		map = virtconsole.config->ports_map[map_i];
+		for (i = 0; i < virtconsole.config->nr_active_ports; i++) {
+			u32 port_nr;
+
+			port_nr = virtconsole_find_next_port(&map, &map_i);
+			if (unlikely(port_nr == VIRTIO_CONSOLE_BAD_ID))
+				continue;
+			virtconsole_add_port(port_nr);
+		}
+	} else
+		virtconsole_add_port(VIRTIO_CONSOLE_CONSOLE_PORT);
+
+	INIT_WORK(&virtconsole.rx_work, &virtio_console_rx_work_handler);
+	INIT_WORK(&virtconsole.tx_work, &virtio_console_tx_work_handler);
+	INIT_WORK(&virtconsole.config_work, &virtio_console_config_work_handler);
 
-	/* Register the input buffer the first time. */
-	add_inbuf();
+	fill_receive_queue();
 	return 0;
 
-free_vqs:
-	vdev->config->del_vqs(vdev);
-free:
-	kfree(inbuf);
 fail:
-	return err;
+	return ret;
+}
+
+static void virtconsole_remove_port_data(struct virtio_console_port *port)
+{
+	struct virtio_console_port_buffer *buf, *buf2;
+
+	device_destroy(virtconsole.class, port->dev->devt);
+	unregister_chrdev_region(port->dev->devt, 1);
+	cdev_del(&port->cdev);
+
+	/* Remove the buffers in which we have unconsumed data */
+	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, next) {
+		list_del(&buf->next);
+		kfree(buf->buf);
+		kfree(buf);
+	}
+}
+
+static void virtconsole_remove(struct virtio_device *vdev)
+{
+	struct virtio_console_port *port, *port2;
+	struct virtio_console_port_buffer *buf, *buf2;
+	char *tmpbuf;
+	int len;
+
+	unregister_chrdev(major, "virtio-console");
+	class_destroy(virtconsole.class);
+
+	cancel_work_sync(&virtconsole.rx_work);
+
+	/* Free up the buffers in the 'received' queue */
+	while ((tmpbuf = virtconsole.in_vq->vq_ops->get_buf(virtconsole.in_vq,
+							    &len)))
+		kfree(tmpbuf);
+
+	vdev->config->del_vqs(vdev);
+
+	/* Free up the buffers that were unused */
+	list_for_each_entry_safe(buf, buf2, &virtconsole.unused_read_head, next) {
+		list_del(&buf->next);
+		kfree(buf->buf);
+		kfree(buf);
+	}
+	list_for_each_entry_safe(port, port2, &virtconsole.port_head, next) {
+		list_del(&port->next);
+		virtconsole_remove_port_data(port);
+		kfree(port);
+	}
+	kfree(virtconsole.config);
 }
 
 static struct virtio_device_id id_table[] = {
@@ -253,6 +890,7 @@ static struct virtio_device_id id_table[] = {
 
 static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_SIZE,
+	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
 static struct virtio_driver virtio_console = {
@@ -262,12 +900,25 @@ static struct virtio_driver virtio_console = {
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
 	.probe =	virtcons_probe,
-	.config_changed = virtcons_apply_config,
+	.config_changed = config_intr,
 };
 
 static int __init init(void)
 {
-	return register_virtio_driver(&virtio_console);
+	int ret;
+
+	virtconsole.class = class_create(THIS_MODULE, "virtio-console");
+	if (IS_ERR(virtconsole.class)) {
+		pr_err("Error creating virtio-console class\n");
+		ret = PTR_ERR(virtconsole.class);
+		return ret;
+	}
+	ret = register_virtio_driver(&virtio_console);
+	if (ret) {
+		class_destroy(virtconsole.class);
+		return ret;
+	}
+	return 0;
 }
 module_init(init);
 
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index dc16111..2197e37 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -10,14 +10,41 @@
 
 /* Feature bits */
 #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) /* Invalid port number */
+
+/* Port at which the virtio console is spawned */
+#define VIRTIO_CONSOLE_CONSOLE_PORT	0
 
 struct virtio_console_config {
 	/* colums of the screens */
 	__u16 cols;
 	/* rows of the screens */
 	__u16 rows;
+	/* max. number of ports supported for each PCI device */
+	__u32 max_nr_ports;
+	/* number of ports in use */
+	__u32 nr_active_ports;
+	/* locations of the ports in use */
+	__u32 ports_map[0 /* (max_nr_ports + 31) / 32 */];
+} __attribute__((packed));
+
+struct virtio_console_control {
+	__u16 event;
+	__u16 value;
+};
+
+/* This struct is put in each buffer that gets passed to userspace and
+ * vice-versa
+ */
+struct virtio_console_header {
+	u32  id; /* Port number */
+	bool internal_message; /* Some message between host and guest */
 } __attribute__((packed));
 
+/* Some events for internal messages */
+#define VIRTIO_CONSOLE_PORT_OPEN	0
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

[Qemu-devel] [PATCH 1/3] char: Emit 'CLOSED' events on char device close

[Amit Shah]

Notify users of the char interface whenever the file / connection is
closed.

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

diff --git a/qemu-char.c b/qemu-char.c
index be27994..c25ed1c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -570,6 +570,7 @@ static void fd_chr_read(void *opaque)
     if (size == 0) {
         /* FD has been closed. Remove it from the active list.  */
         qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
+        qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
     if (size > 0) {
@@ -602,6 +603,7 @@ static void fd_chr_close(struct CharDriverState *chr)
     }
 
     qemu_free(s);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 /* open a character device to a unix fd */
@@ -690,6 +692,7 @@ static void stdio_read(void *opaque)
     if (size == 0) {
         /* stdin has been closed. Remove it from the active list.  */
         qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
+        qemu_chr_event(chr, CHR_EVENT_CLOSED);
         return;
     }
     if (size > 0) {
@@ -943,6 +946,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
     qemu_free(s);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 static CharDriverState *qemu_chr_open_pty(void)
@@ -1264,6 +1268,7 @@ static void pp_close(CharDriverState *chr)
     ioctl(fd, PPRELEASE);
     close(fd);
     qemu_free(drv);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 static CharDriverState *qemu_chr_open_pp(const char *filename)
@@ -1390,6 +1395,8 @@ static void win_chr_close(CharDriverState *chr)
         qemu_del_polling_cb(win_chr_pipe_poll, chr);
     else
         qemu_del_polling_cb(win_chr_poll, chr);
+
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int win_chr_init(CharDriverState *chr, const char *filename)
@@ -1779,6 +1786,7 @@ static void udp_chr_close(CharDriverState *chr)
         closesocket(s->fd);
     }
     qemu_free(s);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 static CharDriverState *qemu_chr_open_udp(const char *def)
@@ -1999,6 +2007,7 @@ static void tcp_chr_read(void *opaque)
         qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
         closesocket(s->fd);
         s->fd = -1;
+        qemu_chr_event(chr, CHR_EVENT_CLOSED);
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2095,6 +2104,7 @@ static void tcp_chr_close(CharDriverState *chr)
         closesocket(s->listen_fd);
     }
     qemu_free(s);
+    qemu_chr_event(chr, CHR_EVENT_CLOSED);
 }
 
 static CharDriverState *qemu_chr_open_tcp(const char *host_str,
diff --git a/qemu-char.h b/qemu-char.h
index 77d4eda..df620bc 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -11,6 +11,7 @@
 #define CHR_EVENT_RESET   2 /* new connection established */
 #define CHR_EVENT_MUX_IN  3 /* mux-focus was set to this terminal */
 #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
+#define CHR_EVENT_CLOSED  5 /* connection closed */
 
 
 #define CHR_IOCTL_SERIAL_SET_PARAMS   1
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/3] virtio-console: rename dvq to ovq

[Amit Shah]

It isn't obvious what 'dvq' stands for. Since it's the output queue and
the corresponding input queue is called 'ivq', call this 'ovq'

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 663c8b9..92c953c 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -20,7 +20,7 @@
 typedef struct VirtIOConsole
 {
     VirtIODevice vdev;
-    VirtQueue *ivq, *dvq;
+    VirtQueue *ivq, *ovq;
     CharDriverState *chr;
 } VirtIOConsole;
 
@@ -135,7 +135,7 @@ VirtIODevice *virtio_console_init(DeviceState *dev)
     s->vdev.get_features = virtio_console_get_features;
 
     s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
-    s->dvq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
+    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
 
     s->chr = qdev_init_chardev(dev);
     qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/3] virtio-console: Add interface for generic guest-host communication

[Amit Shah]

This interface extends the virtio-console device to handle
multiple ports that present a char device from which bits can
be sent and read.

Sample uses for such a device can be obtaining info from the
guest like the file systems used, apps installed, etc. for
offline usage and logged-in users, clipboard copy-paste, etc.
for online usage.

Each port is to be assigned a unique function, for example, the
first 4 ports may be reserved for libvirt usage, the next 4 for
generic streaming data and so on. This port-function mapping
isn't finalised yet.

For requirements, use-cases and some history see

    http://www.linux-kvm.org/page/VMchannel_Requirements

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/pc.c             |   16 ++-
 hw/virtio-console.c |  423 +++++++++++++++++++++++++++++++++++++++++++++------
 hw/virtio-console.h |   47 ++++++
 monitor.c           |    7 +
 qemu-monitor.hx     |   10 ++
 qemu-options.hx     |    2 +-
 sysemu.h            |   10 +-
 vl.c                |   41 +++---
 8 files changed, 479 insertions(+), 77 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index ca681b8..96eb7ac 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1453,11 +1453,17 @@ static void pc_init1(ram_addr_t ram_size,
     }
 
     /* Add virtio console devices */
-    if (pci_enabled) {
-        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-            if (virtcon_hds[i]) {
-                pci_create_simple(pci_bus, -1, "virtio-console-pci");
-            }
+    if (pci_enabled && virtcon_nr_ports) {
+        void *dev;
+
+        dev = pci_create_simple(pci_bus, -1, "virtio-console-pci");
+        if (!dev) {
+            fprintf(stderr, "qemu: could not create virtio console pci device\n");
+            exit(1);
+        }
+
+        for (i = 0; i < virtcon_nr_ports; i++) {
+                virtio_console_new_port(dev, virtcon_idx[i]);
         }
     }
 
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 92c953c..f8f9866 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -2,9 +2,11 @@
  * Virtio Console Device
  *
  * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -12,39 +14,158 @@
  */
 
 #include "hw.h"
+#include "pci.h"
+#include "monitor.h"
 #include "qemu-char.h"
 #include "virtio.h"
 #include "virtio-console.h"
 
-
 typedef struct VirtIOConsole
 {
     VirtIODevice vdev;
+    PCIDevice *dev;
     VirtQueue *ivq, *ovq;
-    CharDriverState *chr;
+    struct VirtIOConsolePort *ports;
+    struct virtio_console_config *config;
+    uint32_t guest_features;
 } VirtIOConsole;
 
-static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
+typedef struct VirtIOConsolePort {
+    VirtIOConsole *vcon;
+    CharDriverState *hd;
+    bool guest_connected;
+} VirtIOConsolePort;
+
+static VirtIOConsole *virtio_console;
+static struct virtio_console_config virtcon_config;
+
+static VirtIOConsolePort *get_port_from_id(uint32_t id)
+{
+    if (id > MAX_VIRTIO_CONSOLE_PORTS)
+        return NULL;
+
+    return &virtio_console->ports[id];
+}
+
+static int get_id_from_port(VirtIOConsolePort *port)
 {
-    return (VirtIOConsole *)vdev;
+    uint32_t i;
+
+    for (i = 0; i < MAX_VIRTIO_CONSOLE_PORTS; i++) {
+        if (port == &virtio_console->ports[i]) {
+            return i;
+        }
+    }
+    return VIRTIO_CONSOLE_BAD_ID;
+}
+
+static bool use_multiport(void)
+{
+    return virtio_console->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+void virtio_console_monitor_command(Monitor *mon,
+                                    const char *command, const char *param)
+{
+    int ret;
+
+    if(!strncmp(command, "add_port", 8)) {
+        if (!param) {
+            monitor_printf(mon, "Error: need port id to add new port\n");
+            return;
+        }
+        ret = init_virtio_console_port(virtcon_nr_ports, param);
+        if (ret < 0) {
+            monitor_printf(mon, "Error: cannot add new port: %s\n",
+                           strerror(-ret));
+            return;
+        }
+        virtio_console_new_port(NULL, virtcon_idx[virtcon_nr_ports]);
+        virtcon_nr_ports++;
+        virtio_console->config->nr_active_ports = cpu_to_le32(virtcon_nr_ports);
+        return;
+    }
+}
+
+static size_t flush_buf(uint32_t id, VirtIOConsolePort *port,
+                        const uint8_t *buf, size_t len)
+{
+    size_t write_len = 0;
+
+    if (port->hd) {
+        write_len = qemu_chr_write(port->hd, buf, len);
+        return write_len;
+    }
+    return 0;
+}
+
+/* Guest wants to notify us of some event */
+static void handle_control_message(VirtIOConsolePort *port,
+                                   struct virtio_console_control *cpkt)
+{
+    switch(cpkt->event) {
+    case VIRTIO_CONSOLE_PORT_OPEN:
+        port->guest_connected = cpkt->value;
+        break;
+    }
 }
 
+/* Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
+ */
 static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOConsole *s = to_virtio_console(vdev);
     VirtQueueElement elem;
 
     while (virtqueue_pop(vq, &elem)) {
-        ssize_t len = 0;
-        int d;
+        VirtIOConsolePort *port;
+        uint8_t *buf;
+        size_t totlen, buf_offset;
+        struct virtio_console_header header;
+        int header_len, i;
+
+        if (use_multiport()) {
+            header_len = sizeof(header);
+
+            memcpy(&header, elem.out_sg[0].iov_base, header_len);
+            port = get_port_from_id(header.id);
+            if (!port) {
+                continue;
+            }
+        } else {
+            header_len = 0;
+            port = get_port_from_id(0);
+        }
+        totlen = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            totlen += elem.out_sg[i].iov_len;
+        }
+        buf = qemu_mallocz(totlen - header_len);
+        buf_offset = 0;
+        for (i = 0; i < elem.out_num; i++) {
+            size_t out_len = elem.out_sg[i].iov_len - header_len;
+
+            memcpy(buf + buf_offset, elem.out_sg[i].iov_base + header_len,
+                   out_len);
+
+            buf_offset += out_len;
+            /* header is only sent with the first packet in each sg */
+            header_len = 0;
+        }
+        header_len = use_multiport() ? sizeof(header) : 0;
 
-        for (d = 0; d < elem.out_num; d++) {
-            len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base,
-                                  elem.out_sg[d].iov_len);
+        if (use_multiport() && header.internal_message) {
+            handle_control_message(port, (struct virtio_console_control *)buf);
+        } else {
+            flush_buf(header.id, port, buf, totlen - header_len);
         }
-        virtqueue_push(vq, &elem, len);
-        virtio_notify(vdev, vq);
+        virtqueue_push(vq, &elem, totlen);
+        qemu_free(buf);
     }
+    virtio_notify(vdev, vq);
 }
 
 static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
@@ -53,56 +174,251 @@ static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
 
 static uint32_t virtio_console_get_features(VirtIODevice *vdev)
 {
-    return 0;
+    return 1 << VIRTIO_CONSOLE_F_MULTIPORT;
 }
 
-static int vcon_can_read(void *opaque)
+static void virtio_console_set_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
+    virtio_console->guest_features = features;
+}
 
-    if (!virtio_queue_ready(s->ivq) ||
-        !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
-        virtio_queue_empty(s->ivq))
-        return 0;
+/* Guest requested config info */
+static void virtio_console_get_config(VirtIODevice *vdev, uint8_t *config_data)
+{
+    memcpy(config_data, &virtcon_config, sizeof(struct virtio_console_config));
+}
 
-    /* current implementations have a page sized buffer.
-     * We fall back to a one byte per read if there is not enough room.
-     * It would be cool to have a function that returns the available byte
-     * instead of checking for a limit */
-    if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0))
-        return TARGET_PAGE_SIZE;
-    if (virtqueue_avail_bytes(s->ivq, 1, 0))
-        return 1;
-    return 0;
+static void virtio_console_set_config(VirtIODevice *vdev,
+                                      const uint8_t *config_data)
+{
+    struct virtio_console_config config;
+
+    memcpy(&config, config_data, sizeof(config));
 }
 
-static void vcon_read(void *opaque, const uint8_t *buf, int size)
+static void write_to_port(VirtIOConsolePort *port,
+			  const uint8_t *buf, size_t size, bool internal)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
+    VirtQueue *vq = port->vcon->ivq;
     VirtQueueElement elem;
-    int offset = 0;
+    size_t offset = 0;
+    size_t len = 0;
+
+    if (!virtio_queue_ready(vq)) {
+        return;
+    }
+
+    if (!use_multiport() && internal) {
+        return;
+    }
 
-    /* The current kernel implementation has only one outstanding input
-     * buffer of PAGE_SIZE. Nevertheless, this function is prepared to
-     * handle multiple buffers with multiple sg element for input */
     while (offset < size) {
-        int i = 0;
-        if (!virtqueue_pop(s->ivq, &elem))
-                break;
-        while (offset < size && i < elem.in_num) {
-            int len = MIN(elem.in_sg[i].iov_len, size - offset);
-            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
+        struct virtio_console_header header;
+        int i, header_len;
+
+        header_len = use_multiport() ? sizeof(header) : 0;
+
+        if (!virtqueue_pop(vq, &elem)) {
+            break;
+        }
+        if (elem.in_sg[0].iov_len < header_len) {
+            /* We can't even store our port number in this buffer. Bug? */
+            fprintf(stderr, "virtio-console: size %zd less than expected\n",
+                    elem.in_sg[0].iov_len);
+            exit(1);
+        }
+        header.id = cpu_to_le32(get_id_from_port(port));
+        header.internal_message = internal;
+        memcpy(elem.in_sg[0].iov_base, &header, header_len);
+
+        for (i = 0; offset < size && i < elem.in_num; i++) {
+            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
+
+            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
             offset += len;
-            i++;
+            header_len = 0;
         }
-        virtqueue_push(s->ivq, &elem, size);
+        header_len = use_multiport() ? sizeof(header) : 0;
+        virtqueue_push(vq, &elem, len + header_len);
+    }
+    virtio_notify(&port->vcon->vdev, vq);
+}
+
+static void send_control_event(VirtIOConsolePort *port,
+                               struct virtio_console_control *cpkt)
+{
+    write_to_port(port, (uint8_t *)cpkt, sizeof(*cpkt), true);
+}
+
+/* Readiness of the guest to accept data on a port */
+static int vcon_can_read(void *opaque)
+{
+    VirtIOConsolePort *port = (VirtIOConsolePort *) opaque;
+    VirtQueue *vq = port->vcon->ivq;
+    int size, header_len;
+
+    if (use_multiport()) {
+        header_len = sizeof(struct virtio_console_header);
+    } else {
+        header_len = 0;
+    }
+
+    if (!virtio_queue_ready(vq) ||
+        !(port->vcon->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        virtio_queue_empty(vq)) {
+        return 0;
+    }
+    if (!port->guest_connected) {
+        return 0;
     }
-    virtio_notify(&s->vdev, s->ivq);
+    size = TARGET_PAGE_SIZE;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    size = header_len + 1;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    return 0;
+}
+
+/* Send data from a char device over to the guest */
+static void vcon_read(void *opaque, const uint8_t *buf, int size)
+{
+    VirtIOConsolePort *port = (VirtIOConsolePort *) opaque;
+
+    write_to_port(port, buf, size, false);
 }
 
 static void vcon_event(void *opaque, int event)
 {
-    /* we will ignore any event for the time being */
+    VirtIOConsolePort *port = (VirtIOConsolePort *) opaque;
+    struct virtio_console_control cpkt;
+    bool update_needed;
+
+    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+
+    update_needed = false;
+    switch (event) {
+    case CHR_EVENT_RESET:
+        cpkt.value = 1;
+        update_needed = true;
+        break;
+    case CHR_EVENT_CLOSED:
+        cpkt.value = 0;
+        update_needed = true;
+        break;
+    default:
+        break;
+    }
+
+    if (!update_needed) {
+        return;
+    }
+    send_control_event(port, &cpkt);
+}
+
+static void virtio_console_set_port_active(uint32_t idx)
+{
+    int i = 0;
+
+    while (idx / 32) {
+        i++;
+        idx -= 32;
+    }
+    virtcon_config.ports_map[i] |= 1U << idx;
+}
+
+static bool virtio_console_is_port_active(uint32_t idx)
+{
+    int i = 0;
+
+    while (idx / 32) {
+        i++;
+        idx -= 32;
+    }
+    return virtcon_config.ports_map[i] & (1U << idx);
+}
+
+/* This function gets called from vl.c during initial options
+ * parsing as well as from the monitor to parse the options.
+ * So it's a good idea to not print out anything and just
+ * return values which can become meaningful.
+ */
+int init_virtio_console_port(int port, const char *opts)
+{
+    char dev[256];
+    const char *prot;
+    const char *idx;
+    uint32_t port_nr;
+    int j, k;
+
+    memset(dev, 0, sizeof(dev));
+    prot = strstr(opts, ",protocol=");
+    idx  = strstr(opts, ",port=");
+
+    port_nr = 0;
+    if (idx) {
+        port_nr = atol(idx + 6); /* skip ',port=' */
+    }
+    if (port_nr >= MAX_VIRTIO_CONSOLE_PORTS) {
+        return -ENOSPC;
+    }
+    if (virtio_console_is_port_active(port_nr)) {
+        return -EEXIST;
+    }
+
+    /* Just to maintain compatibility with other qemu options,
+     * we have the format of
+     *
+     * -virtioconsole unix:/tmp/foo,protocol=bar,port=3
+     *
+     * so to parse the 'unix:', we have to do the following
+     */
+    j = k = 0;
+    while (opts[j] && &opts[j] != prot && &opts[j] != idx) {
+        dev[k++] = opts[j++];
+    }
+
+    if (dev[0] && strncmp(dev, "none", 4)) {
+        char label[32];
+        snprintf(label, sizeof(label), "virtcon%u", port_nr);
+        virtcon_hds[port] = qemu_chr_open(label, dev, NULL);
+        if (!virtcon_hds[port]) {
+            return -EIO;
+        }
+    }
+    virtcon_idx[port] = port_nr;
+    virtio_console_set_port_active(port_nr);
+
+    return 0;
+}
+
+void *virtio_console_new_port(PCIDevice *dev, uint32_t idx)
+{
+    VirtIOConsolePort *port;
+
+    port = get_port_from_id(idx);
+    port->vcon = virtio_console;
+
+    if (!port->vcon->dev && !dev) {
+        return NULL;
+    }
+    if (!port->vcon->dev) {
+        port->vcon->dev = dev;
+    }
+    /* Hot-adding ports to existing device */
+    if (!dev) {
+        dev = port->vcon->dev;
+    }
+    port->hd = qdev_init_chardev(&dev->qdev);
+    if (port->hd) {
+        qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event,
+                              port);
+    }
+    /* Send an update to the guest about this new port added */
+    virtio_notify_config(&port->vcon->vdev);
+    return port;
 }
 
 static void virtio_console_save(QEMUFile *f, void *opaque)
@@ -128,19 +444,30 @@ VirtIODevice *virtio_console_init(DeviceState *dev)
     VirtIOConsole *s;
     s = (VirtIOConsole *)virtio_common_init("virtio-console",
                                             VIRTIO_ID_CONSOLE,
-                                            0, sizeof(VirtIOConsole));
+                                            sizeof(struct virtio_console_config),
+                                            sizeof(VirtIOConsole));
     if (s == NULL)
         return NULL;
 
+    virtio_console = s;
     s->vdev.get_features = virtio_console_get_features;
+    s->vdev.set_features = virtio_console_set_features;
+    s->vdev.get_config = virtio_console_get_config;
+    s->vdev.set_config = virtio_console_set_config;
 
+    /* Add queue for host to guest transfers */
     s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
+    /* Add queue for guest to host transfers */
     s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
 
-    s->chr = qdev_init_chardev(dev);
-    qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
+    /* Allocate space for the max. number of serial ports supported */
+    s->ports = qemu_mallocz(sizeof(VirtIOConsolePort) * MAX_VIRTIO_CONSOLE_PORTS);
+    s->config = &virtcon_config;
+
+    register_savevm("virtio-console", -1, 2, virtio_console_save, virtio_console_load, s);
 
-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
+    virtcon_config.max_nr_ports = cpu_to_le32(MAX_VIRTIO_CONSOLE_PORTS);
+    virtcon_config.nr_active_ports = cpu_to_le32(virtcon_nr_ports);
 
     return &s->vdev;
 }
diff --git a/hw/virtio-console.h b/hw/virtio-console.h
index 84d0717..1b418b3 100644
--- a/hw/virtio-console.h
+++ b/hw/virtio-console.h
@@ -2,9 +2,11 @@
  * Virtio Console Support
  *
  * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -13,7 +15,52 @@
 #ifndef _QEMU_VIRTIO_CONSOLE_H
 #define _QEMU_VIRTIO_CONSOLE_H
 
+#include "sysemu.h"
+
+/* Interface shared between the guest kernel and qemu */
+
 /* The ID for virtio console */
 #define VIRTIO_ID_CONSOLE 3
 
+/* Invalid port number */
+#define VIRTIO_CONSOLE_BAD_ID		(~(uint32_t)0)
+
+/* Port number at which the console is spawned */
+#define VIRTIO_CONSOLE_CONSOLE_PORT	0
+
+/* Features supported */
+#define VIRTIO_CONSOLE_F_MULTIPORT	1
+
+struct virtio_console_config
+{
+    /* These two fields are used by VIRTIO_CONSOLE_F_SIZE which
+     * isn't implemented here yet
+     */
+    uint16_t cols;
+    uint16_t rows;
+
+    uint32_t max_nr_ports;
+    uint32_t nr_active_ports;
+    uint32_t ports_map[(MAX_VIRTIO_CONSOLE_PORTS + 31) / 32];
+};
+
+struct virtio_console_control
+{
+    uint16_t event;
+    uint16_t value;
+};
+
+struct virtio_console_header {
+    uint32_t id; /* Port id */
+    bool     internal_message; /* Some message between host and guest */
+} __attribute__((packed));
+
+/* Some events for the control channel */
+#define VIRTIO_CONSOLE_PORT_OPEN	0
+
+
+/* In-qemu interface */
+void virtio_console_monitor_command(Monitor *mon,
+				    const char *command, const char *param);
+
 #endif
diff --git a/monitor.c b/monitor.c
index 50a3cd2..9679de7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -45,6 +45,7 @@
 #include "kvm.h"
 #include "acl.h"
 #include "exec-all.h"
+#include "hw/virtio-console.h"
 
 #include "qemu-kvm.h"
 
@@ -1824,6 +1825,12 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
     return -1;
 }
 
+static void do_virtio_console_action(Monitor *mon,
+                                     const char *command, const char *param)
+{
+    virtio_console_monitor_command(mon, command, param);
+}
+
 static const mon_cmd_t mon_cmds[] = {
 #include "qemu-monitor.h"
     { NULL, NULL, },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 0dc2ad7..3db5513 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -574,6 +574,16 @@ STEXI
 Change watchdog action.
 ETEXI
 
+    { "virtio-console", "ss?", do_virtio_console_action,
+      "<command> [<parameters>]\n",
+      "virtio-serial write port=3,key=get,value=clipboard\n"
+      "virtio-serial add_port\n"
+      "virtio-serial add_port port=6,name=foo,protocol=keyvalue\n" },
+STEXI
+@item virtio-console
+Hot-add ports or send data to virtio-console port
+ETEXI
+
     { "acl_show", "s", do_acl_show, "aclname",
       "list rules in the access control list" },
 STEXI
diff --git a/qemu-options.hx b/qemu-options.hx
index 0267496..8f83233 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1613,7 +1613,7 @@ ETEXI
 
 DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \
     "-virtioconsole c\n" \
-    "                set virtio console\n")
+    "                define virtio console\n")
 STEXI
 @item -virtioconsole @var{c}
 Set virtio console.
diff --git a/sysemu.h b/sysemu.h
index c9f8a5a..53b82cb 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -2,6 +2,7 @@
 #define SYSEMU_H
 /* Misc. things related to the system emulator.  */
 
+#include <stdbool.h>
 #include "qemu-common.h"
 #include "qemu-option.h"
 #include "sys-queue.h"
@@ -227,9 +228,14 @@ extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 
 /* virtio consoles */
 
-#define MAX_VIRTIO_CONSOLES 1
+#define MAX_VIRTIO_CONSOLE_PORTS 64
+#define VIRTIO_CONSOLE_PROTO_MAX_LEN 30
 
-extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
+extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLE_PORTS];
+extern uint32_t virtcon_idx[MAX_VIRTIO_CONSOLE_PORTS];
+extern int virtcon_nr_ports;
+extern int init_virtio_console_port(int port, const char *opts);
+extern void *virtio_console_new_port(PCIDevice *dev, uint32_t idx);
 
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
diff --git a/vl.c b/vl.c
index dc1a820..677834e 100644
--- a/vl.c
+++ b/vl.c
@@ -214,7 +214,10 @@ static int no_frame = 0;
 int no_quit = 0;
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
-CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
+CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLE_PORTS];
+char virtcon_prot[MAX_VIRTIO_CONSOLE_PORTS][VIRTIO_CONSOLE_PROTO_MAX_LEN];
+uint32_t virtcon_idx[MAX_VIRTIO_CONSOLE_PORTS];
+int virtcon_nr_ports;
 #ifdef TARGET_I386
 int win2k_install_hack = 0;
 int rtc_td_hack = 0;
@@ -4858,8 +4861,7 @@ int main(int argc, char **argv, char **envp)
     int serial_device_index;
     const char *parallel_devices[MAX_PARALLEL_PORTS];
     int parallel_device_index;
-    const char *virtio_consoles[MAX_VIRTIO_CONSOLES];
-    int virtio_console_index;
+    const char *virtio_consoles[MAX_VIRTIO_CONSOLE_PORTS];
     const char *loadvm = NULL;
     QEMUMachine *machine;
     const char *cpu_model;
@@ -4933,9 +4935,9 @@ int main(int argc, char **argv, char **envp)
         parallel_devices[i] = NULL;
     parallel_device_index = 0;
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
+    for(i = 0; i < MAX_VIRTIO_CONSOLE_PORTS; i++)
         virtio_consoles[i] = NULL;
-    virtio_console_index = 0;
+    virtcon_nr_ports = 0;
 
     for (i = 0; i < MAX_NODES; i++) {
         node_mem[i] = 0;
@@ -5379,12 +5381,12 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_virtiocon:
-                if (virtio_console_index >= MAX_VIRTIO_CONSOLES) {
+                if (virtcon_nr_ports >= MAX_VIRTIO_CONSOLE_PORTS) {
                     fprintf(stderr, "qemu: too many virtio consoles\n");
                     exit(1);
                 }
-                virtio_consoles[virtio_console_index] = optarg;
-                virtio_console_index++;
+                virtio_consoles[virtcon_nr_ports] = optarg;
+                virtcon_nr_ports++;
                 break;
             case QEMU_OPTION_parallel:
                 if (parallel_device_index >= MAX_PARALLEL_PORTS) {
@@ -6015,17 +6017,13 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (devname && strcmp(devname, "none")) {
-            char label[32];
-            snprintf(label, sizeof(label), "virtcon%d", i);
-            virtcon_hds[i] = qemu_chr_open(label, devname, NULL);
-            if (!virtcon_hds[i]) {
-                fprintf(stderr, "qemu: could not open virtio console '%s'\n",
-                        devname);
-                exit(1);
-            }
+    for (i = 0; i < virtcon_nr_ports; i++) {
+        int ret;
+
+        ret = init_virtio_console_port(i, virtio_consoles[i]);
+        if (ret < 0) {
+            fprintf(stderr, "qemu: could not init virtio console port at \"%s\"\n", virtio_consoles[i]);
+            exit(1);
         }
     }
 
@@ -6140,11 +6138,12 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
+    for(i = 0; i < MAX_VIRTIO_CONSOLE_PORTS; i++) {
         const char *devname = virtio_consoles[i];
         if (virtcon_hds[i] && devname) {
             if (strstart(devname, "vc", 0))
-                qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n", i);
+                qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n",
+                                virtcon_idx[i]);
         }
     }
 
-- 
1.6.2.5

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
> 
> Hello all,
> 
> Here is a new iteration of the patch series that implements a
> transport for guest and host communications.
> 
> The code has been updated to reuse the virtio-console device instead
> of creating a new virtio-serial device.

And the problem now is that hvc calls the put_chars function with
spinlocks held and we now allocate pages in send_buf(), called from
put_chars.

A few solutions:
- Keep things as they are, virtio_console.c remains as it is and
  virtio_serial.c gets added

- Have separate write paths for console devices in virtio_console.c,
  which would beat the purpose of merging the two drivers and then
  they'd be better off standalone

- Convert hvc's usage of spinlocks to mutexes. I've no idea how this
  will play out; I'm no expert here. But I did try doing this and so far
  it all looks OK. No lockups, lockdep warnings, nothing. I have full
  debugging enabled. But this doesn't mean it's right.

Comments?

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

[cc'ing some people who have made some commits in hvc_console.c]

On (Wed) Aug 26 2009 [16:57:18], Amit Shah wrote:
> On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
> > 
> > Hello all,
> > 
> > Here is a new iteration of the patch series that implements a
> > transport for guest and host communications.
> > 
> > The code has been updated to reuse the virtio-console device instead
> > of creating a new virtio-serial device.
> 
> And the problem now is that hvc calls the put_chars function with
> spinlocks held and we now allocate pages in send_buf(), called from
> put_chars.
> 
> A few solutions:

[snip]

> - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
>   will play out; I'm no expert here. But I did try doing this and so far
>   it all looks OK. No lockups, lockdep warnings, nothing. I have full
>   debugging enabled. But this doesn't mean it's right.

So just to test this further I added the capability to have more than
one hvc console spawn from virtio_console, created two consoles and did
a 'cat' of a file in each of the virtio-consoles. It's been running for
half an hour now without any badness. No spew in debug logs too.

I also checked the code in hvc_console.c that takes the spin_locks.
Nothing there that runs from (or needs to run from) interrupt context.
So the change to mutexes does seem reasonable. Also, the spinlock code
was added really long back -- git blame shows Linus' first git commit
introduced them in the git history, so it's pure legacy baggage.

Also found a bug: hvc_resize() wants to be called with a lock held
(hp->lock) but virtio_console just calls it directly.

Anyway I'm wondering whether all those locks are needed.

		Amit


diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index d97779e..51078a3 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -35,7 +35,7 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/sched.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/freezer.h>
 
@@ -81,7 +81,7 @@ static LIST_HEAD(hvc_structs);
  * Protect the list of hvc_struct instances from inserts and removals during
  * list traversal.
  */
-static DEFINE_SPINLOCK(hvc_structs_lock);
+static DEFINE_MUTEX(hvc_structs_lock);
 
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
@@ -98,23 +98,22 @@ static int last_hvc = -1;
 static struct hvc_struct *hvc_get_by_index(int index)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_lock);
 
 	list_for_each_entry(hp, &hvc_structs, next) {
-		spin_lock_irqsave(&hp->lock, flags);
+		mutex_lock(&hp->lock);
 		if (hp->index == index) {
 			kref_get(&hp->kref);
-			spin_unlock_irqrestore(&hp->lock, flags);
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hp->lock);
+			mutex_unlock(&hvc_structs_lock);
 			return hp;
 		}
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 	}
 	hp = NULL;
 
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 	return hp;
 }
 
@@ -228,15 +227,14 @@ console_initcall(hvc_console_init);
 static void destroy_hvc_struct(struct kref *kref)
 {
 	struct hvc_struct *hp = container_of(kref, struct hvc_struct, kref);
-	unsigned long flags;
 
-	spin_lock(&hvc_structs_lock);
+	mutex_lock(&hvc_structs_lock);
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	list_del(&(hp->next));
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 
 	kfree(hp);
 }
@@ -302,17 +300,16 @@ static void hvc_unthrottle(struct tty_struct *tty)
 static int hvc_open(struct tty_struct *tty, struct file * filp)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 	int rc = 0;
 
 	/* Auto increments kref reference if found. */
 	if (!(hp = hvc_get_by_index(tty->index)))
 		return -ENODEV;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	/* Check and then increment for fast path open. */
 	if (hp->count++ > 0) {
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		hvc_kick();
 		return 0;
 	} /* else count == 0 */
@@ -321,7 +318,7 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 
 	hp->tty = tty;
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (hp->ops->notifier_add)
 		rc = hp->ops->notifier_add(hp, hp->data);
@@ -333,9 +330,9 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 	 * tty fields and return the kref reference.
 	 */
 	if (rc) {
-		spin_lock_irqsave(&hp->lock, flags);
+		mutex_lock(&hp->lock);
 		hp->tty = NULL;
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		tty->driver_data = NULL;
 		kref_put(&hp->kref, destroy_hvc_struct);
 		printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
@@ -349,7 +346,6 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
 static void hvc_close(struct tty_struct *tty, struct file * filp)
 {
 	struct hvc_struct *hp;
-	unsigned long flags;
 
 	if (tty_hung_up_p(filp))
 		return;
@@ -363,12 +359,12 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		return;
 
 	hp = tty->driver_data;
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	if (--hp->count == 0) {
 		/* We are done with the tty pointer now. */
 		hp->tty = NULL;
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 
 		if (hp->ops->notifier_del)
 			hp->ops->notifier_del(hp, hp->data);
@@ -386,7 +382,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		if (hp->count < 0)
 			printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
 				hp->vtermno, hp->count);
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 	}
 
 	kref_put(&hp->kref, destroy_hvc_struct);
@@ -395,7 +391,6 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 static void hvc_hangup(struct tty_struct *tty)
 {
 	struct hvc_struct *hp = tty->driver_data;
-	unsigned long flags;
 	int temp_open_count;
 
 	if (!hp)
@@ -404,7 +399,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	/* cancel pending tty resize work */
 	cancel_work_sync(&hp->tty_resize);
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/*
 	 * The N_TTY line discipline has problems such that in a close vs
@@ -412,7 +407,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	 * that from happening for now.
 	 */
 	if (hp->count <= 0) {
-		spin_unlock_irqrestore(&hp->lock, flags);
+		mutex_unlock(&hp->lock);
 		return;
 	}
 
@@ -421,7 +416,7 @@ static void hvc_hangup(struct tty_struct *tty)
 	hp->n_outbuf = 0;
 	hp->tty = NULL;
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (hp->ops->notifier_hangup)
 			hp->ops->notifier_hangup(hp, hp->data);
@@ -462,7 +457,6 @@ static int hvc_push(struct hvc_struct *hp)
 static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
 	struct hvc_struct *hp = tty->driver_data;
-	unsigned long flags;
 	int rsize, written = 0;
 
 	/* This write was probably executed during a tty close. */
@@ -472,7 +466,7 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 	if (hp->count <= 0)
 		return -EIO;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/* Push pending writes */
 	if (hp->n_outbuf > 0)
@@ -488,7 +482,7 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 		written += rsize;
 		hvc_push(hp);
 	}
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	/*
 	 * Racy, but harmless, kick thread if there is still pending data.
@@ -511,7 +505,6 @@ static int hvc_write(struct tty_struct *tty, const unsigned char *buf, int count
 static void hvc_set_winsz(struct work_struct *work)
 {
 	struct hvc_struct *hp;
-	unsigned long hvc_flags;
 	struct tty_struct *tty;
 	struct winsize ws;
 
@@ -519,14 +512,14 @@ static void hvc_set_winsz(struct work_struct *work)
 	if (!hp)
 		return;
 
-	spin_lock_irqsave(&hp->lock, hvc_flags);
+	mutex_lock(&hp->lock);
 	if (!hp->tty) {
-		spin_unlock_irqrestore(&hp->lock, hvc_flags);
+		mutex_unlock(&hp->lock);
 		return;
 	}
 	ws  = hp->ws;
 	tty = tty_kref_get(hp->tty);
-	spin_unlock_irqrestore(&hp->lock, hvc_flags);
+	mutex_unlock(&hp->lock);
 
 	tty_do_resize(tty, &ws);
 	tty_kref_put(tty);
@@ -576,11 +569,10 @@ int hvc_poll(struct hvc_struct *hp)
 	struct tty_struct *tty;
 	int i, n, poll_mask = 0;
 	char buf[N_INBUF] __ALIGNED__;
-	unsigned long flags;
 	int read_total = 0;
 	int written_total = 0;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 
 	/* Push pending writes */
 	if (hp->n_outbuf > 0)
@@ -622,9 +614,9 @@ int hvc_poll(struct hvc_struct *hp)
 		if (n <= 0) {
 			/* Hangup the tty when disconnected from host */
 			if (n == -EPIPE) {
-				spin_unlock_irqrestore(&hp->lock, flags);
+				mutex_unlock(&hp->lock);
 				tty_hangup(tty);
-				spin_lock_irqsave(&hp->lock, flags);
+				mutex_lock(&hp->lock);
 			} else if ( n == -EAGAIN ) {
 				/*
 				 * Some back-ends can only ensure a certain min
@@ -665,7 +657,7 @@ int hvc_poll(struct hvc_struct *hp)
 		tty_wakeup(tty);
 	}
  bail:
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	if (read_total) {
 		/* Activity is occurring, so reset the polling backoff value to
@@ -714,11 +706,11 @@ static int khvcd(void *unused)
 		try_to_freeze();
 		wmb();
 		if (!cpus_are_in_xmon()) {
-			spin_lock(&hvc_structs_lock);
+			mutex_lock(&hvc_structs_lock);
 			list_for_each_entry(hp, &hvc_structs, next) {
 				poll_mask |= hvc_poll(hp);
 			}
-			spin_unlock(&hvc_structs_lock);
+			mutex_unlock(&hvc_structs_lock);
 		} else
 			poll_mask |= HVC_POLL_READ;
 		if (hvc_kicked)
@@ -777,8 +769,8 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int data,
 	kref_init(&hp->kref);
 
 	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
-	spin_lock_init(&hp->lock);
-	spin_lock(&hvc_structs_lock);
+	mutex_init(&hp->lock);
+	mutex_lock(&hvc_structs_lock);
 
 	/*
 	 * find index to use:
@@ -796,7 +788,7 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int data,
 	hp->index = i;
 
 	list_add_tail(&(hp->next), &hvc_structs);
-	spin_unlock(&hvc_structs_lock);
+	mutex_unlock(&hvc_structs_lock);
 
 	return hp;
 }
@@ -804,10 +796,9 @@ EXPORT_SYMBOL_GPL(hvc_alloc);
 
 int hvc_remove(struct hvc_struct *hp)
 {
-	unsigned long flags;
 	struct tty_struct *tty;
 
-	spin_lock_irqsave(&hp->lock, flags);
+	mutex_lock(&hp->lock);
 	tty = hp->tty;
 
 	if (hp->index < MAX_NR_HVC_CONSOLES)
@@ -815,7 +806,7 @@ int hvc_remove(struct hvc_struct *hp)
 
 	/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
 
-	spin_unlock_irqrestore(&hp->lock, flags);
+	mutex_unlock(&hp->lock);
 
 	/*
 	 * We 'put' the instance that was grabbed when the kref instance
diff --git a/drivers/char/hvc_console.h b/drivers/char/hvc_console.h
index 3c85d78..3c086f8 100644
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -45,7 +45,7 @@
 #define HVC_ALLOC_TTY_ADAPTERS	8
 
 struct hvc_struct {
-	spinlock_t lock;
+	struct mutex lock;
 	int index;
 	struct tty_struct *tty;
 	int count;

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Benjamin Herrenschmidt]

On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote:

> 
> > - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
> >   will play out; I'm no expert here. But I did try doing this and so far
> >   it all looks OK. No lockups, lockdep warnings, nothing. I have full
> >   debugging enabled. But this doesn't mean it's right.
> 
> So just to test this further I added the capability to have more than
> one hvc console spawn from virtio_console, created two consoles and did
> a 'cat' of a file in each of the virtio-consoles. It's been running for
> half an hour now without any badness. No spew in debug logs too.
> 
> I also checked the code in hvc_console.c that takes the spin_locks.
> Nothing there that runs from (or needs to run from) interrupt context.
> So the change to mutexes does seem reasonable. Also, the spinlock code
> was added really long back -- git blame shows Linus' first git commit
> introduced them in the git history, so it's pure legacy baggage.

Two things here:

 - First you seem to have completely missed the fact that hvc_poll() can
be called from interrupt time :-) Look at hvc_irq.c which is used by
some backends. Maybe that can be "fixed" by deferring to a work queue,
though it's nice to have the keyboard input have somewhat of a higher
priority than anything else here.

So unless that's fixed, or I missed something, that's a big NACK for
now.

 - Then, are we certain that there's no case where the tty layer will
call us with some lock held or in an atomic context ? To be honest, I've
totally lost track of the locking rules in tty land lately so it might
well be ok, but something to verify.

Cheers,
Ben.

Re: [Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Thu) Aug 27 2009 [14:07:03], Benjamin Herrenschmidt wrote:
> On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote:
> 
> > 
> > > - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
> > >   will play out; I'm no expert here. But I did try doing this and so far
> > >   it all looks OK. No lockups, lockdep warnings, nothing. I have full
> > >   debugging enabled. But this doesn't mean it's right.
> > 
> > So just to test this further I added the capability to have more than
> > one hvc console spawn from virtio_console, created two consoles and did
> > a 'cat' of a file in each of the virtio-consoles. It's been running for
> > half an hour now without any badness. No spew in debug logs too.
> > 
> > I also checked the code in hvc_console.c that takes the spin_locks.
> > Nothing there that runs from (or needs to run from) interrupt context.
> > So the change to mutexes does seem reasonable. Also, the spinlock code
> > was added really long back -- git blame shows Linus' first git commit
> > introduced them in the git history, so it's pure legacy baggage.
> 
> Two things here:
> 
>  - First you seem to have completely missed the fact that hvc_poll() can
> be called from interrupt time :-) Look at hvc_irq.c which is used by

Right! That's the obvious one.

> some backends. Maybe that can be "fixed" by deferring to a work queue,
> though it's nice to have the keyboard input have somewhat of a higher
> priority than anything else here.

Hm, to maintain the current behaviour of poll() returning some
poll_mask, the poll_mask could be made into an atomic variable with
khvcd() updating it. But to have read at a higher priority than the
other stuff, I don't quite see yet how that can be done.

> So unless that's fixed, or I missed something, that's a big NACK for
> now.
> 
>  - Then, are we certain that there's no case where the tty layer will
> call us with some lock held or in an atomic context ? To be honest, I've

The other routines are open(), close(), write(), etc., and other kernel
context (hvc_instantiate() and the khvcd thread).

> totally lost track of the locking rules in tty land lately so it might
> well be ok, but something to verify.

Yes.

Thanks for the response!

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Alan Cox]

>  - Then, are we certain that there's no case where the tty layer will
> call us with some lock held or in an atomic context ? To be honest,
> I've totally lost track of the locking rules in tty land lately so it
> might well be ok, but something to verify.

Some of the less well behaved line disciplines do this and always have
done.

Alan
 

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Benjamin Herrenschmidt]

On Thu, 2009-08-27 at 10:08 +0100, Alan Cox wrote:
> >  - Then, are we certain that there's no case where the tty layer will
> > call us with some lock held or in an atomic context ? To be honest,
> > I've totally lost track of the locking rules in tty land lately so it
> > might well be ok, but something to verify.
> 
> Some of the less well behaved line disciplines do this and always have
> done.

That was also my understanding but heh, I though that maybe you may have
fixed all of that already :-)

So at this stage, I think the reasonably thing to do is to stick to the
spinlock, but we can try to make it a bit smarter, and we can definitely
attempt to fix the case Amit pointed out where we call resize without a
lock while it seems to expect it (though we also need to be careful
about re-entrancy I believe).

Cheers,
Ben.

[Qemu-devel] [PATCH] hvc_console: provide (un)locked version for hvc_resize()

[Hendrik Brueckner]

On Thu, Aug 27, 2009 at 07:27:23PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2009-08-27 at 10:08 +0100, Alan Cox wrote:
> So at this stage, I think the reasonably thing to do is to stick to the
> spinlock, but we can try to make it a bit smarter, and we can definitely
> attempt to fix the case Amit pointed out where we call resize without a
> lock while it seems to expect it (though we also need to be careful
> about re-entrancy I believe).

I have worked on a patch for providing a locked hvc_resize() function.
Since only two back-ends (virtio_console and hvc_iucv) use the function,
I decided to update my hvc_iucv back-end through renaming the function
call as follows:

Rename the locking free hvc_resize() function to __hvc_resize() and
provide an inline function that locks the hvc_struct and calls
__hvc_resize().

The rationale for this patch is that virtio_console calls the hvc_resize()
function without locking the hvc_struct.
According to naming rules, the unlocked version is renamed and
prefixed with "__".
References to unlocked function calls in hvc back-ends has been updated.

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/char/hvc_console.c |    6 +++---
 drivers/char/hvc_console.h |   12 +++++++++++-
 drivers/char/hvc_iucv.c    |    4 +++-
 3 files changed, 17 insertions(+), 5 deletions(-)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -680,7 +680,7 @@ int hvc_poll(struct hvc_struct *hp)
 EXPORT_SYMBOL_GPL(hvc_poll);
 
 /**
- * hvc_resize() - Update terminal window size information.
+ * __hvc_resize() - Update terminal window size information.
  * @hp:		HVC console pointer
  * @ws:		Terminal window size structure
  *
@@ -689,12 +689,12 @@ EXPORT_SYMBOL_GPL(hvc_poll);
  *
  * Locking:	Locking free; the function MUST be called holding hp->lock
  */
-void hvc_resize(struct hvc_struct *hp, struct winsize ws)
+void __hvc_resize(struct hvc_struct *hp, struct winsize ws)
 {
 	hp->ws = ws;
 	schedule_work(&hp->tty_resize);
 }
-EXPORT_SYMBOL_GPL(hvc_resize);
+EXPORT_SYMBOL_GPL(__hvc_resize);
 
 /*
  * This kthread is either polling or interrupt driven.  This is determined by
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -28,6 +28,7 @@
 #define HVC_CONSOLE_H
 #include <linux/kref.h>
 #include <linux/tty.h>
+#include <linux/spinlock.h>
 
 /*
  * This is the max number of console adapters that can/will be found as
@@ -88,7 +89,16 @@ int hvc_poll(struct hvc_struct *hp);
 void hvc_kick(void);
 
 /* Resize hvc tty terminal window */
-extern void hvc_resize(struct hvc_struct *hp, struct winsize ws);
+extern void __hvc_resize(struct hvc_struct *hp, struct winsize ws);
+
+static inline void hvc_resize(struct hvc_struct *hp, struct winsize ws)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&hp->lock, flags);
+	__hvc_resize(hp, ws);
+	spin_unlock_irqrestore(&hp->lock, flags);
+}
 
 /* default notifier for irq based notification */
 extern int notifier_add_irq(struct hvc_struct *hp, int data);
--- a/drivers/char/hvc_iucv.c
+++ b/drivers/char/hvc_iucv.c
@@ -273,7 +273,9 @@ static int hvc_iucv_write(struct hvc_iuc
 	case MSG_TYPE_WINSIZE:
 		if (rb->mbuf->datalen != sizeof(struct winsize))
 			break;
-		hvc_resize(priv->hvc, *((struct winsize *) rb->mbuf->data));
+		/* The caller must ensure that the hvc is locked, which
+		 * is the case when called from hvc_iucv_get_chars() */
+		__hvc_resize(priv->hvc, *((struct winsize *) rb->mbuf->data));
 		break;
 
 	case MSG_TYPE_ERROR:	/* ignored ... */

Re: [Qemu-devel] Re: Extending virtio_console to support multiple ports

[Jamie Lokier]

Alan Cox wrote:
> >  - Then, are we certain that there's no case where the tty layer will
> > call us with some lock held or in an atomic context ? To be honest,
> > I've totally lost track of the locking rules in tty land lately so it
> > might well be ok, but something to verify.
> 
> Some of the less well behaved line disciplines do this and always have
> done.

I had a backtrace in my kernel log recently which looked like that,
while doing PPP over Bluetooth RFCOMM.  Resulted in AppArmor
complaining that it's hook was being called in irq context.

-- Jamie

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote:
> [cc'ing some people who have made some commits in hvc_console.c]
> 
> On (Wed) Aug 26 2009 [16:57:18], Amit Shah wrote:
> > On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
> > > 
> > > Hello all,
> > > 
> > > Here is a new iteration of the patch series that implements a
> > > transport for guest and host communications.
> > > 
> > > The code has been updated to reuse the virtio-console device instead
> > > of creating a new virtio-serial device.
> > 
> > And the problem now is that hvc calls the put_chars function with
> > spinlocks held and we now allocate pages in send_buf(), called from
> > put_chars.
> > 
> > A few solutions:
> 
> [snip]
> 
> > - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
> >   will play out; I'm no expert here. But I did try doing this and so far
> >   it all looks OK. No lockups, lockdep warnings, nothing. I have full
> >   debugging enabled. But this doesn't mean it's right.
> 
> So just to test this further I added the capability to have more than
> one hvc console spawn from virtio_console, created two consoles and did
> a 'cat' of a file in each of the virtio-consoles. It's been running for
> half an hour now without any badness. No spew in debug logs too.
> 
> I also checked the code in hvc_console.c that takes the spin_locks.
> Nothing there that runs from (or needs to run from) interrupt context.
> So the change to mutexes does seem reasonable. Also, the spinlock code
> was added really long back -- git blame shows Linus' first git commit
> introduced them in the git history, so it's pure legacy baggage.

I won't tell Ryan you called his code "pure legacy baggage" if you
don't ;)

http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020


cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Thu) Aug 27 2009 [15:04:45], Michael Ellerman wrote:
> On Wed, 2009-08-26 at 21:15 +0530, Amit Shah wrote:
> > [cc'ing some people who have made some commits in hvc_console.c]
> > 
> > On (Wed) Aug 26 2009 [16:57:18], Amit Shah wrote:
> > > On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
> > > > 
> > > > Hello all,
> > > > 
> > > > Here is a new iteration of the patch series that implements a
> > > > transport for guest and host communications.
> > > > 
> > > > The code has been updated to reuse the virtio-console device instead
> > > > of creating a new virtio-serial device.
> > > 
> > > And the problem now is that hvc calls the put_chars function with
> > > spinlocks held and we now allocate pages in send_buf(), called from
> > > put_chars.
> > > 
> > > A few solutions:
> > 
> > [snip]
> > 
> > > - Convert hvc's usage of spinlocks to mutexes. I've no idea how this
> > >   will play out; I'm no expert here. But I did try doing this and so far
> > >   it all looks OK. No lockups, lockdep warnings, nothing. I have full
> > >   debugging enabled. But this doesn't mean it's right.
> > 
> > So just to test this further I added the capability to have more than
> > one hvc console spawn from virtio_console, created two consoles and did
> > a 'cat' of a file in each of the virtio-consoles. It's been running for
> > half an hour now without any badness. No spew in debug logs too.
> > 
> > I also checked the code in hvc_console.c that takes the spin_locks.
> > Nothing there that runs from (or needs to run from) interrupt context.
> > So the change to mutexes does seem reasonable. Also, the spinlock code
> > was added really long back -- git blame shows Linus' first git commit
> > introduced them in the git history, so it's pure legacy baggage.
> 
> I won't tell Ryan you called his code "pure legacy baggage" if you
> don't ;)
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020

Thanks for the link!

(and this general area might be the one that doesn't get major upheavals
in 5-yr spans :-)

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Ryan Arnold]

On Thu, 2009-08-27 at 12:22 +0530, Amit Shah wrote:
> On (Thu) Aug 27 2009 [15:04:45], Michael Ellerman wrote:
>  Ryan you called his code "pure legacy baggage" if you
> > don't ;)
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=d450b4ae023fb4be175389c18f4f87677da03020
> 
> Thanks for the link!
> 
> (and this general area might be the one that doesn't get major upheavals
> in 5-yr spans :-)

Actually, quite the opposite... too many cooks in the kitchen in the
last 6 years have left this code with some historical oddities.  When I
added the interrupt context code in the first place I made sure that the
driver did NOTHING in the interrupt handler except awaken the working
thread.  Someone in the subsequent years thought it'd be a good idea to
have the interrupt handler actually do the work.

Furthermore the front and backends were separated so that new front ends
can be used.  This was sometime in the last 5 years as well.

Let's just say I'm happy to pass the torch on this one.

I would suggest that someone perform throughput tests on this driver at
some point.  At the point where I stopped maintaining it I'd gotten the
driver to the point where there was no jerking output or lost data under
heavy I/O load.  I'm not sure where the driver's at now.

Ryan S. Arnold

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
> On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
>   
>> Hello all,
>>
>> Here is a new iteration of the patch series that implements a
>> transport for guest and host communications.
>>
>> The code has been updated to reuse the virtio-console device instead
>> of creating a new virtio-serial device.
>>     
>
> And the problem now is that hvc calls the put_chars function with
> spinlocks held and we now allocate pages in send_buf(), called from
> put_chars.
>   

Don't allocate pages in send_buf.  There's a fixed number of possible 
entries on the ring.  Preallocate them up front and then you don't need 
to sleep.

> A few solutions:
> - Keep things as they are, virtio_console.c remains as it is and
>   virtio_serial.c gets added
>   
Not an option from a QEMU perspective.

Regards,

Anthony Liguori

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Fri) Aug 28 2009 [12:00:08], Anthony Liguori wrote:
> Amit Shah wrote:
>> On (Tue) Aug 25 2009 [11:47:20], Amit Shah wrote:
>>   
>>> Hello all,
>>>
>>> Here is a new iteration of the patch series that implements a
>>> transport for guest and host communications.
>>>
>>> The code has been updated to reuse the virtio-console device instead
>>> of creating a new virtio-serial device.
>>>     
>>
>> And the problem now is that hvc calls the put_chars function with
>> spinlocks held and we now allocate pages in send_buf(), called from
>> put_chars.
>>   
>
> Don't allocate pages in send_buf.  There's a fixed number of possible  
> entries on the ring.  Preallocate them up front and then you don't need  
> to sleep.

I did think about that as well, but there are problems:

- vnc clients (at least tigervnc) wants to receive the entire clipboard
  in a single flush command. So in the pre-allocated buffers scenario we
  could run short of the available buffers in some cases. So there will
  have to be a flag with each buffer that says 'there's more data
  pending for this particular write' which will have to be passed on to
  qemu and qemu will then flush it once it receives all the data

- A lock has to be introduced to fetch one unused buffer from the list
  and pass it on to the host. And this lock has to be a spinlock, just
  because writes can be called from irq context.

We're basically just turning a simple transport into more and more
complex code just to handle this special case.

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
> I did think about that as well, but there are problems:
>
> - vnc clients (at least tigervnc) wants to receive the entire clipboard
>   in a single flush command. So in the pre-allocated buffers scenario we
>   could run short of the available buffers in some cases. So there will
>   have to be a flag with each buffer that says 'there's more data
>   pending for this particular write' which will have to be passed on to
>   qemu and qemu will then flush it once it receives all the data
>   

No flags, assume it's a streaming protocol and don't assume anything 
about message sizes.  IOW, when you send clipboard data, send size and 
then the data.  QEMU consumes bytes until it reaches size.

> - A lock has to be introduced to fetch one unused buffer from the list
>   and pass it on to the host. And this lock has to be a spinlock, just
>   because writes can be called from irq context.
>   

I don't see a problem here.

Regards,

Anthony Liguori

> 		Amit
>   

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Sun) Aug 30 2009 [07:48:37], Anthony Liguori wrote:
> Amit Shah wrote:
>> I did think about that as well, but there are problems:
>>
>> - vnc clients (at least tigervnc) wants to receive the entire clipboard
>>   in a single flush command. So in the pre-allocated buffers scenario we
>>   could run short of the available buffers in some cases. So there will
>>   have to be a flag with each buffer that says 'there's more data
>>   pending for this particular write' which will have to be passed on to
>>   qemu and qemu will then flush it once it receives all the data
>>   
>
> No flags, assume it's a streaming protocol and don't assume anything  
> about message sizes.  IOW, when you send clipboard data, send size and  
> then the data.  QEMU consumes bytes until it reaches size.

Same intent but a different method: I'll have to specify that particular
data is "size" and that data after this special data is the actual data
stream.

>> - A lock has to be introduced to fetch one unused buffer from the list
>>   and pass it on to the host. And this lock has to be a spinlock, just
>>   because writes can be called from irq context.
>
> I don't see a problem here.

You mean you don't see a problem in using a spinlock vs not using one?

Userspace will typically send the entire buffer to be transmitted in one
system call. If it's large, the system call will have to be broken into
several. This results in multiple guest system calls, each one to be
handled with a spinlock held.

Compare this with the entire write handled in one system call in the
current method.

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
>> No flags, assume it's a streaming protocol and don't assume anything  
>> about message sizes.  IOW, when you send clipboard data, send size and  
>> then the data.  QEMU consumes bytes until it reaches size.
>>     
>
> Same intent but a different method: I'll have to specify that particular
> data is "size" and that data after this special data is the actual data
> stream.
>   

Sounds like every stream protocol in existence :-)

>>> - A lock has to be introduced to fetch one unused buffer from the list
>>>   and pass it on to the host. And this lock has to be a spinlock, just
>>>   because writes can be called from irq context.
>>>       
>> I don't see a problem here.
>>     
>
> You mean you don't see a problem in using a spinlock vs not using one?
>   

Right.  This isn't a fast path.

> Userspace will typically send the entire buffer to be transmitted in one
> system call. If it's large, the system call will have to be broken into
> several. This results in multiple guest system calls, each one to be
> handled with a spinlock held.
>
> Compare this with the entire write handled in one system call in the
> current method.
>   

Does it matter?  This isn't a fast path.

Regards,

Anthony Liguori

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Mon) Aug 31 2009 [08:17:21], Anthony Liguori wrote:
>>>> - A lock has to be introduced to fetch one unused buffer from the list
>>>>   and pass it on to the host. And this lock has to be a spinlock, just
>>>>   because writes can be called from irq context.
>>>>       
>>> I don't see a problem here.
>>>     
>>
>> You mean you don't see a problem in using a spinlock vs not using one?
>>   
>
> Right.  This isn't a fast path.
>
>> Userspace will typically send the entire buffer to be transmitted in one
>> system call. If it's large, the system call will have to be broken into
>> several. This results in multiple guest system calls, each one to be
>> handled with a spinlock held.
>>
>> Compare this with the entire write handled in one system call in the
>> current method.
>>   
>
> Does it matter?  This isn't a fast path.

The question isn't just about how much work happens inside the spinlock.
It's also a question about introducing spinlocks where they shouldn't
be.

I don't see why such changes have to creep into the kernel.

Can you please explain your rationale for being so rigid about merging
the two drivers?

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
> Can you please explain your rationale for being so rigid about merging
> the two drivers?
>   

Because they do the same thing.  I'm not going to constantly rehash 
this.  It's been explained multiple times.

If there are implementation issues within the Linux drivers because of 
peculiarities of hvc then hvc needs to be fixed.  It has nothing to do 
with the driver ABI which is what qemu cares about.

Regards,

Anthony Liguori

> 		Amit
>   

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Mon) Aug 31 2009 [09:21:13], Anthony Liguori wrote:
> Amit Shah wrote:
>> Can you please explain your rationale for being so rigid about merging
>> the two drivers?
>>   
>
> Because they do the same thing.  I'm not going to constantly rehash  
> this.  It's been explained multiple times.

It hardly looks like the same thing each passing day.

I've also mentioned that each minimal virtio device would start out
looking the same.

We're ending up having to compromise on the performance or functionality
or simplicity the devices just because of this restriction.

> If there are implementation issues within the Linux drivers because of  
> peculiarities of hvc then hvc needs to be fixed.  It has nothing to do  
> with the driver ABI which is what qemu cares about.

I'd welcome that effort as well. But we all know that's not going to
happen anytime soon.

Also, there's no driver ABI for virtio-serial yet. 

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
> On (Mon) Aug 31 2009 [09:21:13], Anthony Liguori wrote:
>   
>> Amit Shah wrote:
>>     
>>> Can you please explain your rationale for being so rigid about merging
>>> the two drivers?
>>>   
>>>       
>> Because they do the same thing.  I'm not going to constantly rehash  
>> this.  It's been explained multiple times.
>>     
>
> It hardly looks like the same thing each passing day.
>   

That's BS.  The very first time you posted, you received the same 
feedback from both Paul and I.  See 
http://article.gmane.org/gmane.comp.emulators.qemu/44778.  That was back 
in June.  You've consistently received the same feedback both on the ML 
and in private.

> We're ending up having to compromise on the performance or functionality
> or simplicity the devices just because of this restriction.
>   

This is _not_ a high performance device and there so far has been no 
functionality impact.  I don't understand why you keep dragging your 
feet about this.  It's very simple, if you post a functional set of 
patches for a converged virtio-console driver, we'll merge it.  If you 
keep arguing about having a separate virtio-serial driver, it's not 
going to get merged.  I don't know how to be more clear than this.

>> If there are implementation issues within the Linux drivers because of  
>> peculiarities of hvc then hvc needs to be fixed.  It has nothing to do  
>> with the driver ABI which is what qemu cares about.
>>     
>
> I'd welcome that effort as well. But we all know that's not going to
> happen anytime soon.
>   

That is not a justification to add a new device in QEMU.  If we add a 
new device everytime we encounter a less than ideal interface within a 
guest, we're going to end up having hundreds of devices.

Regards,

Anthony Liguori

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Amit Shah]

On (Mon) Aug 31 2009 [10:56:27], Anthony Liguori wrote:
> Amit Shah wrote:
>> On (Mon) Aug 31 2009 [09:21:13], Anthony Liguori wrote:
>>   
>>> Amit Shah wrote:
>>>     
>>>> Can you please explain your rationale for being so rigid about merging
>>>> the two drivers?
>>>>         
>>> Because they do the same thing.  I'm not going to constantly rehash   
>>> this.  It's been explained multiple times.
>>>     
>>
>> It hardly looks like the same thing each passing day.
>>   
>
> That's BS.  The very first time you posted, you received the same  
> feedback from both Paul and I.  See  
> http://article.gmane.org/gmane.comp.emulators.qemu/44778.  That was back  
> in June.  You've consistently received the same feedback both on the ML  
> and in private.

I'm just saying they all start looking the same.

>> We're ending up having to compromise on the performance or functionality
>> or simplicity the devices just because of this restriction.
>>   
>
> This is _not_ a high performance device and there so far has been no  
> functionality impact.  I don't understand why you keep dragging your  
> feet about this.  It's very simple, if you post a functional set of  
> patches for a converged virtio-console driver, we'll merge it.  If you  

I have already posted them and have received no feedback about the
patches since. Let me add another request here for you to review them.

> keep arguing about having a separate virtio-serial driver, it's not  
> going to get merged.  I don't know how to be more clear than this.

I'm not at all arguing for a separate virtio-serial driver. Please note
the difference in what I'm asking for: I'm just asking for a good
justification for the merging of the two since it just makes both the
drivers not simple and also introduces dependencies on code outside our
control.

>>> If there are implementation issues within the Linux drivers because 
>>> of  peculiarities of hvc then hvc needs to be fixed.  It has nothing 
>>> to do  with the driver ABI which is what qemu cares about.
>>>     
>>
>> I'd welcome that effort as well. But we all know that's not going to
>> happen anytime soon.
>>   
>
> That is not a justification to add a new device in QEMU.  If we add a  
> new device everytime we encounter a less than ideal interface within a  
> guest, we're going to end up having hundreds of devices.

I just find this argument funny.

		Amit

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Anthony Liguori]

Amit Shah wrote:
>>> We're ending up having to compromise on the performance or functionality
>>> or simplicity the devices just because of this restriction.
>>>   
>>>       
>> This is _not_ a high performance device and there so far has been no  
>> functionality impact.  I don't understand why you keep dragging your  
>> feet about this.  It's very simple, if you post a functional set of  
>> patches for a converged virtio-console driver, we'll merge it.  If you  
>>     
>
> I have already posted them and have received no feedback about the
> patches since. Let me add another request here for you to review them.
>   

But the guest drivers do not have proper locking.  Have you posted a new 
series with that fixed?

>> keep arguing about having a separate virtio-serial driver, it's not  
>> going to get merged.  I don't know how to be more clear than this.
>>     
>
> I'm not at all arguing for a separate virtio-serial driver. Please note
> the difference in what I'm asking for: I'm just asking for a good
> justification for the merging of the two since it just makes both the
> drivers not simple and also introduces dependencies on code outside our
> control.
>   

Functionally speaking, both virtio-console and virtio-serial do the same 
thing.  In fact, virtio-console is just a subset of virtio-serial.

If there are problems converging the two drivers in Linux, then I 
suggest you have two separate driver modules in Linux.  That would 
obviously be rejected for Linux though because you cannot have two 
drivers for the same device.  Why should qemu have a different policy?

  

>> That is not a justification to add a new device in QEMU.  If we add a  
>> new device everytime we encounter a less than ideal interface within a  
>> guest, we're going to end up having hundreds of devices.
>>     
>
> I just find this argument funny.
>   

I'm finding this discussion not so productive.

Regards,

Anthony Liguori

[Qemu-devel] Re: Extending virtio_console to support multiple ports

[Rusty Russell]

On Tue, 1 Sep 2009 02:07:05 am Anthony Liguori wrote:
> Amit Shah wrote:
> Functionally speaking, both virtio-console and virtio-serial do the same 
> thing.  In fact, virtio-console is just a subset of virtio-serial.
> 
> If there are problems converging the two drivers in Linux, then I 
> suggest you have two separate driver modules in Linux.  That would 
> obviously be rejected for Linux though because you cannot have two 
> drivers for the same device.  Why should qemu have a different policy?

I've been on leave, cheerfully not thinking about this.

AFAICT, a console and serial are very similar, so it makes sense to merge
them.  But a serial port doesn't have framing either; if it does, it's
something else.

And generally using virtio framing in upper layers is a mistake (one we've
made in the qemu implementations, but that's unfortunate).


Thanks,
Rusty.