Re: [Qemu-devel] [PATCH v2 1/1] genius: add genius serial mouse emulation

[Peter Maydell]

On 14 January 2014 22:05, Romain Naour <address@hidden> wrote:
> This patch adds the emulation for a serial Genius mouse using
> Mouse Systems protocol (5bytes).
> This protocol is compatible with most 3-button serial mouse.

"mice".

It might be helpful to note why we should care, ie if there are
any particularly interesting guests which can only deal with this
and not the MS mouse protocol, or if there are mouse features
you can only get support for with this protocol.

> Signed-off-by: Romain Naour <address@hidden>

> ---
> Changes v1 -> v2:
>  Fixes documentation (Paolo Bonzini)
>  Fixes typos
>
>  backends/Makefile.objs |   2 +-
>  backends/gnmouse.c     | 339 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/char.h  |   3 +
>  qapi-schema.json       |   1 +
>  qemu-char.c            |   4 +
>  qemu-options.hx        |  14 +-
>  6 files changed, 360 insertions(+), 3 deletions(-)
>  create mode 100644 backends/gnmouse.c
>
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 42557d5..e4b072c 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -1,7 +1,7 @@
>  common-obj-y += rng.o rng-egd.o
>  common-obj-$(CONFIG_POSIX) += rng-random.o
>
> -common-obj-y += msmouse.o
> +common-obj-y += msmouse.o gnmouse.o

I was going to suggest a separate CONFIG_GNMOUSE for this,
but we don't have a nice place to then enable it, so never mind.

