[PATCH] VirtioConsole support.

[PATCH] VirtioConsole support.

[Miche Baker-Harvey]

Multiple HVC console terminals enabled.

Serialize device and port open and initialization.  Added a mutex
which gates the handling of control messages in virtio_console.c.
This includes adding and removing ports, and making opened ports be
consoles.  Extended the use of the prvdata spinlock to cover some missed
modifications to prvdata.next_vtermno.

I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
for the driver to be ready, and for the one-time call to hvc_init().  It
had been the case that this was sometimes being called mulitple times, and
partially setup state was being used by the second caller of hvc_alloc().

Make separate struct console* for each new port.  There was a single static
struct console* hvc_console, to be used for early printk.  We aren't doing
early printk, but more importantly, there is no code to multiplex on that
one console.  Multiple virtio_console ports were "sharing" this, which was
disasterous since both the index and the flags for the console are stored
there. The console struct is remembered in the hvc_struct, and it is
deallocated when the hvc_struct is deallocated.

------------------

Konrad, thanks for trying this out on Xen.
This is working in my KVM environment, letting me start multiple
virtio_consoles with getty's on them, but I'm really not sure how
all the console pieces fit together yet.  Feedback is welcome.

Signed-off-by: Miche Baker-Harvey <miche@google.com>
---
 drivers/char/virtio_console.c |   22 +++++++++++++++++++---
 drivers/tty/hvc/hvc_console.c |   39 +++++++++++++++++++++++++++++++++------
 drivers/tty/hvc/hvc_console.h |    1 +
 3 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fb68b12..e819d46 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -24,6 +24,7 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -95,6 +96,11 @@ struct console {
 	u32 vtermno;
 };
 
+/* serialize the handling of control messages, which includes
+ * the initialization of the virtio_consoles.
+ */
+static DEFINE_MUTEX(virtio_console_mutex);
+
 struct port_buffer {
 	char *buf;
 
@@ -979,8 +985,14 @@ int init_port_console(struct port *port)
 	 * 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;
+	spin_lock_irq(&pdrvdata_lock);
+	port->cons.vtermno = pdrvdata.next_vtermno++;
+	spin_unlock_irq(&pdrvdata_lock);
 
+	/*
+	 * xxx Use index 0 for now assuming there is no early HVC, since
+	 * we don't support it.
+	 */
 	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);
@@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
 		return ret;
 	}
 	spin_lock_irq(&pdrvdata_lock);
-	pdrvdata.next_vtermno++;
 	list_add_tail(&port->cons.list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 	port->guest_connected = true;
@@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
 	int err;
 
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
-
 	port = find_port_by_id(portdev, cpkt->id);
 	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
 		/* No valid header at start of buffer.  Drop it. */
@@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
 		return;
 	}
 
+	/*
+	 * These are rare initialization-time events that should be
+	 * serialized.
+	 */
+	mutex_lock(&virtio_console_mutex);
 	switch (cpkt->event) {
 	case VIRTIO_CONSOLE_PORT_ADD:
 		if (port) {
@@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
 		}
 		break;
 	}
+	mutex_unlock(&virtio_console_mutex);
 }
 
 static void control_work_handler(struct work_struct *work)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 7430bc3..03ff6ed 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -29,8 +29,9 @@
 #include <linux/kernel.h>
 #include <linux/kthread.h>
 #include <linux/list.h>
-#include <linux/module.h>
 #include <linux/major.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/sysrq.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
  * list traversal.
  */
 static DEFINE_SPINLOCK(hvc_structs_lock);
+/*
+ * only one task does allocation at a time.
+ */
+static DEFINE_MUTEX(hvc_ports_mutex);
 
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
@@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
 
 	spin_unlock(&hvc_structs_lock);
 
+	kfree(hp->hvc_console);
 	kfree(hp);
 }
 
@@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 			     int outbuf_size)
 {
 	struct hvc_struct *hp;
+	struct console *cp;
 	int i;
 
 	/* We wait until a driver actually comes along */
+	mutex_lock(&hvc_ports_mutex);
 	if (!hvc_driver) {
 		int err = hvc_init();
-		if (err)
+		if (err) {
+			mutex_unlock(&hvc_ports_mutex);
 			return ERR_PTR(err);
+		}
 	}
 
 	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
 			GFP_KERNEL);
