[PATCH v2 0/5] Make serial msmouse work

[PATCH v2 0/5] Make serial msmouse work

[Arwed Meyer]

This series of patches makes `-serial msmouse` work in practice.

Tested with FreeDOS/CTMouse driver `ctmouse /V` which identifies a
Logitech compatible 3 button mouse.
It will probably run as well with any other compatible serial mouse
driver on Windows 9x etc.

Arwed Meyer (5):
  msmouse: Handle mouse reset
  chardev: src buffer const for write functions
  msmouse: Use fifo8 instead of array
  msmouse: Add pnp data
  serial: Allow unaligned i/o access

 chardev/char.c          |   4 +-
 chardev/msmouse.c       | 148 ++++++++++++++++++++++++++++++++--------
 hw/char/serial.c        |   3 +
 include/chardev/char.h  |   4 +-
 include/sysemu/replay.h |   2 +-
 replay/replay-char.c    |   2 +-
 stubs/replay-tools.c    |   2 +-
 7 files changed, 131 insertions(+), 34 deletions(-)

--
2.34.1

[PATCH v2 1/5] msmouse: Handle mouse reset

[Arwed Meyer]

Detect mouse reset via RTS or DTR line:
Don't send or process anything while in reset.
When coming out of reset, send ID sequence first thing.
This allows msmouse to be detected by common mouse drivers.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 chardev/msmouse.c | 63 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index eb9231dcdb..95fa488339 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -25,17 +25,20 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "chardev/char.h"
+#include "chardev/char-serial.h"
 #include "ui/console.h"
 #include "ui/input.h"
 #include "qom/object.h"

-#define MSMOUSE_LO6(n) ((n) & 0x3f)
-#define MSMOUSE_HI2(n) (((n) & 0xc0) >> 6)
+#define MSMOUSE_LO6(n)  ((n) & 0x3f)
+#define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
+#define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))

 struct MouseChardev {
     Chardev parent;

     QemuInputHandlerState *hs;
+    int tiocm;
     int axis[INPUT_AXIS__MAX];
     bool btns[INPUT_BUTTON__MAX];
     bool btnc[INPUT_BUTTON__MAX];
@@ -109,6 +112,11 @@ static void msmouse_input_event(DeviceState *dev, QemuConsole *src,
     InputMoveEvent *move;
     InputBtnEvent *btn;

+    /* Ignore events if serial mouse powered down. */
+    if (!MSMOUSE_PWR(mouse->tiocm)) {
+        return;
+    }
+
     switch (evt->type) {
     case INPUT_EVENT_KIND_REL:
         move = evt->u.rel.data;
@@ -132,6 +140,11 @@ static void msmouse_input_sync(DeviceState *dev)
     MouseChardev *mouse = MOUSE_CHARDEV(dev);
     Chardev *chr = CHARDEV(dev);

+    /* Ignore events if serial mouse powered down. */
+    if (!MSMOUSE_PWR(mouse->tiocm)) {
+        return;
+    }
+
     msmouse_queue_event(mouse);
     msmouse_chr_accept_input(chr);
 }
@@ -142,6 +155,50 @@ static int msmouse_chr_write(struct Chardev *s, const uint8_t *buf, int len)
     return len;
 }

+static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
+{
+    MouseChardev *mouse = MOUSE_CHARDEV(chr);
+    int c;
+    int *targ = (int *)arg;
+
+    switch (cmd) {
+    case CHR_IOCTL_SERIAL_SET_TIOCM:
+        c = mouse->tiocm;
+        mouse->tiocm = *(int *)arg;
+        if (MSMOUSE_PWR(mouse->tiocm)) {
+            if (!MSMOUSE_PWR(c)) {
+                /*
+                 * Power on after reset: send "M3"
+                 * cause we behave like a 3 button logitech
+                 * mouse.
+                 */
+                mouse->outbuf[0] = 'M';
+                mouse->outbuf[1] = '3';
+                mouse->outlen = 2;
+                /* Start sending data to serial. */
+                msmouse_chr_accept_input(chr);
+            }
+            break;
+        }
+        /*
+         * Reset mouse buffers on power down.
+         * Mouse won't send anything without power.
+         */
+        mouse->outlen = 0;
+        memset(mouse->axis, 0, sizeof(mouse->axis));
+        memset(mouse->btns, false, sizeof(mouse->btns));
+        memset(mouse->btnc, false, sizeof(mouse->btns));
+        break;
+    case CHR_IOCTL_SERIAL_GET_TIOCM:
+        /* Remember line control status. */
+        *targ = mouse->tiocm;
+        break;
+    default:
+        return -ENOTSUP;
+    }
+    return 0;
+}
+
 static void char_msmouse_finalize(Object *obj)
 {
     MouseChardev *mouse = MOUSE_CHARDEV(obj);
@@ -166,6 +223,7 @@ static void msmouse_chr_open(Chardev *chr,
     *be_opened = false;
     mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
                                             &msmouse_handler);
+    mouse->tiocm = 0;
 }

 static void char_msmouse_class_init(ObjectClass *oc, void *data)
@@ -175,6 +233,7 @@ static void char_msmouse_class_init(ObjectClass *oc, void *data)
     cc->open = msmouse_chr_open;
     cc->chr_write = msmouse_chr_write;
     cc->chr_accept_input = msmouse_chr_accept_input;
+    cc->chr_ioctl = msmouse_ioctl;
 }

 static const TypeInfo char_msmouse_type_info = {
--
2.34.1

[PATCH v2 2/5] chardev: src buffer const for write functions

[Arwed Meyer]

Make source buffers const for char be write functions.
This allows using buffers returned by fifo as buf parameter and source buffer
should not be changed by write functions anyway.

Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 chardev/char.c          | 4 ++--
 include/chardev/char.h  | 4 ++--
 include/sysemu/replay.h | 2 +-
 replay/replay-char.c    | 2 +-
 stubs/replay-tools.c    | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 0169d8dde4..b005df3ccf 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -193,7 +193,7 @@ int qemu_chr_be_can_write(Chardev *s)
     return be->chr_can_read(be->opaque);
 }

