[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple po
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication |
Date: |
Wed, 23 Sep 2009 15:13:40 +0530 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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
- [Qemu-devel] [PATCH 4/5] vnc: add a is_vnc_active() helper, (continued)
[Qemu-devel] Re: Multiple Port Support for virtio-console, Amit Shah, 2009/09/10
[Qemu-devel] Multiple port support for virtio-console, Amit Shah, 2009/09/22
- [Qemu-devel] [PATCH 1/4] char: Emit 'OPENED' events on char device open, Amit Shah, 2009/09/22
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Gerd Hoffmann, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication,
Amit Shah <=
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Gerd Hoffmann, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Amit Shah, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Gerd Hoffmann, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Amit Shah, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Gerd Hoffmann, 2009/09/23
- Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication, Amit Shah, 2009/09/23