-	if (!hp)
+	if (!hp) {
+		mutex_unlock(&hvc_ports_mutex);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	hp->vtermno = vtermno;
 	hp->data = data;
@@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 	kref_init(&hp->kref);
 
 	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
+	/*
+	 * make each console its own struct console.
+	 * No need to do allocation and copy under lock.
+	 */
+	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
+	if (!cp) {
+		kfree(hp);
+		mutex_unlock(&hvc_ports_mutex);
+		return ERR_PTR(-ENOMEM);
+	}
+	memcpy(cp, &hvc_console, sizeof(*cp));
+	hp->hvc_console = cp;
+
 	spin_lock_init(&hp->lock);
 	spin_lock(&hvc_structs_lock);
 
@@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 		i = ++last_hvc;
 
 	hp->index = i;
-	hvc_console.index = i;
+	cp->index = i;
 	vtermnos[i] = vtermno;
 	cons_ops[i] = ops;
 
 	list_add_tail(&(hp->next), &hvc_structs);
 	spin_unlock(&hvc_structs_lock);
-	register_console(&hvc_console);
+	register_console(cp);
+	mutex_unlock(&hvc_ports_mutex);
 
 	return hp;
 }
@@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
 	unsigned long flags;
 	struct tty_struct *tty;
 
-	unregister_console(&hvc_console);
+	BUG_ON(!hp->hvc_console);
+	unregister_console(hp->hvc_console);
 	spin_lock_irqsave(&hp->lock, flags);
 	tty = tty_kref_get(hp->tty);
 
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index c335a14..2d20ab7 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -58,6 +58,7 @@ struct hvc_struct {
 	const struct hv_ops *ops;
 	int irq_requested;
 	int data;
+	struct console *hvc_console;
 	struct winsize ws;
 	struct work_struct tty_resize;
 	struct list_head next;
-- 
1.7.3.1

Re: [PATCH] VirtioConsole support.

[Konrad Rzeszutek Wilk]

On Thu, Oct 27, 2011 at 11:39:53AM -0700, Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.
> 
> Serialize device and port open and initialization.  Added a mutex
> which gates the handling of control messages in virtio_console.c.
> This includes adding and removing ports, and making opened ports be
> consoles.  Extended the use of the prvdata spinlock to cover some missed
> modifications to prvdata.next_vtermno.
> 
> I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
> for the driver to be ready, and for the one-time call to hvc_init().  It
> had been the case that this was sometimes being called mulitple times, and
> partially setup state was being used by the second caller of hvc_alloc().
> 
> Make separate struct console* for each new port.  There was a single static
> struct console* hvc_console, to be used for early printk.  We aren't doing
> early printk, but more importantly, there is no code to multiplex on that
> one console.  Multiple virtio_console ports were "sharing" this, which was
> disasterous since both the index and the flags for the console are stored
> there. The console struct is remembered in the hvc_struct, and it is
> deallocated when the hvc_struct is deallocated.
> 
> ------------------
> 
> Konrad, thanks for trying this out on Xen.

And you can stick

Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


> This is working in my KVM environment, letting me start multiple
> virtio_consoles with getty's on them, but I'm really not sure how
> all the console pieces fit together yet.  Feedback is welcome.
> 
> Signed-off-by: Miche Baker-Harvey <miche@google.com>
> ---
>  drivers/char/virtio_console.c |   22 +++++++++++++++++++---
>  drivers/tty/hvc/hvc_console.c |   39 +++++++++++++++++++++++++++++++++------
>  drivers/tty/hvc/hvc_console.h |    1 +
>  3 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fb68b12..e819d46 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -24,6 +24,7 @@
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -95,6 +96,11 @@ struct console {
>  	u32 vtermno;
>  };
>  
> +/* serialize the handling of control messages, which includes
> + * the initialization of the virtio_consoles.
> + */
> +static DEFINE_MUTEX(virtio_console_mutex);
> +
>  struct port_buffer {
>  	char *buf;
>  
> @@ -979,8 +985,14 @@ int init_port_console(struct port *port)
>  	 * 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;
> +	spin_lock_irq(&pdrvdata_lock);
> +	port->cons.vtermno = pdrvdata.next_vtermno++;
> +	spin_unlock_irq(&pdrvdata_lock);
>  
> +	/*
> +	 * xxx Use index 0 for now assuming there is no early HVC, since
> +	 * we don't support it.
> +	 */
>  	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);
> @@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
>  		return ret;
>  	}
>  	spin_lock_irq(&pdrvdata_lock);
> -	pdrvdata.next_vtermno++;
>  	list_add_tail(&port->cons.list, &pdrvdata.consoles);
>  	spin_unlock_irq(&pdrvdata_lock);
>  	port->guest_connected = true;
> @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
>  	int err;
>  
>  	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
> -
>  	port = find_port_by_id(portdev, cpkt->id);
>  	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
>  		/* No valid header at start of buffer.  Drop it. */
> @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
>  		return;
>  	}
>  
> +	/*
> +	 * These are rare initialization-time events that should be
> +	 * serialized.
> +	 */
> +	mutex_lock(&virtio_console_mutex);
>  	switch (cpkt->event) {
>  	case VIRTIO_CONSOLE_PORT_ADD:
>  		if (port) {
> @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
>  		}
>  		break;
>  	}
> +	mutex_unlock(&virtio_console_mutex);
>  }
>  
>  static void control_work_handler(struct work_struct *work)
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 7430bc3..03ff6ed 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -29,8 +29,9 @@
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
>  #include <linux/list.h>
> -#include <linux/module.h>
>  #include <linux/major.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/sysrq.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> @@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
>   * list traversal.
>   */
>  static DEFINE_SPINLOCK(hvc_structs_lock);
> +/*
> + * only one task does allocation at a time.
> + */
> +static DEFINE_MUTEX(hvc_ports_mutex);
>  
>  /*
>   * This value is used to assign a tty->index value to a hvc_struct based
> @@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
>  
>  	spin_unlock(&hvc_structs_lock);
>  
> +	kfree(hp->hvc_console);
>  	kfree(hp);
>  }
>  
> @@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  			     int outbuf_size)
>  {
>  	struct hvc_struct *hp;
> +	struct console *cp;
>  	int i;
>  
>  	/* We wait until a driver actually comes along */
> +	mutex_lock(&hvc_ports_mutex);
>  	if (!hvc_driver) {
>  		int err = hvc_init();
> -		if (err)
> +		if (err) {
> +			mutex_unlock(&hvc_ports_mutex);
>  			return ERR_PTR(err);
> +		}
>  	}
>  
>  	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
>  			GFP_KERNEL);
> -	if (!hp)
> +	if (!hp) {
> +		mutex_unlock(&hvc_ports_mutex);
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	hp->vtermno = vtermno;
>  	hp->data = data;
> @@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  	kref_init(&hp->kref);
>  
>  	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
> +	/*
> +	 * make each console its own struct console.
> +	 * No need to do allocation and copy under lock.
> +	 */
> +	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> +	if (!cp) {
> +		kfree(hp);
> +		mutex_unlock(&hvc_ports_mutex);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	memcpy(cp, &hvc_console, sizeof(*cp));
> +	hp->hvc_console = cp;
> +
>  	spin_lock_init(&hp->lock);
>  	spin_lock(&hvc_structs_lock);
>  
> @@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  		i = ++last_hvc;
>  
>  	hp->index = i;
> -	hvc_console.index = i;
> +	cp->index = i;
>  	vtermnos[i] = vtermno;
>  	cons_ops[i] = ops;
>  
>  	list_add_tail(&(hp->next), &hvc_structs);
>  	spin_unlock(&hvc_structs_lock);
> -	register_console(&hvc_console);
> +	register_console(cp);
> +	mutex_unlock(&hvc_ports_mutex);
>  
>  	return hp;
>  }
> @@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
>  	unsigned long flags;
>  	struct tty_struct *tty;
>  
> -	unregister_console(&hvc_console);
> +	BUG_ON(!hp->hvc_console);
> +	unregister_console(hp->hvc_console);
>  	spin_lock_irqsave(&hp->lock, flags);
>  	tty = tty_kref_get(hp->tty);
>  
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index c335a14..2d20ab7 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -58,6 +58,7 @@ struct hvc_struct {
>  	const struct hv_ops *ops;
>  	int irq_requested;
>  	int data;
> +	struct console *hvc_console;
>  	struct winsize ws;
>  	struct work_struct tty_resize;
>  	struct list_head next;
> -- 
> 1.7.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] VirtioConsole support.

[Amit Shah]

On (Thu) 27 Oct 2011 [11:39:53], Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.
> 
> Serialize device and port open and initialization.  Added a mutex
> which gates the handling of control messages in virtio_console.c.
> This includes adding and removing ports, and making opened ports be
> consoles.  Extended the use of the prvdata spinlock to cover some missed
> modifications to prvdata.next_vtermno.
> 
> I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
> for the driver to be ready, and for the one-time call to hvc_init().  It
> had been the case that this was sometimes being called mulitple times, and
> partially setup state was being used by the second caller of hvc_alloc().
> 
> Make separate struct console* for each new port.  There was a single static
> struct console* hvc_console, to be used for early printk.  We aren't doing
> early printk, but more importantly, there is no code to multiplex on that
> one console.  Multiple virtio_console ports were "sharing" this, which was
> disasterous since both the index and the flags for the console are stored
> there. The console struct is remembered in the hvc_struct, and it is
> deallocated when the hvc_struct is deallocated.
> 
> ------------------
> 
> Konrad, thanks for trying this out on Xen.
> This is working in my KVM environment, letting me start multiple
> virtio_consoles with getty's on them, but I'm really not sure how
> all the console pieces fit together yet.  Feedback is welcome.
> 
> Signed-off-by: Miche Baker-Harvey <miche@google.com>
> ---
>  drivers/char/virtio_console.c |   22 +++++++++++++++++++---
>  drivers/tty/hvc/hvc_console.c |   39 +++++++++++++++++++++++++++++++++------
>  drivers/tty/hvc/hvc_console.h |    1 +
>  3 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fb68b12..e819d46 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -24,6 +24,7 @@
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -95,6 +96,11 @@ struct console {
>  	u32 vtermno;
>  };
>  
> +/* serialize the handling of control messages, which includes
> + * the initialization of the virtio_consoles.
> + */

Comments in this file are of the type

/*
 * ...
 */

(others below are fine.)

> +static DEFINE_MUTEX(virtio_console_mutex);
> +
>  struct port_buffer {
>  	char *buf;
>  
> @@ -979,8 +985,14 @@ int init_port_console(struct port *port)
>  	 * 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;
> +	spin_lock_irq(&pdrvdata_lock);
> +	port->cons.vtermno = pdrvdata.next_vtermno++;
> +	spin_unlock_irq(&pdrvdata_lock);

This needs to be decremented in case of an error below, or you just
need the locking around the existing usage.

>  
> +	/*
> +	 * xxx Use index 0 for now assuming there is no early HVC, since
> +	 * we don't support it.
> +	 */

correction: x86 doesn't have early HVC console support yet, but s390
(and likely ppc) use this today.

I think it's safe to use 0 for the early console, it's not likely we
have anything else using hvc consoles till this point.

>  	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);
> @@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
>  		return ret;
>  	}
>  	spin_lock_irq(&pdrvdata_lock);
> -	pdrvdata.next_vtermno++;
>  	list_add_tail(&port->cons.list, &pdrvdata.consoles);
>  	spin_unlock_irq(&pdrvdata_lock);
>  	port->guest_connected = true;
> @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
>  	int err;
>  
>  	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
> -