-void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
+void qemu_chr_be_write_impl(Chardev *s, const uint8_t *buf, int len)
 {
     CharBackend *be = s->be;

@@ -202,7 +202,7 @@ void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
     }
 }

-void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len)
+void qemu_chr_be_write(Chardev *s, const uint8_t *buf, int len)
 {
     if (qemu_chr_replay(s)) {
         if (replay_mode == REPLAY_MODE_PLAY) {
diff --git a/include/chardev/char.h b/include/chardev/char.h
index a319b5fdff..44cd82e405 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -186,7 +186,7 @@ int qemu_chr_be_can_write(Chardev *s);
  * the caller should call @qemu_chr_be_can_write to determine how much data
  * the front end can currently accept.
  */
-void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len);
+void qemu_chr_be_write(Chardev *s, const uint8_t *buf, int len);

 /**
  * qemu_chr_be_write_impl:
@@ -195,7 +195,7 @@ void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len);
  *
  * Implementation of back end writing. Used by replay module.
  */
-void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
+void qemu_chr_be_write_impl(Chardev *s, const uint8_t *buf, int len);

 /**
  * qemu_chr_be_update_read_handlers:
diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index 73dee9ccdf..7ec0882b50 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -198,7 +198,7 @@ uint64_t blkreplay_next_id(void);
 /*! Registers char driver to save it's events */
 void replay_register_char_driver(struct Chardev *chr);
 /*! Saves write to char device event to the log */
-void replay_chr_be_write(struct Chardev *s, uint8_t *buf, int len);
+void replay_chr_be_write(struct Chardev *s, const uint8_t *buf, int len);
 /*! Writes char write return value to the replay log. */
 void replay_char_write_event_save(int res, int offset);
 /*! Reads char write return value from the replay log. */
diff --git a/replay/replay-char.c b/replay/replay-char.c
index d2025948cf..a31aded032 100644
--- a/replay/replay-char.c
+++ b/replay/replay-char.c
@@ -48,7 +48,7 @@ void replay_register_char_driver(Chardev *chr)
     char_drivers[drivers_count++] = chr;
 }

-void replay_chr_be_write(Chardev *s, uint8_t *buf, int len)
+void replay_chr_be_write(Chardev *s, const uint8_t *buf, int len)
 {
     CharEvent *event = g_new0(CharEvent, 1);

diff --git a/stubs/replay-tools.c b/stubs/replay-tools.c
index f2e72bb225..3e8ca3212d 100644
--- a/stubs/replay-tools.c
+++ b/stubs/replay-tools.c
@@ -53,7 +53,7 @@ void replay_register_char_driver(struct Chardev *chr)
 {
 }

-void replay_chr_be_write(struct Chardev *s, uint8_t *buf, int len)
+void replay_chr_be_write(struct Chardev *s, const uint8_t *buf, int len)
 {
     abort();
 }
--
2.34.1

[PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Arwed Meyer]

Make use of fifo8 functions instead of implementing own fifo code.
This makes the code more readable and reduces risk of bugs.

Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index 95fa488339..e9aa3eab55 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -24,6 +24,7 @@

 #include "qemu/osdep.h"
 #include "qemu/module.h"
+#include "qemu/fifo8.h"
 #include "chardev/char.h"
 #include "chardev/char-serial.h"
 #include "ui/console.h"
@@ -34,6 +35,12 @@
 #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
 #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))

+/* Serial fifo size. */
+#define MSMOUSE_BUF_SZ 64
+
+/* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
+const uint8_t mouse_id[] = {'M', '3'};
+
 struct MouseChardev {
     Chardev parent;

@@ -42,8 +49,7 @@ struct MouseChardev {
     int axis[INPUT_AXIS__MAX];
     bool btns[INPUT_BUTTON__MAX];
     bool btnc[INPUT_BUTTON__MAX];
-    uint8_t outbuf[32];
-    int outlen;
+    Fifo8 outbuf;
 };
 typedef struct MouseChardev MouseChardev;

@@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
 static void msmouse_chr_accept_input(Chardev *chr)
 {
     MouseChardev *mouse = MOUSE_CHARDEV(chr);
-    int len;
+    uint32_t len_out, len;

-    len = qemu_chr_be_can_write(chr);
-    if (len > mouse->outlen) {
-        len = mouse->outlen;
-    }
-    if (!len) {
+    len_out = qemu_chr_be_can_write(chr);
+    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
         return;
     }
-
-    qemu_chr_be_write(chr, mouse->outbuf, len);
-    mouse->outlen -= len;
-    if (mouse->outlen) {
-        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
-    }
+    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
+    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
+            len_out);
 }

 static void msmouse_queue_event(MouseChardev *mouse)
@@ -94,12 +94,11 @@ static void msmouse_queue_event(MouseChardev *mouse)
         mouse->btnc[INPUT_BUTTON_MIDDLE]) {
         bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
         mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
-        count = 4;
+        count++;
     }

-    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
-        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
-        mouse->outlen += count;
+    if (fifo8_num_free(&mouse->outbuf) >= count) {
+        fifo8_push_all(&mouse->outbuf, bytes, count);
     } else {
         /* queue full -> drop event */
     }
@@ -172,9 +171,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
                  * cause we behave like a 3 button logitech
                  * mouse.
                  */
-                mouse->outbuf[0] = 'M';
-                mouse->outbuf[1] = '3';
-                mouse->outlen = 2;
+                fifo8_push_all(&mouse->outbuf, mouse_id, sizeof(mouse_id));
                 /* Start sending data to serial. */
                 msmouse_chr_accept_input(chr);
             }
@@ -184,7 +181,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
          * Reset mouse buffers on power down.
          * Mouse won't send anything without power.
          */
