[PATCH v3 00/11] PS/2 controller related fixes

[PATCH v3 00/11] PS/2 controller related fixes

[Volker Rümelin]

This patch series fixes two different PS/2 mouse stream corruptions
and adds a feature that allows some old misbehaving DOS programs to
have a working keyboard. With the last few patches, the PS/2 con-
troller behaves more like a real controller.

v2:
Introduce the function kbd_pending() in a preliminary patch to ease
the review of patch "pckbd: correctly disable PS/2 communication",
as Phillipe suggested.

v3:
Patch "pckbd: correctly disable PS/2 communication" exposed a bug
in SeaBIOS. The updated patch keeps the relevant code. Until
SeaBIOS is fixed, the PS/2 controller command KBD_CCMD_KBD_DISABLE
must disable the keyboard interrupt.

In patch "pckbd: PS/2 keyboard throttle" in function
kbd_throttle_timeout() an unnecessary if statement was removed.
The KBD_STAT_OBF flag is never set when kbd_throttle_timeout()
gets called.

Volker Rümelin (11):
   ps2: fix mouse stream corruption
   ps2: don't raise an interrupt if queue is full
   ps2: don't deassert irq twice if queue is empty
   pckbd: split out interrupt line changing code
   pckbd: don't update OBF flags if KBD_STAT_OBF is set
   pckbd: PS/2 keyboard throttle
   pckbd: add state variable for interrupt source
   pckbd: add controller response queue
   pckbd: add function kbd_pending()
   pckbd: correctly disable PS/2 communication
   pckbd: remove duplicated keyboard and mouse defines

  hw/input/pckbd.c | 289 ++++++++++++++++++++++++++++++++++-------------
  hw/input/ps2.c   |  11 +-
  2 files changed, 221 insertions(+), 79 deletions(-)

-- 
2.26.2

[PATCH v3 01/11] ps2: fix mouse stream corruption

[Volker Rümelin]

Commit 7abe7eb294 "ps2: Fix mouse stream corruption due to lost data"
added code to avoid mouse stream corruptions but the calculation of
the needed free queue size was wrong. Fix this.

To reproduce, open a text file with the vim 7.3 32 bit for DOS exe-
cutable in a FreeDOS client started with -display sdl and move the
mouse around for a few seconds. You will quickly see erratic mouse
movements and unexpected mouse clicks. CuteMouse (ctmouse.exe) in
FreeDOS doesn't try to re-sync the mouse stream.

Fixes: 7abe7eb294 ("ps2: Fix mouse stream corruption due to lost data")
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 72cdb80ae1..d9f79e8260 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -645,7 +645,8 @@ void ps2_keyboard_set_translation(void *opaque, int mode)
 
 static int ps2_mouse_send_packet(PS2MouseState *s)
 {
-    const int needed = 3 + (s->mouse_type - 2);
+    /* IMPS/2 and IMEX send 4 bytes, PS2 sends 3 bytes */
+    const int needed = s->mouse_type ? 4 : 3;
     unsigned int b;
     int dx1, dy1, dz1;
 
-- 
2.26.2

[PATCH v3 02/11] ps2: don't raise an interrupt if queue is full

[Volker Rümelin]

ps2_queue() behaves differently than the very similar functions
ps2_queue_2() to ps2_queue_4(). The first one calls update_irq()
even if the queue is full, the others don't. Change ps2_queue()
to be consistent with the others.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index d9f79e8260..55a81a0c51 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -212,6 +212,10 @@ void ps2_raise_irq(PS2State *s)
 
 void ps2_queue(PS2State *s, int b)
 {
+    if (PS2_QUEUE_SIZE - s->queue.count < 1) {
+        return;
+    }
+
     ps2_queue_noirq(s, b);
     s->update_irq(s->update_arg, 1);
 }
-- 
2.26.2

[PATCH v3 03/11] ps2: don't deassert irq twice if queue is empty

[Volker Rümelin]

Don't deassert the irq twice if the queue is empty. While the
second deassertion doesn't do any harm, it's unnecessary.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 55a81a0c51..4aee46a595 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -519,7 +519,9 @@ uint32_t ps2_read_data(PS2State *s)
         /* reading deasserts IRQ */
         s->update_irq(s->update_arg, 0);
         /* reassert IRQs if data left */
-        s->update_irq(s->update_arg, q->count != 0);
+        if (q->count) {
+            s->update_irq(s->update_arg, 1);
+        }
     }
     return val;
 }
-- 
2.26.2

[PATCH v3 04/11] pckbd: split out interrupt line changing code

[Volker Rümelin]

Split out the interrupt line changing code from kbd_update_irq().
This is a preparation for the next patch. There is no functional
change.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/pckbd.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index dde85ba6c6..90b33954a8 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -148,15 +148,34 @@ typedef struct KBDState {
     hwaddr mask;
 } KBDState;
 
