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-ayl...@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.
>
>
