diff mbox

[3/6] virtio-console: Add a virtio-console bus, support for multiple ports

Message ID 1254225888-17093-4-git-send-email-amit.shah@redhat.com
State Superseded
Headers show

Commit Message

Amit Shah Sept. 29, 2009, 12:04 p.m. UTC
This patch migrates virtio-console to the qdev infrastructure, and
creates a new virtio-console bus on which multiple ports are exposed as
devices. The bulk of the code now resides in a new file with
virtio-console.c being just a simple qdev device.

This interface extends the virtio-console device to handle
multiple ports from which bits can be sent and read.

The older -virtioconsole argument is now deprecated in favour of:

    -device virtio-console-pci -device virtconsole

The virtconsole device type accepts a chardev as an argument and a 'name'
argument to identify the corresponding consoles on the host as well as the
guest. The name, if given, is exposed via the 'name' sysfs attribute.

Care has been taken to ensure compatibility with kernels that do not
support multiple ports.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 Makefile.target         |    2 +-
 hw/pc.c                 |    9 -
 hw/qdev.c               |    8 +-
 hw/virtio-console-bus.c |  738 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-console.c     |  148 +++-------
 hw/virtio-console.h     |  130 +++++++++
 qemu-options.hx         |    8 -
 sysemu.h                |    6 -
 vl.c                    |   37 ---
 9 files changed, 907 insertions(+), 179 deletions(-)
 create mode 100644 hw/virtio-console-bus.c

Comments