-        mouse->outlen = 0;
+        fifo8_reset(&mouse->outbuf);
         memset(mouse->axis, 0, sizeof(mouse->axis));
         memset(mouse->btns, false, sizeof(mouse->btns));
         memset(mouse->btnc, false, sizeof(mouse->btns));
@@ -204,6 +201,7 @@ static void char_msmouse_finalize(Object *obj)
     MouseChardev *mouse = MOUSE_CHARDEV(obj);

     qemu_input_handler_unregister(mouse->hs);
+    fifo8_destroy(&mouse->outbuf);
 }

 static QemuInputHandler msmouse_handler = {
@@ -224,6 +222,7 @@ static void msmouse_chr_open(Chardev *chr,
     mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
                                             &msmouse_handler);
     mouse->tiocm = 0;
+    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
 }

 static void char_msmouse_class_init(ObjectClass *oc, void *data)
--
2.34.1

[PATCH v2 4/5] msmouse: Add pnp data

[Arwed Meyer]

Make msmouse send serial pnp data.
Enables you to see nice qemu device name in Win9x.

Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 chardev/msmouse.c | 58 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index e9aa3eab55..29eb97fedc 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -35,11 +35,24 @@
 #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
 #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))

+/* Serial PnP for 6 bit devices/mice sends all ASCII chars - 0x20 */
+#define M(c) (c - 0x20)
 /* Serial fifo size. */
 #define MSMOUSE_BUF_SZ 64

 /* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
 const uint8_t mouse_id[] = {'M', '3'};
+/*
+ * PnP start "(", PnP version (1.0), vendor ID, product ID, '\\',
+ * serial ID (omitted), '\\', MS class name, '\\', driver ID (omitted), '\\',
+ * product description, checksum, ")"
+ * Missing parts are inserted later.
+ */
+const uint8_t pnp_data[] = {M('('), 1, '$', M('Q'), M('M'), M('U'),
+                         M('0'), M('0'), M('0'), M('1'),
+                         M('\\'), M('\\'),
+                         M('M'), M('O'), M('U'), M('S'), M('E'),
+                         M('\\'), M('\\')};

 struct MouseChardev {
     Chardev parent;
@@ -154,11 +167,22 @@ static int msmouse_chr_write(struct Chardev *s, const uint8_t *buf, int len)
     return len;
 }

