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

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

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