Gerd Hoffmann Sept. 29, 2009, 6:04 p.m. UTC | #1
> +typedef struct VirtIOConsole
> +{

You are reusing this struct name a few times.
Better don't do that, it is confusing.

> +static VirtIOConsole *virtio_console;

Why do you need this?

> +static void flush_queue(VirtConPort *port)
> +{
> +    VirtConPortBuffer *buf, *buf2;
> +    uint8_t *outbuf;
> +    size_t outlen, outsize;
> +
> +    /*
> +     * If the app isn't interested in buffering packets till it's
> +     * opened, just drop the data guest sends us till a connection is
> +     * established.
> +     */
> +    if (!port->host_connected&&  !port->flush_buffers)
> +        return;

Hmm.  Who needs that buffering?  Only user seems to be the port driver 
(patch 4/6).  So move the buffering (and the host_connected state 
tracking) from the core to that driver?

> +/* Readiness of the guest to accept data on a port */
> +static int vcon_can_read(void *opaque)

int vcon_can_read(VirtConPort *port)

> +static void vcon_read(void *opaque, const uint8_t *buf, int size)
> +static void vcon_event(void *opaque, int event)

Likewise.

> +static VirtConBus *virtcon_bus;

Why do you need this?

> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
> +{

> +    if (port->chr) {
> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
> +                              port);

Should be handled by the VirtConPort drivers.

> +    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
> +        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
> +         * We anyway use up that much space for the bitmap and it
> +         * simplifies some calculations
> +         */
> +        return NULL;
> +    }

Huh?  Runtime check for a compile-time constant?

> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
> +    virtcon_bus_new(dev);

s->bus = virtcon_bus_new(dev);
port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for 
backward compat */
qdev_prop_set_*(port0, ...);
qdev_init(port0);

Or does that happen somewhere else?

> +typedef struct VirtConBus VirtConBus;
> +typedef struct VirtConPort VirtConPort;
> +typedef struct VirtConPortInfo VirtConPortInfo;
> +
> +typedef struct VirtConDevice {
> +    DeviceState qdev;
> +    VirtConPortInfo *info;
> +} VirtConDevice;

Leftover from old patch version?

> +/*
> + * This is the state that's shared between all the ports.  Some of the
> + * state is configurable via command-line options. Some of it can be
> + * set by individual devices in their initfn routines. Some of the
> + * state is set by the generic qdev device init routine.
> + */
> +struct VirtConPort {
> +    DeviceState dev;
> +    VirtConPortInfo *info;
> +
> +    /* State for the chardevice associated with this port */
> +    CharDriverState *chr;

That should go to the port driver if needed.

> +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
> +
> +struct VirtConPortInfo {
> +    DeviceInfo qdev;
> +    virtcon_port_qdev_initfn init;
> +
> +    /* Callbacks for guest events */
> +    void (*guest_open)(VirtConPort *port);
> +    void (*guest_close)(VirtConPort *port);
> +
> +    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);

Maybe it is a good idea to pass a VirtConPortBuffer here instead of 
buf+len, especially when it becomes the port drivers job to do any 
buffering if needed.

cheers,
   Gerd
Amit Shah Sept. 30, 2009, 4:47 a.m. UTC | #2
On (Tue) Sep 29 2009 [20:04:36], Gerd Hoffmann wrote:
>
>> +typedef struct VirtIOConsole
>> +{
>
> You are reusing this struct name a few times.
> Better don't do that, it is confusing.

Yeah; I still have to go through the entire naming thing. I also really
think virtio-console-bus, virtio-console and virtio-console-port are
confusing. I thought it's better to do:

virtio-serial-bus (and the corresponding -device virtio-serial-pci)
virtio-console
virtio-serial-port
virtio-serial-vnc

Is that OK with all?

>> +static VirtIOConsole *virtio_console;
>
> Why do you need this?

Just to keep the current behaviour of restricting to one virtio-console
device. This can be later reworked to allow multiple devices.

>> +static void flush_queue(VirtConPort *port)
>> +{
>> +    VirtConPortBuffer *buf, *buf2;
>> +    uint8_t *outbuf;
>> +    size_t outlen, outsize;
>> +
>> +    /*
>> +     * If the app isn't interested in buffering packets till it's
>> +     * opened, just drop the data guest sends us till a connection is
>> +     * established.
>> +     */
>> +    if (!port->host_connected&&  !port->flush_buffers)
>> +        return;
>
> Hmm.  Who needs that buffering?  Only user seems to be the port driver  
> (patch 4/6).  So move the buffering (and the host_connected state  
> tracking) from the core to that driver?

The buffering could be used by other devices too I guess. The other two
users that we currently have, vnc (which is just a demo patch) and
console, don't need the buffering, but it's a general facility.

If more users need the buffering, the code could get duplicated so I
thought of keeping it here.

>> +/* Readiness of the guest to accept data on a port */
>> +static int vcon_can_read(void *opaque)
>
> int vcon_can_read(VirtConPort *port)

OK

>> +static VirtConBus *virtcon_bus;
>
> Why do you need this?

I could put this in the VirtIOConsole struct.

>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>> +{
>
>> +    if (port->chr) {
>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>> +                              port);
>
> Should be handled by the VirtConPort drivers.

There are two reasons why I didn't put this there: virtio-console as
well as virtio-serial-port will need char drivers. There could be others
as well.

Also, some virtio-specific checks are done in vcon_can_read. So it's
better this is put in the common code.

>> +    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
>> +        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
>> +         * We anyway use up that much space for the bitmap and it
>> +         * simplifies some calculations
>> +         */
>> +        return NULL;
>> +    }
>
> Huh?  Runtime check for a compile-time constant?

Yeah; bad enough it was this way but I now think I can get rid of this
entirely as I don't depend on a ports_map.

>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>> +    virtcon_bus_new(dev);
>
> s->bus = virtcon_bus_new(dev);
> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for  
> backward compat */
> qdev_prop_set_*(port0, ...);
> qdev_init(port0);
>
> Or does that happen somewhere else?

I'm nowhere assuming now any port number - function mapping and so
spawning a virtio-console on port#0 is not necessary.

And yeah; as mentioned previously, I'll put the bus into VirtIOConsole
so that it doesn't have to be static.

>> +typedef struct VirtConBus VirtConBus;
>> +typedef struct VirtConPort VirtConPort;
>> +typedef struct VirtConPortInfo VirtConPortInfo;
>> +
>> +typedef struct VirtConDevice {
>> +    DeviceState qdev;
>> +    VirtConPortInfo *info;
>> +} VirtConDevice;
>
> Leftover from old patch version?

You mean this should not be in the .h? Yeah.

>> + * This is the state that's shared between all the ports.  Some of the
>> + * state is configurable via command-line options. Some of it can be
>> + * set by individual devices in their initfn routines. Some of the
>> + * state is set by the generic qdev device init routine.
>> + */
>> +struct VirtConPort {
>> +    DeviceState dev;
>> +    VirtConPortInfo *info;
>> +
>> +    /* State for the chardevice associated with this port */
>> +    CharDriverState *chr;
>
> That should go to the port driver if needed.

Wrote about the char driver above.

>> +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
>> +
>> +struct VirtConPortInfo {
>> +    DeviceInfo qdev;
>> +    virtcon_port_qdev_initfn init;
>> +
>> +    /* Callbacks for guest events */
>> +    void (*guest_open)(VirtConPort *port);
>> +    void (*guest_close)(VirtConPort *port);
>> +
>> +    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);
>
> Maybe it is a good idea to pass a VirtConPortBuffer here instead of  
> buf+len, especially when it becomes the port drivers job to do any  
> buffering if needed.

Also touched upon above.

Thanks,
		Amit
Gerd Hoffmann Sept. 30, 2009, 8:59 a.m. UTC | #3
> virtio-serial-bus (and the corresponding -device virtio-serial-pci)
> virtio-console
> virtio-serial-port
> virtio-serial-vnc
>
> Is that OK with all?

Looks fine to me.

>>> +static VirtIOConsole *virtio_console;
>>
>> Why do you need this?
>
> Just to keep the current behaviour of restricting to one virtio-console
> device. This can be later reworked to allow multiple devices.

I'd drop the limit right from the start.

>>> +static void flush_queue(VirtConPort *port)
>>> +{
>>> +    VirtConPortBuffer *buf, *buf2;
>>> +    uint8_t *outbuf;
>>> +    size_t outlen, outsize;
>>> +
>>> +    /*
>>> +     * If the app isn't interested in buffering packets till it's
>>> +     * opened, just drop the data guest sends us till a connection is
>>> +     * established.
>>> +     */
>>> +    if (!port->host_connected&&   !port->flush_buffers)
>>> +        return;
>>
>> Hmm.  Who needs that buffering?  Only user seems to be the port driver
>> (patch 4/6).  So move the buffering (and the host_connected state
>> tracking) from the core to that driver?
>
> The buffering could be used by other devices too I guess. The other two
> users that we currently have, vnc (which is just a demo patch) and
> console, don't need the buffering, but it's a general facility.
>
> If more users need the buffering, the code could get duplicated so I
> thought of keeping it here.

I'd suggest to look what do do when another user which wants buffering 
comes along.  Might be the requirements are different, so it wouldn't 
work anyway.

Also when you pass around pointers to VirtConPortBuffer the port drivers 
can implement buffering very easily.

>>> +static VirtConBus *virtcon_bus;
>>
>> Why do you need this?
>
> I could put this in the VirtIOConsole struct.

Yes, please.  You'll need that for multiple devices anyway.

>>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>>> +{
>>
>>> +    if (port->chr) {
>>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>>> +                              port);
>>
>> Should be handled by the VirtConPort drivers.
>
> There are two reasons why I didn't put this there: virtio-console as
> well as virtio-serial-port will need char drivers. There could be others
> as well.

They are handled differently though.  I think console will not do any 
buffering at all, whereas serial-port provides the option to do buffering.

There is also the option to stick both console and serial-port 
implementations into the same source file and make them share the 
functions which handle the chardev interaction.

> Also, some virtio-specific checks are done in vcon_can_read. So it's
> better this is put in the common code.

Having a helper function for the port drivers which fill the role 
vcon_can_read() fills right now certainly makes sense.

The naming is bad though.  vcon_can_read() is named from the chardevs 
point of view.  It actually checks how many bytes can be written to the 
virtio ring.  It should be named accordingly.

Also the parameters should make sense from a port drivers point of view, 
not tweaked for chardev.  Port drivers which don't need a chardev at all 
might want to use this too.  Port drivers which link to a chardev can 
have a trivial one-liner wrapper function to map chardev callbacks into 
virtio-serial helper functions.

>>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>>> +    virtcon_bus_new(dev);
>>
>> s->bus = virtcon_bus_new(dev);
>> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for
>> backward compat */
>> qdev_prop_set_*(port0, ...);
>> qdev_init(port0);
>>
>> Or does that happen somewhere else?
>
> I'm nowhere assuming now any port number - function mapping and so
> spawning a virtio-console on port#0 is not necessary.

How old kernels which expect a single console are supposed to work if 
you don't create one?

cheers,
   Gerd
Amit Shah Sept. 30, 2009, 3:55 p.m. UTC | #4
On (Wed) Sep 30 2009 [10:59:47], Gerd Hoffmann wrote:
>> virtio-serial-bus (and the corresponding -device virtio-serial-pci)
>> virtio-console
>> virtio-serial-port
>> virtio-serial-vnc
>>
>> Is that OK with all?
>
> Looks fine to me.

OK; that'll replace the current virtio-console-pci device but since it's
not in use yet (from the command line explicity) that doesn't break
anything.

>>>> +static VirtIOConsole *virtio_console;
>>>
>>> Why do you need this?
>>
>> Just to keep the current behaviour of restricting to one virtio-console
>> device. This can be later reworked to allow multiple devices.
>
> I'd drop the limit right from the start.

BTW the kernel too doesn't support multiple devices so far.

>>>> +static void flush_queue(VirtConPort *port)
>>>> +{
>>>> +    VirtConPortBuffer *buf, *buf2;
>>>> +    uint8_t *outbuf;
>>>> +    size_t outlen, outsize;
>>>> +
>>>> +    /*
>>>> +     * If the app isn't interested in buffering packets till it's
>>>> +     * opened, just drop the data guest sends us till a connection is
>>>> +     * established.
>>>> +     */
>>>> +    if (!port->host_connected&&   !port->flush_buffers)
>>>> +        return;
>>>
>>> Hmm.  Who needs that buffering?  Only user seems to be the port driver
>>> (patch 4/6).  So move the buffering (and the host_connected state
>>> tracking) from the core to that driver?
>>
>> The buffering could be used by other devices too I guess. The other two
>> users that we currently have, vnc (which is just a demo patch) and
>> console, don't need the buffering, but it's a general facility.
>>
>> If more users need the buffering, the code could get duplicated so I
>> thought of keeping it here.
>
> I'd suggest to look what do do when another user which wants buffering  
> comes along.  Might be the requirements are different, so it wouldn't  
> work anyway.
>
> Also when you pass around pointers to VirtConPortBuffer the port drivers  
> can implement buffering very easily.

It will mostly be duplicated. Right now, I pass on an entire message as
was passed on by the kernel in one write() request. If I start passing
around buffers, it'll be incomplete messages (eg, an 8K write request in
the kernel will be split in two buffers of 4K each -- or three buffers
with the last one containing a few bytes). I don't think I want the ports
to worry about that. The ports should just get the entire message.

In fact, it is this way because vnc needs the entire message handed out
to it in one go.

So I don't think I want to expose the buffers to individual devices.

>>>> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
>>>> +{
>>>
>>>> +    if (port->chr) {
>>>> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
>>>> +                              port);
>>>
>>> Should be handled by the VirtConPort drivers.
>>
>> There are two reasons why I didn't put this there: virtio-console as
>> well as virtio-serial-port will need char drivers. There could be others
>> as well.
>
> They are handled differently though.  I think console will not do any  
> buffering at all, whereas serial-port provides the option to do 
> buffering.

But the buffering is independent of the char drivers.

> There is also the option to stick both console and serial-port  
> implementations into the same source file and make them share the  
> functions which handle the chardev interaction.

Sure; that's a possibility.

>> Also, some virtio-specific checks are done in vcon_can_read. So it's
>> better this is put in the common code.
>
> Having a helper function for the port drivers which fill the role  
> vcon_can_read() fills right now certainly makes sense.

Yeah; if the char stuff is to be taken out of the bus file.

> The naming is bad though.  vcon_can_read() is named from the chardevs  
> point of view.  It actually checks how many bytes can be written to the  
> virtio ring.  It should be named accordingly.
>
> Also the parameters should make sense from a port drivers point of view,  
> not tweaked for chardev.  Port drivers which don't need a chardev at all  
> might want to use this too.  Port drivers which link to a chardev can  
> have a trivial one-liner wrapper function to map chardev callbacks into  
> virtio-serial helper functions.

Yeah; again, if the chardev stuff is to be moved out of the bus file,
then yes.

>>>> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
>>>> +    virtcon_bus_new(dev);
>>>
>>> s->bus = virtcon_bus_new(dev);
>>> port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for
>>> backward compat */
>>> qdev_prop_set_*(port0, ...);
>>> qdev_init(port0);
>>>
>>> Or does that happen somewhere else?
>>
>> I'm nowhere assuming now any port number - function mapping and so
>> spawning a virtio-console on port#0 is not necessary.
>
> How old kernels which expect a single console are supposed to work if  
> you don't create one?

Basically I've now dropped the old -virtioconsole argument.

So one has to do:

-device virtio-console-pci -device virtioconsole ...

to start a console.

		Amit
Gerd Hoffmann Sept. 30, 2009, 6:39 p.m. UTC | #5
> BTW the kernel too doesn't support multiple devices so far.

Needs fixing too ;)

>> I'd suggest to look what do do when another user which wants buffering
>> comes along.  Might be the requirements are different, so it wouldn't
>> work anyway.
>>
>> Also when you pass around pointers to VirtConPortBuffer the port drivers
>> can implement buffering very easily.
>
> It will mostly be duplicated. Right now, I pass on an entire message as
> was passed on by the kernel in one write() request. If I start passing
> around buffers, it'll be incomplete messages (eg, an 8K write request in
> the kernel will be split in two buffers of 4K each -- or three buffers
> with the last one containing a few bytes). I don't think I want the ports
> to worry about that. The ports should just get the entire message.

Sure.  I think there is a misunderstanding here.  I meant only the 
"buffer messages when unconnected" thing.  Reassembling the messages in 
the core and forward only complete messages to the ports is fine.

> So I don't think I want to expose the buffers to individual devices.

My idea was to pass the (reassembled) message to the port drivers in a 
form they can just stick into a list without memcpy()ing the message in 
case they want to keep it.  Maybe to buffer it because the chardev is 
unconnected.  Maybe just keep the most recent message because it 
contains some kind of guest status.

VirtConPortBuffer looked like it could do the job.  Well, not perfectly, 
it has extra status which is needed only by the core for reassembling 
messages.

>> They are handled differently though.  I think console will not do any
>> buffering at all, whereas serial-port provides the option to do
>> buffering.
>
> But the buffering is independent of the char drivers.

See above ;)

> Basically I've now dropped the old -virtioconsole argument.
>
> So one has to do:
>
> -device virtio-console-pci -device virtioconsole ...
>
> to start a console.

If you do '-device virtio-console-pci -device virtio-port' (i.e. no 
console) and boot a old guest kernel which expects a (single) console 
being there, what will happen?

cheers,
   Gerd
Amit Shah Oct. 1, 2009, 4:54 a.m. UTC | #6
On (Wed) Sep 30 2009 [20:39:34], Gerd Hoffmann wrote:
>> BTW the kernel too doesn't support multiple devices so far.
>
> Needs fixing too ;)

Agreed :-) I've fixed multiple devices in qemu.

>>> I'd suggest to look what do do when another user which wants buffering
>>> comes along.  Might be the requirements are different, so it wouldn't
>>> work anyway.
>>>
>>> Also when you pass around pointers to VirtConPortBuffer the port drivers
>>> can implement buffering very easily.
>>
>> It will mostly be duplicated. Right now, I pass on an entire message as
>> was passed on by the kernel in one write() request. If I start passing
>> around buffers, it'll be incomplete messages (eg, an 8K write request in
>> the kernel will be split in two buffers of 4K each -- or three buffers
>> with the last one containing a few bytes). I don't think I want the ports
>> to worry about that. The ports should just get the entire message.
>
> Sure.  I think there is a misunderstanding here.  I meant only the  
> "buffer messages when unconnected" thing.  Reassembling the messages in  
> the core and forward only complete messages to the ports is fine.

OK; so in that case, passing a ptr to the buffer and its length is fine,
no?

>> Basically I've now dropped the old -virtioconsole argument.
>>
>> So one has to do:
>>
>> -device virtio-console-pci -device virtioconsole ...
>>
>> to start a console.
>
> If you do '-device virtio-console-pci -device virtio-port' (i.e. no  
> console) and boot a old guest kernel which expects a (single) console  
> being there, what will happen?

OK -- I get what you're saying now. However, I don't see any problem
here. If there is no virtioconsole specified to qemu, there's no reason
to expect a console in the guest. That was the case in the past and is
the case now as well. The difference is earlier, when probe() in the
guest was called, it definitely meant the existence of a console. Now,
even if probe() is invoked, it doesn't mean a console was found. But
this hardly is a concern.

Also, the way to attach to a virtioconsole is by spawning a tty on
/dev/hvc0. If no -virtioconsole is specified on the host command line,
no hvc device is spawned on the guest. So things are as they are.

		Amit
Gerd Hoffmann Oct. 1, 2009, 8:38 a.m. UTC | #7
Hi,

>> Sure.  I think there is a misunderstanding here.  I meant only the
>> "buffer messages when unconnected" thing.  Reassembling the messages in
>> the core and forward only complete messages to the ports is fine.
>
> OK; so in that case, passing a ptr to the buffer and its length is fine,
> no?

Will work.  We need some clear rules for buffer memory though.  Possible 
ways to deal with it:

   (1) core owns the buffer.  If the port driver wants to keep the
       content it has to memcpy() it to own memory.
   (2) have_data() callback transfers buffer ownership from core to
       the port driver.  It is the port drivers job to free the memory
       when it doesn't need it any more.
   (3) reference-count the buffers.

For (1) + (2) both buffer struct and ptr+len will work.

For (3) ptr+len wouldn't work though, you'll need some struct containing 
ptr, len, refcount and helper functions to get/put buffers.

>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
>> console) and boot a old guest kernel which expects a (single) console
>> being there, what will happen?
>
> OK -- I get what you're saying now. However, I don't see any problem
> here. If there is no virtioconsole specified to qemu, there's no reason
> to expect a console in the guest. That was the case in the past and is
> the case now as well. The difference is earlier, when probe() in the
> guest was called, it definitely meant the existence of a console. Now,
> even if probe() is invoked, it doesn't mean a console was found. But
> this hardly is a concern.

True for new guest kernels, they simply don't create a hvc.
Question is what *old* guest kernels will do in that case.

cheers,
   Gerd
Amit Shah Oct. 1, 2009, 8:56 a.m. UTC | #8
On (Thu) Oct 01 2009 [10:38:18], Gerd Hoffmann wrote:
>   Hi,
>
>>> Sure.  I think there is a misunderstanding here.  I meant only the
>>> "buffer messages when unconnected" thing.  Reassembling the messages in
>>> the core and forward only complete messages to the ports is fine.
>>
>> OK; so in that case, passing a ptr to the buffer and its length is fine,
>> no?
>
> Will work.  We need some clear rules for buffer memory though.  Possible  
> ways to deal with it:
>
>   (1) core owns the buffer.  If the port driver wants to keep the
>       content it has to memcpy() it to own memory.
>   (2) have_data() callback transfers buffer ownership from core to
>       the port driver.  It is the port drivers job to free the memory
>       when it doesn't need it any more.
>   (3) reference-count the buffers.
>
> For (1) + (2) both buffer struct and ptr+len will work.
>
> For (3) ptr+len wouldn't work though, you'll need some struct containing  
> ptr, len, refcount and helper functions to get/put buffers.

Yeah; Currently it's only (1). But I'm now thinking of doing (2). One
concern Yaniv raised was a port driver may not consume all the data
right away, so it's better to let the port do the buffer management.

>>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
>>> console) and boot a old guest kernel which expects a (single) console
>>> being there, what will happen?
>>
>> OK -- I get what you're saying now. However, I don't see any problem
>> here. If there is no virtioconsole specified to qemu, there's no reason
>> to expect a console in the guest. That was the case in the past and is
>> the case now as well. The difference is earlier, when probe() in the
>> guest was called, it definitely meant the existence of a console. Now,
>> even if probe() is invoked, it doesn't mean a console was found. But
>> this hardly is a concern.
>
> True for new guest kernels, they simply don't create a hvc.
> Question is what *old* guest kernels will do in that case.

If the guest kernel doesn't support the new virtio feature
VIRTIO_F_MULTIPORT, then we disable all this functionality and only
allow one port. That one port has to be the console port. I've tested
this combination, btw.

However, it's possible that some of the checks got lost in the latest
rework and that port 0 isn't actually a console port. I'll go through
init code again to ensure this. Thanks for explaining the scenario!

		Amit
Amit Shah Oct. 1, 2009, 10:48 a.m. UTC | #9
On (Thu) Oct 01 2009 [14:26:20], Amit Shah wrote:
> >>> If you do '-device virtio-console-pci -device virtio-port' (i.e. no
> >>> console) and boot a old guest kernel which expects a (single) console
> >>> being there, what will happen?
> >>
> >> OK -- I get what you're saying now. However, I don't see any problem
> >> here. If there is no virtioconsole specified to qemu, there's no reason
> >> to expect a console in the guest. That was the case in the past and is
> >> the case now as well. The difference is earlier, when probe() in the
> >> guest was called, it definitely meant the existence of a console. Now,
> >> even if probe() is invoked, it doesn't mean a console was found. But
> >> this hardly is a concern.
> >
> > True for new guest kernels, they simply don't create a hvc.
> > Question is what *old* guest kernels will do in that case.
> 
> If the guest kernel doesn't support the new virtio feature
> VIRTIO_F_MULTIPORT, then we disable all this functionality and only
> allow one port. That one port has to be the console port. I've tested
> this combination, btw.
> 
> However, it's possible that some of the checks got lost in the latest
> rework and that port 0 isn't actually a console port. I'll go through
> init code again to ensure this. Thanks for explaining the scenario!

There are a couple of problems with what I said:
- I had to make some changes in the kernel driver because hvc didn't
  work as I expected. If initial consoles were spawned (currently only
  on s390 and powerpc), new ports have to arrive in the same order as
  init consoles were spawned.

  This means /dev/console will be bound to the first console port that
  gets initialised. It's desirable to always have port0 get that
  distinction.

- We won't know if the guest supports multiple ports till the guest
  driver does a probe(). That's much much later than when port drivers
  in qemu are initialised.

So we will have to spawn a console port at id 0 when the bus is
initialised.

There are a couple of problems though:
- How to identify which chardev to associate the port#0 with?
- If there is a virtioconsole device specified on the command line,
  should that be used as port0? Or should that mean we spawn two
  consoles?

I guess for both these cases, some special command line tweaks will be
needed? Or keep the old '-virtioconsole' parameter and put that up as
port0?

		Amit
Gerd Hoffmann Oct. 1, 2009, 12:15 p.m. UTC | #10
Hi,

> So we will have to spawn a console port at id 0 when the bus is
> initialised.
>
> There are a couple of problems though:
> - How to identify which chardev to associate the port#0 with?

I'd suggest to give the serial-bus a chardev attribute, which is then 
simply passed through, i.e.

   -device virtio-serial-bus,chardev=<name>

automatically creates a virtioconsole with port=0 and chardev=<name> on 
the newly created bus.

> - If there is a virtioconsole device specified on the command line,
>    should that be used as port0? Or should that mean we spawn two
>    consoles?

Two consoles.

> I guess for both these cases, some special command line tweaks will be
> needed? Or keep the old '-virtioconsole' parameter and put that up as
> port0?

See above.

Keeping -virtioconsole for backward compatibility is easy, it would 
basically create a chardev with a virtio<nr> label as it does today, 
then create virtio-serial-bus with chardev=virtio<nr>.

cheers,
   Gerd
Amit Shah Oct. 7, 2009, 9:25 a.m. UTC | #11
On (Thu) Oct 01 2009 [14:15:53], Gerd Hoffmann wrote:
>   Hi,
>
>> So we will have to spawn a console port at id 0 when the bus is
>> initialised.
>>
>> There are a couple of problems though:
>> - How to identify which chardev to associate the port#0 with?
>
> I'd suggest to give the serial-bus a chardev attribute, which is then  
> simply passed through, i.e.
>
>   -device virtio-serial-bus,chardev=<name>
>
> automatically creates a virtioconsole with port=0 and chardev=<name> on  
> the newly created bus.

Hm, this looks weird. Because on one hand we're talking about decoupling
the char driver from the core (virtio-serial-bus) and here we're
actually attaching a char driver to the bus.

>> I guess for both these cases, some special command line tweaks will be
>> needed? Or keep the old '-virtioconsole' parameter and put that up as
>> port0?
>
> See above.
>
> Keeping -virtioconsole for backward compatibility is easy, it would  
> basically create a chardev with a virtio<nr> label as it does today,  
> then create virtio-serial-bus with chardev=virtio<nr>.

I prefer to remove the -virtioconsole argument because we won't be able
to specify the bus that's to be attached to.

Also, we can't say if a virtio-serial-bus is also specified. If one
is, we'd want the console to attach to that bus. If one isn't, we'd want
to spawn a new one in that case. But as per above, a virtio-serial-bus
should always spawn a virtio-console port.

I fear we'll end up in a mess of tricky if-else conditions.

And I can't think of a nice solution to the problem above right now.

		Amit
Gerd Hoffmann Oct. 7, 2009, 9:51 a.m. UTC | #12
On 10/07/09 11:25, Amit Shah wrote:
>>    -device virtio-serial-bus,chardev=<name>
>>
>> automatically creates a virtioconsole with port=0 and chardev=<name>  on
>> the newly created bus.
>
> Hm, this looks weird. Because on one hand we're talking about decoupling
> the char driver from the core (virtio-serial-bus) and here we're
> actually attaching a char driver to the bus.

We don't actually attach the chardev to the bus though.  It is just 
passed through to the auto-created console port #0.

But it looks a bit weird indeed.  I'm open to better suggestions to 
address the "port #0 must be console for backward compatibility reasons" 
issue.

>> Keeping -virtioconsole for backward compatibility is easy, it would
>> basically create a chardev with a virtio<nr>  label as it does today,
>> then create virtio-serial-bus with chardev=virtio<nr>.
>
> I prefer to remove the -virtioconsole argument because we won't be able
> to specify the bus that's to be attached to.

Oh, I thought it would create a new virtio-serial-bus (plus auto-created 
port0 console) unconditionally.  Just do enougth to keep existing users 
of the switch working.

If you want new features (i.e. two consoles ports attached to one 
virtio-serial-bus device) you must use the new syntax.

cheers,
   Gerd
Amit Shah Oct. 7, 2009, 10:06 a.m. UTC | #13
On (Wed) Oct 07 2009 [11:51:14], Gerd Hoffmann wrote:
> On 10/07/09 11:25, Amit Shah wrote:
>>>    -device virtio-serial-bus,chardev=<name>
>>>
>>> automatically creates a virtioconsole with port=0 and chardev=<name>  on
>>> the newly created bus.
>>
>> Hm, this looks weird. Because on one hand we're talking about decoupling
>> the char driver from the core (virtio-serial-bus) and here we're
>> actually attaching a char driver to the bus.
>
> We don't actually attach the chardev to the bus though.  It is just  
> passed through to the auto-created console port #0.

Yes, but it's unituitive... and weird.

> But it looks a bit weird indeed.  I'm open to better suggestions to  
> address the "port #0 must be console for backward compatibility reasons"  
> issue.

Hm, me too. I'll think about this more.

>>> Keeping -virtioconsole for backward compatibility is easy, it would
>>> basically create a chardev with a virtio<nr>  label as it does today,
>>> then create virtio-serial-bus with chardev=virtio<nr>.
>>
>> I prefer to remove the -virtioconsole argument because we won't be able
>> to specify the bus that's to be attached to.
>
> Oh, I thought it would create a new virtio-serial-bus (plus auto-created  
> port0 console) unconditionally.  Just do enougth to keep existing users  
> of the switch working.
>
> If you want new features (i.e. two consoles ports attached to one  
> virtio-serial-bus device) you must use the new syntax.

So it's better overall to drop the old syntax altogether, right? It
could get easily confusing otherwise.

We can easily end up having:

-virtioconsole <chardev>
<auto-creates a bus and attaches a console port to it>

-device virtio-serial-pci,id=blah
<a second bus>

-device virtport,bus=blah.0

<and no way to connect a device to the bus that got created by
virtioconsole>

		Amit
Gerd Hoffmann Oct. 7, 2009, 11:33 a.m. UTC | #14
On 10/07/09 12:06, Amit Shah wrote:
>> Oh, I thought it would create a new virtio-serial-bus (plus auto-created
>> port0 console) unconditionally.  Just do enougth to keep existing users
>> of the switch working.
>>
>> If you want new features (i.e. two consoles ports attached to one
>> virtio-serial-bus device) you must use the new syntax.
>
> So it's better overall to drop the old syntax altogether, right? It
> could get easily confusing otherwise.
>
> We can easily end up having:
>
> -virtioconsole<chardev>
> <auto-creates a bus and attaches a console port to it>
>
> -device virtio-serial-pci,id=blah
> <a second bus>
>
> -device virtport,bus=blah.0
>
> <and no way to connect a device to the bus that got created by
> virtioconsole>

It isn't that bad.

First, the busses get names based on the bus type by default, i.e. when 
creating a scsi adapter without specifying id=seomthing the bus is 
simply named "scsi.0".  Likewise the -virtioconsole created bus would be 
"port.0" or simliar (depends on the name in BusInfo).

Second, the bus= argument is optional.  If not specified, qdev will pick 
the first bus of a matching type it finds.  So as long you have a single 
port/scsi/usb/... bus only you don't need bus= at all.  You can do:

  -virtioconsole <chardev> -device virtport,<args>

and it will work just fine (creating a bus with the autocreated console 
and additionally a virtport device attached to the same bus).

cheers,
   Gerd
Amit Shah Oct. 7, 2009, 11:42 a.m. UTC | #15
On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
> On 10/07/09 12:06, Amit Shah wrote:
>>> Oh, I thought it would create a new virtio-serial-bus (plus auto-created
>>> port0 console) unconditionally.  Just do enougth to keep existing users
>>> of the switch working.
>>>
>>> If you want new features (i.e. two consoles ports attached to one
>>> virtio-serial-bus device) you must use the new syntax.
>>
>> So it's better overall to drop the old syntax altogether, right? It
>> could get easily confusing otherwise.
>>
>> We can easily end up having:
>>
>> -virtioconsole<chardev>
>> <auto-creates a bus and attaches a console port to it>
>>
>> -device virtio-serial-pci,id=blah
>> <a second bus>
>>
>> -device virtport,bus=blah.0
>>
>> <and no way to connect a device to the bus that got created by
>> virtioconsole>
>
> It isn't that bad.
>
> First, the busses get names based on the bus type by default, i.e. when  
> creating a scsi adapter without specifying id=seomthing the bus is  
> simply named "scsi.0".  Likewise the -virtioconsole created bus would be  
> "port.0" or simliar (depends on the name in BusInfo).
>
> Second, the bus= argument is optional.  If not specified, qdev will pick  
> the first bus of a matching type it finds.  So as long you have a single  
> port/scsi/usb/... bus only you don't need bus= at all.  You can do:

The problem with this is that the management solution needs to know then
what is the default bus name (which could change if the code gets
updated).

And also there's the other problem of a console port spawning a bus
(which could end up spawning another console port at #0...)

So IMO it's better to leave that command line param out.

		Amit
Gerd Hoffmann Oct. 7, 2009, 1:06 p.m. UTC | #16
On 10/07/09 13:42, Amit Shah wrote:
> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>> Second, the bus= argument is optional.  If not specified, qdev will pick
>> the first bus of a matching type it finds.  So as long you have a single
>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>
> The problem with this is that the management solution needs to know then
> what is the default bus name (which could change if the code gets
> updated).

No problem.  Just don't use -virtioconsole.  Go with -device 
virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly 
name your devices (and thereby the buses too).

-virtioconsole should *really* be a pure backward compatibility thing. 
Use case:  You have a script starting qemu using -virtioconsole.  After 
upgrading qemu it should continue to work, i.e. create a device which 
the guest can use as before the upgrade and which is linked up to a 
chardev as it was before.

Anything which wants to use the new features can (and should) completely 
ignore -virtioconsole.  I just wanted to point out that mixing old and 
new style is *possible*.  It wasn't my intention to imply that I 
*recommend* doing that.

> And also there's the other problem of a console port spawning a bus
> (which could end up spawning another console port at #0...)

parse error.
I don't understand what problem you are trying to point out.

cheers,
   Gerd
Amit Shah Oct. 7, 2009, 1:53 p.m. UTC | #17
On (Wed) Oct 07 2009 [15:06:08], Gerd Hoffmann wrote:
> On 10/07/09 13:42, Amit Shah wrote:
>> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>>> Second, the bus= argument is optional.  If not specified, qdev will pick
>>> the first bus of a matching type it finds.  So as long you have a single
>>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>>
>> The problem with this is that the management solution needs to know then
>> what is the default bus name (which could change if the code gets
>> updated).
>
> No problem.  Just don't use -virtioconsole.  Go with -device  
> virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly  
> name your devices (and thereby the buses too).
>
> -virtioconsole should *really* be a pure backward compatibility thing.  
> Use case:  You have a script starting qemu using -virtioconsole.  After  
> upgrading qemu it should continue to work, i.e. create a device which  
> the guest can use as before the upgrade and which is linked up to a  
> chardev as it was before.
>
> Anything which wants to use the new features can (and should) completely  
> ignore -virtioconsole.  I just wanted to point out that mixing old and  
> new style is *possible*.  It wasn't my intention to imply that I  
> *recommend* doing that.

There should be some way of deprecating commands in qemu. Maybe in 1-2
release cycles.

>> And also there's the other problem of a console port spawning a bus
>> (which could end up spawning another console port at #0...)
>
> parse error.
> I don't understand what problem you are trying to point out.

Oh; I was stuck at the earlier suggestion made by you:

-device virtio-serial-pci,chardev=...

which implies -device virtioconsole (with a port at id 0)

and then someone also doing

-virtioconsole ...

which would end up creating another bus for this current backward compat
reason.

		Amit
Anthony Liguori Oct. 7, 2009, 2 p.m. UTC | #18
Gerd Hoffmann wrote:
> On 10/07/09 13:42, Amit Shah wrote:
>> On (Wed) Oct 07 2009 [13:33:57], Gerd Hoffmann wrote:
>>> Second, the bus= argument is optional.  If not specified, qdev will 
>>> pick
>>> the first bus of a matching type it finds.  So as long you have a 
>>> single
>>> port/scsi/usb/... bus only you don't need bus= at all.  You can do:
>>
>> The problem with this is that the management solution needs to know then
>> what is the default bus name (which could change if the code gets
>> updated).
>
> No problem.  Just don't use -virtioconsole.  Go with -device 
> virtio-serial-bus,id=... + -device virtport,bus=.. then and explicitly 
> name your devices (and thereby the buses too).
>
> -virtioconsole should *really* be a pure backward compatibility thing. 
> Use case:  You have a script starting qemu using -virtioconsole.  
> After upgrading qemu it should continue to work, i.e. create a device 
> which the guest can use as before the upgrade and which is linked up 
> to a chardev as it was before.
>
> Anything which wants to use the new features can (and should) 
> completely ignore -virtioconsole.  I just wanted to point out that 
> mixing old and new style is *possible*.  It wasn't my intention to 
> imply that I *recommend* doing that.

I agree.

Regards,

Anthony Liguori
Gerd Hoffmann Oct. 7, 2009, 2:03 p.m. UTC | #19
On 10/07/09 15:53, Amit Shah wrote:
> On (Wed) Oct 07 2009 [15:06:08], Gerd Hoffmann wrote:
> There should be some way of deprecating commands in qemu. Maybe in 1-2
> release cycles.

Yes, we should think about that, especially as the qdev switch brings 
alot of changes here.

> Oh; I was stuck at the earlier suggestion made by you:
>
> -device virtio-serial-pci,chardev=...
>
> which implies -device virtioconsole (with a port at id 0)
>
> and then someone also doing
>
> -virtioconsole ...
>
> which would end up creating another bus for this current backward compat
> reason.

Yes, mixing old and new style is good for surprises.
Better avoid that ;)

cheers,
   Gerd
diff mbox

Patch

diff --git a/Makefile.target b/Makefile.target
index 609015b..22a2644 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -173,7 +173,7 @@  obj-y = vl.o monitor.o pci.o isa_mmio.o machine.o \
         gdbstub.o gdbstub-xml.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
-obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
+obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-console-bus.o virtio-pci.o
 obj-$(CONFIG_KVM) += kvm.o kvm-all.o
 
 LIBS+=-lz
diff --git a/hw/pc.c b/hw/pc.c
index 240cfe0..70b1ce9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1463,15 +1463,6 @@  static void pc_init1(ram_addr_t ram_size,
 	extboot_init(info->bdrv, 1);
     }
 
-    /* Add virtio console devices */
-    if (pci_enabled) {
-        for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-            if (virtcon_hds[i]) {
-                pci_create_simple(pci_bus, -1, "virtio-console-pci");
-            }
-        }
-    }
-
 #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
     if (kvm_enabled()) {
         add_assigned_devices(pci_bus, assigned_devices, assigned_devices_index);
diff --git a/hw/qdev.c b/hw/qdev.c
index 43b1beb..07cb718 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -243,13 +243,9 @@  void qdev_free(DeviceState *dev)
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
     static int next_serial;
-    static int next_virtconsole;
+
     /* FIXME: This is a nasty hack that needs to go away.  */
-    if (strncmp(dev->info->name, "virtio", 6) == 0) {
-        return virtcon_hds[next_virtconsole++];
-    } else {
-        return serial_hds[next_serial++];
-    }
+    return serial_hds[next_serial++];
 }
 
 BusState *qdev_get_parent_bus(DeviceState *dev)
diff --git a/hw/virtio-console-bus.c b/hw/virtio-console-bus.c
new file mode 100644
index 0000000..a5b732a
--- /dev/null
+++ b/hw/virtio-console-bus.c
@@ -0,0 +1,738 @@ 
+/*
+ * A bus for connecting virtio-console ports
+ *
+ * Copyright (c) 2009 Red Hat, Inc.
+ *
+ * Author(s):
+ *  Amit Shah <amit.shah@redhat.com>
+ *
+ * Some earlier parts are:
+ *   Copyright IBM, Corp. 2008
+ * authored by
+ *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "hw.h"
+#include "monitor.h"
+#include "qemu-queue.h"
+#include "qemu-char.h"
+#include "sysbus.h"
+#include "virtio.h"
+#include "virtio-console.h"
+
+typedef struct VirtIOConsole
+{
+    VirtIODevice vdev;
+    VirtQueue *ivq, *ovq;
+
+    VirtConDevice* ports[MAX_VIRTIO_CONSOLE_PORTS];
+    struct virtio_console_config config;
+
+    uint32_t guest_features;
+} VirtIOConsole;
+
+/* This struct holds individual buffers received for each port */
+typedef struct VirtConPortBuffer {
+    QTAILQ_ENTRY(VirtConPortBuffer) next;
+
+    uint8_t *buf;
+
+    size_t len; /* length of the buffer */
+
+    /* The size of one write request as issued by the guest. The
+     * buffer could be split in this list but using the size value in
+     * the first buffer for each write we can identify complete
+     * writes
+     */
+    size_t size;
+} VirtConPortBuffer;
+
+
+static VirtIOConsole *virtio_console;
+
+static VirtConPort *get_port_from_id(VirtIOConsole *vcon, uint32_t id)
+{
+    VirtConDevice *dev;
+    VirtConPort *port;
+
+    if (id > MAX_VIRTIO_CONSOLE_PORTS)
+        return NULL;
+
+    dev = vcon->ports[id];
+    port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+    return port;
+}
+
+static uint32_t get_id_from_port(VirtConPort *port)
+{
+    return port->id;
+}
+
+static bool use_multiport(void)
+{
+    return virtio_console->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+}
+
+static bool is_internal(uint32_t flags)
+{
+    return flags & VIRTIO_CONSOLE_ID_INTERNAL;
+}
+
+static bool has_complete_data(VirtConPort *port)
+{
+    VirtConPortBuffer *buf;
+    size_t len, size;
+
+    len = 0;
+    size = 0;
+    QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+        if (!buf->size && buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) {
+            /* We have a buffer that's lost its way; just flush it */
+            return true;
+        }
+        if (size && buf->size) {
+            /* Start of the next write request */
+            return true;
+        }
+        if (buf->size) {
+            size = buf->size;
+        }
+        len += buf->len;
+        if (len == size) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static void flush_queue(VirtConPort *port)
+{
+    VirtConPortBuffer *buf, *buf2;
+    uint8_t *outbuf;
+    size_t outlen, outsize;
+
+    /*
+     * If the app isn't interested in buffering packets till it's
+     * opened, just drop the data guest sends us till a connection is
+     * established.
+     */
+    if (!port->host_connected && !port->flush_buffers)
+        return;
+
+    while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) {
+        if (!has_complete_data(port)) {
+            break;
+        }
+
+        buf = QTAILQ_FIRST(&port->unflushed_buffer_head);
+        if (!buf->size) {
+            /* This is a buf that didn't get consumed as part of a
+             * previous data stream. Bad thing, shouldn't
+             * happen. But let's handle it nonetheless
+             */
+            port->info->have_data(port, buf->buf, buf->len);
+            QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+            continue;
+        }
+
+        outlen = 0;
+        outsize = buf->size;
+        outbuf = qemu_mallocz(outsize);
+        QTAILQ_FOREACH_SAFE(buf, &port->unflushed_buffer_head, next, buf2) {
+            memcpy(outbuf + outlen, buf->buf, buf->len);
+            outlen += buf->len;
+            QTAILQ_REMOVE(&port->unflushed_buffer_head, buf, next);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+
+            if (outlen == outsize)
+                break;
+        }
+        if (port->host_connected) {
+            port->info->have_data(port, outbuf, outlen);
+        }
+        qemu_free(outbuf);
+    }
+}
+
+
+static size_t write_to_port(VirtConPort *port,
+                            const uint8_t *buf, size_t size, uint32_t flags)
+{
+    VirtQueue *vq = virtio_console->ivq;
+    VirtQueueElement elem;
+    size_t offset = 0;
+    size_t len = 0;
+
+    if (!virtio_queue_ready(vq)) {
+        return 0;
+    }
+
+    if (!use_multiport() && is_internal(flags)) {
+        return 0;
+    }
+
+    while (offset < size) {
+        struct virtio_console_header header;
+        int i, header_len;
+
+        header_len = use_multiport() ? sizeof(header) : 0;
+
+        if (!virtqueue_pop(vq, &elem)) {
+            break;
+        }
+        if (elem.in_sg[0].iov_len < header_len) {
+            /* We can't even store our port number in this buffer. Bug? */
+            qemu_error("virtio-console: size %zd less than expected\n",
+                    elem.in_sg[0].iov_len);
+            exit(1);
+        }
+        header.id = get_id_from_port(port);
+        header.flags = flags;
+        memcpy(elem.in_sg[0].iov_base, &header, header_len);
+
+        for (i = 0; offset < size && i < elem.in_num; i++) {
+            len = MIN(elem.in_sg[i].iov_len - header_len, size - offset);
+
+            memcpy(elem.in_sg[i].iov_base + header_len, buf + offset, len);
+            offset += len;
+            header_len = 0;
+        }
+        header_len = use_multiport() ? sizeof(header) : 0;
+        virtqueue_push(vq, &elem, len + header_len);
+    }
+    virtio_notify(&virtio_console->vdev, vq);
+    return offset;
+}
+
+static void send_control_event(VirtConPort *port,
+                               struct virtio_console_control *cpkt)
+{
+    write_to_port(port, (uint8_t *)cpkt, sizeof(*cpkt),
+                  VIRTIO_CONSOLE_ID_INTERNAL);
+}
+
+
+
+/* Functions for use inside qemu to open and read from/write to ports */
+int virtio_console_open(VirtConPort *port)
+{
+    struct virtio_console_control cpkt;
+
+    /* Don't allow opening an already-open port */
+    if (port->host_connected) {
+        return 0;
+    }
+
+    /* Send port open notification to the guest */
+    port->host_connected = true;
+    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+    cpkt.value = 1;
+    send_control_event(port, &cpkt);
+
+    /* Flush any buffers that were pending while the port was closed */
+    flush_queue(port);
+
+    return 0;
+}
+
+void virtio_console_close(VirtConPort *port)
+{
+    struct virtio_console_control cpkt;
+
+    port->host_connected = false;
+    cpkt.event = VIRTIO_CONSOLE_PORT_OPEN;
+    cpkt.value = 0;
+    send_control_event(port, &cpkt);
+}
+
+/*
+ * Individual ports call this function to write to the guest.
+ */
+size_t virtio_console_write(VirtConPort *port, const uint8_t *buf, size_t size)
+{
+    if (!port || !port->host_connected) {
+        return 0;
+    }
+    return write_to_port(port, buf, size, false);
+}
+
+
+/* Guest wants to notify us of some event */
+static void handle_control_message(VirtConPort *port,
+                                   struct virtio_console_control *cpkt)
+{
+    uint8_t *buffer;
+    size_t buffer_len;
+
+    switch(cpkt->event) {
+    case VIRTIO_CONSOLE_PORT_OPEN:
+        port->guest_connected = cpkt->value;
+        if (cpkt->value && port->info->guest_open) {
+            port->info->guest_open(port);
+        }
+        if (!cpkt->value && port->info->guest_close) {
+            port->info->guest_close(port);
+        }
+        break;
+    case VIRTIO_CONSOLE_PORT_NAME:
+        if (port->name) {
+            buffer_len = sizeof(*cpkt) + strlen(port->name) + 1;
+            buffer = qemu_malloc(buffer_len);
+
+            memcpy(buffer, cpkt, sizeof(*cpkt));
+            memcpy(buffer + sizeof(*cpkt), port->name, strlen(port->name));
+            buffer[buffer_len - 1] = 0;
+
+            write_to_port(port, buffer, buffer_len, VIRTIO_CONSOLE_ID_INTERNAL);
+            qemu_free(buffer);
+        }
+        /*
+         * Now that we know the guest asked for the port name, we're
+         * sure the guest has initialised whatever state is necessary
+         * for this port. Now's a good time to let the guest know if
+         * this port is a console port so that the guest can hook it
+         * up to hvc.
+         */
+        if (port->is_console) {
+            cpkt->event = VIRTIO_CONSOLE_CONSOLE_PORT;
+            cpkt->value = true;
+            send_control_event(port, cpkt);
+        }
+        break;
+    }
+}
+
+/* Guest wrote something to some port.
+ *
+ * Flush the data in the entire chunk that we received rather than
+ * splitting it into multiple buffers. VNC clients don't consume split
+ * buffers
+ */
+static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIOConsole *vcon;
+    VirtQueueElement elem;
+
+    vcon = DO_UPCAST(VirtIOConsole, vdev, vdev);
+
+    while (virtqueue_pop(vq, &elem)) {
+        VirtConPort *port;
+        VirtConPortBuffer *buf;
+        struct virtio_console_header header;
+        int header_len;
+
+        buf = qemu_mallocz(sizeof(*buf));
+
+        if (use_multiport()) {
+            header_len = sizeof(header);
+
+            memcpy(&header, elem.out_sg[0].iov_base, header_len);
+            port = get_port_from_id(vcon, header.id);
+            if (!port) {
+                qemu_free(buf);
+                goto next_buf;
+            }
+        } else {
+            header_len = 0;
+            port = get_port_from_id(vcon, 0);
+        }
+
+        /* The guest always sends only one sg */
+        buf->len = elem.out_sg[0].iov_len - header_len;
+        buf->buf = qemu_mallocz(buf->len);
+        memcpy(buf->buf, elem.out_sg[0].iov_base + header_len, buf->len);
+
+        if (use_multiport() && is_internal(header.flags)) {
+            handle_control_message(port,
+                                   (struct virtio_console_control *)buf->buf);
+            qemu_free(buf->buf);
+            qemu_free(buf);
+            goto next_buf;
+        }
+        /*
+         * A port may not have any handler registered for consuming the
+         * data that the guest sends or it may not have a chardev associated
+         * with it. Just ignore the data in that case
+         */
+        if (!port->info->have_data) {
+            goto next_buf;
+        }
+
+        QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
+        if (use_multiport()) {
+            /* Only the first buffer in a stream will have this
+             * set. This will help us identify the first buffer and
+             * the remaining buffers in the stream based on length
+             */
+            buf->size = header.size;
+        } else {
+            /* We always want to flush all the buffers in this case */
+            buf->size = buf->len;
+        }
+        flush_queue(port);
+    next_buf:
+        virtqueue_push(vq, &elem, elem.out_sg[0].iov_len);
+    }
+    virtio_notify(vdev, vq);
+}
+
+static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static uint32_t virtio_console_get_features(VirtIODevice *vdev)
+{
+    return 1 << VIRTIO_CONSOLE_F_MULTIPORT;
+}
+
+static void virtio_console_set_features(VirtIODevice *vdev, uint32_t features)
+{
+    virtio_console->guest_features = features;
+}
+
+/* Guest requested config info */
+static void virtio_console_get_config(VirtIODevice *vdev, uint8_t *config_data)
+{
+    VirtIOConsole *vcon;
+
+    vcon = DO_UPCAST(VirtIOConsole, vdev, vdev);
+    memcpy(config_data, &vcon->config, sizeof(struct virtio_console_config));
+}
+
+static void virtio_console_set_config(VirtIODevice *vdev,
+                                      const uint8_t *config_data)
+{
+    struct virtio_console_config config;
+
+    memcpy(&config, config_data, sizeof(config));
+}
+
+/* Readiness of the guest to accept data on a port */
+static int vcon_can_read(void *opaque)
+{
+    VirtConPort *port = opaque;
+    VirtQueue *vq = virtio_console->ivq;
+    int size, header_len;
+
+    if (use_multiport()) {
+        header_len = sizeof(struct virtio_console_header);
+    } else {
+        header_len = 0;
+    }
+
+    if (!virtio_queue_ready(vq) ||
+        !(virtio_console->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        virtio_queue_empty(vq)) {
+        return 0;
+    }
+    if (use_multiport() && !port->guest_connected) {
+        return 0;
+    }
+    size = TARGET_PAGE_SIZE;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    size = header_len + 1;
+    if (virtqueue_avail_bytes(vq, size, 0)) {
+        return size - header_len;
+    }
+    return 0;
+}
+
+/* Send data from a char device over to the guest */
+static void vcon_read(void *opaque, const uint8_t *buf, int size)
+{
+    VirtConPort *port = opaque;
+
+    virtio_console_write(port, buf, size);
+}
+
+static void vcon_event(void *opaque, int event)
+{
+    VirtConPort *port = opaque;
+
+    switch (event) {
+    case CHR_EVENT_OPENED: {
+        virtio_console_open(port);
+        break;
+    }
+    case CHR_EVENT_CLOSED:
+        virtio_console_close(port);
+        break;
+    }
+}
+
+static void set_active_in_map(uint32_t *map, uint32_t idx)
+{
+    int i;
+
+    i = idx / 32;
+    idx %= 32;
+
+    map[i] |= 1U << idx;
+}
+
+/*
+ * Get the guest_connected status after a migration since it's all
+ * packed in the same way as ports_map in the config space is packed.
+ *
+ * return VIRTIO_CONSOLE_BAD_ID if nothing was found in the map, else
+ * return the port number whose guest was connected before the
+ * migration
+ */
+static uint32_t find_next_active_in_map(uint32_t *map, unsigned int i)
+{
+    uint32_t port_nr;
+
+    port_nr = ffs(*map);
+    if (!port_nr) {
+        return VIRTIO_CONSOLE_BAD_ID;
+    }
+    /* We used ffs above */
+    port_nr--;
+
+    *map &= ~(1U << port_nr);
+
+    port_nr += i * 32;
+    return port_nr;
+}
+
+static void virtio_console_save(QEMUFile *f, void *opaque)
+{
+    VirtIOConsole *s = opaque;
+    VirtConPort *port;
+    uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32];
+    unsigned int i, nr_bufs;
+
+    /* The virtio device */
+    virtio_save(&s->vdev, f);
+    /* The config space */
+    qemu_put_be16s(f, &s->config.cols);
+    qemu_put_be16s(f, &s->config.rows);
+    qemu_put_be32s(f, &s->config.nr_active_ports);
+
+    /* Items in struct VirtIOConsole */
+    qemu_put_be32s(f, &s->guest_features);
+
+    /*
+     * Items in struct VirtConPort
+     *   guest_connected is a single bit in each port; pack all these
+     *   bits in uint32_t words and then send them across
+     */
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        guest_connected_map[i] = 0;
+    }
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        port = get_port_from_id(s, i);
+        if (port->guest_connected) {
+            set_active_in_map(guest_connected_map, i);
+        }
+    }
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        qemu_put_be32s(f, &guest_connected_map[i]);
+    }
+
+    /* All the pending buffers from active ports */
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        VirtConPortBuffer *buf;
+
+        nr_bufs = 0;
+        port = get_port_from_id(s, i);
+        QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+            nr_bufs++;
+        }
+        /* First the port number, then the nr of bufs and then the bufs */
+        qemu_put_be32s(f, &i);
+        qemu_put_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        QTAILQ_FOREACH(buf, &port->unflushed_buffer_head, next) {
+            qemu_put_be64s(f, &buf->len);
+            qemu_put_be64s(f, &buf->size);
+            qemu_put_buffer(f, buf->buf, buf->len);
+        }
+    }
+}
+
+static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
+{
+    VirtIOConsole *s = opaque;
+    VirtConPort *port;
+    uint32_t guest_connected_map[MAX_VIRTIO_CONSOLE_PORTS / 32];
+    unsigned int i;
+
+    if (version_id > 2)
+        return -EINVAL;
+
+    /* The virtio device */
+    virtio_load(&s->vdev, f);
+
+    if (version_id < 2)
+        return 0;
+
+    /* The config space */
+    qemu_get_be16s(f, &s->config.cols);
+    qemu_get_be16s(f, &s->config.rows);
+    s->config.nr_active_ports = qemu_get_be32(f);
+
+    /* Items in struct VirtIOConsole */
+    qemu_get_be32s(f, &virtio_console->guest_features);
+
+    /* Items in struct VirtConPort */
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        guest_connected_map[i] = qemu_get_be32(f);
+    }
+
+    for (i = 0; i < s->config.nr_active_ports / 32; i++) {
+        uint32_t port_nr, map;
+
+        port = get_port_from_id(s, i);
+        map = guest_connected_map[i];
+        while (1) {
+            port_nr = find_next_active_in_map(&map, i);
+            if (port_nr == VIRTIO_CONSOLE_BAD_ID) {
+                break;
+            }
+            port->guest_connected = true;
+        }
+    }
+
+    /* All the pending buffers from active ports */
+    for (i = 0; i < s->config.nr_active_ports; i++) {
+        VirtConPortBuffer *buf;
+        unsigned int nr, nr_bufs;
+
+        /* First the port number, then the nr of bufs and then the bufs */
+        qemu_get_be32s(f, &nr);
+        qemu_get_be32s(f, &nr_bufs);
+        if (!nr_bufs) {
+            continue;
+        }
+        port = get_port_from_id(s, nr);
+        for (; nr_bufs; nr_bufs--) {
+            buf = qemu_malloc(sizeof(*buf));
+
+            qemu_get_be64s(f, &buf->len);
+            qemu_get_be64s(f, &buf->size);
+            buf->buf = qemu_malloc(buf->len);
+            qemu_get_buffer(f, buf->buf, buf->len);
+            QTAILQ_INSERT_TAIL(&port->unflushed_buffer_head, buf, next);
+        }
+    }
+
+    return 0;
+}
+
+/* The virtio-console bus on top of which the ports will ride as devices */
+struct VirtConBus {
+    BusState qbus;
+//    uint32_t assigned;
+};
+static VirtConBus *virtcon_bus;
+
+static struct BusInfo virtcon_bus_info = {
+    .name      = "virtio-console-bus",
+    .size      = sizeof(VirtConBus),
+};
+
+static VirtConBus *virtcon_bus_new(DeviceState *dev)
+{
+    if (virtcon_bus) {
+        qemu_error("Can't create a second virtio-console bus\n");
+        return NULL;
+    }
+
+    virtcon_bus = FROM_QBUS(VirtConBus, qbus_create(&virtcon_bus_info, dev, NULL));
+    return virtcon_bus;
+}
+
+static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
+{
+    VirtConDevice *dev = DO_UPCAST(VirtConDevice, qdev, qdev);
+    VirtConPortInfo *info = DO_UPCAST(VirtConPortInfo, qdev, base);
+    VirtConPort *port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
+    int ret;
+
+    if (virtio_console->config.nr_active_ports == MAX_VIRTIO_CONSOLE_PORTS) {
+        qemu_error("virtio-console-bus: Maximum device limit reached\n");
+        return -1;
+    }
+
+    dev->info = info;
+
+    ret = info->init(dev);
+    if (ret) {
+        return ret;
+    }
+    QTAILQ_INIT(&port->unflushed_buffer_head);
+
+    if (port->chr) {
+        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
+                              port);
+    }
+    port->id = virtio_console->config.nr_active_ports++;
+    virtio_console->ports[port->id] = dev;
+    /* Send an update to the guest about this new port added */
+    virtio_notify_config(&virtio_console->vdev);
+
+    return ret;
+}
+
+void virtcon_port_qdev_register(VirtConPortInfo *info)
+{
+    info->qdev.init = virtcon_port_qdev_init;
+    info->qdev.bus_info = &virtcon_bus_info;
+    qdev_register(&info->qdev);
+}
+
+VirtIODevice *virtio_console_init(DeviceState *dev)
+{
+    VirtIOConsole *s;
+
+    if (virtio_console) {
+        /*
+         * Linux guests don't support more than one virtio-console devices
+         * at the moment
+         */
+        return NULL;
+    }
+
+    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
+        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
+         * We anyway use up that much space for the bitmap and it
+         * simplifies some calculations
+         */
+        return NULL;
+    }
+
+    s = (VirtIOConsole *)virtio_common_init("virtio-console",
+                                            VIRTIO_ID_CONSOLE,
+                                            sizeof(struct virtio_console_config),
+                                            sizeof(VirtIOConsole));
+
+    virtio_console = s;
+    s->vdev.get_features = virtio_console_get_features;
+    s->vdev.set_features = virtio_console_set_features;
+    s->vdev.get_config = virtio_console_get_config;
+    s->vdev.set_config = virtio_console_set_config;
+
+    /* Add queue for host to guest transfers */
+    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
+    /* Add queue for guest to host transfers */
+    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
+
+    register_savevm("virtio-console", -1, 2, virtio_console_save,
+                    virtio_console_load, s);
+
+    /* Spawn a new virtio-console bus on which the ports will ride as devices */
+    virtcon_bus_new(dev);
+
+    return &s->vdev;
+}
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 92c953c..8007279 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -1,10 +1,10 @@ 
 /*
  * Virtio Console Device
  *
- * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
- *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -16,131 +16,55 @@ 
 #include "virtio.h"
 #include "virtio-console.h"
 
-
 typedef struct VirtIOConsole
 {
-    VirtIODevice vdev;
-    VirtQueue *ivq, *ovq;
-    CharDriverState *chr;
+    VirtConPort port;
 } VirtIOConsole;
 
-static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
-{
-    return (VirtIOConsole *)vdev;
-}
-
-static void virtio_console_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-    VirtIOConsole *s = to_virtio_console(vdev);
-    VirtQueueElement elem;
-
-    while (virtqueue_pop(vq, &elem)) {
-        ssize_t len = 0;
-        int d;
-
-        for (d = 0; d < elem.out_num; d++) {
-            len += qemu_chr_write(s->chr, (uint8_t *)elem.out_sg[d].iov_base,
-                                  elem.out_sg[d].iov_len);
-        }
-        virtqueue_push(vq, &elem, len);
-        virtio_notify(vdev, vq);
-    }
-}
-
-static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
-{
-}
-
-static uint32_t virtio_console_get_features(VirtIODevice *vdev)
-{
-    return 0;
-}
-
-static int vcon_can_read(void *opaque)
+static size_t flush_buf(VirtConPort *port, const uint8_t *buf, size_t len)
 {
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-
-    if (!virtio_queue_ready(s->ivq) ||
-        !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
-        virtio_queue_empty(s->ivq))
-        return 0;
-
-    /* current implementations have a page sized buffer.
-     * We fall back to a one byte per read if there is not enough room.
-     * It would be cool to have a function that returns the available byte
-     * instead of checking for a limit */
-    if (virtqueue_avail_bytes(s->ivq, TARGET_PAGE_SIZE, 0))
-        return TARGET_PAGE_SIZE;
-    if (virtqueue_avail_bytes(s->ivq, 1, 0))
-        return 1;
-    return 0;
-}
-
-static void vcon_read(void *opaque, const uint8_t *buf, int size)
-{
-    VirtIOConsole *s = (VirtIOConsole *) opaque;
-    VirtQueueElement elem;
-    int offset = 0;
-
-    /* The current kernel implementation has only one outstanding input
-     * buffer of PAGE_SIZE. Nevertheless, this function is prepared to
-     * handle multiple buffers with multiple sg element for input */
-    while (offset < size) {
-        int i = 0;
-        if (!virtqueue_pop(s->ivq, &elem))
-                break;
-        while (offset < size && i < elem.in_num) {
-            int len = MIN(elem.in_sg[i].iov_len, size - offset);
-            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
-            offset += len;
-            i++;
-        }
-        virtqueue_push(s->ivq, &elem, size);
-    }
-    virtio_notify(&s->vdev, s->ivq);
+    return qemu_chr_write(port->chr, buf, len);
 }
 