unrelated, please drop.

>  	port = find_port_by_id(portdev, cpkt->id);
>  	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
>  		/* No valid header at start of buffer.  Drop it. */
> @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
>  		return;
>  	}
>  
> +	/*
> +	 * These are rare initialization-time events that should be
> +	 * serialized.
> +	 */
> +	mutex_lock(&virtio_console_mutex);
>  	switch (cpkt->event) {
>  	case VIRTIO_CONSOLE_PORT_ADD:
>  		if (port) {
> @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
>  		}
>  		break;
>  	}
> +	mutex_unlock(&virtio_console_mutex);

Not really rare init-time events; ports can be hot-plugged.

BTW what does serialising just these add events gain us?  I think if
this is necessary, the mutex might be necessary for other operations
too.

I'll leave the review of hvc_console bits to others.

Overall, I think you need to split this patch up into 3-4 parts, one
fixing the spinlock usage around next_vtermno above, another adding
the mutex, and the others dealing with hvc_console.c.

Thanks,
		Amit

[PATCH] VirtioConsole support.

[Miche Baker-Harvey]

Multiple HVC console terminals enabled.

Serialize device and port open and initialization.  Added a mutex
which gates the handling of control messages in virtio_console.c.
This includes adding and removing ports, and making opened ports be
consoles.  Extended the use of the prvdata spinlock to cover some missed
modifications to prvdata.next_vtermno.

