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

[Lubomir Rintel <lkundrak@v3.sk>]

[-- Attachment #1: Type: text/plain, Size: 3569 bytes --]

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.

> 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 <lkundrak@v3.sk>

[-- Attachment #2: qemu-20081224-msmouse.patch --]
[-- Type: text/x-patch, Size: 4862 bytes --]

QEMU Microsoft serial mouse emulation

Adds "msmouse" character device, which emulates a serial mouse.
Use it with -serial msmouse.

Signed-Off-By: Lubomir Rintel <lkundrak@v3.sk>

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.
+@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 <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);