-static void vcon_event(void *opaque, int event)
+static int vcon_port_initfn(VirtConDevice *dev)
 {
-    /* we will ignore any event for the time being */
-}
+    VirtConPort *port = DO_UPCAST(VirtConPort, dev, &dev->qdev);
 
-static void virtio_console_save(QEMUFile *f, void *opaque)
-{
-    VirtIOConsole *s = opaque;
+    port->info = dev->info;
 
-    virtio_save(&s->vdev, f);
-}
+    /*
+     * We're not interested in data the guest sends while nothing's
+     * connected on the host side. Just ignore it instead of saving it
+     * for later consumption
+     */
+    port->flush_buffers = 1;
 
-static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
-{
-    VirtIOConsole *s = opaque;
+    /* Tell the guest we're a console so it attaches us to an hvc console */
+    port->is_console = true;
 
-    if (version_id != 1)
-        return -EINVAL;
+    /*
+     * For console devices, a tty is spawned on /dev/hvc0 and our
+     * /dev/vconNN will never be opened. Set this here.
+     */
+    port->guest_connected = true;
 
-    virtio_load(&s->vdev, f);
     return 0;
 }
 
-VirtIODevice *virtio_console_init(DeviceState *dev)
+static VirtConPortInfo virtcon_console_info = {
+    .qdev.name     = "virtconsole",
+    .qdev.size     = sizeof(VirtConPort),
+    .init          = vcon_port_initfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", VirtConPort, chr),
+        DEFINE_PROP_STRING("name", VirtConPort, name),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtcon_console_register(void)
 {
-    VirtIOConsole *s;
-    s = (VirtIOConsole *)virtio_common_init("virtio-console",
-                                            VIRTIO_ID_CONSOLE,
-                                            0, sizeof(VirtIOConsole));
-    if (s == NULL)
-        return NULL;
-
-    s->vdev.get_features = virtio_console_get_features;
-
-    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
-    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
-
-    s->chr = qdev_init_chardev(dev);
-    qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
-
-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
-
-    return &s->vdev;
+    virtcon_port_qdev_register(&virtcon_console_info);
 }
+device_init(virtcon_console_register)
diff --git a/hw/virtio-console.h b/hw/virtio-console.h
index 84d0717..b0df6c5 100644
--- a/hw/virtio-console.h
+++ b/hw/virtio-console.h
@@ -2,9 +2,11 @@ 
  * Virtio Console Support
  *
  * Copyright IBM, Corp. 2008
+ * Copyright Red Hat, Inc. 2009
  *
  * Authors:
  *  Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ *  Amit Shah <amit.shah@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -13,7 +15,135 @@ 
 #ifndef _QEMU_VIRTIO_CONSOLE_H
 #define _QEMU_VIRTIO_CONSOLE_H
 
+#include <stdbool.h>
+#include "qdev.h"
+
+/* Interface shared between the guest kernel and qemu */
+
 /* The ID for virtio console */
 #define VIRTIO_ID_CONSOLE 3
 
+/* Invalid port number */
+#define VIRTIO_CONSOLE_BAD_ID		(~(uint32_t)0)
+
+/* Features supported */
+#define VIRTIO_CONSOLE_F_MULTIPORT	1
+
+struct virtio_console_config
+{
+    /*
+     * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
+     * isn't implemented here yet
+     */
+    uint16_t cols;
+    uint16_t rows;
+
+    uint32_t nr_active_ports;
+} __attribute__((packed));
+
+struct virtio_console_control
+{
+    uint16_t event;
+    uint16_t value;
+};
+
+struct virtio_console_header {
+    uint32_t id; /* Port id */
+    uint32_t flags; /* Some message between host and guest */
+    uint32_t size; /* Size that's sent with the first buffer of each stream */
+} __attribute__((packed));
+
+/* Messages between host and guest */
+#define VIRTIO_CONSOLE_ID_INTERNAL	(1 << 0)
+
+/* Some events for the internal messages (control packets) */
+#define VIRTIO_CONSOLE_PORT_OPEN	0
+#define VIRTIO_CONSOLE_PORT_NAME	1
+#define VIRTIO_CONSOLE_CONSOLE_PORT	2
+
+/* In-qemu interface */
+
+/* Max. virtio console ports per device */
+#define MAX_VIRTIO_CONSOLE_PORTS	64
+
+typedef struct VirtConBus VirtConBus;
+typedef struct VirtConPort VirtConPort;
+typedef struct VirtConPortInfo VirtConPortInfo;
+
+typedef struct VirtConDevice {
+    DeviceState qdev;
+    VirtConPortInfo *info;
+} VirtConDevice;
+
+/*
+ * This is the state that's shared between all the ports.  Some of the
+ * state is configurable via command-line options. Some of it can be
+ * set by individual devices in their initfn routines. Some of the
+ * state is set by the generic qdev device init routine.
+ */
+struct VirtConPort {
+    DeviceState dev;
+    VirtConPortInfo *info;
+
+    /* State for the chardevice associated with this port */
+    CharDriverState *chr;
+
+    /*
+     * This name is sent to the guest and exported via sysfs.
+     * The guest could create symlinks based on this information.
+     * The name is in the reverse fqdn format, like org.qemu.console.0
+     */
+    char *name;
+
+    /*
+     * This list holds buffers pushed by the guest in case the guest
+     * sent incomplete messages or the host connection was down and
+     * the device requested to cache the data.
+     */
+    QTAILQ_HEAD(, VirtConPortBuffer) unflushed_buffer_head;
+
+    /*
+     * This id helps identify ports between the guest and the host.
+     * The guest sends a "header" with this id with each data packet
+     * that it sends and the host can then find out which associated
+     * device to send out this data to
+     */
+    uint32_t id;
+
+    /*
+     * This boolean, when set, means "don't queue data that gets sent
+     * to this port when the host is not connected".
+     */
+    int flush_buffers;
+
+    /* Identify if this is a port that binds with hvc in the guest */
+    bool is_console;
+
+    /* Is the corresponding guest device open? */
+    bool guest_connected;
+    /* Is this device open for IO on the host? */
+    bool host_connected;
+};
+
+
+typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
+
+struct VirtConPortInfo {
+    DeviceInfo qdev;
+    virtcon_port_qdev_initfn init;
+
+    /* Callbacks for guest events */
+    void (*guest_open)(VirtConPort *port);
+    void (*guest_close)(VirtConPort *port);
+
+    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);
+};
+
+void virtcon_port_qdev_register(VirtConPortInfo *info);
+
+int virtio_console_open(VirtConPort *port);
+void virtio_console_close(VirtConPort *port);
+size_t virtio_console_write(VirtConPort *port, const uint8_t *buf, size_t size);
+
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 2f4291d..ca412bf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1604,14 +1604,6 @@  character to Control-t.
 @end table
 ETEXI
 
