[6/9] spice: add keyboard

Open keyboard channel.  Now you can type into the spice client and the
keyboard events are sent to your guest.  You'll need some other display
like vnc to actually see the guest responding to them though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs |    2 +-
 qemu-spice.h  |    1 +
 spice-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 spice.c       |    2 ++
 4 files changed, 61 insertions(+), 1 deletions(-)
 create mode 100644 spice-input.c

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Open keyboard channel.  Now you can type into the spice client and the
> keyboard events are sent to your guest.  You'll need some other display
> like vnc to actually see the guest responding to them though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.objs |    2 +-
>   qemu-spice.h  |    1 +
>   spice-input.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   spice.c       |    2 ++
>   4 files changed, 61 insertions(+), 1 deletions(-)
>   create mode 100644 spice-input.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 021067b..6ddb373 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -88,7 +88,7 @@ common-obj-y += pflib.o
>   common-obj-$(CONFIG_BRLAPI) += baum.o
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
>
> -common-obj-$(CONFIG_SPICE) += spice.o
> +common-obj-$(CONFIG_SPICE) += spice.o spice-input.o
>
>   audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>   audio-obj-$(CONFIG_SDL) += sdlaudio.o
> diff --git a/qemu-spice.h b/qemu-spice.h
> index 5597576..ceb3db2 100644
> --- a/qemu-spice.h
> +++ b/qemu-spice.h
> @@ -12,6 +12,7 @@ extern SpiceServer *spice_server;
>   extern int using_spice;
>
>   void qemu_spice_init(void);
> +void qemu_spice_input_init(void);
>
>   #else  /* CONFIG_SPICE */
>
> diff --git a/spice-input.c b/spice-input.c
> new file mode 100644
> index 0000000..e1014d7
> --- /dev/null
> +++ b/spice-input.c
> @@ -0,0 +1,57 @@
> +#include<stdlib.h>
> +#include<stdio.h>
> +#include<string.h>
>    

copyright and extra headers.

> +
> +#include<spice.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-spice.h"
> +#include "console.h"
> +
> +/* keyboard bits */
> +
> +typedef struct QemuSpiceKbd {
> +    SpiceKbdInstance sin;
> +    int ledstate;
> +} QemuSpiceKbd;
> +
> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag);
> +static uint8_t kbd_get_leds(SpiceKbdInstance *sin);
> +static void kbd_leds(void *opaque, int l);
> +
> +static const SpiceKbdInterface kbd_interface = {
> +    .base.type          = SPICE_INTERFACE_KEYBOARD,
> +    .base.description   = "qemu keyboard",
> +    .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
> +    .push_scan_freg     = kbd_push_key,
> +    .get_leds           = kbd_get_leds,
> +};
> +
> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
> +{
> +    kbd_put_keycode(frag);
> +}
>    

Instead of using this interface which includes multi-byte sequences, it 
would probably make sense to use the same compressed encoding that VNC 
uses and introduce a new function that takes this encoding type.  The 
advantage is that one call == one keycode so it's far less prone to 
goofiness.

> +
> +static uint8_t kbd_get_leds(SpiceKbdInstance *sin)
> +{
> +    QemuSpiceKbd *kbd = container_of(sin, QemuSpiceKbd, sin);
> +    return kbd->ledstate;
> +}
> +
> +static void kbd_leds(void *opaque, int ledstate)
> +{
> +    QemuSpiceKbd *kbd = opaque;
> +    kbd->ledstate = ledstate;
> +    spice_server_kbd_leds(&kbd->sin, ledstate);
> +}
>    

It's probably more robust in the long term to explicitly convert the 
ledstate to from the QEMU format to the libspice format even if they 
both happen to be the same.

Regards,

Anthony Liguori

