| Message ID | 20130531123117.GA8400@t430s.nay.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: > On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: > > Amos Kong <akong@redhat.com> writes: > > > > > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE, > > > but ps2 backend doesn't process it and no auto-repeat implementation. > > > This patch adds support of auto-repeat feature. The repeated events > > > from host are ignored and re-implements ps2's auto-repeat. > > > > > > Guest ps2 driver sets autorepeat to fastest possible in reset, > > > period: 250ms, delay: 33ms > > > > > > Tested by 'sendkey' monitor command. > > > Tested by Linux & Windows guests with SDL, VNC, SPICE, GTK+ > > > > > > referenced: http://www.computer-engineering.org/ps2keyboard/ > > > > > > Signed-off-by: Amos Kong <akong@redhat.com> > > > --- > > > hw/input/ps2.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 41 insertions(+) > > > > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > > > index 3412079..8adbb4a 100644 > > > --- a/hw/input/ps2.c > > > +++ b/hw/input/ps2.c > > > @@ -94,6 +94,10 @@ typedef struct { > > > int translate; > > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ > > > int ledstate; > > > + int repeat_period; /* typematic period, ms */ > > > + int repeat_delay; /* typematic delay, ms */ > > > + int repeat_key; /* keycode to repeat */ > > > + QEMUTimer *repeat_timer; > > > > This state needs to be migrated, no? I suspect it can/should be done > > via a subsection too. > > It sounds only reasonable for 'sendkey' command. We want to repeat one > key for 100 times, the key should be continaully repeated in the dest > vm until it reaches to 100 times. > > For implement this, we should also migrate key_timer in ui/input.c, > then it will send a release event to ps2 queue when the key_timer > is expired. The bottom patch migrates repeat_timer & repeat_key, > where should we save key_timer for migration? > > ---- > > For the VNC/SPICE/SDL/GTK+ windows, qemu gets repeated press events > from host. After vm migrates to dest host, if we don't touch the > keyboard, there will be no repeat / release event comes from host, > > 1) continue to repeat? but qemu no long gets press event from host > 2) stop to repeat? but qemu has not got the release event, auto-repeat > should continue in real keyboard in this condition. > > I prefect 2), because we need to emulate a release event in the > prepare stage of migrate. > > For implement 2), we should not load/restart the repeat_timer if > the migrated key_timer is already expired. > > --- > > Or just migrate the repeat_rate/repeat_period. We don't have > real request of auto-repeat in libvirt. > > This might be the best solution as Paolo said ;) > http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02207.html ping Anthony, Paolo I don't think we need to migrate the repeat timer/state. only need to migrate related setup (rate/delay) as in last patch. If you agree with this, I can merge those to patches together for avoiding to break the bisect. Amos. > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index cdb18e6..fdb9912 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void > *opaque) > { > PS2KbdState *s = opaque; > > - return s->repeat_period || s->repeat_delay; > + return s->repeat_period || s->repeat_delay || s->repeat_key || > s->repeat_timer; > +} > + > +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int > version_id) > +{ > + PS2KbdState *s = opaque; > + qemu_get_timer(f, s->repeat_timer); > + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + > + muldiv64(get_ticks_per_sec(), s->repeat_period, > 1000)); > + > + return 0; > } > > static bool ps2_keyboard_ledstate_needed(void *opaque) > @@ -638,9 +648,12 @@ static const VMStateDescription > vmstate_ps2_keyboard_repeatstate = { > .version_id = 3, > .minimum_version_id = 2, > .minimum_version_id_old = 2, > + .load_state_old = ps2_kbd_repeatstate_load, > .fields = (VMStateField[]) { > VMSTATE_INT32(repeat_period, PS2KbdState), > VMSTATE_INT32(repeat_delay, PS2KbdState), > + VMSTATE_INT32(repeat_key, PS2KbdState), > + VMSTATE_TIMER(repeat_timer, PS2KbdState), > VMSTATE_END_OF_LIST() > } > }; >
Am 31.05.2013 14:31, schrieb Amos Kong: > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index cdb18e6..fdb9912 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void > *opaque) > { > PS2KbdState *s = opaque; > > - return s->repeat_period || s->repeat_delay; > + return s->repeat_period || s->repeat_delay || s->repeat_key || > s->repeat_timer; > +} > + > +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int > version_id) > +{ > + PS2KbdState *s = opaque; > + qemu_get_timer(f, s->repeat_timer); > + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + > + muldiv64(get_ticks_per_sec(), s->repeat_period, > 1000)); > + > + return 0; > } > > static bool ps2_keyboard_ledstate_needed(void *opaque) > @@ -638,9 +648,12 @@ static const VMStateDescription > vmstate_ps2_keyboard_repeatstate = { > .version_id = 3, > .minimum_version_id = 2, > .minimum_version_id_old = 2, > + .load_state_old = ps2_kbd_repeatstate_load, > .fields = (VMStateField[]) { > VMSTATE_INT32(repeat_period, PS2KbdState), > VMSTATE_INT32(repeat_delay, PS2KbdState), > + VMSTATE_INT32(repeat_key, PS2KbdState), > + VMSTATE_TIMER(repeat_timer, PS2KbdState), You can't just add fields here, they'd need to be specific to a new version 4. Requested was to make it a subsection instead. Andreas > VMSTATE_END_OF_LIST() > } > }; >
Amos Kong <akong@redhat.com> writes: > On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: >> On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: >> > Amos Kong <akong@redhat.com> writes: >> > >> > > Guest driver sets repeat rate and delay time by KBD_CMD_SET_RATE, >> > > but ps2 backend doesn't process it and no auto-repeat implementation. >> > > This patch adds support of auto-repeat feature. The repeated events >> > > from host are ignored and re-implements ps2's auto-repeat. >> > > >> > > Guest ps2 driver sets autorepeat to fastest possible in reset, >> > > period: 250ms, delay: 33ms >> > > >> > > Tested by 'sendkey' monitor command. >> > > Tested by Linux & Windows guests with SDL, VNC, SPICE, GTK+ >> > > >> > > referenced: http://www.computer-engineering.org/ps2keyboard/ >> > > >> > > Signed-off-by: Amos Kong <akong@redhat.com> >> > > --- >> > > hw/input/ps2.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> > > 1 file changed, 41 insertions(+) >> > > >> > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> > > index 3412079..8adbb4a 100644 >> > > --- a/hw/input/ps2.c >> > > +++ b/hw/input/ps2.c >> > > @@ -94,6 +94,10 @@ typedef struct { >> > > int translate; >> > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ >> > > int ledstate; >> > > + int repeat_period; /* typematic period, ms */ >> > > + int repeat_delay; /* typematic delay, ms */ >> > > + int repeat_key; /* keycode to repeat */ >> > > + QEMUTimer *repeat_timer; >> > >> > This state needs to be migrated, no? I suspect it can/should be done >> > via a subsection too. >> >> It sounds only reasonable for 'sendkey' command. We want to repeat one >> key for 100 times, the key should be continaully repeated in the dest >> vm until it reaches to 100 times. >> >> For implement this, we should also migrate key_timer in ui/input.c, >> then it will send a release event to ps2 queue when the key_timer >> is expired. The bottom patch migrates repeat_timer & repeat_key, >> where should we save key_timer for migration? >> >> ---- >> >> For the VNC/SPICE/SDL/GTK+ windows, qemu gets repeated press events >> from host. After vm migrates to dest host, if we don't touch the >> keyboard, there will be no repeat / release event comes from host, >> >> 1) continue to repeat? but qemu no long gets press event from host >> 2) stop to repeat? but qemu has not got the release event, auto-repeat >> should continue in real keyboard in this condition. >> >> I prefect 2), because we need to emulate a release event in the >> prepare stage of migrate. >> >> For implement 2), we should not load/restart the repeat_timer if >> the migrated key_timer is already expired. >> >> --- >> >> Or just migrate the repeat_rate/repeat_period. We don't have >> real request of auto-repeat in libvirt. >> >> This might be the best solution as Paolo said ;) >> http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg02207.html > > ping Anthony, Paolo > > I don't think we need to migrate the repeat timer/state. > only need to migrate related setup (rate/delay) as in last patch. I'd feel more comfortable migrating all of the state. I suspect you're neglecting to consider seamless migration mode with Spice. In that case, there would be a release event I suspect (perhaps Gerd can chime in). Regards, Anthony Liguori > If you agree with this, I can merge those to patches together for > avoiding to break the bisect. > > Amos. > >> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> index cdb18e6..fdb9912 100644 >> --- a/hw/input/ps2.c >> +++ b/hw/input/ps2.c >> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void >> *opaque) >> { >> PS2KbdState *s = opaque; >> >> - return s->repeat_period || s->repeat_delay; >> + return s->repeat_period || s->repeat_delay || s->repeat_key || >> s->repeat_timer; >> +} >> + >> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int >> version_id) >> +{ >> + PS2KbdState *s = opaque; >> + qemu_get_timer(f, s->repeat_timer); >> + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + >> + muldiv64(get_ticks_per_sec(), s->repeat_period, >> 1000)); >> + >> + return 0; >> } >> >> static bool ps2_keyboard_ledstate_needed(void *opaque) >> @@ -638,9 +648,12 @@ static const VMStateDescription >> vmstate_ps2_keyboard_repeatstate = { >> .version_id = 3, >> .minimum_version_id = 2, >> .minimum_version_id_old = 2, >> + .load_state_old = ps2_kbd_repeatstate_load, >> .fields = (VMStateField[]) { >> VMSTATE_INT32(repeat_period, PS2KbdState), >> VMSTATE_INT32(repeat_delay, PS2KbdState), >> + VMSTATE_INT32(repeat_key, PS2KbdState), >> + VMSTATE_TIMER(repeat_timer, PS2KbdState), >> VMSTATE_END_OF_LIST() >> } >> }; >>
Il 13/06/2013 06:19, Andreas Färber ha scritto: > Am 31.05.2013 14:31, schrieb Amos Kong: >> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> index cdb18e6..fdb9912 100644 >> --- a/hw/input/ps2.c >> +++ b/hw/input/ps2.c >> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void >> *opaque) >> { >> PS2KbdState *s = opaque; >> >> - return s->repeat_period || s->repeat_delay; >> + return s->repeat_period || s->repeat_delay || s->repeat_key || >> s->repeat_timer; >> +} >> + >> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int >> version_id) >> +{ >> + PS2KbdState *s = opaque; >> + qemu_get_timer(f, s->repeat_timer); >> + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + >> + muldiv64(get_ticks_per_sec(), s->repeat_period, >> 1000)); >> + >> + return 0; >> } >> >> static bool ps2_keyboard_ledstate_needed(void *opaque) >> @@ -638,9 +648,12 @@ static const VMStateDescription >> vmstate_ps2_keyboard_repeatstate = { >> .version_id = 3, >> .minimum_version_id = 2, >> .minimum_version_id_old = 2, >> + .load_state_old = ps2_kbd_repeatstate_load, >> .fields = (VMStateField[]) { >> VMSTATE_INT32(repeat_period, PS2KbdState), >> VMSTATE_INT32(repeat_delay, PS2KbdState), >> + VMSTATE_INT32(repeat_key, PS2KbdState), >> + VMSTATE_TIMER(repeat_timer, PS2KbdState), > > You can't just add fields here, they'd need to be specific to a new > version 4. Requested was to make it a subsection instead. This is already a subsection, and this patch is just a proposal to be squashed in this series (which adds the subsection). But I think Amos is right and only the period/delay need to be migrated. Otherwise, you'll get an endless stream of repeats on the destination. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 13/06/2013 06:19, Andreas Färber ha scritto: >> Am 31.05.2013 14:31, schrieb Amos Kong: >>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >>> index cdb18e6..fdb9912 100644 >>> --- a/hw/input/ps2.c >>> +++ b/hw/input/ps2.c >>> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void >>> *opaque) >>> { >>> PS2KbdState *s = opaque; >>> >>> - return s->repeat_period || s->repeat_delay; >>> + return s->repeat_period || s->repeat_delay || s->repeat_key || >>> s->repeat_timer; >>> +} >>> + >>> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int >>> version_id) >>> +{ >>> + PS2KbdState *s = opaque; >>> + qemu_get_timer(f, s->repeat_timer); >>> + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + >>> + muldiv64(get_ticks_per_sec(), s->repeat_period, >>> 1000)); >>> + >>> + return 0; >>> } >>> >>> static bool ps2_keyboard_ledstate_needed(void *opaque) >>> @@ -638,9 +648,12 @@ static const VMStateDescription >>> vmstate_ps2_keyboard_repeatstate = { >>> .version_id = 3, >>> .minimum_version_id = 2, >>> .minimum_version_id_old = 2, >>> + .load_state_old = ps2_kbd_repeatstate_load, >>> .fields = (VMStateField[]) { >>> VMSTATE_INT32(repeat_period, PS2KbdState), >>> VMSTATE_INT32(repeat_delay, PS2KbdState), >>> + VMSTATE_INT32(repeat_key, PS2KbdState), >>> + VMSTATE_TIMER(repeat_timer, PS2KbdState), >> >> You can't just add fields here, they'd need to be specific to a new >> version 4. Requested was to make it a subsection instead. > > This is already a subsection, and this patch is just a proposal to be > squashed in this series (which adds the subsection). But I think Amos > is right and only the period/delay need to be migrated. Otherwise, > you'll get an endless stream of repeats on the destination. Not with seamless migration and spice. Even with a non-seamless VNC reconnect, if it happens behind the scenes the release would still be sent. Regards, Anthony Liguori > > Paolo
Il 13/06/2013 09:01, Anthony Liguori ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 13/06/2013 06:19, Andreas Färber ha scritto: >>> Am 31.05.2013 14:31, schrieb Amos Kong: >>>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >>>> index cdb18e6..fdb9912 100644 >>>> --- a/hw/input/ps2.c >>>> +++ b/hw/input/ps2.c >>>> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void >>>> *opaque) >>>> { >>>> PS2KbdState *s = opaque; >>>> >>>> - return s->repeat_period || s->repeat_delay; >>>> + return s->repeat_period || s->repeat_delay || s->repeat_key || >>>> s->repeat_timer; >>>> +} >>>> + >>>> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int >>>> version_id) >>>> +{ >>>> + PS2KbdState *s = opaque; >>>> + qemu_get_timer(f, s->repeat_timer); >>>> + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + >>>> + muldiv64(get_ticks_per_sec(), s->repeat_period, >>>> 1000)); >>>> + >>>> + return 0; >>>> } >>>> >>>> static bool ps2_keyboard_ledstate_needed(void *opaque) >>>> @@ -638,9 +648,12 @@ static const VMStateDescription >>>> vmstate_ps2_keyboard_repeatstate = { >>>> .version_id = 3, >>>> .minimum_version_id = 2, >>>> .minimum_version_id_old = 2, >>>> + .load_state_old = ps2_kbd_repeatstate_load, >>>> .fields = (VMStateField[]) { >>>> VMSTATE_INT32(repeat_period, PS2KbdState), >>>> VMSTATE_INT32(repeat_delay, PS2KbdState), >>>> + VMSTATE_INT32(repeat_key, PS2KbdState), >>>> + VMSTATE_TIMER(repeat_timer, PS2KbdState), >>> >>> You can't just add fields here, they'd need to be specific to a new >>> version 4. Requested was to make it a subsection instead. >> >> This is already a subsection, and this patch is just a proposal to be >> squashed in this series (which adds the subsection). But I think Amos >> is right and only the period/delay need to be migrated. Otherwise, >> you'll get an endless stream of repeats on the destination. > > Not with seamless migration and spice. Which BTW is broken right now in Fedora, though I didn't investigate who's the culprit. :) > Even with a non-seamless VNC reconnect, if it happens behind the scenes > the release would still be sent. Where is the code for that? Are SDL/GTK/whatnot covered as well? Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 13/06/2013 09:01, Anthony Liguori ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 13/06/2013 06:19, Andreas Färber ha scritto: >>>> Am 31.05.2013 14:31, schrieb Amos Kong: >>>>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >>>>> index cdb18e6..fdb9912 100644 >>>>> --- a/hw/input/ps2.c >>>>> +++ b/hw/input/ps2.c >>>>> @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void >>>>> *opaque) >>>>> { >>>>> PS2KbdState *s = opaque; >>>>> >>>>> - return s->repeat_period || s->repeat_delay; >>>>> + return s->repeat_period || s->repeat_delay || s->repeat_key || >>>>> s->repeat_timer; >>>>> +} >>>>> + >>>>> +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int >>>>> version_id) >>>>> +{ >>>>> + PS2KbdState *s = opaque; >>>>> + qemu_get_timer(f, s->repeat_timer); >>>>> + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + >>>>> + muldiv64(get_ticks_per_sec(), s->repeat_period, >>>>> 1000)); >>>>> + >>>>> + return 0; >>>>> } >>>>> >>>>> static bool ps2_keyboard_ledstate_needed(void *opaque) >>>>> @@ -638,9 +648,12 @@ static const VMStateDescription >>>>> vmstate_ps2_keyboard_repeatstate = { >>>>> .version_id = 3, >>>>> .minimum_version_id = 2, >>>>> .minimum_version_id_old = 2, >>>>> + .load_state_old = ps2_kbd_repeatstate_load, >>>>> .fields = (VMStateField[]) { >>>>> VMSTATE_INT32(repeat_period, PS2KbdState), >>>>> VMSTATE_INT32(repeat_delay, PS2KbdState), >>>>> + VMSTATE_INT32(repeat_key, PS2KbdState), >>>>> + VMSTATE_TIMER(repeat_timer, PS2KbdState), >>>> >>>> You can't just add fields here, they'd need to be specific to a new >>>> version 4. Requested was to make it a subsection instead. >>> >>> This is already a subsection, and this patch is just a proposal to be >>> squashed in this series (which adds the subsection). But I think Amos >>> is right and only the period/delay need to be migrated. Otherwise, >>> you'll get an endless stream of repeats on the destination. >> >> Not with seamless migration and spice. > > Which BTW is broken right now in Fedora, though I didn't investigate > who's the culprit. :) > >> Even with a non-seamless VNC reconnect, if it happens behind the scenes >> the release would still be sent. > > Where is the code for that? Are SDL/GTK/whatnot covered as well? What I mean by non-seamless migration is a management tool transparently reconnecting after live migration. I assume virt-manager does this but we certainly have management products that do this. The (remote) user see a blib in the session but if they had a key held down, then the release will certainly still be sent to the other end. Even with GTK/SDL, if a key was depressed it will continue to be depressed which will cause odd behavior with accelerators. At least with key repeat happening it's more obvious to the user that a key is stuck. Regards, Anthony Liguori > > Paolo
On Thu, Jun 13, 2013 at 01:28:14PM -0500, Anthony Liguori wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > Il 13/06/2013 09:01, Anthony Liguori ha scritto: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >>>>> static bool ps2_keyboard_ledstate_needed(void *opaque) > >>>>> @@ -638,9 +648,12 @@ static const VMStateDescription > >>>>> vmstate_ps2_keyboard_repeatstate = { > >>>>> .version_id = 3, > >>>>> .minimum_version_id = 2, > >>>>> .minimum_version_id_old = 2, > >>>>> + .load_state_old = ps2_kbd_repeatstate_load, > >>>>> .fields = (VMStateField[]) { > >>>>> VMSTATE_INT32(repeat_period, PS2KbdState), > >>>>> VMSTATE_INT32(repeat_delay, PS2KbdState), > >>>>> + VMSTATE_INT32(repeat_key, PS2KbdState), > >>>>> + VMSTATE_TIMER(repeat_timer, PS2KbdState), > >>>> > >>>> You can't just add fields here, they'd need to be specific to a new > >>>> version 4. Requested was to make it a subsection instead. > >>> > >>> This is already a subsection, and this patch is just a proposal to be > >>> squashed in this series (which adds the subsection). But I think Amos > >>> is right and only the period/delay need to be migrated. Otherwise, > >>> you'll get an endless stream of repeats on the destination. > > >> Not with seamless migration and spice. Auto-repeat is just a feature of keyboard hardware. ps2 keyboard will repeatedly emit events to guest. But for guest side, it could not repeatedly input/display one key if it only receive a press event and wait a long time. I used a timer to emulate hardware to repeatedly emit press events to guest. (1) IF we migrate the timer: (problem occured :( when we execute migrate/re-connect, and qemu doesn't receive the release event from host system. Auto-repeat will continua, even qemu doesn't get events from host system. (1) IF we DON'T migrate the repeat_timer: (everything is ok :) when we execute re-connection or migration, qemu can continually receive press events from host system if the key is pressed in the new vnc/spice side. The key _can_be auto-repeatedly inputed. SO we should not migrate the repeat_timer. The auto-repeated input after migration/re-connection should be decided by if the key is pressed in the new spice/vnc client. Amos. > > Which BTW is broken right now in Fedora, though I didn't investigate > > who's the culprit. :) > > > >> Even with a non-seamless VNC reconnect, if it happens behind the scenes > >> the release would still be sent. > > > Where is the code for that? Are SDL/GTK/whatnot covered as well? > > What I mean by non-seamless migration is a management tool transparently > reconnecting after live migration. > > I assume virt-manager does this but we certainly have management > products that do this. > > The (remote) user see a blib in the session but if they had a key held > down, then the release will certainly still be sent to the other end. > > Even with GTK/SDL, if a key was depressed it will continue to be > depressed which will cause odd behavior with accelerators. At least > with key repeat happening it's more obvious to the user that a key is > stuck. > > Regards, > > Anthony Liguori > > > > > Paolo
On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: > On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: > > Amos Kong <akong@redhat.com> writes: > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > > > index 3412079..8adbb4a 100644 > > > --- a/hw/input/ps2.c > > > +++ b/hw/input/ps2.c > > > @@ -94,6 +94,10 @@ typedef struct { > > > int translate; > > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ > > > int ledstate; > > > + int repeat_period; /* typematic period, ms */ > > > + int repeat_delay; /* typematic delay, ms */ > > > + int repeat_key; /* keycode to repeat */ > > > + QEMUTimer *repeat_timer; > > > > This state needs to be migrated, no? I suspect it can/should be done > > via a subsection too. > > It sounds only reasonable for 'sendkey' command. We want to repeat one > key for 100 times, the key should be continaully repeated in the dest > vm until it reaches to 100 times. > > For implement this, we should also migrate key_timer in ui/input.c, > then it will send a release event to ps2 queue when the key_timer > is expired. The bottom patch migrates repeat_timer & repeat_key, > where should we save key_timer for migration? Luiz, any suggestion about migrate the key_timer in ui/input.c? We need to migrate it, then sendkey can continually work in dest vm until the timer is expired. Thanks.
On Fri, 14 Jun 2013 13:46:41 +0800 Amos Kong <akong@redhat.com> wrote: > On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: > > On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: > > > Amos Kong <akong@redhat.com> writes: > > > > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > > > > index 3412079..8adbb4a 100644 > > > > --- a/hw/input/ps2.c > > > > +++ b/hw/input/ps2.c > > > > @@ -94,6 +94,10 @@ typedef struct { > > > > int translate; > > > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ > > > > int ledstate; > > > > + int repeat_period; /* typematic period, ms */ > > > > + int repeat_delay; /* typematic delay, ms */ > > > > + int repeat_key; /* keycode to repeat */ > > > > + QEMUTimer *repeat_timer; > > > > > > This state needs to be migrated, no? I suspect it can/should be done > > > via a subsection too. > > > > It sounds only reasonable for 'sendkey' command. We want to repeat one > > key for 100 times, the key should be continaully repeated in the dest > > vm until it reaches to 100 times. > > > > For implement this, we should also migrate key_timer in ui/input.c, > > then it will send a release event to ps2 queue when the key_timer > > is expired. The bottom patch migrates repeat_timer & repeat_key, > > where should we save key_timer for migration? > > Luiz, any suggestion about migrate the key_timer in ui/input.c? I don't have any. Maybe Markus or Juan can help (CC'ed). > > We need to migrate it, then sendkey can continually work in dest vm > until the timer is expired. > > Thanks.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 14 Jun 2013 13:46:41 +0800 > Amos Kong <akong@redhat.com> wrote: > >> On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: >> > On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: >> > > Amos Kong <akong@redhat.com> writes: >> >> >> > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> > > > index 3412079..8adbb4a 100644 >> > > > --- a/hw/input/ps2.c >> > > > +++ b/hw/input/ps2.c >> > > > @@ -94,6 +94,10 @@ typedef struct { >> > > > int translate; >> > > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ >> > > > int ledstate; >> > > > + int repeat_period; /* typematic period, ms */ >> > > > + int repeat_delay; /* typematic delay, ms */ >> > > > + int repeat_key; /* keycode to repeat */ >> > > > + QEMUTimer *repeat_timer; >> > > >> > > This state needs to be migrated, no? I suspect it can/should be done >> > > via a subsection too. >> > >> > It sounds only reasonable for 'sendkey' command. We want to repeat one >> > key for 100 times, the key should be continaully repeated in the dest >> > vm until it reaches to 100 times. >> > >> > For implement this, we should also migrate key_timer in ui/input.c, >> > then it will send a release event to ps2 queue when the key_timer >> > is expired. The bottom patch migrates repeat_timer & repeat_key, >> > where should we save key_timer for migration? >> >> Luiz, any suggestion about migrate the key_timer in ui/input.c? > > I don't have any. Maybe Markus or Juan can help (CC'ed). > >> >> We need to migrate it, then sendkey can continually work in dest vm >> until the timer is expired. I read the thread, but I'm not sure I have enough context for a sensible answer. Let me try to piece it together. On a real PC keyboard, key down generates that key's make code, key up generates the key's break code. If the key is typematic, the make code repeats while the key is down. First repeat is after a programmable delay, subsequent repeats happen at a programmable rate. Which keys are typematic is programmable in scan code set 3, but we don't implement the commands to do so. Oh well, the world is full of crappy clone keyboards, this is just one more. What problem are you trying to solve? Your cover letter mentions the sendkey command. Takes an array of keys and an optional hold-time, defaulting to 100ms. Aside: array of keys + a single hold time is a rotten design. Outside the scope of this patch. 100ms is well below the smallest typematic delay, so by default no repeat happens. But if you specify a sufficiently large hold-time, it arguably should. Is that the problem you're trying to fix? Why is this problem worth fixing? Does your patch affect anything but the sendkey command? I'm asking because I'm not at all sure injecting emulated repeat between the user's keyboard and the guest OS is a good idea. I'd expect the user's keyboard to repeat according to the user's wishes already, and I'm concerned a second repeat could mess up things.
On Wed, Jun 26, 2013 at 01:56:33PM +0200, Markus Armbruster wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Fri, 14 Jun 2013 13:46:41 +0800 > > Amos Kong <akong@redhat.com> wrote: > > > >> On Fri, May 31, 2013 at 08:31:17PM +0800, Amos Kong wrote: > >> > On Thu, May 30, 2013 at 11:48:46AM -0500, Anthony Liguori wrote: > >> > > Amos Kong <akong@redhat.com> writes: > >> > >> > >> > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > >> > > > index 3412079..8adbb4a 100644 > >> > > > --- a/hw/input/ps2.c > >> > > > +++ b/hw/input/ps2.c > >> > > > @@ -94,6 +94,10 @@ typedef struct { > >> > > > int translate; > >> > > > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ > >> > > > int ledstate; > >> > > > + int repeat_period; /* typematic period, ms */ > >> > > > + int repeat_delay; /* typematic delay, ms */ > >> > > > + int repeat_key; /* keycode to repeat */ > >> > > > + QEMUTimer *repeat_timer; > >> > > > >> > > This state needs to be migrated, no? I suspect it can/should be done > >> > > via a subsection too. > >> > > >> > It sounds only reasonable for 'sendkey' command. We want to repeat one > >> > key for 100 times, the key should be continaully repeated in the dest > >> > vm until it reaches to 100 times. > >> > > >> > For implement this, we should also migrate key_timer in ui/input.c, > >> > then it will send a release event to ps2 queue when the key_timer > >> > is expired. The bottom patch migrates repeat_timer & repeat_key, > >> > where should we save key_timer for migration? > >> > >> Luiz, any suggestion about migrate the key_timer in ui/input.c? > > > > I don't have any. Maybe Markus or Juan can help (CC'ed). > > > >> > >> We need to migrate it, then sendkey can continually work in dest vm > >> until the timer is expired. > > I read the thread, but I'm not sure I have enough context for a sensible > answer. Let me try to piece it together. > > On a real PC keyboard, key down generates that key's make code, key up > generates the key's break code. If the key is typematic, the make code > repeats while the key is down. First repeat is after a programmable > delay, subsequent repeats happen at a programmable rate. > > Which keys are typematic is programmable in scan code set 3, but we > don't implement the commands to do so. Oh well, the world is full of > crappy clone keyboards, this is just one more. > > What problem are you trying to solve? Your cover letter mentions the > sendkey command. Takes an array of keys and an optional hold-time, > defaulting to 100ms. > > Aside: array of keys + a single hold time is a rotten design. Outside > the scope of this patch. > > 100ms is well below the smallest typematic delay, so by default no > repeat happens. But if you specify a sufficiently large hold-time, it > arguably should. Is that the problem you're trying to fix? The events qemu gets from host userspace is auto-repeated (using host's repeat rate), it's ok to just pass the events to guest. What my patch is doing: 1) process the events from host userspace to the raw events from keyboard hardware, then implement the auto-repeat function in qemu, then it can be configured by guest os(delay/rate). 2) for the sendkey. I had planed to just sent repeated events from sendkey code by calling interface in ps2 code. The discussion wishes to implement real auto-repeat for ps2 emulated device. Actually it's not a urgent/necessary request. Host provided auto-repeat works, and we didn't have real application of holding key by sendkey command now. > Why is this problem worth fixing? > > Does your patch affect anything but the sendkey command? I'm asking > because I'm not at all sure injecting emulated repeat between the user's > keyboard and the guest OS is a good idea. I'd expect the user's > keyboard to repeat according to the user's wishes already, and I'm > concerned a second repeat could mess up things.
On 07/02/13 08:49, Amos Kong wrote: > Actually it's not a urgent/necessary request. Host provided > auto-repeat works, and we didn't have real application of holding > key by sendkey command now. So it's a solution for a non-existing problem ... Which guests do care about the hardware repeat rate in the first place? I know linux emulates it in software for a loooooooooong time (IIRC basically since usb keyboards exist). I wouldn't be surprised if other guests do the same. So I think we should simply not worry about it ;) cheers, Gerd
diff --git a/hw/input/ps2.c b/hw/input/ps2.c index cdb18e6..fdb9912 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -615,7 +615,17 @@ static bool ps2_keyboard_repeatstate_needed(void *opaque) { PS2KbdState *s = opaque; - return s->repeat_period || s->repeat_delay; + return s->repeat_period || s->repeat_delay || s->repeat_key || s->repeat_timer; +} + +static int ps2_kbd_repeatstate_load(QEMUFile *f, void *opaque, int version_id) +{ + PS2KbdState *s = opaque; + qemu_get_timer(f, s->repeat_timer); + qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) + + muldiv64(get_ticks_per_sec(), s->repeat_period, 1000)); +