diff mbox

[Qemu-devel] Re: virtio-serial: An interface for host-guest communication

Message ID 20090814081518.GA7418@amit-x200.redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Amit Shah Aug. 14, 2009, 8:15 a.m. UTC
On (Mon) Aug 10 2009 [11:59:31], Anthony Liguori wrote:
>
> However, as I've mentioned repeatedly, the reason I won't merge  
> virtio-serial is that it duplicates functionality with virtio-console.   
> If the two are converged, I'm happy to merge it.  I'm not opposed to  
> having more functionality.

The guest code sort-of ends up looking like this after merging
virtio_console into virtio_serial. Diff is against virtio_serial in my
git tree, but that should be pretty close to the last submission I made
at

http://patchwork.kernel.org/patch/39335/

or the tree at

git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git

I've merged bits from virtio_console.c into virtio_serial.c. If needed,
virtio_serial can be renamed to virtio_console. The VIRITIO_ID also
needs to change to that of virtio_console's.

Similar changes are needed for userspace.

Since there's support for only one console as of now, I've assigned port
#2 as the console port so that we hook into hvc when a port is found at
that location.

One issue that crops up for put_chars: a copy of the buffer to be sent
has to be made as we don't wait for host to ack the buffer before we
move on.

The biggest loss so far is Rusty's excellent comments: they will have to
be reworked and added for the whole of the new file.

Is this approach acceptable?



		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Anthony Liguori Aug. 14, 2009, 1:29 p.m. UTC | #1
Amit Shah wrote:
> On (Mon) Aug 10 2009 [11:59:31], Anthony Liguori wrote:
>   
>> However, as I've mentioned repeatedly, the reason I won't merge  
>> virtio-serial is that it duplicates functionality with virtio-console.   
>> If the two are converged, I'm happy to merge it.  I'm not opposed to  
>> having more functionality.
>>     
>
> The guest code sort-of ends up looking like this after merging
> virtio_console into virtio_serial. Diff is against virtio_serial in my
> git tree, but that should be pretty close to the last submission I made
> at
>
> http://patchwork.kernel.org/patch/39335/
>
> or the tree at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git
>
> I've merged bits from virtio_console.c into virtio_serial.c. If needed,
> virtio_serial can be renamed to virtio_console. The VIRITIO_ID also
> needs to change to that of virtio_console's.
>
> Similar changes are needed for userspace.
>
> Since there's support for only one console as of now, I've assigned port
> #2 as the console port so that we hook into hvc when a port is found at
> that location.
>
> One issue that crops up for put_chars: a copy of the buffer to be sent
> has to be made as we don't wait for host to ack the buffer before we
> move on.
>
> The biggest loss so far is Rusty's excellent comments: they will have to
> be reworked and added for the whole of the new file.
>
> Is this approach acceptable?
>   

I think we want to keep virtio_console.c and definitely continue using 
the virtio_console id.  It looks like you are still creating character 
devices as opposed to tty devices.  Is this just an incremental step or 
are you choosing to not do tty devices for a reason (as if so, what's 
that reason)?

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Shah Aug. 14, 2009, 1:41 p.m. UTC | #2
On (Fri) Aug 14 2009 [08:29:28], Anthony Liguori wrote:
> Amit Shah wrote:
>> On (Mon) Aug 10 2009 [11:59:31], Anthony Liguori wrote:
>>   
>>> However, as I've mentioned repeatedly, the reason I won't merge   
>>> virtio-serial is that it duplicates functionality with 
>>> virtio-console.   If the two are converged, I'm happy to merge it.  
>>> I'm not opposed to  having more functionality.
>>>     
>>
>> The guest code sort-of ends up looking like this after merging
>> virtio_console into virtio_serial. Diff is against virtio_serial in my
>> git tree, but that should be pretty close to the last submission I made
>> at
>>
>> http://patchwork.kernel.org/patch/39335/
>>
>> or the tree at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git
>>
>> I've merged bits from virtio_console.c into virtio_serial.c. If needed,
>> virtio_serial can be renamed to virtio_console. The VIRITIO_ID also
>> needs to change to that of virtio_console's.
>>
>> Similar changes are needed for userspace.
>>
>> Since there's support for only one console as of now, I've assigned port
>> #2 as the console port so that we hook into hvc when a port is found at
>> that location.
>>
>> One issue that crops up for put_chars: a copy of the buffer to be sent
>> has to be made as we don't wait for host to ack the buffer before we
>> move on.
>>
>> The biggest loss so far is Rusty's excellent comments: they will have to
>> be reworked and added for the whole of the new file.
>>
>> Is this approach acceptable?
>>   
>
> I think we want to keep virtio_console.c and definitely continue using  
> the virtio_console id.  It looks like you are still creating character  
> devices as opposed to tty devices.  Is this just an incremental step or  
> are you choosing to not do tty devices for a reason (as if so, what's  
> that reason)?

