ps2: Fix issue #501 and #502

ps2: Fix issue #501 and #502

[Volker Rümelin]

Since commit ff6e1624b3 (pckbd: don't update OBF flags if
KBD_STAT_OBF is set) the OSes Minoca OS and Visopsys no longer
have a working PS/2 keyboard and mouse. This is caused by a
PS/2 queue stall due to a lost interrupt in the guest OS. This
already happened before commit ff6e1624b3, but no one noticed
because up to that point QEMU sent gratuitous keyboard and mouse
interrupts and the queue restarted with keyboard input or mouse
movement.

The lost interrupt is a guest bug. The fact that it's always
lost is due to an inexact PS/2 keyboard emulation. The way in
which the two operating systems e.g. set the keyboard LEDs,
leaves a keyboard ACK reply in the keyboard queue that is
unexpected for the guests.

This patch series improves the PS/2 keyboard emulation.

There's a workaround for issue #501 and #502 so I don't think
this is rc3 material. But that decision is up to the maintainers.

To verify patch 2/3 I plugged in an additional PS/2 keyboard
into the host and started Linux with the command line option
initcall_blacklist=i8042_init. Here is an example of the sequence
to set the keyboard LEDs.

# #regular sequence to set the keyboard LEDs
# inb --hex 0x64
1c
# #PS/2 queue is empty
# outb 0x60 0xed
# inb --hex 0x64
15
# inb --hex 0x60
fa
# inb --hex 0x64
14
# outb 0x60 0x01
# inb --hex 0x64
15
# inb --hex 0x60
fa
# inb --hex 0x64
14

# #alternative sequence to set the keyboard LEDs
# inb --hex 0x64
14
# outb 0x60 0xed
# outb 0x60 0x01
# inb --hex 0x64
15
# inb --hex 0x60
fa
# inb --hex 0x64
14

Volker Rümelin (3):
   ps2: use the whole ps2 buffer but keep queue size
   ps2: use a separate keyboard command reply queue
   ps2: migration support for command reply queue

  hw/input/ps2.c | 214 ++++++++++++++++++++++++++++++-------------------
  1 file changed, 133 insertions(+), 81 deletions(-)

-- 
2.26.2

[PATCH 1/3] ps2: use the whole ps2 buffer but keep queue size

[Volker Rümelin]

Extend the used ps2 buffer size to the available buffer size but
keep the maximum ps2 queue size.

The next patch needs a few bytes of the larger buffer size.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 69 +++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 49 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 8dd482c1f6..23e7befee5 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -74,7 +74,12 @@
 #define MOUSE_STATUS_ENABLED    0x20
 #define MOUSE_STATUS_SCALE21    0x10
 
-#define PS2_QUEUE_SIZE 16  /* Buffer size required by PS/2 protocol */
+/*
+ * PS/2 buffer size. Keep 256 bytes for compatibility with
+ * older QEMU versions.
+ */
+#define PS2_BUFFER_SIZE     256
+#define PS2_QUEUE_SIZE      16  /* Queue size required by PS/2 protocol */
 
 /* Bits for 'modifiers' field in PS2KbdState */
 #define MOD_CTRL_L  (1 << 0)
@@ -85,9 +90,7 @@
 #define MOD_ALT_R   (1 << 5)
 
 typedef struct {
-    /* Keep the data array 256 bytes long, which compatibility
-     with older qemu versions. */
-    uint8_t data[256];
+    uint8_t data[PS2_BUFFER_SIZE];
     int rptr, wptr, count;
 } PS2Queue;
 
@@ -200,8 +203,9 @@ void ps2_queue_noirq(PS2State *s, int b)
     }
 
     q->data[q->wptr] = b;
-    if (++q->wptr == PS2_QUEUE_SIZE)
+    if (++q->wptr == PS2_BUFFER_SIZE) {
         q->wptr = 0;
+    }
     q->count++;
 }
 
@@ -509,13 +513,15 @@ uint32_t ps2_read_data(PS2State *s)
            (needed for EMM386) */
         /* XXX: need a timer to do things correctly */
         index = q->rptr - 1;
