[PATCH 00/28] virtio: console: Fixes, support for generic ports

[PATCH 00/28] virtio: console: Fixes, support for generic ports

[Amit Shah]

Hey Rusty,

This is a respin of my patches on top of the series you posted.

I've taken the liberty to modify your patches for style consistency
and comments where needed, keeping the authorship info intact.

I've had to update a few of your patches for functional changes, I've
changed the authorship info on those patches. The (only) major
functional change is to have a list for ports instead of an array
created at device probe time since we will have hotplug support where
we'll need to add new ports anyway.

I've incorporated some of your comments in this series. My
justifications to a few things that differ (some of this may be a
repeat from my previous mails):

* Only one write buffer: to write entire chunks of a write request, we
  have to have some buffer pool. Entire chunks are needed to be
  written in case of vnc copy/paste buffers as vnc clients only use
  the data in the first buffer otherwise. Better than spinning. Also,
  we can't sleep on the output path since we can be called from irq
  context (from hvc).
  
* One vq per port: We have to pre-declare the number of vqs needed at
  probe-time as a result of the MSI support; port hot-plug becomes
  difficult then.

I've passed each patch through an automated test suite that tests for
all the functionality here (caching, throttling, hotplug, hot-unplug,
console IO) twice: once with a qemu that supports this functionality
and once without. It all works fine.

I've also tested with -smp 2 (though looks like qemu's chardevs
rearrange some buffers in case of fast transfers -- there's a race
somewhere there; I've confirmed the kernel sends out all the data in
proper sequence and that the host virtio device queues it up for the
chardevs in proper sequence as well).

The testsuite tests for:
* the 'name' attribute and the symlinks that get created via udev
  scripts in /dev/virtio-ports
* caching
* throttling
* console - with running 'find /'
* open/close of chardevs
* read/write/poll
* sending a large file (> 100 MB) in either direction and matching
  sha1sums.

Since you pointed out that we need not add new feature bits for each
new functionality here, splitting became easier and I've now come up
with even more (but smaller) patches :-)

Please review.

Thanks,
		Amit

Amit Shah (22):
  virtio: console: We support only one device at a time
  virtio: console: don't assume a single console port.
  virtio: console: struct ports for multiple ports per device.
  virtio: console: ensure console size is updated on hvc open
  virtio: console: Introduce a workqueue for handling host->guest
    notifications
  virtio: console: Buffer data that comes in from the host
  virtio: console: Create a buffer pool for sending data to host
  virtio: console: Separate out console-specific data into a separate
    struct
  virtio: console: Separate out console init into a new function
  virtio: console: Introduce a 'header' for each buffer towards
    supporting multiport
  virtio: console: Add a new MULTIPORT feature, support for generic
    ports
  virtio: console: Associate each port with a char device
  virtio: console: Prepare for writing to / reading from userspace
    buffers
  virtio: console: Add file operations to ports for
    open/read/write/poll
  virtio: console: Ensure only one process can have a port open at a
    time
  virtio: console: Register with sysfs and create a 'name' attribute
    for ports
  virtio: console: Add throttling support to prevent flooding ports
  virtio: console: Add option to remove cached buffers on port close
  virtio: console: Handle port hot-plug
  hvc_console: Export (GPL'ed) hvc_remove
  virtio: console: Add ability to hot-unplug ports
  virtio: console: Add debugfs files for each port to expose debug info

Rusty Russell (6):
  virtio: console: comment cleanup
  virtio: console: statically initialize virtio_cons
  hvc_console: make the ops pointer const.
  virtio: console: port encapsulation
  virtio: console: use vdev->priv to avoid accessing global var.
  virtio: console: remove global var

 drivers/char/Kconfig           |    8 +
 drivers/char/hvc_beat.c        |    2 +-
 drivers/char/hvc_console.c     |    8 +-
 drivers/char/hvc_console.h     |    7 +-
 drivers/char/hvc_iseries.c     |    2 +-
 drivers/char/hvc_iucv.c        |    2 +-
 drivers/char/hvc_rtas.c        |    2 +-
 drivers/char/hvc_udbg.c        |    2 +-
 drivers/char/hvc_vio.c         |    2 +-
 drivers/char/hvc_xen.c         |    2 +-
 drivers/char/virtio_console.c  | 1500 ++++++++++++++++++++++++++++++++++++----
 include/linux/virtio_console.h |   46 ++-
 12 files changed, 1415 insertions(+), 168 deletions(-)

[PATCH 01/28] virtio: console: comment cleanup

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

Remove old lguest-style comments.

[Amit: - wingify comments acc. to kernel style
       - indent comments ]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c  |  108 ++++++++++++++++++++--------------------
 include/linux/virtio_console.h |    6 ++-
 2 files changed, 58 insertions(+), 56 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a035ae3..26e238c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1,18 +1,5 @@
-/*D:300
- * The Guest console driver
- *
- * Writing console drivers is one of the few remaining Dark Arts in Linux.
- * Fortunately for us, the path of virtual consoles has been well-trodden by
- * the PowerPC folks, who wrote "hvc_console.c" to generically support any
- * virtual console.  We use that infrastructure which only requires us to write
- * the basic put_chars and get_chars functions and call the right register
- * functions.
- :*/
-
-/*M:002 The console can be flooded: while the Guest is processing input the
- * 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) 2006, 2007, 2009 Rusty Russell, IBM Corporation
  *
  * 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
@@ -34,8 +21,6 @@
 #include <linux/virtio_console.h>
 #include "hvc_console.h"
 
-/*D:340 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;
 
@@ -49,12 +34,14 @@ static struct hv_ops virtio_cons;
 /* The hvc device */
 static struct hvc_struct *hvc;
 
-/*D:310 The put_chars() callback is pretty straightforward.
+/*
+ * The 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). */
+ * 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)
 {
 	struct scatterlist sg[1];
@@ -63,8 +50,10 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	/* 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. */
+	/*
+	 * 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);
@@ -77,8 +66,10 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	return count;
 }
 
-/* Create a scatter-gather list representing our input buffer and put it in the
- * queue. */
+/*
+ * Create a scatter-gather list representing our input buffer and put
+ * it in the queue.
+ */
 static void add_inbuf(void)
 {
 	struct scatterlist sg[1];
@@ -90,12 +81,14 @@ static void add_inbuf(void)
 	in_vq->vq_ops->kick(in_vq);
 }
 
-/*D:350 get_chars() is the callback from the hvc_console infrastructure when
- * an interrupt is received.
+/*
+ * 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. */
+ * 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)
 {
 	/* If we don't have an input queue yet, we can't get input. */
@@ -123,14 +116,16 @@ static int get_chars(u32 vtermno, char *buf, int count)
 
 	return count;
 }
-/*:*/
 
-/*D:320 Console drivers are initialized very early so boot messages can go out,
- * so we do things slightly differently from the generic virtio initialization
- * of the net and block drivers.
+/*
+ * Console drivers are initialized very early so boot messages can go
+ * out, so we do things slightly differently from the generic virtio
+ * initialization of the net and block drivers.
  *
- * At this stage, the console is output-only.  It's too early to set up a
- * virtqueue, so we let the drivers do some boutique early-output thing. */
+ * At this stage, the console is output-only.  It's too early to set
+ * up a virtqueue, so we let the drivers do some boutique early-output
+ * thing.
+ */
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 {
 	virtio_cons.put_chars = put_chars;
@@ -157,8 +152,8 @@ 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
+ * 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)
 {
@@ -179,13 +174,17 @@ static void hvc_handle_input(struct virtqueue *vq)
 		hvc_kick();
 }
 
-/*D:370 Once we're further in boot, we get probed like any other virtio device.
- * At this stage we set up the output virtqueue.
+/*
+ * Once we're further in boot, we get probed like any other virtio
+ * device.  At this stage we set up the output virtqueue.
  *
- * To set up and manage our virtual console, we call hvc_alloc().  Since we
- * never remove the console device we never need this pointer again.
+ * To set up and manage our virtual console, we call hvc_alloc().
+ * Since we never remove the console device we never need this pointer
+ * again.
  *
- * Finally we put our input buffer in the input queue, ready to receive. */
+ * Finally we put our input buffer in the input queue, ready to
+ * receive.
+ */
 static int __devinit virtcons_probe(struct virtio_device *dev)
 {
 	vq_callback_t *callbacks[] = { hvc_handle_input, NULL};
@@ -203,8 +202,6 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	}
 
 	/* 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;
@@ -219,15 +216,18 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	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 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. */
+	 * 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);
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index fe88517..9e0da40 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -3,8 +3,10 @@
 #include <linux/types.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
-/* This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
- * anyone can use the definitions to implement compatible drivers/servers. */
+/*
+ * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
+ * anyone can use the definitions to implement compatible drivers/servers.
+ */
 
 /* Feature bits */
 #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
-- 
1.6.2.5

[PATCH 02/28] virtio: console: statically initialize virtio_cons

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

That way, we can make it const as is good kernel style.  We use a separate
indirection for the early console, rather than mugging ops.put_chars.

We rename it hv_ops, too.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   60 +++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 26e238c..1d844a4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -28,12 +28,12 @@ static struct virtio_device *vdev;
 static unsigned int in_len;
 static char *in, *inbuf;
 
-/* The operations for our console. */
-static struct hv_ops virtio_cons;
-
 /* The hvc device */
 static struct hvc_struct *hvc;
 
+/* This is the very early arch-specified put chars function. */
+static int (*early_put_chars)(u32, const char *, int);
+
 /*
  * The put_chars() callback is pretty straightforward.
  *
@@ -47,6 +47,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	struct scatterlist sg[1];
 	unsigned int len;
 
+	if (unlikely(early_put_chars))
+		return early_put_chars(vtermno, buf, count);
+
 	/* This is a convenient routine to initialize a single-elem sg list */
 	sg_init_one(sg, buf, count);
 
@@ -118,21 +121,6 @@ static int get_chars(u32 vtermno, char *buf, int count)
 }
 
 /*
- * Console drivers are initialized very early so boot messages can go
- * out, so we do things slightly differently from the generic virtio
- * initialization of the net and block drivers.
- *
- * At this stage, the console is output-only.  It's too early to set
- * up a virtqueue, so we let the drivers do some boutique early-output
- * thing.
- */
-int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
-{
-	virtio_cons.put_chars = put_chars;
-	return hvc_instantiate(0, 0, &virtio_cons);
-}
-
-/*
  * virtio console configuration. This supports:
  * - console resize
  */
@@ -174,6 +162,30 @@ static void hvc_handle_input(struct virtqueue *vq)
 		hvc_kick();
 }
 
+/* The operations for the console. */
+static struct hv_ops hv_ops = {
+	.get_chars = get_chars,
+	.put_chars = put_chars,
+	.notifier_add = notifier_add_vio,
+	.notifier_del = notifier_del_vio,
+	.notifier_hangup = notifier_del_vio,
+};
+
+/*
+ * Console drivers are initialized very early so boot messages can go
+ * out, so we do things slightly differently from the generic virtio
+ * initialization of the net and block drivers.
+ *
+ * At this stage, the console is output-only.  It's too early to set
+ * up a virtqueue, so we let the drivers do some boutique early-output
+ * thing.
+ */
+int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
+{
+	early_put_chars = put_chars;
+	return hvc_instantiate(0, 0, &hv_ops);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.  At this stage we set up the output virtqueue.
@@ -209,13 +221,6 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	in_vq = vqs[0];
 	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
@@ -228,7 +233,7 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	 * 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);
+	hvc = hvc_alloc(0, 0, &hv_ops, PAGE_SIZE);
 	if (IS_ERR(hvc)) {
 		err = PTR_ERR(hvc);
 		goto free_vqs;
@@ -236,6 +241,9 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 
 	/* Register the input buffer the first time. */
 	add_inbuf();
+
+	/* Start using the new console output. */
+	early_put_chars = NULL;
 	return 0;
 
 free_vqs:
-- 
1.6.2.5

[PATCH 03/28] hvc_console: make the ops pointer const.

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

This is nicer for modern R/O protection.  And noone needs it non-const, so
constify the callers as well.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
To: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/char/hvc_beat.c       |    2 +-
 drivers/char/hvc_console.c    |    7 ++++---
 drivers/char/hvc_console.h    |    7 ++++---
 drivers/char/hvc_iseries.c    |    2 +-
 drivers/char/hvc_iucv.c       |    2 +-
 drivers/char/hvc_rtas.c       |    2 +-
 drivers/char/hvc_udbg.c       |    2 +-
 drivers/char/hvc_vio.c        |    2 +-
 drivers/char/hvc_xen.c        |    2 +-
 drivers/char/virtio_console.c |    2 +-
 10 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/char/hvc_beat.c b/drivers/char/hvc_beat.c
index 0afc8b8..6913fc3 100644
--- a/drivers/char/hvc_beat.c
+++ b/drivers/char/hvc_beat.c
@@ -84,7 +84,7 @@ static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
 	return cnt;
 }
 
-static struct hv_ops hvc_beat_get_put_ops = {
+static const struct hv_ops hvc_beat_get_put_ops = {
 	.get_chars = hvc_beat_get_chars,
 	.put_chars = hvc_beat_put_chars,
 };
diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index a632f25..299772f 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -125,7 +125,7 @@ static struct hvc_struct *hvc_get_by_index(int index)
  * console interfaces but can still be used as a tty device.  This has to be
  * static because kmalloc will not work during early console init.
  */
-static struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
+static const struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
 static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
 	{[0 ... MAX_NR_HVC_CONSOLES - 1] = -1};
 
@@ -247,7 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
  * vty adapters do NOT get an hvc_instantiate() callback since they
  * appear after early console init.
  */
-int hvc_instantiate(uint32_t vtermno, int index, struct hv_ops *ops)
+int hvc_instantiate(uint32_t vtermno, int index, const struct hv_ops *ops)
 {
 	struct hvc_struct *hp;
 
@@ -749,7 +749,8 @@ static const struct tty_operations hvc_ops = {
 };
 
 struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int data,
-					struct hv_ops *ops, int outbuf_size)
+				       const struct hv_ops *ops,
+				       int outbuf_size)
 {
 	struct hvc_struct *hp;
 	int i;
diff --git a/drivers/char/hvc_console.h b/drivers/char/hvc_console.h
index 10950ca..52ddf4d 100644
--- a/drivers/char/hvc_console.h
+++ b/drivers/char/hvc_console.h
@@ -55,7 +55,7 @@ struct hvc_struct {
 	int outbuf_size;
 	int n_outbuf;
 	uint32_t vtermno;
-	struct hv_ops *ops;
+	const struct hv_ops *ops;
 	int irq_requested;
 	int data;
 	struct winsize ws;
@@ -76,11 +76,12 @@ struct hv_ops {
 };
 
 /* Register a vterm and a slot index for use as a console (console_init) */
-extern int hvc_instantiate(uint32_t vtermno, int index, struct hv_ops *ops);
+extern int hvc_instantiate(uint32_t vtermno, int index,
+			   const struct hv_ops *ops);
 
 /* register a vterm for hvc tty operation (module_init or hotplug add) */
 extern struct hvc_struct * __devinit hvc_alloc(uint32_t vtermno, int data,
-				struct hv_ops *ops, int outbuf_size);
+				const struct hv_ops *ops, int outbuf_size);
 /* remove a vterm from hvc tty operation (module_exit or hotplug remove) */
 extern int hvc_remove(struct hvc_struct *hp);
 
diff --git a/drivers/char/hvc_iseries.c b/drivers/char/hvc_iseries.c
index 936d05b..fd02426 100644
--- a/drivers/char/hvc_iseries.c
+++ b/drivers/char/hvc_iseries.c
@@ -197,7 +197,7 @@ done:
 	return sent;
 }
 