Just an incremental step. In fact, I haven't yet thought about exposing
a tty device and any problems that might come up. I'll get to doing that
too, of course.

Currently I plan to:
- finish sending connect/disconnect notifications
- finish merge of virtio-console and serial
- look at tty

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gerd Hoffmann Aug. 14, 2009, 1:49 p.m. UTC | #3
On 08/14/09 10:15, Amit Shah wrote:
> The guest code sort-of ends up looking like this after merging
> virtio_console into virtio_serial.

I think it should better go the other way around: add multichannel 
support to virtio-concole, probably guarded by a feature flag so old 
host+new guest and new host+old guest combinations continue to work.

> Since there's support for only one console as of now, I've assigned port
> #2 as the console port so that we hook into hvc when a port is found at
> that location.

Doesn't sound like this is going to be backward compatible ...

Also I still think passing a 'protocol' string for each port is a good 
idea, so you can stick that into a sysfs file for guests use.

Note it is probably easy to make virtio-console support multiple hvc 
lines by having one protocol for that (named 'console' for example).

cheers,
   Gerd

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anthony Liguori Aug. 14, 2009, 4:25 p.m. UTC | #4
Gerd Hoffmann wrote:
> On 08/14/09 10:15, Amit Shah wrote:
>> The guest code sort-of ends up looking like this after merging
>> virtio_console into virtio_serial.
>
> I think it should better go the other way around: add multichannel 
> support to virtio-concole, probably guarded by a feature flag so old 
> host+new guest and new host+old guest combinations continue to work.
>
>> Since there's support for only one console as of now, I've assigned port
>> #2 as the console port so that we hook into hvc when a port is found at
>> that location.
>
> Doesn't sound like this is going to be backward compatible ...
>
> Also I still think passing a 'protocol' string for each port is a good 
> idea, so you can stick that into a sysfs file for guests use.

Or drops ports altogether and just use protocol strings...

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rusty Russell Aug. 20, 2009, 7:31 a.m. UTC | #5
On Sat, 15 Aug 2009 01:55:32 am Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> > Also I still think passing a 'protocol' string for each port is a good 
> > idea, so you can stick that into a sysfs file for guests use.
> 
> Or drops ports altogether and just use protocol strings...

Both is silly, yes.

I guess strings + HAL magic can make the /dev names sane.  I don't want to
see userspace trolling through sysfs to figure out what device to open.

Which is why I prefer assigned numbers, which get mapped to minors.

Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gerd Hoffmann Aug. 20, 2009, 7:44 a.m. UTC | #6
On 08/20/09 09:31, Rusty Russell wrote:
> On Sat, 15 Aug 2009 01:55:32 am Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> Also I still think passing a 'protocol' string for each port is a good
>>> idea, so you can stick that into a sysfs file for guests use.
>> Or drops ports altogether and just use protocol strings...
>
> Both is silly, yes.
>
> I guess strings + HAL magic can make the /dev names sane.  I don't want to
> see userspace trolling through sysfs to figure out what device to open.

udev can create sane /dev names (or symlinks) by checking sysfs 
attributes, apps just open the /dev/whatever then.

> Which is why I prefer assigned numbers, which get mapped to minors.

ports map trivially to minors.  When using protocol strings minors can 
simply be dynamically auto-allocated by the guest and we don't need the 
port numbers in the host<->guest protocol any more.

I think strings are better as numbers for identifying protocols as you 
can work without a central registry for the numbers then.

