[Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Juan Quintela]

From: Christophe Fergeau <cfergeau@redhat.com>

Make the ps2 device track its ledstate so that we can migrate it.
Otherwise it gets lost across migration, and spice-server gets
confused about the actual keyboard state and sends bogus
caps/scroll/num key events. This fixes RH bug #729294

We only need to migrate the state when it is different of the default
one (0).

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ps2.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..1d9057b 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -92,6 +92,7 @@ typedef struct {
        not the keyboard controller.  */
     int translate;
     int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
+    int ledstate;
 } PS2KbdState;

 typedef struct {
@@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
     return val;
 }

+static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
+{
+    s->ledstate = ledstate;
+    kbd_put_ledstate(ledstate);
+}
+
 static void ps2_reset_keyboard(PS2KbdState *s)
 {
     s->scan_enabled = 1;
     s->scancode_set = 2;
-    kbd_put_ledstate(0);
+    ps2_set_ledstate(s, 0);
 }

 void ps2_write_keyboard(void *opaque, int val)
@@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_LEDS:
-        kbd_put_ledstate(val);
+        ps2_set_ledstate(s, val);
         ps2_queue(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
@@ -557,6 +564,33 @@ static const VMStateDescription vmstate_ps2_common = {
     }
 };

+static bool ps2_keyboard_ledstate_needed(void *opaque)
+{
+    PS2KbdState *s = opaque;
+
+    return s->ledstate != 0; /* 0 is default state */
+}
+
+static int ps2_kbd_ledstate_post_load(void *opaque, int version_id)
+{
+    PS2KbdState *s = opaque;
+
+    kbd_put_ledstate(s->ledstate);
+    return 0;
+}
+
+static const VMStateDescription vmstate_ps2_keyboard_ledstate = {
+    .name = "ps2kbd/ledstate",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .post_load = ps2_kbd_ledstate_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_INT32(ledstate, PS2KbdState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int ps2_kbd_post_load(void* opaque, int version_id)
 {
     PS2KbdState *s = (PS2KbdState*)opaque;
@@ -578,6 +612,14 @@ static const VMStateDescription vmstate_ps2_keyboard = {
         VMSTATE_INT32(translate, PS2KbdState),
         VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_ps2_keyboard_ledstate,
+            .needed = ps2_keyboard_ledstate_needed,
+        }, {
+            /* empty */
+        }
     }
 };

-- 
1.7.6.4

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Christophe Fergeau]

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

Hey,

On Mon, Oct 17, 2011 at 01:37:34PM +0200, Juan Quintela wrote:
> Make the ps2 device track its ledstate so that we can migrate it.
> Otherwise it gets lost across migration, and spice-server gets
> confused about the actual keyboard state and sends bogus
> caps/scroll/num key events. This fixes RH bug #729294
> 
> We only need to migrate the state when it is different of the default
> one (0).

I tested this version of the patch, and it works for me. Thanks!

Christophe

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Anthony Liguori]

On 10/17/2011 06:37 AM, Juan Quintela wrote:
> From: Christophe Fergeau<cfergeau@redhat.com>
>
> Make the ps2 device track its ledstate so that we can migrate it.
> Otherwise it gets lost across migration, and spice-server gets
> confused about the actual keyboard state and sends bogus
> caps/scroll/num key events. This fixes RH bug #729294
>
> We only need to migrate the state when it is different of the default
> one (0).

Er, isn't it going to be different from the default in like 99% of circumstances?

I think in cases where you're usually going to send a subsection, bumping the 
version is better.

Regards,

Anthony Liguori