> +void qemu_spice_input_init(void)
> +{
> +    QemuSpiceKbd *kbd;
> +
> +    kbd = qemu_mallocz(sizeof(*kbd));
> +    kbd->sin.base.sif =&kbd_interface.base;
> +    spice_server_add_interface(spice_server,&kbd->sin.base);
> +    qemu_add_led_event_handler(kbd_leds, kbd);
> +}
> diff --git a/spice.c b/spice.c
> index 50fa5ca..c763d52 100644
> --- a/spice.c
> +++ b/spice.c
> @@ -148,4 +148,6 @@ void qemu_spice_init(void)
>
>       spice_server_init(spice_server,&core_interface);
>       using_spice = 1;
> +
> +    qemu_spice_input_init();
>   }
>

>> +static const SpiceKbdInterface kbd_interface = {
>> + .base.type = SPICE_INTERFACE_KEYBOARD,
>> + .base.description = "qemu keyboard",
>> + .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
>> + .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
>> + .push_scan_freg = kbd_push_key,
>> + .get_leds = kbd_get_leds,
>> +};
>> +
>> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
>> +{
>> + kbd_put_keycode(frag);
>> +}
>
> Instead of using this interface which includes multi-byte sequences, it
> would probably make sense to use the same compressed encoding that VNC
> uses and introduce a new function that takes this encoding type. The
> advantage is that one call == one keycode so it's far less prone to
> goofiness.

Hmm?  Not sure what you want here ...

kbd_push_key is called by libspice which feeds us with a ps/2 data 
stream for the keyboard.  Yes, one of those x86-isms in spice.  Yes, the 
ps/2 data stream also travels over the wire, i.e. the spice client has 
to hop through loops to convert whatever it gets into a ps/2 data stream 
for us.

Luckily ps/2 data is exactly what qemu expects to get via 
kbd_put_keycode, so I can simply pass on what I get, and it is IMHO 
pretty pointless to do something else ...

> It's probably more robust in the long term to explicitly convert the
> ledstate to from the QEMU format to the libspice format even if they
> both happen to be the same.

Ok.

cheers,
   Gerd

On 08/20/2010 07:34 AM, Gerd Hoffmann wrote:
>
>>> +static const SpiceKbdInterface kbd_interface = {
>>> + .base.type = SPICE_INTERFACE_KEYBOARD,
>>> + .base.description = "qemu keyboard",
>>> + .base.major_version = SPICE_INTERFACE_KEYBOARD_MAJOR,
>>> + .base.minor_version = SPICE_INTERFACE_KEYBOARD_MINOR,
>>> + .push_scan_freg = kbd_push_key,
>>> + .get_leds = kbd_get_leds,
>>> +};
>>> +
>>> +static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag)
>>> +{
>>> + kbd_put_keycode(frag);
>>> +}
>>
>> Instead of using this interface which includes multi-byte sequences, it
>> would probably make sense to use the same compressed encoding that VNC
>> uses and introduce a new function that takes this encoding type. The
>> advantage is that one call == one keycode so it's far less prone to
>> goofiness.
>
> Hmm?  Not sure what you want here ...
>
> kbd_push_key is called by libspice which feeds us with a ps/2 data 
> stream for the keyboard.  Yes, one of those x86-isms in spice.  Yes, 
> the ps/2 data stream also travels over the wire, i.e. the spice client 
> has to hop through loops to convert whatever it gets into a ps/2 data 
> stream for us.
>
> Luckily ps/2 data is exactly what qemu expects to get via 
> kbd_put_keycode, so I can simply pass on what I get, and it is IMHO 
> pretty pointless to do something else ...

It's not actually ps/2 data.  It's AT scan codes plus an internal 
encoding to indicate press vs. release using the high bit.  
Additionally, some special keys are encoded with two calls to 
kbd_put_keycode using the 0xe0 prefix (the grey code).

That gets converted to ps/2 data by decoding the high bit and generating 
a special PS/2 keyboard sequence if necessary.  The grey sequence just 
happens to work because the logic in ps2_put_keycode is simple.  IOW, 
the ps/2 protocol looks like:

[0xe0,][0xf0,]raw_keycode