cheers,
   Gerd
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Shah Aug. 20, 2009, 7:55 a.m. UTC | #7
On (Thu) Aug 20 2009 [09:44:29], Gerd Hoffmann wrote:
> On 08/20/09 09:31, Rusty Russell wrote:
>> On Sat, 15 Aug 2009 01:55:32 am Anthony Liguori wrote:
>>> Gerd Hoffmann wrote:
>>>> Also I still think passing a 'protocol' string for each port is a good
>>>> idea, so you can stick that into a sysfs file for guests use.
>>> Or drops ports altogether and just use protocol strings...
>>
>> Both is silly, yes.
>>
>> I guess strings + HAL magic can make the /dev names sane.  I don't want to
>> see userspace trolling through sysfs to figure out what device to open.
>
> udev can create sane /dev names (or symlinks) by checking sysfs  
> attributes, apps just open the /dev/whatever then.

There still will have to be some way in transferring all the strings
from qemu to the guest. Could be done from the config space, but will
have to be done one port at a time (config space is limited in size).

>> Which is why I prefer assigned numbers, which get mapped to minors.
>
> ports map trivially to minors.  When using protocol strings minors can  
> simply be dynamically auto-allocated by the guest and we don't need the  
> port numbers in the host<->guest protocol any more.
>
> I think strings are better as numbers for identifying protocols as you  
> can work without a central registry for the numbers then.

I like the way assigned numbers work: it's simpler to code, needs a
bitmap for all the ports that fits in nicely in the config space and
udev rules / scripts can point /dev/vmch02 to /dev/console.

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Shah Aug. 20, 2009, 1:42 p.m. UTC | #8
On (Fri) Aug 14 2009 [08:29:28], Anthony Liguori wrote:
> Amit Shah wrote:
>> On (Mon) Aug 10 2009 [11:59:31], Anthony Liguori wrote:
>>   
>>> However, as I've mentioned repeatedly, the reason I won't merge   
>>> virtio-serial is that it duplicates functionality with 
>>> virtio-console.   If the two are converged, I'm happy to merge it.  
>>> I'm not opposed to  having more functionality.
>>>     
>>
>> The guest code sort-of ends up looking like this after merging
>> virtio_console into virtio_serial. Diff is against virtio_serial in my
>> git tree, but that should be pretty close to the last submission I made
>> at
>>
>> http://patchwork.kernel.org/patch/39335/
>>
>> or the tree at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/amit/vs-kernel.git
>>
>> I've merged bits from virtio_console.c into virtio_serial.c. If needed,
>> virtio_serial can be renamed to virtio_console. The VIRITIO_ID also
>> needs to change to that of virtio_console's.
>>
>> Similar changes are needed for userspace.
>>
>> Since there's support for only one console as of now, I've assigned port
>> #2 as the console port so that we hook into hvc when a port is found at
>> that location.
>>
>> One issue that crops up for put_chars: a copy of the buffer to be sent
>> has to be made as we don't wait for host to ack the buffer before we
>> move on.
>>
>> The biggest loss so far is Rusty's excellent comments: they will have to
>> be reworked and added for the whole of the new file.
>>
>> Is this approach acceptable?
>>   
>
> I think we want to keep virtio_console.c and definitely continue using  
> the virtio_console id.

I've now seen some more code here and to me it looks like virtioconsole
is not used on any of the guests that qemu supports. The virtio_console
kernel module only works with lguest and s390 currently. There is one
feature and some config values supported by the kernel module but not in
qemu.

So it looks as if we have virtio-console merged but no one uses it. Is
this right?

If that's the case, I'll send patches to replace virtio-console with
virtio-serial, making sure we keep the command line arg,

-virtioconsole <qemu char dev>

which will be translated to something like

-virtioserial <qemu char dev>,port=2

or

-virtioserial <qemu char dev>,protocol=console

depending on what we finalise on.

Does anyone have objections to this or pointers for me to see where
virtioconsole is actually used by qemu-supported guests?

I'm also open to convert the guest kernel virtio_console driver over
to support the new functionality, or just let that be and have a new
virtio-serial module.

Rusty, what's your take on this?

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel P. Berrangé Aug. 20, 2009, 2:25 p.m. UTC | #9
On Thu, Aug 20, 2009 at 07:12:41PM +0530, Amit Shah wrote:
> 
> I've now seen some more code here and to me it looks like virtioconsole
> is not used on any of the guests that qemu supports. The virtio_console
> kernel module only works with lguest and s390 currently. There is one
> feature and some config values supported by the kernel module but not in
> qemu.
> 
> So it looks as if we have virtio-console merged but no one uses it. Is
> this right?

