[Qemu-devel] [PATCH v2 0/4] keyboard led status tracking

[Qemu-devel] [PATCH v2 0/4] keyboard led status tracking

[Gerd Hoffmann]

  Hi,

This patch series adds support for keyboard led status tracking to qemu.
The first patch adds the infrastructure to set the led status and to
register notify callbacks.  Patches #2 and #3 add support for the usb
and ps/2 keyboard drivers.  Patch #4 makes vnc use the led notification
to improve capslock and numlock status tracking.  spice will use the
kbd led notifiers too.

Changes in v2:
 - use QTAILQ for the handler list.
 - add a comment saying our constants are identical to ps/2 ones.
 - improve vnc patch description.

cheers,
  Gerd

[Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

Adds infrastructure for keyboard led status tracking to qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 console.h |   15 +++++++++++++++
 input.c   |   31 +++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/console.h b/console.h
index 916859d..71e8ff2 100644
--- a/console.h
+++ b/console.h
@@ -10,10 +10,16 @@
 #define MOUSE_EVENT_RBUTTON 0x02
 #define MOUSE_EVENT_MBUTTON 0x04
 
+/* identical to the ps/2 keyboard bits */
+#define QEMU_SCROLL_LOCK_LED (1 << 0)
+#define QEMU_NUM_LOCK_LED    (1 << 1)
+#define QEMU_CAPS_LOCK_LED   (1 << 2)
+
 /* in ms */
 #define GUI_REFRESH_INTERVAL 30
 
 typedef void QEMUPutKBDEvent(void *opaque, int keycode);
+typedef void QEMUPutLEDEvent(void *opaque, int ledstate);
 typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state);
 
 typedef struct QEMUPutMouseEntry {
@@ -26,13 +32,22 @@ typedef struct QEMUPutMouseEntry {
     struct QEMUPutMouseEntry *next;
 } QEMUPutMouseEntry;
 
+typedef struct QEMUPutLEDEntry {
+    QEMUPutLEDEvent *put_led;
+    void *opaque;
+    QTAILQ_ENTRY(QEMUPutLEDEntry) next;
+} QEMUPutLEDEntry;
+
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);
 void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry);
+QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque);
+void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 
 void kbd_put_keycode(int keycode);
+void kbd_put_ledstate(int ledstate);
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
 int kbd_mouse_is_absolute(void);
 
diff --git a/input.c b/input.c
index 955b9ab..baaa4c6 100644
--- a/input.c
+++ b/input.c
@@ -33,6 +33,7 @@ static QEMUPutKBDEvent *qemu_put_kbd_event;
 static void *qemu_put_kbd_event_opaque;
 static QEMUPutMouseEntry *qemu_put_mouse_event_head;
 static QEMUPutMouseEntry *qemu_put_mouse_event_current;
+static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
 
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
 {
@@ -102,6 +103,27 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
     qemu_free(entry);
 }
 
+QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func,
+                                            void *opaque)
+{
+    QEMUPutLEDEntry *s;
+
+    s = qemu_mallocz(sizeof(QEMUPutLEDEntry));
+
+    s->put_led = func;
+    s->opaque = opaque;
+    QTAILQ_INSERT_TAIL(&led_handlers, s, next);
+    return s;
+}
+
+void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
+{
+    if (entry == NULL)
+        return;
+    QTAILQ_REMOVE(&led_handlers, entry, next);
+    qemu_free(entry);
+}
+
 void kbd_put_keycode(int keycode)
 {
     if (qemu_put_kbd_event) {
@@ -109,6 +131,15 @@ void kbd_put_keycode(int keycode)
     }
 }
 