-static struct hv_ops hvc_get_put_ops = {
+static const struct hv_ops hvc_get_put_ops = {
 	.get_chars = get_chars,
 	.put_chars = put_chars,
 	.notifier_add = notifier_add_irq,
diff --git a/drivers/char/hvc_iucv.c b/drivers/char/hvc_iucv.c
index b8a5d65..619f7d1 100644
--- a/drivers/char/hvc_iucv.c
+++ b/drivers/char/hvc_iucv.c
@@ -922,7 +922,7 @@ static int hvc_iucv_pm_restore_thaw(struct device *dev)
 
 
 /* HVC operations */
-static struct hv_ops hvc_iucv_ops = {
+static const struct hv_ops hvc_iucv_ops = {
 	.get_chars = hvc_iucv_get_chars,
 	.put_chars = hvc_iucv_put_chars,
 	.notifier_add = hvc_iucv_notifier_add,
diff --git a/drivers/char/hvc_rtas.c b/drivers/char/hvc_rtas.c
index 88590d0..61c4a61 100644
--- a/drivers/char/hvc_rtas.c
+++ b/drivers/char/hvc_rtas.c
@@ -71,7 +71,7 @@ static int hvc_rtas_read_console(uint32_t vtermno, char *buf, int count)
 	return i;
 }
 
-static struct hv_ops hvc_rtas_get_put_ops = {
+static const struct hv_ops hvc_rtas_get_put_ops = {
 	.get_chars = hvc_rtas_read_console,
 	.put_chars = hvc_rtas_write_console,
 };
diff --git a/drivers/char/hvc_udbg.c b/drivers/char/hvc_udbg.c
index bd63ba8..b0957e6 100644
--- a/drivers/char/hvc_udbg.c
+++ b/drivers/char/hvc_udbg.c
@@ -58,7 +58,7 @@ static int hvc_udbg_get(uint32_t vtermno, char *buf, int count)
 	return i;
 }
 
-static struct hv_ops hvc_udbg_ops = {
+static const struct hv_ops hvc_udbg_ops = {
 	.get_chars = hvc_udbg_get,
 	.put_chars = hvc_udbg_put,
 };
diff --git a/drivers/char/hvc_vio.c b/drivers/char/hvc_vio.c
index 10be343..27370e9 100644
--- a/drivers/char/hvc_vio.c
+++ b/drivers/char/hvc_vio.c
@@ -77,7 +77,7 @@ static int filtered_get_chars(uint32_t vtermno, char *buf, int count)
 	return got;
 }
 
-static struct hv_ops hvc_get_put_ops = {
+static const struct hv_ops hvc_get_put_ops = {
 	.get_chars = filtered_get_chars,
 	.put_chars = hvc_put_chars,
 	.notifier_add = notifier_add_irq,
diff --git a/drivers/char/hvc_xen.c b/drivers/char/hvc_xen.c
index a6ee32b..94f8c26 100644
--- a/drivers/char/hvc_xen.c
+++ b/drivers/char/hvc_xen.c
@@ -120,7 +120,7 @@ static int read_console(uint32_t vtermno, char *buf, int len)
 	return recv;
 }
 
-static struct hv_ops hvc_ops = {
+static const struct hv_ops hvc_ops = {
 	.get_chars = read_console,
 	.put_chars = write_console,
 	.notifier_add = notifier_add_irq,
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1d844a4..791be4e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -163,7 +163,7 @@ static void hvc_handle_input(struct virtqueue *vq)
 }
 
 /* The operations for the console. */
-static struct hv_ops hv_ops = {
+static const struct hv_ops hv_ops = {
 	.get_chars = get_chars,
 	.put_chars = put_chars,
 	.notifier_add = notifier_add_vio,
-- 
1.6.2.5

[PATCH 04/28] virtio: console: We support only one device at a time

[Amit Shah]

We support only one virtio_console device at a time. If multiple are
found, error out if one is already initialized.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 791be4e..bfc0abf 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -204,6 +204,11 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	struct virtqueue *vqs[2];
 	int err;
 
+	if (vdev) {
+		dev_warn(&vdev->dev,
+			 "Multiple virtio-console devices not supported yet\n");
+		return -EEXIST;
+	}
 	vdev = dev;
 
 	/* This is the scratch page we use to receive console input */
-- 
1.6.2.5

[PATCH 05/28] virtio: console: port encapsulation

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

We are heading towards a multiple-"port" system, so as part of weaning off
globals we encapsulate the information into 'struct port'.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |  103 +++++++++++++++++++++-------------------
 1 files changed, 54 insertions(+), 49 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfc0abf..99d9927 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -21,15 +21,19 @@
 #include <linux/virtio_console.h>
 #include "hvc_console.h"
 
-static struct virtqueue *in_vq, *out_vq;
-static struct virtio_device *vdev;
-
-/* This is our input buffer, and how much data is left in it. */
-static unsigned int in_len;
-static char *in, *inbuf;
+struct port {
+	struct virtqueue *in_vq, *out_vq;
+	struct virtio_device *vdev;
+	/* This is our input buffer, and how much data is left in it. */
+	char *inbuf;
+	unsigned int used_len, offset;
+
+	/* The hvc device */
+	struct hvc_struct *hvc;
+};
 
-/* The hvc device */
-static struct hvc_struct *hvc;
+/* We have one port ready to go immediately, for a console. */
+static struct port console;
 
 /* This is the very early arch-specified put chars function. */
 static int (*early_put_chars)(u32, const char *, int);
@@ -46,6 +50,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct scatterlist sg[1];
 	unsigned int len;
+	struct port *port = &console;
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -53,15 +58,11 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	/* 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) {
+	/* This shouldn't fail: if it does, we lose chars. */
+	if (port->out_vq->vq_ops->add_buf(port->out_vq, sg, 1, 0, port) >= 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))
+		port->out_vq->vq_ops->kick(port->out_vq);
+		while (!port->out_vq->vq_ops->get_buf(port->out_vq, &len))
 			cpu_relax();
 	}
 
@@ -73,15 +74,15 @@ static int put_chars(u32 vtermno, const char *buf, int count)
  * Create a scatter-gather list representing our input buffer and put
  * it in the queue.
  */
-static void add_inbuf(void)
+static void add_inbuf(struct port *port)
 {
 	struct scatterlist sg[1];
-	sg_init_one(sg, inbuf, PAGE_SIZE);
+	sg_init_one(sg, port->inbuf, PAGE_SIZE);
 
-	/* 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)
+	/* Should always be able to add one buffer to an empty queue. */
+	if (port->in_vq->vq_ops->add_buf(port->in_vq, sg, 0, 1, port) < 0)
 		BUG();
-	in_vq->vq_ops->kick(in_vq);
+	port->in_vq->vq_ops->kick(port->in_vq);
 }
 
 /*
@@ -94,28 +95,29 @@ static void add_inbuf(void)
  */
 static int get_chars(u32 vtermno, char *buf, int count)
 {
+	struct port *port = &console;
+
 	/* If we don't have an input queue yet, we can't get input. */
-	BUG_ON(!in_vq);
+	BUG_ON(!port->in_vq);
 
-	/* No buffer?  Try to get one. */
-	if (!in_len) {
-		in = in_vq->vq_ops->get_buf(in_vq, &in_len);
-		if (!in)
+	/* No more in buffer?  See if they've (re)used it. */
+	if (port->offset == port->used_len) {
+		if (!port->in_vq->vq_ops->get_buf(port->in_vq, &port->used_len))
 			return 0;
+		port->offset = 0;
 	}
 
 	/* You want more than we have to give?  Well, try wanting less! */
-	if (in_len < count)
-		count = in_len;
+	if (port->offset + count > port->used_len)
+		count = port->used_len - port->offset;
 
 	/* Copy across to their buffer and increment offset. */
-	memcpy(buf, in, count);
-	in += count;
-	in_len -= count;
+	memcpy(buf, port->inbuf + port->offset, count);
+	port->offset += count;
 
 	/* Finished?  Re-register buffer so Host will use it again. */
-	if (in_len == 0)
-		add_inbuf();
+	if (port->offset == port->used_len)
+		add_inbuf(port);
 
 	return count;
 }
@@ -135,7 +137,7 @@ static void virtcons_apply_config(struct virtio_device *dev)
 		dev->config->get(dev,
 				 offsetof(struct virtio_console_config, rows),
 				 &ws.ws_row, sizeof(u16));
-		hvc_resize(hvc, ws);
+		hvc_resize(console.hvc, ws);
 	}
 }
 
@@ -146,7 +148,7 @@ static void virtcons_apply_config(struct virtio_device *dev)
 static int notifier_add_vio(struct hvc_struct *hp, int data)
 {
 	hp->irq_requested = 1;
-	virtcons_apply_config(vdev);
+	virtcons_apply_config(console.vdev);
 
 	return 0;
 }
@@ -158,7 +160,7 @@ static void notifier_del_vio(struct hvc_struct *hp, int data)
 
 static void hvc_handle_input(struct virtqueue *vq)
 {
-	if (hvc_poll(hvc))
+	if (hvc_poll(console.hvc))
 		hvc_kick();
 }
 
@@ -197,23 +199,26 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
  * 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};
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
+	struct port *port;
 	int err;
 
-	if (vdev) {
-		dev_warn(&vdev->dev,
+	port = &console;
+	if (port->vdev) {
+		dev_warn(&port->vdev->dev,
 			 "Multiple virtio-console devices not supported yet\n");
 		return -EEXIST;
 	}
-	vdev = dev;
+	port->vdev = vdev;
 
 	/* This is the scratch page we use to receive console input */
-	inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!inbuf) {
+	port->used_len = 0;
+	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!port->inbuf) {
 		err = -ENOMEM;
 		goto fail;
 	}
@@ -223,8 +228,8 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	if (err)
 		goto free;
 
-	in_vq = vqs[0];
-	out_vq = vqs[1];
+	port->in_vq = vqs[0];
+	port->out_vq = vqs[1];
 
 	/*
 	 * The first argument of hvc_alloc() is the virtual console
@@ -238,14 +243,14 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 	 * 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, &hv_ops, PAGE_SIZE);
-	if (IS_ERR(hvc)) {
-		err = PTR_ERR(hvc);
+	port->hvc = hvc_alloc(0, 0, &hv_ops, PAGE_SIZE);
+	if (IS_ERR(port->hvc)) {
+		err = PTR_ERR(port->hvc);
 		goto free_vqs;
 	}
 
 	/* Register the input buffer the first time. */
-	add_inbuf();
+	add_inbuf(port);
 
 	/* Start using the new console output. */
 	early_put_chars = NULL;
@@ -254,7 +259,7 @@ static int __devinit virtcons_probe(struct virtio_device *dev)
 free_vqs:
 	vdev->config->del_vqs(vdev);
 free:
-	kfree(inbuf);
+	kfree(port->inbuf);
 fail:
 	return err;
 }
-- 
1.6.2.5

[PATCH 06/28] virtio: console: use vdev->priv to avoid accessing global var.

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

Part of removing our "one console" assumptions, use vdev->priv to point
to the port (currently == the global console).

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 99d9927..2bbdbf5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -128,6 +128,7 @@ static int get_chars(u32 vtermno, char *buf, int count)
  */
 static void virtcons_apply_config(struct virtio_device *dev)
 {
+	struct port *port = dev->priv;
 	struct winsize ws;
 
 	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
@@ -137,7 +138,7 @@ static void virtcons_apply_config(struct virtio_device *dev)
 		dev->config->get(dev,
 				 offsetof(struct virtio_console_config, rows),
 				 &ws.ws_row, sizeof(u16));
-		hvc_resize(console.hvc, ws);
+		hvc_resize(port->hvc, ws);
 	}
 }
 
@@ -160,7 +161,9 @@ static void notifier_del_vio(struct hvc_struct *hp, int data)
 
 static void hvc_handle_input(struct virtqueue *vq)
 {
-	if (hvc_poll(console.hvc))
+	struct port *port = vq->vdev->priv;
+
+	if (hvc_poll(port->hvc))
 		hvc_kick();
 }
 
@@ -213,7 +216,10 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 			 "Multiple virtio-console devices not supported yet\n");
 		return -EEXIST;
 	}
+
+	/* Attach this port to this virtio_device, and vice-versa. */
 	port->vdev = vdev;
+	vdev->priv = port;
 
 	/* This is the scratch page we use to receive console input */
 	port->used_len = 0;
-- 
1.6.2.5

[PATCH 07/28] virtio: console: don't assume a single console port.

[Amit Shah]

Keep a list of all ports being used as a console, and provide a lock
and a lookup function.  The hvc callbacks only give us a vterm number,
so we need to map this.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2bbdbf5..c133bf6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -17,10 +17,28 @@
  */
 #include <linux/err.h>
 #include <linux/init.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
 #include <linux/virtio.h>
 #include <linux/virtio_console.h>
 #include "hvc_console.h"
 
+/*
+ * This is a global struct for storing common data for all the devices
+ * this driver handles.
+ *
+ * Mainly, it has a linked list for all the consoles in one place so
+ * that callbacks from hvc for get_chars(), put_chars() work properly
+ * across multiple devices and multiple ports per device.
+ */
+struct ports_driver_data {
+	/* All the console devices handled by this driver */
+	struct list_head consoles;
+};
+static struct ports_driver_data pdrvdata;
+
+DEFINE_SPINLOCK(pdrvdata_lock);
+
 struct port {
 	struct virtqueue *in_vq, *out_vq;
 	struct virtio_device *vdev;
@@ -28,8 +46,15 @@ struct port {
 	char *inbuf;
 	unsigned int used_len, offset;
 
+	/* For console ports, hvc != NULL and these are valid. */
 	/* The hvc device */
 	struct hvc_struct *hvc;
+
+	/* We'll place all consoles in a list in the pdrvdata struct */
+	struct list_head list;
+
+	/* Our vterm number. */
+	u32 vtermno;
 };
 
 /* We have one port ready to go immediately, for a console. */
@@ -38,6 +63,22 @@ static struct port console;
 /* This is the very early arch-specified put chars function. */
 static int (*early_put_chars)(u32, const char *, int);
 
+static struct port *find_port_by_vtermno(u32 vtermno)
+{
+	struct port *port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pdrvdata_lock, flags);
+	list_for_each_entry(port, &pdrvdata.consoles, list) {
+		if (port->vtermno == vtermno)
+			goto out;
+	}
+	port = NULL;
+out:
+	spin_unlock_irqrestore(&pdrvdata_lock, flags);
+	return port;
+}
+
 /*
  * The put_chars() callback is pretty straightforward.
  *
@@ -49,8 +90,12 @@ static int (*early_put_chars)(u32, const char *, int);
 static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct scatterlist sg[1];
+	struct port *port;
 	unsigned int len;
-	struct port *port = &console;
+
+	port = find_port_by_vtermno(vtermno);
+	if (!port)
+		return 0;
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -95,7 +140,11 @@ static void add_inbuf(struct port *port)
  */
 static int get_chars(u32 vtermno, char *buf, int count)
 {
-	struct port *port = &console;
+	struct port *port;
+
+	port = find_port_by_vtermno(vtermno);
+	if (!port)
+		return 0;
 
 	/* If we don't have an input queue yet, we can't get input. */
 	BUG_ON(!port->in_vq);
@@ -142,14 +191,17 @@ 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
- */
+/* We set the configuration at this point, since we now have a tty */
 static int notifier_add_vio(struct hvc_struct *hp, int data)
 {
+	struct port *port;
+
+	port = find_port_by_vtermno(hp->vtermno);
+	if (!port)
+		return -EINVAL;
+
 	hp->irq_requested = 1;
-	virtcons_apply_config(console.vdev);
+	virtcons_apply_config(port->vdev);
 
 	return 0;
 }
@@ -255,6 +307,11 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 		goto free_vqs;
 	}
 
+	/* Add to vtermno list. */
+	spin_lock_irq(&pdrvdata_lock);
+	list_add(&port->list, &pdrvdata.consoles);
+	spin_unlock_irq(&pdrvdata_lock);
+
 	/* Register the input buffer the first time. */
 	add_inbuf(port);
 
@@ -291,6 +348,7 @@ static struct virtio_driver virtio_console = {
 
 static int __init init(void)
 {
+	INIT_LIST_HEAD(&pdrvdata.consoles);
 	return register_virtio_driver(&virtio_console);
 }
 module_init(init);
-- 
1.6.2.5

[PATCH 08/28] virtio: console: remove global var

[Amit Shah]

From: Rusty Russell <rusty@rustcorp.com.au>

Now we can use an allocation function to remove our global console variable.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   70 +++++++++++++++++++++++++++-------------
 1 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c133bf6..98a5249 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -32,6 +32,18 @@
  * across multiple devices and multiple ports per device.
  */
 struct ports_driver_data {
+	/*
+	 * This is used to keep track of the number of hvc consoles
+	 * spawned by this driver.  This number is given as the first
+	 * argument to hvc_alloc().  To correctly map an initial
+	 * console spawned via hvc_instantiate to the console being
+	 * hooked up via hvc_alloc, we need to pass the same vtermno.
+	 *
+	 * We also just assume the first console being initialised was
+	 * the first one that got used as the initial console.
+	 */
+	unsigned int next_vtermno;
+
 	/* All the console devices handled by this driver */
 	struct list_head consoles;
 };
@@ -57,9 +69,6 @@ struct port {
 	u32 vtermno;
 };
 
-/* We have one port ready to go immediately, for a console. */
-static struct port console;
-
 /* This is the very early arch-specified put chars function. */
 static int (*early_put_chars)(u32, const char *, int);
 
@@ -243,6 +252,31 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
+static struct port *__devinit add_port(u32 vtermno)
+{
+	struct port *port;
+
+	port = kzalloc(sizeof *port, GFP_KERNEL);
+	if (!port)
+		return NULL;
+
+	port->used_len = port->offset = 0;
+	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!port->inbuf) {
+		kfree(port);
+		return NULL;
+	}
+	port->hvc = NULL;
+	port->vtermno = vtermno;
+	return port;
+}
+
+static void free_port(struct port *port)
+{
+	kfree(port->inbuf);
+	kfree(port);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.  At this stage we set up the output virtqueue.
@@ -262,25 +296,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	struct port *port;
 	int err;
 
-	port = &console;
-	if (port->vdev) {
-		dev_warn(&port->vdev->dev,
-			 "Multiple virtio-console devices not supported yet\n");
-		return -EEXIST;
-	}
+	err = -ENOMEM;
+	port = add_port(pdrvdata.next_vtermno);
+	if (!port)
+		goto fail;
 
 	/* Attach this port to this virtio_device, and vice-versa. */
 	port->vdev = vdev;
 	vdev->priv = port;
 
-	/* This is the scratch page we use to receive console input */
-	port->used_len = 0;
-	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!port->inbuf) {
-		err = -ENOMEM;
-		goto fail;
-	}
-
 	/* Find the queues. */
 	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
 	if (err)
@@ -291,17 +315,16 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	/*
 	 * 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.
+	 * number.  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.
 	 */
-	port->hvc = hvc_alloc(0, 0, &hv_ops, PAGE_SIZE);
+	port->hvc = hvc_alloc(port->vtermno, 0, &hv_ops, PAGE_SIZE);
 	if (IS_ERR(port->hvc)) {
 		err = PTR_ERR(port->hvc);
 		goto free_vqs;
@@ -309,6 +332,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	/* Add to vtermno list. */
 	spin_lock_irq(&pdrvdata_lock);
+	pdrvdata.next_vtermno++;
 	list_add(&port->list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 
@@ -322,7 +346,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 free_vqs:
 	vdev->config->del_vqs(vdev);
 free:
-	kfree(port->inbuf);
+	free_port(port);
 fail:
 	return err;
 }
-- 
1.6.2.5

[PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

Rather than assume a single port, add a 'struct ports_device' which
stores data related to all the ports for that device.

Currently, there's only one port and is hooked up with hvc, but that
will change.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 98a5249..d4e2327 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -51,9 +51,20 @@ static struct ports_driver_data pdrvdata;
 
 DEFINE_SPINLOCK(pdrvdata_lock);
 
-struct port {
+/*
+ * This is a per-device struct that stores data common to all the
+ * ports for that device (vdev->priv).
+ */
+struct ports_device {
 	struct virtqueue *in_vq, *out_vq;
 	struct virtio_device *vdev;
+};
+
+/* This struct holds the per-port data */
+struct port {
+	/* Pointer to the parent virtio_console device */
+	struct ports_device *portdev;
+
 	/* This is our input buffer, and how much data is left in it. */
 	char *inbuf;
 	unsigned int used_len, offset;
@@ -100,6 +111,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct scatterlist sg[1];
 	struct port *port;
+	struct virtqueue *out_vq;
 	unsigned int len;
 
 	port = find_port_by_vtermno(vtermno);
@@ -109,14 +121,15 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
 
+	out_vq = port->portdev->out_vq;
 	/* This is a convenient routine to initialize a single-elem sg list */
 	sg_init_one(sg, buf, count);
 
 	/* This shouldn't fail: if it does, we lose chars. */
-	if (port->out_vq->vq_ops->add_buf(port->out_vq, sg, 1, 0, port) >= 0) {
+	if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, port) >= 0) {
 		/* Tell Host to go! */
-		port->out_vq->vq_ops->kick(port->out_vq);
-		while (!port->out_vq->vq_ops->get_buf(port->out_vq, &len))
+		out_vq->vq_ops->kick(out_vq);
+		while (!out_vq->vq_ops->get_buf(out_vq, &len))
 			cpu_relax();
 	}
 
@@ -130,13 +143,16 @@ static int put_chars(u32 vtermno, const char *buf, int count)
  */
 static void add_inbuf(struct port *port)
 {
+	struct virtqueue *in_vq;
 	struct scatterlist sg[1];
+
 	sg_init_one(sg, port->inbuf, PAGE_SIZE);
 
+	in_vq = port->portdev->in_vq;
 	/* Should always be able to add one buffer to an empty queue. */
-	if (port->in_vq->vq_ops->add_buf(port->in_vq, sg, 0, 1, port) < 0)
+	if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, port) < 0)
 		BUG();
-	port->in_vq->vq_ops->kick(port->in_vq);
+	in_vq->vq_ops->kick(in_vq);
 }
 
 /*
@@ -150,18 +166,23 @@ static void add_inbuf(struct port *port)
 static int get_chars(u32 vtermno, char *buf, int count)
 {
 	struct port *port;
+	struct virtqueue *in_vq;
 
 	port = find_port_by_vtermno(vtermno);
 	if (!port)
 		return 0;
 
+	in_vq = port->portdev->in_vq;
 	/* If we don't have an input queue yet, we can't get input. */
-	BUG_ON(!port->in_vq);
+	BUG_ON(!in_vq);
 
 	/* No more in buffer?  See if they've (re)used it. */
 	if (port->offset == port->used_len) {
-		if (!port->in_vq->vq_ops->get_buf(port->in_vq, &port->used_len))
+		unsigned int len;
+
+		if (!in_vq->vq_ops->get_buf(in_vq, &len))
 			return 0;
+		port->used_len = len;
 		port->offset = 0;
 	}
 
@@ -186,7 +207,6 @@ static int get_chars(u32 vtermno, char *buf, int count)
  */
 static void virtcons_apply_config(struct virtio_device *dev)
 {
-	struct port *port = dev->priv;
 	struct winsize ws;
 
 	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
@@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device *dev)
 		dev->config->get(dev,
 				 offsetof(struct virtio_console_config, rows),
 				 &ws.ws_row, sizeof(u16));
-		hvc_resize(port->hvc, ws);
+		/* This is the pre-multiport style: we use control messages
+		 * these days which specify the port.  So this means port 0. */
+		hvc_resize(find_port_by_vtermno(0)->hvc, ws);
 	}
 }
 