-        if (index < 0)
-            index = PS2_QUEUE_SIZE - 1;
+        if (index < 0) {
+            index = PS2_BUFFER_SIZE - 1;
+        }
         val = q->data[index];
     } else {
         val = q->data[q->rptr];
-        if (++q->rptr == PS2_QUEUE_SIZE)
+        if (++q->rptr == PS2_BUFFER_SIZE) {
             q->rptr = 0;
+        }
         q->count--;
         /* reading deasserts IRQ */
         s->update_irq(s->update_arg, 0);
@@ -926,30 +932,17 @@ static void ps2_common_reset(PS2State *s)
 static void ps2_common_post_load(PS2State *s)
 {
     PS2Queue *q = &s->queue;
-    uint8_t i, size;
-    uint8_t tmp_data[PS2_QUEUE_SIZE];
 
-    /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
-    size = q->count;
+    /* set the useful data buffer queue size <= PS2_QUEUE_SIZE */
     if (q->count < 0) {
-        size = 0;
+        q->count = 0;
     } else if (q->count > PS2_QUEUE_SIZE) {
-        size = PS2_QUEUE_SIZE;
-    }
-
-    /* move the queue elements to the start of data array */
-    for (i = 0; i < size; i++) {
-        if (q->rptr < 0 || q->rptr >= sizeof(q->data)) {
-            q->rptr = 0;
-        }
-        tmp_data[i] = q->data[q->rptr++];
+        q->count = PS2_QUEUE_SIZE;
     }
-    memcpy(q->data, tmp_data, size);
 
-    /* reset rptr/wptr/count */
-    q->rptr = 0;
-    q->wptr = (size == PS2_QUEUE_SIZE) ? 0 : size;
-    q->count = size;
+    /* sanitize rptr and recalculate wptr */
+    q->rptr = q->rptr & (PS2_BUFFER_SIZE - 1);
+    q->wptr = (q->rptr + q->count) & (PS2_BUFFER_SIZE - 1);
 }
 
 static void ps2_kbd_reset(void *opaque)
@@ -1053,22 +1046,11 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
     return 0;
 }
 