Nope. Grab a Fedora 11 live CD, and boot with

  # qemu-kvm -virtioconsole stdio -cdrom Fedora-11-i686-Live.iso  -m 500

Once it completes booting & logs into gnome, open a terminal and run
as root

   agetty /dev/hvc0 9600 vt100  

You'll get a login prompt on the host machine now.

What appears to not be working, is early kernel boot messages. eg, I
ought to be able todo

  # qemu-kvm -virtioconsole stdio -kernel vmlinuz -initrd initrd.img \
      -append "console=hvc0"  -m 500

and see the kernel boot messages, but this doesn't work with Fedora
kernels at least. Not tried upstream, or looked to see if this is just
an oversight in the Kconfig use for Fedora kernels.

Regards,
Daniel
Amit Shah Aug. 20, 2009, 2:38 p.m. UTC | #10
On (Thu) Aug 20 2009 [15:25:09], Daniel P. Berrange wrote:
> On Thu, Aug 20, 2009 at 07:12:41PM +0530, Amit Shah wrote:
> > 
> > I've now seen some more code here and to me it looks like virtioconsole
> > is not used on any of the guests that qemu supports. The virtio_console
> > kernel module only works with lguest and s390 currently. There is one
> > feature and some config values supported by the kernel module but not in
> > qemu.
> > 
> > So it looks as if we have virtio-console merged but no one uses it. Is
> > this right?
> 
> Nope. Grab a Fedora 11 live CD, and boot with
> 
>   # qemu-kvm -virtioconsole stdio -cdrom Fedora-11-i686-Live.iso  -m 500
> 
> Once it completes booting & logs into gnome, open a terminal and run
> as root
> 
>    agetty /dev/hvc0 9600 vt100  
> 
> You'll get a login prompt on the host machine now.
> 
> What appears to not be working, is early kernel boot messages. eg, I
> ought to be able todo

Oh; ok. So the console device is exposed only in the userspace; it's not
used for the early boot messages and not registered early-on. That's
only done for lguest and s390.

>   # qemu-kvm -virtioconsole stdio -kernel vmlinuz -initrd initrd.img \
>       -append "console=hvc0"  -m 500
> 
> and see the kernel boot messages, but this doesn't work with Fedora
> kernels at least. Not tried upstream, or looked to see if this is just
> an oversight in the Kconfig use for Fedora kernels.

Thanks,

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Shah Aug. 20, 2009, 2:42 p.m. UTC | #11
On (Thu) Aug 20 2009 [20:08:02], Amit Shah wrote:
> On (Thu) Aug 20 2009 [15:25:09], Daniel P. Berrange wrote:
> > On Thu, Aug 20, 2009 at 07:12:41PM +0530, Amit Shah wrote:
> > > 
> > > I've now seen some more code here and to me it looks like virtioconsole
> > > is not used on any of the guests that qemu supports. The virtio_console
> > > kernel module only works with lguest and s390 currently. There is one
> > > feature and some config values supported by the kernel module but not in
> > > qemu.
> > > 
> > > So it looks as if we have virtio-console merged but no one uses it. Is
> > > this right?
> > 
> > Nope. Grab a Fedora 11 live CD, and boot with
> > 
> >   # qemu-kvm -virtioconsole stdio -cdrom Fedora-11-i686-Live.iso  -m 500
> > 
> > Once it completes booting & logs into gnome, open a terminal and run
> > as root
> > 
> >    agetty /dev/hvc0 9600 vt100  
> > 
> > You'll get a login prompt on the host machine now.
> > 
> > What appears to not be working, is early kernel boot messages. eg, I
> > ought to be able todo
> 
> Oh; ok. So the console device is exposed only in the userspace; it's not
> used for the early boot messages and not registered early-on. That's
> only done for lguest and s390.

which, by the way, doesn't change what I wrote above: since it's not
used for earlyboot messages anyway it's not really used as a 'console'
in its real sense and so we could just replace it with the newer version
which makes it much easier for it to work with the kernel driver. Else
supporting older qemu with newer kernel driver will be difficult (if not
impossible).