>
> Signed-off-by: Christophe Fergeau<cfergeau@redhat.com>
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   hw/ps2.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ps2.c b/hw/ps2.c
> index 24228c1..1d9057b 100644
> --- a/hw/ps2.c
> +++ b/hw/ps2.c
> @@ -92,6 +92,7 @@ typedef struct {
>          not the keyboard controller.  */
>       int translate;
>       int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
> +    int ledstate;
>   } PS2KbdState;
>
>   typedef struct {
> @@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
>       return val;
>   }
>
> +static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
> +{
> +    s->ledstate = ledstate;
> +    kbd_put_ledstate(ledstate);
> +}
> +
>   static void ps2_reset_keyboard(PS2KbdState *s)
>   {
>       s->scan_enabled = 1;
>       s->scancode_set = 2;
> -    kbd_put_ledstate(0);
> +    ps2_set_ledstate(s, 0);
>   }
>
>   void ps2_write_keyboard(void *opaque, int val)
> @@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
>           s->common.write_cmd = -1;
>           break;
>       case KBD_CMD_SET_LEDS:
> -        kbd_put_ledstate(val);
> +        ps2_set_ledstate(s, val);
>           ps2_queue(&s->common, KBD_REPLY_ACK);
>           s->common.write_cmd = -1;
>           break;
> @@ -557,6 +564,33 @@ static const VMStateDescription vmstate_ps2_common = {
>       }
>   };
>
> +static bool ps2_keyboard_ledstate_needed(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    return s->ledstate != 0; /* 0 is default state */
> +}
> +
> +static int ps2_kbd_ledstate_post_load(void *opaque, int version_id)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    kbd_put_ledstate(s->ledstate);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ps2_keyboard_ledstate = {
> +    .name = "ps2kbd/ledstate",
> +    .version_id = 3,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = ps2_kbd_ledstate_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_INT32(ledstate, PS2KbdState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static int ps2_kbd_post_load(void* opaque, int version_id)
>   {
>       PS2KbdState *s = (PS2KbdState*)opaque;
> @@ -578,6 +612,14 @@ static const VMStateDescription vmstate_ps2_keyboard = {
>           VMSTATE_INT32(translate, PS2KbdState),
>           VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection []) {
> +        {
> +            .vmsd =&vmstate_ps2_keyboard_ledstate,
> +            .needed = ps2_keyboard_ledstate_needed,
> +        }, {
> +            /* empty */
> +        }
>       }
>   };
>

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Gerd Hoffmann]

   Hi,

>> We only need to migrate the state when it is different of the default
>> one (0).
>
> Er, isn't it going to be different from the default in like 99% of
> circumstances?

Why?  caps lock and scroll lock are off usually.   numlock depends on 
what the user prefers.  In case he cares in the first place of course. 
I expect most users don't and just go with the default.

On my physical machines numlock is off by default.  Likewise in my 
virtual machines:  SeaBIOS keeps it off by default, and Linux+Windows 
guests don't muck with it at boot.

I remember physical machines used to have numlock on by default in the 
90ies.  They even had a BIOS Setup option to pick the initial numlock 
state.  That seems to be out-fashioned these days though, I havn't seen 
that for quite a while now ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> We only need to migrate the state when it is different of the default
>>> one (0).
>>
>> Er, isn't it going to be different from the default in like 99% of
>> circumstances?
>
> Why?  caps lock and scroll lock are off usually.   numlock depends on
> what the user prefers.  In case he cares in the first place of
> course. I expect most users don't and just go with the default.
>
> On my physical machines numlock is off by default.  Likewise in my
> virtual machines:  SeaBIOS keeps it off by default, and Linux+Windows
> guests don't muck with it at boot.
>
> I remember physical machines used to have numlock on by default in the
> 90ies.  They even had a BIOS Setup option to pick the initial numlock
> state.  That seems to be out-fashioned these days though, I havn't
> seen that for quite a while now ...

I have nothing else to add.  This is exactly my point.  I think that I
have _never_ setup NumLock by default in any of my machines.  And
capslock use is very, very sporadic.  In my case, it would be 0 about
99.9% of the time.

Later, Juan.

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Anthony Liguori]