0xe0 indicates that the keycode is a "grey" code and 0xf0 indicates it's 
a key release event.  raw_keycode is a PS/2 raw keycode that has a 1-1 
mapping from AT scan codes.

To generate the possible sequences, we would do:

// normal key press; PS/2: at2raw(at_keycode)
kbd_put_keycode(at_keycode);

// grey key press; PS/2 0xe0, at2raw(at_keycode)
kbd_put_keycode(0xe0);
kbd_put_keycode(at_keycode);

// normal key release; PS/2 0xf0, at2raw(at_keycode)
kbd_put_keycode(at_keycode | 0x80);

// grey key release; PS/2 0xe0, 0xf0, at2raw(at_keycode)
kbd_put_keycode(0xe0);
kbd_put_keycode(at_keycode | 0x80);

In the VNC encoding, instead of using the high bit to represent the key 
down vs. key up event, we use separate messages for key down vs. key 
up.  We then use the high bit to encode whether the grey code is needed 
which let's us send keycodes in a single message with a fixed keycode 
size of 8 bits.

So what I'm proposing is that we modify kbd_put_keycode to also reflect 
this:

// normal keys
kbd_keycode_press(at_keycode);      // PS/2 at2raw(at_keycode)
kbd_keycode_release(at_keycode);   // PS/2 0xf0, at2raw(at_keycode)

// grey keys; PS/2 0xe0, at2raw(at_keycode)
kbd_keycode_press(0x80 | at_keycode);      // PS/2 0xe0, 0xf0, 
at2raw(at_keycode)
kbd_keycode_release(0x80 | at_keycode);   // PS/2 0xe0, 0xf0, 
at2raw(at_keycode)

If it's not already too late, I'd suggest making this the Spice protocol 
interface.  The current kbd_put_keycode interface is just a QEMU 
implementation detail and having different size byte sequences is a very 
awkward interface.

Regards,

Anthony Liguori

>> It's probably more robust in the long term to explicitly convert the
>> ledstate to from the QEMU format to the libspice format even if they
>> both happen to be the same.
>
> Ok.
>
> cheers,
>   Gerd
>

Hi,

> It's not actually ps/2 data. It's AT scan codes plus an internal
> encoding to indicate press vs. release using the high bit. Additionally,
> some special keys are encoded with two calls to kbd_put_keycode using
> the 0xe0 prefix (the grey code).

Wheee.  From a brief look at the code it seems this *is* the spice wire 
protocol.  One more place where spice uses knowledge about qemu 
internals.  Unfortunaly this one escaped my attention until now, so it 
didn't got fixed :-(

