diff mbox series

[v3,05/11] pckbd: don't update OBF flags if KBD_STAT_OBF is set

Message ID 20210515113254.6245-5-vr_qemu@t-online.de
State New
Headers show
Series PS/2 controller related fixes | expand

Commit Message

Volker Rümelin May 15, 2021, 11:32 a.m. UTC 
Don't update the OBF flags in the status register and the cor-
responding IRQ lines if KBD_STAT_OBF is set. Otherwise this
may change the PS/2 event type. If the guest ISR was already
scheduled, the changed event type will be rather surprising for
the guest.

This fixes a mouse event stream corruption. To reproduce the
problem start a FreeDOS 1.2 guest with -machine pc,accel=kvm
and -display gtk. The KVM in-kernel irqchip has to be enabled.
Now open a text file with edit.exe in the guest and hold down
the cursor right key and at the same time move the mouse around.
You will quickly notice erratic mouse movements and unexpected
mouse clicks.

A trace file shows the mouse event stream corruption. Guest
rip 0xce93 (f000:ce93) is the in al,0x60 instruction in the
seabios mouse ISR, guest rip 0xceca (f000:ceca) is the
in al,0x60 instruction in the seabios keyboard ISR.

qemu-system-x86-5659  [007] ....   280.971116:
 tracing_mark_write: pckbd_kbd_update_irq kbd=0 aux=1
 # gtk queues a mouse event

qemu-system-x86-5665  [000] ....   280.971121:
 kvm_exit: reason EXTERNAL_INTERRUPT rip 0x22da info 0 800000fd
qemu-system-x86-5665  [000] d..1   280.971122:
 kvm_entry: vcpu 0, rip 0x22da
qemu-system-x86-5665  [000] ....   280.971123:
 kvm_exit: reason EXTERNAL_INTERRUPT rip 0x22da info 0 800000fd
qemu-system-x86-5665  [000] d..1   280.971124:
 kvm_entry: vcpu 0, rip 0x22da
qemu-system-x86-5665  [000] ....   280.971126:
 kvm_exit: reason IO_INSTRUCTION rip 0x110c8c info 640008 0
qemu-system-x86-5665  [000] ....   280.971176:
 tracing_mark_write: pckbd_kbd_read_status 0x3d
 # KBD_STAT_OBF and KBD_STAT_MOUSE_OBF set, the mouse ISR will
 # read data from the PS/2 controller.

qemu-system-x86-5665  [000] d..1   280.971180:
 kvm_entry: vcpu 0, rip 0x110c8d
qemu-system-x86-5665  [000] ....   280.971191:
 kvm_exit: reason EXTERNAL_INTERRUPT rip 0x110c8d info 0 800000f6
qemu-system-x86-5665  [000] d..1   280.971191:
 kvm_entry: vcpu 0, rip 0x110c8d
qemu-system-x86-5665  [000] ....   280.971193:
 kvm_exit: reason IO_INSTRUCTION rip 0xce93 info 600048 0
 # the mouse ISR wants to read data from the PS/2 controller

qemu-system-x86-5659  [007] ....   280.971231:
 tracing_mark_write: pckbd_kbd_update_irq kbd=1 aux=0
qemu-system-x86-5659  [007] ....   280.971238:
 tracing_mark_write: pckbd_kbd_update_irq kbd=1 aux=0
 # gtk queues a keyboard event 0xe0 0x4d (key right)

qemu-system-x86-5665  [000] ....   280.971257:
 tracing_mark_write: pckbd_kbd_update_irq kbd=0 aux=1
qemu-system-x86-5665  [000] ....   280.971262:
 tracing_mark_write: pckbd_kbd_update_irq kbd=1 aux=0
 # ps2_read_data() deasserts and reasserts the keyboard IRQ

qemu-system-x86-5665  [000] ....   280.971266:
 tracing_mark_write: pckbd_kbd_read_data 0xe0 kbd
 # -> the mouse ISR receives keyboard data

qemu-system-x86-5665  [000] d..1   280.971268:
 kvm_entry: vcpu 0, rip 0xce95
qemu-system-x86-5665  [000] ....   280.971269:
 kvm_exit: reason IO_INSTRUCTION rip 0xe828 info a00040 0