+void kbd_put_ledstate(int ledstate)
+{
+    QEMUPutLEDEntry *cursor;
+
+    QTAILQ_FOREACH(cursor, &led_handlers, next) {
+        cursor->put_led(cursor->opaque, ledstate);
+    }
+}
+
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
 {
     QEMUPutMouseEvent *mouse_event;
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
> Adds infrastructure for keyboard led status tracking to qemu.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>    
Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   console.h |   15 +++++++++++++++
>   input.c   |   31 +++++++++++++++++++++++++++++++
>   2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/console.h b/console.h
> index 916859d..71e8ff2 100644
> --- a/console.h
> +++ b/console.h
> @@ -10,10 +10,16 @@
>   #define MOUSE_EVENT_RBUTTON 0x02
>   #define MOUSE_EVENT_MBUTTON 0x04
>
> +/* identical to the ps/2 keyboard bits */
> +#define QEMU_SCROLL_LOCK_LED (1<<  0)
> +#define QEMU_NUM_LOCK_LED    (1<<  1)
> +#define QEMU_CAPS_LOCK_LED   (1<<  2)
> +
>   /* in ms */
>   #define GUI_REFRESH_INTERVAL 30
>
>   typedef void QEMUPutKBDEvent(void *opaque, int keycode);
> +typedef void QEMUPutLEDEvent(void *opaque, int ledstate);
>   typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state);
>
>   typedef struct QEMUPutMouseEntry {
> @@ -26,13 +32,22 @@ typedef struct QEMUPutMouseEntry {
>       struct QEMUPutMouseEntry *next;
>   } QEMUPutMouseEntry;
>
> +typedef struct QEMUPutLEDEntry {
> +    QEMUPutLEDEvent *put_led;
> +    void *opaque;
> +    QTAILQ_ENTRY(QEMUPutLEDEntry) next;
> +} QEMUPutLEDEntry;
> +
>   void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque);
>   QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
>                                                   void *opaque, int absolute,
>                                                   const char *name);
>   void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry);
> +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque);
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
>
>   void kbd_put_keycode(int keycode);
> +void kbd_put_ledstate(int ledstate);
>   void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
>   int kbd_mouse_is_absolute(void);
>
> diff --git a/input.c b/input.c
> index 955b9ab..baaa4c6 100644
> --- a/input.c
> +++ b/input.c
> @@ -33,6 +33,7 @@ static QEMUPutKBDEvent *qemu_put_kbd_event;
>   static void *qemu_put_kbd_event_opaque;
>   static QEMUPutMouseEntry *qemu_put_mouse_event_head;
>   static QEMUPutMouseEntry *qemu_put_mouse_event_current;
> +static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
>
>   void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
>   {
> @@ -102,6 +103,27 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
>       qemu_free(entry);
>   }
>
> +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func,
> +                                            void *opaque)
> +{
> +    QEMUPutLEDEntry *s;
> +
> +    s = qemu_mallocz(sizeof(QEMUPutLEDEntry));
> +
> +    s->put_led = func;
> +    s->opaque = opaque;
> +    QTAILQ_INSERT_TAIL(&led_handlers, s, next);
> +    return s;
> +}
> +
> +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry)
> +{
> +    if (entry == NULL)
> +        return;
> +    QTAILQ_REMOVE(&led_handlers, entry, next);
> +    qemu_free(entry);
> +}
> +
>   void kbd_put_keycode(int keycode)
>   {
>       if (qemu_put_kbd_event) {
> @@ -109,6 +131,15 @@ void kbd_put_keycode(int keycode)
>       }
>   }
>
> +void kbd_put_ledstate(int ledstate)
> +{
> +    QEMUPutLEDEntry *cursor;
> +
> +    QTAILQ_FOREACH(cursor,&led_handlers, next) {
> +        cursor->put_led(cursor->opaque, ledstate);
> +    }
> +}
> +
>   void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
>   {
>       QEMUPutMouseEvent *mouse_event;
>    

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Paul Brook]

> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
> > Adds infrastructure for keyboard led status tracking to qemu.
> >
> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> 
> Applied.  Thanks.

What about guests that use the capslock LED for something useful, instead of 
capslock?

Paul

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 03/09/2010 09:58 AM, Paul Brook wrote:
>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>      
>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>>        
>> Applied.  Thanks.
>>      
> What about guests that use the capslock LED for something useful, instead of
> capslock?
>    

For VNC, tracking modifier state is already a heuristic so this series 
just adjusts that heuristic in a way that is better for 99% of use-cases.

I agree that we have to be careful about how we use information like 
this particularly when it comes to issues of correctness, but I don't 
believe this is such an instance.


Regards,

Anthony Liguori

> Paul
>    

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

On 03/09/10 16:58, Paul Brook wrote:
>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>
>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>
>> Applied.  Thanks.
>
> What about guests that use the capslock LED for something useful, instead of
> capslock?