> So what I'm proposing is that we modify kbd_put_keycode to also reflect
> this:
>
> // normal keys
> kbd_keycode_press(at_keycode); // PS/2 at2raw(at_keycode)
> kbd_keycode_release(at_keycode); // PS/2 0xf0, at2raw(at_keycode)
>
> // grey keys; PS/2 0xe0, at2raw(at_keycode)
> kbd_keycode_press(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> at2raw(at_keycode)
> kbd_keycode_release(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> at2raw(at_keycode)
>
> If it's not already too late, I'd suggest making this the Spice protocol
> interface.

No.  I think for now I have to deal with the mess in case qemu decides 
to change the internal interface.  And when ever touching the spice wire 
protocol to fixup this mess I will *not* use AT keycodes.  Handling 
anything with extra internet / multimedia / whatever keys in a sane way 
is simply impossible with AT keycodes.  linux input layer key codes 
should do.  maybe usb hid is usable too, need to check.

cheers,
   Gerd

On Fri, Aug 20, 2010 at 03:56:52PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >It's not actually ps/2 data. It's AT scan codes plus an internal
> >encoding to indicate press vs. release using the high bit. Additionally,
> >some special keys are encoded with two calls to kbd_put_keycode using
> >the 0xe0 prefix (the grey code).
> 
> Wheee.  From a brief look at the code it seems this *is* the spice wire 
> protocol.  One more place where spice uses knowledge about qemu 
> internals.  Unfortunaly this one escaped my attention until now, so it 
> didn't got fixed :-(
> 
> >So what I'm proposing is that we modify kbd_put_keycode to also reflect
> >this:
> >
> >// normal keys
> >kbd_keycode_press(at_keycode); // PS/2 at2raw(at_keycode)
> >kbd_keycode_release(at_keycode); // PS/2 0xf0, at2raw(at_keycode)
> >
> >// grey keys; PS/2 0xe0, at2raw(at_keycode)
> >kbd_keycode_press(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> >at2raw(at_keycode)
> >kbd_keycode_release(0x80 | at_keycode); // PS/2 0xe0, 0xf0,
> >at2raw(at_keycode)
> >
> >If it's not already too late, I'd suggest making this the Spice protocol
> >interface.
> 
> No.  I think for now I have to deal with the mess in case qemu decides 
> to change the internal interface.  And when ever touching the spice wire 
> protocol to fixup this mess I will *not* use AT keycodes.  Handling 
> anything with extra internet / multimedia / whatever keys in a sane way 
> is simply impossible with AT keycodes.  linux input layer key codes 
> should do.  maybe usb hid is usable too, need to check.

AT (well XT) keycodes aren't that bad a choice, at least if you go for the
extended mapping used by the Linux keyboard driver. This supports pretty
much all of the internet/multimedia keys, AFAICT, more than USB hid
does (at least in the Linux USB HID driver). In order to properly support
the VNC keycode extension with GTK-VNC under  Xorg + Win32, OS-X and Linux,
as well as native OS-X & Win32, I've created a giant CSV mapping file for
all keycode sets that I've encountered so far.

The master mapping is from Linux keycodes to other sets:

  http://git.gnome.org/browse/gtk-vnc/tree/src/keymaps.csv

And a tool that can then create you C arrays for mapping between
arbitrary keycode sets in any direction (potentially lossy
of course, depending on choice of keycode sets):

  http://git.gnome.org/browse/gtk-vnc/tree/src/keymap-gen.pl

Regards,
Daniel

Hi,

> AT (well XT) keycodes aren't that bad a choice, at least if you go for the
> extended mapping used by the Linux keyboard driver.

Hmm, as far I know those extended mappings are not standardized.  Uses 
linux this just as internal representation?  Or can you actually feed a 
linux guest with them and expect it to work?  How about non-linux guests?

When using it as wire protocol guest compatibility doesn't matter that 
much though ...

> The master mapping is from Linux keycodes to other sets:
>
>    http://git.gnome.org/browse/gtk-vnc/tree/src/keymaps.csv
>
> And a tool that can then create you C arrays for mapping between
> arbitrary keycode sets in any direction (potentially lossy
> of course, depending on choice of keycode sets):
>
>    http://git.gnome.org/browse/gtk-vnc/tree/src/keymap-gen.pl

Nice.

cheers,
   Gerd

On Fri, Aug 20, 2010 at 04:49:20PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >AT (well XT) keycodes aren't that bad a choice, at least if you go for the
> >extended mapping used by the Linux keyboard driver.
> 
> Hmm, as far I know those extended mappings are not standardized.  Uses 
> linux this just as internal representation?  Or can you actually feed a 
> linux guest with them and expect it to work?  How about non-linux guests?

Internally Linux uses Linux keycodes throughout. The keyboard driver
(drivers/char/keyboard.c) has a table that maps Linux keycodes to XT 
keycodes when running in RAW mode that has entries for many of the
internet/multimedia keys. 

IIUC, Microsoft sets out standard XT mappings for internet, multimedia keys,
etc as part of WHQL tests for keyboards. There is a doc hiding somewhere
on the web that details these, but its location escapes me currently. 
I've not  checked whether the extended mappings used by the Linux 
keyboard driver in RAW mode, because it wasn't relevant for my immediate
needs - I just needed to know what the mapping was, not who standardized
it :-)

Regards,
Daniel