From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MqPtt-0004CN-Ir for qemu-devel@nongnu.org; Wed, 23 Sep 2009 07:20:45 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MqPto-00049p-J7 for qemu-devel@nongnu.org; Wed, 23 Sep 2009 07:20:44 -0400 Received: from [199.232.76.173] (port=59734 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MqPto-00049f-1s for qemu-devel@nongnu.org; Wed, 23 Sep 2009 07:20:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11924) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MqPtn-0001F5-HI for qemu-devel@nongnu.org; Wed, 23 Sep 2009 07:20:39 -0400 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8NBKbW9029503 for ; Wed, 23 Sep 2009 07:20:37 -0400 Message-ID: <4ABA0481.6090603@redhat.com> Date: Wed, 23 Sep 2009 13:20:33 +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> <4AB9E536.4050001@redhat.com> <20090923094340.GA27483@amit-x200.redhat.com> In-Reply-To: <20090923094340.GA27483@amit-x200.redhat.com> Content-Type: multipart/mixed; boundary="------------000605010206020801060905" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------000605010206020801060905 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit >>> +struct VirtIOConsolePort { >>> + DeviceState dev; >>> + >>> + VirtIOConsole *vcon; >>> + CharDriverState *hd; >> >> This looks wrong. > > We shouldn't have a charstate at all? Not here. More below ... >>> + 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? The structs. Pick a prefix (say VirtCon) and stick with that. Then for the bus implementation: VirtConBus (fine) VirtConPort (VirtIOConsolePort now) VirtConPortInfo (VirtConPortDeviceInfo now) The console port driver could name its state info this way: VirtConPortConsole (doesn't exist right now it seems ...). > 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. This is about more than just the init ... >>> +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. It indicates a bug somewhere though, doesn't it? I'd suggest to not paper over it. >>> +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. chardev should go into VirtConPortConsole. >> 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. Ah, ok. Fine then. Aborting qemu on guest bugs would be insane. >>> +/* 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? No, have a way to notify the port driver about the state change. >> 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. A port driver should look roughly like the attached one. That one does something completely different: Implement a watchdog ;) Warning: didn't even compile it. The console port driver would have the chardev instead of the timer in the driver state struct and would basically forward the data between the port and the chardev. HTH, Gerd --------------000605010206020801060905 Content-Type: text/x-csrc; name="wdt_vmchannel.c" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="wdt_vmchannel.c" #include "hw.h" #include "virtio-channel.h" typedef struct VirtConPortWatchdog { VirtConPort dev; QEMUTimer *timer; uint32_t timeout; } VirtConPortWatchdog; static void timer_expired(void *ptr) { VirtConPortWatchdog *s = ptr; watchdog_perform_action(); qemu_del_timer(s->timer); } static int watchdog_open(VirtConPort *dev) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + s->timeout * get_ticks_per_sec()); } static int watchdog_data(VirtConPort *dev, uint8_t *buf, int len) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + s->timeout * get_ticks_per_sec()); } static int watchdog_init(VirtConPort *dev) { VirtConPortWatchdog *s = DO_UPCAST(VirtConPortWatchdog, dev, dev); s->dev.name = "org.qemu.watchdog"; s->timer = qemu_new_timer(vm_clock, timer_expired, s); } VirtConPortInfo watchdog_info = { .qdev.name = "vmch-watchdog", .qdev.size = sizeof(VirtConPortWatchdog), .qdev.vmsd = /* todo */, .qdev.reset = /* todo */, .init = watchdog_init, .guest_open = watchdog_open, .data_for_you = watchdog_data, .qdev.props = (Property[]) { DEFINE_PROP_UINT32("timeout", VirtConPortWatchdog, timeout, 120), DEFINE_PROP_END_OF_LIST }, }; void watchdog_register(void) { virtcon_port_qdev_register(&watchdog_info); } device_init(watchdog_register) --------------000605010206020801060905--