Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication

[Amit Shah <amit.shah@redhat.com>]

On (Wed) Sep 23 2009 [11:07:02], Gerd Hoffmann wrote:
>
>>   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.  */
>
> Please don't drop this comment.  The whole function is a nasty hack ;)

OK :-)

>> +VirtConBus *virtcon_bus_new(DeviceState *dev)
>> +{
>> +    if (virtcon_bus) {
>> +        fprintf(stderr, "Can't create a second virtio-console bus\n");
>> +        return NULL;
>> +    }
>> +    if (!dev) {
>> +        dev = qdev_create(NULL, "virtio-console-sysbus");
>> +        qdev_init(dev);
>> +    }
>
> Can this actually happen?  I think you'll allways have a virtio-console  
> device as parent, right?
>
> Looks like cut+pasted from isa-bus.c.  The ISA bus needs this for  
> PCI-less machines where no PCI-ISA bridge is present and thus we need a  
> isabus-sysbus bridge to hook up the ISA bus into the device tree.  Try  
> 'info qtree' on -M pc and -M isapc to see what I mean ;)

You're right; I've mostly picked up the bits from isa-bus. I've not done
any thinking yet.

>> +VirtConDevice *virtcon_create(const char *name)
>> +VirtConDevice *virtcon_create_simple(const char *name)
>
> These functions should get a VirtConBus passed in as first argument.
>
> Looks like cut+paste from ISA bus again.  The ISA bus is special here as  
> you never ever can have two ISA busses in a single machine.  That isn't  
> true in general though.

OK; I think receiving the bus argument might be helpful when we support
more than 1 device.

> Might also be you don't need these functions at all.  They usually used  
> in case device creation is hard-coded somewhere (like standard isa  
> devices present in every pc) or to keep old-style init functions working  
> in the new qdev world (isa_ne2000_init for example).  Devices which are  
> only ever created via -device don't take this code path ...

I think deprecating the old-style -virtioconsole is the better option.

>> +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>> +{
>> +    VirtConDevice *d = DO_UPCAST(VirtConDevice, qdev, dev);
>> +    if (&d->qdev) {
>> +      ;
>> +    }
>> +}
>
> print callback isn't mandatory.  If you don't want to print anything  
> just drop it.

Yeah; I thought I'd have something to be displayed here later.

>> +static int virtcon_bus_init(SysBusDevice *dev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static SysBusDeviceInfo virtcon_sysbus_info = {
>> +    .init = virtcon_bus_init,
>> +    .qdev.name  = "virtio-console-sysbus",
>> +    .qdev.size  = sizeof(SysBusDevice),
>> +    .qdev.no_user = 1,
>> +};
>> +
>> +static void virtcon_sysbus_register_devices(void)
>> +{
>> +    sysbus_register_withprop(&virtcon_sysbus_info);
>> +}
>
> See above.  I'm sure you don't need that.

Yeah; will drop.

>> +struct VirtIOConsolePort {
>> +    DeviceState dev;
>> +
>> +    VirtIOConsole *vcon;
>> +    CharDriverState *hd;
>
> This looks wrong.

We shouldn't have a charstate at all?

>> +    char *name;
>> +
>> +    QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head;
>> +
>> +    bool guest_connected;
>> +    bool host_connected;
>> +};
>
> Sticking a pointer to VirtConPortDeviceInfo here is probably handy.
> More consistent naming please.

Consistent naming for what?

I've only put the new bus stuff in the new file and this file has
largely remained as-is, with a few functions changed to accomodate the
new init methods.

>> +static int get_id_from_port(VirtIOConsolePort *port)
>> +{
>> +    uint32_t i;
>> +
>> +    for (i = 0; i<  MAX_VIRTIO_CONSOLE_PORTS; i++) {
>> +        if (port == port->vcon->ports[i]) {
>> +            return i;
>> +        }
>> +    }
>> +    return VIRTIO_CONSOLE_BAD_ID;
>> +}
>
> Just sick a id element into VirtIOConsolePort?

Yes, that's on the todo list.

>> +static bool has_complete_data(VirtIOConsolePort *port)
>> +{
>> +    VirtIOConsolePortBuffer *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 */
>
> Can this happen?  If not, assert() instead?

Shouldn't happen; but it's not a serious thing to error out instead.

>> +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t len)
>> +{
>> +    if (!port->hd) {
>> +        return 0;
>> +    }
>> +    return qemu_chr_write(port->hd, buf, len);
>
> port->info->data_for_you(port, buf, len);

OK; haven't yet seen how the charstate can be made transparent.

>> +static void flush_queue(VirtIOConsolePort *port)
>> +{
>> +    VirtIOConsolePortBuffer *buf, *buf2;
>> +    uint8_t *outbuf;
>> +    size_t outlen, outsize;
>> +
>> +    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
>> +             */
>
> If it shoudn't happen, then use assert().  If it triggers, find the bug.

The bug could actually be in the guest and not in qemu. Would be wrong
to penalise in that case, I guess.

>> +/* Guest wants to notify us of some event */
>> +static void handle_control_message(VirtIOConsolePort *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;
>
> port->info->guest_open() notify callback?

You mean handle it in an async path?

>> +static int vcon_port_initfn(VirtConDevice *dev)
>> +{
>> +    VirtIOConsolePort *port;
>> +
>> +    port = DO_UPCAST(VirtIOConsolePort, dev,&dev->qdev);
>> +
>> +    QTAILQ_INIT(&port->unflushed_buffer_head);
>> +
>> +    port->vcon = virtio_console;
>> +
>> +    qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event, port);
>
> Why have a chardev here?

1. Don't know how to make it transparent,
2. Don't know how to init these function handlers otherwise

>> +typedef struct VirtConPortDeviceInfo {
>> +    DeviceInfo qdev;
>> +    virtcon_port_qdev_initfn init;
>
> Stick in more function pointers here.  guest_open(), data_for_you(), ...
>
>
> Well.  The whole thing is still *way* to mixed up.  It should be cleanly  
> separated.

Yeah; I've only got it working with -device so far. So I guess I'll have
to bug you more to get this further into shape :-)

> You should be able to move the port driver(s) to a separate source file  
> without much trouble.  Only the port driver should deal with a chardev.  

Oh OK; maybe I understand what you're saying about the chardevs now.

> The virtio-console core should not care at all how the data is piped to 
> the (host side) users.  It just drives the ring, forwards events,  
> accepts data for the guest (via helper function), passes on data from  
> the guest (via callback in VirtConPortDeviceInfo).

Hm, let me think over this.

		Amit