@@ -210,7 +232,7 @@ static int notifier_add_vio(struct hvc_struct *hp, int data)
 		return -EINVAL;
 
 	hp->irq_requested = 1;
-	virtcons_apply_config(port->vdev);
+	virtcons_apply_config(port->portdev->vdev);
 
 	return 0;
 }
@@ -222,9 +244,13 @@ static void notifier_del_vio(struct hvc_struct *hp, int data)
 
 static void hvc_handle_input(struct virtqueue *vq)
 {
-	struct port *port = vq->vdev->priv;
+	struct port *port;
+	bool activity = false;
 
-	if (hvc_poll(port->hvc))
+	list_for_each_entry(port, &pdrvdata.consoles, list)
+		activity |= hvc_poll(port->hvc);
+
+	if (activity)
 		hvc_kick();
 }
 
@@ -252,67 +278,21 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
-static struct port *__devinit add_port(u32 vtermno)
+static int __devinit add_port(struct ports_device *portdev)
 {
 	struct port *port;
-
-	port = kzalloc(sizeof *port, GFP_KERNEL);
-	if (!port)
-		return NULL;
-
-	port->used_len = port->offset = 0;
-	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!port->inbuf) {
-		kfree(port);
-		return NULL;
-	}
-	port->hvc = NULL;
-	port->vtermno = vtermno;
-	return port;
-}
-
-static void free_port(struct port *port)
-{
-	kfree(port->inbuf);
-	kfree(port);
-}
-
-/*
- * Once we're further in boot, we get probed like any other virtio
- * device.  At this stage we set up the output virtqueue.
- *
- * To set up and manage our virtual console, we call hvc_alloc().
- * Since we 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 *vdev)
-{
-	vq_callback_t *callbacks[] = { hvc_handle_input, NULL};
-	const char *names[] = { "input", "output" };
-	struct virtqueue *vqs[2];
-	struct port *port;
 	int err;
 
 	err = -ENOMEM;
-	port = add_port(pdrvdata.next_vtermno);
+	port = kzalloc(sizeof *port, GFP_KERNEL);
 	if (!port)
 		goto fail;
 
-	/* Attach this port to this virtio_device, and vice-versa. */
-	port->vdev = vdev;
-	vdev->priv = port;
-
-	/* Find the queues. */
-	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
-	if (err)
+	port->portdev = portdev;
+	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!port->inbuf)
 		goto free;
 
-	port->in_vq = vqs[0];
-	port->out_vq = vqs[1];
-
 	/*
 	 * The first argument of hvc_alloc() is the virtual console
 	 * number.  The second argument is the parameter for the
@@ -324,10 +304,11 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	 * pointers.  The final argument is the output buffer size: we
 	 * can do any size, so we put PAGE_SIZE here.
 	 */
+	port->vtermno = pdrvdata.next_vtermno;
 	port->hvc = hvc_alloc(port->vtermno, 0, &hv_ops, PAGE_SIZE);
 	if (IS_ERR(port->hvc)) {
 		err = PTR_ERR(port->hvc);
-		goto free_vqs;
+		goto free;
 	}
 
 	/* Add to vtermno list. */
@@ -339,6 +320,47 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	/* Register the input buffer the first time. */
 	add_inbuf(port);
 
+	return 0;
+free:
+	kfree(port);
+fail:
+	return err;
+}
+
+/*
+ * Once we're further in boot, we get probed like any other virtio
+ * device.
+ */
+static int __devinit virtcons_probe(struct virtio_device *vdev)
+{
+	vq_callback_t *callbacks[] = { hvc_handle_input, NULL};
+	const char *names[] = { "input", "output" };
+	struct virtqueue *vqs[2];
+	struct ports_device *portdev;
+	int err;
+
+	err = -ENOMEM;
+	portdev = kzalloc(sizeof *portdev, GFP_KERNEL);
+	if (!portdev)
+		goto fail;
+
+	/* Attach this portdev to this virtio_device, and vice-versa. */
+	portdev->vdev = vdev;
+	vdev->priv = portdev;
+
+	/* Find the queues. */
+	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
+	if (err)
+		goto free;
+
+	portdev->in_vq = vqs[0];
+	portdev->out_vq = vqs[1];
+
+	/* We only have one port. */
+	err = add_port(portdev);
+	if (err)
+		goto free_vqs;
+
 	/* Start using the new console output. */
 	early_put_chars = NULL;
 	return 0;
@@ -346,7 +368,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 free_vqs:
 	vdev->config->del_vqs(vdev);
 free:
-	free_port(port);
+	kfree(portdev);
 fail:
 	return err;
 }
-- 
1.6.2.5

[PATCH 10/28] virtio: console: ensure console size is updated on hvc open

[Amit Shah]

When multiple console support is added, ensure each port's size gets
updated when a new one is opened via hvc.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d4e2327..5a3d5df 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -201,27 +201,28 @@ static int get_chars(u32 vtermno, char *buf, int count)
 	return count;
 }
 
-/*
- * virtio console configuration. This supports:
- * - console resize
- */
-static void virtcons_apply_config(struct virtio_device *dev)
+static void resize_console(struct port *port)
 {
+	struct virtio_device *vdev;
 	struct winsize ws;
 
-	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
-		dev->config->get(dev,
-				 offsetof(struct virtio_console_config, cols),
-				 &ws.ws_col, sizeof(u16));
-		dev->config->get(dev,
-				 offsetof(struct virtio_console_config, rows),
-				 &ws.ws_row, sizeof(u16));
-		/* This is the pre-multiport style: we use control messages
-		 * these days which specify the port.  So this means port 0. */
-		hvc_resize(find_port_by_vtermno(0)->hvc, ws);
+	vdev = port->portdev->vdev;
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) {
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_console_config, cols),
+				  &ws.ws_col, sizeof(u16));
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_console_config, rows),
+				  &ws.ws_row, sizeof(u16));
+		hvc_resize(port->hvc, ws);
 	}
 }
 
+static void virtcons_apply_config(struct virtio_device *vdev)
+{
+	resize_console(find_port_by_vtermno(0));
+}
+
 /* We set the configuration at this point, since we now have a tty */
 static int notifier_add_vio(struct hvc_struct *hp, int data)
 {
@@ -232,7 +233,7 @@ static int notifier_add_vio(struct hvc_struct *hp, int data)
 		return -EINVAL;
 
 	hp->irq_requested = 1;
-	virtcons_apply_config(port->portdev->vdev);
+	resize_console(port);
 
 	return 0;
 }
-- 
1.6.2.5

[PATCH 11/28] virtio: console: Introduce a workqueue for handling host->guest notifications

[Amit Shah]

We currently only maintain one buffer per port for any data the
host sends us. If we're slow in consuming that data, we might lose
old data. To buffer the data that the host sends us, we need to be
able to allocate buffers as and when the host sends us some.

Using a workqueue gives us the freedom to allocate kernel buffers
since the workqueues are executed in process context.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5a3d5df..a80d15c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -21,6 +21,7 @@
 #include <linux/spinlock.h>
 #include <linux/virtio.h>
 #include <linux/virtio_console.h>
+#include <linux/workqueue.h>
 #include "hvc_console.h"
 
 /*
@@ -58,6 +59,12 @@ DEFINE_SPINLOCK(pdrvdata_lock);
 struct ports_device {
 	struct virtqueue *in_vq, *out_vq;
 	struct virtio_device *vdev;
+
+	/*
+	 * Workqueue handlers where we process deferred work after an
+	 * interrupt
+	 */
+	struct work_struct rx_work;
 };
 
 /* This struct holds the per-port data */
@@ -243,18 +250,6 @@ static void notifier_del_vio(struct hvc_struct *hp, int data)
 	hp->irq_requested = 0;
 }
 
-static void hvc_handle_input(struct virtqueue *vq)
-{
-	struct port *port;
-	bool activity = false;
-
-	list_for_each_entry(port, &pdrvdata.consoles, list)
-		activity |= hvc_poll(port->hvc);
-
-	if (activity)
-		hvc_kick();
-}
-
 /* The operations for the console. */
 static const struct hv_ops hv_ops = {
 	.get_chars = get_chars,
@@ -279,6 +274,26 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
+static void rx_work_handler(struct work_struct *work)
+{
+	struct port *port;
+	bool activity = false;
+
+	list_for_each_entry(port, &pdrvdata.consoles, list)
+		activity |= hvc_poll(port->hvc);
+
+	if (activity)
+		hvc_kick();
+}
+
+static void rx_intr(struct virtqueue *vq)
+{
+	struct ports_device *portdev;
+
+	portdev = vq->vdev->priv;
+	schedule_work(&portdev->rx_work);
+}
+
 static int __devinit add_port(struct ports_device *portdev)
 {
 	struct port *port;
@@ -334,7 +349,7 @@ fail:
  */
 static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
-	vq_callback_t *callbacks[] = { hvc_handle_input, NULL};
+	vq_callback_t *callbacks[] = { rx_intr, NULL};
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct ports_device *portdev;
@@ -357,6 +372,8 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	portdev->in_vq = vqs[0];
 	portdev->out_vq = vqs[1];
 
+	INIT_WORK(&portdev->rx_work, &rx_work_handler);
+
 	/* We only have one port. */
 	err = add_port(portdev);
 	if (err)
-- 
1.6.2.5

[PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Amit Shah]

The console could be flooded with data from the host; handle
this situation by buffering the data.

We move from the per-port inbuf to a per-device queue of buffers,
shared by all the ports for that device, ready to receive host data.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a80d15c..e8dabae 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -65,6 +65,23 @@ struct ports_device {
 	 * interrupt
 	 */
 	struct work_struct rx_work;
+
+	struct list_head unused_read_head;
+
+	/* To protect the list of unused read buffers and the in_vq */
+	spinlock_t read_list_lock;
+};
+
+/* This struct holds individual buffers received for each port */
+struct port_buffer {
+	struct list_head list;
+
+	char *buf;
+
+	/* length of the buffer */
+	size_t len;
+	/* offset in the buf from which to consume data */
+	size_t offset;
 };
 
 /* This struct holds the per-port data */
@@ -72,9 +89,15 @@ struct port {
 	/* Pointer to the parent virtio_console device */
 	struct ports_device *portdev;
 
-	/* This is our input buffer, and how much data is left in it. */
-	char *inbuf;
-	unsigned int used_len, offset;
+	/* Buffer management */
+	struct list_head readbuf_head;
+
+	/*
+	 * To protect the readbuf_head list.  Has to be a spinlock
+	 * because it can be called from interrupt context
+	 * (get_char())
+	 */
+	spinlock_t readbuf_list_lock;
 
 	/* For console ports, hvc != NULL and these are valid. */
 	/* The hvc device */
@@ -145,67 +168,71 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 }
 
 /*
- * Create a scatter-gather list representing our input buffer and put
- * it in the queue.
+ * Give out the data that's requested from the buffers that we have
+ * queued up.
  */
-static void add_inbuf(struct port *port)
+static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count)
 {
-	struct virtqueue *in_vq;
-	struct scatterlist sg[1];
-
-	sg_init_one(sg, port->inbuf, PAGE_SIZE);
+	struct port_buffer *buf, *buf2;
+	ssize_t out_offset, ret;
+	unsigned long flags;
 
-	in_vq = port->portdev->in_vq;
-	/* Should always be able to add one buffer to an empty queue. */
-	if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, port) < 0)
-		BUG();
-	in_vq->vq_ops->kick(in_vq);
+	out_offset = 0;
+	/*
+	 * Not taking the port->readbuf_list_lock here relying on the
+	 * fact that buffers are taken out from the list only in this
+	 * function so buf2 should be available all the time.
+	 */
+	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, list) {
+		size_t copy_size;
+
+		copy_size = out_count;
+		if (copy_size > buf->len - buf->offset)
+			copy_size = buf->len - buf->offset;
+
+		memcpy(out_buf + out_offset, buf->buf + buf->offset, copy_size);
+
+		/* Return the number of bytes actually copied */
+		ret = copy_size;
+		buf->offset += ret;
+		out_offset += ret;
+		out_count -= ret;
+
+		if (buf->len - buf->offset == 0) {
+			spin_lock_irqsave(&port->readbuf_list_lock, flags);
+			list_del(&buf->list);
+			spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
+			kfree(buf->buf);
+			kfree(buf);
+		}
+		if (!out_count)
+			break;
+	}
+	return out_offset;
 }
 
 /*
  * 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.
+ * We call out to fill_readbuf that gets us the required data from the
+ * buffers that are queued up.
  */
 static int get_chars(u32 vtermno, char *buf, int count)
 {
 	struct port *port;
-	struct virtqueue *in_vq;
 
 	port = find_port_by_vtermno(vtermno);
 	if (!port)
 		return 0;
 
-	in_vq = port->portdev->in_vq;
 	/* If we don't have an input queue yet, we can't get input. */
-	BUG_ON(!in_vq);
-
-	/* No more in buffer?  See if they've (re)used it. */
-	if (port->offset == port->used_len) {
-		unsigned int len;
-
-		if (!in_vq->vq_ops->get_buf(in_vq, &len))
-			return 0;
-		port->used_len = len;
-		port->offset = 0;
-	}
-
-	/* You want more than we have to give?  Well, try wanting less! */
-	if (port->offset + count > port->used_len)
-		count = port->used_len - port->offset;
+	BUG_ON(!port->portdev->in_vq);
 
-	/* Copy across to their buffer and increment offset. */
-	memcpy(buf, port->inbuf + port->offset, count);
-	port->offset += count;
-
-	/* Finished?  Re-register buffer so Host will use it again. */
-	if (port->offset == port->used_len)
-		add_inbuf(port);
+	if (list_empty(&port->readbuf_head))
+		return 0;
 
-	return count;
+	return fill_readbuf(port, buf, count);
 }
 
 static void resize_console(struct port *port)
@@ -274,16 +301,103 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
+static struct port_buffer *get_buf(size_t buf_size)
+{
+	struct port_buffer *buf;
+
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		goto out;
+	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	if (!buf->buf) {
+		kfree(buf);
+		goto out;
+	}
+	buf->len = buf_size;
+out:
+	return buf;
+}
+
+static void fill_receive_queue(struct ports_device *portdev)
+{
+	struct scatterlist sg[1];
+	struct port_buffer *buf;
+	struct virtqueue *vq;
+	int buf_size, ret;
+
+	vq = portdev->in_vq;
+	buf_size = PAGE_SIZE;
+	do {
+		buf = get_buf(buf_size);
+		if (!buf)
+			break;
+		sg_init_one(sg, buf->buf, buf_size);
+
+		spin_lock(&portdev->read_list_lock);
+		ret = vq->vq_ops->add_buf(vq, sg, 0, 1, buf);
+		if (ret < 0) {
+			spin_unlock(&portdev->read_list_lock);
+			kfree(buf->buf);
+			kfree(buf);
+			break;
+		}
+		/*
+		 * We have to keep track of the unused buffers so that
+		 * they can be freed when the module is being removed
+		 */
+		list_add_tail(&buf->list, &portdev->unused_read_head);
+		spin_unlock(&portdev->read_list_lock);
+	} while (ret > 0);
+
+	spin_lock(&portdev->read_list_lock);
+	vq->vq_ops->kick(vq);
+	spin_unlock(&portdev->read_list_lock);
+}
+
+static void *get_incoming_buf(struct ports_device *portdev,
+			      unsigned int *len)
+{
+	struct port_buffer *buf;
+	struct virtqueue *vq;
+
+	vq = portdev->in_vq;
+
+	spin_lock(&portdev->read_list_lock);
+	buf = vq->vq_ops->get_buf(vq, len);
+	if (buf) {
+		/* The buffer is no longer unused */
+		list_del(&buf->list);
+	}
+	spin_unlock(&portdev->read_list_lock);
+	return buf;
+}
+
 static void rx_work_handler(struct work_struct *work)
 {
+	struct ports_device *portdev;
 	struct port *port;
+	struct port_buffer *buf;
+	unsigned int tmplen;
 	bool activity = false;
 
-	list_for_each_entry(port, &pdrvdata.consoles, list)
-		activity |= hvc_poll(port->hvc);
+	portdev = container_of(work, struct ports_device, rx_work);
+
+	/* We currently have only one port */
+	port = find_port_by_vtermno(0);
+	while ((buf = get_incoming_buf(portdev, &tmplen))) {
+		buf->len = tmplen;
+		buf->offset = 0;
+
+		spin_lock_irq(&port->readbuf_list_lock);
+		list_add_tail(&buf->list, &port->readbuf_head);
+		spin_unlock_irq(&port->readbuf_list_lock);
 
+		activity |= hvc_poll(port->hvc);
+	}
 	if (activity)
 		hvc_kick();
+
+	fill_receive_queue(portdev);
 }
 
 static void rx_intr(struct virtqueue *vq)
@@ -305,9 +419,6 @@ static int __devinit add_port(struct ports_device *portdev)
 		goto fail;
 
 	port->portdev = portdev;
-	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!port->inbuf)
-		goto free;
 
 	/*
 	 * The first argument of hvc_alloc() is the virtual console
@@ -327,15 +438,15 @@ static int __devinit add_port(struct ports_device *portdev)
 		goto free;
 	}
 
+	spin_lock_init(&port->readbuf_list_lock);
+	INIT_LIST_HEAD(&port->readbuf_head);
+
 	/* Add to vtermno list. */
 	spin_lock_irq(&pdrvdata_lock);
 	pdrvdata.next_vtermno++;
 	list_add(&port->list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 
-	/* Register the input buffer the first time. */
-	add_inbuf(port);
-
 	return 0;
 free:
 	kfree(port);
@@ -372,8 +483,14 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	portdev->in_vq = vqs[0];
 	portdev->out_vq = vqs[1];
 
+	spin_lock_init(&portdev->read_list_lock);
+
+	INIT_LIST_HEAD(&portdev->unused_read_head);
+
 	INIT_WORK(&portdev->rx_work, &rx_work_handler);
 
+	fill_receive_queue(portdev);
+
 	/* We only have one port. */
 	err = add_port(portdev);
 	if (err)
-- 
1.6.2.5

[PATCH 13/28] virtio: console: Create a buffer pool for sending data to host

[Amit Shah]

The old way of sending data to the host was to populate one buffer and
then wait till the host consumed it before sending the next chunk of
data.

Also, there was no support to send large chunks of data.

We now maintain a per-device list of buffers that are ready to be
passed on to the host.

This patch adds support to send big chunks in multiple buffers of
PAGE_SIZE each to the host.

When the host consumes the data and signals to us via an interrupt, we
add the consumed buffer back to our unconsumed list.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e8dabae..3111e4c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -67,9 +67,13 @@ struct ports_device {
 	struct work_struct rx_work;
 
 	struct list_head unused_read_head;
+	struct list_head unused_write_head;
 
 	/* To protect the list of unused read buffers and the in_vq */
 	spinlock_t read_list_lock;
+
+	/* To protect the list of unused write buffers and the out_vq */
+	spinlock_t write_list_lock;
 };
 
 /* This struct holds individual buffers received for each port */
@@ -129,42 +133,64 @@ out:
 	return port;
 }
 
