ps2: prevent changing irq state on save and load

PATCH Applied Pavel Dovgalyuk 4 participants 7 messages May 11, 2018 – Sep 11, 2018

Share & tools

Pavel Dovgalyuk

author

2018-05-11 08:16 UTC #0 lore raw

PATCH 1/1 ps2: prevent changing irq state on save and load

Commit 2858ab09e6f708e381fc1a1cc87e747a690c4884 changed
PS/2 keyboard/mouse buffers to the standard size. However, its state may change when migrating from the old buffer size and therefore irq needs updating. But this change made wrong, because it throws the whole queue if there are too much data instead of cropping it.

That commit also updates irq (because the queue state may change). But updating the irq may change the VM state (and determinism of the execution). E.g., when replaying the execution, one may save the VM state and the state of the interrupt controller will be updated at the moment of saving, instead of using the recorded update events.

This patch makes the queue update deterministic: it removes the update_irq call and crops the queue to prevent losing the characters and changing the required irq status.

Signed-off-by Pavel Dovgalyuk <[email protected]>

---
 hw/input/ps2.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

hw/input/ps2.c +6 -2

index 06f5d2a..8b1931b 100644
@@ -837,7 +837,12 @@ static void ps2_common_post_load(PS2State *s)
     uint8_t tmp_data[PS2_QUEUE_SIZE];
 
     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
-    size = (q->count < 0 || q->count > PS2_QUEUE_SIZE) ? 0 : q->count;
+    size = q->count;
+    if (q->count < 0) {
+        size = 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++) {

@@ -852,7 +857,6 @@ static void ps2_common_post_load(PS2State *s)
     q->rptr = 0;
     q->wptr = size;
     q->count = size;
-    s->update_irq(s->update_arg, q->count != 0);
 }
 
 static void ps2_kbd_reset(void *opaque)

Paolo Bonzini

Applied

2018-05-11 09:13 UTC #1 lore raw

Quoted 41 lines · Pavel Dovgalyuk

Commit 2858ab09e6f708e381fc1a1cc87e747a690c4884 changed
PS/2 keyboard/mouse buffers to the standard size. However, its state may change when migrating from the old buffer size and therefore irq needs updating. But this change made wrong, because it throws the whole queue if there are too much data instead of cropping it.
That commit also updates irq (because the queue state may change). But updating the irq may change the VM state (and determinism of the execution). E.g., when replaying the execution, one may save the VM state and the state of the interrupt controller will be updated at the moment of saving, instead of using the recorded update events.
This patch makes the queue update deterministic: it removes the update_irq call and crops the queue to prevent losing the characters and changing the required irq status.
Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 hw/input/ps2.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
hw/input/ps2.c +6 -1
index 06f5d2a..8b1931b 100644
@@ -837,7 +837,12 @@ static void ps2_common_post_load(PS2State *s)
     uint8_t tmp_data[PS2_QUEUE_SIZE];
 
     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
-    size = (q->count < 0 || q->count > PS2_QUEUE_SIZE) ? 0 : q->count;
+    size = q->count;
+    if (q->count < 0) {
+        size = 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++) {

hw/input/ps2.c static void ps2_common_post_load(PS2State *s) · line 862

Quoted 7 lines

hw/input/ps2.c +0 -1

@@ -852,7 +857,6 @@ static void ps2_common_post_load(PS2State *s)
     q->rptr = 0;
     q->wptr = size;
     q->count = size;
-    s->update_irq(s->update_arg, q->count != 0);
 }
 
 static void ps2_kbd_reset(void *opaque)

Queued, thanks.

Paolo

Pavel Dovgalyuk

2018-09-11 10:18 UTC #2 lore raw

Paolo, have you forgot about this?

Pavel Dovgalyuk

Quoted 61 lines

-----Original Message-----
From: Paolo Bonzini [mailto:[email protected]]
Sent: Friday, May 11, 2018 12:14 PM
To: Pavel Dovgalyuk; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] ps2: prevent changing irq state on save and load
Quoted 41 lines · Pavel Dovgalyuk
Commit 2858ab09e6f708e381fc1a1cc87e747a690c4884 changed
PS/2 keyboard/mouse buffers to the standard size. However, its state may change when migrating from the old buffer size and therefore irq needs updating. But this change made wrong, because it throws the whole queue if there are too much data instead of cropping it.
That commit also updates irq (because the queue state may change). But updating the irq may change the VM state (and determinism of the execution). E.g., when replaying the execution, one may save the VM state and the state of the interrupt controller will be updated at the moment of saving, instead of using the recorded update events.
This patch makes the queue update deterministic: it removes the update_irq call and crops the queue to prevent losing the characters and changing the required irq status.
Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 hw/input/ps2.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
hw/input/ps2.c +6 -1
index 06f5d2a..8b1931b 100644
@@ -837,7 +837,12 @@ static void ps2_common_post_load(PS2State *s)
     uint8_t tmp_data[PS2_QUEUE_SIZE];
 
     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
-    size = (q->count < 0 || q->count > PS2_QUEUE_SIZE) ? 0 : q->count;
+    size = q->count;
+    if (q->count < 0) {
+        size = 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++) {
hw/input/ps2.c static void ps2_common_post_load(PS2State *s) · line 862
Quoted 7 lines
hw/input/ps2.c +0 -1
@@ -852,7 +857,6 @@ static void ps2_common_post_load(PS2State *s)
     q->rptr = 0;
     q->wptr = size;
     q->count = size;
-    s->update_irq(s->update_arg, q->count != 0);
 }
 
 static void ps2_kbd_reset(void *opaque)
Queued, thanks.
Paolo

Paolo Bonzini

2018-09-11 11:11 UTC #3 lore raw

Quoted 3 lines · Pavel Dovgalyuk

Paolo, have you forgot about this?
Pavel Dovgalyuk

Yes, though Gerd might be a better match for the patch.

Paolo

Gerd Hoffmann

2018-09-11 11:25 UTC #4 lore raw

Quoted 6 lines · Paolo Bonzini

Quoted 3 lines · Pavel Dovgalyuk
Paolo, have you forgot about this?
Pavel Dovgalyuk
Yes, though Gerd might be a better match for the patch.

Dropped it from my patch mailbox after seeing your 'queued' message.

Pavel, can you resend?

thanks,
Gerd

Paolo Bonzini

2018-09-11 11:26 UTC #5 lore raw

Quoted 11 lines · Gerd Hoffmann

Quoted 6 lines · Paolo Bonzini
Quoted 3 lines · Pavel Dovgalyuk
Paolo, have you forgot about this?
Pavel Dovgalyuk
Yes, though Gerd might be a better match for the patch.
Dropped it from my patch mailbox after seeing your 'queued' message.
Pavel, can you resend?

No need, I'll queue it for real now. :)

Paolo

Pavel Dovgalyuk

2018-09-11 11:27 UTC #6 lore raw

Quoted 8 lines

From: Paolo Bonzini [mailto:[email protected]]
Quoted 6 lines · Gerd Hoffmann
Quoted 5 lines · Paolo Bonzini
Quoted 3 lines · Pavel Dovgalyuk
Paolo, have you forgot about this?
Pavel Dovgalyuk

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Yes, though Gerd might be a better match for the patch.
Dropped it from my patch mailbox after seeing your 'queued' message.
Pavel, can you resend?
No need, I'll queue it for real now. :)

Thank you.

Pavel Dovgalyuk