diff mbox series

[09/10] pckbd: correctly disable PS/2 communication

Message ID 20210505192133.7480-9-vr_qemu@t-online.de (mailing list archive)
State New, archived
Headers show
Series PS/2 controller related fixes | expand

Commit Message

Volker Rümelin May 5, 2021, 7:21 p.m. UTC 
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 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 | 48 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

Comments

Philippe Mathieu-Daudé May 6, 2021, 6:27 a.m. UTC | #1 
Hi Volker,

On 5/5/21 9:21 PM, Volker Rümelin wrote:
> 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 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 | 48 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 13 deletions(-)

> +static uint8_t kbd_pending(KBDState *s)
> +{
> +    return s->pending & (~s->mode | ~(KBD_PENDING_KBD | KBD_PENDING_AUX));
> +}

Please introduce kbd_pending() in a preliminary patch.

>  /* 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;
Volker Rümelin May 6, 2021, 6:06 p.m. UTC | #2 
> Hi Volker,
>
> On 5/5/21 9:21 PM, Volker Rümelin wrote:
>> 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 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 | 48 +++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 35 insertions(+), 13 deletions(-)
>> +static uint8_t kbd_pending(KBDState *s)
>> +{
>> +    return s->pending & (~s->mode | ~(KBD_PENDING_KBD | KBD_PENDING_AUX));
>> +}
> Please introduce kbd_pending() in a preliminary patch.

OK, I will send a version 2 patch series with an additional patch.

With best regards,
Volker

>>   /* 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;
diff mbox series

Patch

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 3c41c11841..7c476f7a3e 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
 
@@ -163,8 +165,6 @@  typedef struct KBDState {
     hwaddr mask;
 } KBDState;
 
-/* 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_lines(KBDState *s)
 {
     int irq_kbd_level, irq_mouse_level;
@@ -178,8 +178,7 @@  static void kbd_update_irq_lines(KBDState *s)
                 irq_mouse_level = 1;
             }
         } else {
-            if ((s->mode & KBD_MODE_KBD_INT) &&
-                !(s->mode & KBD_MODE_DISABLE_KBD)) {
+            if (s->mode & KBD_MODE_KBD_INT) {
                 irq_kbd_level = 1;
             }
         }
@@ -195,21 +194,28 @@  static void kbd_deassert_irq(KBDState *s)
     kbd_update_irq_lines(s);
 }
 
+static uint8_t kbd_pending(KBDState *s)
+{
+    return s->pending & (~s->mode | ~(KBD_PENDING_KBD | KBD_PENDING_AUX));
+}
+
 /* 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 +239,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);
     }
 }
@@ -269,7 +275,7 @@  static void kbd_throttle_timeout(void *opaque)
     if (s->status & KBD_STAT_OBF) {
         return;
     }
-    if (s->pending) {
+    if (kbd_pending(s)) {
         kbd_update_irq(s);
     }
 }
@@ -297,7 +303,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;
@@ -350,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);
@@ -429,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;
@@ -455,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;
@@ -532,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);
     }