| Message ID | 20171023091947.20771-5-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 23, 2017 at 11:19:42AM +0200, Gerd Hoffmann wrote: > From: "Daniel P. Berrange" <berrange@redhat.com> > > The 'Print' key is special in the AT set 1 / set 2 scancode definitions. > > An unmodified 'Print' key is supposed to send > > AT Set 1: e0 2a e0 37 (Down) e0 b7 e0 aa (Up) > AT Set 2: e0 12 e0 7c (Down) e0 f0 7c e0 f0 12 (Up) > > which QEMU gets right. When pressed in combination with the 'Alt_L' or 'Alt_R' > keys (which signify SysRq), the scancodes are required to follow a different > scheme. With Alt_L, the expected sequences are > > AT set 1: 38, 54 (Down) d4, b8 (Up) > AT set 2: 11, 84 (Down) f0 84, f0 11 (Up) > > And with Alt_R > > AT set 1: e0 38, 54 (Down) d4, e0 b8 (Up) > AT set 2: e0 11, 84 (Down) f0 84, f0 e0 11 (Up) > > It is actually slightly more complicated than that, because (according results > of 'showkey -s', keyboards will in fact first release the currently pressed > modifier before sending the sequence above (which effectively re-presses & > then releases the modifier) and finally re-press the original modifier > afterwards. IOW, with Alt_L we need to send > > AT set 1: b8, 38, 54 (Down) d4, b8, 38 (Up) > AT set 2: f0 11, 11, 84 (Down) f0 84, f0 11, 11 (Up) > > And with Alt_R > > AT set 1: e0 b8, e0 38, 54 (Down) d4, e0 b8, e0 38 (Up) > AT set 2: e0 f0 11, e0 11, 84 (Down) f0 84, e0 f0 11, e0 11 (Up) > > The AT set 3 scancodes have no special handling for Alt-Print. > > Rather than fixing the handling of the 'print' key in the ps2 driver to consider > the Alt modifiers, way back, a patch was commited that defined an extra 'sysrq' > key name: > > commit f2289cb6924afc97b2a75d21bfc9217024d11741 > Author: balrog <balrog@c046a42c-6fe2-441c-8c8c-71466251a162> > Date: Wed Jun 4 10:14:16 2008 +0000 > > Add sysrq to key names known by "sendkey". > > Adding sysrq keycode to the table enabling running sysrq debugging in > the guest via the monitor sendkey command, like: > > (qemu) sendkey alt-sysrq-t > > Tested on x86-64 target and Linux guest. > > Signed-off-by: Ryan Harper <ryanh@us.ibm.com> > > With this patch QEMU would send > > AT set 1: 38, 54 (Down) d4, b8 (Up) > AT set 2: 11, 84 (Down) f0 84, f0 11 (Up) > > but this doesn't match what actual real keyboards send, as it is not releasing > the original modifier & pressing it again afterwards. In addition the original > problem remains, and a new problem was added: > > - The sequence 'alt-print-t' is still broken, acting as if 'print-t' was > requested > - The sequence 'sysrq-t' is broken, injecting an undefine scancode sequence > tot he guest os (bare 0x54) > > To deal with this mess we make these changes to the ps2 code, so that we track > the state of modifier keys (Alt, Shift, Ctrl - both left & right). Then we can > vary what scancodes are sent for Q_KEY_CODE_PRINT according to the Alt key > modifier state > > Interestingly, it appears that of operating systems I've checked (Linux, FreeBSD > and OpenSolaris), none of them actually bother to validate the full sequences > for a unmodified 'Print' key. They all just ignore the leading "e0 2a" and > trigger based off "e0 37" alone. The latter two byte sequence is what keyboards > send with 'Print' is combined with 'Shift' or 'Ctrl' modifiers. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > Message-id: 20171019142848.572-5-berrange@redhat.com > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/input/ps2.c | 137 ++++++++++++++++++++++++++++++++++++++++++-------- > hw/input/trace-events | 1 + > 2 files changed, 118 insertions(+), 20 deletions(-) > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index dff3f1e024..1e6f6ae9b6 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -78,6 +78,14 @@ > > #define PS2_QUEUE_SIZE 16 /* Buffer size required by PS/2 protocol */ > > +/* Bits for 'modifiers' field in PS2KbdState */ > +#define MOD_CTRL_L (1 << 0) > +#define MOD_SHIFT_L (1 << 1) > +#define MOD_ALT_L (1 << 2) > +#define MOD_CTRL_R (1 << 3) > +#define MOD_SHIFT_R (1 << 4) > +#define MOD_ALT_R (1 << 5) > + > typedef struct { > /* Keep the data array 256 bytes long, which compatibility > with older qemu versions. */ > @@ -99,6 +107,7 @@ typedef struct { > int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ > int ledstate; > bool need_high_bit; > + unsigned int modifiers; /* bitmask of MOD_* constants above */ > } PS2KbdState; No one confirmed whether or not this new field needs to be added to the VMSTATE sections, and if so, how todo this in a back compatible manner.... Regards, Daniel
Hi, > No one confirmed whether or not this new field needs to be added to > the > VMSTATE sections, and if so, how todo this in a back compatible > manner.... Good question. Sky isn't falling if we don't send this over, worst case is a minor keyboard glitch, affecting only the SysRq key, right? Annoying things like sticky modifier keys should not happen. So I'd tend to simply leave things as-is. When sending it over we need a subsection for it. Simplest way would be to only send over the section in the unlikely case that the field is non-zero. But this might fail migration to older qemu versions in case someone hits a modifier key the wrong moment. Alternatively send the sibsection only on new machine types using a compat property, but that needs a bit more boilerplate. cheers, Gerd
On Wed, Nov 01, 2017 at 08:58:32AM +0100, Gerd Hoffmann wrote: > Hi, > > > No one confirmed whether or not this new field needs to be added to > > the > > VMSTATE sections, and if so, how todo this in a back compatible > > manner.... > > Good question. Sky isn't falling if we don't send this over, worst > case is a minor keyboard glitch, affecting only the SysRq key, right? > > Annoying things like sticky modifier keys should not happen. So I'd > tend to simply leave things as-is. Yeah, we would only gitch that key combo. It should be (practically) impossible to hit, because from the user input POV its just a single key, that internally we turn into a sequence of keys when injecting. > > When sending it over we need a subsection for it. Simplest way would > be to only send over the section in the unlikely case that the field is > non-zero. But this might fail migration to older qemu versions in case > someone hits a modifier key the wrong moment. Alternatively send the > sibsection only on new machine types using a compat property, but that > needs a bit more boilerplate. Regards, Daniel
diff --git a/hw/input/ps2.c b/hw/input/ps2.c index dff3f1e024..1e6f6ae9b6 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -78,6 +78,14 @@ #define PS2_QUEUE_SIZE 16 /* Buffer size required by PS/2 protocol */ +/* Bits for 'modifiers' field in PS2KbdState */ +#define MOD_CTRL_L (1 << 0) +#define MOD_SHIFT_L (1 << 1) +#define MOD_ALT_L (1 << 2) +#define MOD_CTRL_R (1 << 3) +#define MOD_SHIFT_R (1 << 4) +#define MOD_ALT_R (1 << 5) + typedef struct { /* Keep the data array 256 bytes long, which compatibility with older qemu versions. */ @@ -99,6 +107,7 @@ typedef struct { int scancode_set; /* 1=XT, 2=AT, 3=PS/2 */ int ledstate; bool need_high_bit; + unsigned int modifiers; /* bitmask of MOD_* constants above */ } PS2KbdState; typedef struct { @@ -545,6 +554,26 @@ static uint8_t translate_table[256] = { 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff, }; +static unsigned int ps2_modifier_bit(QKeyCode key) +{ + switch (key) { + case Q_KEY_CODE_CTRL: + return MOD_CTRL_L; + case Q_KEY_CODE_CTRL_R: + return MOD_CTRL_R; + case Q_KEY_CODE_SHIFT: + return MOD_SHIFT_L; + case Q_KEY_CODE_SHIFT_R: + return MOD_SHIFT_R; + case Q_KEY_CODE_ALT: + return MOD_ALT_L; + case Q_KEY_CODE_ALT_R: + return MOD_ALT_R; + default: + return 0; + } +} + static void ps2_reset_queue(PS2State *s) { PS2Queue *q = &s->queue; @@ -596,11 +625,20 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, InputKeyEvent *key = evt->u.key.data; int qcode; uint16_t keycode; + int mod; qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); assert(evt->type == INPUT_EVENT_KIND_KEY); qcode = qemu_input_key_value_to_qcode(key->key); + mod = ps2_modifier_bit(qcode); + trace_ps2_keyboard_event(s, qcode, key->down, mod, s->modifiers); + if (key->down) { + s->modifiers |= mod; + } else { + s->modifiers &= ~mod; + } + if (s->scancode_set == 1) { if (qcode == Q_KEY_CODE_PAUSE) { if (key->down) { @@ -612,16 +650,42 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, ps2_put_keycode(s, 0xc5); } } else if (qcode == Q_KEY_CODE_PRINT) { - if (key->down) { - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0x2a); - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0x37); + if (s->modifiers & MOD_ALT_L) { + if (key->down) { + ps2_put_keycode(s, 0xb8); + ps2_put_keycode(s, 0x38); + ps2_put_keycode(s, 0x54); + } else { + ps2_put_keycode(s, 0xd4); + ps2_put_keycode(s, 0xb8); + ps2_put_keycode(s, 0x38); + } + } else if (s->modifiers & MOD_ALT_R) { + if (key->down) { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xb8); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x38); + ps2_put_keycode(s, 0x54); + } else { + ps2_put_keycode(s, 0xd4); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xb8); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x38); + } } else { - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0xb7); - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0xaa); + if (key->down) { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x2a); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x37); + } else { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xb7); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xaa); + } } } else { keycode = qcode_to_keycode_set1[qcode]; @@ -651,18 +715,50 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, ps2_put_keycode(s, 0x77); } } else if (qcode == Q_KEY_CODE_PRINT) { - if (key->down) { - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0x12); - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0x7c); + if (s->modifiers & MOD_ALT_L) { + if (key->down) { + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0x84); + } else { + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x84); + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0x11); + } + } else if (s->modifiers & MOD_ALT_R) { + if (key->down) { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0x84); + } else { + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x84); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x11); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x11); + } } else { - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0xf0); - ps2_put_keycode(s, 0x7c); - ps2_put_keycode(s, 0xe0); - ps2_put_keycode(s, 0xf0); - ps2_put_keycode(s, 0x12); + if (key->down) { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x12); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0x7c); + } else { + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x7c); + ps2_put_keycode(s, 0xe0); + ps2_put_keycode(s, 0xf0); + ps2_put_keycode(s, 0x12); + } } } else { keycode = qcode_to_keycode_set2[qcode]; @@ -1125,6 +1221,7 @@ static void ps2_kbd_reset(void *opaque) s->scan_enabled = 0; s->translate = 0; s->scancode_set = 2; + s->modifiers = 0; } static void ps2_mouse_reset(void *opaque) diff --git a/hw/input/trace-events b/hw/input/trace-events index d04132d342..88150ef7a6 100644 --- a/hw/input/trace-events +++ b/hw/input/trace-events @@ -2,6 +2,7 @@ # hw/input/ps2.c ps2_put_keycode(void *opaque, int keycode) "%p keycode 0x%02x" +ps2_keyboard_event(void *opaque, int qcode, int down, unsigned int modifier, unsigned int modifiers) "%p qcode %d down %d modifier 0x%x modifiers 0x%x" ps2_read_data(void *opaque) "%p" ps2_set_ledstate(void *s, int ledstate) "%p ledstate %d" ps2_reset_keyboard(void *s) "%p"