>  common-obj-$(CONFIG_BRLAPI) += baum.o
>  $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
>
> diff --git a/backends/gnmouse.c b/backends/gnmouse.c
> new file mode 100644
> index 0000000..9581419
> --- /dev/null
> +++ b/backends/gnmouse.c
> @@ -0,0 +1,339 @@
> +/*
> + * QEMU Genius GM-6 serial mouse emulation
> + *
> + * Adapted from msmouse
> + *
> + * Copyright (c) 2014 Romain Naour
> + *
> + * 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 <string.h>
> +#include <sys/time.h>
> +
> +#include "qemu-common.h"
> +#include "sysemu/char.h"
> +#include "ui/console.h"
> +#include "qemu/timer.h"
> +
> +/* #define DEBUG_GENIUS_MOUSE */
> +
> +#ifdef DEBUG_GENIUS_MOUSE
> +#define DPRINTF(fmt, ...) \
> +do { fprintf(stderr, "gnmouse: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +do {} while (0)
> +#endif
> +
> +/*
> + * struct gnmouse_save:
> + * This structure is used to save private info for Genius mouse.
> + *
> + * dx: deltas on x-axis saved since last frame send to emulated system.
> + * dy: deltas on y-axis saved since last frame send to emulated system.
> + * transmit_timer: QEMU's timer
> + * transmit_time: reload value for transmit_timer
> + * data: frame to be sent
> + * index: used to save current state of the state machine. see type states 
> below
> + */
> +typedef struct gnmouse_save {

Coding style says struct names (and struct typedef names) should be
CamelCase.

> +    int dx;
> +    int dy;
> +    int button;
> +    struct QEMUTimer *transmit_timer; /* QEMU timer */
> +    uint64_t transmit_time;           /* time to transmit a char in ticks */
> +    unsigned char data[5];
> +    int index;
> +} gnmouse_save;

How does all this state get migrated at VM migration? I know
how this works for device models, but does anybody know
the answer for char backends?

> +
> +
> +/* states */
> +typedef enum {
> +    START,  /* 0 */
> +    CHAR_1, /* 1 : BP */
> +    CHAR_2, /* 2 : Dx */
> +    CHAR_3, /* 3 : Dy */
> +    CHAR_4, /* 4 : Dx */
> +    CHAR_5, /* 5 : Dy */
> +    STOP    /* 6 */
> +}
> +states;

Stray newline after '}'.

> +
> +/**
> + * gnmouse_chr_write: this function is used when QEMU
> + * try to write something to mouse port.
> + * Nothing is send to the emulated mouse.

"sent"

> + *
> + * Return: lengh of the buffer

"length"

> + *
> + * @s: address of the CharDriverState used by the mouse
> + * @buf: buffer to write
> + * @len: lengh of the buffer to write

"length"

> + */
> +static int gnmouse_chr_write(struct CharDriverState *s, const uint8_t *buf,
> +                             int len)
> +{
> +    /* Ignore writes to mouse port */
> +    return len;
> +}
> +
> +/**
> + * gnmouse_chr_close: this function close the mouse port.

"closes"

> + * It stop and free the QEMU's timer and free gnmouse_save struct.

"stops and frees the QEMU timer and frees the"

> + *
> + * Return: void
> + *
> + * @chr: address of the CharDriverState used by the mouse
> + */
> +static void gnmouse_chr_close(struct CharDriverState *chr)
> +{
> +    /* stop and free the QEMU's timer */

You don't need to say this again.

> +    timer_del(((gnmouse_save *)chr->opaque)->transmit_timer);
> +    timer_free(((gnmouse_save *)chr->opaque)->transmit_timer);

These casts are ugly. Create a local variable and assign chr->opaque
to it.

> +    /* free gnmouse_save struct */
> +    g_free(chr->opaque);
> +    g_free(chr);
> +}
> +
> +/**
> + * gnmouse_handler: send a byte on serial port to the guest system
> + * This handler is called on each timer timeout or directly by 
> gnmouse_event()
> + * when no transmission is underway.
> + * It use a state machine in order to know which byte of the frame must be 
> send.

"sent".

> + *
> + * Returns void
> + *
> + * @opaque: address of the CharDriverState used by the mouse
> + */
> +static void gnmouse_handler(void *opaque)
> +{
> +    CharDriverState *chr = (CharDriverState *)opaque;
> +    gnmouse_save *save = (gnmouse_save *)chr->opaque;
> +    unsigned char *data = save->data;
> +    int dx_tmp, dy_tmp;
> +/*
> + * Byte 0:  1,  0,  0,  0,  0,  L,  M,  R
> + * Byte 1: X7, X6, X5, X4, X3, X2, X1, X0
> + * Byte 2: Y7, Y6, Y5, Y4, Y3, Y2, Y1, Y0
> + * Byte 3: X7, X6, X5, X4, X3, X2, X1, X0
> + * Byte 4: Y7, Y6, Y5, Y4, Y3, Y2, Y1, Y0
> + */

Indent this comment to the same column as the code, please.

> +    switch (save->index) {
> +    case CHAR_4:
> +        if (save->dx && save->dy) {

This looks wrong. If there's a saved delta-x then we
still want to send it to the guest, even if the save->dy is
correct, right?

Also, it could use a comment. If I understand the logic correctly,
something like:
 /* Send as much of our accumulated delta as we can fit into
  * the packet format. Anything remaining will get sent in a
  * subsequent packet.
  */

> +            if (save->dx >= 128) {
> +                DPRINTF("overflow dx= %d\n", save->dx);
> +                save->dx -= 128;
> +                dx_tmp = 128;
> +            } else if (save->dx <= -127) {
> +                DPRINTF("overflow dx= %d\n", save->dx);
> +                save->dx += 127;
> +                dx_tmp = -127;
> +            } else {
> +                dx_tmp = save->dx;
> +                save->dx = 0;
> +            }
> +
> +            if (save->dy >= 128) {
> +                DPRINTF("overflow dy= %d\n", save->dy);
> +                save->dy -= 128;
> +                dy_tmp = 128;
> +            } else if (save->dy <= -127) {
> +                DPRINTF("overflow dy= %d\n", save->dy);
> +                save->dy += 127;
> +                dy_tmp = -127;
> +            } else {
> +                dy_tmp = save->dy;
> +                save->dy = 0;
> +            }
> +
> +            DPRINTF("dx= %d\n", save->dx);
> +            DPRINTF("dy= %d\n", save->dy);
> +
> +            data[3] = dx_tmp;
> +            data[4] = -(dy_tmp);
> +        }
> +        break;
> +
> +    case STOP:
> +        if (!(save->dx && save->dy)) {
> +            /* no more data */
> +            DPRINTF("no more data\n");
> +            return;
> +        } else {
> +            /* data saved */
> +            DPRINTF("data saved\n");
> +            save->index = START;
> +        }
> +        /* No break, pass-through START */

The standard way to note this is by having a comment
    /* fall through */

(some coding style tools check for this exact wording)

> +
> +    case START:
> +        /* New serial frame */
> +        /* Buttons */
> +        data[0] = save->button;
> +        save->index = CHAR_1;
> +        /* No break, pass-through CHAR_1 */
> +
> +    case CHAR_1:
> +        /* avoid overflow on dx or dy */
> +        if (save->dx >= 128) {
> +            DPRINTF("overflow dx= %d\n", save->dx);
> +            save->dx -= 128;
> +            dx_tmp = 128;
> +        } else if (save->dx <= -127) {
> +            DPRINTF("overflow dx= %d\n", save->dx);
> +            save->dx += 127;
> +            dx_tmp = -127;
> +        } else{
> +            dx_tmp = save->dx;
> +            save->dx = 0;
> +        }
> +
> +        if (save->dy >= 128) {
> +            DPRINTF("overflow dy= %d\n", save->dy);
> +            save->dy -= 128;
> +            dy_tmp = 128;
> +        } else if (save->dy <= -127) {
> +            DPRINTF("overflow dy= %d\n", save->dy);
> +            save->dy += 127;
> +            dy_tmp = -127;
> +        } else{
> +            dy_tmp = save->dy;
> +            save->dy = 0;
> +        }

Pretty much identical logic to above -- this needs to be factored
out into a common utility function.

> +
> +        DPRINTF("dx= %d\n", save->dx);
> +        DPRINTF("dy= %d\n", save->dy);
> +
> +        /* Movement deltas */
> +        data[1] = dx_tmp;
> +        data[2] = -(dy_tmp);
> +        data[3] = 0;
> +        data[4] = 0;
> +
> +    case CHAR_2:
> +    case CHAR_3:
> +    case CHAR_5:
> +        break;
> +    default:
> +        return;
> +    }
> +
> +    /* reload timer */

This comment is stating the obvious. It would be more interesting
to know why we have a timer at all...

> +    timer_mod(save->transmit_timer,
> +              qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + save->transmit_time);
> +    DPRINTF("mod_timer: %d\n", save->index);
> +    /* write data on serial port */
> +    qemu_chr_be_write(chr, &(data[save->index - 1]), 1);
> +    DPRINTF("write :%x\n", data[save->index - 1]);
> +    /* next state */
> +    save->index += 1;

   save->index++;

> +}
> +
> +/**
> + * gnmouse_event: event handler called by SDL functions

Not just SDL; "by the UI layer".

> + * on each mouse movement or button press.
> + *
> + * Return void

You don't need to say that, we can tell by looking at the prototype...

> + *
> + * @opaque: address of the CharDriverState used by the mouse
> + * @dx: deltas on the x-axis since last event
> + * @dy: deltas on the y-axis since last event
> + * @dz: deltas on the z-axis since last event (not used)
> + * @button_state: status of mouse button
> + */
> +static void gnmouse_event(void *opaque,
> +                          int dx, int dy, int dz, int buttons_state)
> +{
> +    CharDriverState *chr = (CharDriverState *)opaque;
> +    gnmouse_save *save = (gnmouse_save *)chr->opaque;
> +    char BP = 0x80;

All-caps is usually reserved for macro names. Lower case for
variable names, please.

> +
> +    /* save deltas */
> +    save->dx += dx;
> +    save->dy += dy;
> +
> +    DPRINTF("dx= %d; dy= %d; buttons=%x\n", dx, dy, buttons_state);
> +
> +    /* Buttons */
> +    BP |= (buttons_state & 0x01 ? 0x00 : 0x04); /* BP1 = L */
> +    BP |= (buttons_state & 0x02 ? 0x00 : 0x01); /* BP2 = R */
> +    BP |= (buttons_state & 0x04 ? 0x00 : 0x02); /* BP4 = M */

Are these really 'bit set if button is up' ? If so that could use a comment,
because that's a bit of a weird protocol design.

> +
> +    save->button = BP;
> +    if (save->index == STOP) {
> +        /* no transmission is underway, start a new transmission */
> +        save->index = START;
> +        gnmouse_handler((void *) chr);

This void* cast is unnecessary.

> +    }
> +}
> +
> +/**
> + * qemu_chr_open_gnmouse: Init function for Genius mouse
> + * allocate a gnmouse_save structure to save data used by gnmouse emulation.
> + * allocate a new CharDriverState.
> + * create a new QEMU's timer with gnmouse_handler() as timeout handler.
> + * calculate the transmit_time for 1200 bauds transmission.

This is all stating the obvious.

> + *
> + * Return address of the initialized CharDriverState
> + *
> + * @opts: argument not used
> + */
> +CharDriverState *qemu_chr_open_gnmouse(void)
> +{
> +    CharDriverState *chr;
> +    gnmouse_save *save;
> +
> +    DPRINTF("qemu_chr_open_gnmouse\n");
> +
> +    /* allocate CharDriverState and gnmouse_save */

Again, don't use comments to state what's obviously the
case from glancing at the code.

> +    chr = g_malloc0(sizeof(CharDriverState));
> +    save = g_malloc0(sizeof(gnmouse_save));
> +
> +    chr->chr_write = gnmouse_chr_write;
> +    chr->chr_close = gnmouse_chr_close;
> +    chr->explicit_be_open = true;
> +
> +    /* create a new QEMU's timer with gnmouse_handler() as timeout handler. 
> */
> +    save->transmit_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                        (QEMUTimerCB *) gnmouse_handler, 
> chr);
> +    /* calculate the transmit_time for 1200 bauds transmission */
> +    save->transmit_time = (get_ticks_per_sec() / 1200) * 10; /* 1200 bauds */
> +
> +    DPRINTF("transmit_time = %lld\n", save->transmit_time);
> +
> +    /* init state machine */
> +    save->index = STOP;
> +
> +    /* keep address of gnmouse_save */
> +    chr->opaque = save;
> +
> +    qemu_add_mouse_event_handler(gnmouse_event, chr, 0,
> +                                 "QEMU Genius GM-6 Mouse");
> +
> +    return chr;
> +}
> +
> +static void register_types(void)
> +{
> +    register_char_driver_qapi("gnmouse", CHARDEV_BACKEND_KIND_GNMOUSE, NULL);
> +}
> +
> +type_init(register_types);

>  @item -chardev msmouse ,address@hidden
>
> -Forward QEMU's emulated msmouse events to the guest. @option{msmouse} does 
> not
> -take any options.
> +Forward events from QEMU's emulated mouse to the guest using the
> +Microsoft protocol. @option{msmouse} does not take any options.
> +
> address@hidden -chardev gnmouse ,address@hidden
> +
> +Forward events from QEMU's emulated mouse to the guest using the Genius
> +(Mouse Systems) protocol. @option{gnmouse} does not take any options.

If we have more than one supported protocol, we should probably say in the
documentation which one we recommend for which purposes, rather than
leaving people to guess which one might be better supported/more efficient/etc.
My guess is that the answer is "use msmouse unless your guest OS only
supports the Mouse Systems protocol", but your patch doesn't include any
rationale, so that is just a guess...

thanks
-- PMM