[Qemu-devel] [PATCH v2 0/2] ps2 auto-repeat

[Qemu-devel] [PATCH v2 0/2] ps2 auto-repeat

[Amos Kong]

When I execute sendkey command, the hold parameter doesn't work,
no key is repeated in guest. Because ps2 doesn't support auto-repeat
feature. This patchset adds this support.

V2: ignore repeat events from host, re-implement in ps2 device.
    move repeat state to PS2KbdState, fix repeat timer.
    support migrate of repeat state

Amos Kong (2):
  ps2: add support of auto-repeat
  ps2: preserve repeat state on migration

 hw/input/ps2.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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;
 } PS2KbdState;
 
 typedef struct {
@@ -146,6 +150,15 @@ void ps2_queue(void *opaque, int b)
     s->update_irq(s->update_arg, 1);
 }
 
+static void repeat_ps2_queue(void *opaque)
+{
+    PS2KbdState *s = opaque;
+
+    qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) +
+                   muldiv64(get_ticks_per_sec(), s->repeat_period, 1000));
+    ps2_queue(&s->common, s->repeat_key);
+}
+
 /*
    keycode is expressed as follow:
    bit 7    - 0 key pressed, 1 = key released
@@ -167,7 +180,23 @@ static void ps2_put_keycode(void *opaque, int keycode)
             keycode = ps2_raw_keycode_set3[keycode & 0x7f];
         }
       }
+
+    /* ignore repeated events from host, re-implement it */
+    if (keycode == s->repeat_key) {
+        return;
+    }
     ps2_queue(&s->common, keycode);
+    qemu_del_timer(s->repeat_timer);
+
+    /* only auto-repeat press event */
+    if (!(keycode & 0x80)) {
+        s->repeat_key = keycode;
+        /* delay a while before first repeat */
+        qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) +
+                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
+    } else {
+        s->repeat_key = -1;
+    }
 }
 
 uint32_t ps2_read_data(void *opaque)
@@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
 
 void ps2_write_keyboard(void *opaque, int val)
 {
+    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
+    const short period[32] = { 33,  37,  42,  46,  50,  54,  58,  63,  67,  75,
+                83,  92, 100, 109, 116, 125, 133, 149, 167, 182, 200, 217, 232,
+                250, 270, 303, 333, 370, 400, 435, 470, 500 };
+    const short delay[4] = { 250, 500, 750, 1000 };
     PS2KbdState *s = (PS2KbdState *)opaque;
 
     switch(s->common.write_cmd) {
@@ -288,6 +322,10 @@ void ps2_write_keyboard(void *opaque, int val)
         s->common.write_cmd = -1;
         break;
     case KBD_CMD_SET_RATE:
+       /* Bit0-4 specifies the repeat rate */
+        s->repeat_period = period[val & 0x1f];
+       /* Bit5-6 bit specifies the delay time */
+        s->repeat_delay = delay[val >> 5 & 0x3];
         ps2_queue(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
@@ -536,6 +574,9 @@ static void ps2_kbd_reset(void *opaque)
     s->scan_enabled = 0;
     s->translate = 0;
     s->scancode_set = 0;
+    s->repeat_period = 92;
+    s->repeat_delay = 500;
+    s->repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
 }
 
 static void ps2_mouse_reset(void *opaque)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Anthony Liguori]

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.

Regards,

Anthony Liguori

>  } PS2KbdState;
>  
>  typedef struct {
> @@ -146,6 +150,15 @@ void ps2_queue(void *opaque, int b)
>      s->update_irq(s->update_arg, 1);
>  }
>  
> +static void repeat_ps2_queue(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                   muldiv64(get_ticks_per_sec(), s->repeat_period, 1000));
> +    ps2_queue(&s->common, s->repeat_key);
> +}
> +
>  /*
>     keycode is expressed as follow:
>     bit 7    - 0 key pressed, 1 = key released
> @@ -167,7 +180,23 @@ static void ps2_put_keycode(void *opaque, int keycode)
>              keycode = ps2_raw_keycode_set3[keycode & 0x7f];
>          }
>        }
> +
> +    /* ignore repeated events from host, re-implement it */
> +    if (keycode == s->repeat_key) {
> +        return;
> +    }
>      ps2_queue(&s->common, keycode);
> +    qemu_del_timer(s->repeat_timer);
> +
> +    /* only auto-repeat press event */
> +    if (!(keycode & 0x80)) {
> +        s->repeat_key = keycode;
> +        /* delay a while before first repeat */
> +        qemu_mod_timer(s->repeat_timer, qemu_get_clock_ns(vm_clock) +
> +                       muldiv64(get_ticks_per_sec(), s->repeat_delay, 1000));
> +    } else {
> +        s->repeat_key = -1;
> +    }
>  }
>  
>  uint32_t ps2_read_data(void *opaque)
> @@ -213,6 +242,11 @@ static void ps2_reset_keyboard(PS2KbdState *s)
>  
>  void ps2_write_keyboard(void *opaque, int val)
>  {
> +    /* repeat period/delay table from kernel (drivers/input/keyboard/atkbd.c) */
> +    const short period[32] = { 33,  37,  42,  46,  50,  54,  58,  63,  67,  75,
> +                83,  92, 100, 109, 116, 125, 133, 149, 167, 182, 200, 217, 232,
> +                250, 270, 303, 333, 370, 400, 435, 470, 500 };
> +    const short delay[4] = { 250, 500, 750, 1000 };
>      PS2KbdState *s = (PS2KbdState *)opaque;
>  
>      switch(s->common.write_cmd) {
> @@ -288,6 +322,10 @@ void ps2_write_keyboard(void *opaque, int val)
>          s->common.write_cmd = -1;
>          break;
>      case KBD_CMD_SET_RATE:
> +       /* Bit0-4 specifies the repeat rate */
> +        s->repeat_period = period[val & 0x1f];
> +       /* Bit5-6 bit specifies the delay time */
> +        s->repeat_delay = delay[val >> 5 & 0x3];
>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
>          break;
> @@ -536,6 +574,9 @@ static void ps2_kbd_reset(void *opaque)
>      s->scan_enabled = 0;
>      s->translate = 0;
>      s->scancode_set = 0;
> +    s->repeat_period = 92;
> +    s->repeat_delay = 500;
> +    s->repeat_timer = qemu_new_timer_ns(vm_clock, repeat_ps2_queue, s);
>  }
>  
>  static void ps2_mouse_reset(void *opaque)
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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



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()
     }
 };

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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()
>      }
>  };
> 

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Anthony Liguori]

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()
>>      }
>>  };
>> 

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Andreas Färber]

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()
>      }
>  };
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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.
-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Amos Kong]

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.

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 1/2] ps2: add support of auto-repeat