+static QemuInputHandler msmouse_handler = {
+    .name  = "QEMU Microsoft Mouse",
+    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
+    .event = msmouse_input_event,
+    .sync  = msmouse_input_sync,
+};
+
 static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
 {
     MouseChardev *mouse = MOUSE_CHARDEV(chr);
-    int c;
+    int c, i, j;
+    uint8_t bytes[MSMOUSE_BUF_SZ / 2];
     int *targ = (int *)arg;
+    const uint8_t hexchr[16] = {M('0'), M('1'), M('2'), M('3'), M('4'), M('5'),
+                             M('6'), M('7'), M('8'), M('9'), M('A'), M('B'),
+                             M('C'), M('D'), M('E'), M('F')};

     switch (cmd) {
     case CHR_IOCTL_SERIAL_SET_TIOCM:
@@ -167,11 +191,30 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
         if (MSMOUSE_PWR(mouse->tiocm)) {
             if (!MSMOUSE_PWR(c)) {
                 /*
-                 * Power on after reset: send "M3"
-                 * cause we behave like a 3 button logitech
-                 * mouse.
+                 * Power on after reset: Send ID and PnP data
+                 * No need to check fifo space as it is empty at this point.
                  */
                 fifo8_push_all(&mouse->outbuf, mouse_id, sizeof(mouse_id));
+                /* Add PnP data: */
+                fifo8_push_all(&mouse->outbuf, pnp_data, sizeof(pnp_data));
+                /*
+                 * Add device description from qemu handler name.
+                 * Make sure this all fits into the queue beforehand!
+                 */
+                c = M(')');
+                for (i = 0; msmouse_handler.name[i]; i++) {
+                    bytes[i] = M(msmouse_handler.name[i]);
+                    c += bytes[i];
+                }
+                /* Calc more of checksum */
+                for (j = 0; j < sizeof(pnp_data); j++) {
+                    c += pnp_data[j];
+                }
+                c &= 0xff;
+                bytes[i++] = hexchr[c >> 4];
+                bytes[i++] = hexchr[c & 0x0f];
+                bytes[i++] = M(')');
+                fifo8_push_all(&mouse->outbuf, bytes, i);
                 /* Start sending data to serial. */
                 msmouse_chr_accept_input(chr);
             }
@@ -204,13 +247,6 @@ static void char_msmouse_finalize(Object *obj)
     fifo8_destroy(&mouse->outbuf);
 }

-static QemuInputHandler msmouse_handler = {
-    .name  = "QEMU Microsoft Mouse",
-    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
-    .event = msmouse_input_event,
-    .sync  = msmouse_input_sync,
-};
-
 static void msmouse_chr_open(Chardev *chr,
                              ChardevBackend *backend,
                              bool *be_opened,
--
2.34.1

[PATCH v2 5/5] serial: Allow unaligned i/o access

[Arwed Meyer]

Unaligned i/o access on serial UART works on real PCs.
This is used for example by FreeDOS CTMouse driver. Without this it
can't reset and detect serial mice.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 hw/char/serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 7061aacbce..41b5e61977 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -961,6 +961,9 @@ void serial_set_frequency(SerialState *s, uint32_t frequency)
 const MemoryRegionOps serial_io_ops = {
     .read = serial_ioport_read,
     .write = serial_ioport_write,
+    .valid = {
+        .unaligned = 1,
+    },
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
--
2.34.1

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Peter Maydell]

On Thu, 8 Sept 2022 at 18:43, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>
> Detect mouse reset via RTS or DTR line:
> Don't send or process anything while in reset.
> When coming out of reset, send ID sequence first thing.
> This allows msmouse to be detected by common mouse drivers.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>

How does this work across migration? There doesn't seem
to be anything that handles migration of the existing
state inside the MouseChardev either, though...

-- PMM

Re: [PATCH v2 2/5] chardev: src buffer const for write functions

[Marc-André Lureau]

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

Hi

On Thu, Sep 8, 2022 at 9:44 PM Arwed Meyer <arwed.meyer@gmx.de> wrote:

> Make source buffers const for char be write functions.
> This allows using buffers returned by fifo as buf parameter and source
> buffer
> should not be changed by write functions anyway.
>
> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/char.c          | 4 ++--
>  include/chardev/char.h  | 4 ++--
>  include/sysemu/replay.h | 2 +-
>  replay/replay-char.c    | 2 +-
>  stubs/replay-tools.c    | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 0169d8dde4..b005df3ccf 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -193,7 +193,7 @@ int qemu_chr_be_can_write(Chardev *s)
>      return be->chr_can_read(be->opaque);
>  }
>
> -void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
> +void qemu_chr_be_write_impl(Chardev *s, const uint8_t *buf, int len)
>  {
>      CharBackend *be = s->be;
>
> @@ -202,7 +202,7 @@ void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf,
> int len)
>      }
>  }
>
> -void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len)
> +void qemu_chr_be_write(Chardev *s, const uint8_t *buf, int len)
>  {
>      if (qemu_chr_replay(s)) {
>          if (replay_mode == REPLAY_MODE_PLAY) {
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index a319b5fdff..44cd82e405 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -186,7 +186,7 @@ int qemu_chr_be_can_write(Chardev *s);
>   * the caller should call @qemu_chr_be_can_write to determine how much
> data
>   * the front end can currently accept.
>   */
> -void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len);
> +void qemu_chr_be_write(Chardev *s, const uint8_t *buf, int len);
>
>  /**
>   * qemu_chr_be_write_impl:
> @@ -195,7 +195,7 @@ void qemu_chr_be_write(Chardev *s, uint8_t *buf, int
> len);
>   *
>   * Implementation of back end writing. Used by replay module.
>   */
> -void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
> +void qemu_chr_be_write_impl(Chardev *s, const uint8_t *buf, int len);
>
>  /**
>   * qemu_chr_be_update_read_handlers:
> diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> index 73dee9ccdf..7ec0882b50 100644
> --- a/include/sysemu/replay.h
> +++ b/include/sysemu/replay.h
> @@ -198,7 +198,7 @@ uint64_t blkreplay_next_id(void);
>  /*! Registers char driver to save it's events */
>  void replay_register_char_driver(struct Chardev *chr);
>  /*! Saves write to char device event to the log */
> -void replay_chr_be_write(struct Chardev *s, uint8_t *buf, int len);
> +void replay_chr_be_write(struct Chardev *s, const uint8_t *buf, int len);
>  /*! Writes char write return value to the replay log. */
>  void replay_char_write_event_save(int res, int offset);
>  /*! Reads char write return value from the replay log. */
> diff --git a/replay/replay-char.c b/replay/replay-char.c
> index d2025948cf..a31aded032 100644
> --- a/replay/replay-char.c
> +++ b/replay/replay-char.c
> @@ -48,7 +48,7 @@ void replay_register_char_driver(Chardev *chr)
>      char_drivers[drivers_count++] = chr;
>  }
>
> -void replay_chr_be_write(Chardev *s, uint8_t *buf, int len)
> +void replay_chr_be_write(Chardev *s, const uint8_t *buf, int len)
>  {
>      CharEvent *event = g_new0(CharEvent, 1);
>
> diff --git a/stubs/replay-tools.c b/stubs/replay-tools.c
> index f2e72bb225..3e8ca3212d 100644
> --- a/stubs/replay-tools.c
> +++ b/stubs/replay-tools.c
> @@ -53,7 +53,7 @@ void replay_register_char_driver(struct Chardev *chr)
>  {
>  }
>
> -void replay_chr_be_write(struct Chardev *s, uint8_t *buf, int len)
> +void replay_chr_be_write(struct Chardev *s, const uint8_t *buf, int len)
>  {
>      abort();
>  }
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4906 bytes --]

Re: [PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Marc-André Lureau]

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

Hi

On Thu, Sep 8, 2022 at 9:38 PM Arwed Meyer <arwed.meyer@gmx.de> wrote:

> Make use of fifo8 functions instead of implementing own fifo code.
> This makes the code more readable and reduces risk of bugs.
>
> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/chardev/msmouse.c b/chardev/msmouse.c
> index 95fa488339..e9aa3eab55 100644
> --- a/chardev/msmouse.c
> +++ b/chardev/msmouse.c
> @@ -24,6 +24,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
> +#include "qemu/fifo8.h"
>  #include "chardev/char.h"
>  #include "chardev/char-serial.h"
>  #include "ui/console.h"
> @@ -34,6 +35,12 @@
>  #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
>  #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))
>
> +/* Serial fifo size. */
> +#define MSMOUSE_BUF_SZ 64
>

Why bump to 64 bytes?


> +
> +/* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
> +const uint8_t mouse_id[] = {'M', '3'};
> +
>  struct MouseChardev {
>      Chardev parent;
>
> @@ -42,8 +49,7 @@ struct MouseChardev {
>      int axis[INPUT_AXIS__MAX];
>      bool btns[INPUT_BUTTON__MAX];
>      bool btnc[INPUT_BUTTON__MAX];
> -    uint8_t outbuf[32];
> -    int outlen;
> +    Fifo8 outbuf;
>  };
>  typedef struct MouseChardev MouseChardev;
>
> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>  static void msmouse_chr_accept_input(Chardev *chr)
>  {
>      MouseChardev *mouse = MOUSE_CHARDEV(chr);
> -    int len;
> +    uint32_t len_out, len;
>
> -    len = qemu_chr_be_can_write(chr);
> -    if (len > mouse->outlen) {
> -        len = mouse->outlen;
> -    }
> -    if (!len) {
> +    len_out = qemu_chr_be_can_write(chr);
> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>          return;
>      }
> -
> -    qemu_chr_be_write(chr, mouse->outbuf, len);
> -    mouse->outlen -= len;
> -    if (mouse->outlen) {
> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
> -    }
> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
> +            len_out);
>  }
>
>  static void msmouse_queue_event(MouseChardev *mouse)
> @@ -94,12 +94,11 @@ static void msmouse_queue_event(MouseChardev *mouse)
>          mouse->btnc[INPUT_BUTTON_MIDDLE]) {
>          bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
>          mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
> -        count = 4;
> +        count++;
>

a bit superfluous,  ok

     }
>
> -    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
> -        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
> -        mouse->outlen += count;
> +    if (fifo8_num_free(&mouse->outbuf) >= count) {
> +        fifo8_push_all(&mouse->outbuf, bytes, count);
>      } else {
>          /* queue full -> drop event */
>      }
> @@ -172,9 +171,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void
> *arg)
>                   * cause we behave like a 3 button logitech
>                   * mouse.
>                   */
> -                mouse->outbuf[0] = 'M';
> -                mouse->outbuf[1] = '3';
> -                mouse->outlen = 2;
> +                fifo8_push_all(&mouse->outbuf, mouse_id,
> sizeof(mouse_id));
>                  /* Start sending data to serial. */
>                  msmouse_chr_accept_input(chr);
>              }
> @@ -184,7 +181,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void
> *arg)
>           * Reset mouse buffers on power down.
>           * Mouse won't send anything without power.
>           */
> -        mouse->outlen = 0;
> +        fifo8_reset(&mouse->outbuf);
>          memset(mouse->axis, 0, sizeof(mouse->axis));
>          memset(mouse->btns, false, sizeof(mouse->btns));
>          memset(mouse->btnc, false, sizeof(mouse->btns));
> @@ -204,6 +201,7 @@ static void char_msmouse_finalize(Object *obj)
>      MouseChardev *mouse = MOUSE_CHARDEV(obj);
>
>      qemu_input_handler_unregister(mouse->hs);
> +    fifo8_destroy(&mouse->outbuf);
>  }
>
>  static QemuInputHandler msmouse_handler = {
> @@ -224,6 +222,7 @@ static void msmouse_chr_open(Chardev *chr,
>      mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
>                                              &msmouse_handler);
>      mouse->tiocm = 0;
> +    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
>  }
>
>  static void char_msmouse_class_init(ObjectClass *oc, void *data)
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6667 bytes --]

