Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard

[Anthony Liguori]

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