| Message ID | 1318851454-22335-1-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 */ > + } > } > }; >
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
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.
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 > >
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 */ > + } > } > }; >
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 */ + } } };