I offered write add a patch with a option to this off if you think it is 
not needed.  Never got an answer, so I didn't.  Patch is on the list now.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 03/09/2010 10:13 AM, Gerd Hoffmann wrote:
> On 03/09/10 16:58, Paul Brook wrote:
>>> On 02/26/2010 10:17 AM, Gerd Hoffmann wrote:
>>>> Adds infrastructure for keyboard led status tracking to qemu.
>>>>
>>>> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
>>>
>>> Applied.  Thanks.
>>
>> What about guests that use the capslock LED for something useful, 
>> instead of
>> capslock?
>
> I offered write add a patch with a option to this off if you think it 
> is not needed.  Never got an answer, so I didn't.  Patch is on the 
> list now.

If a person has a guest that twiddles with LEDs, what are the chances 
they are going to figure out that there's an option with VNC to not use 
LED status as part of the modifier tracking heuristics?

I don't think it's really useful.  Like I said in an earlier note, we're 
already dealing with a heuristics based algorithm because we're 
attempting to detect dangling modifier presses.  I'd rather see an 
option that disabled the whole thing verses something specific to the LEDs.

Regards,

Anthony Liguori

> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Gerd Hoffmann]

On 03/09/10 17:28, Anthony Liguori wrote:
> If a person has a guest that twiddles with LEDs, what are the chances
> they are going to figure out that there's an option with VNC to not use
> LED status as part of the modifier tracking heuristics?
>
> I don't think it's really useful. Like I said in an earlier note, we're
> already dealing with a heuristics based algorithm because we're
> attempting to detect dangling modifier presses. I'd rather see an option
> that disabled the whole thing verses something specific to the LEDs.

You are referring to the 
do-i-should-insert-extra-capslock/numlock-events logic in do_key_event() 
?  Well, we can turn off both, fine with me, after all they sort-of 
belong together.  Do you have an idea for a short 
'no-capslock-and-numlock-tracking-heuristics' option name?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v2 1/4] kbd leds: infrastructure

[Anthony Liguori]

On 03/09/2010 10:41 AM, Gerd Hoffmann wrote:
> On 03/09/10 17:28, Anthony Liguori wrote:
>> If a person has a guest that twiddles with LEDs, what are the chances
>> they are going to figure out that there's an option with VNC to not use
>> LED status as part of the modifier tracking heuristics?
>>
>> I don't think it's really useful. Like I said in an earlier note, we're
>> already dealing with a heuristics based algorithm because we're
>> attempting to detect dangling modifier presses. I'd rather see an option
>> that disabled the whole thing verses something specific to the LEDs.
>
> You are referring to the 
> do-i-should-insert-extra-capslock/numlock-events logic in do_key_event() ?

Yup.

>   Well, we can turn off both, fine with me, after all they sort-of 
> belong together.  Do you have an idea for a short 
> 'no-capslock-and-numlock-tracking-heuristics' option name?

Maybe 'toggle-key-tracking=on|off' with an appropriate documentation 
blurb.  I don't know, it doesn't sound like a good name to me.

Regards,

Anthony Liguori


>
> cheers,
>   Gerd
>

[Qemu-devel] [PATCH v2 2/4] kbd leds: ps/2 kbd

[Gerd Hoffmann]

Add led status notification support to the ps/2 kbd driver.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/ps2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/ps2.c b/hw/ps2.c
index 15a6650..f0b206a 100644
--- a/hw/ps2.c
+++ b/hw/ps2.c
@@ -185,6 +185,7 @@ static void ps2_reset_keyboard(PS2KbdState *s)
 {
     s->scan_enabled = 1;
     s->scancode_set = 2;
+    kbd_put_ledstate(0);
 }
 
 void ps2_write_keyboard(void *opaque, int val)
@@ -259,6 +260,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_queue(&s->common, KBD_REPLY_ACK);
         s->common.write_cmd = -1;
         break;
-- 
1.6.6.1

[Qemu-devel] [PATCH v2 3/4] kbd leds: usb kbd

[Gerd Hoffmann]