I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
for the driver to be ready, and for the one-time call to hvc_init().  It
had been the case that this was sometimes being called mulitple times, and
partially setup state was being used by the second caller of hvc_alloc().

Make separate struct console* for each new port.  There was a single static
struct console* hvc_console, to be used for early printk.  We aren't doing
early printk, but more importantly, there is no code to multiplex on that
one console.  Multiple virtio_console ports were "sharing" this, which was
disasterous since both the index and the flags for the console are stored
there. The console struct is remembered in the hvc_struct, and it is
deallocated when the hvc_struct is deallocated.

Signed-off-by: Miche Baker-Harvey <miche@google.com>
Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/char/virtio_console.c |   22 +++++++++++++++++++---
 drivers/tty/hvc/hvc_console.c |   39 +++++++++++++++++++++++++++++++++------
 drivers/tty/hvc/hvc_console.h |    1 +
 3 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fb68b12..e819d46 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -24,6 +24,7 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -95,6 +96,11 @@ struct console {
 	u32 vtermno;
 };
 
+/* serialize the handling of control messages, which includes
+ * the initialization of the virtio_consoles.
+ */
+static DEFINE_MUTEX(virtio_console_mutex);
+
 struct port_buffer {
 	char *buf;
 
@@ -979,8 +985,14 @@ int init_port_console(struct port *port)
 	 * 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;
+	spin_lock_irq(&pdrvdata_lock);
+	port->cons.vtermno = pdrvdata.next_vtermno++;
+	spin_unlock_irq(&pdrvdata_lock);
 
+	/*
+	 * xxx Use index 0 for now assuming there is no early HVC, since
+	 * we don't support it.
+	 */
 	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);
