Convert hw backends to use keycodemapdb
v7 cover Convert hw backends to use keycodemapdb
This is a followup to
v1: https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02047.html
v2: https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02471.html
v3: https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02517.html
v4: https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg02708.html
v5: https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02950.html
v6: https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01995.html
This new series contains just the patches for converting the hardware backends to use keycodemapdb.
The main benefit is adding various missing key mappings that were previously accidentally left out
Changed in v7:
- Drop Xen patch as equiv already merged
- Add patch to fix keycodemap ordering in source
Daniel P. Berrange (4):
hw: convert ps2 device to keycodemapdb
hw: convert the escc device to keycodemapdb
ui: fix alphabetical ordering of keymaps
hw: convert virtio-input-hid device to keycodemapdb
Makefile | 6 +- hw/char/escc.c | 126 +------------- hw/input/ps2.c | 406 +------------------------------------------- hw/input/virtio-input-hid.c | 136 +-------------- include/ui/input.h | 18 +- ui/input-keymap.c | 6 +- 6 files changed, 46 insertions(+), 652 deletions(-)
Signature · 1 line
2.14.3
PATCH v7 1/4 hw: convert ps2 device to keycodemapdb
Replace the qcode_to_keycode_set1, qcode_to_keycode_set2, and qcode_to_keycode_set3 tables with automatically generated tables.
Missing entries in qcode_to_keycode_set1 now fixed:
- Q_KEY_CODE_SYSRQ -> 0x54
- Q_KEY_CODE_PRINT -> 0x54 (NB ignored due to special case)
- Q_KEY_CODE_AGAIN -> 0xe005
- Q_KEY_CODE_PROPS -> 0xe006
- Q_KEY_CODE_UNDO -> 0xe007
- Q_KEY_CODE_FRONT -> 0xe00c
- Q_KEY_CODE_COPY -> 0xe078
- Q_KEY_CODE_OPEN -> 0x64
- Q_KEY_CODE_PASTE -> 0x65
- Q_KEY_CODE_CUT -> 0xe03c
- Q_KEY_CODE_LF -> 0x5b
- Q_KEY_CODE_HELP -> 0xe075
- Q_KEY_CODE_COMPOSE -> 0xe05d
- Q_KEY_CODE_PAUSE -> 0xe046
- Q_KEY_CODE_KP_EQUALS -> 0x59
And some mistakes corrected:
- Q_KEY_CODE_HIRAGANA was mapped to 0x70 (Katakanahiragana) instead of of 0x77 (Hirigana)
- Q_KEY_CODE_MENU was incorrectly mapped to the compose scancode (0xe05d) and is now mapped to 0xe01e
- Q_KEY_CODE_FIND was mapped to 0xe065 (Search) instead of to 0xe041 (Find)
- Q_KEY_CODE_POWER, SLEEP & WAKE had 0x0e instead of 0xe0 as the prefix
Missing entries in qcode_to_keycode_set2 now fixed:
- Q_KEY_CODE_PRINT -> 0x7f (NB ignored due to special case)
- Q_KEY_CODE_COMPOSE -> 0xe02f
- Q_KEY_CODE_PAUSE -> 0xe077
- Q_KEY_CODE_KP_EQUALS -> 0x0f
And some mistakes corrected:
- Q_KEY_CODE_HIRAGANA was mapped to 0x13 (Katakanahiragana) instead of of 0x62 (Hirigana)
- Q_KEY_CODE_MENU was incorrectly mapped to the compose scancode (0xe02f) and is now not mapped
- Q_KEY_CODE_FIND was mapped to 0xe010 (Search) and is now not mapped.
- Q_KEY_CODE_POWER, SLEEP & WAKE had 0x0e instead of 0xe0 as the prefix
Missing entries in qcode_to_keycode_set3 now fixed:
- Q_KEY_CODE_ASTERISK -> 0x7e
- Q_KEY_CODE_SYSRQ -> 0x57
- Q_KEY_CODE_LESS -> 0x13
- Q_KEY_CODE_STOP -> 0x0a
- Q_KEY_CODE_AGAIN -> 0x0b
- Q_KEY_CODE_PROPS -> 0x0c
- Q_KEY_CODE_UNDO -> 0x10
- Q_KEY_CODE_COPY -> 0x18
- Q_KEY_CODE_OPEN -> 0x20
- Q_KEY_CODE_PASTE -> 0x28
- Q_KEY_CODE_FIND -> 0x30
- Q_KEY_CODE_CUT -> 0x38
- Q_KEY_CODE_HELP -> 0x09
- Q_KEY_CODE_COMPOSE -> 0x8d
- Q_KEY_CODE_AUDIONEXT -> 0x93
- Q_KEY_CODE_AUDIOPREV -> 0x94
- Q_KEY_CODE_AUDIOSTOP -> 0x98
- Q_KEY_CODE_AUDIOMUTE -> 0x9c
- Q_KEY_CODE_VOLUMEUP -> 0x95
- Q_KEY_CODE_VOLUMEDOWN -> 0x9d
- Q_KEY_CODE_CALCULATOR -> 0xa3
- Q_KEY_CODE_AC_HOME -> 0x97
And some mistakes corrected:
- Q_KEY_CODE_MENU was incorrectly mapped to the compose scancode (0x8d) and is now 0x91
--- Makefile | 3 + hw/input/ps2.c | 406 +---------------------------------------------------- include/ui/input.h | 9 ++ ui/input-keymap.c | 3 + 4 files changed, 22 insertions(+), 399 deletions(-)
Makefile +3 -0
@@ -233,6 +233,9 @@ KEYCODEMAP_CSV = $(SRC_PATH)/ui/keycodemapdb/data/keymaps.csv KEYCODEMAP_FILES = \ ui/input-keymap-linux-to-qcode.c \ + ui/input-keymap-qcode-to-atset1.c \ + ui/input-keymap-qcode-to-atset2.c \ + ui/input-keymap-qcode-to-atset3.c \ ui/input-keymap-qcode-to-qnum.c \ ui/input-keymap-qnum-to-qcode.c \ ui/input-keymap-qcode-to-linux.c \
hw/input/ps2.c +7 -399
436 lines not on this page · raw message
include/ui/input.h +9 -0
@@ -71,6 +71,15 @@ void qemu_remove_mouse_mode_change_notifier(Notifier *notify);extern const guint qemu_input_map_linux_to_qcode_len;extern const guint16 qemu_input_map_linux_to_qcode[]; +extern const guint qemu_input_map_qcode_to_atset1_len; +extern const guint16 qemu_input_map_qcode_to_atset1[]; + +extern const guint qemu_input_map_qcode_to_atset2_len; +extern const guint16 qemu_input_map_qcode_to_atset2[]; + +extern const guint qemu_input_map_qcode_to_atset3_len; +extern const guint16 qemu_input_map_qcode_to_atset3[]; +extern const guint qemu_input_map_qcode_to_qnum_len;extern const guint16 qemu_input_map_qcode_to_qnum[];
ui/input-keymap.c +3 -0
@@ -6,6 +6,9 @@#include "standard-headers/linux/input.h" #include "ui/input-keymap-linux-to-qcode.c" +#include "ui/input-keymap-qcode-to-atset1.c" +#include "ui/input-keymap-qcode-to-atset2.c" +#include "ui/input-keymap-qcode-to-atset3.c" #include "ui/input-keymap-qcode-to-qnum.c" #include "ui/input-keymap-qnum-to-qcode.c" #include "ui/input-keymap-qcode-to-linux.c"
Signature · 1 line
2.14.3
PATCH v7 2/4 hw: convert the escc device to keycodemapdb
In reply to Daniel P. BerrangeReplace the qcode_to_keycode table with automatically generated tables.
Missing entries in qcode_to_keycode now fixed:
- Q_KEY_CODE_KP_COMMA -> 0x2d
--- Makefile | 1 + hw/char/escc.c | 126 +++-------------------------------------------------- include/ui/input.h | 3 ++ ui/input-keymap.c | 1 + 4 files changed, 10 insertions(+), 121 deletions(-)
Makefile +1 -0
@@ -237,6 +237,7 @@ KEYCODEMAP_FILES = \ ui/input-keymap-qcode-to-atset2.c \ ui/input-keymap-qcode-to-atset3.c \ ui/input-keymap-qcode-to-qnum.c \ + ui/input-keymap-qcode-to-sun.c \ ui/input-keymap-qnum-to-qcode.c \ ui/input-keymap-qcode-to-linux.c \ $(NULL)
hw/char/escc.c +5 -121
138 lines not on this page · raw message
include/ui/input.h +3 -0
@@ -83,6 +83,9 @@ extern const guint16 qemu_input_map_qcode_to_atset3[];extern const guint qemu_input_map_qcode_to_qnum_len;extern const guint16 qemu_input_map_qcode_to_qnum[]; +extern const guint qemu_input_map_qcode_to_sun_len; +extern const guint16 qemu_input_map_qcode_to_sun[]; +extern const guint qemu_input_map_qnum_to_qcode_len;extern const guint16 qemu_input_map_qnum_to_qcode[];
ui/input-keymap.c +1 -0
@@ -10,6 +10,7 @@#include "ui/input-keymap-qcode-to-atset2.c" #include "ui/input-keymap-qcode-to-atset3.c" #include "ui/input-keymap-qcode-to-qnum.c" +#include "ui/input-keymap-qcode-to-sun.c" #include "ui/input-keymap-qnum-to-qcode.c" #include "ui/input-keymap-qcode-to-linux.c"
Signature · 1 line
2.14.3
PATCH v7 3/4 ui: fix alphabetical ordering of keymaps
In reply to Daniel P. BerrangeThe qcode-to-linux keymaps was accidentally added in the wrong place by
commit de80d78594b4c3767a12d8d42debcf12cbf85a5b
Author: Owen Smith <[email protected]>
Date: Fri Nov 3 11:56:28 2017 +0000
ui: generate qcode to linux mappings
breaking the alphabetical ordering of keymaps
--- Makefile | 2 +- include/ui/input.h | 6 +++--- ui/input-keymap.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)
Makefile +1 -1
@@ -236,10 +236,10 @@ KEYCODEMAP_FILES = \ ui/input-keymap-qcode-to-atset1.c \ ui/input-keymap-qcode-to-atset2.c \ ui/input-keymap-qcode-to-atset3.c \ + ui/input-keymap-qcode-to-linux.c \ ui/input-keymap-qcode-to-qnum.c \ ui/input-keymap-qcode-to-sun.c \ ui/input-keymap-qnum-to-qcode.c \ - ui/input-keymap-qcode-to-linux.c \ $(NULL) GENERATED_FILES += $(KEYCODEMAP_FILES)
include/ui/input.h +3 -3
@@ -80,6 +80,9 @@ extern const guint16 qemu_input_map_qcode_to_atset2[];extern const guint qemu_input_map_qcode_to_atset3_len;extern const guint16 qemu_input_map_qcode_to_atset3[]; +extern const guint qemu_input_map_qcode_to_linux_len; +extern const guint16 qemu_input_map_qcode_to_linux[]; +extern const guint qemu_input_map_qcode_to_qnum_len;extern const guint16 qemu_input_map_qcode_to_qnum[];
@@ -89,7 +92,4 @@ extern const guint16 qemu_input_map_qcode_to_sun[];extern const guint qemu_input_map_qnum_to_qcode_len;extern const guint16 qemu_input_map_qnum_to_qcode[]; -extern const guint qemu_input_map_qcode_to_linux_len; -extern const guint16 qemu_input_map_qcode_to_linux[]; -#endif /* INPUT_H */
ui/input-keymap.c +1 -1
@@ -9,10 +9,10 @@#include "ui/input-keymap-qcode-to-atset1.c" #include "ui/input-keymap-qcode-to-atset2.c" #include "ui/input-keymap-qcode-to-atset3.c" +#include "ui/input-keymap-qcode-to-linux.c" #include "ui/input-keymap-qcode-to-qnum.c" #include "ui/input-keymap-qcode-to-sun.c" #include "ui/input-keymap-qnum-to-qcode.c" -#include "ui/input-keymap-qcode-to-linux.c" int qemu_input_linux_to_qcode (unsigned int lnx) {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 3/4] ui: fix alphabetical ordering of keymaps
Quoted 7 lines · Daniel P. Berrange
The qcode-to-linux keymaps was accidentally added in the wrong place by
commit
de80d78594b4c3767a12d8d42debcf12cbf85a5b
Author: Owen Smith <[email protected]>
Date: Fri Nov 3 11:56:28 2017 +0000
Quoted 5 lines
ui: generate qcode to linux mappings
breaking the alphabetical ordering of keymaps
Signed-off-by Daniel P. Berrange <[email protected]>
Quoted 61 lines
--- Makefile | 2 +- include/ui/input.h | 6 +++--- ui/input-keymap.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)Makefile +1
-1@@ -236,10 +236,10 @@ KEYCODEMAP_FILES = \ ui/input-keymap-qcode-to-atset1.c \ ui/input-keymap-qcode-to-atset2.c \ ui/input-keymap-qcode-to-atset3.c \ + ui/input-keymap-qcode-to-linux.c \ ui/input-keymap-qcode-to-qnum.c \ ui/input-keymap-qcode-to-sun.c \ ui/input-keymap-qnum-to-qcode.c \ - ui/input-keymap-qcode-to-linux.c \ $(NULL) GENERATED_FILES += $(KEYCODEMAP_FILES)include/ui/input.h +3
-3@@ -80,6 +80,9 @@ extern const guint16 qemu_input_map_qcode_to_atset2[];extern const guint qemu_input_map_qcode_to_atset3_len;extern const guint16 qemu_input_map_qcode_to_atset3[]; +extern const guint qemu_input_map_qcode_to_linux_len; +extern const guint16 qemu_input_map_qcode_to_linux[]; +extern const guint qemu_input_map_qcode_to_qnum_len;extern const guint16 qemu_input_map_qcode_to_qnum[];@@ -89,7 +92,4 @@ extern const guint16 qemu_input_map_qcode_to_sun[];extern const guint qemu_input_map_qnum_to_qcode_len;extern const guint16 qemu_input_map_qnum_to_qcode[]; -extern const guint qemu_input_map_qcode_to_linux_len; -extern const guint16 qemu_input_map_qcode_to_linux[]; -#endif /* INPUT_H */ ui/input-keymap.c +1
-1@@ -9,10 +9,10 @@#include "ui/input-keymap-qcode-to-atset1.c" #include "ui/input-keymap-qcode-to-atset2.c" #include "ui/input-keymap-qcode-to-atset3.c" +#include "ui/input-keymap-qcode-to-linux.c" #include "ui/input-keymap-qcode-to-qnum.c" #include "ui/input-keymap-qcode-to-sun.c" #include "ui/input-keymap-qnum-to-qcode.c" -#include "ui/input-keymap-qcode-to-linux.c" int qemu_input_linux_to_qcode (unsigned int lnx) {
PATCH v7 4/4 hw: convert virtio-input-hid device to keycodemapdb
In reply to Daniel P. BerrangeReplace 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
NB, the virtio-input device reports a bitmask to the guest driver that has a bit set for each Linux keycode that the host is able to send to the guest.
Thus by adding these extra key mappings we are technically changing the host<->guest ABI. This would also happen any time we defined new mappings for QEMU keycodes in future.
When a keycode is removed from the list of possible keycodes that host can send to the guest, it means that the guest OS will think it is possible to receive a key that in pratice can never be generated, which is harmless.
When a keycode is added to the list of possible keycodes that the host can send to the guest, it means that the guest OS can see an unexpected event. The Linux virtio_input.c driver code simply forwards this event to the input_event() method in the Linux input subsystem. This in turn calls input_handle_event(), which then calls input_get_disposition(). This method checks if the input event is present in the permitted keys bitmap, and if not returns INPUT_IGNORE_EVENT. Thus the unexpected event will get dropped, which is harmless.
If the guest OS reboots, or otherwise re-initializes the virt-input device, it will read the new keycode bitmap. No matter how many keys are defined, the config space has a fixed 128 byte bitmap. There is, however, a size field defiend which says how many bytes in the bitmap are used. So the guest OS reads the size of the bitmap, and then it reads the data from bitmap upto the designated size. So if the guest OS re-initializes at precisely the time that QEMU is migrated across versions, in the worst case, it could conceivably read the old size field, but then get the newly updated bitmap. If a key were added this is harmless, since it simply means it may not process the newly added key. If a key were removed, then it could be readnig a byte from the bitmap that was not initialized. Fortunately QEMU always memsets() the entire bitmap to 0, prior to setting keybits. Thus the guest OS will simply read zeros, which is again harmless.
Based on this analysis, it is believed that there is no need to preserve the virtio-input-hid keymaps across migration, as the host<->guest ABI change is harmless and self-resolving at time of guest reboot.
NB, this behaviour should perhaps be formalized in the virtio-input spec to declare how guest OS drivers should be written to be robust in their handling of the potentially changable key bitmaps.
--- hw/input/virtio-input-hid.c | 136 +++----------------------------------------- 1 file changed, 9 insertions(+), 127 deletions(-)
hw/input/virtio-input-hid.c +9 -127
@@ -22,126 +22,7 @@/* ----------------------------------------------------------------- */ -static const unsigned int keymap_qcode[Q_KEY_CODE__MAX ] = { - [Q_KEY_CODE_ESC ] =KEY_ESC , - [Q_KEY_CODE_1 ] =KEY_1 , - [Q_KEY_CODE_2 ] =KEY_2 , - [Q_KEY_CODE_3 ] =KEY_3 , - [Q_KEY_CODE_4 ] =KEY_4 , - [Q_KEY_CODE_5 ] =KEY_5 , - [Q_KEY_CODE_6 ] =KEY_6 , - [Q_KEY_CODE_7 ] =KEY_7 , - [Q_KEY_CODE_8 ] =KEY_8 , - [Q_KEY_CODE_9 ] =KEY_9 , - [Q_KEY_CODE_0 ] =KEY_0 , - [Q_KEY_CODE_MINUS ] =KEY_MINUS , - [Q_KEY_CODE_EQUAL ] =KEY_EQUAL , - [Q_KEY_CODE_BACKSPACE ] =KEY_BACKSPACE , - - [Q_KEY_CODE_TAB ] =KEY_TAB , - [Q_KEY_CODE_Q ] =KEY_Q , - [Q_KEY_CODE_W ] =KEY_W , - [Q_KEY_CODE_E ] =KEY_E , - [Q_KEY_CODE_R ] =KEY_R , - [Q_KEY_CODE_T ] =KEY_T , - [Q_KEY_CODE_Y ] =KEY_Y , - [Q_KEY_CODE_U ] =KEY_U , - [Q_KEY_CODE_I ] =KEY_I , - [Q_KEY_CODE_O ] =KEY_O , - [Q_KEY_CODE_P ] =KEY_P , - [Q_KEY_CODE_BRACKET_LEFT ] =KEY_LEFTBRACE , - [Q_KEY_CODE_BRACKET_RIGHT ] =KEY_RIGHTBRACE , - [Q_KEY_CODE_RET ] =KEY_ENTER , - - [Q_KEY_CODE_CTRL ] =KEY_LEFTCTRL , - [Q_KEY_CODE_A ] =KEY_A , - [Q_KEY_CODE_S ] =KEY_S , - [Q_KEY_CODE_D ] =KEY_D , - [Q_KEY_CODE_F ] =KEY_F , - [Q_KEY_CODE_G ] =KEY_G , - [Q_KEY_CODE_H ] =KEY_H , - [Q_KEY_CODE_J ] =KEY_J , - [Q_KEY_CODE_K ] =KEY_K , - [Q_KEY_CODE_L ] =KEY_L , - [Q_KEY_CODE_SEMICOLON ] =KEY_SEMICOLON , - [Q_KEY_CODE_APOSTROPHE ] =KEY_APOSTROPHE , - [Q_KEY_CODE_GRAVE_ACCENT ] =KEY_GRAVE , - - [Q_KEY_CODE_SHIFT ] =KEY_LEFTSHIFT , - [Q_KEY_CODE_BACKSLASH ] =KEY_BACKSLASH , - [Q_KEY_CODE_LESS ] =KEY_102ND , - [Q_KEY_CODE_Z ] =KEY_Z , - [Q_KEY_CODE_X ] =KEY_X , - [Q_KEY_CODE_C ] =KEY_C , - [Q_KEY_CODE_V ] =KEY_V , - [Q_KEY_CODE_B ] =KEY_B , - [Q_KEY_CODE_N ] =KEY_N , - [Q_KEY_CODE_M ] =KEY_M , - [Q_KEY_CODE_COMMA ] =KEY_COMMA , - [Q_KEY_CODE_DOT ] =KEY_DOT , - [Q_KEY_CODE_SLASH ] =KEY_SLASH , - [Q_KEY_CODE_SHIFT_R ] =KEY_RIGHTSHIFT , - - [Q_KEY_CODE_ALT ] =KEY_LEFTALT , - [Q_KEY_CODE_SPC ] =KEY_SPACE , - [Q_KEY_CODE_CAPS_LOCK ] =KEY_CAPSLOCK , - - [Q_KEY_CODE_F1 ] =KEY_F1 , - [Q_KEY_CODE_F2 ] =KEY_F2 , - [Q_KEY_CODE_F3 ] =KEY_F3 , - [Q_KEY_CODE_F4 ] =KEY_F4 , - [Q_KEY_CODE_F5 ] =KEY_F5 , - [Q_KEY_CODE_F6 ] =KEY_F6 , - [Q_KEY_CODE_F7 ] =KEY_F7 , - [Q_KEY_CODE_F8 ] =KEY_F8 , - [Q_KEY_CODE_F9 ] =KEY_F9 , - [Q_KEY_CODE_F10 ] =KEY_F10 , - [Q_KEY_CODE_NUM_LOCK ] =KEY_NUMLOCK , - [Q_KEY_CODE_SCROLL_LOCK ] =KEY_SCROLLLOCK , - - [Q_KEY_CODE_KP_0 ] =KEY_KP0 , - [Q_KEY_CODE_KP_1 ] =KEY_KP1 , - [Q_KEY_CODE_KP_2 ] =KEY_KP2 , - [Q_KEY_CODE_KP_3 ] =KEY_KP3 , - [Q_KEY_CODE_KP_4 ] =KEY_KP4 , - [Q_KEY_CODE_KP_5 ] =KEY_KP5 , - [Q_KEY_CODE_KP_6 ] =KEY_KP6 , - [Q_KEY_CODE_KP_7 ] =KEY_KP7 , - [Q_KEY_CODE_KP_8 ] =KEY_KP8 , - [Q_KEY_CODE_KP_9 ] =KEY_KP9 , - [Q_KEY_CODE_KP_SUBTRACT ] =KEY_KPMINUS , - [Q_KEY_CODE_KP_ADD ] =KEY_KPPLUS , - [Q_KEY_CODE_KP_DECIMAL ] =KEY_KPDOT , - [Q_KEY_CODE_KP_ENTER ] =KEY_KPENTER , - [Q_KEY_CODE_KP_DIVIDE ] =KEY_KPSLASH , - [Q_KEY_CODE_KP_MULTIPLY ] =KEY_KPASTERISK , - - [Q_KEY_CODE_F11 ] =KEY_F11 , - [Q_KEY_CODE_F12 ] =KEY_F12 , - - [Q_KEY_CODE_CTRL_R ] =KEY_RIGHTCTRL , - [Q_KEY_CODE_SYSRQ ] =KEY_SYSRQ , - [Q_KEY_CODE_PRINT ] =KEY_SYSRQ , - [Q_KEY_CODE_PAUSE ] =KEY_PAUSE , - [Q_KEY_CODE_ALT_R ] =KEY_RIGHTALT , - - [Q_KEY_CODE_HOME ] =KEY_HOME , - [Q_KEY_CODE_UP ] =KEY_UP , - [Q_KEY_CODE_PGUP ] =KEY_PAGEUP , - [Q_KEY_CODE_LEFT ] =KEY_LEFT , - [Q_KEY_CODE_RIGHT ] =KEY_RIGHT , - [Q_KEY_CODE_END ] =KEY_END , - [Q_KEY_CODE_DOWN ] =KEY_DOWN , - [Q_KEY_CODE_PGDN ] =KEY_PAGEDOWN , - [Q_KEY_CODE_INSERT ] =KEY_INSERT , - [Q_KEY_CODE_DELETE ] =KEY_DELETE , - - [Q_KEY_CODE_META_L ] =KEY_LEFTMETA , - [Q_KEY_CODE_META_R ] =KEY_RIGHTMETA , - [Q_KEY_CODE_MENU ] =KEY_MENU , -}; - -static const unsigned int keymap_button[INPUT_BUTTON__MAX ] = { +static const unsigned short keymap_button[INPUT_BUTTON__MAX ] = { [INPUT_BUTTON_LEFT ] =BTN_LEFT , [INPUT_BUTTON_RIGHT ] =BTN_RIGHT , [INPUT_BUTTON_MIDDLE ] =BTN_MIDDLE ,
@@ -149,12 +30,12 @@ static const unsigned int keymap_button[INPUT_BUTTON__MAX] = { [INPUT_BUTTON_WHEEL_DOWN] = BTN_GEAR_DOWN, }; -static const unsigned int axismap_rel[INPUT_AXIS__MAX ] = { +static const unsigned short axismap_rel[INPUT_AXIS__MAX ] = { [INPUT_AXIS_X ] =REL_X , [INPUT_AXIS_Y ] =REL_Y , }; -static const unsigned int axismap_abs[INPUT_AXIS__MAX ] = { +static const unsigned short axismap_abs[INPUT_AXIS__MAX ] = { [INPUT_AXIS_X ] =ABS_X , [INPUT_AXIS_Y ] =ABS_Y , };
@@ -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;
@@ -202,9 +83,10 @@ static void virtio_input_handle_event(DeviceState *dev, QemuConsole *src,case INPUT_EVENT_KIND_KEY : key = evt->u.key.data; qcode =qemu_input_key_value_to_qcode (key->key); -if (qcode && keymap_qcode[qcode]) { +if (qcode < qemu_input_map_qcode_to_linux_len && + qemu_input_map_qcode_to_linux[qcode]) { event.type =cpu_to_le16 (EV_KEY ); - event.code =cpu_to_le16 (keymap_qcode[qcode]); + event.code =cpu_to_le16 (qemu_input_map_qcode_to_linux[qcode]); event.value =cpu_to_le32 (key->down ?1 :0 );virtio_input_send (vinput, &event); }else {
@@ -395,8 +277,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); }static const TypeInfo virtio_keyboard_info = {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 4/4] hw: convert virtio-input-hid device to keycodemapdb
Quoted 10 lines · Daniel P. Berrange
Replace the
keymap_qcodetable with automatically generated tables.Missing entries in
keymap_qcodenow fixed:When a keycode is removed from the list of possible keycodes that host can send to the guest, it means that the guest OS will think it is possible to receive a key that in pratice can never be generated, which is harmless.
s/pratice/practice/
Quoted 10 lines
When a keycode is added to the list of possible keycodes that the host can send to the guest, it means that the guest OS can see an unexpected event. The Linux
virtio_input.cdriver code simply forwards this event to theinput_event()method in the Linux input subsystem. This in turn callsinput_handle_event(), which then callsinput_get_disposition(). This method checks if the input event is present in the permitted keys bitmap, and if not returnsINPUT_IGNORE_EVENT. Thus the unexpected event will get dropped, which is harmless.
Quoted 4 lines
If the guest OS reboots, or otherwise re-initializes the virt-input device, it will read the new keycode bitmap. No matter how many keys are defined, the config space has a fixed 128 byte bitmap. There is, however, a size field defiend which says how many bytes in the bitmap are used. So the guest
s/defiend/defined/
Quoted 1 line
OS reads the size of the bitmap, and then it reads the data from bitmap upto
s/upto/up to/
Quoted 5 lines
the designated size. So if the guest OS re-initializes at precisely the time that QEMU is migrated across versions, in the worst case, it could conceivably read the old size field, but then get the newly updated bitmap. If a key were added this is harmless, since it simply means it may not process the newly added key. If a key were removed, then it could be readnig a byte from the
s/readnig/reading/
Quoted 1 line
bitmap that was not initialized. Fortunately QEMU always
memsets()the entire
maybe s/memsets()/memset()s/
Quoted 10 lines
bitmap to 0, prior to setting keybits. Thus the guest OS will simply read zeros, which is again harmless.
Based on this analysis, it is believed that there is no need to preserve the virtio-input-hid keymaps across migration, as the host<->guest ABI change is harmless and self-resolving at time of guest reboot.
NB, this behaviour should perhaps be formalized in the virtio-input spec to declare how guest OS drivers should be written to be robust in their handling of the potentially changable key bitmaps.
Your analysis of the issues, as well as the advice to enhance the virtio-input spec to document that a guest must not react negatively to the change, both sound reasonable.
Quoted 5 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- hw/input/virtio-input-hid.c | 136 +++----------------------------------------- 1 file changed, 9 insertions(+), 127 deletions(-)
Fun diffstat ratio!
I have not closely reviewed the series, so much as noticing the grammar on the fly.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v7 4/4] hw: convert virtio-input-hid device to keycodemapdb
Quoted 54 lines · Eric Blake
Quoted 10 lines · Daniel P. Berrange
Replace the
keymap_qcodetable with automatically generated tables.Missing entries in
keymap_qcodenow fixed:When a keycode is removed from the list of possible keycodes that host can send to the guest, it means that the guest OS will think it is possible to receive a key that in pratice can never be generated, which is harmless.
s/pratice/practice/
Quoted 10 lines
When a keycode is added to the list of possible keycodes that the host can send to the guest, it means that the guest OS can see an unexpected event. The Linux
virtio_input.cdriver code simply forwards this event to theinput_event()method in the Linux input subsystem. This in turn callsinput_handle_event(), which then callsinput_get_disposition(). This method checks if the input event is present in the permitted keys bitmap, and if not returnsINPUT_IGNORE_EVENT. Thus the unexpected event will get dropped, which is harmless.Quoted 4 lines
If the guest OS reboots, or otherwise re-initializes the virt-input device, it will read the new keycode bitmap. No matter how many keys are defined, the config space has a fixed 128 byte bitmap. There is, however, a size field defiend which says how many bytes in the bitmap are used. So the guest
s/defiend/defined/
Quoted 1 line
OS reads the size of the bitmap, and then it reads the data from bitmap upto
s/upto/up to/
Quoted 5 lines
the designated size. So if the guest OS re-initializes at precisely the time that QEMU is migrated across versions, in the worst case, it could conceivably read the old size field, but then get the newly updated bitmap. If a key were added this is harmless, since it simply means it may not process the newly added key. If a key were removed, then it could be readnig a byte from the
s/readnig/reading/
Quoted 1 line
bitmap that was not initialized. Fortunately QEMU always
memsets()the entiremaybe s/memsets()/memset()s/
Quoted 7 lines
bitmap to 0, prior to setting keybits. Thus the guest OS will simply read zeros, which is again harmless.
Based on this analysis, it is believed that there is no need to preserve the virtio-input-hid keymaps across migration, as the host<->guest ABI change is harmless and self-resolving at time of guest reboot.
Quoted 7 lines
Quoted 3 lines
NB, this behaviour should perhaps be formalized in the virtio-input spec to declare how guest OS drivers should be written to be robust in their handling of the potentially changable key bitmaps.
Your analysis of the issues, as well as the advice to enhance the virtio-input spec to document that a guest must not react negatively to the change, both sound reasonable.
If anyone has the time to finalize the spec patch and send it to the virtio tc, that would be very welcome!
IIRC the last revision was
[virtio-dev] [PATCH v2] Add virtio input device specification.
Quoted 16 lines
Quoted 5 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- hw/input/virtio-input-hid.c | 136 +++----------------------------------------- 1 file changed, 9 insertions(+), 127 deletions(-)Fun diffstat ratio!
I have not closely reviewed the series, so much as noticing the grammar on the fly.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v7 4/4] hw: convert virtio-input-hid device to keycodemapdb
In reply to Daniel P. BerrangeHi,
Quoted 4 lines
If the guest OS reboots, or otherwise re-initializes the virt-input device, it will read the new keycode bitmap. No matter how many keys are defined, the config space has a fixed 128 byte bitmap. There is, however, a size field defiend which says how many bytes in the bitmap are used.
No. There is a size field saying how big the bitmap is. config space (as in: virtio device config space) is only as big as is actually needed, i.e. basically the highest set bit of the bitmap determines the config space size.
Debug patch ...
hw/input/virtio-input.c +0 -0
@@ -255,6 +255,8 @@ static void virtio_input_device_realize(DeviceState
*dev, Error **errp)
}
vinput->cfg_size += 8;
assert(vinput->cfg_size <= sizeof(virtio_input_config));+ fprintf(stderr, "%s: %s: %d bytes cfg space\n", __func__,
+ object_get_typename(OBJECT(dev)), vinput->cfg_size);
virtio_init(vdev, "virtio-input", VIRTIO_ID_INPUT,
vinput->cfg_size);
... prints this without the patch ...
virtio_input_device_realize: virtio-keyboard-device: 29 bytes cfg space
... and this with the patch:
virtio_input_device_realize: virtio-keyboard-device: 37 bytes cfg space
That seems to not have any bad side effects on live migration though. I can vmsave with unpatched qemu and vmload with patched qemu (and visa versa).
Agreeing with the analysis that guests should cope with the change just fine, worst case being that the newly added keys are not working on updated qemu without guest reboot.
So I think we can go forward with this one without breaking anything.
thanks,
Gerd
Re: [Qemu-devel] [PATCH v7 4/4] hw: convert virtio-input-hid device to keycodemapdb
Quoted 11 lines · Gerd Hoffmann
Hi,
Quoted 4 lines
If the guest OS reboots, or otherwise re-initializes the virt-input device, it will read the new keycode bitmap. No matter how many keys are defined, the config space has a fixed 128 byte bitmap. There is, however, a size field defiend which says how many bytes in the bitmap are used.
No. There is a size field saying how big the bitmap is. config space (as in: virtio device config space) is only as big as is actually needed, i.e. basically the highest set bit of the bitmap determines the config space size.
Oopps, I missed that subtlety, thinking we always read at least the size of "struct virtio_input_config"
Quoted 20 lines
Debug patch ...
hw/input/virtio-input.c +2
-0@@ -255,6 +255,8 @@ static void virtio_input_device_realize(DeviceState *dev, Error **errp) } vinput->cfg_size +=8 ;assert (vinput->cfg_size <=sizeof (virtio_input_config)); +fprintf (stderr,"%s: %s: %d bytes cfg space\n" , __func__, +object_get_typename (OBJECT (dev)), vinput->cfg_size); virtio_init(vdev, "virtio-input", VIRTIO_ID_INPUT,vinput->cfg_size);... prints this without the patch ...
virtio_input_device_realize: virtio-keyboard-device: 29 bytes cfg space
Quoted 8 lines
... and this with the patch:
virtio_input_device_realize: virtio-keyboard-device: 37 bytes cfg spaceThat seems to not have any bad side effects on live migration though. I can vmsave with unpatched qemu and vmload with patched qemu (and visa versa).
IIUC, the guest OS will only read this cfg data when the driver loads.
So during vmload, ths guest won't trigger this code path.
IIUC, to be affected by the incompatibility, the guest would have to be vmsave+vmload'd / migrated, at the exact time window between reading the config space size, and reading the config space data.
In the old -> new case, that's still safe as we simply don't read all the data. In the new -> old case, we could be reading 37 bytes when only 29 bytes of cfg space are mapped.
This is exceedingly unlikely to happen in practice, but I'm still curious what happens if we try to read too much cfg space ?
Quoted 3 lines
Agreeing with the analysis that guests should cope with the change just fine, worst case being that the newly added keys are not working on updated qemu without guest reboot.
Regards,
Daniel
Signature · 3 lines
|: 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 :|