-/*
- * The 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 put_chars(u32 vtermno, const char *buf, int count)
+static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 {
 	struct scatterlist sg[1];
-	struct port *port;
 	struct virtqueue *out_vq;
-	unsigned int len;
+	struct port_buffer *buf;
+	size_t in_offset, copy_size;
+	ssize_t ret;
+	unsigned long irqf;
 
-	port = find_port_by_vtermno(vtermno);
-	if (!port)
+	if (!in_count)
 		return 0;
 
-	if (unlikely(early_put_chars))
-		return early_put_chars(vtermno, buf, count);
-
 	out_vq = port->portdev->out_vq;
-	/* This is a convenient routine to initialize a single-elem sg list */
-	sg_init_one(sg, buf, count);
-
-	/* This shouldn't fail: if it does, we lose chars. */
-	if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, port) >= 0) {
-		/* Tell Host to go! */
-		out_vq->vq_ops->kick(out_vq);
-		while (!out_vq->vq_ops->get_buf(out_vq, &len))
-			cpu_relax();
+
+	in_offset = 0; /* offset in the user buffer */
+	spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
+	while (in_count - in_offset) {
+		copy_size = min(in_count - in_offset, PAGE_SIZE);
+
+		if (list_empty(&port->portdev->unused_write_head))
+			break;
+
+		buf = list_first_entry(&port->portdev->unused_write_head,
+				       struct port_buffer, list);
+		list_del(&buf->list);
+		spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
+
+		/*
+		 * 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(buf->buf, in_buf + in_offset, copy_size);
+
+		buf->len = copy_size;
+		sg_init_one(sg, buf->buf, buf->len);
+
+		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
+		ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, buf);
+		if (ret < 0) {
+			memset(buf->buf, 0, buf->len);
+			list_add_tail(&buf->list,
+				      &port->portdev->unused_write_head);
+			break;
+		}
+		in_offset += buf->len;
+
+		/* No space left in the vq anyway */
+		if (!ret)
+			break;
 	}
+	/* Tell Host to go! */
+	out_vq->vq_ops->kick(out_vq);
+	spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
 
-	/* We're expected to return the amount of data we wrote: all of it. */
-	return count;
+	/* We're expected to return the amount of data we wrote */
+	return in_offset;
 }
 
 /*
@@ -212,6 +238,29 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count)
 }
 
 /*
+ * The 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.
+ *
+ * If the data to be output spans more than a page, it's split into
+ * page-sized buffers and then individual buffers are pushed to the Host.
+ */
+static int put_chars(u32 vtermno, const char *buf, int count)
+{
+	struct port *port;
+
+	port = find_port_by_vtermno(vtermno);
+	if (!port)
+		return 0;
+
+	if (unlikely(early_put_chars))
+		return early_put_chars(vtermno, buf, count);
+
+	return send_buf(port, buf, count);
+}
+
+/*
  * get_chars() is the callback from the hvc_console infrastructure
  * when an interrupt is received.
  *
@@ -318,6 +367,23 @@ out:
 	return buf;
 }
 
+/*
+ * This function is only called from the init routine so the spinlock
+ * for the unused_write_head list isn't taken
+ */
+static void alloc_write_bufs(struct ports_device *portdev)
+{
+	struct port_buffer *buf;
+	int i;
+
+	for (i = 0; i < 30; i++) {
+		buf = get_buf(PAGE_SIZE);
+		if (!buf)
+			break;
+		list_add_tail(&buf->list, &portdev->unused_write_head);
+	}
+}
+
 static void fill_receive_queue(struct ports_device *portdev)
 {
 	struct scatterlist sg[1];
@@ -408,6 +474,40 @@ static void rx_intr(struct virtqueue *vq)
 	schedule_work(&portdev->rx_work);
 }
 
+/*
+ * This is the interrupt handler for buffers that get received on the
+ * output virtqueue, which is an indication that Host consumed the
+ * data we sent it.  Since all our buffers going out are of a fixed
+ * size we can just reuse them instead of freeing them and allocating
+ * new ones.
+ *
+ * Zero out the buffer so that we don't leak any information from
+ * other processes.  There's a small optimisation here as well: the
+ * buffers are PAGE_SIZE-sized; but instead of zeroing the entire
+ * page, we just zero the length that was most recently used and we
+ * can be sure the rest of the page is already set to 0s.
+ *
+ * So once we zero them out we add them back to the unused buffers
+ * list.
+ */
+static void tx_intr(struct virtqueue *vq)
+{
+	struct ports_device *portdev;
+	struct port_buffer *buf;
+	unsigned long flags;
+	unsigned int tmplen;
+
+	portdev = vq->vdev->priv;
+
+	spin_lock_irqsave(&portdev->write_list_lock, flags);
+	while ((buf = vq->vq_ops->get_buf(vq, &tmplen))) {
+		/* 0 the buffer to not leak data from other processes */
+		memset(buf->buf, 0, buf->len);
+		list_add_tail(&buf->list, &portdev->unused_write_head);
+	}
+	spin_unlock_irqrestore(&portdev->write_list_lock, flags);
+}
+
 static int __devinit add_port(struct ports_device *portdev)
 {
 	struct port *port;
@@ -460,7 +560,7 @@ fail:
  */
 static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
-	vq_callback_t *callbacks[] = { rx_intr, NULL};
+	vq_callback_t *callbacks[] = { rx_intr, tx_intr };
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct ports_device *portdev;
@@ -484,12 +584,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	portdev->out_vq = vqs[1];
 
 	spin_lock_init(&portdev->read_list_lock);
+	spin_lock_init(&portdev->write_list_lock);
 
 	INIT_LIST_HEAD(&portdev->unused_read_head);
+	INIT_LIST_HEAD(&portdev->unused_write_head);
 
 	INIT_WORK(&portdev->rx_work, &rx_work_handler);
 
 	fill_receive_queue(portdev);
+	alloc_write_bufs(portdev);
 
 	/* We only have one port. */
 	err = add_port(portdev);
-- 
1.6.2.5

[PATCH 14/28] virtio: console: Separate out console-specific data into a separate struct

[Amit Shah]

Move out console-specific stuff into a separate struct from 'struct
port' as we need to maintain two lists: one for all the ports (which
includes consoles) and one only for consoles since the hvc callbacks
only give us the vtermno.

This makes console handling cleaner.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3111e4c..da8a3ff 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -52,6 +52,24 @@ static struct ports_driver_data pdrvdata;
 
 DEFINE_SPINLOCK(pdrvdata_lock);
 
+/* This struct holds information that's relevant only for console ports */
+struct console {
+	/* We'll place all consoles in a list in the pdrvdata struct */
+	struct list_head list;
+
+	/* The hvc device associated with this console port */
+	struct hvc_struct *hvc;
+
+	/*
+	 * This number identifies the number that we used to register
+	 * with hvc in hvc_instantiate() and hvc_alloc(); this is the
+	 * number passed on by the hvc callbacks to us to
+	 * differentiate between the other console ports handled by
+	 * this driver
+	 */
+	u32 vtermno;
+};
+
 /*
  * This is a per-device struct that stores data common to all the
  * ports for that device (vdev->priv).
@@ -103,15 +121,11 @@ struct port {
 	 */
 	spinlock_t readbuf_list_lock;
 
-	/* For console ports, hvc != NULL and these are valid. */
-	/* The hvc device */
-	struct hvc_struct *hvc;
-
-	/* We'll place all consoles in a list in the pdrvdata struct */
-	struct list_head list;
-
-	/* Our vterm number. */
-	u32 vtermno;
+	/*
+	 * The entries in this struct will be valid if this port is
+	 * hooked up to an hvc console
+	 */
+	struct console cons;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -120,12 +134,15 @@ static int (*early_put_chars)(u32, const char *, int);
 static struct port *find_port_by_vtermno(u32 vtermno)
 {
 	struct port *port;
+	struct console *cons;
 	unsigned long flags;
 
 	spin_lock_irqsave(&pdrvdata_lock, flags);
-	list_for_each_entry(port, &pdrvdata.consoles, list) {
-		if (port->vtermno == vtermno)
+	list_for_each_entry(cons, &pdrvdata.consoles, list) {
+		if (cons->vtermno == vtermno) {
+			port = container_of(cons, struct port, cons);
 			goto out;
+		}
 	}
 	port = NULL;
 out:
@@ -297,7 +314,7 @@ static void resize_console(struct port *port)
 		vdev->config->get(vdev,
 				  offsetof(struct virtio_console_config, rows),
 				  &ws.ws_row, sizeof(u16));
-		hvc_resize(port->hvc, ws);
+		hvc_resize(port->cons.hvc, ws);
 	}
 }
 
@@ -444,7 +461,7 @@ static void rx_work_handler(struct work_struct *work)
 	struct port *port;
 	struct port_buffer *buf;
 	unsigned int tmplen;
-	bool activity = false;
+	bool console_activity = false;
 
 	portdev = container_of(work, struct ports_device, rx_work);
 
@@ -458,9 +475,9 @@ static void rx_work_handler(struct work_struct *work)
 		list_add_tail(&buf->list, &port->readbuf_head);
 		spin_unlock_irq(&port->readbuf_list_lock);
 
-		activity |= hvc_poll(port->hvc);
+		console_activity |= hvc_poll(port->cons.hvc);
 	}
-	if (activity)
+	if (console_activity)
 		hvc_kick();
 
 	fill_receive_queue(portdev);
@@ -531,10 +548,10 @@ static int __devinit add_port(struct ports_device *portdev)
 	 * pointers.  The final argument is the output buffer size: we
 	 * can do any size, so we put PAGE_SIZE here.
 	 */
-	port->vtermno = pdrvdata.next_vtermno;
-	port->hvc = hvc_alloc(port->vtermno, 0, &hv_ops, PAGE_SIZE);
-	if (IS_ERR(port->hvc)) {
-		err = PTR_ERR(port->hvc);
+	port->cons.vtermno = pdrvdata.next_vtermno;
+	port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
+	if (IS_ERR(port->cons.hvc)) {
+		err = PTR_ERR(port->cons.hvc);
 		goto free;
 	}
 
@@ -544,7 +561,7 @@ static int __devinit add_port(struct ports_device *portdev)
 	/* Add to vtermno list. */
 	spin_lock_irq(&pdrvdata_lock);
 	pdrvdata.next_vtermno++;
-	list_add(&port->list, &pdrvdata.consoles);
+	list_add(&port->cons.list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 
 	return 0;
-- 
1.6.2.5

[PATCH 15/28] virtio: console: Separate out console init into a new function

[Amit Shah]

Console ports could be hot-added. Also, with the new multiport support,
a port is identified as a console port only if the host sends a control
message.

Move the console port init into a separate function so it can be invoked
from other places.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index da8a3ff..64a927c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -367,6 +367,43 @@ int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int))
 	return hvc_instantiate(0, 0, &hv_ops);
 }
 
+int init_port_console(struct port *port)
+{
+	int ret;
+
+	/*
+	 * The Host's telling us this port is a console port.  Hook it
+	 * up with an hvc console.
+	 *
+	 * To set up and manage our virtual console, we call
+	 * hvc_alloc().
+	 *
+	 * The first argument of hvc_alloc() is the virtual console
+	 * number.  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.
+	 */
+	port->cons.vtermno = pdrvdata.next_vtermno;
+
+	port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
+	if (IS_ERR(port->cons.hvc)) {
+		ret = PTR_ERR(port->cons.hvc);
+		port->cons.hvc = NULL;
+		return ret;
+	}
+	spin_lock_irq(&pdrvdata_lock);
+	pdrvdata.next_vtermno++;
+	list_add_tail(&port->cons.list, &pdrvdata.consoles);
+	spin_unlock_irq(&pdrvdata_lock);
+
+	return 0;
+}
+
 static struct port_buffer *get_buf(size_t buf_size)
 {
 	struct port_buffer *buf;
@@ -537,32 +574,12 @@ static int __devinit add_port(struct ports_device *portdev)
 
 	port->portdev = portdev;
 
-	/*
-	 * The first argument of hvc_alloc() is the virtual console
-	 * number.  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.
-	 */
-	port->cons.vtermno = pdrvdata.next_vtermno;
-	port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
-	if (IS_ERR(port->cons.hvc)) {
-		err = PTR_ERR(port->cons.hvc);
-		goto free;
-	}
-
 	spin_lock_init(&port->readbuf_list_lock);
 	INIT_LIST_HEAD(&port->readbuf_head);
 
-	/* Add to vtermno list. */
-	spin_lock_irq(&pdrvdata_lock);
-	pdrvdata.next_vtermno++;
-	list_add(&port->cons.list, &pdrvdata.consoles);
-	spin_unlock_irq(&pdrvdata_lock);
+	err = init_port_console(port);
+	if (err)
+		goto free;
 
 	return 0;
 free:
-- 
1.6.2.5

[PATCH 16/28] virtio: console: Introduce a 'header' for each buffer towards supporting multiport

[Amit Shah]

To identify which port data that gets sent to the Host belongs to, we'll
have to add a 'header' to each outgoing buffer that will tell the Host
the port number.

This is also done for each buffer that's received from userspace.

The header is currently a 0-length field, but will contain the port id
among other things when support for multiple ports per device is added.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 64a927c..def1678 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -150,24 +150,39 @@ out:
 	return port;
 }
 
