From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Msr6k-0005Ec-GL for qemu-devel@nongnu.org; Wed, 30 Sep 2009 00:48:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Msr6f-0005AQ-EX for qemu-devel@nongnu.org; Wed, 30 Sep 2009 00:48:05 -0400 Received: from [199.232.76.173] (port=55509 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Msr6f-0005AG-BT for qemu-devel@nongnu.org; Wed, 30 Sep 2009 00:48:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1676) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Msr6e-0006zn-NR for qemu-devel@nongnu.org; Wed, 30 Sep 2009 00:48:01 -0400 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8U4lxwo016550 for ; Wed, 30 Sep 2009 00:47:59 -0400 Date: Wed, 30 Sep 2009 10:17:24 +0530 From: Amit Shah Subject: Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports Message-ID: <20090930044724.GA28188@amit-x200.redhat.com> References: <1254225888-17093-1-git-send-email-amit.shah@redhat.com> <1254225888-17093-2-git-send-email-amit.shah@redhat.com> <1254225888-17093-3-git-send-email-amit.shah@redhat.com> <1254225888-17093-4-git-send-email-amit.shah@redhat.com> <4AC24C34.2080609@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4AC24C34.2080609@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 (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