[Gerd Hoffmann]

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

[Qemu-devel] [PATCH v2 2/2] ps2: preserve repeat state on migration

[Amos Kong]

Use a subsection to migrate repeat state (repate period and first
delay).

Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/input/ps2.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 8adbb4a..cdb18e6 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -611,6 +611,13 @@ static const VMStateDescription vmstate_ps2_common = {
     }
 };
 
+static bool ps2_keyboard_repeatstate_needed(void *opaque)
+{
+    PS2KbdState *s = opaque;
+
+    return s->repeat_period || s->repeat_delay;
+}
+
 static bool ps2_keyboard_ledstate_needed(void *opaque)
 {
     PS2KbdState *s = opaque;
@@ -626,6 +633,18 @@ static int ps2_kbd_ledstate_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_ps2_keyboard_repeatstate = {
+    .name = "ps2kbd/repeatstate",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .fields      = (VMStateField[]) {
+        VMSTATE_INT32(repeat_period, PS2KbdState),
+        VMSTATE_INT32(repeat_delay, PS2KbdState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_ps2_keyboard_ledstate = {
     .name = "ps2kbd/ledstate",
     .version_id = 3,
@@ -665,6 +684,9 @@ static const VMStateDescription vmstate_ps2_keyboard = {
             .vmsd = &vmstate_ps2_keyboard_ledstate,
             .needed = ps2_keyboard_ledstate_needed,
         }, {
+            .vmsd = &vmstate_ps2_keyboard_repeatstate,
+            .needed = ps2_keyboard_repeatstate_needed,
+        }, {
             /* empty */
         }
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 2/2] ps2: preserve repeat state on migration

[Anthony Liguori]

Amos Kong <akong@redhat.com> writes:

> Use a subsection to migrate repeat state (repate period and first
> delay).
>
> Signed-off-by: Amos Kong <akong@redhat.com>

Ah,

You should fold this into 1/2.  Otherwise you break migration during
bisecting.

Regards,

Anthony Liguori

> ---
>  hw/input/ps2.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 8adbb4a..cdb18e6 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -611,6 +611,13 @@ static const VMStateDescription vmstate_ps2_common = {
>      }
>  };
>  
> +static bool ps2_keyboard_repeatstate_needed(void *opaque)
> +{
> +    PS2KbdState *s = opaque;
> +
> +    return s->repeat_period || s->repeat_delay;
> +}
> +
>  static bool ps2_keyboard_ledstate_needed(void *opaque)
>  {
>      PS2KbdState *s = opaque;
> @@ -626,6 +633,18 @@ static int ps2_kbd_ledstate_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static const VMStateDescription vmstate_ps2_keyboard_repeatstate = {
> +    .name = "ps2kbd/repeatstate",
> +    .version_id = 3,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_INT32(repeat_period, PS2KbdState),
> +        VMSTATE_INT32(repeat_delay, PS2KbdState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_ps2_keyboard_ledstate = {
>      .name = "ps2kbd/ledstate",
>      .version_id = 3,
> @@ -665,6 +684,9 @@ static const VMStateDescription vmstate_ps2_keyboard = {
>              .vmsd = &vmstate_ps2_keyboard_ledstate,
>              .needed = ps2_keyboard_ledstate_needed,
>          }, {
> +            .vmsd = &vmstate_ps2_keyboard_repeatstate,
> +            .needed = ps2_keyboard_repeatstate_needed,
> +        }, {
>              /* empty */
>          }
>      }
> -- 
> 1.8.1.4