Linux Virtualization - Nov 2009 - [PATCH 1/8] virtio: console: comment cleanup

Remove old lguest-style comments.

Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
---
 drivers/char/virtio_console.c |   30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
--- 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,7 +34,7 @@ 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:
@@ -90,7 +75,7 @@ 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
+/* 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
@@ -123,9 +108,8 @@ static int get_chars(u32 vtermno, char *
 
 	return count;
 }
-/*:*/
 
-/*D:320 Console drivers are initialized very early so boot messages can go out,
+/* 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.
  *
@@ -179,7 +163,7 @@ static void hvc_handle_input(struct virt
 		hvc_kick();
 }
 
-/*D:370 Once we're further in boot, we get probed like any other virtio
device.
+/* 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
@@ -203,8 +187,6 @@ static int __devinit virtcons_probe(stru
 	}
 
 	/* 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;

On Tue, 10 Nov 2009 04:57:30 pm Rusty Russell wrote:
> 
> Rather than assume a single port, add a 'struct ports' with an
array
> of ports.  Currently, there's always only one, but that will change.
Now, from here we need several more patches.  At least:

1) Generalize the output path so we can sleep, rather than spinning.
   This means a non-NULL callback, a 'done' flag and a struct
completion.

2) Do we really need more than input buffer at a time?  If not, it's easy to
   generalize the input callback.  This will be slow, but shouldn't be a
   problem.

3) Add a header.  I suggest we change your proposed format a little, rather
   than an explicit length use a "continues" bit:

	struct virtio_console_header {
		/* Port number */
		__u32 id;
		/* VIRTIO_CONSOLE_HDR_CONTROLMSG, VIRTIO_CONSOLE_HDR_CONTINUES. */
		__u32 flags;
	} __attribute__((packed));

   This should be really easy to construct, and for input in the !multiport
   path we can fake one up.  We ignore CONTINUES on input since we don't
have
   a userspace API which understands framing (we'd need recvmsg).

4) Hook it all together with the new feature bit.

5) Add the debugfs and sysfs stuff in stages.

6) See if we really need throttling now we're only allowing 1 buffer at a
   time.

Cheers,
Rusty.

Am Dienstag 10 November 2009 07:27:30 schrieb Rusty
Russell:
> 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 at rustcorp.com.au>
> To: Christian Borntraeger <borntraeger at de.ibm.com>
> Cc: linuxppc-dev at 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(-)
Ok, I started with patches 1-3. I like the result.

So for the first 3 patches you can add
Tested-by: Christian Borntraeger <borntraeger at de.ibm.com> (on s390)
Reviewed-by: Christian Borntraeger <borntraeger at de.ibm.com>

I have not yet looked at the other ones, but I will try to find some time to 
look at them.

Christian

On (Tue) Nov 10 2009 [16:57:30], Rusty Russell wrote:
> 
> Rather than assume a single port, add a 'struct ports' with an
array
> of ports.  Currently, there's always only one, but that will change.
Hey Rusty,
>  static void virtcons_apply_config(struct virtio_device *dev)
>  {
> -	struct port *port = dev->priv;
> +	struct ports *ports = dev->priv;
>  	struct winsize ws;
>  
>  	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
> @@ -158,7 +168,9 @@ static void virtcons_apply_config(struct
>  		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(ports->port[0].hvc, ws);
I've fixed a bug in the latest patches that I have (just on git so far;
not sent to lists) -- this function is also called from hvc's
notifier_add, so instead of passing vdev here, we can just pass the port
and look up the port in the notifier_add_vio() function via
get_port_from_vtermno().
> -static struct port *__devinit alloc_port(u32 vtermno)
> +static struct ports *__devinit alloc_ports(unsigned int num)
This will have to be changed when we add support for hotplug. So instead
of doing this, just have a linked list from the start?
>  {
> -	struct port *port = kmalloc(sizeof *port, GFP_KERNEL);
> +	struct ports *ports;
> +	int i;
>  
> -	if (!port)
> +	ports = kmalloc(sizeof *ports + sizeof(ports->port[0]) * num,
> +			GFP_KERNEL);
> +	if (!ports)
>  		return NULL;

Other than this, the series is good; I can base my patches on top of
these.

I guess we can also assign a number to each vdev that gets probed so
that sysfs and debugfs entries for ports can be put in their
vdev-specific directories, like

/sys/class/virtio-console0/vcon0/name

etc.

Also, if you think the send/receive workqueues are fine and we move to
those, they will have to be introduced slightly earlier in this patch
series.

(I'll send out my patches to the list in a while; some polishing is
required still but they're all style issues rather than functionality
ones.)

		Amit