Add led status notification support to the usb kbd driver.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-hid.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index bf456bb..2e4e647 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -600,12 +600,20 @@ static int usb_keyboard_poll(USBKeyboardState *s, uint8_t *buf, int len)
 static int usb_keyboard_write(USBKeyboardState *s, uint8_t *buf, int len)
 {
     if (len > 0) {
+        int ledstate = 0;
         /* 0x01: Num Lock LED
          * 0x02: Caps Lock LED
          * 0x04: Scroll Lock LED
          * 0x08: Compose LED
          * 0x10: Kana LED */
         s->leds = buf[0];
+        if (s->leds & 0x04)
+            ledstate |= QEMU_SCROLL_LOCK_LED;
+        if (s->leds & 0x01)
+            ledstate |= QEMU_NUM_LOCK_LED;
+        if (s->leds & 0x02)
+            ledstate |= QEMU_CAPS_LOCK_LED;
+        kbd_put_ledstate(ledstate);
     }
     return 0;
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH v2 4/4] kbd keds: vnc

[Gerd Hoffmann]

Use led status notification support in vnc.

The qemu vnc server keeps track of the capslock and numlock states based
on the key presses it receives from the vnc client.  But this fails in
case the guests idea of the capslock and numlock state changes for other
reasons.  One case is guest reboot (+ keyboard reset).  Another case are
more recent windows versions which reset capslock state before
presenting the login screen.

Usually guests use the keyboard leds to signal the capslock and numlock
state to the user, so we can use this to better keep track of capslock
and numlock state in the qemu vnc server.

Also toggle the numlock and capslock states on keydown events (instead
of keyup).  Guests do the same.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vnc.c |   20 +++++++++++++++++++-
 vnc.h |    1 +
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/vnc.c b/vnc.c
index db34b0e..38690e2 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1111,6 +1111,7 @@ static void vnc_disconnect_finish(VncState *vs)
     }
 
     vnc_remove_timer(vs->vd);
+    qemu_remove_led_event_handler(vs->led);
     qemu_free(vs);
 }
 
@@ -1496,6 +1497,22 @@ static void press_key(VncState *vs, int keysym)
     kbd_put_keycode(keysym2scancode(vs->vd->kbd_layout, keysym) | 0x80);
 }
 
+static void kbd_leds(void *opaque, int ledstate)
+{
+    VncState *vs = opaque;
+    int caps, num;
+
+    caps = ledstate & QEMU_CAPS_LOCK_LED ? 1 : 0;
+    num  = ledstate & QEMU_NUM_LOCK_LED  ? 1 : 0;
+
+    if (vs->modifiers_state[0x3a] != caps) {
+        vs->modifiers_state[0x3a] = caps;
+    }
+    if (vs->modifiers_state[0x45] != num) {
+        vs->modifiers_state[0x45] = num;
+    }
+}
+
 static void do_key_event(VncState *vs, int down, int keycode, int sym)
 {
     /* QEMU console switch */
@@ -1521,7 +1538,7 @@ static void do_key_event(VncState *vs, int down, int keycode, int sym)
         break;
     case 0x3a:                        /* CapsLock */
     case 0x45:                        /* NumLock */
-        if (!down)
+        if (down)
             vs->modifiers_state[keycode] ^= 1;
         break;
     }
@@ -2407,6 +2424,7 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vnc_flush(vs);
     vnc_read_when(vs, protocol_version, 12);
     reset_keys(vs);
+    vs->led = qemu_add_led_event_handler(kbd_leds, vs);
 
     vnc_init_timer(vd);
 
diff --git a/vnc.h b/vnc.h
index ff9a699..0fc89bd 100644
--- a/vnc.h
+++ b/vnc.h
@@ -161,6 +161,7 @@ struct VncState
     size_t read_handler_expect;
     /* input */
     uint8_t modifiers_state[256];
+    QEMUPutLEDEntry *led;
 
     Buffer zlib;
     Buffer zlib_tmp;
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH v2 0/4] keyboard led status tracking

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> This patch series adds support for keyboard led status tracking to qemu.
> The first patch adds the infrastructure to set the led status and to
> register notify callbacks.  Patches #2 and #3 add support for the usb
> and ps/2 keyboard drivers.  Patch #4 makes vnc use the led notification
> to improve capslock and numlock status tracking.  spice will use the
> kbd led notifiers too.
>
> Changes in v2:
>  - use QTAILQ for the handler list.
>  - add a comment saying our constants are identical to ps/2 ones.
>  - improve vnc patch description.
>
> cheers,
>   Gerd

Acked-by: Juan Quintela <quintela@redhat.com>