@@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
 		return ret;
 	}
 	spin_lock_irq(&pdrvdata_lock);
-	pdrvdata.next_vtermno++;
 	list_add_tail(&port->cons.list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 	port->guest_connected = true;
@@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
 	int err;
 
 	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
-
 	port = find_port_by_id(portdev, cpkt->id);
 	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
 		/* No valid header at start of buffer.  Drop it. */
@@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
 		return;
 	}
 
+	/*
+	 * These are rare initialization-time events that should be
+	 * serialized.
+	 */
+	mutex_lock(&virtio_console_mutex);
 	switch (cpkt->event) {
 	case VIRTIO_CONSOLE_PORT_ADD:
 		if (port) {
@@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
 		}
 		break;
 	}
+	mutex_unlock(&virtio_console_mutex);
 }
 
 static void control_work_handler(struct work_struct *work)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 7430bc3..03ff6ed 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -29,8 +29,9 @@
 #include <linux/kernel.h>
 #include <linux/kthread.h>
 #include <linux/list.h>
-#include <linux/module.h>
 #include <linux/major.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/sysrq.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
  * list traversal.
  */
 static DEFINE_SPINLOCK(hvc_structs_lock);
+/*
+ * only one task does allocation at a time.
+ */
+static DEFINE_MUTEX(hvc_ports_mutex);
 
 /*
  * This value is used to assign a tty->index value to a hvc_struct based
@@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
 
 	spin_unlock(&hvc_structs_lock);
 
+	kfree(hp->hvc_console);
 	kfree(hp);
 }
 
@@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 			     int outbuf_size)
 {
 	struct hvc_struct *hp;
+	struct console *cp;
 	int i;
 
 	/* We wait until a driver actually comes along */
+	mutex_lock(&hvc_ports_mutex);
 	if (!hvc_driver) {
 		int err = hvc_init();
-		if (err)
+		if (err) {
+			mutex_unlock(&hvc_ports_mutex);
 			return ERR_PTR(err);
+		}
 	}
 
 	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
 			GFP_KERNEL);