qemu-system-x86-5665  [000] ....   280.971270:
 kvm_ack_irq: irqchip PIC slave pin 12
qemu-system-x86-5665  [000] d..1   280.971270:
 kvm_entry: vcpu 0, rip 0xe82a
qemu-system-x86-5665  [000] ....   280.971271:
 kvm_exit: reason IO_INSTRUCTION rip 0xe82a info 200040 0
qemu-system-x86-5665  [000] ....   280.971271:
 kvm_ack_irq: irqchip PIC master pin 2
qemu-system-x86-5665  [000] d..1   280.971271:
 kvm_entry: vcpu 0, rip 0xe82c
qemu-system-x86-5665  [000] ....   280.971272:
 kvm_exit: reason PENDING_INTERRUPT rip 0x22da info 0 0
qemu-system-x86-5665  [000] d..1   280.971273:
 kvm_entry: vcpu 0, rip 0x22da
qemu-system-x86-5665  [000] ....   280.971274:
 kvm_exit: reason IO_INSTRUCTION rip 0x110c8c info 640008 0
qemu-system-x86-5665  [000] ....   280.971275:
 tracing_mark_write: pckbd_kbd_read_status 0x1d
qemu-system-x86-5665  [000] d..1   280.971276:
 kvm_entry: vcpu 0, rip 0x110c8d
qemu-system-x86-5665  [000] ....   280.971277:
 kvm_exit: reason IO_INSTRUCTION rip 0xceca info 600048 0
 # the keyboard ISR wants to read data from the PS/2 controller

qemu-system-x86-5665  [000] ....   280.971279:
 tracing_mark_write: pckbd_kbd_update_irq kbd=0 aux=1
qemu-system-x86-5665  [000] ....   280.971282:
 tracing_mark_write: pckbd_kbd_read_data 0x4d kbd
 # the keyboard ISR receives the second byte of the keyboard event

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/pckbd.c | 91 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 23 deletions(-)

Comments

Gerd Hoffmann May 18, 2021, 10:49 a.m. UTC | #1 
Hi,