On 10/17/2011 10:15 AM, Gerd Hoffmann wrote:
> Hi,
>
>>> We only need to migrate the state when it is different of the default
>>> one (0).
>>
>> Er, isn't it going to be different from the default in like 99% of
>> circumstances?
>
> Why? caps lock and scroll lock are off usually. numlock depends on what the user
> prefers. In case he cares in the first place of course. I expect most users
> don't and just go with the default.
>
> On my physical machines numlock is off by default. Likewise in my virtual
> machines: SeaBIOS keeps it off by default, and Linux+Windows guests don't muck
> with it at boot.
>
> I remember physical machines used to have numlock on by default in the 90ies.
> They even had a BIOS Setup option to pick the initial numlock state. That seems
> to be out-fashioned these days though, I havn't seen that for quite a while now ...

Fair enough.

Regards,

Anthony Liguori

>
> cheers,
> Gerd
>
>

Re: [Qemu-devel] [PATCH v4] ps2: migrate ledstate

[Anthony Liguori]

On 10/17/2011 06:37 AM, Juan Quintela wrote:
> From: Christophe Fergeau<cfergeau@redhat.com>
>
> Make the ps2 device track its ledstate so that we can migrate it.
> Otherwise it gets lost across migration, and spice-server gets
> confused about the actual keyboard state and sends bogus
> caps/scroll/num key events. This fixes RH bug #729294
>
> We only need to migrate the state when it is different of the default
> one (0).
>
> Signed-off-by: Christophe Fergeau<cfergeau@redhat.com>
> Signed-off-by: Juan Quintela<quintela@redhat.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/ps2.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ps2.c b/hw/ps2.c
> index 24228c1..1d9057b 100644
> --- a/hw/ps2.c
> +++ b/hw/ps2.c
> @@ -92,6 +92,7 @@ typedef struct {
>          not the keyboard controller.  */
>       int translate;
>       int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */
> +    int ledstate;
>   } PS2KbdState;
>
>   typedef struct {
> @@ -195,11 +196,17 @@ uint32_t ps2_read_data(void *opaque)
>       return val;
>   }
>
> +static void ps2_set_ledstate(PS2KbdState *s, int ledstate)
> +{
> +    s->ledstate = ledstate;
> +    kbd_put_ledstate(ledstate);
> +}
> +
>   static void ps2_reset_keyboard(PS2KbdState *s)
>   {
>       s->scan_enabled = 1;
>       s->scancode_set = 2;
> -    kbd_put_ledstate(0);
> +    ps2_set_ledstate(s, 0);
>   }
>
>   void ps2_write_keyboard(void *opaque, int val)
> @@ -274,7 +281,7 @@ void ps2_write_keyboard(void *opaque, int val)
>           s->common.write_cmd = -1;
>           break;
>       case KBD_CMD_SET_LEDS:
> -        kbd_put_ledstate(val);
> +        ps2_set_ledstate(s, val);
>           ps2_queue(&s->common, KBD_REPLY_ACK);
>           s->common.write_cmd = -1;
>           break;
> @@ -557,6 +564,33 @@ static const VMStateDescription vmstate_ps2_common = {
>       }
>   };
>
> +static bool ps2_keyboard_ledstate_needed(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    return s->ledstate != 0; /* 0 is default state */
> +}
> +
> +static int ps2_kbd_ledstate_post_load(void *opaque, int version_id)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    kbd_put_ledstate(s->ledstate);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_ps2_keyboard_ledstate = {
> +    .name = "ps2kbd/ledstate",
> +    .version_id = 3,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .post_load = ps2_kbd_ledstate_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_INT32(ledstate, PS2KbdState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static int ps2_kbd_post_load(void* opaque, int version_id)
>   {
>       PS2KbdState *s = (PS2KbdState*)opaque;
> @@ -578,6 +612,14 @@ static const VMStateDescription vmstate_ps2_keyboard = {
>           VMSTATE_INT32(translate, PS2KbdState),
>           VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection []) {
> +        {
> +            .vmsd =&vmstate_ps2_keyboard_ledstate,
> +            .needed = ps2_keyboard_ledstate_needed,
> +        }, {
> +            /* empty */
> +        }
>       }
>   };
>