+static inline bool use_multiport(struct ports_device *portdev)
+{
+	/*
+	 * This condition can be true when put_chars is called from
+	 * early_init
+	 */
+	if (!portdev->vdev)
+		return 0;
+	/* Check for feature bit once multiport support has been added */
+	return 0;
+}
+
 static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 {
 	struct scatterlist sg[1];
+	struct virtio_console_header header;
 	struct virtqueue *out_vq;
 	struct port_buffer *buf;
 	size_t in_offset, copy_size;
 	ssize_t ret;
 	unsigned long irqf;
+	unsigned int header_len;
 
 	if (!in_count)
 		return 0;
 
 	out_vq = port->portdev->out_vq;
+	header_len = use_multiport(port->portdev) ? sizeof(header) : 0;
 
 	in_offset = 0; /* offset in the user buffer */
 	spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
 	while (in_count - in_offset) {
-		copy_size = min(in_count - in_offset, PAGE_SIZE);
+		copy_size = min(in_count - in_offset + header_len, PAGE_SIZE);
 
 		if (list_empty(&port->portdev->unused_write_head))
 			break;
@@ -177,15 +192,19 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 		list_del(&buf->list);
 		spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
 
+		if (header_len) {
+			memcpy(buf->buf, &header, header_len);
+			copy_size -= header_len;
+		}
 		/*
 		 * 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(buf->buf, in_buf + in_offset, copy_size);
+		memcpy(buf->buf + header_len, in_buf + in_offset, copy_size);
 
-		buf->len = copy_size;
+		buf->len = header_len + copy_size;
 		sg_init_one(sg, buf->buf, buf->len);
 
 		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
@@ -196,7 +215,7 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 				      &port->portdev->unused_write_head);
 			break;
 		}
-		in_offset += buf->len;
+		in_offset += buf->len - header_len;
 
 		/* No space left in the vq anyway */
 		if (!ret)
@@ -494,19 +513,25 @@ static void *get_incoming_buf(struct ports_device *portdev,
 
 static void rx_work_handler(struct work_struct *work)
 {
+	struct virtio_console_header header;
 	struct ports_device *portdev;
 	struct port *port;
 	struct port_buffer *buf;
-	unsigned int tmplen;
+	unsigned int tmplen, header_len;
 	bool console_activity = false;
 
 	portdev = container_of(work, struct ports_device, rx_work);
+	header_len = use_multiport(portdev) ? sizeof(header) : 0;
 
 	/* We currently have only one port */
 	port = find_port_by_vtermno(0);
 	while ((buf = get_incoming_buf(portdev, &tmplen))) {
+
+		if (use_multiport(portdev))
+			memcpy(&header, buf->buf, header_len);
+
 		buf->len = tmplen;
-		buf->offset = 0;
+		buf->offset = header_len;
 
 		spin_lock_irq(&port->readbuf_list_lock);
 		list_add_tail(&buf->list, &port->readbuf_head);
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 9e0da40..3dfc7d1 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -18,6 +18,13 @@ struct virtio_console_config {
 	__u16 rows;
 } __attribute__((packed));
 
+/*
+ * This struct is put at the start of each buffer that gets passed to
+ * Host and vice-versa.
+ */
+struct virtio_console_header {
+	/* Empty till multiport support is added */
+} __attribute__((packed));
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

[PATCH 17/28] virtio: console: Add a new MULTIPORT feature, support for generic ports

[Amit Shah]

This commit adds a new feature, MULTIPORT. If the host supports this
feature as well, the config space has the number of ports defined for
that device. New ports are spawned according to this information.

Using this feature, generic ports can be created which are not tied to
hvc consoles.

We also open up a private channel between the host and the guest via
which some "control" messages are exchanged for the ports, like whether
the port being spawned is a console port, resizing the console window,
etc.

Next commits will add support for hotplugging and presenting char
devices in /dev/ for bi-directional guest-host communication.

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

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 6aad99e..006b815 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -679,6 +679,14 @@ config VIRTIO_CONSOLE
 	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/vportNpn will be created when corresponding ports are
+	  found, where N is the device number and n is the port number
+	  within that device.  If specified by the host, a sysfs
+	  attribute called 'name' will be populated with a name for
+	  the port which can be used by udev scripts to create a
+	  symlink to the device.
 
 config HVCS
 	tristate "IBM Hypervisor Virtual Console Server support"
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index def1678..a26781b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2006, 2007, 2009 Rusty Russell, IBM Corporation
+ * Copyright (C) 2009, 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
@@ -84,14 +85,21 @@ struct ports_device {
 	 */
 	struct work_struct rx_work;
 
+	struct list_head ports_head;
 	struct list_head unused_read_head;
 	struct list_head unused_write_head;
 
+	/* To protect the list of ports */
+	spinlock_t ports_list_lock;
+
 	/* To protect the list of unused read buffers and the in_vq */
 	spinlock_t read_list_lock;
 
 	/* To protect the list of unused write buffers and the out_vq */
 	spinlock_t write_list_lock;
+
+	/* The current config space is stored here */
+	struct virtio_console_config config;
 };
 
 /* This struct holds individual buffers received for each port */
@@ -108,6 +116,9 @@ struct port_buffer {
 
 /* This struct holds the per-port data */
 struct port {
+	/* Next port in the list, head is in the ports_device */
+	struct list_head list;
+
 	/* Pointer to the parent virtio_console device */
 	struct ports_device *portdev;
 
@@ -126,6 +137,9 @@ struct port {
 	 * hooked up to an hvc console
 	 */
 	struct console cons;
+
+	/* The 'id' to identify the port with the Host */
+	u32 id;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -150,6 +164,28 @@ out:
 	return port;
 }
 
+static struct port *find_port_by_id(struct ports_device *portdev, u32 id)
+{
+	struct port *port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&portdev->ports_list_lock, flags);
+	list_for_each_entry(port, &portdev->ports_head, list)
+		if (port->id == id)
+			goto out;
+	port = NULL;
+out:
+	spin_unlock_irqrestore(&portdev->ports_list_lock, flags);
+	return port;
+}
+
+static bool is_console_port(struct port *port)
+{
+	if (port->cons.hvc)
+		return true;
+	return false;
+}
+
 static inline bool use_multiport(struct ports_device *portdev)
 {
 	/*
@@ -158,11 +194,16 @@ static inline bool use_multiport(struct ports_device *portdev)
 	 */
 	if (!portdev->vdev)
 		return 0;
-	/* Check for feature bit once multiport support has been added */
-	return 0;
+	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+static inline bool is_control(u32 flags)
+{
+	return flags & VIRTIO_CONSOLE_HDR_CONTROL;
 }
 
-static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
+static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
+			u32 flags)
 {
 	struct scatterlist sg[1];
 	struct virtio_console_header header;
@@ -177,6 +218,16 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 		return 0;
 
 	out_vq = port->portdev->out_vq;
+
+	/*
+	 * We should not send control messages to a host that won't
+	 * understand them
+	 */
+	if (!use_multiport(port->portdev) && is_control(flags))
+		return 0;
+
+	header.id = port->id;
+	header.flags = flags;
 	header_len = use_multiport(port->portdev) ? sizeof(header) : 0;
 
 	in_offset = 0; /* offset in the user buffer */
@@ -229,6 +280,17 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count)
 	return in_offset;
 }
 
+static ssize_t send_control_msg(struct port *port, unsigned int event,
+				unsigned int value)
+{
+	struct virtio_console_control cpkt;
+
+	cpkt.event = event;
+	cpkt.value = value;
+	return send_buf(port, (char *)&cpkt, sizeof(cpkt),
+			VIRTIO_CONSOLE_HDR_CONTROL);
+}
+
 /*
  * Give out the data that's requested from the buffers that we have
  * queued up.
@@ -293,7 +355,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
 
-	return send_buf(port, buf, count);
+	return send_buf(port, buf, count, 0);
 }
 
 /*
@@ -362,7 +424,7 @@ static void notifier_del_vio(struct hvc_struct *hp, int data)
 	hp->irq_requested = 0;
 }
 
-/* The operations for the console. */
+/* The operations for console ports. */
 static const struct hv_ops hv_ops = {
 	.get_chars = get_chars,
 	.put_chars = put_chars,
@@ -493,6 +555,35 @@ static void fill_receive_queue(struct ports_device *portdev)
 	spin_unlock(&portdev->read_list_lock);
 }
 
+/* Any private messages that the Host and Guest want to share */
+static void handle_control_message(struct port *port, struct port_buffer *buf)
+{
+	struct virtio_console_control *cpkt;
+
+	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
+
+	switch (cpkt->event) {
+	case VIRTIO_CONSOLE_CONSOLE_PORT:
+		if (!cpkt->value)
+			break;
+		if (is_console_port(port))
+			break;
+
+		init_port_console(port);
+		/*
+		 * Could remove the port here in case init fails - but
+		 * have to notify the host first.
+		 */
+		break;
+	case VIRTIO_CONSOLE_RESIZE:
+		if (!is_console_port(port))
+			break;
+		port->cons.hvc->irq_requested = 1;
+		resize_console(port);
+		break;
+	}
+}
+
 static void *get_incoming_buf(struct ports_device *portdev,
 			      unsigned int *len)
 {
@@ -523,21 +614,43 @@ static void rx_work_handler(struct work_struct *work)
 	portdev = container_of(work, struct ports_device, rx_work);
 	header_len = use_multiport(portdev) ? sizeof(header) : 0;
 
-	/* We currently have only one port */
-	port = find_port_by_vtermno(0);
 	while ((buf = get_incoming_buf(portdev, &tmplen))) {
-
+		header.id = 0;
 		if (use_multiport(portdev))
 			memcpy(&header, buf->buf, header_len);
 
+		port = find_port_by_id(portdev, header.id);
+		if (!port) {
+			/* No valid header at start of buffer. Drop it. */
+			dev_dbg(&portdev->vdev->dev,
+				"invalid index %u in header\n",	header.id);
+			/*
+			 * OPT: This buffer can be added to the unused
+			 * list to avoid free / alloc
+			 */
+			kfree(buf->buf);
+			kfree(buf);
+			continue;
+		}
 		buf->len = tmplen;
 		buf->offset = header_len;
 
+		if (use_multiport(portdev) && is_control(header.flags)) {
+			handle_control_message(port, buf);
+			/*
+			 * OPT: This buffer can be added to the unused
+			 * list to avoid free/alloc
+			 */
+			kfree(buf->buf);
+			kfree(buf);
+			continue;
+		}
 		spin_lock_irq(&port->readbuf_list_lock);
 		list_add_tail(&buf->list, &port->readbuf_head);
 		spin_unlock_irq(&port->readbuf_list_lock);
 
-		console_activity |= hvc_poll(port->cons.hvc);
+		if (is_console_port(port))
+			console_activity |= hvc_poll(port->cons.hvc);
 	}
 	if (console_activity)
 		hvc_kick();
@@ -587,7 +700,7 @@ static void tx_intr(struct virtqueue *vq)
 	spin_unlock_irqrestore(&portdev->write_list_lock, flags);
 }
 
-static int __devinit add_port(struct ports_device *portdev)
+static int __devinit add_port(struct ports_device *portdev, u32 id)
 {
 	struct port *port;
 	int err;
@@ -598,13 +711,30 @@ static int __devinit add_port(struct ports_device *portdev)
 		goto fail;
 
 	port->portdev = portdev;
+	port->id = id;
 
 	spin_lock_init(&port->readbuf_list_lock);
 	INIT_LIST_HEAD(&port->readbuf_head);
 
-	err = init_port_console(port);
-	if (err)
-		goto free;
+	/*
+	 * If we're not using multiport support, this has to be a console port
+	 */
+	if (!use_multiport(port->portdev)) {
+		err = init_port_console(port);
+		if (err)
+			goto free;
+	}
+	spin_lock_irq(&portdev->ports_list_lock);
+	list_add_tail(&port->list, &port->portdev->ports_head);
+	spin_unlock_irq(&portdev->ports_list_lock);
+
+	/*
+	 * Tell the host we're set so that the Host can send us
+	 * various configuration parameters for this port (eg, port
+	 * name; caching, throttling parameters; whether this is a
+	 * console port, etc.)
+	 */
+	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
 
 	return 0;
 free:
@@ -616,6 +746,10 @@ fail:
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
+ *
+ * If the host also supports multiple console ports, we check the
+ * config space to see how many ports the host has spawned.  We
+ * initialize each port found.
  */
 static int __devinit virtcons_probe(struct virtio_device *vdev)
 {
@@ -623,7 +757,9 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	const char *names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct ports_device *portdev;
+	u32 i;
 	int err;
+	bool multiport;
 
 	err = -ENOMEM;
 	portdev = kzalloc(sizeof *portdev, GFP_KERNEL);
@@ -634,6 +770,18 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	portdev->vdev = vdev;
 	vdev->priv = portdev;
 
+	multiport = false;
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+		multiport = true;
+		vdev->features[0] |= 1 << VIRTIO_CONSOLE_F_MULTIPORT;
+
+		vdev->config->get(vdev, offsetof(struct virtio_console_config,
+						 nr_ports),
+				  &portdev->config.nr_ports,
+				  sizeof(portdev->config.nr_ports));
+	}
+	vdev->config->finalize_features(vdev);
+
 	/* Find the queues. */
 	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
 	if (err)
@@ -644,7 +792,9 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	spin_lock_init(&portdev->read_list_lock);
 	spin_lock_init(&portdev->write_list_lock);
+	spin_lock_init(&portdev->ports_list_lock);
 
+	INIT_LIST_HEAD(&portdev->ports_head);
 	INIT_LIST_HEAD(&portdev->unused_read_head);
 	INIT_LIST_HEAD(&portdev->unused_write_head);
 
@@ -653,17 +803,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	fill_receive_queue(portdev);
 	alloc_write_bufs(portdev);
 
-	/* We only have one port. */
-	err = add_port(portdev);
-	if (err)
-		goto free_vqs;
+	add_port(portdev, 0);
+	if (multiport)
+		for (i = 1; i < portdev->config.nr_ports; i++)
+			add_port(portdev, i);
 
 	/* Start using the new console output. */
 	early_put_chars = NULL;
 	return 0;
 
-free_vqs:
-	vdev->config->del_vqs(vdev);
 free:
 	kfree(portdev);
 fail:
@@ -677,6 +825,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 = {
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 3dfc7d1..ad9557d 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -6,26 +6,51 @@
 /*
  * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
  * anyone can use the definitions to implement compatible drivers/servers.
+ *
+ * Copyright (C) Red Hat, Inc., 2009
  */
 
 /* Feature bits */
 #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
+#define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
 
 struct virtio_console_config {
 	/* colums of the screens */
 	__u16 cols;
 	/* rows of the screens */
 	__u16 rows;
+	/* number of ports added so far */
+	__u32 nr_ports;
 } __attribute__((packed));
 
 /*
+ * A message that's passed between the Host and the Guest for a
+ * particular port.
+ */
+struct virtio_console_control {
+	__u16 event;
+	__u16 value;
+};
+
+/* Some events for control messages */
+#define VIRTIO_CONSOLE_PORT_READY	0
+#define VIRTIO_CONSOLE_CONSOLE_PORT	1
+#define VIRTIO_CONSOLE_RESIZE		2
+
+/*
  * This struct is put at the start of each buffer that gets passed to
  * Host and vice-versa.
  */
 struct virtio_console_header {
-	/* Empty till multiport support is added */
+	/* Port number */
+	u32 id;
+	/* Some message between host and guest */
+	u32 flags;
 } __attribute__((packed));
 
+/* Messages between host and guest ('flags' field in the header above) */
+#define VIRTIO_CONSOLE_HDR_CONTROL	(1 << 0)
+
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
 #endif /* __KERNEL__ */
-- 
1.6.2.5

[PATCH 18/28] virtio: console: Associate each port with a char device

[Amit Shah]

The char device will be used as an interface by applications on the
guest to communicate with apps on the host.

The devices created are placed in /dev/vportNpn where N is the
virtio-console device number and n is the port number for that device.

The file operation for the char devs will be added in the following
commits.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index a26781b..56b7cfa 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -16,6 +16,8 @@
  * 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/init.h>
 #include <linux/list.h>
@@ -34,6 +36,12 @@
  * across multiple devices and multiple ports per device.
  */
 struct ports_driver_data {
+	/* Used for registering chardevs */
+	struct class *class;
+
+	/* Number of devices this driver is handling */
+	unsigned int index;
+
 	/*
 	 * This is used to keep track of the number of hvc consoles
 	 * spawned by this driver.  This number is given as the first
@@ -100,6 +108,12 @@ struct ports_device {
 
 	/* The current config space is stored here */
 	struct virtio_console_config config;
+
+	/* Used for numbering devices for sysfs and debugfs */
+	unsigned int drv_index;
+
+	/* Major number for this device. Ports will be created as minors. */
+	int chr_major;
 };
 
 /* This struct holds individual buffers received for each port */
@@ -132,6 +146,10 @@ struct port {
 	 */
 	spinlock_t readbuf_list_lock;
 
+	/* Each port associates with a separate char device */
+	struct cdev cdev;
+	struct device *dev;
+
 	/*
 	 * The entries in this struct will be valid if this port is
 	 * hooked up to an hvc console
@@ -703,6 +721,7 @@ static void tx_intr(struct virtqueue *vq)
 static int __devinit add_port(struct ports_device *portdev, u32 id)
 {
 	struct port *port;
+	dev_t devt;
 	int err;
 
 	err = -ENOMEM;
@@ -713,6 +732,25 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	port->portdev = portdev;
 	port->id = id;
 
+	cdev_init(&port->cdev, NULL);
+
+	devt = MKDEV(portdev->chr_major, id);
+	err = cdev_add(&port->cdev, devt, 1);
+	if (err < 0) {
+		dev_err(&port->portdev->vdev->dev,
+			"error %d adding cdev for port %u\n", err, id);
+		goto free_port;
+	}
+	port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
+				  devt, port, "vport%up%u",
+				  port->portdev->drv_index, id);
+	if (IS_ERR(port->dev)) {
+		err = PTR_ERR(port->dev);
+		dev_err(&port->portdev->vdev->dev,
+			"error %d creating device for port %u\n",
+			err, id);
+		goto free_cdev;
+	}
 	spin_lock_init(&port->readbuf_list_lock);
 	INIT_LIST_HEAD(&port->readbuf_head);
 
@@ -722,7 +760,7 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	if (!use_multiport(port->portdev)) {
 		err = init_port_console(port);
 		if (err)
-			goto free;
+			goto free_device;
 	}
 	spin_lock_irq(&portdev->ports_list_lock);
 	list_add_tail(&port->list, &port->portdev->ports_head);
@@ -737,12 +775,21 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
 
 	return 0;
-free:
+
+free_device:
+	device_destroy(pdrvdata.class, port->dev->devt);
+free_cdev:
+	cdev_del(&port->cdev);
+free_port:
 	kfree(port);
 fail:
 	return err;
 }
 
+static struct file_operations portdev_fops = {
+	.owner = THIS_MODULE,
+};
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
@@ -770,6 +817,18 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	portdev->vdev = vdev;
 	vdev->priv = portdev;
 
+	spin_lock_irq(&pdrvdata_lock);
+	portdev->drv_index = pdrvdata.index++;
+	spin_unlock_irq(&pdrvdata_lock);
+
+	portdev->chr_major = register_chrdev(0, "virtio-portsdev",
+					     &portdev_fops);
+	if (portdev->chr_major < 0) {
+		dev_err(&vdev->dev,
+			"error %d registering chrdev for device %u\n",
+			portdev->chr_major, portdev->drv_index);
+		goto free;
+	}
 	multiport = false;
 	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
 		multiport = true;
@@ -785,7 +844,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	/* Find the queues. */
 	err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
 	if (err)
-		goto free;
+		goto free_chrdev;
 
 	portdev->in_vq = vqs[0];
 	portdev->out_vq = vqs[1];
@@ -812,6 +871,8 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	early_put_chars = NULL;
 	return 0;
 
+free_chrdev:
+	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
 	kfree(portdev);
 fail:
@@ -840,7 +901,16 @@ static struct virtio_driver virtio_console = {
 
 static int __init init(void)
 {
+	int err;
+
+	pdrvdata.class = class_create(THIS_MODULE, "virtio-ports");
+	if (IS_ERR(pdrvdata.class)) {
+		err = PTR_ERR(pdrvdata.class);
+		pr_err("Error %d creating virtio-ports class\n", err);
+		return err;
+	}
 	INIT_LIST_HEAD(&pdrvdata.consoles);
+
 	return register_virtio_driver(&virtio_console);
 }
 module_init(init);
-- 
1.6.2.5

[PATCH 19/28] virtio: console: Prepare for writing to / reading from userspace buffers

[Amit Shah]

When ports get advertised as char devices, the buffers will come from
userspace. Equip the send_buf and fill_readbuf functions with the
ability to write to / read from userspace buffers respectively.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 56b7cfa..4d55f0f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -221,7 +221,7 @@ static inline bool is_control(u32 flags)
 }
 
 static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
-			u32 flags)
+			u32 flags, bool from_user)
 {
 	struct scatterlist sg[1];
 	struct virtio_console_header header;
@@ -265,15 +265,21 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
 			memcpy(buf->buf, &header, header_len);
 			copy_size -= header_len;
 		}
-		/*
-		 * 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(buf->buf + header_len, in_buf + in_offset, copy_size);
-
-		buf->len = header_len + copy_size;
+		if (from_user) {
+			ret = copy_from_user(buf->buf + 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(buf->buf + header_len,
+			       in_buf + in_offset, copy_size);
+			ret = 0; /* Emulate copy_from_user behaviour */
+		}
+		buf->len = header_len + copy_size - ret;
 		sg_init_one(sg, buf->buf, buf->len);
 
 		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
@@ -306,14 +312,15 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	cpkt.event = event;
 	cpkt.value = value;
 	return send_buf(port, (char *)&cpkt, sizeof(cpkt),
-			VIRTIO_CONSOLE_HDR_CONTROL);
+			VIRTIO_CONSOLE_HDR_CONTROL, false);
 }
 
 /*
  * Give out the data that's requested from the buffers that we have
  * queued up.
  */
-static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count)
+static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
+			    bool to_user)
 {
 	struct port_buffer *buf, *buf2;
 	ssize_t out_offset, ret;
@@ -324,6 +331,8 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count)
 	 * Not taking the port->readbuf_list_lock here relying on the
 	 * fact that buffers are taken out from the list only in this
 	 * function so buf2 should be available all the time.
+	 *
+	 * Also, copy_to_user() might sleep.
 	 */
 	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, list) {
 		size_t copy_size;
@@ -332,10 +341,20 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count)
 		if (copy_size > buf->len - buf->offset)
 			copy_size = buf->len - buf->offset;
 
-		memcpy(out_buf + out_offset, buf->buf + buf->offset, copy_size);
+		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 = copy_size - ret;
 		buf->offset += ret;
 		out_offset += ret;
 		out_count -= ret;
@@ -373,7 +392,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
 
-	return send_buf(port, buf, count, 0);
+	return send_buf(port, buf, count, 0, false);
 }
 
 /*
@@ -397,7 +416,7 @@ static int get_chars(u32 vtermno, char *buf, int count)
 	if (list_empty(&port->readbuf_head))
 		return 0;
 
-	return fill_readbuf(port, buf, count);
+	return fill_readbuf(port, buf, count, false);
 }
 
 static void resize_console(struct port *port)
-- 
1.6.2.5

[PATCH 20/28] virtio: console: Add file operations to ports for open/read/write/poll

[Amit Shah]

Allow guest userspace applications to open, read from, write to, poll
the ports via the char dev interface.

When a port gets opened, a notification is sent to the host via a
control message indicating a connection has been established. Similarly,
on closing of the port, a notification is sent indicating disconnection.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 4d55f0f..66875d6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -19,11 +19,15 @@
 #include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/virtio.h>
 #include <linux/virtio_console.h>
+#include <linux/wait.h>
 #include <linux/workqueue.h>
 #include "hvc_console.h"
 
@@ -146,6 +150,9 @@ struct port {
 	 */
 	spinlock_t readbuf_list_lock;
 
+	/* A waitqueue for poll() or blocking read operations */
+	wait_queue_head_t waitqueue;
+
 	/* Each port associates with a separate char device */
 	struct cdev cdev;
 	struct device *dev;
