[Qemu-devel] [PATCH] spice-input: migrate ledstate

[Qemu-devel] [PATCH] spice-input: migrate ledstate

[Alon Levy]

The caps/scroll/num lock state is not tracked by the ps2 device, but by
spice-input QemuSpiceKbd->ledstate. To fix losing it across migration,
and then having the server send caps/scroll/nums keys when the client
sends a SPICE_MSGC_INPUTS_KEY_MODIFIERS, migrate it.

RHBZ# 729294

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 trace-events     |    3 +++
 ui/spice-input.c |   24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/trace-events b/trace-events
index 63d8c8e..605dc79 100644
--- a/trace-events
+++ b/trace-events
@@ -538,3 +538,6 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
 # monitor.c
 handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
 monitor_protocol_emitter(void *mon) "mon %p"
+
+# ui/spice-input.c
+disable spice_input_post_load(int ledstate) "leds %d"
diff --git a/ui/spice-input.c b/ui/spice-input.c
index af4223d..2f0d3af 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -26,6 +26,8 @@
 #include "qemu-common.h"
 #include "qemu-spice.h"
 #include "console.h"
+#include "hw/hw.h"
+#include "trace.h"
 
 /* keyboard bits */
 
@@ -195,6 +197,27 @@ static void mouse_mode_notifier(Notifier *notifier, void *data)
     pointer->absolute = is_absolute;
 }
 
