From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MqNod-0006hI-JT for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:07:11 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MqNoZ-0006ge-QW for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:07:11 -0400 Received: from [199.232.76.173] (port=52900 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MqNoZ-0006gU-KV for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:07:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13587) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MqNoY-0005FQ-KB for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:07:07 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8N976Ax020802 for ; Wed, 23 Sep 2009 05:07:06 -0400 Message-ID: <4AB9E536.4050001@redhat.com> Date: Wed, 23 Sep 2009 11:07:02 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication References: <1253636627-12746-1-git-send-email-amit.shah@redhat.com> <1253636627-12746-2-git-send-email-amit.shah@redhat.com> <1253636627-12746-3-git-send-email-amit.shah@redhat.com> <1253636627-12746-4-git-send-email-amit.shah@redhat.com> In-Reply-To: <1253636627-12746-4-git-send-email-amit.shah@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org > 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 ;) > +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 ;) > +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. 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 ... > +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. > +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. > +struct VirtIOConsolePort { > + DeviceState dev; > + > + VirtIOConsole *vcon; > + CharDriverState *hd; This looks wrong. > + 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. > +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? > +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? > +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); > +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. > +/* 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? > +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? > +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. 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. 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). cheers, Gerd