Re: [PATCH v2 4/5] msmouse: Add pnp data

[Marc-André Lureau]

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

On Thu, Sep 8, 2022 at 9:34 PM Arwed Meyer <arwed.meyer@gmx.de> wrote:

> Make msmouse send serial pnp data.
> Enables you to see nice qemu device name in Win9x.
>
> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>

lgtm
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/msmouse.c | 58 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/chardev/msmouse.c b/chardev/msmouse.c
> index e9aa3eab55..29eb97fedc 100644
> --- a/chardev/msmouse.c
> +++ b/chardev/msmouse.c
> @@ -35,11 +35,24 @@
>  #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
>  #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))
>
> +/* Serial PnP for 6 bit devices/mice sends all ASCII chars - 0x20 */
> +#define M(c) (c - 0x20)
>  /* Serial fifo size. */
>  #define MSMOUSE_BUF_SZ 64
>
>  /* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
>  const uint8_t mouse_id[] = {'M', '3'};
> +/*
> + * PnP start "(", PnP version (1.0), vendor ID, product ID, '\\',
> + * serial ID (omitted), '\\', MS class name, '\\', driver ID (omitted),
> '\\',
> + * product description, checksum, ")"
> + * Missing parts are inserted later.
> + */
> +const uint8_t pnp_data[] = {M('('), 1, '$', M('Q'), M('M'), M('U'),
> +                         M('0'), M('0'), M('0'), M('1'),
> +                         M('\\'), M('\\'),
> +                         M('M'), M('O'), M('U'), M('S'), M('E'),
> +                         M('\\'), M('\\')};
>
>  struct MouseChardev {
>      Chardev parent;
> @@ -154,11 +167,22 @@ static int msmouse_chr_write(struct Chardev *s,
> const uint8_t *buf, int len)
>      return len;
>  }
>
> +static QemuInputHandler msmouse_handler = {
> +    .name  = "QEMU Microsoft Mouse",
> +    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
> +    .event = msmouse_input_event,
> +    .sync  = msmouse_input_sync,
> +};
> +
>  static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
>  {
>      MouseChardev *mouse = MOUSE_CHARDEV(chr);
> -    int c;
> +    int c, i, j;
> +    uint8_t bytes[MSMOUSE_BUF_SZ / 2];
>      int *targ = (int *)arg;
> +    const uint8_t hexchr[16] = {M('0'), M('1'), M('2'), M('3'), M('4'),
> M('5'),
> +                             M('6'), M('7'), M('8'), M('9'), M('A'),
> M('B'),
> +                             M('C'), M('D'), M('E'), M('F')};
>
>      switch (cmd) {
>      case CHR_IOCTL_SERIAL_SET_TIOCM:
> @@ -167,11 +191,30 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void
> *arg)
>          if (MSMOUSE_PWR(mouse->tiocm)) {
>              if (!MSMOUSE_PWR(c)) {
>                  /*
> -                 * Power on after reset: send "M3"
> -                 * cause we behave like a 3 button logitech
> -                 * mouse.
> +                 * Power on after reset: Send ID and PnP data
> +                 * No need to check fifo space as it is empty at this
> point.
>                   */
>                  fifo8_push_all(&mouse->outbuf, mouse_id,
> sizeof(mouse_id));
> +                /* Add PnP data: */
> +                fifo8_push_all(&mouse->outbuf, pnp_data,
> sizeof(pnp_data));
> +                /*
> +                 * Add device description from qemu handler name.
> +                 * Make sure this all fits into the queue beforehand!
> +                 */
> +                c = M(')');
> +                for (i = 0; msmouse_handler.name[i]; i++) {
> +                    bytes[i] = M(msmouse_handler.name[i]);
> +                    c += bytes[i];
> +                }
> +                /* Calc more of checksum */
> +                for (j = 0; j < sizeof(pnp_data); j++) {
> +                    c += pnp_data[j];
> +                }
> +                c &= 0xff;
> +                bytes[i++] = hexchr[c >> 4];
> +                bytes[i++] = hexchr[c & 0x0f];
> +                bytes[i++] = M(')');
> +                fifo8_push_all(&mouse->outbuf, bytes, i);
>                  /* Start sending data to serial. */
>                  msmouse_chr_accept_input(chr);
>              }
> @@ -204,13 +247,6 @@ static void char_msmouse_finalize(Object *obj)
>      fifo8_destroy(&mouse->outbuf);
>  }
>
> -static QemuInputHandler msmouse_handler = {
> -    .name  = "QEMU Microsoft Mouse",
> -    .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
> -    .event = msmouse_input_event,
> -    .sync  = msmouse_input_sync,
> -};
> -
>  static void msmouse_chr_open(Chardev *chr,
>                               ChardevBackend *backend,
>                               bool *be_opened,
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6694 bytes --]

Re: [PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Volker Rümelin]

Am 08.09.22 um 19:31 schrieb Arwed Meyer:

> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>   static void msmouse_chr_accept_input(Chardev *chr)
>   {
>       MouseChardev *mouse = MOUSE_CHARDEV(chr);
> -    int len;
> +    uint32_t len_out, len;
>
> -    len = qemu_chr_be_can_write(chr);
> -    if (len > mouse->outlen) {
> -        len = mouse->outlen;
> -    }
> -    if (!len) {
> +    len_out = qemu_chr_be_can_write(chr);
> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>           return;
>       }
> -
> -    qemu_chr_be_write(chr, mouse->outbuf, len);
> -    mouse->outlen -= len;
> -    if (mouse->outlen) {
> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
> -    }
> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
> +            len_out);

Hi Arwed,

I think C function arguments are not evaluated in a defined order. It's 
not defined if the third argument of function qemu_chr_be_write() is 
len_out before or after the call to fifo8_pop_buf().

The fifo_pop_buf() function uses a ringbuffer. When the buffer wraps 
around at the end and the ringbuffer contains more than one byte you may 
need two fifo8_pop_buf() and qemu_chr_be_write() calls to write all 
bytes. The code you replace doesn't have that problem.

Some chardev frontends don't return the total number of bytes to write 
in qemu_chr_be_can_write(). They return the number of bytes that can be 
written with one qemu_chr_be_write() call. You need another 
qemu_chr_be_can_write() call after the qemu_chr_be_write() call to see 
if more bytes can be written.

The code in function gd_vc_send_chars() in ui/gtk.c could be used as a 
template to avoid the three issues above.

With best regards,
Volker

>   }
>
>   static void msmouse_queue_event(MouseChardev *mouse)

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Arwed Meyer]