-/* update irq and KBD_STAT_[MOUSE_]OBF */
 /* XXX: not generating the irqs if KBD_MODE_DISABLE_KBD is set may be
    incorrect, but it avoids having to simulate exact delays */
-static void kbd_update_irq(KBDState *s)
+static void kbd_update_irq_lines(KBDState *s)
 {
     int irq_kbd_level, irq_mouse_level;
 
     irq_kbd_level = 0;
     irq_mouse_level = 0;
+
+    if (s->status & KBD_STAT_OBF) {
+        if (s->status & KBD_STAT_MOUSE_OBF) {
+            if (s->mode & KBD_MODE_MOUSE_INT) {
+                irq_mouse_level = 1;
+            }
+        } else {
+            if ((s->mode & KBD_MODE_KBD_INT) &&
+                !(s->mode & KBD_MODE_DISABLE_KBD)) {
+                irq_kbd_level = 1;
+            }
+        }
+    }
+    qemu_set_irq(s->irq_kbd, irq_kbd_level);
+    qemu_set_irq(s->irq_mouse, irq_mouse_level);
+}
+
+/* update irq and KBD_STAT_[MOUSE_]OBF */
+static void kbd_update_irq(KBDState *s)
+{
     s->status &= ~(KBD_STAT_OBF | KBD_STAT_MOUSE_OBF);
     s->outport &= ~(KBD_OUT_OBF | KBD_OUT_MOUSE_OBF);
     if (s->pending) {
@@ -166,16 +185,9 @@ static void kbd_update_irq(KBDState *s)
         if (s->pending == KBD_PENDING_AUX) {
             s->status |= KBD_STAT_MOUSE_OBF;
             s->outport |= KBD_OUT_MOUSE_OBF;
-            if (s->mode & KBD_MODE_MOUSE_INT)
-                irq_mouse_level = 1;
-        } else {
-            if ((s->mode & KBD_MODE_KBD_INT) &&
-                !(s->mode & KBD_MODE_DISABLE_KBD))
-                irq_kbd_level = 1;
         }
     }
-    qemu_set_irq(s->irq_kbd, irq_kbd_level);
-    qemu_set_irq(s->irq_mouse, irq_mouse_level);
+    kbd_update_irq_lines(s);
 }
 
 static void kbd_update_kbd_irq(void *opaque, int level)
-- 
2.26.2

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

[Volker Rümelin]

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(-)

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()
     }
 };
-- 
2.26.2

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

[Gerd Hoffmann]

  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

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

[Volker Rümelin]

>    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
>

[PATCH v3 06/11] pckbd: PS/2 keyboard throttle

[Volker Rümelin]

Limit the keyboard data rate to the serial link speed. Some old
DOS software relies on being able to read an incoming scan-code
more than once. After reading keyboard data from the i8042
controller, the guest software has 1ms to read the same data
again.

Use -global i8042.kbd-throttle=on to enable this feature.

To see how this patch works, start a FreeDOS 1.2 guest with the
qemu option -global i8042.kbd-throttle=on and open a text file
with the vim 7.3 32 bit for DOS executable. Then use the cursor
keys (not the cursor keys on the numeric keypad) to move through
the text. Without the kbd-throttle option enabled each keystroke
will move the cursor two positions.

Buglink: https://bugs.launchpad.net/bugs/1895363
Buglink: https://bugs.launchpad.net/bugs/1897568
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/pckbd.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 8336f4e4b3..ebf08c7cb8 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -24,12 +24,14 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qemu/timer.h"
 #include "hw/isa/isa.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/input/ps2.h"
 #include "hw/irq.h"
 #include "hw/input/i8042.h"
+#include "hw/qdev-properties.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 
@@ -131,17 +133,21 @@
 #define KBD_PENDING_KBD         1
 #define KBD_PENDING_AUX         2
 
+#define KBD_MIGR_TIMER_PENDING  0x1
+
 typedef struct KBDState {
     uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
     uint8_t status;
     uint8_t mode;
     uint8_t outport;
+    uint32_t migration_flags;
     bool outport_present;
     /* Bitmask of devices with data available.  */
     uint8_t pending;
     uint8_t obdata;
     void *kbd;
     void *mouse;
+    QEMUTimer *throttle_timer;
 
     qemu_irq irq_kbd;
     qemu_irq irq_mouse;
@@ -206,6 +212,10 @@ static void kbd_safe_update_irq(KBDState *s)
     if (s->status & KBD_STAT_OBF) {
         return;
     }
+    /* the throttle timer is pending and will call kbd_update_irq() */
+    if (s->throttle_timer && timer_pending(s->throttle_timer)) {
+        return;
+    }
     if (s->pending) {
         kbd_update_irq(s);
     }
@@ -235,6 +245,15 @@ static void kbd_update_aux_irq(void *opaque, int level)
     kbd_safe_update_irq(s);
 }
 
