Re: [Qemu-devel] [PATCH v5 05/12] input: convert virtio-input-hid device to keycodemapdb

[Daniel P. Berrange]

On Tue, Sep 12, 2017 at 01:37:37PM +0100, Daniel P. Berrange wrote:
> Replace the keymap_qcode table with automatically generated
> tables.
> 
> Missing entries in keymap_qcode now fixed:
> 
>   Q_KEY_CODE_ASTERISK -> KEY_KPASTERISK
>   Q_KEY_CODE_KP_MULTIPLY -> KEY_KPASTERISK
>   Q_KEY_CODE_STOP -> KEY_STOP
>   Q_KEY_CODE_AGAIN -> KEY_AGAIN
>   Q_KEY_CODE_PROPS -> KEY_PROPS
>   Q_KEY_CODE_UNDO -> KEY_UNDO
>   Q_KEY_CODE_FRONT -> KEY_FRONT
>   Q_KEY_CODE_COPY -> KEY_COPY
>   Q_KEY_CODE_OPEN -> KEY_OPEN
>   Q_KEY_CODE_PASTE -> KEY_PASTE
>   Q_KEY_CODE_FIND -> KEY_FIND
>   Q_KEY_CODE_CUT -> KEY_CUT
>   Q_KEY_CODE_LF -> KEY_LINEFEED
>   Q_KEY_CODE_HELP -> KEY_HELP
>   Q_KEY_CODE_COMPOSE -> KEY_COMPOSE
>   Q_KEY_CODE_RO -> KEY_RO
>   Q_KEY_CODE_HIRAGANA -> KEY_HIRAGANA
>   Q_KEY_CODE_HENKAN -> KEY_HENKAN
>   Q_KEY_CODE_YEN -> KEY_YEN
>   Q_KEY_CODE_KP_COMMA -> KEY_KPCOMMA
>   Q_KEY_CODE_KP_EQUALS -> KEY_KPEQUAL
>   Q_KEY_CODE_POWER -> KEY_POWER
>   Q_KEY_CODE_SLEEP -> KEY_SLEEP
>   Q_KEY_CODE_WAKE -> KEY_WAKEUP
>   Q_KEY_CODE_AUDIONEXT -> KEY_NEXTSONG
>   Q_KEY_CODE_AUDIOPREV -> KEY_PREVIOUSSONG
>   Q_KEY_CODE_AUDIOSTOP -> KEY_STOPCD
>   Q_KEY_CODE_AUDIOPLAY -> KEY_PLAYPAUSE
>   Q_KEY_CODE_AUDIOMUTE -> KEY_MUTE
>   Q_KEY_CODE_VOLUMEUP -> KEY_VOLUMEUP
>   Q_KEY_CODE_VOLUMEDOWN -> KEY_VOLUMEDOWN
>   Q_KEY_CODE_MEDIASELECT -> KEY_MEDIA
>   Q_KEY_CODE_MAIL -> KEY_MAIL
>   Q_KEY_CODE_CALCULATOR -> KEY_CALC
>   Q_KEY_CODE_COMPUTER -> KEY_COMPUTER
>   Q_KEY_CODE_AC_HOME -> KEY_HOMEPAGE
>   Q_KEY_CODE_AC_BACK -> KEY_BACK
>   Q_KEY_CODE_AC_FORWARD -> KEY_FORWARD
>   Q_KEY_CODE_AC_REFRESH -> KEY_REFRESH
>   Q_KEY_CODE_AC_BOOKMARKS -> KEY_BOOKMARKS
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  hw/input/virtio-input-hid.c      | 136 +++-------------------------------
>  include/ui/input.h               |   3 +
>  ui/Makefile.objs                 |   1 +
>  ui/input-keymap-qcode-to-linux.c | 156 
> +++++++++++++++++++++++++++++++++++++++
>  ui/input-keymap.c                |   1 +
>  5 files changed, 170 insertions(+), 127 deletions(-)
>  create mode 100644 ui/input-keymap-qcode-to-linux.c
> 

> @@ -162,7 +43,7 @@ static const unsigned int axismap_abs[INPUT_AXIS__MAX] = {
>  /* ----------------------------------------------------------------- */
>  
>  static void virtio_input_key_config(VirtIOInput *vinput,
> -                                    const unsigned int *keymap,
> +                                    const unsigned short *keymap,
>                                      size_t mapsize)
>  {
>      virtio_input_config keys;

[snip]

> @@ -387,8 +269,8 @@ static void virtio_keyboard_init(Object *obj)
>  
>      vhid->handler = &virtio_keyboard_handler;
>      virtio_input_init_config(vinput, virtio_keyboard_config);
> -    virtio_input_key_config(vinput, keymap_qcode,
> -                            ARRAY_SIZE(keymap_qcode));
> +    virtio_input_key_config(vinput, qemu_input_map_qcode_to_linux,
> +                            qemu_input_map_qcode_to_linux_len);
>  }

Revisiting Gerd's comment on an earlier posting of this patch. Gerd
mentioned that this change affects guest ABI, but I'm wondering
whether we actually need to care about that or not.

IIUC, the array we pass in here is used to populate a bitmap that is
passed to the guest OS. The bitmap has one bit set for each Linux
keycode that the input driver is capable of sending.

>From what I can see the guest OS will read this bitmap when it first
probes the virtio device, and never updates it.  If so, then is it
actually a problem if we don't preserve the same map across save/
restore/migrate ?

If we removed a key from the bitmap in newer QEMU, that does not
appear a problem as we'd never send an event to the guest with
that key code.

If we added a key to the bitmap, we would potentially be sending
a key to the guest that we hadn't previously declared support for.
Either the guest OS will handle this normally, despite it not
being in the declared bitmap, or it will silently drop the event.
Neither behaviour seems problematic.

If we did need to preserve ABI for this bitmap, then it seems we're
going to create alot of work for ourselves, having to maintain
many historical copies of the keycode maps tables, adding a new
copy every time support for a new key is added in QEMU. I'd
rather we avoided that complexity & burden unless there's clear
bad behaviour by not maintaining stable keycode bitmap data across
QEMU releases.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|