From f155cbd57b37fa600c580ed30d593f47383ecd38 Mon Sep 17 00:00:00 2001
From: Carl Hauser <chauser@pullman.com>
Date: Fri, 16 Aug 2024 09:20:36 -0700
Subject: [PATCH] hw/char: suppress sunmouse events with no changes

Sun optical mice circa 1993 were based on the Mouse Systems
Corp. optical mice. The technical manual for those mice
states that mice only send events when there is motion or the
button state changes. The Solaris 2.5.6 mouse driver seems
to be confused by updates that don't follow this specification.

This patch adds a field to the ESCCChannelState to contain
the button state sent in the last event and uses that in
sunmouse_event to avoid sending unnecessary updates.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2518
Signed-off-by: Carl Hauser <chauser@pullman.com>
---
  hw/char/escc.c         | 10 ++++++++++
  include/hw/char/escc.h |  1 +
  2 files changed, 11 insertions(+)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index d450d70eda..7732141cf5 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -287,6 +287,7 @@ static void escc_reset_chn(ESCCChannelState *s)
      s->rxint = s->txint = 0;
      s->rxint_under_svc = s->txint_under_svc = 0;
      s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
+    s->sunmouse_prev_state = 0;
      clear_queue(s);
  }

@@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque,
      int ch;

      trace_escc_sunmouse_event(dx, dy, buttons_state);
