From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MqOOb-0002dy-K0 for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:44:21 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MqOOW-0002cd-J3 for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:44:20 -0400 Received: from [199.232.76.173] (port=39790 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MqOOW-0002cW-2j for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:44:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29819) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MqOOV-0002uY-6U for qemu-devel@nongnu.org; Wed, 23 Sep 2009 05:44:15 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8N9iB0g009747 for ; Wed, 23 Sep 2009 05:44:11 -0400 Date: Wed, 23 Sep 2009 15:13:40 +0530 From: Amit Shah Subject: Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication Message-ID: <20090923094340.GA27483@amit-x200.redhat.com> 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> <4AB9E536.4050001@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4AB9E536.4050001@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org 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