> >   # qemu-kvm -virtioconsole stdio -kernel vmlinuz -initrd initrd.img \
> >       -append "console=hvc0"  -m 500
> > 
> > and see the kernel boot messages, but this doesn't work with Fedora
> > kernels at least. Not tried upstream, or looked to see if this is just
> > an oversight in the Kconfig use for Fedora kernels.

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jamie Lokier Aug. 20, 2009, 5:10 p.m. UTC | #12
Amit Shah wrote:
> > I think strings are better as numbers for identifying protocols as you  
> > can work without a central registry for the numbers then.
> 
> I like the way assigned numbers work: it's simpler to code, needs a
> bitmap for all the ports that fits in nicely in the config space and
> udev rules / scripts can point /dev/vmch02 to /dev/console.

How would a third party go about assigning themselves a number?

For the sake of example, imagine they develop a simple service like
"guesttop" which let's the host get a listing of guest processes.

They'll have to distributed app-specific udev rule patches for every
guest distro, which sounds like a lot of work.  The app itself is
probably a very simple C program; the hardest part of making it
portable across distros would be the udev rules, which is silly.

Anyway, every other device has a name or uuid these days.  You can
still use /dev/sda1 to refer to your boot partition, but LABEL=boot is
also available if you prefer.  Isn't that the ethos these days?

Why not both?  /dev/vmch05 if you prefer, plus symlink
/dev/vmch-guesttop -> /dev/vmch05 if name=guesttop was given to QEMU.

If you do stay with numbers only, note that it's not like TCP/UDP port
numbers because the number space is far smaller.  Picking a random
number that you hope nobody else uses is harder.

... Back to technical bits.  If config space is tight, use a channel!
Dedicate channel 0 to control, used to fetch the name (if there is
one) for each number.

-- Jamie
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rusty Russell Aug. 25, 2009, 12:43 p.m. UTC | #13
On Thu, 20 Aug 2009 05:14:29 pm Gerd Hoffmann wrote:
> I think strings are better as numbers for identifying protocols as you 
> can work without a central registry for the numbers then.

Yep, all you need is a central registry for names :)

Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gerd Hoffmann Aug. 25, 2009, 1 p.m. UTC | #14
On 08/25/09 14:43, Rusty Russell wrote:
> On Thu, 20 Aug 2009 05:14:29 pm Gerd Hoffmann wrote:
>> I think strings are better as numbers for identifying protocols as you
>> can work without a central registry for the numbers then.
>
> Yep, all you need is a central registry for names :)

There are schemes to do without (reverse domain names, i.e. 
au.com.rustcorp.* is all yours and you don't have to register).  Also 
with names the namespace is much bigger and clashes are much less 
likely, so chances that it works out with everybody simply picking a 
sane name are much higher.

cheers,
   Gerd
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 5e1915b..ab9c914 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -53,7 +53,6 @@  obj-$(CONFIG_HVC_IRQ)		+= hvc_irq.o
 obj-$(CONFIG_HVC_XEN)		+= hvc_xen.o
 obj-$(CONFIG_HVC_IUCV)		+= hvc_iucv.o
 obj-$(CONFIG_HVC_UDBG)		+= hvc_udbg.o
-obj-$(CONFIG_VIRTIO_CONSOLE)	+= virtio_console.o
 obj-$(CONFIG_VIRTIO_SERIAL)	+= virtio_serial.o
 obj-$(CONFIG_RAW_DRIVER)	+= raw.o
 obj-$(CONFIG_SGI_SNSC)		+= snsc.o snsc_event.o
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c74dacf..f82c036 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -43,39 +43,6 @@  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;
-
-/*D:310 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)
-{
-	struct scatterlist sg[1];
-	unsigned int len;
-
-	/* 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) {
-		/* 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))
-			cpu_relax();
-	}
-
-	/* We're expected to return the amount of data we wrote: all of it. */
-	return count;
-}
 
 /* Create a scatter-gather list representing our input buffer and put it in the
  * queue. */