-DEF("virtioconsole", HAS_ARG, QEMU_OPTION_virtiocon, \
-    "-virtioconsole c\n" \
-    "                set virtio console\n")
-STEXI
-@item -virtioconsole @var{c}
-Set virtio console.
-ETEXI
-
 DEF("show-cursor", 0, QEMU_OPTION_show_cursor, \
     "-show-cursor    show cursor\n")
 STEXI
diff --git a/sysemu.h b/sysemu.h
index a8d1549..f5b287e 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -233,12 +233,6 @@  extern CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 
 extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 
-/* virtio consoles */
-
-#define MAX_VIRTIO_CONSOLES 1
-
-extern CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
-
 #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
 
 #ifdef HAS_AUDIO
diff --git a/vl.c b/vl.c
index 3b49c56..b5215b2 100644
--- a/vl.c
+++ b/vl.c
@@ -213,7 +213,6 @@  static int no_frame = 0;
 int no_quit = 0;
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
-CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
 #ifdef TARGET_I386
 int win2k_install_hack = 0;
 int rtc_td_hack = 0;
@@ -4690,8 +4689,6 @@  int main(int argc, char **argv, char **envp)
     int serial_device_index;
     const char *parallel_devices[MAX_PARALLEL_PORTS];
     int parallel_device_index;