@@ -158,6 +165,9 @@ struct port {
 
 	/* The 'id' to identify the port with the Host */
 	u32 id;
+
+	/* Is the host device open */
+	bool host_connected;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -245,7 +255,7 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
 		return 0;
 
 	header.id = port->id;
-	header.flags = flags;
+	header.flags = flags | VIRTIO_CONSOLE_HDR_START_DATA;
 	header_len = use_multiport(port->portdev) ? sizeof(header) : 0;
 
 	in_offset = 0; /* offset in the user buffer */
@@ -261,10 +271,7 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
 		list_del(&buf->list);
 		spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
 
-		if (header_len) {
-			memcpy(buf->buf, &header, header_len);
-			copy_size -= header_len;
-		}
+		copy_size -= header_len;
 		if (from_user) {
 			ret = copy_from_user(buf->buf + header_len,
 					     in_buf + in_offset, copy_size);
@@ -280,8 +287,13 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
 			ret = 0; /* Emulate copy_from_user behaviour */
 		}
 		buf->len = header_len + copy_size - ret;
-		sg_init_one(sg, buf->buf, buf->len);
+		if (!(in_count - in_offset - (buf->len - header_len)))
+			header.flags |= VIRTIO_CONSOLE_HDR_END_DATA;
 
+		if (header_len)
+			memcpy(buf->buf, &header, header_len);
+
+		sg_init_one(sg, buf->buf, buf->len);
 		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
 		ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, buf);
 		if (ret < 0) {
@@ -292,6 +304,9 @@ static ssize_t send_buf(struct port *port, const char *in_buf, size_t in_count,
 		}
 		in_offset += buf->len - header_len;
 
+		/* Set the START_DATA flag only for the first buffer. */
+		header.flags &= ~VIRTIO_CONSOLE_HDR_START_DATA;
+
 		/* No space left in the vq anyway */
 		if (!ret)
 			break;
@@ -372,6 +387,129 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
 	return out_offset;
 }
 
+/* The condition that must be true for polling to end */
+static bool wait_is_over(struct port *port)
+{
+	return !list_empty(&port->readbuf_head) || !port->host_connected;
+}
+
+static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
+			      size_t count, loff_t *offp)
+{
+	struct port *port;
+	ssize_t ret;
+
+	port = filp->private_data;
+
+	if (list_empty(&port->readbuf_head)) {
+		/*
+		 * If nothing's connected on the host just return 0 in
+		 * case of list_empty; this tells the userspace app
+		 * that there's no connection
+		 */
+		if (!port->host_connected)
+			return 0;
+		if (filp->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(port->waitqueue,
+					       wait_is_over(port));
+		if (ret < 0)
+			return ret;
+	}
+	/*
+	 * We could've received a disconnection message while we were
+	 * waiting for more data.
+	 *
+	 * This check is not clubbed in the if() statement above as we
+	 * might receive some data as well as the host could get
+	 * disconnected after we got woken up from our wait.  So we
+	 * really want to give off whatever data we have and only then
+	 * check for host_connected.
+	 */
+	if (list_empty(&port->readbuf_head) && !port->host_connected)
+		return 0;
+
+	return fill_readbuf(port, ubuf, count, true);
+}
+
+static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
+			       size_t count, loff_t *offp)
+{
+	struct port *port;
+
+	port = filp->private_data;
+
+	return send_buf(port, ubuf, count, 0, true);
+}
+
+static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
+{
+	struct port *port;
+	unsigned int ret;
+
+	port = filp->private_data;
+	poll_wait(filp, &port->waitqueue, wait);
+
+	ret = 0;
+	if (!list_empty(&port->readbuf_head))
+		ret |= POLLIN | POLLRDNORM;
+	if (port->host_connected)
+		ret |= POLLOUT;
+	if (!port->host_connected)
+		ret |= POLLHUP;
+
+	return ret;
+}
+
+static int port_fops_release(struct inode *inode, struct file *filp)
+{
+	struct port *port;
+
+	port = filp->private_data;
+
+	/* Notify host of port being closed */
+	send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+
+	return 0;
+}
+
+static int port_fops_open(struct inode *inode, struct file *filp)
+{
+	struct cdev *cdev = inode->i_cdev;
+	struct port *port;
+
+	port = container_of(cdev, struct port, cdev);
+	filp->private_data = port;
+
+	/*
+	 * Don't allow opening of console port devices -- that's done
+	 * via /dev/hvc
+	 */
+	if (is_console_port(port))
+		return -ENXIO;
+
+	/* Notify host of port being opened */
+	send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
+
+	return 0;
+}
+
+/*
+ * The file operations that we support: programs in the guest can open
+ * a console device, read from it, write to it, poll for data and
+ * close it. The devices are at
+ *   /dev/vport<device number>p<port number>
+ */
+static const struct file_operations port_fops = {
+	.owner = THIS_MODULE,
+	.open  = port_fops_open,
+	.read  = port_fops_read,
+	.write = port_fops_write,
+	.poll  = port_fops_poll,
+	.release = port_fops_release,
+};
+
 /*
  * The put_chars() callback is pretty straightforward.
  *
@@ -618,6 +756,10 @@ static void handle_control_message(struct port *port, struct port_buffer *buf)
 		port->cons.hvc->irq_requested = 1;
 		resize_console(port);
 		break;
+	case VIRTIO_CONSOLE_PORT_OPEN:
+		port->host_connected = cpkt->value;
+		wake_up_interruptible(&port->waitqueue);
+		break;
 	}
 }
 
@@ -682,10 +824,18 @@ static void rx_work_handler(struct work_struct *work)
 			kfree(buf);
 			continue;
 		}
+		/*
+		 * We might have missed a connection notification,
+		 * e.g. before the queues were initialised.
+		 */
+		port->host_connected = true;
+
 		spin_lock_irq(&port->readbuf_list_lock);
 		list_add_tail(&buf->list, &port->readbuf_head);
 		spin_unlock_irq(&port->readbuf_list_lock);
 
+		wake_up_interruptible(&port->waitqueue);
+
 		if (is_console_port(port))
 			console_activity |= hvc_poll(port->cons.hvc);
 	}
@@ -751,7 +901,7 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	port->portdev = portdev;
 	port->id = id;
 
-	cdev_init(&port->cdev, NULL);
+	cdev_init(&port->cdev, &port_fops);
 
 	devt = MKDEV(portdev->chr_major, id);
 	err = cdev_add(&port->cdev, devt, 1);
@@ -772,6 +922,7 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	}
 	spin_lock_init(&port->readbuf_list_lock);
 	INIT_LIST_HEAD(&port->readbuf_head);
+	init_waitqueue_head(&port->waitqueue);
 
 	/*
 	 * If we're not using multiport support, this has to be a console port
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index ad9557d..14af53d 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -36,6 +36,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_PORT_READY	0
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
+#define VIRTIO_CONSOLE_PORT_OPEN	3
 
 /*
  * This struct is put at the start of each buffer that gets passed to
@@ -50,6 +51,8 @@ struct virtio_console_header {
 
 /* Messages between host and guest ('flags' field in the header above) */
 #define VIRTIO_CONSOLE_HDR_CONTROL	(1 << 0)
+#define VIRTIO_CONSOLE_HDR_START_DATA	(1 << 1)
+#define VIRTIO_CONSOLE_HDR_END_DATA	(1 << 2)
 
 #ifdef __KERNEL__
 int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
-- 
1.6.2.5

[PATCH 21/28] virtio: console: Ensure only one process can have a port open at a time

[Amit Shah]

Add a guest_connected field that ensures only one process
can have a port open at a time.

This also ensures we don't have a race when we later add support for
dropping buffers when closing the char dev and buffer caching is turned
off for the particular port.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 66875d6..3a305f8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -168,6 +168,9 @@ struct port {
 
 	/* Is the host device open */
 	bool host_connected;
+
+	/* We should allow only one process to open a port */
+	bool guest_connected;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -471,6 +474,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	/* Notify host of port being closed */
 	send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
+	port->guest_connected = false;
+
 	return 0;
 }
 
@@ -489,6 +494,15 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	if (is_console_port(port))
 		return -ENXIO;
 
+	/* Allow only one process to open a particular port at a time */
+	spin_lock_irq(&port->readbuf_list_lock);
+	if (port->guest_connected) {
+		spin_unlock_irq(&port->readbuf_list_lock);
+		return -EMFILE;
+	}
+	port->guest_connected = true;
+	spin_unlock_irq(&port->readbuf_list_lock);
+
 	/* Notify host of port being opened */
 	send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
@@ -656,6 +670,7 @@ int init_port_console(struct port *port)
 	pdrvdata.next_vtermno++;
 	list_add_tail(&port->cons.list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
+	port->guest_connected = true;
 
 	return 0;
 }
-- 
1.6.2.5

[PATCH 22/28] virtio: console: Register with sysfs and create a 'name' attribute for ports

[Amit Shah]

The host can set a name for ports so that they're easily discoverable
instead of going by the /dev/vportNpn naming. This attribute will be
placed in /sys/class/vportdevN/vportNpn/name. udev scripts can then
create symlinks to the port using the name.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3a305f8..d0bdc18 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -163,6 +163,9 @@ struct port {
 	 */
 	struct console cons;
 
+	/* The 'name' of the port that we expose via sysfs properties */
+	char *name;
+
 	/* The 'id' to identify the port with the Host */
 	u32 id;
 
@@ -745,10 +748,14 @@ static void fill_receive_queue(struct ports_device *portdev)
 	spin_unlock(&portdev->read_list_lock);
 }
 
+static struct attribute_group port_attribute_group;
+
 /* Any private messages that the Host and Guest want to share */
 static void handle_control_message(struct port *port, struct port_buffer *buf)
 {
 	struct virtio_console_control *cpkt;
+	size_t name_size;
+	int err;
 
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
 
@@ -775,6 +782,35 @@ static void handle_control_message(struct port *port, struct port_buffer *buf)
 		port->host_connected = cpkt->value;
 		wake_up_interruptible(&port->waitqueue);
 		break;
+	case VIRTIO_CONSOLE_PORT_NAME:
+		/*
+		 * Skip the size of the header and the cpkt to get the size
+		 * of the name that was sent
+		 */
+		name_size = buf->len - buf->offset - sizeof(*cpkt) + 1;
+
+		port->name = kmalloc(name_size, GFP_KERNEL);
+		if (!port->name) {
+			dev_err(port->dev,
+				"not enough space to store port name\n");
+			break;
+		}
+		strncpy(port->name, buf->buf + buf->offset + sizeof(*cpkt),
+			name_size - 1);
+		port->name[name_size - 1] = 0;
+
+		/*
+		 * Since we only have one sysfs attribute, 'name',
+		 * create it only if we have a name for the port.
+		 */
+		err = sysfs_create_group(&port->dev->kobj,
+					 &port_attribute_group);
+		if (err)
+			dev_err(port->dev,
+				"error creating sysfs device attributes, ret = %d\n",
+				err);
+
+		break;
 	}
 }
 
@@ -902,6 +938,28 @@ static void tx_intr(struct virtqueue *vq)
 	spin_unlock_irqrestore(&portdev->write_list_lock, flags);
 }
 
+static ssize_t show_port_name(struct device *dev,
+			      struct device_attribute *attr, char *buffer)
+{
+	struct port *port;
+
+	port = dev_get_drvdata(dev);
+
+	return sprintf(buffer, "%s\n", port->name);
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_port_name, NULL);
+
+static struct attribute *port_sysfs_entries[] = {
+	&dev_attr_name.attr,
+	NULL
+};
+
+static struct attribute_group port_attribute_group = {
+	.name = NULL,		/* put in device directory */
+	.attrs = port_sysfs_entries,
+};
+
 static int __devinit add_port(struct ports_device *portdev, u32 id)
 {
 	struct port *port;
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 14af53d..d65fdb5 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -37,6 +37,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_CONSOLE_PORT	1
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
+#define VIRTIO_CONSOLE_PORT_NAME	4
 
 /*
  * This struct is put at the start of each buffer that gets passed to
-- 
1.6.2.5

[PATCH 23/28] virtio: console: Add throttling support to prevent flooding ports

[Amit Shah]

Rogue processes on guests or hosts could pump in data
and cause an OOM condition. Add support for throttling
so that a limit to the number of outstanding buffers
can be specified.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d0bdc18..fe8f14f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -169,11 +169,32 @@ struct port {
 	/* The 'id' to identify the port with the Host */
 	u32 id;
 
+	/*
+	 * The Host can set some limit to the number of outstanding
+	 * buffers we can have in the readbuf_head list before we
+	 * should ask the Host to stop any more data from coming in.
+	 * This is to not allow a rogue process on the Host to OOM the
+	 * entire guest.
+	 */
+	u32 buffer_limit;
+
+	/*
+	 * The actual number of buffers we have pending in the
+	 * readbuf_head list
+	 */
+	u32 nr_buffers;
+
 	/* Is the host device open */
 	bool host_connected;
 
 	/* We should allow only one process to open a port */
 	bool guest_connected;
+
+	/* Does the Host not want to accept more data currently?  */
+	bool host_throttled;
+
+	/* Have we sent out a throttle request to the Host? */
+	bool guest_throttled;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -383,6 +404,7 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
 		if (buf->len - buf->offset == 0) {
 			spin_lock_irqsave(&port->readbuf_list_lock, flags);
 			list_del(&buf->list);
+			port->nr_buffers--;
 			spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
 			kfree(buf->buf);
 			kfree(buf);
@@ -390,6 +412,10 @@ static ssize_t fill_readbuf(struct port *port, char *out_buf, size_t out_count,
 		if (!out_count)
 			break;
 	}
+	if (port->guest_throttled && port->nr_buffers < port->buffer_limit) {
+		send_control_msg(port, VIRTIO_CONSOLE_THROTTLE_PORT, 0);
+		port->guest_throttled = false;
+	}
 	return out_offset;
 }
 
@@ -446,6 +472,9 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 
 	port = filp->private_data;
 
+	if (port->host_throttled)
+		return -ENOSPC;
+
 	return send_buf(port, ubuf, count, 0, true);
 }
 
@@ -460,7 +489,7 @@ static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
 	ret = 0;
 	if (!list_empty(&port->readbuf_head))
 		ret |= POLLIN | POLLRDNORM;
-	if (port->host_connected)
+	if (port->host_connected && !port->host_throttled)
 		ret |= POLLOUT;
 	if (!port->host_connected)
 		ret |= POLLHUP;
@@ -811,6 +840,35 @@ static void handle_control_message(struct port *port, struct port_buffer *buf)
 				err);
 
 		break;
+	case VIRTIO_CONSOLE_THROTTLE_PORT:
+		/*
+		 * Hosts can govern some policy to disallow rogue
+		 * processes write indefinitely to ports leading to
+		 * OOM situations.  If we receive this message here,
+		 * it means the Host side of the port either reached
+		 * its max. limit to cache data or signal to us that
+		 * the host is ready to accept more data.
+		 */
+		port->host_throttled = cpkt->value;
+		break;
+	case VIRTIO_CONSOLE_BUFFER_LIMIT:
+		/*
+		 * We can ask the Host to stop sending data to us in
+		 * case some rogue process on the Host injects data to
+		 * OOM the entire guest (as unread buffers keep piling
+		 * up in the kernel space). The Host, via this
+		 * message, can tell us the number of Mbytes to
+		 * buffer.
+		 *
+		 * The calculation is easy: we use 4K-sized pages in
+		 * our readbuf_head list. So on each buffer added to
+		 * the list, increment count by 1 (and decrement by 1
+		 * when one gets removed). So we just store the
+		 * (number-in-MB * 1024) / 4 to calculate the number of
+		 * buffers we can have in the list.
+		 */
+		port->buffer_limit = (u32)cpkt->value * 1024 / 4;
+		break;
 	}
 }
 
@@ -881,10 +939,24 @@ static void rx_work_handler(struct work_struct *work)
 		 */
 		port->host_connected = true;
 
+		if (port->guest_throttled) {
+			kfree(buf->buf);
+			kfree(buf);
+			continue;
+		}
 		spin_lock_irq(&port->readbuf_list_lock);
 		list_add_tail(&buf->list, &port->readbuf_head);
+		port->nr_buffers++;
 		spin_unlock_irq(&port->readbuf_list_lock);
-
+		/*
+		 * If we reached the throttle level, send out a
+		 * message to the host to ask it to stop sending us
+		 * any more data.
+		 */
+		if (port->nr_buffers >= port->buffer_limit) {
+			send_control_msg(port, VIRTIO_CONSOLE_THROTTLE_PORT, 1);
+			port->guest_throttled = true;
+		}
 		wake_up_interruptible(&port->waitqueue);
 
 		if (is_console_port(port))
@@ -974,6 +1046,8 @@ static int __devinit add_port(struct ports_device *portdev, u32 id)
 	port->portdev = portdev;
 	port->id = id;
 
+	port->buffer_limit = ~(u32)0;
+
 	cdev_init(&port->cdev, &port_fops);
 
 	devt = MKDEV(portdev->chr_major, id);
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index d65fdb5..2aa2b42 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -38,6 +38,8 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_RESIZE		2
 #define VIRTIO_CONSOLE_PORT_OPEN	3
 #define VIRTIO_CONSOLE_PORT_NAME	4
+#define VIRTIO_CONSOLE_THROTTLE_PORT	5
+#define VIRTIO_CONSOLE_BUFFER_LIMIT	6
 
 /*
  * This struct is put at the start of each buffer that gets passed to
-- 
1.6.2.5

[PATCH 24/28] virtio: console: Add option to remove cached buffers on port close

[Amit Shah]

If caching of buffers upon closing a port is not desired,
delete all the buffers queued up for the port upon port
close.

This only applies to generic ports; console ports aren't
opened / closed by the chardev interface.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fe8f14f..8fac9eb 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -195,6 +195,9 @@ struct port {
 
 	/* Have we sent out a throttle request to the Host? */
 	bool guest_throttled;
+
+	/* Does the port want to cache data when disconnected? */
+	bool cache_buffers;
 };
 
 /* This is the very early arch-specified put chars function. */
@@ -506,8 +509,25 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	/* Notify host of port being closed */
 	send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
+	spin_lock_irq(&port->readbuf_list_lock);
 	port->guest_connected = false;
 
+	/*
+	 * If the port doesn't want to cache buffers while closed,
+	 * flush the unread buffers queue
+	 */
+	if (!port->cache_buffers) {
+		struct port_buffer *buf, *buf2;
+
+		list_for_each_entry_safe(buf, buf2, &port->readbuf_head, list) {
+			list_del(&buf->list);
+			kfree(buf->buf);
+			kfree(buf);
+			port->nr_buffers--;
+		}
+	}
+	spin_unlock_irq(&port->readbuf_list_lock);
+
 	return 0;
 }
 
@@ -869,6 +889,9 @@ static void handle_control_message(struct port *port, struct port_buffer *buf)
 		 */
 		port->buffer_limit = (u32)cpkt->value * 1024 / 4;
 		break;
+	case VIRTIO_CONSOLE_CACHE_BUFFERS:
+		port->cache_buffers = cpkt->value;
+		break;
 	}
 }
 
@@ -938,7 +961,19 @@ static void rx_work_handler(struct work_struct *work)
 		 * e.g. before the queues were initialised.
 		 */
 		port->host_connected = true;
-
+		/*
+		 * Don't queue up buffers if caching is disabled and
+		 * port is closed. This condition can be reached when
+		 * a console port is not yet connected (no tty is
+		 * spawned) and the host sends out data to console
+		 * ports. For generic serial ports, the host won't
+		 * send data till the guest is connected.
+		 */
+		if (!port->guest_connected && !port->cache_buffers) {
+			kfree(buf->buf);
+			kfree(buf);
+			continue;
+		}
 		if (port->guest_throttled) {
 			kfree(buf->buf);
 			kfree(buf);
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 2aa2b42..70924a1 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -40,6 +40,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_PORT_NAME	4
 #define VIRTIO_CONSOLE_THROTTLE_PORT	5
 #define VIRTIO_CONSOLE_BUFFER_LIMIT	6
+#define VIRTIO_CONSOLE_CACHE_BUFFERS	7
 
 /*
  * This struct is put at the start of each buffer that gets passed to
-- 
1.6.2.5

[PATCH 25/28] virtio: console: Handle port hot-plug

[Amit Shah]

If the 'nr_ports' variable in the config space is updated to
a higher value, that means new ports have been hotplugged.

Introduce a new workqueue to handle such updates and create
new ports.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8fac9eb..47d84d7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -96,6 +96,7 @@ struct ports_device {
 	 * interrupt
 	 */
 	struct work_struct rx_work;
+	struct work_struct config_work;
 
 	struct list_head ports_head;
 	struct list_head unused_read_head;
@@ -640,11 +641,6 @@ static void resize_console(struct port *port)
 	}
 }
 