-	if (!hp)
+	if (!hp) {
+		mutex_unlock(&hvc_ports_mutex);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	hp->vtermno = vtermno;
 	hp->data = data;
@@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 	kref_init(&hp->kref);
 
 	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
+	/*
+	 * make each console its own struct console.
+	 * No need to do allocation and copy under lock.
+	 */
+	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
+	if (!cp) {
+		kfree(hp);
+		mutex_unlock(&hvc_ports_mutex);
+		return ERR_PTR(-ENOMEM);
+	}
+	memcpy(cp, &hvc_console, sizeof(*cp));
+	hp->hvc_console = cp;
+
 	spin_lock_init(&hp->lock);
 	spin_lock(&hvc_structs_lock);
 
@@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 		i = ++last_hvc;
 
 	hp->index = i;
-	hvc_console.index = i;
+	cp->index = i;
 	vtermnos[i] = vtermno;
 	cons_ops[i] = ops;
 
 	list_add_tail(&(hp->next), &hvc_structs);
 	spin_unlock(&hvc_structs_lock);
-	register_console(&hvc_console);
+	register_console(cp);
+	mutex_unlock(&hvc_ports_mutex);
 
 	return hp;
 }
@@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
 	unsigned long flags;
 	struct tty_struct *tty;
 
-	unregister_console(&hvc_console);
+	BUG_ON(!hp->hvc_console);
+	unregister_console(hp->hvc_console);
 	spin_lock_irqsave(&hp->lock, flags);
 	tty = tty_kref_get(hp->tty);
 
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index c335a14..2d20ab7 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -58,6 +58,7 @@ struct hvc_struct {
 	const struct hv_ops *ops;
 	int irq_requested;
 	int data;
+	struct console *hvc_console;
 	struct winsize ws;
 	struct work_struct tty_resize;
 	struct list_head next;
-- 
1.7.3.1

Re: [PATCH] VirtioConsole support.

[Konrad Rzeszutek Wilk]

On Thu, Oct 27, 2011 at 02:43:22PM -0700, Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.

Miche,

You might want to flesh out the description a bit. Perhaps include
such details as : "fixes the infinite loop that git commit XX
caused". Perhaps include some of the serial console output.

Maybe also change the title - the patch name (which is what shows up
if you do 'git log --oneline') is 'VirtioConsole support.' which is
not very informative.

Oh, and make sure to have the maintainers of the drivers/[tty|char]
in the 'To' field.

Cheers!
Konrad
> 
> Serialize device and port open and initialization.  Added a mutex
> which gates the handling of control messages in virtio_console.c.
> This includes adding and removing ports, and making opened ports be
> consoles.  Extended the use of the prvdata spinlock to cover some missed
> modifications to prvdata.next_vtermno.
> 
> I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting
> for the driver to be ready, and for the one-time call to hvc_init().  It
> had been the case that this was sometimes being called mulitple times, and
> partially setup state was being used by the second caller of hvc_alloc().
> 
> Make separate struct console* for each new port.  There was a single static
> struct console* hvc_console, to be used for early printk.  We aren't doing
> early printk, but more importantly, there is no code to multiplex on that
> one console.  Multiple virtio_console ports were "sharing" this, which was
> disasterous since both the index and the flags for the console are stored
> there. The console struct is remembered in the hvc_struct, and it is
> deallocated when the hvc_struct is deallocated.
> 
> Signed-off-by: Miche Baker-Harvey <miche@google.com>
> Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/char/virtio_console.c |   22 +++++++++++++++++++---
>  drivers/tty/hvc/hvc_console.c |   39 +++++++++++++++++++++++++++++++++------
>  drivers/tty/hvc/hvc_console.h |    1 +
>  3 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fb68b12..e819d46 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -24,6 +24,7 @@
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -95,6 +96,11 @@ struct console {
>  	u32 vtermno;
>  };
>  
> +/* serialize the handling of control messages, which includes
> + * the initialization of the virtio_consoles.
> + */
> +static DEFINE_MUTEX(virtio_console_mutex);
> +
>  struct port_buffer {
>  	char *buf;
>  
> @@ -979,8 +985,14 @@ int init_port_console(struct port *port)
>  	 * 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;
> +	spin_lock_irq(&pdrvdata_lock);
> +	port->cons.vtermno = pdrvdata.next_vtermno++;
> +	spin_unlock_irq(&pdrvdata_lock);
>  
> +	/*
> +	 * xxx Use index 0 for now assuming there is no early HVC, since
> +	 * we don't support it.
> +	 */
>  	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);
> @@ -990,7 +1002,6 @@ int init_port_console(struct port *port)
>  		return ret;
>  	}
>  	spin_lock_irq(&pdrvdata_lock);
> -	pdrvdata.next_vtermno++;
>  	list_add_tail(&port->cons.list, &pdrvdata.consoles);
>  	spin_unlock_irq(&pdrvdata_lock);
>  	port->guest_connected = true;
> @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev,
>  	int err;
>  
>  	cpkt = (struct virtio_console_control *)(buf->buf + buf->offset);
> -
>  	port = find_port_by_id(portdev, cpkt->id);
>  	if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) {
>  		/* No valid header at start of buffer.  Drop it. */
> @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev,
>  		return;
>  	}
>  
> +	/*
> +	 * These are rare initialization-time events that should be
> +	 * serialized.
> +	 */
> +	mutex_lock(&virtio_console_mutex);
>  	switch (cpkt->event) {
>  	case VIRTIO_CONSOLE_PORT_ADD:
>  		if (port) {
> @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev,
>  		}
>  		break;
>  	}
> +	mutex_unlock(&virtio_console_mutex);
>  }
>  
>  static void control_work_handler(struct work_struct *work)
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 7430bc3..03ff6ed 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -29,8 +29,9 @@
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
>  #include <linux/list.h>
> -#include <linux/module.h>
>  #include <linux/major.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/sysrq.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> @@ -84,6 +85,10 @@ static LIST_HEAD(hvc_structs);
>   * list traversal.
>   */
>  static DEFINE_SPINLOCK(hvc_structs_lock);
> +/*
> + * only one task does allocation at a time.
> + */
> +static DEFINE_MUTEX(hvc_ports_mutex);
>  
>  /*
>   * This value is used to assign a tty->index value to a hvc_struct based
> @@ -242,6 +247,7 @@ static void destroy_hvc_struct(struct kref *kref)
>  
>  	spin_unlock(&hvc_structs_lock);
>  
> +	kfree(hp->hvc_console);
>  	kfree(hp);
>  }
>  
> @@ -822,19 +828,25 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  			     int outbuf_size)
>  {
>  	struct hvc_struct *hp;
> +	struct console *cp;
>  	int i;
>  
>  	/* We wait until a driver actually comes along */
> +	mutex_lock(&hvc_ports_mutex);
>  	if (!hvc_driver) {
>  		int err = hvc_init();
> -		if (err)
> +		if (err) {
> +			mutex_unlock(&hvc_ports_mutex);
>  			return ERR_PTR(err);
> +		}
>  	}
>  
>  	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
>  			GFP_KERNEL);
> -	if (!hp)
> +	if (!hp) {
> +		mutex_unlock(&hvc_ports_mutex);
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	hp->vtermno = vtermno;
>  	hp->data = data;
> @@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  	kref_init(&hp->kref);
>  
>  	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
> +	/*
> +	 * make each console its own struct console.
> +	 * No need to do allocation and copy under lock.
> +	 */
> +	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> +	if (!cp) {
> +		kfree(hp);
> +		mutex_unlock(&hvc_ports_mutex);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	memcpy(cp, &hvc_console, sizeof(*cp));
> +	hp->hvc_console = cp;
> +
>  	spin_lock_init(&hp->lock);
>  	spin_lock(&hvc_structs_lock);
>  
> @@ -862,13 +887,14 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  		i = ++last_hvc;
>  
>  	hp->index = i;
> -	hvc_console.index = i;
> +	cp->index = i;
>  	vtermnos[i] = vtermno;
>  	cons_ops[i] = ops;
>  
>  	list_add_tail(&(hp->next), &hvc_structs);
>  	spin_unlock(&hvc_structs_lock);
> -	register_console(&hvc_console);
> +	register_console(cp);
> +	mutex_unlock(&hvc_ports_mutex);
>  
>  	return hp;
>  }
> @@ -879,7 +905,8 @@ int hvc_remove(struct hvc_struct *hp)
>  	unsigned long flags;
>  	struct tty_struct *tty;
>  
> -	unregister_console(&hvc_console);
> +	BUG_ON(!hp->hvc_console);
> +	unregister_console(hp->hvc_console);
>  	spin_lock_irqsave(&hp->lock, flags);
>  	tty = tty_kref_get(hp->tty);
>  
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index c335a14..2d20ab7 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -58,6 +58,7 @@ struct hvc_struct {
>  	const struct hv_ops *ops;
>  	int irq_requested;
>  	int data;
> +	struct console *hvc_console;
>  	struct winsize ws;
>  	struct work_struct tty_resize;
>  	struct list_head next;
> -- 
> 1.7.3.1

Re: [PATCH] VirtioConsole support.

[Greg KH]

On Thu, Oct 27, 2011 at 05:55:35PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 27, 2011 at 02:43:22PM -0700, Miche Baker-Harvey wrote:
> > Multiple HVC console terminals enabled.
> 
> Miche,
> 
> You might want to flesh out the description a bit. Perhaps include
> such details as : "fixes the infinite loop that git commit XX
> caused". Perhaps include some of the serial console output.
> 
> Maybe also change the title - the patch name (which is what shows up
> if you do 'git log --oneline') is 'VirtioConsole support.' which is
> not very informative.

Yes, all of these need to be resolved before I can accept this.

thanks,

greg k-h

Re: [PATCH] VirtioConsole support.

[Joe Perches]

On Thu, 2011-10-27 at 14:43 -0700, Miche Baker-Harvey wrote:
> Multiple HVC console terminals enabled.

Just a note on allocation.

> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
[]
> @@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  	kref_init(&hp->kref);
>  
>  	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
> +	/*
> +	 * make each console its own struct console.
> +	 * No need to do allocation and copy under lock.
> +	 */
> +	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> +	if (!cp) {
> +		kfree(hp);
> +		mutex_unlock(&hvc_ports_mutex);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	memcpy(cp, &hvc_console, sizeof(*cp));

The kzalloc should be kmalloc as the allocated
memory is immediately overwritten.

Re: [PATCH] VirtioConsole support.

[Stephen Boyd]

On 10/27/11 19:55, Joe Perches wrote:
>
> @@ -845,6 +857,19 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>  	kref_init(&hp->kref);
>  
>  	INIT_WORK(&hp->tty_resize, hvc_set_winsz);
> +	/*
> +	 * make each console its own struct console.
> +	 * No need to do allocation and copy under lock.
> +	 */
> +	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> +	if (!cp) {
> +		kfree(hp);
> +		mutex_unlock(&hvc_ports_mutex);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	memcpy(cp, &hvc_console, sizeof(*cp));
> The kzalloc should be kmalloc as the allocated
> memory is immediately overwritten.

Even better would be kmemdup().

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] VirtioConsole support.

[Rusty Russell]

On Fri, 28 Oct 2011 11:19:40 -0700, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Even better would be kmemdup().

OK, this driver is horribly unloved, and ugly.  Noone claims to really
understand it, but people keep poking it.

Ben, Christian, can you figure out what's going on?  Is it time for a
complete rewrite?

Thanks,
Rusty.

Re: [PATCH] VirtioConsole support.

[Christian Borntraeger]

On 02/11/11 23:02, Rusty Russell wrote:
> Ben, Christian, can you figure out what's going on?  Is it time for a
> complete rewrite?

You are talking about hvc console? Yes it has so many users that it is now
something like a console/tty layer extension, but it was never designed to be
one. 

I think the hardest part would be to get all users together (for testing and
requirements) and then to find someone that is actually doing that rework. Dont
know if the pain level is already that high?

Christian

Re: [PATCH] VirtioConsole support.

[Konrad Rzeszutek Wilk]

On Thu, Nov 03, 2011 at 01:38:15PM +0100, Christian Borntraeger wrote:
> On 02/11/11 23:02, Rusty Russell wrote:
> > Ben, Christian, can you figure out what's going on?  Is it time for a
> > complete rewrite?
> 
> You are talking about hvc console? Yes it has so many users that it is now
> something like a console/tty layer extension, but it was never designed to be
> one. 
> 
> I think the hardest part would be to get all users together (for testing and
> requirements) and then to find someone that is actually doing that rework. Dont

I am volunteering myself to test the Xen user of it. Perhaps even add some patches,
albeit my knowledge of the hvc console is that "it works" (well, with this minor
speed bump).

> know if the pain level is already that high?
> 
> Christian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/