+static void kbd_throttle_timeout(void *opaque)
+{
+    KBDState *s = opaque;
+
+    if (s->pending) {
+        kbd_update_irq(s);
+    }
+}
+
 static uint64_t kbd_read_status(void *opaque, hwaddr addr,
                                 unsigned size)
 {
@@ -356,6 +375,10 @@ static uint64_t kbd_read_data(void *opaque, hwaddr addr,
         if (status & KBD_STAT_MOUSE_OBF) {
             s->obdata = ps2_read_data(s->mouse);
         } else {
+            if (s->throttle_timer) {
+                timer_mod(s->throttle_timer,
+                          qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + 1000);
+            }
             s->obdata = ps2_read_data(s->kbd);
         }
     }
@@ -417,6 +440,9 @@ static void kbd_reset(void *opaque)
     s->outport_present = false;
     s->pending = 0;
     kbd_deassert_irq(s);
+    if (s->throttle_timer) {
+        timer_del(s->throttle_timer);
+    }
 }
 
 static uint8_t kbd_outport_default(KBDState *s)
@@ -451,6 +477,18 @@ static const VMStateDescription vmstate_kbd_outport = {
     }
 };
 
+static int kbd_pre_save(void *opaque)
+{
+    KBDState *s = opaque;
+
+    s->migration_flags = 0;
+    if (s->throttle_timer && timer_pending(s->throttle_timer)) {
+        s->migration_flags |= KBD_MIGR_TIMER_PENDING;
+    }
+
+    return 0;
+}
+
 static int kbd_post_load(void *opaque, int version_id)
 {
     KBDState *s = opaque;
@@ -458,6 +496,9 @@ static int kbd_post_load(void *opaque, int version_id)
         s->outport = kbd_outport_default(s);
     }
     s->outport_present = false;
+    if (s->migration_flags & KBD_MIGR_TIMER_PENDING) {
+        kbd_throttle_timeout(s);
+    }
     return 0;
 }
 
@@ -465,12 +506,14 @@ static const VMStateDescription vmstate_kbd = {
     .name = "pckbd",
     .version_id = 4,
     .minimum_version_id = 3,
+    .pre_save = kbd_pre_save,
     .post_load = kbd_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(write_cmd, KBDState),
         VMSTATE_UINT8(status, KBDState),
         VMSTATE_UINT8(mode, KBDState),
         VMSTATE_UINT8(pending, KBDState),
+        VMSTATE_UINT32_V(migration_flags, KBDState, 4),
         VMSTATE_UINT8_V(obdata, KBDState, 4),
         VMSTATE_END_OF_LIST()
     },
@@ -534,6 +577,7 @@ struct ISAKBDState {
     ISADevice parent_obj;
 
     KBDState kbd;
+    bool kbd_throttle;
     MemoryRegion io[2];
 };
 
@@ -614,6 +658,10 @@ static void i8042_realizefn(DeviceState *dev, Error **errp)
 
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
     s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
+    if (isa_s->kbd_throttle) {
+        s->throttle_timer = timer_new_us(QEMU_CLOCK_VIRTUAL,
+                                         kbd_throttle_timeout, s);
+    }
     qemu_register_reset(kbd_reset, s);
 }
 
@@ -645,11 +693,17 @@ static void i8042_build_aml(ISADevice *isadev, Aml *scope)
     aml_append(scope, mou);
 }
 