-static int ps2_kbd_pre_save(void *opaque)
-{
-    PS2KbdState *s = (PS2KbdState *)opaque;
-    PS2State *ps2 = &s->common;
-
-    ps2_common_post_load(ps2);
-
-    return 0;
-}
-
 static const VMStateDescription vmstate_ps2_keyboard = {
     .name = "ps2kbd",
     .version_id = 3,
     .minimum_version_id = 2,
     .post_load = ps2_kbd_post_load,
-    .pre_save = ps2_kbd_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(common, PS2KbdState, 0, vmstate_ps2_common, PS2State),
         VMSTATE_INT32(scan_enabled, PS2KbdState),
@@ -1093,22 +1075,11 @@ static int ps2_mouse_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static int ps2_mouse_pre_save(void *opaque)
-{
-    PS2MouseState *s = (PS2MouseState *)opaque;
-    PS2State *ps2 = &s->common;
-
-    ps2_common_post_load(ps2);
-
-    return 0;
-}
-
 static const VMStateDescription vmstate_ps2_mouse = {
     .name = "ps2mouse",
     .version_id = 2,
     .minimum_version_id = 2,
     .post_load = ps2_mouse_post_load,
-    .pre_save = ps2_mouse_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(common, PS2MouseState, 0, vmstate_ps2_common, PS2State),
         VMSTATE_UINT8(mouse_status, PS2MouseState),
-- 
2.26.2

[PATCH 2/3] ps2: use a separate keyboard command reply queue

[Volker Rümelin]

A PS/2 keyboard has a separate command reply queue that is
independant of the key queue. This prevents that command replies
and keyboard input mix. Keyboard command replies take precedence
over queued keystrokes. A new keyboard command removes any
remaining command replies from the command reply queue.

Implement a separate keyboard command reply queue and clear the
command reply queue before command execution. This brings the
PS/2 keyboard emulation much closer to a real PS/2 keyboard.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/501
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/502
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 115 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 84 insertions(+), 31 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 23e7befee5..8c06fd7fb4 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -91,7 +91,7 @@
 
 typedef struct {
     uint8_t data[PS2_BUFFER_SIZE];
-    int rptr, wptr, count;
+    int rptr, wptr, cwptr, count;
 } PS2Queue;
 
 struct PS2State {
@@ -186,6 +186,7 @@ static void ps2_reset_queue(PS2State *s)
 
     q->rptr = 0;
     q->wptr = 0;
+    q->cwptr = -1;
     q->count = 0;
 }
 
@@ -198,7 +199,7 @@ void ps2_queue_noirq(PS2State *s, int b)
 {
     PS2Queue *q = &s->queue;
 
-    if (q->count == PS2_QUEUE_SIZE) {
+    if (q->count >= PS2_QUEUE_SIZE) {
         return;
     }
 
@@ -260,6 +261,63 @@ void ps2_queue_4(PS2State *s, int b1, int b2, int b3, int b4)
     ps2_raise_irq(s);
 }
 
+static void ps2_cqueue_data(PS2Queue *q, int b)
+{
+    q->data[q->cwptr] = b;
+    if (++q->cwptr >= PS2_BUFFER_SIZE) {
+        q->cwptr = 0;
+    }
+    q->count++;
+}
+
+static void ps2_cqueue_1(PS2State *s, int b1)
+{
+    PS2Queue *q = &s->queue;
+
+    q->rptr = (q->rptr - 1) & (PS2_BUFFER_SIZE - 1);
+    q->cwptr = q->rptr;
+    ps2_cqueue_data(q, b1);
+    ps2_raise_irq(s);
+}
+
+static void ps2_cqueue_2(PS2State *s, int b1, int b2)
+{
+    PS2Queue *q = &s->queue;
+
+    q->rptr = (q->rptr - 2) & (PS2_BUFFER_SIZE - 1);
+    q->cwptr = q->rptr;
+    ps2_cqueue_data(q, b1);
+    ps2_cqueue_data(q, b2);
+    ps2_raise_irq(s);
+}
+
+static void ps2_cqueue_3(PS2State *s, int b1, int b2, int b3)
+{
+    PS2Queue *q = &s->queue;
+
+    q->rptr = (q->rptr - 3) & (PS2_BUFFER_SIZE - 1);
+    q->cwptr = q->rptr;
+    ps2_cqueue_data(q, b1);
+    ps2_cqueue_data(q, b2);
+    ps2_cqueue_data(q, b3);
+    ps2_raise_irq(s);
+}
+
+static void ps2_cqueue_reset(PS2State *s)
+{
+    PS2Queue *q = &s->queue;
+    int ccount;
+
+    if (q->cwptr == -1) {
+        return;
+    }
+
+    ccount = (q->cwptr - q->rptr) & (PS2_BUFFER_SIZE - 1);
+    q->count -= ccount;
+    q->rptr = q->cwptr;
+    q->cwptr = -1;
+}
+
 /* keycode is the untranslated scancode in the current scancode set. */
 static void ps2_put_keycode(void *opaque, int keycode)
 {
@@ -523,6 +581,10 @@ uint32_t ps2_read_data(PS2State *s)
             q->rptr = 0;
         }
         q->count--;
+        if (q->rptr == q->cwptr) {
+            /* command reply queue is empty */
+            q->cwptr = -1;
+        }
         /* reading deasserts IRQ */
         s->update_irq(s->update_arg, 0);
         /* reassert IRQs if data left */
@@ -554,92 +616,83 @@ void ps2_write_keyboard(void *opaque, int val)
     PS2KbdState *s = (PS2KbdState *)opaque;
 
     trace_ps2_write_keyboard(opaque, val);
+    ps2_cqueue_reset(&s->common);
     switch(s->common.write_cmd) {
     default:
     case -1:
         switch(val) {
         case 0x00:
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         case 0x05:
-            ps2_queue(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
             break;
         case KBD_CMD_GET_ID:
             /* We emulate a MF2 AT keyboard here */
-            if (s->translate)
-                ps2_queue_3(&s->common,
-                    KBD_REPLY_ACK,
-                    KBD_REPLY_ID,
-                    0x41);
-            else
-                ps2_queue_3(&s->common,
-                    KBD_REPLY_ACK,
-                    KBD_REPLY_ID,
-                    0x83);
+            ps2_cqueue_3(&s->common, KBD_REPLY_ACK, KBD_REPLY_ID,
+                s->translate ? 0x41 : 0x83);
             break;
         case KBD_CMD_ECHO:
-            ps2_queue(&s->common, KBD_CMD_ECHO);
+            ps2_cqueue_1(&s->common, KBD_CMD_ECHO);
             break;
         case KBD_CMD_ENABLE:
             s->scan_enabled = 1;
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         case KBD_CMD_SCANCODE:
         case KBD_CMD_SET_LEDS:
         case KBD_CMD_SET_RATE:
         case KBD_CMD_SET_MAKE_BREAK:
             s->common.write_cmd = val;
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET_DISABLE:
             ps2_reset_keyboard(s);
             s->scan_enabled = 0;
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET_ENABLE:
             ps2_reset_keyboard(s);
             s->scan_enabled = 1;
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         case KBD_CMD_RESET:
             ps2_reset_keyboard(s);
-            ps2_queue_2(&s->common,
+            ps2_cqueue_2(&s->common,
                 KBD_REPLY_ACK,
                 KBD_REPLY_POR);
             break;
         case KBD_CMD_SET_TYPEMATIC:
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
             break;
         default:
-            ps2_queue(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
             break;
         }
         break;
     case KBD_CMD_SET_MAKE_BREAK:
-        ps2_queue(&s->common, KBD_REPLY_ACK);
+        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SCANCODE:
         if (val == 0) {
-            if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) {
-                ps2_queue(&s->common, KBD_REPLY_ACK);
-                ps2_put_keycode(s, s->scancode_set);
-            }
+            ps2_cqueue_2(&s->common, KBD_REPLY_ACK, s->translate ?
+                translate_table[s->scancode_set] : s->scancode_set);
         } else if (val >= 1 && val <= 3) {
             s->scancode_set = val;
-            ps2_queue(&s->common, KBD_REPLY_ACK);
+            ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
         } else {
-            ps2_queue(&s->common, KBD_REPLY_RESEND);
+            ps2_cqueue_1(&s->common, KBD_REPLY_RESEND);
         }
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_LEDS:
         ps2_set_ledstate(s, val);
-        ps2_queue(&s->common, KBD_REPLY_ACK);
+        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_RATE:
-        ps2_queue(&s->common, KBD_REPLY_ACK);
+        ps2_cqueue_1(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
     }
-- 
2.26.2

[PATCH 3/3] ps2: migration support for command reply queue

[Volker Rümelin]

Add migration support for the PS/2 keyboard command reply queue.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/input/ps2.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 8c06fd7fb4..9376a8f4ce 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -80,6 +80,7 @@
  */
 #define PS2_BUFFER_SIZE     256
 #define PS2_QUEUE_SIZE      16  /* Queue size required by PS/2 protocol */
+#define PS2_QUEUE_HEADROOM  8   /* Queue size for keyboard command replies */
 
 /* Bits for 'modifiers' field in PS2KbdState */
 #define MOD_CTRL_L  (1 << 0)
@@ -985,17 +986,27 @@ static void ps2_common_reset(PS2State *s)
 static void ps2_common_post_load(PS2State *s)
 {
     PS2Queue *q = &s->queue;
+    int ccount = 0;
 
-    /* set the useful data buffer queue size <= PS2_QUEUE_SIZE */
-    if (q->count < 0) {
-        q->count = 0;
-    } else if (q->count > PS2_QUEUE_SIZE) {
-        q->count = PS2_QUEUE_SIZE;
+    /* limit the number of queued command replies to PS2_QUEUE_HEADROOM */
+    if (q->cwptr != -1) {
+        ccount = (q->cwptr - q->rptr) & (PS2_BUFFER_SIZE - 1);
+        if (ccount > PS2_QUEUE_HEADROOM) {
+            ccount = PS2_QUEUE_HEADROOM;
+        }
+    }
+
+    /* limit the scancode queue size to PS2_QUEUE_SIZE */
+    if (q->count < ccount) {
+        q->count = ccount;
+    } else if (q->count > ccount + PS2_QUEUE_SIZE) {
+        q->count = ccount + PS2_QUEUE_SIZE;
     }
 
-    /* sanitize rptr and recalculate wptr */
+    /* sanitize rptr and recalculate wptr and cwptr */
     q->rptr = q->rptr & (PS2_BUFFER_SIZE - 1);
     q->wptr = (q->rptr + q->count) & (PS2_BUFFER_SIZE - 1);
+    q->cwptr = ccount ? (q->rptr + ccount) & (PS2_BUFFER_SIZE - 1) : -1;
 }
 
 static void ps2_kbd_reset(void *opaque)
@@ -1086,6 +1097,22 @@ static const VMStateDescription vmstate_ps2_keyboard_need_high_bit = {
     }
 };
 
+static bool ps2_keyboard_cqueue_needed(void *opaque)
+{
+    PS2KbdState *s = opaque;
+
+    return s->common.queue.cwptr != -1; /* the queue is mostly empty */
+}
+
+static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
+    .name = "ps2kbd/command_reply_queue",
+    .needed = ps2_keyboard_cqueue_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int ps2_kbd_post_load(void* opaque, int version_id)
 {
     PS2KbdState *s = (PS2KbdState*)opaque;
@@ -1114,6 +1141,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_ps2_keyboard_ledstate,
         &vmstate_ps2_keyboard_need_high_bit,
+        &vmstate_ps2_keyboard_cqueue,
         NULL
     }
 };
-- 
2.26.2

[PATCH-for-6.1? 0/3] ps2: Fix issue #501 and #502

[Philippe Mathieu-Daudé]

On 8/7/21 2:10 PM, Volker Rümelin wrote:
> Since commit ff6e1624b3 (pckbd: don't update OBF flags if
> KBD_STAT_OBF is set) the OSes Minoca OS and Visopsys no longer
> have a working PS/2 keyboard and mouse. This is caused by a
> PS/2 queue stall due to a lost interrupt in the guest OS. This
> already happened before commit ff6e1624b3, but no one noticed
> because up to that point QEMU sent gratuitous keyboard and mouse
> interrupts and the queue restarted with keyboard input or mouse
> movement.
> 
> The lost interrupt is a guest bug. The fact that it's always
> lost is due to an inexact PS/2 keyboard emulation. The way in
> which the two operating systems e.g. set the keyboard LEDs,
> leaves a keyboard ACK reply in the keyboard queue that is
> unexpected for the guests.
> 
> This patch series improves the PS/2 keyboard emulation.
> 
> There's a workaround for issue #501 and #502 so I don't think
> this is rc3 material. But that decision is up to the maintainers.

Meanwhile, what about reverting ff6e1624b3 for 6.1?

> To verify patch 2/3 I plugged in an additional PS/2 keyboard
> into the host and started Linux with the command line option
> initcall_blacklist=i8042_init. Here is an example of the sequence
> to set the keyboard LEDs.
> 
> # #regular sequence to set the keyboard LEDs
> # inb --hex 0x64
> 1c
> # #PS/2 queue is empty
> # outb 0x60 0xed
> # inb --hex 0x64
> 15
> # inb --hex 0x60
> fa
> # inb --hex 0x64
> 14
> # outb 0x60 0x01
> # inb --hex 0x64
> 15
> # inb --hex 0x60
> fa
> # inb --hex 0x64
> 14
> 
> # #alternative sequence to set the keyboard LEDs
> # inb --hex 0x64
> 14
> # outb 0x60 0xed
> # outb 0x60 0x01
> # inb --hex 0x64
> 15
> # inb --hex 0x60
> fa
> # inb --hex 0x64
> 14
> 
> Volker Rümelin (3):
>   ps2: use the whole ps2 buffer but keep queue size
>   ps2: use a separate keyboard command reply queue
>   ps2: migration support for command reply queue
> 
>  hw/input/ps2.c | 214 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 133 insertions(+), 81 deletions(-)
> 

Re: [PATCH 2/3] ps2: use a separate keyboard command reply queue

[Gerd Hoffmann]

  Hi,

> A PS/2 keyboard has a separate command reply queue that is
> independant of the key queue. This prevents that command replies
> and keyboard input mix. Keyboard command replies take precedence
> over queued keystrokes. A new keyboard command removes any
> remaining command replies from the command reply queue.
> 
> Implement a separate keyboard command reply queue and clear the
> command reply queue before command execution. This brings the
> PS/2 keyboard emulation much closer to a real PS/2 keyboard.

It seems you place both queues in the existing 256-byte buffer,
probably to avoid breaking live migration.

Could you explain the logic a bit more, how the three pointers q->*ptr
variables play together here?

thanks,
  Gerd

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Gerd Hoffmann]

  Hi,

> +static bool ps2_keyboard_cqueue_needed(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    return s->common.queue.cwptr != -1; /* the queue is mostly empty */
> +}
> +
> +static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
> +    .name = "ps2kbd/command_reply_queue",
> +    .needed = ps2_keyboard_cqueue_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has
the same problem btw).  Chicken and egg problem on the receiving side:
How you properly evaluate ps2_keyboard_cqueue_needed() without having
common.queue.cwptr received yet?

With "cqueue not needed" being the common case migration will work
nicely in most cases nevertheless, but when the source machine actually
sends cqueue state things will break ...

Looking at data sent via vmstate works but ordering is important.  You
could check write_cmd because that is sent in the migration data stream
before ps2_keyboard_cqueue_needed() will be evaluated (just an random
example for the ordering, probably wouldn't help much in this case).

There is some redundancy in the data stream (wptr + rptr would be
enough, but we also send count).  Maybe that can be used somehow.
Of course the tricky part is to not confuse old qemu versions on
the receiving end.

If we can't find something we can add a property simliar to the one
for the extended keyboard state.

take care,
  Gerd

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Volker Rümelin]

>    Hi,
>
>> +static bool ps2_keyboard_cqueue_needed(void *opaque)
>> +{
>> +    PS2KbdState *s = opaque;
>> +
>> +    return s->common.queue.cwptr != -1; /* the queue is mostly empty */
>> +}
>> +
>> +static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
>> +    .name = "ps2kbd/command_reply_queue",
>> +    .needed = ps2_keyboard_cqueue_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has
> the same problem btw).  Chicken and egg problem on the receiving side:
> How you properly evaluate ps2_keyboard_cqueue_needed() without having
> common.queue.cwptr received yet?
>
> With "cqueue not needed" being the common case migration will work
> nicely in most cases nevertheless, but when the source machine actually
> sends cqueue state things will break ...

Hi Gerd,

this part actually works. .needed is only evaluated on the sending side. 
For the receiving side subsections are optional. Migration doesn't fail 
if a subsection isn't loaded. Before I sent this patch series one of the 
migration tests was a migration from 6.0.92 to 6.0.92 with one byte in 
the command reply queue and 3 bytes in the scancode queue. The migration 
didn't fail.

Migration will fail in the rare case when a new QEMU sends the 
command_reply_queue subsection to an old QEMU version.

> Looking at data sent via vmstate works but ordering is important.  You
> could check write_cmd because that is sent in the migration data stream
> before ps2_keyboard_cqueue_needed() will be evaluated (just an random
> example for the ordering, probably wouldn't help much in this case).
>
> There is some redundancy in the data stream (wptr + rptr would be
> enough, but we also send count).  Maybe that can be used somehow.
> Of course the tricky part is to not confuse old qemu versions on
> the receiving end.
>
> If we can't find something we can add a property simliar to the one
> for the extended keyboard state.

What is the best way to add such a compat property? The ps2 keyboard 
isn't a qdev device. I can't just add a property to the device class. Do 
I have to add a property to the i8042 and the pl050 device and propagate 
the property value with the ps2_kbd_init() call to the PS2KbdState?

With best regards,
Volker

> take care,
>    Gerd
>

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Gerd Hoffmann]

  Hi,

> this part actually works. .needed is only evaluated on the sending side. For
> the receiving side subsections are optional.  Migration doesn't fail if a
> subsection isn't loaded. Before I sent this patch series one of the
> migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the
> command reply queue and 3 bytes in the scancode queue. The migration didn't
> fail.

Hmm, ok.  If you actually tested it you are probably right.  My memory
tells me ->needed() is evaluated on both sending and receiving side as
the migration data stream does not carry the information whenever a
subsection is present or not.  But maybe my memories are wrong, or
things have changed, I don't follow migration changes that closely.

> > If we can't find something we can add a property simliar to the one
> > for the extended keyboard state.
> 
> What is the best way to add such a compat property? The ps2 keyboard isn't a
> qdev device. I can't just add a property to the device class. Do I have to
> add a property to the i8042 and the pl050 device and propagate the property
> value with the ps2_kbd_init() call to the PS2KbdState?

Yes, I think so.  But double-check the migration thing first, if your
approach works that is the easier way of course.

take care,
  Gerd

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Volker Rümelin]

>    Hi,
>
>> this part actually works. .needed is only evaluated on the sending side. For
>> the receiving side subsections are optional.  Migration doesn't fail if a
>> subsection isn't loaded. Before I sent this patch series one of the
>> migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the
>> command reply queue and 3 bytes in the scancode queue. The migration didn't
>> fail.
> Hmm, ok.  If you actually tested it you are probably right.  My memory
> tells me ->needed() is evaluated on both sending and receiving side as
> the migration data stream does not carry the information whenever a
> subsection is present or not.  But maybe my memories are wrong, or
> things have changed, I don't follow migration changes that closely.
>
>>> If we can't find something we can add a property simliar to the one
>>> for the extended keyboard state.
>> What is the best way to add such a compat property? The ps2 keyboard isn't a
>> qdev device. I can't just add a property to the device class. Do I have to
>> add a property to the i8042 and the pl050 device and propagate the property
>> value with the ps2_kbd_init() call to the PS2KbdState?
> Yes, I think so.  But double-check the migration thing first, if your
> approach works that is the easier way of course.
>
> take care,
>    Gerd
>

Hi Gerd,

this are the results of 5 migrations. I added a trace statement to 
function ps2_common_post_load() in my qemu-master. The first trace line 
is for ps2kbd, the second for ps2mouse.

#1 Migrate qemu 6.0.92 to qemu 6.0.92
   2 scancodes in ps2 keyboard queue

ps2_common_post_load: count 2, ccount 0
ps2_common_post_load: count 0, ccount 0

migration OK

./scripts/analyze-migration.py -f /var/tmp/qemu-state -d desc | less
         {
             "name": "ps2kbd",
             "instance_id": 0,
             "vmsd_name": "ps2kbd",
             "version": 3,
             "fields": [
                 {
                     "name": "common",
                     "type": "struct",
                     "struct": {
                         "vmsd_name": "PS2 Common State",
                         "version": 3,
                         "fields": [
                             {
                                 "name": "write_cmd",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.rptr",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.wptr",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.count",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.data",
                                 "type": "buffer",
                                 "size": 256
                             }
                         ]
                     },
                     "size": 272
                 },

#2 Migrate qemu 6.0.92 to qemu 6.0.92
   1 command reply and 2 scancodes in ps2 keyboard queue

ps2_common_post_load: count 3, ccount 1
ps2_common_post_load: count 0, ccount 0

migration OK

./scripts/analyze-migration.py -f /var/tmp/qemu-state -d desc | less
         {
             "name": "ps2kbd",
             "instance_id": 0,
             "vmsd_name": "ps2kbd",
             "version": 3,
             "fields": [
                 {
                     "name": "common",
                     "type": "struct",
                     "struct": {
                         "vmsd_name": "PS2 Common State",
                         "version": 3,
                         "fields": [
                             {
                                 "name": "write_cmd",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.rptr",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.wptr",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.count",
                                 "type": "int32",
                                 "size": 4
                             },
                             {
                                 "name": "queue.data",
                                 "type": "buffer",
                                 "size": 256
                             }
                         ]
                     },
                     "size": 272
                 },
                 {
                     "name": "scan_enabled",
                     "type": "int32",
                     "size": 4
                 },
                 {
                     "name": "translate",
                     "type": "int32",
                     "size": 4
                 },
                 {
                     "name": "scancode_set",
                     "type": "int32",
                     "size": 4
                 }
             ],
             "subsections": [
                 {
                     "vmsd_name": "ps2kbd/command_reply_queue",
                     "version": 0,
                     "fields": [
                         {
                             "name": "common.queue.cwptr",
                             "type": "int32",
                             "size": 4
                         }
                     ]
                 }
             ]
         },

#3 Migrate qemu 5.2.0 to qemu 6.0.92
   4 scancodes in ps2 keyboard queue

ps2_common_post_load: count 4, ccount 0
ps2_common_post_load: count 0, ccount 0

Migration OK

#4 Migrate qemu 6.0.92 to qemu 5.2.0
   2 scancodes in ps2 keyboard queue

Migration OK

#5 Migrate qemu 6.0.92 to qemu 5.2.0
   1 command reply and 2 scancodes in ps2 keyboard queue

qemu-system-x86_64: error while loading state for instance 0x0 of device 
'ps2kbd'
qemu-system-x86_64: load of migration failed: No such file or directory

Migration FAILED

With best regards,
Volker

Re: [PATCH-for-6.1? 0/3] ps2: Fix issue #501 and #502

[Gerd Hoffmann]

  Hi,

> > This patch series improves the PS/2 keyboard emulation.
> > 
> > There's a workaround for issue #501 and #502 so I don't think
> > this is rc3 material. But that decision is up to the maintainers.

Phew.  I'm a little nervous on adding it that late, so yes, I'd tend to
consider it 6.2 material too.

> Meanwhile, what about reverting ff6e1624b3 for 6.1?

Not going to fly due to patch dependencies, we would have to revert all
the ps2 fixes, which IMHO is likewise a bad idea for -rc3 ...

take care,
  Gerd