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

[Gerd Hoffmann <kraxel@redhat.com>]

> 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