+
+    /* Don't send duplicate events without motion */
+    if (dx == 0 &&
+        dy == 0 &&
+        (s->sunmouse_prev_state ^ buttons_state) == 0) {
+        return;
+    }
+    s->sunmouse_prev_state = buttons_state;
+
      ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */

      if (buttons_state & MOUSE_EVENT_LBUTTON) {
diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h
index 5669a5b811..bc5ba4f564 100644
--- a/include/hw/char/escc.h
+++ b/include/hw/char/escc.h
@@ -46,6 +46,7 @@ typedef struct ESCCChannelState {
      uint8_t rx, tx;
      QemuInputHandlerState *hs;
      char *sunkbd_layout;
+    int sunmouse_prev_state;
  } ESCCChannelState;

  struct ESCCState {
-- 
2.34.1

On 8/20/24 09:18, Carl Hauser wrote:
> @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque,
>       int ch;
> 
>       trace_escc_sunmouse_event(dx, dy, buttons_state);
> +
> +    /* Don't send duplicate events without motion */
> +    if (dx == 0 &&
> +        dy == 0 &&
> +        (s->sunmouse_prev_state ^ buttons_state) == 0) {

Were you intending to mask vs MOUSE_EVENT_*BUTTON?
Otherwise this is just plain equality.

> diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h
> index 5669a5b811..bc5ba4f564 100644
> --- a/include/hw/char/escc.h
> +++ b/include/hw/char/escc.h
> @@ -46,6 +46,7 @@ typedef struct ESCCChannelState {
>       uint8_t rx, tx;
>       QemuInputHandlerState *hs;
>       char *sunkbd_layout;
> +    int sunmouse_prev_state;

This adds new state that must be migrated.

While the patch is relatively simple, I do wonder if this code could be improved by 
converting away from the legacy mouse interface to qemu_input_handler_register. 
Especially if that might help avoid needing to add migration state that isn't "really" 
part of the device.

Mark?


r~

On 20/08/2024 08:34, Richard Henderson wrote:

> On 8/20/24 09:18, Carl Hauser wrote:
>> @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque,
>>       int ch;
>>
>>       trace_escc_sunmouse_event(dx, dy, buttons_state);
>> +
>> +    /* Don't send duplicate events without motion */
>> +    if (dx == 0 &&
>> +        dy == 0 &&
>> +        (s->sunmouse_prev_state ^ buttons_state) == 0) {
> 
> Were you intending to mask vs MOUSE_EVENT_*BUTTON?
> Otherwise this is just plain equality.
> 
>> diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h
>> index 5669a5b811..bc5ba4f564 100644
>> --- a/include/hw/char/escc.h
>> +++ b/include/hw/char/escc.h
>> @@ -46,6 +46,7 @@ typedef struct ESCCChannelState {
>>       uint8_t rx, tx;
>>       QemuInputHandlerState *hs;
>>       char *sunkbd_layout;
>> +    int sunmouse_prev_state;
> 
> This adds new state that must be migrated.
> 
> While the patch is relatively simple, I do wonder if this code could be improved by 
> converting away from the legacy mouse interface to qemu_input_handler_register. 
> Especially if that might help avoid needing to add migration state that isn't 
> "really" part of the device.
> 
> Mark?

Ooof I didn't even realise that qemu_add_mouse_event_handler() was legacy - is that 
documented anywhere at all?

At first glance (e.g. 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/input/ps2.c?ref_type=heads#L789) 
it appears that movement and button events are handled separately which I think would 
solve the problem.

I can try and put something together quickly for Carl to test and improve if that 
helps, although I'm quite tied up with client work and life in general right now(!).


ATB,

Mark.

More on whether the sunmouse_prev_state needs to be migrated. I think
that if it were NOT included in the migration state the following
would hold, assuming that it would be initialized to 0 in the "to"
environment:
1. If prev_state is 0 in the "from" environment the prev_state in the
"to" environment would be correct -- and this would be the case almost
all the time.
2. If it is non-0 in the "from" environment (so the guest OS believes
there is a mouse button pressed) and the first mouse event in the "to"
environment has motion, the event will be sent correctly and the
prev_state is then correct; likely almost all of the rest of the time.
3. If it is non-0 in the "from" environment (so the guest OS believes
there is a mouse button pressed) and the first mouse event in the "to"
environment has no motion and non-zero button state the event will be
sent and there are two cases:
3a. if the button state is NOT the same as the prev state in the
"from" environment the event will have been sent correctly.
*3b. if the button state IS the same as prev_state  in the "from"
environment (so the guest OS believes there is a mouse button pressed)
the event will have been sent incorrectly and is a duplicate. The
prev_state in the "to" environment will now be correct and no further
duplicates will be sent.
*4. If it is non-0 in the "from" environment (so the guest OS believes
there is a mouse button pressed) and the first mouse event(s) in the
"to" environment has no motion and zero button state, the event will
not be sent. It is not apparent that the code that carries mouse
events from the host to the escc driver level would ever produce such
an event as the first one or ones, but if it did the state would
persist until there was mouse motion or a button press event. The
consequence would be that the guest would continue to understand that
a mouse button was pressed when it was not. The situation gets
corrected on any mouse movement or button press.

So the starred cases, 3b and 4, are the only incorrect behaviors. Both
are likely extremely rare and hard to make happen. Case 3b results in
one spurious duplicate event sent to the guest. I have only seen
Solaris misbehave in  the face of a flood of duplicate events. My
testing with linux didn't uncover any misbehavior even when flooded
with duplicates. NetBSD 10 has other mouse misbehaviors that make it
hard to tell whether or not duplicates matter there. In case 4, the
guest would continue to believe a mouse button was depressed when it
wasn't but this would be cured by any button press or movement.

It appears that this is all quite similar to the way caps_lock_mode
and num_lock_mode are handled in escc -- they are both part of the
ESCCChannelState but not part of the migration state and I would
expect would exhibit similar need to see transitions in some cases
before getting the state fully correct after a migration.

On Wed, Aug 21, 2024 at 7:18 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 20/08/2024 08:34, Richard Henderson wrote:
>
> > On 8/20/24 09:18, Carl Hauser wrote:
> >> @@ -959,6 +960,15 @@ static void sunmouse_event(void *opaque,
> >>       int ch;
> >>
> >>       trace_escc_sunmouse_event(dx, dy, buttons_state);
> >> +
> >> +    /* Don't send duplicate events without motion */
> >> +    if (dx == 0 &&
> >> +        dy == 0 &&
> >> +        (s->sunmouse_prev_state ^ buttons_state) == 0) {
> >
> > Were you intending to mask vs MOUSE_EVENT_*BUTTON?
> > Otherwise this is just plain equality.
> >
> >> diff --git a/include/hw/char/escc.h b/include/hw/char/escc.h
> >> index 5669a5b811..bc5ba4f564 100644
> >> --- a/include/hw/char/escc.h
> >> +++ b/include/hw/char/escc.h
> >> @@ -46,6 +46,7 @@ typedef struct ESCCChannelState {
> >>       uint8_t rx, tx;
> >>       QemuInputHandlerState *hs;
> >>       char *sunkbd_layout;
> >> +    int sunmouse_prev_state;
> >
> > This adds new state that must be migrated.
> >
> > While the patch is relatively simple, I do wonder if this code could be improved by
> > converting away from the legacy mouse interface to qemu_input_handler_register.
> > Especially if that might help avoid needing to add migration state that isn't
> > "really" part of the device.
> >
> > Mark?
>
> Ooof I didn't even realise that qemu_add_mouse_event_handler() was legacy - is that
> documented anywhere at all?
>
> At first glance (e.g.
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/input/ps2.c?ref_type=heads#L789)
> it appears that movement and button events are handled separately which I think would
> solve the problem.
>
> I can try and put something together quickly for Carl to test and improve if that
> helps, although I'm quite tied up with client work and life in general right now(!).
>
>
> ATB,
>
> Mark.
>
>