Am 08.09.22 um 23:11 schrieb Peter Maydell:
> On Thu, 8 Sept 2022 at 18:43, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>>
>> Detect mouse reset via RTS or DTR line:
>> Don't send or process anything while in reset.
>> When coming out of reset, send ID sequence first thing.
>> This allows msmouse to be detected by common mouse drivers.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
>> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>
> How does this work across migration? There doesn't seem
> to be anything that handles migration of the existing
> state inside the MouseChardev either, though...
>
> -- PMM
Hi,

can you please explain in more detail what you don't understand and what
you mean by "migration"?

Re: [PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Arwed Meyer]

Hi,

Am 09.09.22 um 15:18 schrieb Marc-André Lureau:
> Hi
>
> On Thu, Sep 8, 2022 at 9:38 PM Arwed Meyer <arwed.meyer@gmx.de
> <mailto:arwed.meyer@gmx.de>> wrote:
>
>     Make use of fifo8 functions instead of implementing own fifo code.
>     This makes the code more readable and reduces risk of bugs.
>
>     Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de
>     <mailto:arwed.meyer@gmx.de>>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
> <mailto:marcandre.lureau@redhat.com>>
>
>     ---
>       chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
>       1 file changed, 21 insertions(+), 22 deletions(-)
>
>     diff --git a/chardev/msmouse.c b/chardev/msmouse.c
>     index 95fa488339..e9aa3eab55 100644
>     --- a/chardev/msmouse.c
>     +++ b/chardev/msmouse.c
>     @@ -24,6 +24,7 @@
>
>       #include "qemu/osdep.h"
>       #include "qemu/module.h"
>     +#include "qemu/fifo8.h"
>       #include "chardev/char.h"
>       #include "chardev/char-serial.h"
>       #include "ui/console.h"
>     @@ -34,6 +35,12 @@
>       #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
>       #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))
>
>     +/* Serial fifo size. */
>     +#define MSMOUSE_BUF_SZ 64
>
>
> Why bump to 64 bytes?
That's to make some extra space for PnP data (see PATCH v2 4/5). Didn't
want to leave it at 32 (which seems rather random as well) just to
change it again in the next patch.
>
>     +
>     +/* Mouse ID: Send "M3" cause we behave like a 3 button logitech
>     mouse. */
>     +const uint8_t mouse_id[] = {'M', '3'};
>     +
>       struct MouseChardev {
>           Chardev parent;
>
>     @@ -42,8 +49,7 @@ struct MouseChardev {
>           int axis[INPUT_AXIS__MAX];
>           bool btns[INPUT_BUTTON__MAX];
>           bool btnc[INPUT_BUTTON__MAX];
>     -    uint8_t outbuf[32];
>     -    int outlen;
>     +    Fifo8 outbuf;
>       };
>       typedef struct MouseChardev MouseChardev;
>
>     @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev,
>     MOUSE_CHARDEV,
>       static void msmouse_chr_accept_input(Chardev *chr)
>       {
>           MouseChardev *mouse = MOUSE_CHARDEV(chr);
>     -    int len;
>     +    uint32_t len_out, len;
>
>     -    len = qemu_chr_be_can_write(chr);
>     -    if (len > mouse->outlen) {
>     -        len = mouse->outlen;
>     -    }
>     -    if (!len) {
>     +    len_out = qemu_chr_be_can_write(chr);
>     +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>               return;
>           }
>     -
>     -    qemu_chr_be_write(chr, mouse->outbuf, len);
>     -    mouse->outlen -= len;
>     -    if (mouse->outlen) {
>     -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
>     -    }
>     +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
>     +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len,
>     &len_out),
>     +            len_out);
>       }
>
>       static void msmouse_queue_event(MouseChardev *mouse)
>     @@ -94,12 +94,11 @@ static void msmouse_queue_event(MouseChardev *mouse)
>               mouse->btnc[INPUT_BUTTON_MIDDLE]) {
>               bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
>               mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
>     -        count = 4;
>     +        count++;
>
>
> a bit superfluous,  ok
Well... at least on x86 I think this may save a byte or two.
>
>           }
>
>     -    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
>     -        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
>     -        mouse->outlen += count;
>     +    if (fifo8_num_free(&mouse->outbuf) >= count) {
>     +        fifo8_push_all(&mouse->outbuf, bytes, count);
>           } else {
>               /* queue full -> drop event */
>           }
>     @@ -172,9 +171,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd,
>     void *arg)
>                        * cause we behave like a 3 button logitech
>                        * mouse.
>                        */
>     -                mouse->outbuf[0] = 'M';
>     -                mouse->outbuf[1] = '3';
>     -                mouse->outlen = 2;
>     +                fifo8_push_all(&mouse->outbuf, mouse_id,
>     sizeof(mouse_id));
>                       /* Start sending data to serial. */
>                       msmouse_chr_accept_input(chr);
>                   }
>     @@ -184,7 +181,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd,
>     void *arg)
>                * Reset mouse buffers on power down.
>                * Mouse won't send anything without power.
>                */
>     -        mouse->outlen = 0;
>     +        fifo8_reset(&mouse->outbuf);
>               memset(mouse->axis, 0, sizeof(mouse->axis));
>               memset(mouse->btns, false, sizeof(mouse->btns));
>               memset(mouse->btnc, false, sizeof(mouse->btns));
>     @@ -204,6 +201,7 @@ static void char_msmouse_finalize(Object *obj)
>           MouseChardev *mouse = MOUSE_CHARDEV(obj);
>
>           qemu_input_handler_unregister(mouse->hs);
>     +    fifo8_destroy(&mouse->outbuf);
>       }
>
>       static QemuInputHandler msmouse_handler = {
>     @@ -224,6 +222,7 @@ static void msmouse_chr_open(Chardev *chr,
>           mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
>                                                   &msmouse_handler);
>           mouse->tiocm = 0;
>     +    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
>       }
>
>       static void char_msmouse_class_init(ObjectClass *oc, void *data)
>     --
>     2.34.1
>
>
>
>
> --
> Marc-André Lureau