>  static const VMStateDescription vmstate_kbd = {
>      .name = "pckbd",
> -    .version_id = 3,
> +    .version_id = 4,
>      .minimum_version_id = 3,
>      .post_load = kbd_post_load,
>      .fields = (VMStateField[]) {
> @@ -435,6 +471,7 @@ static const VMStateDescription vmstate_kbd = {
>          VMSTATE_UINT8(status, KBDState),
>          VMSTATE_UINT8(mode, KBDState),
>          VMSTATE_UINT8(pending, KBDState),
> +        VMSTATE_UINT8_V(obdata, KBDState, 4),
>          VMSTATE_END_OF_LIST()
>      },
>      .subsections = (const VMStateDescription*[]) {
> @@ -512,12 +549,20 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
>      qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
>  }

Unfortunately live migration isn't that easy.  Reason is we want able to
live-migrate both ways (old qemu -> new qemu but also new qemu -> old qemu).
With version ids we can do old qemu -> new qemu only.

So the usual way to deal with this is that new features which require
additional state information can be enabled/disabled at runtime using
device properties.  The feature is turned off for the compatibility
machine types.  The additional state is added using a conditional
subsection, which is only sent in case the feature is enabled.  That way
qemu with -- says -- "-machine pc-q35-5.0" will only enable features and
sent vmstate which qemu 5.0 is able to deal with.

I think there is no way around such a property for the ps2 fixes, even
if we use it for sending/not sending the additional ps2 state
information needed by the bugfixes.  Making the fixes itself conditional
should not be needed I think.

take care,
  Gerd
Volker Rümelin May 18, 2021, 7:37 p.m. UTC | #2 
>    Hi,
>
>>   static const VMStateDescription vmstate_kbd = {
>>       .name = "pckbd",
>> -    .version_id = 3,
>> +    .version_id = 4,
>>       .minimum_version_id = 3,
>>       .post_load = kbd_post_load,
>>       .fields = (VMStateField[]) {
>> @@ -435,6 +471,7 @@ static const VMStateDescription vmstate_kbd = {
>>           VMSTATE_UINT8(status, KBDState),
>>           VMSTATE_UINT8(mode, KBDState),
>>           VMSTATE_UINT8(pending, KBDState),
>> +        VMSTATE_UINT8_V(obdata, KBDState, 4),
>>           VMSTATE_END_OF_LIST()
>>       },
>>       .subsections = (const VMStateDescription*[]) {
>> @@ -512,12 +549,20 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
>>       qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
>>   }
> Unfortunately live migration isn't that easy.  Reason is we want able to
> live-migrate both ways (old qemu -> new qemu but also new qemu -> old qemu).
> With version ids we can do old qemu -> new qemu only.
>
> So the usual way to deal with this is that new features which require
> additional state information can be enabled/disabled at runtime using
> device properties.  The feature is turned off for the compatibility
> machine types.  The additional state is added using a conditional
> subsection, which is only sent in case the feature is enabled.  That way
> qemu with -- says -- "-machine pc-q35-5.0" will only enable features and
> sent vmstate which qemu 5.0 is able to deal with.
>
> I think there is no way around such a property for the ps2 fixes, even
> if we use it for sending/not sending the additional ps2 state
> information needed by the bugfixes.  Making the fixes itself conditional
> should not be needed I think.

Thank you for the detailed information. This is really helpful.
I will send a version 4 series. It may take a few days.

With best regards,
Volker

> take care,
>    Gerd
>
diff mbox series

Patch

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 90b33954a8..8336f4e4b3 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -139,6 +139,7 @@  typedef struct KBDState {
     bool outport_present;
     /* Bitmask of devices with data available.  */
     uint8_t pending;
+    uint8_t obdata;
     void *kbd;
     void *mouse;
 
@@ -173,6 +174,13 @@  static void kbd_update_irq_lines(KBDState *s)
     qemu_set_irq(s->irq_mouse, irq_mouse_level);
 }
 
+static void kbd_deassert_irq(KBDState *s)
+{
+    s->status &= ~(KBD_STAT_OBF | KBD_STAT_MOUSE_OBF);
+    s->outport &= ~(KBD_OUT_OBF | KBD_OUT_MOUSE_OBF);
+    kbd_update_irq_lines(s);
+}
+
 /* update irq and KBD_STAT_[MOUSE_]OBF */
 static void kbd_update_irq(KBDState *s)
 {
@@ -181,7 +189,6 @@  static void kbd_update_irq(KBDState *s)
     if (s->pending) {
         s->status |= KBD_STAT_OBF;
         s->outport |= KBD_OUT_OBF;
-        /* kbd data takes priority over aux data.  */
         if (s->pending == KBD_PENDING_AUX) {
             s->status |= KBD_STAT_MOUSE_OBF;
             s->outport |= KBD_OUT_MOUSE_OBF;
@@ -190,26 +197,42 @@  static void kbd_update_irq(KBDState *s)
     kbd_update_irq_lines(s);
 }
 
+static void kbd_safe_update_irq(KBDState *s)
+{
+    /*
+     * with KBD_STAT_OBF set, a call to kbd_read_data() will eventually call
+     * kbd_update_irq()
+     */
+    if (s->status & KBD_STAT_OBF) {
+        return;
+    }
+    if (s->pending) {
+        kbd_update_irq(s);
+    }
+}
+
 static void kbd_update_kbd_irq(void *opaque, int level)
 {
-    KBDState *s = (KBDState *)opaque;
+    KBDState *s = opaque;
 
-    if (level)
+    if (level) {
         s->pending |= KBD_PENDING_KBD;
-    else
+    } else {
         s->pending &= ~KBD_PENDING_KBD;
-    kbd_update_irq(s);
+    }
+    kbd_safe_update_irq(s);
 }
 
 static void kbd_update_aux_irq(void *opaque, int level)
 {
-    KBDState *s = (KBDState *)opaque;
+    KBDState *s = opaque;
 
-    if (level)
+    if (level) {
         s->pending |= KBD_PENDING_AUX;
-    else
+    } else {
         s->pending &= ~KBD_PENDING_AUX;
-    kbd_update_irq(s);
+    }
+    kbd_safe_update_irq(s);
 }
 
 static uint64_t kbd_read_status(void *opaque, hwaddr addr,
@@ -290,11 +313,10 @@  static void kbd_write_command(void *opaque, hwaddr addr,
         break;
     case KBD_CCMD_KBD_DISABLE:
         s->mode |= KBD_MODE_DISABLE_KBD;
-        kbd_update_irq(s);
         break;
     case KBD_CCMD_KBD_ENABLE:
         s->mode &= ~KBD_MODE_DISABLE_KBD;
-        kbd_update_irq(s);
+        kbd_safe_update_irq(s);
         break;
     case KBD_CCMD_READ_INPORT:
         kbd_queue(s, 0x80, 0);
@@ -327,15 +349,19 @@  static uint64_t kbd_read_data(void *opaque, hwaddr addr,
                               unsigned size)
 {
     KBDState *s = opaque;
-    uint32_t val;
+    uint8_t status = s->status;
 
-    if (s->pending == KBD_PENDING_AUX)
-        val = ps2_read_data(s->mouse);
-    else
-        val = ps2_read_data(s->kbd);
+    if (status & KBD_STAT_OBF) {
+        kbd_deassert_irq(s);
+        if (status & KBD_STAT_MOUSE_OBF) {
+            s->obdata = ps2_read_data(s->mouse);
+        } else {
+            s->obdata = ps2_read_data(s->kbd);
+        }
+    }
 
-    trace_pckbd_kbd_read_data(val);
-    return val;
+    trace_pckbd_kbd_read_data(s->obdata);
+    return s->obdata;
 }
 
 static void kbd_write_data(void *opaque, hwaddr addr,
@@ -352,8 +378,16 @@  static void kbd_write_data(void *opaque, hwaddr addr,
     case KBD_CCMD_WRITE_MODE:
         s->mode = val;
         ps2_keyboard_set_translation(s->kbd, (s->mode & KBD_MODE_KCC) != 0);
-        /* ??? */
-        kbd_update_irq(s);
+        /*
+         * a write to the mode byte interrupt enable flags directly updates
+         * the irq lines
+         */
+        kbd_update_irq_lines(s);
+        /*
+         * a write to the mode byte disable interface flags may raise
+         * an irq if there is pending data in the PS/2 queues.
+         */
+        kbd_safe_update_irq(s);
         break;
     case KBD_CCMD_WRITE_OBUF:
         kbd_queue(s, val, 0);
@@ -381,6 +415,8 @@  static void kbd_reset(void *opaque)
     s->status = KBD_STAT_CMD | KBD_STAT_UNLOCKED;
     s->outport = KBD_OUT_RESET | KBD_OUT_A20 | KBD_OUT_ONES;
     s->outport_present = false;
+    s->pending = 0;
+    kbd_deassert_irq(s);
 }
 
 static uint8_t kbd_outport_default(KBDState *s)
@@ -427,7 +463,7 @@  static int kbd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_kbd = {
     .name = "pckbd",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .post_load = kbd_post_load,
     .fields = (VMStateField[]) {
@@ -435,6 +471,7 @@  static const VMStateDescription vmstate_kbd = {
         VMSTATE_UINT8(status, KBDState),
         VMSTATE_UINT8(mode, KBDState),
         VMSTATE_UINT8(pending, KBDState),
+        VMSTATE_UINT8_V(obdata, KBDState, 4),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
@@ -512,12 +549,20 @@  void i8042_setup_a20_line(ISADevice *dev, qemu_irq a20_out)
     qdev_connect_gpio_out_named(DEVICE(dev), I8042_A20_LINE, 0, a20_out);
 }
 
+static bool vmstate_version_id_3(void *opaque, int version_id)
+{
+    return version_id == 3;
+}
+
 static const VMStateDescription vmstate_kbd_isa = {
     .name = "pckbd",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT(kbd, ISAKBDState, 0, vmstate_kbd, KBDState),
+        VMSTATE_VSTRUCT_TEST(kbd, ISAKBDState, vmstate_version_id_3,
+                             3, vmstate_kbd, KBDState, 3),
+        VMSTATE_VSTRUCT_TEST(kbd, ISAKBDState, NULL,
+                             4, vmstate_kbd, KBDState, 4),
         VMSTATE_END_OF_LIST()
     }
 };