WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] VirtioConsole support.

To: Miche Baker-Harvey <miche@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] VirtioConsole support.
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Thu, 27 Oct 2011 17:55:35 -0400
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>, Greg Kroah-Hartman <gregkh@xxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx, Anton Blanchard <anton@xxxxxxxxx>, Amit Shah <amit.shah@xxxxxxxxxx>
Delivery-date: Thu, 27 Oct 2011 14:57:56 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1319751802-27013-1-git-send-email-miche@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20111027173527.GA23839@xxxxxxxxxxxxxxxxxxx> <1319751802-27013-1-git-send-email-miche@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> Reported-by-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>