-static void virtcons_apply_config(struct virtio_device *vdev)
-{
-	resize_console(find_port_by_vtermno(0));
-}
-
 /* We set the configuration at this point, since we now have a tty */
 static int notifier_add_vio(struct hvc_struct *hp, int data)
 {
@@ -1045,6 +1041,24 @@ static void tx_intr(struct virtqueue *vq)
 	spin_unlock_irqrestore(&portdev->write_list_lock, flags);
 }
 
+static void config_intr(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+
+	portdev = vdev->priv;
+	if (use_multiport(portdev)) {
+		/* Handle port hot-add */
+		schedule_work(&portdev->config_work);
+	}
+	/*
+	 * We'll use this way of resizing only for legacy support. For
+	 * newer userspace (VIRTIO_CONSOLE_F_MULTPORT+), use control
+	 * messages to indicate console size changes so that it can be
+	 * done per-port
+	 */
+	resize_console(find_port_by_id(portdev, 0));
+}
+
 static ssize_t show_port_name(struct device *dev,
 			      struct device_attribute *attr, char *buffer)
 {
@@ -1067,7 +1081,7 @@ static struct attribute_group port_attribute_group = {
 	.attrs = port_sysfs_entries,
 };
 
-static int __devinit add_port(struct ports_device *portdev, u32 id)
+static int add_port(struct ports_device *portdev, u32 id)
 {
 	struct port *port;
 	dev_t devt;
@@ -1143,6 +1157,50 @@ static struct file_operations portdev_fops = {
 };
 
 /*
+ * The workhandler for config-space updates
+ *
+ * This is used when new ports are added
+ */
+static void config_work_handler(struct work_struct *work)
+{
+	struct virtio_console_config virtconconf;
+	struct ports_device *portdev;
+	struct virtio_device *vdev;
+	int err;
+
+	portdev = container_of(work, struct ports_device, config_work);
+
+	vdev = portdev->vdev;
+	vdev->config->get(vdev,
+			  offsetof(struct virtio_console_config, nr_ports),
+			  &virtconconf.nr_ports,
+			  sizeof(virtconconf.nr_ports));
+
+	if (portdev->config.nr_ports == virtconconf.nr_ports) {
+		/*
+		 * Port 0 got hot-added. Since we already did all the other
+		 * initialisation for it, just ask the Host for the name
+		 * if set
+		 */
+		struct port *port;
+
+		port = find_port_by_id(portdev, 0);
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_NAME, 1);
+		return;
+	}
+	if (virtconconf.nr_ports < portdev->config.nr_ports)
+		return;
+
+	/* Hot-add ports */
+	while(virtconconf.nr_ports - portdev->config.nr_ports) {
+		err = add_port(portdev, portdev->config.nr_ports);
+		if (err)
+			break;
+		portdev->config.nr_ports++;
+	}
+}
+
+/*
  * Once we're further in boot, we get probed like any other virtio
  * device.
  *
@@ -1210,6 +1268,7 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 	INIT_LIST_HEAD(&portdev->unused_write_head);
 
 	INIT_WORK(&portdev->rx_work, &rx_work_handler);
+	INIT_WORK(&portdev->config_work, &config_work_handler);
 
 	fill_receive_queue(portdev);
 	alloc_write_bufs(portdev);
@@ -1248,7 +1307,7 @@ 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)
-- 
1.6.2.5

[PATCH 26/28] hvc_console: Export (GPL'ed) hvc_remove

[Amit Shah]

The virtio console, which uses hvc, will get the ability to hot-unplug
ports. Export hvc_remove so that virtio_console can disassociate with
hvc.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/char/hvc_console.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index 299772f..d8dac58 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -833,6 +833,7 @@ int hvc_remove(struct hvc_struct *hp)
 		tty_hangup(tty);
 	return 0;
 }
+EXPORT_SYMBOL_GPL(hvc_remove);
 
 /* Driver initialization: called as soon as someone uses hvc_alloc(). */
 static int hvc_init(void)
-- 
1.6.2.5

[PATCH 27/28] virtio: console: Add ability to hot-unplug ports

[Amit Shah]

Remove port data; deregister from the hvc core if it's a console port.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 47d84d7..8ed57c2 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -795,6 +795,43 @@ static void fill_receive_queue(struct ports_device *portdev)
 
 static struct attribute_group port_attribute_group;
 
+/* Remove port-specific data. */
+static int remove_port_data(struct port *port)
+{
+	struct port_buffer *buf, *buf2;
+
+	spin_lock_irq(&port->portdev->ports_list_lock);
+	list_del(&port->list);
+	spin_unlock_irq(&port->portdev->ports_list_lock);
+
+	if (port->guest_connected)
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+
+	if (is_console_port(port)) {
+		spin_lock_irq(&pdrvdata_lock);
+		list_del(&port->cons.list);
+		spin_unlock_irq(&pdrvdata_lock);
+		hvc_remove(port->cons.hvc);
+	}
+	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
+	device_destroy(pdrvdata.class, port->dev->devt);
+	cdev_del(&port->cdev);
+
+	kfree(port->name);
+
+	/* Remove the buffers in which we have unconsumed data */
+	spin_lock_irq(&port->readbuf_list_lock);
+	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, list) {
+		list_del(&buf->list);
+		kfree(buf->buf);
+		kfree(buf);
+	}
+	spin_unlock_irq(&port->readbuf_list_lock);
+
+	kfree(port);
+	return 0;
+}
+
 /* Any private messages that the Host and Guest want to share */
 static void handle_control_message(struct port *port, struct port_buffer *buf)
 {
@@ -888,6 +925,30 @@ static void handle_control_message(struct port *port, struct port_buffer *buf)
 	case VIRTIO_CONSOLE_CACHE_BUFFERS:
 		port->cache_buffers = cpkt->value;
 		break;
+	case VIRTIO_CONSOLE_PORT_REMOVE:
+		/*
+		 * Hot unplug the port. We don't decrement nr_ports
+		 * since we don't want to deal with extra complexities
+		 * of using the lowest-available port id: We can just
+		 * pick up the nr_ports number as the id and not have
+		 * userspace send it to us. This helps us in two ways:
+		 *
+		 * - We don't need to have a 'port_id' field in the
+		 *   config space when a port is hot-added. This is a
+		 *   good thing as we might queue up multiple hotplug
+		 *   requests issued in our workqueue.
+		 *
+		 * - Another way to deal with this would have been to
+		 *   use a bitmap of the active ports and select the
+		 *   lowest non-active port from that map. That bloats
+		 *   the already tight config space and we would end
+		 *   up artificially limiting the max. number of ports
+		 *   to sizeof(bitmap). Right now we can support 2^32
+		 *   ports (as the port id is stored in a u32 type).
+		 *
+		 */
+		remove_port_data(port);
+		break;
 	}
 }
 
@@ -1152,10 +1213,6 @@ fail:
 	return err;
 }
 
-static struct file_operations portdev_fops = {
-	.owner = THIS_MODULE,
-};
-
 /*
  * The workhandler for config-space updates
  *
@@ -1200,6 +1257,10 @@ static void config_work_handler(struct work_struct *work)
 	}
 }
 
+static struct file_operations portdev_fops = {
+	.owner = THIS_MODULE,
+};
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
diff --git a/include/linux/virtio_console.h b/include/linux/virtio_console.h
index 70924a1..4941c91 100644
--- a/include/linux/virtio_console.h
+++ b/include/linux/virtio_console.h
@@ -41,6 +41,7 @@ struct virtio_console_control {
 #define VIRTIO_CONSOLE_THROTTLE_PORT	5
 #define VIRTIO_CONSOLE_BUFFER_LIMIT	6
 #define VIRTIO_CONSOLE_CACHE_BUFFERS	7
+#define VIRTIO_CONSOLE_PORT_REMOVE	8
 
 /*
  * This struct is put at the start of each buffer that gets passed to
-- 
1.6.2.5

[PATCH 28/28] virtio: console: Add debugfs files for each port to expose debug info

[Amit Shah]

This is helpful in examining ports' state.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ed57c2..1ca9675 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -17,6 +17,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 #include <linux/cdev.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/fs.h>
@@ -43,6 +44,9 @@ struct ports_driver_data {
 	/* Used for registering chardevs */
 	struct class *class;
 
+	/* Used for exporting per-port information to debugfs */
+	struct dentry *debugfs_dir;
+
 	/* Number of devices this driver is handling */
 	unsigned int index;
 
@@ -141,6 +145,9 @@ struct port {
 	/* Pointer to the parent virtio_console device */
 	struct ports_device *portdev;
 
+	/* File in the debugfs directory that exposes this port's information */
+	struct dentry *debugfs_file;
+
 	/* Buffer management */
 	struct list_head readbuf_head;
 
@@ -827,6 +834,7 @@ static int remove_port_data(struct port *port)
 		kfree(buf);
 	}
 	spin_unlock_irq(&port->readbuf_list_lock);
+	debugfs_remove(port->debugfs_file);
 
 	kfree(port);
 	return 0;
@@ -1142,9 +1150,62 @@ static struct attribute_group port_attribute_group = {
 	.attrs = port_sysfs_entries,
 };
 
+static int debugfs_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t debugfs_read(struct file *filp, char __user *ubuf,
+			    size_t count, loff_t *offp)
+{
+	struct port *port;
+	char *buf;
+	ssize_t ret, out_offset, out_count;
+
+	out_count = 1024;
+	buf = kmalloc(out_count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	port = filp->private_data;
+	out_offset = 0;
+	out_offset += snprintf(buf + out_offset, out_count,
+			       "name: %s\n", port->name);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "guest_connected: %d\n", port->guest_connected);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "guest_throttled: %d\n", port->guest_throttled);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "buffer_limit: %u\n", port->buffer_limit);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "nr_buffers: %u\n", port->nr_buffers);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "cache_buffers: %d\n", port->cache_buffers);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "host_connected: %d\n", port->host_connected);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "host_throttled: %d\n", port->host_throttled);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "is_console: %d\n", port->cons.hvc ? 1 : 0);
+	out_offset += snprintf(buf + out_offset, out_count - out_offset,
+			       "console_vtermno: %u\n", port->cons.vtermno);
+
+	ret = simple_read_from_buffer(ubuf, count, offp, buf, out_offset);
+	kfree(buf);
+	return ret;
+}
+
+static const struct file_operations port_debugfs_ops = {
+	.owner = THIS_MODULE,
+	.open  = debugfs_open,
+	.read  = debugfs_read,
+};
+
 static int add_port(struct ports_device *portdev, u32 id)
 {
 	struct port *port;
+	char debugfs_name[8];
 	dev_t devt;
 	int err;
 
@@ -1201,6 +1262,18 @@ static int add_port(struct ports_device *portdev, u32 id)
 	 */
 	send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
 
+	if (pdrvdata.debugfs_dir) {
+		/*
+		 * Finally, create the debugfs file that we can use to
+		 * inspect a port's state at any time
+		 */
+		sprintf(debugfs_name, "vport%up%u",
+			port->portdev->drv_index, id);
+		port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
+							 pdrvdata.debugfs_dir,
+							 port,
+							 &port_debugfs_ops);
+	}
 	return 0;
 
 free_device:
@@ -1381,6 +1454,11 @@ static int __init init(void)
 		pr_err("Error %d creating virtio-ports class\n", err);
 		return err;
 	}
+	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
+	if (!pdrvdata.debugfs_dir) {
+		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
+			   PTR_ERR(pdrvdata.debugfs_dir));
+	}
 	INIT_LIST_HEAD(&pdrvdata.consoles);
 
 	return register_virtio_driver(&virtio_console);
-- 
1.6.2.5

Re: [PATCH 13/28] virtio: console: Create a buffer pool for sending data to host

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:36 pm Amit Shah wrote:
> The old way of sending data to the host was to populate one buffer and
> then wait till the host consumed it before sending the next chunk of
> data.
> 
> Also, there was no support to send large chunks of data.
> 
> We now maintain a per-device list of buffers that are ready to be
> passed on to the host.
> 
> This patch adds support to send big chunks in multiple buffers of
> PAGE_SIZE each to the host.
> 
> When the host consumes the data and signals to us via an interrupt, we
> add the consumed buffer back to our unconsumed list.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c |  159 +++++++++++++++++++++++++++++++++-------
>  1 files changed, 131 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e8dabae..3111e4c 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -67,9 +67,13 @@ struct ports_device {
>  	struct work_struct rx_work;
>  
>  	struct list_head unused_read_head;
> +	struct list_head unused_write_head;
>  
>  	/* To protect the list of unused read buffers and the in_vq */
>  	spinlock_t read_list_lock;
> +
> +	/* To protect the list of unused write buffers and the out_vq */
> +	spinlock_t write_list_lock;
>  };

Let's simplify this a little with a single "buffer_lock" or such in the
previous patch.

> +	if (!in_count)
>  		return 0;

Not necessary: if it happens all we'll do is gratuitously kick the host.

