From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LWBz6-0002P8-Lc for qemu-devel@nongnu.org; Sun, 08 Feb 2009 10:54:16 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LWBz5-0002O2-Ug for qemu-devel@nongnu.org; Sun, 08 Feb 2009 10:54:15 -0500 Received: from [199.232.76.173] (port=38583 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LWBz5-0002Nq-By for qemu-devel@nongnu.org; Sun, 08 Feb 2009 10:54:15 -0500 Received: from hall.aurel32.net ([88.191.82.174]:37154) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LWBz3-0004K7-8U for qemu-devel@nongnu.org; Sun, 08 Feb 2009 10:54:13 -0500 Date: Sun, 8 Feb 2009 16:53:58 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] [RESEND] [PATCH] Support for serial mouse emulation Message-ID: <20090208155358.GF11649@volta.aurel32.net> References: <44179.89.102.207.186.1228258258.squirrel@mail.netbsd.sk> <20081210180036.GG19379@volta.aurel32.net> <1233130204.5389.7.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <1233130204.5389.7.camel@localhost.localdomain> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lubomir Rintel Cc: qemu-devel@nongnu.org On Wed, Jan 28, 2009 at 09:10:04AM +0100, Lubomir Rintel wrote: > Hi, > > On Wed Dec 24 19:41:53 CET 2008, Aurelien Jarno wrote: > > Please find my comments inline. > > Thanks a lot for your review. I am attaching the revised version, > addressing most of your concerns. Thanks, applied > > This should be replaced by a Signed-off-by: line. > > Done > > >> Index: Makefile.target > >> =================================================================== > ... > >> -OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o > >> +OBJS+= ide.o pckbd.o ps2.o msmouse.o vga.o $(SOUND_HW) dma.o > > > ... (Lots of msmouse.o additions) ... > > > > Is it really necessary to enable that in all targets, especially when > > this is inited only in hw/pc.c? > > Well, given it's a character device, it's generally possible to use > everywhere a serial port can be used. I limited this to a single occurence > of adding to OBJS in the new version of patch though. > > >> +CharDriverState *msmouse_chr = NULL; > > > > A global variable is not a good idea. You should instead create > > structure containing this variable (and maybe others like > > CharDriverState) called for example VMMouseState. > > Originally I created a global variable because the serial mouse did not > work for me when attached at the serial port initialization time. Now I > figured out multiple mouse can be attached at once, though only one can be > selected at the time from the console. I am wondering if it would make > sense to somehow make the serial mouse have preference over the PS/2 mouse > by default, since it's most likely the way one would expect it to work? > > I did not create the structure with the state yet, since the only data I > keep for the instance of serial mouse is its serial port character device. > It's also consistent with original two-button mouse hardware using MS > protocol, since it was basically stateless. > > On the other hand, three button mouses keep state about the past state of > the third button, and use the two-button compatible protocol if the third > button is depressed and its state did not change, to retain compatibility > with two-button drivers, which is what this emulation doesn't support. > (See the comment around the relevant part of the code.) > > I'll create a separate structure for serial mice and support this > backward-compatible behavior if you want me to. > > >> + /* Only one serial mouse per instance is allowed */ > > > I don't think it is a good idea. I guess this is due to the use of a > > global variable. > > Right. Fixed. > > ... (the character device methods)... > > > I really think all this part that you add in vl.c should be moved into > > hw/vmmouse.c. The qemu_chr_open_msmouse should be merged into > > msmouse_init and called from vl.c. Then the structure should be > > allocated, and passed instead of the NULL argument of > > qemu_add_mouse_event_handler(). > > I think you meant "qemu-char.c", it hat split from "vl.c". And I guess it > shouldn't be moved to "vmmouse.c", since that's a VMWare mouse, but it > could have been a typo as well. If you meant "msmouse.c", then that was > done in the new revision of the patch. > > I'm not completely sure it is right though -- see, qemu-char.c contains a > lot of similar examples of complete implementation of character devices. > I find separation to msmouse.c a lot cleaner tough. > > >> + /* Buttons */ > >> + bytes[0] |= (buttons_state & 0x01 ? 0x20 : 0x00); > >> + bytes[0] |= (buttons_state & 0x02 ? 0x10 : 0x00); > >> + bytes[3] |= (buttons_state & 0x04 ? 0x20 : 0x00); > > > > There is probably a copy&paste error in the indices here. > > It's actually correct. It is just that the serial mouse protocol evolved > into being a little bit weird. > > -- > Lubomir Rintel > QEMU Microsoft serial mouse emulation > > Adds "msmouse" character device, which emulates a serial mouse. > Use it with -serial msmouse. > > Signed-Off-By: Lubomir Rintel > > Index: Makefile.target > =================================================================== > --- Makefile.target (revision 6107) > +++ Makefile.target (working copy) > @@ -633,6 +633,9 @@ > OBJS += rtl8139.o > OBJS += e1000.o > > +# Serial mouse > +OBJS += msmouse.o > + > ifeq ($(TARGET_BASE_ARCH), i386) > # Hardware support > OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o > Index: qemu-char.c > =================================================================== > --- qemu-char.c (revision 6107) > +++ qemu-char.c (working copy) > @@ -30,6 +30,7 @@ > #include "block.h" > #include "hw/usb.h" > #include "hw/baum.h" > +#include "hw/msmouse.h" > > #include > #include > @@ -2153,6 +2154,8 @@ > } else { > printf("Unable to open driver: %s\n", p); > } > + } else if (!strcmp(filename, "msmouse")) { > + chr = qemu_chr_open_msmouse(); > } else > #ifndef _WIN32 > if (strstart(filename, "unix:", &p)) { > Index: qemu-doc.texi > =================================================================== > --- qemu-doc.texi (revision 6107) > +++ qemu-doc.texi (working copy) > @@ -914,6 +914,8 @@ > When @var{remote_host} or @var{src_ip} are not specified > they default to @code{0.0.0.0}. > When not using a specified @var{src_port} a random port is automatically chosen. > +@item msmouse > +Three button serial mouse. Configure the guest to use Microsoft protocol. > > If you just want a simple readonly console you can use @code{netcat} or > @code{nc}, by starting qemu with: @code{-serial udp::4555} and nc as: > Index: hw/msmouse.c > =================================================================== > --- hw/msmouse.c (revision 0) > +++ hw/msmouse.c (revision 0) > @@ -0,0 +1,78 @@ > +/* > + * QEMU Microsoft serial mouse emulation > + * > + * Copyright (c) 2008 Lubomir Rintel > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > +#include > +#include "../qemu-common.h" > +#include "../qemu-char.h" > +#include "../console.h" > +#include "msmouse.h" > + > +#define MSMOUSE_LO6(n) ((n) & 0x3f) > +#define MSMOUSE_HI2(n) (((n) & 0xc0) >> 6) > + > +static void msmouse_event(void *opaque, > + int dx, int dy, int dz, int buttons_state) > +{ > + CharDriverState *chr = (CharDriverState *)opaque; > + > + char bytes[4] = { 0x40, 0x00, 0x00, 0x00 }; > + > + /* Movement deltas */ > + bytes[0] |= (MSMOUSE_HI2(dy) << 2) | MSMOUSE_HI2(dx); > + bytes[1] |= MSMOUSE_LO6(dx); > + bytes[2] |= MSMOUSE_LO6(dy); > + > + /* Buttons */ > + bytes[0] |= (buttons_state & 0x01 ? 0x20 : 0x00); > + bytes[0] |= (buttons_state & 0x02 ? 0x10 : 0x00); > + bytes[3] |= (buttons_state & 0x04 ? 0x20 : 0x00); > + > + /* We always send the packet of, so that we do not have to keep track > + of previous state of the middle button. This can potentially confuse > + some very old drivers for two button mice though. */ > + qemu_chr_read(chr, bytes, 4); > +} > + > +static int msmouse_chr_write (struct CharDriverState *s, const uint8_t *buf, int len) > +{ > + /* Ignore writes to mouse port */ > + return len; > +} > + > +static void msmouse_chr_close (struct CharDriverState *chr) > +{ > + qemu_free (chr); > +} > + > +CharDriverState *qemu_chr_open_msmouse(void) > +{ > + CharDriverState *chr; > + > + chr = qemu_mallocz(sizeof(CharDriverState)); > + chr->chr_write = msmouse_chr_write; > + chr->chr_close = msmouse_chr_close; > + > + qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft Mouse"); > + > + return chr; > +} > Index: hw/msmouse.h > =================================================================== > --- hw/msmouse.h (revision 0) > +++ hw/msmouse.h (revision 0) > @@ -0,0 +1,2 @@ > +/* hw/msmouse.c */ > +CharDriverState *qemu_chr_open_msmouse(void); -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net