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

[Aurelien Jarno]

On Tue, Dec 02, 2008 at 05:50:58PM -0500, address@hidden wrote:
> Attaching (sorry for not posting it inline, but my e-mail client mangles
> whitespace) a patch for mouse emulation on a serial port. Emulates a
> three-button mouse that uses Microsoft protocol.
> 
> Tested with 386BSD 0.1 and XFree86 2.1. It seems that my guest is not the
> only one that lacks PS/2 mouse support though [1] :)

Please find my comments inline.

> [1] http://www.archivum.info/address@hidden/2005-09/msg00055.html
> QEMU Microsoft serial mouse emulation
> Lubomir Rintel <address@hidden>

This should be replaced by a Signed-off-by: line.

> Index: Makefile.target
> ===================================================================
> --- Makefile.target   (revision 5799)
> +++ Makefile.target   (working copy)
> @@ -659,7 +659,7 @@
>  
>  ifeq ($(TARGET_BASE_ARCH), i386)
>  # Hardware support
> -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
>  OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
>  OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
>  OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o
> @@ -670,7 +670,7 @@
>  # shared objects
>  OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o
>  # PREP target
> -OBJS+= pckbd.o ps2.o serial.o i8259.o i8254.o fdc.o m48t59.o mc146818rtc.o
> +OBJS+= pckbd.o ps2.o msmouse.o serial.o i8259.o i8254.o fdc.o m48t59.o 
> mc146818rtc.o
>  OBJS+= prep_pci.o ppc_prep.o
>  # Mac shared devices
>  OBJS+= macio.o cuda.o adb.o mac_nvram.o mac_dbdma.o
> @@ -685,7 +685,7 @@
>  OBJS+= mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
>  OBJS+= mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
>  OBJS+= g364fb.o jazz_led.o
> -OBJS+= ide.o gt64xxx.o pckbd.o ps2.o fdc.o mc146818rtc.o usb-uhci.o acpi.o 
> ds1225y.o
> +OBJS+= ide.o gt64xxx.o pckbd.o ps2.o msmouse.o fdc.o mc146818rtc.o 
> usb-uhci.o acpi.o ds1225y.o
>  OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o $(SOUND_HW)
>  OBJS+= mipsnet.o
>  OBJS+= pflash_cfi01.o
> @@ -704,7 +704,7 @@
>  endif
>  ifeq ($(TARGET_BASE_ARCH), sparc)
>  ifeq ($(TARGET_ARCH), sparc64)
> -OBJS+= sun4u.o ide.o pckbd.o ps2.o vga.o apb_pci.o
> +OBJS+= sun4u.o ide.o pckbd.o ps2.o msmouse.o vga.o apb_pci.o
>  OBJS+= fdc.o mc146818rtc.o serial.o m48t59.o
>  OBJS+= cirrus_vga.o parallel.o ptimer.o
>  else
> @@ -714,7 +714,7 @@
>  endif
>  endif
>  ifeq ($(TARGET_BASE_ARCH), arm)
> -OBJS+= integratorcp.o versatilepb.o ps2.o smc91c111.o arm_pic.o arm_timer.o
> +OBJS+= integratorcp.o versatilepb.o ps2.o msmouse.o smc91c111.o arm_pic.o 
> arm_timer.o
>  OBJS+= arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o
>  OBJS+= versatile_pci.o ptimer.o
>  OBJS+= realview_gic.o realview.o arm_sysctl.o mpcore.o

Is it really necessary to enable that in all targets, especially when
this is inited only in hw/pc.c?

> Index: qemu-char.c
> ===================================================================
> --- qemu-char.c       (revision 5799)
> +++ 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>
> @@ -2120,6 +2121,41 @@
>      return NULL;
>  }
>  
> +/***********************************************************/
> +/* Microsoft serial mouse */
> +
> +/* This is written to by hw/msmouse.c if non-NULL */
> +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.

> +int msmouse_chr_write (struct CharDriverState *s, const uint8_t *buf, int 
> len)
> +{
> +    /* Ignore writes to mouse port */
> +    return len;
> +}
> +
> +void msmouse_chr_close (struct CharDriverState *s)
> +{
> +    qemu_free (s);
> +    msmouse_chr = NULL;
> +}
> +
> +static CharDriverState *qemu_chr_open_msmouse(void)
> +{
> +    CharDriverState *chr;
> +
> +    /* Only one serial mouse per instance is allowed */
> +    if (msmouse_chr != NULL) {
> +        return NULL;
> +    }

I don't think it is a good idea. I guess this is due to the use of a
global variable.

> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    chr->chr_write = msmouse_chr_write;
> +    chr->chr_close = msmouse_chr_close;
> +    msmouse_chr = chr;
> +
> +    return chr;
> +}
> +

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().

>  static TAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs
>  = TAILQ_HEAD_INITIALIZER(chardevs);
>  
> @@ -2154,6 +2190,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 5799)
> +++ qemu-doc.texi     (working copy)
> @@ -904,6 +904,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,61 @@
> +/*
> + * 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)
> +{
> +    if (!msmouse_chr)
> +        return;
> +
> +    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);

There is probably a copy&paste error in the indices here.

> +
> +    /* 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(msmouse_chr, bytes, 4);
> +}
> +
> +void *msmouse_init()
> +{
> +    qemu_add_mouse_event_handler(msmouse_event, NULL, 0, "QEMU Microsoft 
> Mouse");
> +    return NULL;
> +}
> Index: hw/msmouse.h
> ===================================================================
> --- hw/msmouse.h      (revision 0)
> +++ hw/msmouse.h      (revision 0)
> @@ -0,0 +1,2 @@
> +extern CharDriverState *msmouse_chr;
> +void *msmouse_init(void);
> Index: hw/pc.c
> ===================================================================
> --- hw/pc.c   (revision 5799)
> +++ hw/pc.c   (working copy)
> @@ -33,6 +33,7 @@
>  #include "boards.h"
>  #include "console.h"
>  #include "fw_cfg.h"
> +#include "msmouse.h"
>  
>  /* output Bochs bios info messages */
>  //#define DEBUG_BIOS
> @@ -1092,6 +1093,8 @@
>           }
>          }
>      }
> +
> +    msmouse_init ();
>  }
>  
>  static void pc_init_pci(ram_addr_t ram_size, int vga_ram_size,

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net