Re: [Qemu-devel] [RESEND] [PATCH] Support for serial mouse emulation

[Aurelien Jarno]

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 <address@hidden>

> QEMU Microsoft serial mouse emulation
> 
> Adds "msmouse" character device, which emulates a serial mouse.
> Use it with -serial msmouse.
> 
> Signed-Off-By: Lubomir Rintel <address@hidden>
> 
> 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 <unistd.h>
>  #include <fcntl.h>
> @@ -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.
> address@hidden 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 <stdlib.h>
> +#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
address@hidden                 http://www.aurel32.net