@@ -90,94 +57,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
- * 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. */
-static int get_chars(u32 vtermno, char *buf, int count)
-{
-	/* If we don't have an input queue yet, we can't get input. */
-	BUG_ON(!in_vq);
-
-	/* No buffer?  Try to get one. */
-	if (!in_len) {
-		in = in_vq->vq_ops->get_buf(in_vq, &in_len);
-		if (!in)
-			return 0;
-	}
-
-	/* You want more than we have to give?  Well, try wanting less! */
-	if (in_len < count)
-		count = in_len;
-
-	/* Copy across to their buffer and increment offset. */
-	memcpy(buf, in, count);
-	in += count;
-	in_len -= count;
 
-	/* Finished?  Re-register buffer so Host will use it again. */
-	if (in_len == 0)
-		add_inbuf();
-
-	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.
- *
- * 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
- */
-static void virtcons_apply_config(struct virtio_device *dev)
-{
-	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));
-		hvc_resize(hvc, ws);
-	}
-}
-
-/*
- * 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)
-{
-	hp->irq_requested = 1;
-	virtcons_apply_config(vdev);
-
-	return 0;
-}
-
-static void notifier_del_vio(struct hvc_struct *hp, int data)
-{
-	hp->irq_requested = 0;
-}
-
-static void hvc_handle_input(struct virtqueue *vq)
-{
-	if (hvc_poll(hvc))
-		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.
@@ -212,27 +92,7 @@  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 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. */
-	hvc = hvc_alloc(0, 0, &virtio_cons, PAGE_SIZE);
-	if (IS_ERR(hvc)) {
-		err = PTR_ERR(hvc);
-		goto free_vqs;
-	}
 
 	/* Register the input buffer the first time. */
 	add_inbuf();
diff --git a/drivers/char/virtio_serial.c b/drivers/char/virtio_serial.c
index ef2d730..ff6ad06 100644
--- a/drivers/char/virtio_serial.c
+++ b/drivers/char/virtio_serial.c
@@ -37,6 +37,7 @@ 
 #include <linux/virtio.h>
 #include <linux/virtio_serial.h>
 #include <linux/workqueue.h>
+#include "hvc_console.h"
 
 struct virtio_serial_struct {
 	struct work_struct rx_work;
@@ -131,27 +132,13 @@  static int send_control_event(struct virtio_serial_control *sercontrol)
 	return ret;
 }
 