+static Property i8042_properties[] = {
+    DEFINE_PROP_BOOL("kbd-throttle", ISAKBDState, kbd_throttle, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void i8042_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
+    device_class_set_props(dc, i8042_properties);
     dc->realize = i8042_realizefn;
     dc->vmsd = &vmstate_kbd_isa;
     isa->build_aml = i8042_build_aml;
-- 
2.26.2

[PATCH v3 07/11] pckbd: add state variable for interrupt source

[Volker Rümelin]

Currently there is only one flag to distinguish between two
interrupt sources and there are no available flags for more
sources. Add an internal state variable to store the interrupt
source. The next patch will introduce an additional interrupt
source. There is no functional change.

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

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index ebf08c7cb8..adce525b88 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -135,12 +135,16 @@
 
 #define KBD_MIGR_TIMER_PENDING  0x1
 
+#define KBD_OBSRC_KBD           0x01
+#define KBD_OBSRC_MOUSE         0x02
+
 typedef struct KBDState {
     uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
     uint8_t status;
     uint8_t mode;
     uint8_t outport;
     uint32_t migration_flags;
+    uint32_t obsrc;
     bool outport_present;
     /* Bitmask of devices with data available.  */
     uint8_t pending;
@@ -198,6 +202,9 @@ static void kbd_update_irq(KBDState *s)
         if (s->pending == KBD_PENDING_AUX) {
             s->status |= KBD_STAT_MOUSE_OBF;
             s->outport |= KBD_OUT_MOUSE_OBF;
+            s->obsrc = KBD_OBSRC_MOUSE;
+        } else {
+            s->obsrc = KBD_OBSRC_KBD;
         }
     }
     kbd_update_irq_lines(s);
@@ -368,18 +375,17 @@ static uint64_t kbd_read_data(void *opaque, hwaddr addr,
                               unsigned size)
 {
     KBDState *s = opaque;
-    uint8_t status = s->status;
 
-    if (status & KBD_STAT_OBF) {
+    if (s->status & KBD_STAT_OBF) {
         kbd_deassert_irq(s);
-        if (status & KBD_STAT_MOUSE_OBF) {
-            s->obdata = ps2_read_data(s->mouse);
-        } else {
+        if (s->obsrc & KBD_OBSRC_KBD) {
             if (s->throttle_timer) {
                 timer_mod(s->throttle_timer,
                           qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + 1000);
             }
             s->obdata = ps2_read_data(s->kbd);
+        } else if (s->obsrc & KBD_OBSRC_MOUSE) {
+            s->obdata = ps2_read_data(s->mouse);
         }
     }
 
@@ -496,6 +502,11 @@ static int kbd_post_load(void *opaque, int version_id)
         s->outport = kbd_outport_default(s);
     }
     s->outport_present = false;
+    if (version_id < 4) {
+        s->obsrc = s->status & KBD_STAT_OBF ?
+            (s->status & KBD_STAT_MOUSE_OBF ? KBD_OBSRC_MOUSE : KBD_OBSRC_KBD) :
+            0;
+    }
     if (s->migration_flags & KBD_MIGR_TIMER_PENDING) {
         kbd_throttle_timeout(s);
     }
@@ -514,6 +525,7 @@ static const VMStateDescription vmstate_kbd = {
         VMSTATE_UINT8(mode, KBDState),
         VMSTATE_UINT8(pending, KBDState),
         VMSTATE_UINT32_V(migration_flags, KBDState, 4),
+        VMSTATE_UINT32_V(obsrc, KBDState, 4),
         VMSTATE_UINT8_V(obdata, KBDState, 4),
         VMSTATE_END_OF_LIST()
     },
-- 
2.26.2

[PATCH v3 08/11] pckbd: add controller response queue

[Volker Rümelin]

Add a separate queue for PS/2 controller responses. The
responses no longer get queued in the keyboard or mouse queues.
The advantage of this can be seen after the next two patches,
where the guest can disable the PS/2 communication with keyboard
and mouse and still talk to the PS/2 controller.

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

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index adce525b88..e9523e164f 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -132,11 +132,14 @@
 
 #define KBD_PENDING_KBD         1
 #define KBD_PENDING_AUX         2
+#define KBD_PENDING_CTRL_KBD    0x04
+#define KBD_PENDING_CTRL_AUX    0x08
 
 #define KBD_MIGR_TIMER_PENDING  0x1
 
 #define KBD_OBSRC_KBD           0x01
 #define KBD_OBSRC_MOUSE         0x02
+#define KBD_OBSRC_CTRL          0x04
 
 typedef struct KBDState {
     uint8_t write_cmd; /* if non zero, write data to port 60 is expected */
@@ -149,6 +152,7 @@ typedef struct KBDState {
     /* Bitmask of devices with data available.  */
     uint8_t pending;
     uint8_t obdata;
+    uint8_t cbdata;
     void *kbd;
     void *mouse;
     QEMUTimer *throttle_timer;
@@ -199,12 +203,18 @@ static void kbd_update_irq(KBDState *s)
     if (s->pending) {
         s->status |= KBD_STAT_OBF;
         s->outport |= KBD_OUT_OBF;
-        if (s->pending == KBD_PENDING_AUX) {
+        if (s->pending & KBD_PENDING_CTRL_KBD) {
+            s->obsrc = KBD_OBSRC_CTRL;
+        } else if (s->pending & KBD_PENDING_CTRL_AUX) {
             s->status |= KBD_STAT_MOUSE_OBF;
             s->outport |= KBD_OUT_MOUSE_OBF;
-            s->obsrc = KBD_OBSRC_MOUSE;
-        } else {
+            s->obsrc = KBD_OBSRC_CTRL;
+        } else if (s->pending & KBD_PENDING_KBD) {
             s->obsrc = KBD_OBSRC_KBD;
+        } else {
+            s->status |= KBD_STAT_MOUSE_OBF;
+            s->outport |= KBD_OUT_MOUSE_OBF;
+            s->obsrc = KBD_OBSRC_MOUSE;
         }
     }
     kbd_update_irq_lines(s);
@@ -273,10 +283,21 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr,
 
 static void kbd_queue(KBDState *s, int b, int aux)
 {
-    if (aux)
-        ps2_queue(s->mouse, b);
-    else
-        ps2_queue(s->kbd, b);
+    s->cbdata = b;
+    s->pending &= ~KBD_PENDING_CTRL_KBD & ~KBD_PENDING_CTRL_AUX;
+    s->pending |= aux ? KBD_PENDING_CTRL_AUX : KBD_PENDING_CTRL_KBD;
+    kbd_safe_update_irq(s);
+}
+
+static uint8_t kbd_dequeue(KBDState *s)
+{
+    uint8_t b = s->cbdata;
+
+    s->pending &= ~KBD_PENDING_CTRL_KBD & ~KBD_PENDING_CTRL_AUX;
+    if (s->pending) {
+        kbd_update_irq(s);
+    }
+    return b;
 }
 
 static void outport_write(KBDState *s, uint32_t val)
@@ -386,6 +407,8 @@ static uint64_t kbd_read_data(void *opaque, hwaddr addr,
             s->obdata = ps2_read_data(s->kbd);
         } else if (s->obsrc & KBD_OBSRC_MOUSE) {
             s->obdata = ps2_read_data(s->mouse);
+        } else if (s->obsrc & KBD_OBSRC_CTRL) {
+            s->obdata = kbd_dequeue(s);
         }
     }
 
@@ -527,6 +550,7 @@ static const VMStateDescription vmstate_kbd = {
         VMSTATE_UINT32_V(migration_flags, KBDState, 4),
         VMSTATE_UINT32_V(obsrc, KBDState, 4),
         VMSTATE_UINT8_V(obdata, KBDState, 4),
+        VMSTATE_UINT8_V(cbdata, KBDState, 4),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription*[]) {
-- 
2.26.2

[PATCH v3 09/11] pckbd: add function kbd_pending()

[Volker Rümelin]

Replace reads of the variable s->pending with a call to a new
function kbd_pending() to ease the review of the next patch.
There is no functional change.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/pckbd.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index e9523e164f..199367dcab 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -195,21 +195,28 @@ static void kbd_deassert_irq(KBDState *s)
     kbd_update_irq_lines(s);
 }
 
+static uint8_t kbd_pending(KBDState *s)
+{
+    return s->pending;
+}
+
 /* update irq and KBD_STAT_[MOUSE_]OBF */
 static void kbd_update_irq(KBDState *s)
 {
+    uint8_t pending = kbd_pending(s);
+
     s->status &= ~(KBD_STAT_OBF | KBD_STAT_MOUSE_OBF);
     s->outport &= ~(KBD_OUT_OBF | KBD_OUT_MOUSE_OBF);
-    if (s->pending) {
+    if (pending) {
         s->status |= KBD_STAT_OBF;
         s->outport |= KBD_OUT_OBF;
-        if (s->pending & KBD_PENDING_CTRL_KBD) {
+        if (pending & KBD_PENDING_CTRL_KBD) {
             s->obsrc = KBD_OBSRC_CTRL;
-        } else if (s->pending & KBD_PENDING_CTRL_AUX) {
+        } else if (pending & KBD_PENDING_CTRL_AUX) {
             s->status |= KBD_STAT_MOUSE_OBF;
             s->outport |= KBD_OUT_MOUSE_OBF;
             s->obsrc = KBD_OBSRC_CTRL;
-        } else if (s->pending & KBD_PENDING_KBD) {
+        } else if (pending & KBD_PENDING_KBD) {
             s->obsrc = KBD_OBSRC_KBD;
         } else {
             s->status |= KBD_STAT_MOUSE_OBF;
@@ -233,7 +240,7 @@ static void kbd_safe_update_irq(KBDState *s)
     if (s->throttle_timer && timer_pending(s->throttle_timer)) {
         return;
     }
-    if (s->pending) {
+    if (kbd_pending(s)) {
         kbd_update_irq(s);
     }
 }
@@ -266,7 +273,7 @@ static void kbd_throttle_timeout(void *opaque)
 {
     KBDState *s = opaque;
 
-    if (s->pending) {
+    if (kbd_pending(s)) {
         kbd_update_irq(s);
     }
 }
@@ -294,7 +301,7 @@ static uint8_t kbd_dequeue(KBDState *s)
     uint8_t b = s->cbdata;
 
     s->pending &= ~KBD_PENDING_CTRL_KBD & ~KBD_PENDING_CTRL_AUX;
-    if (s->pending) {
+    if (kbd_pending(s)) {
         kbd_update_irq(s);
     }
     return b;
-- 
2.26.2

[PATCH v3 10/11] pckbd: correctly disable PS/2 communication

[Volker Rümelin]

Currently the PS/2 controller command KBD_CCMD_MOUSE_DISABLE
doesn't disable the PS/2 mouse communication at all, and the
PS/2 controller commands KBD_CCMD_KBD_DISABLE and
KBD_CCMD_KBD_ENABLE only disable and enable the keyboard
interrupt, which is very different from what a real PS/2
controller does. A guest may notice the difference.

Mask out pending data on disabled queues to correctly disable
the PS/2 controller communication.

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

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 199367dcab..cf2f4ee27a 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -130,10 +130,12 @@
 #define MOUSE_STATUS_ENABLED    0x20
 #define MOUSE_STATUS_SCALE21    0x10
 
-#define KBD_PENDING_KBD         1
-#define KBD_PENDING_AUX         2
+#define KBD_PENDING_KBD_V3      0x01
+#define KBD_PENDING_AUX_V3      0x02
 #define KBD_PENDING_CTRL_KBD    0x04
 #define KBD_PENDING_CTRL_AUX    0x08
+#define KBD_PENDING_KBD         KBD_MODE_DISABLE_KBD    /* 0x10 */
+#define KBD_PENDING_AUX         KBD_MODE_DISABLE_MOUSE  /* 0x20 */
 
 #define KBD_MIGR_TIMER_PENDING  0x1
 
@@ -197,7 +199,7 @@ static void kbd_deassert_irq(KBDState *s)
 
 static uint8_t kbd_pending(KBDState *s)
 {
-    return s->pending;
+    return s->pending & (~s->mode | ~(KBD_PENDING_KBD | KBD_PENDING_AUX));
 }
 
 /* update irq and KBD_STAT_[MOUSE_]OBF */
@@ -354,6 +356,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
         break;
     case KBD_CCMD_MOUSE_ENABLE:
         s->mode &= ~KBD_MODE_DISABLE_MOUSE;
+        kbd_safe_update_irq(s);
         break;
     case KBD_CCMD_TEST_MOUSE:
         kbd_queue(s, 0x00, 0);
@@ -433,6 +436,9 @@ static void kbd_write_data(void *opaque, hwaddr addr,
     switch(s->write_cmd) {
     case 0:
         ps2_write_keyboard(s->kbd, val);
+        /* sending data to the keyboard reenables PS/2 communication */
+        s->mode &= ~KBD_MODE_DISABLE_KBD;
+        kbd_safe_update_irq(s);
         break;
     case KBD_CCMD_WRITE_MODE:
         s->mode = val;
@@ -459,6 +465,9 @@ static void kbd_write_data(void *opaque, hwaddr addr,
         break;
     case KBD_CCMD_WRITE_MOUSE:
         ps2_write_mouse(s->mouse, val);
+        /* sending data to the mouse reenables PS/2 communication */
+        s->mode &= ~KBD_MODE_DISABLE_MOUSE;
+        kbd_safe_update_irq(s);
         break;
     default:
         break;
@@ -536,7 +545,16 @@ static int kbd_post_load(void *opaque, int version_id)
         s->obsrc = s->status & KBD_STAT_OBF ?
             (s->status & KBD_STAT_MOUSE_OBF ? KBD_OBSRC_MOUSE : KBD_OBSRC_KBD) :
             0;
+        if (s->pending & KBD_PENDING_KBD_V3) {
+            s->pending |= KBD_PENDING_KBD;
+        }
+        if (s->pending & KBD_PENDING_AUX_V3) {
+            s->pending |= KBD_PENDING_AUX;
+        }
     }
+    /* clear all unused flags */
+    s->pending &= KBD_PENDING_CTRL_KBD | KBD_PENDING_CTRL_AUX |
+                  KBD_PENDING_KBD | KBD_PENDING_AUX;
     if (s->migration_flags & KBD_MIGR_TIMER_PENDING) {
         kbd_throttle_timeout(s);
     }
-- 
2.26.2

[PATCH v3 11/11] pckbd: remove duplicated keyboard and mouse defines

[Volker Rümelin]

In 2005 the author of commit daa579632d "PS2 mouse and keyboard
separation (Paul Brook)" and 0e43e99c04 "PS2 mouse and keyboard
separation (Paul Brook)" separated the PS/2 controller code and
the PS/2 keyboard and mouse code. It seems he forgot to remove
a few defines. Remove them now.

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

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index cf2f4ee27a..9675849cc1 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -61,21 +61,6 @@
 #define KBD_CCMD_RESET          0xFE    /* Pulse bit 0 of the output port P2 = CPU reset. */
 #define KBD_CCMD_NO_OP          0xFF    /* Pulse no bits of the output port P2. */
 
-/* Keyboard Commands */
-#define KBD_CMD_SET_LEDS	0xED	/* Set keyboard leds */
-#define KBD_CMD_ECHO     	0xEE
-#define KBD_CMD_GET_ID 	        0xF2	/* get keyboard ID */
-#define KBD_CMD_SET_RATE	0xF3	/* Set typematic rate */
-#define KBD_CMD_ENABLE		0xF4	/* Enable scanning */
-#define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
-#define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
-#define KBD_CMD_RESET		0xFF	/* Reset */
-
-/* Keyboard Replies */
-#define KBD_REPLY_POR		0xAA	/* Power on reset */
-#define KBD_REPLY_ACK		0xFA	/* Command ACK */
-#define KBD_REPLY_RESEND	0xFE	/* Command NACK, send the cmd again */
-
 /* Status Register Bits */
 #define KBD_STAT_OBF 		0x01	/* Keyboard output buffer full */
 #define KBD_STAT_IBF 		0x02	/* Keyboard input buffer full */
@@ -108,28 +93,6 @@
  */
 #define KBD_OUT_ONES            0xcc
 
-/* Mouse Commands */
-#define AUX_SET_SCALE11		0xE6	/* Set 1:1 scaling */
-#define AUX_SET_SCALE21		0xE7	/* Set 2:1 scaling */
-#define AUX_SET_RES		0xE8	/* Set resolution */
-#define AUX_GET_SCALE		0xE9	/* Get scaling factor */
-#define AUX_SET_STREAM		0xEA	/* Set stream mode */
-#define AUX_POLL		0xEB	/* Poll */
-#define AUX_RESET_WRAP		0xEC	/* Reset wrap mode */
-#define AUX_SET_WRAP		0xEE	/* Set wrap mode */
-#define AUX_SET_REMOTE		0xF0	/* Set remote mode */
-#define AUX_GET_TYPE		0xF2	/* Get type */
-#define AUX_SET_SAMPLE		0xF3	/* Set sample rate */
-#define AUX_ENABLE_DEV		0xF4	/* Enable aux device */
-#define AUX_DISABLE_DEV		0xF5	/* Disable aux device */
-#define AUX_SET_DEFAULT		0xF6
-#define AUX_RESET		0xFF	/* Reset aux device */
-#define AUX_ACK			0xFA	/* Command byte ACK. */
-
-#define MOUSE_STATUS_REMOTE     0x40
-#define MOUSE_STATUS_ENABLED    0x20
-#define MOUSE_STATUS_SCALE21    0x10
-
 #define KBD_PENDING_KBD_V3      0x01
 #define KBD_PENDING_AUX_V3      0x02
 #define KBD_PENDING_CTRL_KBD    0x04
-- 
2.26.2

Re: [PATCH v3 00/11] PS/2 controller related fixes

[Philippe Mathieu-Daudé]

On 5/15/21 1:31 PM, Volker Rümelin wrote:
> This patch series fixes two different PS/2 mouse stream corruptions
> and adds a feature that allows some old misbehaving DOS programs to
> have a working keyboard. With the last few patches, the PS/2 con-
> troller behaves more like a real controller.
> 
> v2:
> Introduce the function kbd_pending() in a preliminary patch to ease
> the review of patch "pckbd: correctly disable PS/2 communication",
> as Phillipe suggested.
> 
> v3:
> Patch "pckbd: correctly disable PS/2 communication" exposed a bug
> in SeaBIOS. The updated patch keeps the relevant code. Until
> SeaBIOS is fixed, the PS/2 controller command KBD_CCMD_KBD_DISABLE
> must disable the keyboard interrupt.
> 
> In patch "pckbd: PS/2 keyboard throttle" in function
> kbd_throttle_timeout() an unnecessary if statement was removed.
> The KBD_STAT_OBF flag is never set when kbd_throttle_timeout()
> gets called.
> 
> Volker Rümelin (11):
>   ps2: fix mouse stream corruption
>   ps2: don't raise an interrupt if queue is full
>   ps2: don't deassert irq twice if queue is empty
>   pckbd: split out interrupt line changing code
>   pckbd: don't update OBF flags if KBD_STAT_OBF is set
>   pckbd: PS/2 keyboard throttle
>   pckbd: add state variable for interrupt source
>   pckbd: add controller response queue
>   pckbd: add function kbd_pending()
>   pckbd: correctly disable PS/2 communication
>   pckbd: remove duplicated keyboard and mouse defines

Zoltan, you might want to test this series with your Pegasos2
machine. It makes the keyboard detected correctly.

There is still a problem with the mouse interaction with the
host. Pressing Ctrl+Alt+G to ungrab the mouse, my host mouse
is still responding to guest events... (unrelated to this series).

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
(PPC Pegasos2 so far)

Re: [PATCH v3 00/11] PS/2 controller related fixes

[BALATON Zoltan]

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

On Sat, 15 May 2021, Philippe Mathieu-Daudé wrote:
> On 5/15/21 1:31 PM, Volker Rümelin wrote:
>> This patch series fixes two different PS/2 mouse stream corruptions
>> and adds a feature that allows some old misbehaving DOS programs to
>> have a working keyboard. With the last few patches, the PS/2 con-
>> troller behaves more like a real controller.
>>
>> v2:
>> Introduce the function kbd_pending() in a preliminary patch to ease
>> the review of patch "pckbd: correctly disable PS/2 communication",
>> as Phillipe suggested.
>>
>> v3:
>> Patch "pckbd: correctly disable PS/2 communication" exposed a bug
>> in SeaBIOS. The updated patch keeps the relevant code. Until
>> SeaBIOS is fixed, the PS/2 controller command KBD_CCMD_KBD_DISABLE
>> must disable the keyboard interrupt.
>>
>> In patch "pckbd: PS/2 keyboard throttle" in function
>> kbd_throttle_timeout() an unnecessary if statement was removed.
>> The KBD_STAT_OBF flag is never set when kbd_throttle_timeout()
>> gets called.
>>
>> Volker Rümelin (11):
>>   ps2: fix mouse stream corruption
>>   ps2: don't raise an interrupt if queue is full
>>   ps2: don't deassert irq twice if queue is empty
>>   pckbd: split out interrupt line changing code
>>   pckbd: don't update OBF flags if KBD_STAT_OBF is set
>>   pckbd: PS/2 keyboard throttle
>>   pckbd: add state variable for interrupt source
>>   pckbd: add controller response queue
>>   pckbd: add function kbd_pending()
>>   pckbd: correctly disable PS/2 communication
>>   pckbd: remove duplicated keyboard and mouse defines
>
> Zoltan, you might want to test this series with your Pegasos2
> machine. It makes the keyboard detected correctly.
>
> There is still a problem with the mouse interaction with the
> host. Pressing Ctrl+Alt+G to ungrab the mouse, my host mouse
> is still responding to guest events... (unrelated to this series).
>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> (PPC Pegasos2 so far)

I don't get the errors you reported. Keyboard and mouse seems to work OK 
for me with SDL on Linux and never got errors detecting it so far. What 
config are you using?

Regards,
BALATON Zoltan

Re: [PATCH v3 00/11] PS/2 controller related fixes

[Philippe Mathieu-Daudé]

On 5/15/21 4:35 PM, BALATON Zoltan wrote:
> On Sat, 15 May 2021, Philippe Mathieu-Daudé wrote:
>> On 5/15/21 1:31 PM, Volker Rümelin wrote:
>>> This patch series fixes two different PS/2 mouse stream corruptions
>>> and adds a feature that allows some old misbehaving DOS programs to
>>> have a working keyboard. With the last few patches, the PS/2 con-
>>> troller behaves more like a real controller.
>>>
>>> v2:
>>> Introduce the function kbd_pending() in a preliminary patch to ease
>>> the review of patch "pckbd: correctly disable PS/2 communication",
>>> as Phillipe suggested.
>>>
>>> v3:
>>> Patch "pckbd: correctly disable PS/2 communication" exposed a bug
>>> in SeaBIOS. The updated patch keeps the relevant code. Until
>>> SeaBIOS is fixed, the PS/2 controller command KBD_CCMD_KBD_DISABLE
>>> must disable the keyboard interrupt.
>>>
>>> In patch "pckbd: PS/2 keyboard throttle" in function
>>> kbd_throttle_timeout() an unnecessary if statement was removed.
>>> The KBD_STAT_OBF flag is never set when kbd_throttle_timeout()
>>> gets called.
>>>
>>> Volker Rümelin (11):
>>>   ps2: fix mouse stream corruption
>>>   ps2: don't raise an interrupt if queue is full
>>>   ps2: don't deassert irq twice if queue is empty
>>>   pckbd: split out interrupt line changing code
>>>   pckbd: don't update OBF flags if KBD_STAT_OBF is set
>>>   pckbd: PS/2 keyboard throttle
>>>   pckbd: add state variable for interrupt source
>>>   pckbd: add controller response queue
>>>   pckbd: add function kbd_pending()
>>>   pckbd: correctly disable PS/2 communication
>>>   pckbd: remove duplicated keyboard and mouse defines
>>
>> Zoltan, you might want to test this series with your Pegasos2
>> machine. It makes the keyboard detected correctly.
>>
>> There is still a problem with the mouse interaction with the
>> host. Pressing Ctrl+Alt+G to ungrab the mouse, my host mouse
>> is still responding to guest events... (unrelated to this series).
>>
>> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> (PPC Pegasos2 so far)
> 
> I don't get the errors you reported. Keyboard and mouse seems to work OK
> for me with SDL on Linux and never got errors detecting it so far. What
> config are you using?

OK, apparently a timing problem related to slow emulation.
I can reproduce simply using "--enable-debug --extra-cflags=-ggdb",
not without it.

Using Volker's series it is harder to reproduce.