-    const char *virtio_consoles[MAX_VIRTIO_CONSOLES];
-    int virtio_console_index;
     const char *loadvm = NULL;
     QEMUMachine *machine;
     const char *cpu_model;
@@ -4765,10 +4762,6 @@  int main(int argc, char **argv, char **envp)
         parallel_devices[i] = NULL;
     parallel_device_index = 0;
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++)
-        virtio_consoles[i] = NULL;
-    virtio_console_index = 0;
-
     monitor_devices[0] = "vc:80Cx24C";
     for (i = 1; i < MAX_MONITOR_DEVICES; i++) {
         monitor_devices[i] = NULL;
@@ -5228,14 +5221,6 @@  int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
-            case QEMU_OPTION_virtiocon:
-                if (virtio_console_index >= MAX_VIRTIO_CONSOLES) {
-                    fprintf(stderr, "qemu: too many virtio consoles\n");
-                    exit(1);
-                }
-                virtio_consoles[virtio_console_index] = optarg;
-                virtio_console_index++;
-                break;
             case QEMU_OPTION_parallel:
                 if (parallel_device_index >= MAX_PARALLEL_PORTS) {
                     fprintf(stderr, "qemu: too many parallel ports\n");
@@ -5827,20 +5812,6 @@  int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (devname && strcmp(devname, "none")) {
-            char label[32];
-            snprintf(label, sizeof(label), "virtcon%d", i);
-            virtcon_hds[i] = qemu_chr_open(label, devname, NULL);
-            if (!virtcon_hds[i]) {
-                fprintf(stderr, "qemu: could not open virtio console '%s'\n",
-                        devname);
-                exit(1);
-            }
-        }
-    }
-
     module_call_init(MODULE_INIT_DEVICE);
 
     if (watchdog) {
@@ -5968,14 +5939,6 @@  int main(int argc, char **argv, char **envp)
         }
     }
 
-    for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
-        const char *devname = virtio_consoles[i];
-        if (virtcon_hds[i] && devname) {
-            if (strstart(devname, "vc", 0))
-                qemu_chr_printf(virtcon_hds[i], "virtio console%d\r\n", i);
-        }
-    }
-
     if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
         fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",
                 gdbstub_dev);