[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxx>


> 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@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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.