[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, s
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports |
Date: |
Wed, 30 Sep 2009 10:17:24 +0530 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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
- [Qemu-devel] [PATCH 2/6] qdev: add string property., (continued)
- [Qemu-devel] [PATCH 2/6] qdev: add string property., Amit Shah, 2009/09/29
- [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports, Amit Shah, 2009/09/29
- [Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication, Amit Shah, 2009/09/29
- [Qemu-devel] [PATCH 5/6] vnc: add a is_vnc_active() helper, Amit Shah, 2009/09/29
- [Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard, Amit Shah, 2009/09/29
- Re: [Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard, Gerd Hoffmann, 2009/09/29
- Re: [Qemu-devel] [PATCH 6/6] vnc: Add a virtio-console-bus device to send / receive guest clipboard, Amit Shah, 2009/09/30
- Re: [Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication, Gerd Hoffmann, 2009/09/29
- Re: [Qemu-devel] [PATCH 4/6] virtio-console-port: Add a new device on the virtio-console-bus for generic host-guest communication, Nathan Baum, 2009/09/30
- Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports, Gerd Hoffmann, 2009/09/29
- Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports,
Amit Shah <=
- Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports, Gerd Hoffmann, 2009/09/30
- Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports, Amit Shah, 2009/09/30
- Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports, Gerd Hoffmann, 2009/09/30
Re: [Qemu-devel] [PATCH 1/6] char: Emit 'OPENED' events on char device open, Anthony Liguori, 2009/09/30
Re: [Qemu-devel] virtio-console-bus, multiport, virtio-console-port, Amit Shah, 2009/09/29