> +	in_offset = 0; /* offset in the user buffer */
> +	spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
> +	while (in_count - in_offset) {

while (in_offset < in_count) seems clearer to me here.

> +		copy_size = min(in_count - in_offset, PAGE_SIZE);

Shouldn't you be using buf->size here?

> +		spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
> +
> +		/*
> +		 * 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(buf->buf, in_buf + in_offset, copy_size);
> +
> +		buf->len = copy_size;
> +		sg_init_one(sg, buf->buf, buf->len);
> +
> +		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);

Dropping the lock here seems gratuitous.

> +/*
> + * This function is only called from the init routine so the spinlock
> + * for the unused_write_head list isn't taken
> + */
> +static void alloc_write_bufs(struct ports_device *portdev)
> +{
> +	struct port_buffer *buf;
> +	int i;
> +
> +	for (i = 0; i < 30; i++) {

30?  And why aren't we allocating these somehow as they're
consumed?

>  	fill_receive_queue(portdev);
> +	alloc_write_bufs(portdev);

What happens if this fails?

Rusty.

Re: [PATCH 13/28] virtio: console: Create a buffer pool for sending data to host

[Amit Shah]

On (Wed) Dec 02 2009 [19:00:35], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:36 pm Amit Shah wrote:
> > The old way of sending data to the host was to populate one buffer and
> > then wait till the host consumed it before sending the next chunk of
> > data.
> > 
> > Also, there was no support to send large chunks of data.
> > 
> > We now maintain a per-device list of buffers that are ready to be
> > passed on to the host.
> > 
> > This patch adds support to send big chunks in multiple buffers of
> > PAGE_SIZE each to the host.
> > 
> > When the host consumes the data and signals to us via an interrupt, we
> > add the consumed buffer back to our unconsumed list.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/virtio_console.c |  159 +++++++++++++++++++++++++++++++++-------
> >  1 files changed, 131 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index e8dabae..3111e4c 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -67,9 +67,13 @@ struct ports_device {
> >  	struct work_struct rx_work;
> >  
> >  	struct list_head unused_read_head;
> > +	struct list_head unused_write_head;
> >  
> >  	/* To protect the list of unused read buffers and the in_vq */
> >  	spinlock_t read_list_lock;
> > +
> > +	/* To protect the list of unused write buffers and the out_vq */
> > +	spinlock_t write_list_lock;
> >  };
> 
> Let's simplify this a little with a single "buffer_lock" or such in the
> previous patch.

You mean a common lock for the in_vq and out_vq?

> > +	if (!in_count)
> >  		return 0;
> 
> Not necessary: if it happens all we'll do is gratuitously kick the host.

Will drop.

> > +	in_offset = 0; /* offset in the user buffer */
> > +	spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
> > +	while (in_count - in_offset) {
> 
> while (in_offset < in_count) seems clearer to me here.

Sure.

> > +		copy_size = min(in_count - in_offset, PAGE_SIZE);
> 
> Shouldn't you be using buf->size here?

You mean introduce a new field in the buf struct and put the size there?
Yes, I'll do that.

> > +		spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
> > +
> > +		/*
> > +		 * 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(buf->buf, in_buf + in_offset, copy_size);
> > +
> > +		buf->len = copy_size;
> > +		sg_init_one(sg, buf->buf, buf->len);
> > +
> > +		spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
> 
> Dropping the lock here seems gratuitous.

In a later patch when we add support for reading from userspace, we'll add
a copy_from_user here, so dropping the lock is done in this patch to
reduce the noise in the diff for easier reviewing.

> > +/*
> > + * This function is only called from the init routine so the spinlock
> > + * for the unused_write_head list isn't taken
> > + */
> > +static void alloc_write_bufs(struct ports_device *portdev)
> > +{
> > +	struct port_buffer *buf;
> > +	int i;
> > +
> > +	for (i = 0; i < 30; i++) {
> 
> 30?

Yeah; 30 is an arbitrary number. Basically just enough to have two
simultaneous 'copy /dev/vport0p2 > blah' run without having to return a
number < requested bytes. (libc uses 32k buffers for transfers.)


>  And why aren't we allocating these somehow as they're
> consumed?

Used-up buffers get added to the list again once host has consumed them
in tx_intr.

> >  	fill_receive_queue(portdev);
> > +	alloc_write_bufs(portdev);
> 
> What happens if this fails?

Hm, if not a single buf got allocated, any write() requests will not
succeed. dev_warn()?

		Amit

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> The console could be flooded with data from the host; handle
> this situation by buffering the data.

All this complexity makes me really wonder if we should just
have the host say the max # ports it will ever use, and just do this
really dumbly.  Yes, it's a limitation, but it'd be much simpler.

> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -65,6 +65,23 @@ struct ports_device {
>  	 * interrupt
>  	 */
>  	struct work_struct rx_work;
> +
> +	struct list_head unused_read_head;

You should name lists after plurals, rather than using "head" which is
an implementation detail.  eg. "queued_inbufs" and below "used_inbufs".

Though Shirly Ma was working on a "destroy_bufs" patch which would avoid
your need for this list at all, AFAICT.

> +		/* Return the number of bytes actually copied */
> +		ret = copy_size;
> +		buf->offset += ret;
> +		out_offset += ret;
> +		out_count -= ret;

We don't actually use ret.

> +		if (buf->len - buf->offset == 0) {

I prefer the simpler "if (buf->offset == buf->len)" myself.

> +			spin_lock_irqsave(&port->readbuf_list_lock, flags);
> +			list_del(&buf->list);
> +			spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
> +			kfree(buf->buf);
> +			kfree(buf);

Does it become cleaner later to have this in a separate function?  Usually
I prefer matching alloc and free fns.

> +static struct port_buffer *get_buf(size_t buf_size)
> +{
> +	struct port_buffer *buf;
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		goto out;
> +	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +	if (!buf->buf) {
> +		kfree(buf);
> +		goto out;

No, that would return non-NULL.  I'd stick with the standard multi-part exit:

	if (!buf)
		goto fail;
	buf->buf = kzalloc(buf_size, GFP_KERNEL);
	if (!buf->buf)
		goto fail_free_buf;
	buf->len = buf_size;
	return buf;

fail_free_buf:
	kfree(buf);
fail:
	return NULL;

Thanks,
Rusty.

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Amit Shah]

On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > The console could be flooded with data from the host; handle
> > this situation by buffering the data.
> 
> All this complexity makes me really wonder if we should just
> have the host say the max # ports it will ever use, and just do this
> really dumbly.  Yes, it's a limitation, but it'd be much simpler.

As in make sure the max nr ports is less than 255 and have per-port vqs?
And then the buffering will be done inside the vqs themselves?

> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -65,6 +65,23 @@ struct ports_device {
> >  	 * interrupt
> >  	 */
> >  	struct work_struct rx_work;
> > +
> > +	struct list_head unused_read_head;
> 
> You should name lists after plurals, rather than using "head" which is
> an implementation detail.  eg. "queued_inbufs" and below "used_inbufs".

OK.

> Though Shirly Ma was working on a "destroy_bufs" patch which would avoid
> your need for this list at all, AFAICT.
> 
> > +		/* Return the number of bytes actually copied */
> > +		ret = copy_size;
> > +		buf->offset += ret;
> > +		out_offset += ret;
> > +		out_count -= ret;
> 
> We don't actually use ret.

In a later patch, when copy_to_user is added, ret will be used. So I
kept it this way to reduce the noise in the diffs later.

> > +		if (buf->len - buf->offset == 0) {
> 
> I prefer the simpler "if (buf->offset == buf->len)" myself.

Will update.

> > +			spin_lock_irqsave(&port->readbuf_list_lock, flags);
> > +			list_del(&buf->list);
> > +			spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
> > +			kfree(buf->buf);
> > +			kfree(buf);
> 
> Does it become cleaner later to have this in a separate function?  Usually
> I prefer matching alloc and free fns.

Adding a function is easy, sure. I should've done that though; something
that got overlooked.

> > +static struct port_buffer *get_buf(size_t buf_size)
> > +{
> > +	struct port_buffer *buf;
> > +
> > +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > +	if (!buf)
> > +		goto out;
> > +	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > +	if (!buf->buf) {
> > +		kfree(buf);
> > +		goto out;
> 
> No, that would return non-NULL.  I'd stick with the standard multi-part exit:
> 
> 	if (!buf)
> 		goto fail;
> 	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> 	if (!buf->buf)
> 		goto fail_free_buf;
> 	buf->len = buf_size;
> 	return buf;
> 
> fail_free_buf:
> 	kfree(buf);
> fail:
> 	return NULL;

Ow, indeed.

Thanks! 

		Amit

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Rusty Russell]

On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > The console could be flooded with data from the host; handle
> > > this situation by buffering the data.
> > 
> > All this complexity makes me really wonder if we should just
> > have the host say the max # ports it will ever use, and just do this
> > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> 
> As in make sure the max nr ports is less than 255 and have per-port vqs?
> And then the buffering will be done inside the vqs themselves?

Well < 128 (two vqs per port).  The config would say (with a feature bit)
how many vq pairs there are.  If we want expect sparse port numbers, it can
also provide an array mapping the vq pairs to the port number (65535 or
something meaning unused).  In practice I suspect 4 or 8 will be plenty
for now.

That makes hotplugging ports pretty simple: change the array, ping
config_changed.

Or we could abandon numbers and just use the names; I don't mind (we still
need a way to figure out what ports are active I guess, maybe a bitmap in
the config?)

But we get the vq buffering and flow control for free with this approach.

Cheers,
Rusty.

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Amit Shah]

On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > The console could be flooded with data from the host; handle
> > > > this situation by buffering the data.
> > > 
> > > All this complexity makes me really wonder if we should just
> > > have the host say the max # ports it will ever use, and just do this
> > > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> > 
> > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > And then the buffering will be done inside the vqs themselves?
> 
> Well < 128 (two vqs per port).  The config would say (with a feature bit)
> how many vq pairs there are.

Sure. This was how the previous versions behaved as well.

>  If we want expect sparse port numbers, it can
> also provide an array mapping the vq pairs to the port number (65535 or
> something meaning unused).  In practice I suspect 4 or 8 will be plenty
> for now.
> 
> That makes hotplugging ports pretty simple: change the array, ping
> config_changed.

The very early versions of this patch used this approach: a nr_ports u32
and a bitmap...

> Or we could abandon numbers and just use the names; I don't mind (we still
> need a way to figure out what ports are active I guess, maybe a bitmap in
> the config?)

... but we dropped that approach and are now just using the 'names' as
a means of identifying ports. Currently, there's only a 'nr_ports' entry
in the config. For hotplug, the entry is bumped by the host and the
guest adds the new ports.

That's definitely simpler than maintaining the bitmap, and also we don't
associate any special meaning to the port numbers themselves, which is a
big plus. (Hot plug after hot unplug can be made to work with a control
message as well, but that support can come in later.)

Looks like we're just going back to what this patch looked like when it
was sent out first.

> But we get the vq buffering and flow control for free with this approach.

I'll see if I can send out this spin today.

		Amit

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Amit Shah]

On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > The console could be flooded with data from the host; handle
> > > > > this situation by buffering the data.
> > > > 
> > > > All this complexity makes me really wonder if we should just
> > > > have the host say the max # ports it will ever use, and just do this
> > > > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> > > 
> > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > And then the buffering will be done inside the vqs themselves?
> > 
> > Well < 128 (two vqs per port).  The config would say (with a feature bit)
> > how many vq pairs there are.
> 
> Sure. This was how the previous versions behaved as well.

I forgot one detail:

http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html

Some API changes are needed to pre-declare the number of vqs and the
selectively enable them as ports get added.

How I think this could work is:

<device drv>

probe:
- get max_nr_ports from config_get
- declare the intent to use max_nr_ports * 2 vqs with callbacks
  associated with half of them (so this allows us to have 512 vqs with 256
  callbacks with the x86 MSI limit.

<virtio_pci>:
request_msix_vectors for all max_nr_ports vectors

new functions to enable / disable vqs

		Amit

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Rusty Russell]

On Fri, 4 Dec 2009 09:32:46 pm Amit Shah wrote:
> On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> > On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > > The console could be flooded with data from the host; handle
> > > > > > this situation by buffering the data.
> > > > > 
> > > > > All this complexity makes me really wonder if we should just
> > > > > have the host say the max # ports it will ever use, and just do this
> > > > > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> > > > 
> > > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > > And then the buffering will be done inside the vqs themselves?
> > > 
> > > Well < 128 (two vqs per port).  The config would say (with a feature bit)
> > > how many vq pairs there are.
> > 
> > Sure. This was how the previous versions behaved as well.
> 
> I forgot one detail:
> 
> http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html
> 
> Some API changes are needed to pre-declare the number of vqs and the
> selectively enable them as ports get added.

Couldn't we make it that all vqs *exist*, they're just unused unless the
bitmap (or whatever) indicates?

Thanks,
Rusty.

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Amit Shah]

On (Tue) Dec 08 2009 [11:46:10], Rusty Russell wrote:
> On Fri, 4 Dec 2009 09:32:46 pm Amit Shah wrote:
> > On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> > > On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > > > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > > > The console could be flooded with data from the host; handle
> > > > > > > this situation by buffering the data.
> > > > > > 
> > > > > > All this complexity makes me really wonder if we should just
> > > > > > have the host say the max # ports it will ever use, and just do this
> > > > > > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> > > > > 
> > > > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > > > And then the buffering will be done inside the vqs themselves?
> > > > 
> > > > Well < 128 (two vqs per port).  The config would say (with a feature bit)
> > > > how many vq pairs there are.
> > > 
> > > Sure. This was how the previous versions behaved as well.
> > 
> > I forgot one detail:
> > 
> > http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html
> > 
> > Some API changes are needed to pre-declare the number of vqs and the
> > selectively enable them as ports get added.
> 
> Couldn't we make it that all vqs *exist*, they're just unused unless the
> bitmap (or whatever) indicates?

Yes, but the current interface makes that a bit difficult: find_vqs
needs the entire array for the callbacks. So if instead of find_vqs, we
could have two functions,

 ret =  init_vqs(vdev, nr_vqs);
 for (i = 0; i < nr_vqs; i += 2)
	enable_vqs(vdev, i, 2, callbacks, names);

this would be simplified and we can also then enabling and disabling vqs
as ports get hot plugged / unplugged.

		Amit

Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host

[Rusty Russell]

On Tue, 15 Dec 2009 09:15:19 pm Amit Shah wrote:
> On (Tue) Dec 08 2009 [11:46:10], Rusty Russell wrote:
> > On Fri, 4 Dec 2009 09:32:46 pm Amit Shah wrote:
> > > On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> > > > On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > > > > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > > > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > > > > The console could be flooded with data from the host; handle
> > > > > > > > this situation by buffering the data.
> > > > > > > 
> > > > > > > All this complexity makes me really wonder if we should just
> > > > > > > have the host say the max # ports it will ever use, and just do this
> > > > > > > really dumbly.  Yes, it's a limitation, but it'd be much simpler.
> > > > > > 
> > > > > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > > > > And then the buffering will be done inside the vqs themselves?
> > > > > 
> > > > > Well < 128 (two vqs per port).  The config would say (with a feature bit)
> > > > > how many vq pairs there are.
> > > > 
> > > > Sure. This was how the previous versions behaved as well.
> > > 
> > > I forgot one detail:
> > > 
> > > http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html
> > > 
> > > Some API changes are needed to pre-declare the number of vqs and the
> > > selectively enable them as ports get added.
> > 
> > Couldn't we make it that all vqs *exist*, they're just unused unless the
> > bitmap (or whatever) indicates?
> 
> Yes, but the current interface makes that a bit difficult: find_vqs
> needs the entire array for the callbacks. So if instead of find_vqs, we
> could have two functions,
> 
>  ret =  init_vqs(vdev, nr_vqs);
>  for (i = 0; i < nr_vqs; i += 2)
> 	enable_vqs(vdev, i, 2, callbacks, names);
> 
> this would be simplified and we can also then enabling and disabling vqs
> as ports get hot plugged / unplugged.

No, I was thinking we find_vqs them all.  We just don't put any buffers in
the unused ones.

Cheers,
Rusty.

Re: [PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:32 pm Amit Shah wrote:
> @@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device *dev)
>  		dev->config->get(dev,
>  				 offsetof(struct virtio_console_config, rows),
>  				 &ws.ws_row, sizeof(u16));
> -		hvc_resize(port->hvc, ws);
> +		/* This is the pre-multiport style: we use control messages
> +		 * these days which specify the port.  So this means port 0. */
> +		hvc_resize(find_port_by_vtermno(0)->hvc, ws);
>  	}

We end up doing this in a couple of places; perhaps we should have something
like:
	/*
	 * Before multiple console support, we always had a single console.
	 * Code paths without those features can use this.
	 */
	static struct port *old_style_unique_console(void)
	{
		return find_port_by_vtermno(0);
	}

>  	err = -ENOMEM;
> -	port = add_port(pdrvdata.next_vtermno);
> +	port = kzalloc(sizeof *port, GFP_KERNEL);
>  	if (!port)
>  		goto fail;

I still dislike kzalloc.  I think I've said this before :)

Thanks,
Rusty.

Re: [PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

On (Mon) Nov 30 2009 [12:39:52], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:32 pm Amit Shah wrote:
> > @@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device *dev)
> >  		dev->config->get(dev,
> >  				 offsetof(struct virtio_console_config, rows),
> >  				 &ws.ws_row, sizeof(u16));
> > -		hvc_resize(port->hvc, ws);
> > +		/* This is the pre-multiport style: we use control messages
> > +		 * these days which specify the port.  So this means port 0. */
> > +		hvc_resize(find_port_by_vtermno(0)->hvc, ws);
> >  	}
> 
> We end up doing this in a couple of places; perhaps we should have something
> like:
> 	/*
> 	 * Before multiple console support, we always had a single console.
> 	 * Code paths without those features can use this.
> 	 */
> 	static struct port *old_style_unique_console(void)
> 	{
> 		return find_port_by_vtermno(0);
> 	}

(Again) in a later patch, I rename the function to resize_console() and
call that one from the config interrupt as well as from the
hvc notifier.

> >  	err = -ENOMEM;
> > -	port = add_port(pdrvdata.next_vtermno);
> > +	port = kzalloc(sizeof *port, GFP_KERNEL);
> >  	if (!port)
> >  		goto fail;
> 
> I still dislike kzalloc.  I think I've said this before :)

I remember seeing it in another thread I think (or was it in a blog post
about you liking the ability to run it through valgrind?).

The init function becomes a bit longer in that case, since we'll have to
init all the known state. But I'm fine with that.

However, there's one exception: when allocating buffers, I'd prefer to
do a kzalloc() since the buffers are sent across the guest/host boundary
and we don't want to leak data in either direction.

(Just mentioning this again since it's the last comment: I'll respin the
series once you have a chance to review the other patches too.)

Also: if you think the approach is OK and give an ACK for the approach,
I can also proceed in parallel with the host bits for QEMU.

Thanks, Rusty!

		Amit

Re: [PATCH 08/28] virtio: console: remove global var

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:31 pm Amit Shah wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
> 
> Now we can use an allocation function to remove our global console variable.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c |   70 +++++++++++++++++++++++++++-------------
>  1 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index c133bf6..98a5249 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -32,6 +32,18 @@
>   * across multiple devices and multiple ports per device.
>   */
>  struct ports_driver_data {
> +	/*
> +	 * This is used to keep track of the number of hvc consoles
> +	 * spawned by this driver.  This number is given as the first
> +	 * argument to hvc_alloc().  To correctly map an initial
> +	 * console spawned via hvc_instantiate to the console being
> +	 * hooked up via hvc_alloc, we need to pass the same vtermno.
> +	 *
> +	 * We also just assume the first console being initialised was
> +	 * the first one that got used as the initial console.
> +	 */
> +	unsigned int next_vtermno;

Let's just make this a global?

> +static struct port *__devinit add_port(u32 vtermno)
> +{
> +	struct port *port;
> +
> +	port = kzalloc(sizeof *port, GFP_KERNEL);
> +	if (!port)
> +		return NULL;
> +
> +	port->used_len = port->offset = 0;
> +	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!port->inbuf) {
> +		kfree(port);
> +		return NULL;
> +	}
> +	port->hvc = NULL;
> +	port->vtermno = vtermno;
> +	return port;
> +}

Rename this to add_console_port(), and split off the core part which
does ->port setup into "int setup_port(struct port *)" for later re-use?

> +static void free_port(struct port *port)
> +{
> +	kfree(port->inbuf);
> +	kfree(port);
> +}

Similarly, free_console_port/free_port?

> +	err = -ENOMEM;
> +	port = add_port(pdrvdata.next_vtermno);
> +	if (!port)
> +		goto fail;

I realize other coders do this pre-init, and it saves a line, but it also
means that you don't get a warning about err being uninitialized if it doesn't
get set correctly later on.

So I prefer:
	port = add...
	if (!port) {
		err = -ENOMEM;
		goto fail;
	}

Thanks,
Rusty.

Re: [PATCH 08/28] virtio: console: remove global var

[Amit Shah]

On (Mon) Nov 30 2009 [12:27:21], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:31 pm Amit Shah wrote:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > Now we can use an allocation function to remove our global console variable.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/virtio_console.c |   70 +++++++++++++++++++++++++++-------------
> >  1 files changed, 47 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index c133bf6..98a5249 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -32,6 +32,18 @@
> >   * across multiple devices and multiple ports per device.
> >   */
> >  struct ports_driver_data {
> > +	/*
> > +	 * This is used to keep track of the number of hvc consoles
> > +	 * spawned by this driver.  This number is given as the first
> > +	 * argument to hvc_alloc().  To correctly map an initial
> > +	 * console spawned via hvc_instantiate to the console being
> > +	 * hooked up via hvc_alloc, we need to pass the same vtermno.
> > +	 *
> > +	 * We also just assume the first console being initialised was
> > +	 * the first one that got used as the initial console.
> > +	 */
> > +	unsigned int next_vtermno;
> 
> Let's just make this a global?

There will be a few more globals introduced in the next few patches, so
I just put all the globals in a new struct to keep them in one place.

> > +static struct port *__devinit add_port(u32 vtermno)
> > +{
> > +	struct port *port;
> > +
> > +	port = kzalloc(sizeof *port, GFP_KERNEL);
> > +	if (!port)
> > +		return NULL;
> > +
> > +	port->used_len = port->offset = 0;
> > +	port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!port->inbuf) {
> > +		kfree(port);
> > +		return NULL;
> > +	}
> > +	port->hvc = NULL;
> > +	port->vtermno = vtermno;
> > +	return port;
> > +}
> 
> Rename this to add_console_port(),

I do that later (in patch 15), splitting this off into
init_console_port().

> and split off the core part which
> does ->port setup into "int setup_port(struct port *)" for later re-use?

yeah; this function then becomes the core port init routine in patch 15.

> > +static void free_port(struct port *port)
> > +{
> > +	kfree(port->inbuf);
> > +	kfree(port);
> > +}
> 
> Similarly, free_console_port/free_port?

This comes with port hot unplug support later in the series. This part
is again what I left intact from your series.

> > +	err = -ENOMEM;
> > +	port = add_port(pdrvdata.next_vtermno);
> > +	if (!port)
> > +		goto fail;
> 
> I realize other coders do this pre-init, and it saves a line, but it also
> means that you don't get a warning about err being uninitialized if it doesn't
> get set correctly later on.
> 
> So I prefer:
> 	port = add...
> 	if (!port) {
> 		err = -ENOMEM;
> 		goto fail;
> 	}

Sure; I can do that. I just made sure all the style was consistent.

		Amit

Re: [PATCH 07/28] virtio: console: don't assume a single console port.

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:30 pm Amit Shah wrote:
> Keep a list of all ports being used as a console, and provide a lock
> and a lookup function.  The hvc callbacks only give us a vterm number,
> so we need to map this.

OK, I think we can do better than this.

How about we introduce a 'struct console_port', like so:

	/* A port which is used as a console. */
	struct console_port {
		struct port port;

		u32 vtermno;
		struct list_head list;
	};

	static DEFINE_SPINLOCK(console_lock);
	static LIST_HEAD(consoles);

ie. now we have nicely partitioned off the console-specific parts, and
can use ->port and container_of() to get from this to a generic port and
vice-versa.

It will make allocations a little more complex, since we need to allocate
a 'struct console_port' or a 'struct port' depending on what we want, but the
rest should be significantly cleaner.

Thanks,
Rusty.

Re: [PATCH 07/28] virtio: console: don't assume a single console port.

[Amit Shah]

On (Mon) Nov 30 2009 [12:20:02], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:30 pm Amit Shah wrote:
> > Keep a list of all ports being used as a console, and provide a lock
> > and a lookup function.  The hvc callbacks only give us a vterm number,
> > so we need to map this.
> 
> OK, I think we can do better than this.
> 
> How about we introduce a 'struct console_port', like so:
> 
> 	/* A port which is used as a console. */
> 	struct console_port {
> 		struct port port;
> 
> 		u32 vtermno;
> 		struct list_head list;
> 	};
> 
> 	static DEFINE_SPINLOCK(console_lock);
> 	static LIST_HEAD(consoles);

I tried to keep your patches close to the ones you sent -- I make a
change similar to this in patch 14. I can fold the two into one,
obviously.

(I'll wait for comments to the other patches in the series before
sending out a new respin.)

Thanks!
		Amit