+static int qemu_spice_input_post_load(void *opaque, int version_id)
+{
+    QemuSpiceKbd *s = (QemuSpiceKbd *)opaque;
+
+    trace_spice_input_post_load(s->ledstate);
+    kbd_leds(s, s->ledstate);
+    return 0;
+}
+
+static const VMStateDescription vmstate_spice_kbd = {
+    .name = "spice-input",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .post_load = qemu_spice_input_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_INT32(ledstate, QemuSpiceKbd),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void qemu_spice_input_init(void)
 {
     QemuSpiceKbd *kbd;
@@ -204,6 +227,7 @@ void qemu_spice_input_init(void)
     kbd->sin.base.sif = &kbd_interface.base;
     qemu_spice_add_interface(&kbd->sin.base);
     qemu_add_led_event_handler(kbd_leds, kbd);
+    vmstate_register(NULL, 0, &vmstate_spice_kbd, kbd);
 
     pointer = g_malloc0(sizeof(*pointer));
     pointer->mouse.base.sif  = &mouse_interface.base;
-- 
1.7.6.4

[Qemu-devel] [PATCH] spice-input: migrate ledstate

[Alon Levy]

The caps/scroll/num lock state is not tracked by the ps2 device, but by
spice-input QemuSpiceKbd->ledstate. To fix losing it across migration,
and then having the server send caps/scroll/nums keys when the client
sends a SPICE_MSGC_INPUTS_KEY_MODIFIERS, migrate it.

RHBZ# 729294

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 trace-events     |    3 +++
 ui/spice-input.c |   24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/trace-events b/trace-events
index 63d8c8e..605dc79 100644
--- a/trace-events
+++ b/trace-events
@@ -538,3 +538,6 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
 # monitor.c
 handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
 monitor_protocol_emitter(void *mon) "mon %p"
+
+# ui/spice-input.c
+disable spice_input_post_load(int ledstate) "leds %d"
diff --git a/ui/spice-input.c b/ui/spice-input.c
index af4223d..2f0d3af 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -26,6 +26,8 @@
 #include "qemu-common.h"
 #include "qemu-spice.h"
 #include "console.h"
+#include "hw/hw.h"
+#include "trace.h"
 
 /* keyboard bits */
 
@@ -195,6 +197,27 @@ static void mouse_mode_notifier(Notifier *notifier, void *data)
     pointer->absolute = is_absolute;
 }
 
+static int qemu_spice_input_post_load(void *opaque, int version_id)
+{
+    QemuSpiceKbd *s = (QemuSpiceKbd *)opaque;
+
+    trace_spice_input_post_load(s->ledstate);
+    kbd_leds(s, s->ledstate);
+    return 0;
+}
+
+static const VMStateDescription vmstate_spice_kbd = {
+    .name = "spice-input",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .post_load = qemu_spice_input_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_INT32(ledstate, QemuSpiceKbd),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void qemu_spice_input_init(void)
 {
     QemuSpiceKbd *kbd;
@@ -204,6 +227,7 @@ void qemu_spice_input_init(void)
     kbd->sin.base.sif = &kbd_interface.base;
     qemu_spice_add_interface(&kbd->sin.base);
     qemu_add_led_event_handler(kbd_leds, kbd);
+    vmstate_register(NULL, 0, &vmstate_spice_kbd, kbd);
 
     pointer = g_malloc0(sizeof(*pointer));
     pointer->mouse.base.sif  = &mouse_interface.base;
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] spice-input: migrate ledstate

[Alon Levy]

On Tue, Oct 11, 2011 at 06:42:48PM +0200, Alon Levy wrote:
> The caps/scroll/num lock state is not tracked by the ps2 device, but by
> spice-input QemuSpiceKbd->ledstate. To fix losing it across migration,
> and then having the server send caps/scroll/nums keys when the client
> sends a SPICE_MSGC_INPUTS_KEY_MODIFIERS, migrate it.
> 

Self NACK, messed the vmstate name.

> RHBZ# 729294
> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  trace-events     |    3 +++
>  ui/spice-input.c |   24 ++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/trace-events b/trace-events
> index 63d8c8e..605dc79 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -538,3 +538,6 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
>  # monitor.c
>  handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
>  monitor_protocol_emitter(void *mon) "mon %p"
> +
> +# ui/spice-input.c
> +disable spice_input_post_load(int ledstate) "leds %d"
> diff --git a/ui/spice-input.c b/ui/spice-input.c
> index af4223d..2f0d3af 100644
> --- a/ui/spice-input.c
> +++ b/ui/spice-input.c
> @@ -26,6 +26,8 @@
>  #include "qemu-common.h"
>  #include "qemu-spice.h"
>  #include "console.h"
> +#include "hw/hw.h"
> +#include "trace.h"
>  
>  /* keyboard bits */
>  
> @@ -195,6 +197,27 @@ static void mouse_mode_notifier(Notifier *notifier, void *data)
>      pointer->absolute = is_absolute;
>  }
>  
> +static int qemu_spice_input_post_load(void *opaque, int version_id)
> +{
> +    QemuSpiceKbd *s = (QemuSpiceKbd *)opaque;
> +
> +    trace_spice_input_post_load(s->ledstate);
> +    kbd_leds(s, s->ledstate);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_spice_kbd = {
> +    .name = "spice-input",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,
> +    .post_load = qemu_spice_input_post_load,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_INT32(ledstate, QemuSpiceKbd),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  void qemu_spice_input_init(void)
>  {
>      QemuSpiceKbd *kbd;
> @@ -204,6 +227,7 @@ void qemu_spice_input_init(void)
>      kbd->sin.base.sif = &kbd_interface.base;
>      qemu_spice_add_interface(&kbd->sin.base);
>      qemu_add_led_event_handler(kbd_leds, kbd);
> +    vmstate_register(NULL, 0, &vmstate_spice_kbd, kbd);
>  
>      pointer = g_malloc0(sizeof(*pointer));
>      pointer->mouse.base.sif  = &mouse_interface.base;
> -- 
> 1.7.6.4
> 

Re: [Qemu-devel] [PATCH] spice-input: migrate ledstate

[Anthony Liguori]

On 10/11/2011 11:28 AM, Alon Levy wrote:
> The caps/scroll/num lock state is not tracked by the ps2 device, but by
> spice-input QemuSpiceKbd->ledstate. To fix losing it across migration,
> and then having the server send caps/scroll/nums keys when the client
> sends a SPICE_MSGC_INPUTS_KEY_MODIFIERS, migrate it.
>
> RHBZ# 729294
>
> Signed-off-by: Alon Levy<alevy@redhat.com>
> ---
>   trace-events     |    3 +++
>   ui/spice-input.c |   24 ++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/trace-events b/trace-events
> index 63d8c8e..605dc79 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -538,3 +538,6 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
>   # monitor.c
>   handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
>   monitor_protocol_emitter(void *mon) "mon %p"
> +
> +# ui/spice-input.c
> +disable spice_input_post_load(int ledstate) "leds %d"
> diff --git a/ui/spice-input.c b/ui/spice-input.c
> index af4223d..2f0d3af 100644
> --- a/ui/spice-input.c
> +++ b/ui/spice-input.c
> @@ -26,6 +26,8 @@
>   #include "qemu-common.h"
>   #include "qemu-spice.h"
>   #include "console.h"
> +#include "hw/hw.h"
> +#include "trace.h"
>
>   /* keyboard bits */
>
> @@ -195,6 +197,27 @@ static void mouse_mode_notifier(Notifier *notifier, void *data)
>       pointer->absolute = is_absolute;
>   }
>
> +static int qemu_spice_input_post_load(void *opaque, int version_id)
> +{
> +    QemuSpiceKbd *s = (QemuSpiceKbd *)opaque;
> +
> +    trace_spice_input_post_load(s->ledstate);
> +    kbd_leds(s, s->ledstate);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_spice_kbd = {
> +    .name = "spice-input",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,
> +    .post_load = qemu_spice_input_post_load,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_INT32(ledstate, QemuSpiceKbd),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +

This is wrong IMHO.  Spice is not a device.  Why not just add LED state to the 
migration of the PS/2 keyboard?

Regards,

Anthony Liguori

>   void qemu_spice_input_init(void)
>   {
>       QemuSpiceKbd *kbd;
> @@ -204,6 +227,7 @@ void qemu_spice_input_init(void)
>       kbd->sin.base.sif =&kbd_interface.base;
>       qemu_spice_add_interface(&kbd->sin.base);
>       qemu_add_led_event_handler(kbd_leds, kbd);
> +    vmstate_register(NULL, 0,&vmstate_spice_kbd, kbd);
>
>       pointer = g_malloc0(sizeof(*pointer));
>       pointer->mouse.base.sif  =&mouse_interface.base;

Re: [Qemu-devel] [PATCH] spice-input: migrate ledstate

[Alon Levy]

On Tue, Oct 11, 2011 at 01:23:00PM -0500, Anthony Liguori wrote:
> On 10/11/2011 11:28 AM, Alon Levy wrote:
> >The caps/scroll/num lock state is not tracked by the ps2 device, but by
> >spice-input QemuSpiceKbd->ledstate. To fix losing it across migration,
> >and then having the server send caps/scroll/nums keys when the client
> >sends a SPICE_MSGC_INPUTS_KEY_MODIFIERS, migrate it.
> >
> >RHBZ# 729294
> >
> >Signed-off-by: Alon Levy<alevy@redhat.com>
> >---
> >  trace-events     |    3 +++
> >  ui/spice-input.c |   24 ++++++++++++++++++++++++
> >  2 files changed, 27 insertions(+), 0 deletions(-)
> >
> >diff --git a/trace-events b/trace-events
> >index 63d8c8e..605dc79 100644
> >--- a/trace-events
> >+++ b/trace-events
> >@@ -538,3 +538,6 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
> >  # monitor.c
> >  handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
> >  monitor_protocol_emitter(void *mon) "mon %p"
> >+
> >+# ui/spice-input.c
> >+disable spice_input_post_load(int ledstate) "leds %d"
> >diff --git a/ui/spice-input.c b/ui/spice-input.c
> >index af4223d..2f0d3af 100644
> >--- a/ui/spice-input.c
> >+++ b/ui/spice-input.c
> >@@ -26,6 +26,8 @@
> >  #include "qemu-common.h"
> >  #include "qemu-spice.h"
> >  #include "console.h"
> >+#include "hw/hw.h"
> >+#include "trace.h"
> >
> >  /* keyboard bits */
> >
> >@@ -195,6 +197,27 @@ static void mouse_mode_notifier(Notifier *notifier, void *data)
> >      pointer->absolute = is_absolute;
> >  }
> >
> >+static int qemu_spice_input_post_load(void *opaque, int version_id)
> >+{
> >+    QemuSpiceKbd *s = (QemuSpiceKbd *)opaque;
> >+
> >+    trace_spice_input_post_load(s->ledstate);
> >+    kbd_leds(s, s->ledstate);
> >+    return 0;
> >+}
> >+
> >+static const VMStateDescription vmstate_spice_kbd = {
> >+    .name = "spice-input",
> >+    .version_id = 0,
> >+    .minimum_version_id = 0,
> >+    .minimum_version_id_old = 0,
> >+    .post_load = qemu_spice_input_post_load,
> >+    .fields      = (VMStateField[]) {
> >+        VMSTATE_INT32(ledstate, QemuSpiceKbd),
> >+        VMSTATE_END_OF_LIST()
> >+    }
> >+};
> >+
> 
> This is wrong IMHO.  Spice is not a device.  Why not just add LED
> state to the migration of the PS/2 keyboard?
> 
> Regards,
> 
> Anthony Liguori

There is no ledstate in a PS/2 keyboard (or I'm reading too much into
the implementation in qemu). usb-hid does have it, but PS/2
just passes fragments to the PC, and get's led setting commands back
(for some specific keys). Or are you suggesting to put this stuff in
there anyway?

> 
> >  void qemu_spice_input_init(void)
> >  {
> >      QemuSpiceKbd *kbd;
> >@@ -204,6 +227,7 @@ void qemu_spice_input_init(void)
> >      kbd->sin.base.sif =&kbd_interface.base;
> >      qemu_spice_add_interface(&kbd->sin.base);
> >      qemu_add_led_event_handler(kbd_leds, kbd);
> >+    vmstate_register(NULL, 0,&vmstate_spice_kbd, kbd);
> >
> >      pointer = g_malloc0(sizeof(*pointer));
> >      pointer->mouse.base.sif  =&mouse_interface.base;
> 

Re: [Qemu-devel] [PATCH] spice-input: migrate ledstate

[Gerd Hoffmann]

   Hi,

> There is no ledstate in a PS/2 keyboard (or I'm reading too much into
> the implementation in qemu).

There is.  It isn't explicitly stored into the state struct though 
because the ps/2 keyboard itself doesn't use it, it just calls 
kbd_put_ledstate() to inform others about it.

cheers,
   Gerd

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

[Christophe Fergeau]

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.

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

diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..f19ea18 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;
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
 
     if (version_id == 2)
         s->scancode_set=2;
+    kbd_put_ledstate(s->ledstate);
     return 0;
 }
 
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
         VMSTATE_INT32(scan_enabled, PS2KbdState),
         VMSTATE_INT32(translate, PS2KbdState),
         VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+        VMSTATE_INT32(ledstate, PS2KbdState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.6.4

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

[Anthony Liguori]

On 10/12/2011 11:35 AM, Christophe Fergeau 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.
>
> Signed-off-by: Christophe Fergeau<cfergeau@redhat.com>
> ---
>   hw/ps2.c |   13 +++++++++++--
>   1 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ps2.c b/hw/ps2.c
> index 24228c1..f19ea18 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;
> @@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
>
>       if (version_id == 2)
>           s->scancode_set=2;
> +    kbd_put_ledstate(s->ledstate);
>       return 0;
>   }
>
> @@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
>           VMSTATE_INT32(scan_enabled, PS2KbdState),
>           VMSTATE_INT32(translate, PS2KbdState),
>           VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
> +        VMSTATE_INT32(ledstate, PS2KbdState),

If you're adding a field, you need to bump the version.

Regards,

Anthony Liguori

>           VMSTATE_END_OF_LIST()
>       }
>   };

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

[Christophe Fergeau]

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

Hey,

On Wed, Oct 12, 2011 at 06:35:28PM +0200, Christophe Fergeau 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 patch adds a ledstate to the PS2 keyboard driver as an alternative
to the original patch. I forgot to mention in the changelog entry that
this fixes https://bugzilla.redhat.com/show_bug.cgi?id=729294
One thing that could be improved after this patch is that the ledstate
is now present both in spice-input and hw/ps2.c, but I couldn't find
an easy way to get the ps2 ledstate from spice-input so I left things
this way for now.

Christophe

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

[Qemu-devel] [PATCHv2] ps2: migrate ledstate

[Christophe Fergeau]

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

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
---
 hw/ps2.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..3a88681 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;
@@ -544,7 +551,7 @@ static void ps2_mouse_reset(void *opaque)
 
 static const VMStateDescription vmstate_ps2_common = {
     .name = "PS2 Common State",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 2,
     .minimum_version_id_old = 2,
     .fields      = (VMStateField []) {
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
 
     if (version_id == 2)
         s->scancode_set=2;
+    kbd_put_ledstate(s->ledstate);
     return 0;
 }
 
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
         VMSTATE_INT32(scan_enabled, PS2KbdState),
         VMSTATE_INT32(translate, PS2KbdState),
         VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+        VMSTATE_INT32(ledstate, PS2KbdState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.6.4

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

[Gerd Hoffmann]

   Hi,

> One thing that could be improved after this patch is that the ledstate
> is now present both in spice-input and hw/ps2.c, but I couldn't find
> an easy way to get the ps2 ledstate from spice-input so I left things
> this way for now.

Directly linking ps/2 and spice-input would be wrong anyway as there are 
other keyboards (usb) too ...

input.c could cache the ledstate and offer a kbd_get_ledstate function, 
so anyone interested in the state doesn't need to keep it itself.  That 
is a pretty minor thing though IMHO.

cheers,
   Gerd

[Qemu-devel] [PATCHv3] ps2: migrate ledstate

[Christophe Fergeau]

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

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

---
v3: use VMSTATE_UINT32_V to indicate the ledstate field was added
    in version_id == 4
---
 hw/ps2.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index 24228c1..f37112b 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;
@@ -544,7 +551,7 @@ static void ps2_mouse_reset(void *opaque)
 
 static const VMStateDescription vmstate_ps2_common = {
     .name = "PS2 Common State",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 2,
     .minimum_version_id_old = 2,
     .fields      = (VMStateField []) {
@@ -563,6 +570,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
 
     if (version_id == 2)
         s->scancode_set=2;
+    kbd_put_ledstate(s->ledstate);
     return 0;
 }
 
@@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
         VMSTATE_INT32(scan_enabled, PS2KbdState),
         VMSTATE_INT32(translate, PS2KbdState),
         VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
+        VMSTATE_INT32_V(ledstate, PS2KbdState, 4),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.6.4

Re: [Qemu-devel] [PATCHv3] ps2: migrate ledstate

[Gerd Hoffmann]

>
>   static const VMStateDescription vmstate_ps2_common = {
>       .name = "PS2 Common State",
> -    .version_id = 3,
> +    .version_id = 4,
>       .minimum_version_id = 2,
>       .minimum_version_id_old = 2,
>       .fields      = (VMStateField []) {

> @@ -577,6 +585,7 @@ static const VMStateDescription vmstate_ps2_keyboard = {
>           VMSTATE_INT32(scan_enabled, PS2KbdState),
>           VMSTATE_INT32(translate, PS2KbdState),
>           VMSTATE_INT32_V(scancode_set, PS2KbdState,3),
> +        VMSTATE_INT32_V(ledstate, PS2KbdState, 4),
>           VMSTATE_END_OF_LIST()
>       }

version_id in vmstate_ps2_keyboard must be updated too.

The version update in vmstate_ps2_common might not be needed, IIRC the 
versions for stuff referenced via VMSTATE_STRUCT() isn't used anyway, Juan?

cheers,
   Gerd

Re: [Qemu-devel] [PATCHv3] ps2: migrate ledstate

[Christophe Fergeau]

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

On Mon, Oct 17, 2011 at 11:25:42AM +0200, Gerd Hoffmann wrote:
> >
> >  static const VMStateDescription vmstate_ps2_common = {
> >      .name = "PS2 Common State",
> >-    .version_id = 3,
> >+    .version_id = 4,
> >      .minimum_version_id = 2,
> >      .minimum_version_id_old = 2,
> >      .fields      = (VMStateField []) {
> 
> version_id in vmstate_ps2_keyboard must be updated too.

Yeah, I somehow updated the field in the wrong struct, /me blushes and
hides. I don't think this struct version needs to be updated.

> The version update in vmstate_ps2_common might not be needed, IIRC
> the versions for stuff referenced via VMSTATE_STRUCT() isn't used
> anyway, Juan?

Ah, ok, I hoped it would help to handle migration between versions with and
without this field, I guess I was too optimistic :)

Thanks,

Christophe

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