-static ssize_t virtserial_read(struct file *filp, char __user *ubuf,
-			       size_t count, loff_t *offp)
+static ssize_t fill_readbuf(struct virtio_serial_port *port, char *out_buf,
+			    size_t count, bool to_user)
 {
-	struct virtio_serial_port *port;
 	struct virtio_serial_port_buffer *buf, *buf2;
-	ssize_t ubuf_offset, ret;
-
-	port = filp->private_data;
-
-	ret = 0;
-	if (list_empty(&port->readbuf_head)) {
-		if (filp->f_flags & O_NONBLOCK)
-			return -EAGAIN;
+	ssize_t out_offset, ret;
 
-		ret = wait_event_interruptible(port->waitqueue,
-					       !list_empty(&port->readbuf_head));
-	}
-	if (ret < 0)
-		return ret;
-
-	ubuf_offset = 0;
+	out_offset = 0;
 	list_for_each_entry_safe(buf, buf2, &port->readbuf_head, next) {
 		size_t copy_size;
 
@@ -159,16 +146,24 @@  static ssize_t virtserial_read(struct file *filp, char __user *ubuf,
 		if (copy_size > buf->len - buf->offset)
 			copy_size = buf->len - buf->offset;
 
-		ret = copy_to_user(ubuf + ubuf_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 */
+		}
 
-		/* FIXME: Deal with ret != 0 */
 		/* Return the number of bytes actually copied */
 		ret = copy_size - ret;
 		buf->offset += ret;
-		ubuf_offset += ret;
+		out_offset += ret;
 		count -= ret;
-		ret = ubuf_offset;
+		ret = out_offset;
 
 		if (buf->len - buf->offset == 0) {
 			list_del(&buf->next);
@@ -178,6 +173,30 @@  static ssize_t virtserial_read(struct file *filp, char __user *ubuf,
 		if (!count)
 			break;
 	}
+	return out_offset;
+}
+
+static ssize_t virtserial_read(struct file *filp, char __user *ubuf,
+			       size_t count, loff_t *offp)
+{
+	struct virtio_serial_port *port;
+	ssize_t ret;
+
+	port = filp->private_data;
+
+	ret = 0;
+	if (list_empty(&port->readbuf_head)) {
+		if (filp->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(port->waitqueue,
+					       !list_empty(&port->readbuf_head));
+	}
+	if (ret < 0)
+		return ret;
+
+	ret = fill_readbuf(port, ubuf, count, 1);
+
 	return ret;
 }
 
@@ -196,21 +215,19 @@  struct vbuf {
 	unsigned int nent;
 };
 
-static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
-				size_t count, loff_t *offp)
+static ssize_t send_writebuf(struct virtio_serial_port *port,
+			     const char *in_buf, size_t count, bool from_user)
 {
 	struct virtqueue *out_vq;
-	struct virtio_serial_port *port;
 	struct virtio_serial_id id;
 	struct vbuf *vbuf;
-	size_t offset, size;
+	size_t in_offset, copy_size;
 	ssize_t ret;
 	unsigned int i, id_len;
 
 	if (!count)
 		return 0;
 
-	port = filp->private_data;
 	id.id = get_id_from_port(port);
 	out_vq = virtserial.out_vq;
 
@@ -234,10 +251,10 @@  static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
 	sg_init_table(vbuf->sg, vbuf->nent);
 
 	i = 0; /* vbuf->bufs[i] */
-	offset = 0; /* offset in the user buffer */
-	while (count - offset) {
-		size = min(count - offset + id_len, PAGE_SIZE);
-		vbuf->bufs[i] = kzalloc(size, GFP_KERNEL);
+	in_offset = 0; /* offset in the source buffer */
+	while (count - in_offset) {
+		copy_size = min(count - in_offset + id_len, PAGE_SIZE);
+		vbuf->bufs[i] = kzalloc(copy_size, GFP_KERNEL);
 		if (!vbuf->bufs[i]) {
 			ret = -ENOMEM;
 			if (!i)
@@ -246,12 +263,25 @@  static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
 		}
 		if (id_len) {
 			memcpy(vbuf->bufs[i], &id, id_len);
-			size -= id_len;
+			copy_size -= id_len;
 		}
-		ret = copy_from_user(vbuf->bufs[i] + id_len, ubuf + offset, size);
-		offset += size - ret;
+		if (from_user)
+			ret = copy_from_user(vbuf->bufs[i] + id_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 string
+			 */
+			memcpy(vbuf->bufs[i] + id_len,
+			       in_buf + in_offset, copy_size);
+			ret = 0; /* Emulate copy_from_user */
+		}
+		in_offset += copy_size - ret;
 
-		sg_set_buf(&vbuf->sg[i], vbuf->bufs[i], size - ret + id_len);
+		sg_set_buf(&vbuf->sg[i], vbuf->bufs[i],
+			   copy_size - ret + id_len);
 		id_len = 0; /* Pass the port id only in the first buffer */
 		i++;
 	}
@@ -263,7 +293,7 @@  static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
 	out_vq->vq_ops->kick(out_vq);
 
 	/* We're expected to return the amount of data we wrote */
-	return offset;
+	return in_offset;
 free_buffers:
 	while (--i >= 0)
 		kfree(vbuf->bufs[i]);
@@ -276,6 +306,16 @@  free_vbuf:
 	return ret;
 }
 
+static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
+				size_t count, loff_t *offp)
+{
+	struct virtio_serial_port *port;
+
+	port = filp->private_data;
+
+	return send_writebuf(port, ubuf, count, 1);
+}
+
 static int virtserial_release(struct inode *inode, struct file *filp)
 {
 	struct virtio_serial_control *sercontrol;
@@ -343,6 +383,104 @@  static const struct file_operations virtserial_fops = {
 	.release = virtserial_release,
 };
 
+/* Some routines for supporting virtio console */
+
+/* The operations for our console. */
+static struct hv_ops virtio_cons;
+
+/* The hvc device */
+static struct hvc_struct *hvc;
+
+/*D:310 The console_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 console_put_chars(u32 vtermno, const char *buf, int count)
+{
+	struct virtio_serial_port *port;
+
+	port = get_port_from_id(VIRTIO_SERIAL_CONSOLE_PORT);
+	if (!port)
+		return 0;
+
+	return send_writebuf(port, buf, count, 0);
+}
+
+/*D:350 console_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. */
+static int console_get_chars(u32 vtermno, char *out_buf, int count)
+{
+	struct virtio_serial_port *port;
+
+	/* If we don't have an input queue yet, we can't get input. */
+	BUG_ON(!virtserial.in_vq);
+
+	port = get_port_from_id(VIRTIO_SERIAL_CONSOLE_PORT);
+	if (!port)
+		return 0;
+
+	if (list_empty(&port->readbuf_head))
+		return 0;
+
+	return fill_readbuf(port, out_buf, count, 0);
+}
+/*:*/
+
+/*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.
+ *
+ * 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
+ */
+static void virtcons_apply_config(struct virtio_device *dev)
+{
+	struct winsize ws;
+
+	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
+		dev->config->get(dev,
+				 offsetof(struct virtio_serial_config, cols),
+				 &ws.ws_col, sizeof(u16));
+		dev->config->get(dev,
+				 offsetof(struct virtio_serial_config, rows),
+				 &ws.ws_row, sizeof(u16));
+		hvc_resize(hvc, ws);
+	}
+}
+
+/*
+ * 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 console_notifier_add_vio(struct hvc_struct *hp, int data)
+{
+	hp->irq_requested = 1;
+	virtcons_apply_config(virtserial.vdev);
+
+	return 0;
+}
+
+static void console_notifier_del_vio(struct hvc_struct *hp, int data)
+{
+	hp->irq_requested = 0;
+}
+
+
 static void virtio_serial_queue_work_handler(struct work_struct *work)
 {
 	struct scatterlist sg[1];
@@ -410,6 +548,10 @@  static void virtio_serial_rx_work_handler(struct work_struct *work)
 
 		wake_up_interruptible(&port->waitqueue);
 	}
+
+	if (get_id_from_port(port) == VIRTIO_SERIAL_CONSOLE_PORT && hvc_poll(hvc))
+		hvc_kick();
+
 	/* Allocate buffers for all the ones that got used up */
 	schedule_work(&virtserial.queue_work);
 }
@@ -555,6 +697,33 @@  static int virtserial_add_port(u32 port_nr)
 
 	list_add_tail(&port->next, &virtserial.port_head);
 
+	if (port_nr == VIRTIO_SERIAL_CONSOLE_PORT) {
+		/* Start using the new console output. */
+		virtio_cons.get_chars = console_get_chars;
+		virtio_cons.put_chars = console_put_chars;
+		virtio_cons.notifier_add = console_notifier_add_vio;
+		virtio_cons.notifier_del = console_notifier_del_vio;
+		virtio_cons.notifier_hangup = console_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 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)) {
+			ret = PTR_ERR(hvc);
+			goto free_cdev;
+		}
+	}
+
 	pr_info("virtio-serial port found at id %u\n", port_nr);
 
 	return 0;
