From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Msv2W-0003yZ-Tv for qemu-devel@nongnu.org; Wed, 30 Sep 2009 05:00:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Msv2S-0003we-La for qemu-devel@nongnu.org; Wed, 30 Sep 2009 05:00:00 -0400 Received: from [199.232.76.173] (port=39296 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Msv2S-0003wY-Ek for qemu-devel@nongnu.org; Wed, 30 Sep 2009 04:59:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13008) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Msv2R-0002gt-Qm for qemu-devel@nongnu.org; Wed, 30 Sep 2009 04:59:56 -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 n8U8xpM7019138 for ; Wed, 30 Sep 2009 04:59:51 -0400 Message-ID: <4AC31E03.8000904@redhat.com> Date: Wed, 30 Sep 2009 10:59:47 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports 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> <20090930044724.GA28188@amit-x200.redhat.com> In-Reply-To: <20090930044724.GA28188@amit-x200.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 > virtio-serial-bus (and the corresponding -device virtio-serial-pci) > virtio-console > virtio-serial-port > virtio-serial-vnc > > Is that OK with all? Looks fine to me. >>> +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. I'd drop the limit right from the start. >>> +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. I'd suggest to look what do do when another user which wants buffering comes along. Might be the requirements are different, so it wouldn't work anyway. Also when you pass around pointers to VirtConPortBuffer the port drivers can implement buffering very easily. >>> +static VirtConBus *virtcon_bus; >> >> Why do you need this? > > I could put this in the VirtIOConsole struct. Yes, please. You'll need that for multiple devices anyway. >>> +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. They are handled differently though. I think console will not do any buffering at all, whereas serial-port provides the option to do buffering. There is also the option to stick both console and serial-port implementations into the same source file and make them share the functions which handle the chardev interaction. > Also, some virtio-specific checks are done in vcon_can_read. So it's > better this is put in the common code. Having a helper function for the port drivers which fill the role vcon_can_read() fills right now certainly makes sense. The naming is bad though. vcon_can_read() is named from the chardevs point of view. It actually checks how many bytes can be written to the virtio ring. It should be named accordingly. Also the parameters should make sense from a port drivers point of view, not tweaked for chardev. Port drivers which don't need a chardev at all might want to use this too. Port drivers which link to a chardev can have a trivial one-liner wrapper function to map chardev callbacks into virtio-serial helper functions. >>> + /* 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. How old kernels which expect a single console are supposed to work if you don't create one? cheers, Gerd