From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33952 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OmRUM-0005YO-CI for qemu-devel@nongnu.org; Fri, 20 Aug 2010 09:18:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OmRUL-0008Mj-5D for qemu-devel@nongnu.org; Fri, 20 Aug 2010 09:18:30 -0400 Received: from mail-iw0-f173.google.com ([209.85.214.173]:53111) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OmRUL-0008Md-0x for qemu-devel@nongnu.org; Fri, 20 Aug 2010 09:18:29 -0400 Received: by iwn8 with SMTP id 8so2357640iwn.4 for ; Fri, 20 Aug 2010 06:18:28 -0700 (PDT) Message-ID: <4C6E809D.9050408@codemonkey.ws> Date: Fri, 20 Aug 2010 08:18:21 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 6/9] spice: add keyboard References: <1282221625-29501-1-git-send-email-kraxel@redhat.com> <1282221625-29501-7-git-send-email-kraxel@redhat.com> <4C6D3E92.8060604@codemonkey.ws> <4C6E766A.2000809@redhat.com> In-Reply-To: <4C6E766A.2000809@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org 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 >