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

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

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