@@ -721,9 +890,13 @@  static struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
+static unsigned int features[] = {
+	VIRTIO_CONSOLE_F_SIZE,
+};
+
 static struct virtio_driver virtio_serial = {
-  //	.feature_table = virtserial_features,
-  //	.feature_table_size = ARRAY_SIZE(virtserial_features),
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
 	.driver.name =	KBUILD_MODNAME,
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
diff --git a/include/linux/virtio_serial.h b/include/linux/virtio_serial.h
index 26ccd83..1c9f853 100644
--- a/include/linux/virtio_serial.h
+++ b/include/linux/virtio_serial.h
@@ -10,7 +10,16 @@ 
 
 #define VIRTIO_SERIAL_BAD_ID		(~(u32)0)
 
+/* Feature bits */
+#define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
+
 struct virtio_serial_config {
+	/* first two values come from virtio_console */
+	/* colums of the screens */
+	__u16 cols;
+	/* rows of the screens */
+	__u16 rows;
+
 	__u32 max_nr_ports;
 	__u32 nr_active_ports;
 	__u32 ports_map[0 /* (max_nr_ports + 31) / 32 */];
@@ -22,6 +31,9 @@  struct virtio_serial_control {
 	__u16 value;
 };
 
+/* Predefined ports */
+#define VIRTIO_SERIAL_CONSOLE_PORT 2
+
 /* Some events for the control channel */
 /*   Guest -> Host  range 1..256 */
 #define VIRTIO_SERIAL_GUEST_PORT_OPEN	1
@@ -31,7 +43,7 @@  struct virtio_serial_control {
 
 #ifdef __KERNEL__
 
-/* Guest kernel - Guest userspace interface */
+int __init virtio_cons_early_init(int (*put_chars)(u32, const char *, int));
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_VIRTIO_SERIAL_H */