Re: [PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Arwed Meyer]

Am 11.09.22 um 08:12 schrieb Volker Rümelin:
> Am 08.09.22 um 19:31 schrieb Arwed Meyer:
>
>> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>>   static void msmouse_chr_accept_input(Chardev *chr)
>>   {
>>       MouseChardev *mouse = MOUSE_CHARDEV(chr);
>> -    int len;
>> +    uint32_t len_out, len;
>>
>> -    len = qemu_chr_be_can_write(chr);
>> -    if (len > mouse->outlen) {
>> -        len = mouse->outlen;
>> -    }
>> -    if (!len) {
>> +    len_out = qemu_chr_be_can_write(chr);
>> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>>           return;
>>       }
>> -
>> -    qemu_chr_be_write(chr, mouse->outbuf, len);
>> -    mouse->outlen -= len;
>> -    if (mouse->outlen) {
>> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
>> -    }
>> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
>> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
>> +            len_out);
>
> Hi Arwed,
>
> I think C function arguments are not evaluated in a defined order. It's
> not defined if the third argument of function qemu_chr_be_write() is
> len_out before or after the call to fifo8_pop_buf().
>
> The fifo_pop_buf() function uses a ringbuffer. When the buffer wraps
> around at the end and the ringbuffer contains more than one byte you may
> need two fifo8_pop_buf() and qemu_chr_be_write() calls to write all
> bytes. The code you replace doesn't have that problem.
>
> Some chardev frontends don't return the total number of bytes to write
> in qemu_chr_be_can_write(). They return the number of bytes that can be
> written with one qemu_chr_be_write() call. You need another
> qemu_chr_be_can_write() call after the qemu_chr_be_write() call to see
> if more bytes can be written.
>
> The code in function gd_vc_send_chars() in ui/gtk.c could be used as a
> template to avoid the three issues above.
>
> With best regards,
> Volker
>
>>   }
>>
>>   static void msmouse_queue_event(MouseChardev *mouse)
>

Hi Volker,

have to admit I was not completely sure about the order in which
function arguments get evaluated either. Thanks for the confirmation.
I'll change this to be safe.

I guess you probably won't see much difference between old and new code
in practice since serial I/O is slow anyway, but since I'll change the
patch anyway, I'll take the code from gd_vc_send_chars. Indeed looks better.

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Peter Maydell]

On Sun, 11 Sept 2022 at 18:14, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>
> Am 08.09.22 um 23:11 schrieb Peter Maydell:
> > On Thu, 8 Sept 2022 at 18:43, Arwed Meyer <arwed.meyer@gmx.de> wrote:
> >>
> >> Detect mouse reset via RTS or DTR line:
> >> Don't send or process anything while in reset.
> >> When coming out of reset, send ID sequence first thing.
> >> This allows msmouse to be detected by common mouse drivers.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
> >> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
> >
> > How does this work across migration? There doesn't seem
> > to be anything that handles migration of the existing
> > state inside the MouseChardev either, though...

> can you please explain in more detail what you don't understand and what
> you mean by "migration"?

Migration is when the state of the whole VM is copied from
one host to another, or, equivalently, when it is saved to
the disk image with 'savevm' to be restarted later. For this
to work all the guest-changeable state in the whole VM (all
device registers, internal state, etc) has to be saved so
it can be reloaded on the destination. My question is basically
how this new state in mouse->tiocm, and more generally how
the existing other state fields in that struct, are saved
and loaded during the migration process. I know how this
works for device models, but I'm not sure how chardevs
figure into it. (Perhaps the answer is "this shouldn't be
a chardev at all" ???)

thanks
-- PMM

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Arwed Meyer]

Am 11.09.22 um 20:27 schrieb Peter Maydell:
> On Sun, 11 Sept 2022 at 18:14, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>>
>> Am 08.09.22 um 23:11 schrieb Peter Maydell:
>>> On Thu, 8 Sept 2022 at 18:43, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>>>>
>>>> Detect mouse reset via RTS or DTR line:
>>>> Don't send or process anything while in reset.
>>>> When coming out of reset, send ID sequence first thing.
>>>> This allows msmouse to be detected by common mouse drivers.
>>>>
>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
>>>> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>>>
>>> How does this work across migration? There doesn't seem
>>> to be anything that handles migration of the existing
>>> state inside the MouseChardev either, though...
>
>> can you please explain in more detail what you don't understand and what
>> you mean by "migration"?
>
> Migration is when the state of the whole VM is copied from
> one host to another, or, equivalently, when it is saved to
> the disk image with 'savevm' to be restarted later. For this
> to work all the guest-changeable state in the whole VM (all
> device registers, internal state, etc) has to be saved so
> it can be reloaded on the destination. My question is basically
> how this new state in mouse->tiocm, and more generally how
> the existing other state fields in that struct, are saved
> and loaded during the migration process. I know how this
> works for device models, but I'm not sure how chardevs
> figure into it. (Perhaps the answer is "this shouldn't be
> a chardev at all" ???)
>
> thanks
> -- PMM

Hi,

thanks for adding some context. Good question.
Unfortunately I don't know the device and migration code much, so I
can't really say anything about this. I guess(!) it should be enough to
save/load contents of struct MouseChardev. No idea if and how this can
be done though.
Also since saving/loading device state wasn't supported in the msmouse
code to begin with, for me this is a new feature and out of scope for
this patch set.
The result of this missing feature might be that mouse buttons are
detected as not pressed and mouse movement data is lost after migration.


Regards

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Arwed Meyer]

Am 12.09.22 um 19:45 schrieb Arwed Meyer:
> Am 11.09.22 um 20:27 schrieb Peter Maydell:
>> On Sun, 11 Sept 2022 at 18:14, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>>>
>>> Am 08.09.22 um 23:11 schrieb Peter Maydell:
>>>> On Thu, 8 Sept 2022 at 18:43, Arwed Meyer <arwed.meyer@gmx.de> wrote:
>>>>>
>>>>> Detect mouse reset via RTS or DTR line:
>>>>> Don't send or process anything while in reset.
>>>>> When coming out of reset, send ID sequence first thing.
>>>>> This allows msmouse to be detected by common mouse drivers.
>>>>>
>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/77
>>>>> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>>>>
>>>> How does this work across migration? There doesn't seem
>>>> to be anything that handles migration of the existing
>>>> state inside the MouseChardev either, though...
>>
>>> can you please explain in more detail what you don't understand and what
>>> you mean by "migration"?
>>
>> Migration is when the state of the whole VM is copied from
>> one host to another, or, equivalently, when it is saved to
>> the disk image with 'savevm' to be restarted later. For this
>> to work all the guest-changeable state in the whole VM (all
>> device registers, internal state, etc) has to be saved so
>> it can be reloaded on the destination. My question is basically
>> how this new state in mouse->tiocm, and more generally how
>> the existing other state fields in that struct, are saved
>> and loaded during the migration process. I know how this
>> works for device models, but I'm not sure how chardevs
>> figure into it. (Perhaps the answer is "this shouldn't be
>> a chardev at all" ???)
>>
>> thanks
>> -- PMM
>
> Hi,
>
> thanks for adding some context. Good question.
> Unfortunately I don't know the device and migration code much, so I
> can't really say anything about this. I guess(!) it should be enough to
> save/load contents of struct MouseChardev. No idea if and how this can
> be done though.
> Also since saving/loading device state wasn't supported in the msmouse
> code to begin with, for me this is a new feature and out of scope for
> this patch set.
> The result of this missing feature might be that mouse buttons are
> detected as not pressed and mouse movement data is lost after migration.
>
>
> Regards

... more specific regarding to mouse->tiocm:
Since it is initialized to 0, the device is put to reset after restoring
state/migrating and won't react. You'd probably have to reload the
client's mouse driver to get the mouse up and running again.


Regards

Re: [PATCH v2 1/5] msmouse: Handle mouse reset

[Peter Maydell]

On Mon, 12 Sept 2022 at 18:45, Arwed Meyer <arwed.meyer@gmx.de> wrote:
> thanks for adding some context. Good question.
> Unfortunately I don't know the device and migration code much, so I
> can't really say anything about this. I guess(!) it should be enough to
> save/load contents of struct MouseChardev. No idea if and how this can
> be done though.
> Also since saving/loading device state wasn't supported in the msmouse
> code to begin with, for me this is a new feature and out of scope for
> this patch set.

Yes, agreed. I was really hoping to catch the attention of one
of the reviewers who's more familiar with chardev...

-- PMM