| Message ID | 20170724164601.21063-1-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le 24/07/2017 à 18:46, Daniel P. Berrange a écrit : > The processing of the scancodes for PAUSE/BREAK has been broken since > the conversion to qcodes in: > > commit 8c10e0baf0260b59a4e984744462a18016662e3e > Author: Hervé Poussineau <hpoussin@reactos.org> > Date: Thu Sep 15 22:06:26 2016 +0200 > > ps2: use QEMU qcodes instead of scancodes > > When using a VNC client, with the raw scancode extension, the client > will send a scancode of 0xc6 for both PAUSE and BREAK. There is mistakenly > no entry in the qcode_to_number table for this scancode, so > ps2_keyboard_event() just generates a log message and discards the > scancode > > When using a SPICE client, it will also send 0xc6 for BREAK, but > will send 0xe1 0x1d 0x45 0xe1 0x9d 0xc5 for PAUSE. There is no > entry in the qcode_to_number table for the scancode 0xe1 because > it is a special XT keyboard prefix not mapping to any QKeyCode. > Again ps2_keyboard_event() just generates a log message and discards > the scancode. The following 0x1d, 0x45, 0x9d, 0xc5 scancodes get > handled correctly. Fixing this just requires special casing 0xe1 so > it is directly queued for sending to the guest, skipping any conversion > to QKeyCode. Your commit message is divided in 2 parts, and you're touching 1 file for each part of the commit message. Should it be 2 patches instead? > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > hw/input/ps2.c | 7 +++++++ > ui/input-keymap.c | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index 3ba05efd06..a132d1ba72 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -607,6 +607,13 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, > assert(evt->type == INPUT_EVENT_KIND_KEY); > qcode = qemu_input_key_value_to_qcode(key->key); > > + if (qcode == 0 && > + key->key->type == KEY_VALUE_KIND_NUMBER && > + key->key->u.number.data == 0x61) { > + ps2_put_keycode(s, 0xe1); > + return; > + } > + You're putting some specific code for spice in ps2 emulation. IMO, the workaround should be moved to spice keyboard handling (ui/spice-input.c), which needs to generate a qcode instead of a scancode. Here, you're making ps2 emulation work again with spice, but you're missing all other emulations using qcodes. Do you want to also modify them? I understand that it may be the easiest way to fix the regression for 2.10, but it needs at least a comment. > if (s->scancode_set == 1) { > if (qcode == Q_KEY_CODE_PAUSE) { > if (key->down) { > diff --git a/ui/input-keymap.c b/ui/input-keymap.c > index 8a1476fc48..9211f835be 100644 > --- a/ui/input-keymap.c > +++ b/ui/input-keymap.c > @@ -98,6 +98,7 @@ static const int qcode_to_number[] = { > [Q_KEY_CODE_KP_ENTER] = 0x9c, > [Q_KEY_CODE_KP_DECIMAL] = 0x53, > [Q_KEY_CODE_SYSRQ] = 0x54, > + [Q_KEY_CODE_PAUSE] = 0xc6, This part: Acked-by: Hervé Poussineau <hpoussin@reactos.org> > > [Q_KEY_CODE_KP_0] = 0x52, > [Q_KEY_CODE_KP_1] = 0x4f, >
On Mon, Jul 24, 2017 at 09:55:20PM +0200, Hervé Poussineau wrote: > Le 24/07/2017 à 18:46, Daniel P. Berrange a écrit : > > The processing of the scancodes for PAUSE/BREAK has been broken since > > the conversion to qcodes in: > > > > commit 8c10e0baf0260b59a4e984744462a18016662e3e > > Author: Hervé Poussineau <hpoussin@reactos.org> > > Date: Thu Sep 15 22:06:26 2016 +0200 > > > > ps2: use QEMU qcodes instead of scancodes > > > > When using a VNC client, with the raw scancode extension, the client > > will send a scancode of 0xc6 for both PAUSE and BREAK. There is mistakenly > > no entry in the qcode_to_number table for this scancode, so > > ps2_keyboard_event() just generates a log message and discards the > > scancode > > > > When using a SPICE client, it will also send 0xc6 for BREAK, but > > will send 0xe1 0x1d 0x45 0xe1 0x9d 0xc5 for PAUSE. There is no > > entry in the qcode_to_number table for the scancode 0xe1 because > > it is a special XT keyboard prefix not mapping to any QKeyCode. > > Again ps2_keyboard_event() just generates a log message and discards > > the scancode. The following 0x1d, 0x45, 0x9d, 0xc5 scancodes get > > handled correctly. Fixing this just requires special casing 0xe1 so > > it is directly queued for sending to the guest, skipping any conversion > > to QKeyCode. > > Your commit message is divided in 2 parts, and you're touching 1 file for each part of the commit message. > Should it be 2 patches instead? > > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > hw/input/ps2.c | 7 +++++++ > > ui/input-keymap.c | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > > index 3ba05efd06..a132d1ba72 100644 > > --- a/hw/input/ps2.c > > +++ b/hw/input/ps2.c > > @@ -607,6 +607,13 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, > > assert(evt->type == INPUT_EVENT_KIND_KEY); > > qcode = qemu_input_key_value_to_qcode(key->key); > > > > + if (qcode == 0 && > > + key->key->type == KEY_VALUE_KIND_NUMBER && > > + key->key->u.number.data == 0x61) { > > + ps2_put_keycode(s, 0xe1); > > + return; > > + } > > + > > You're putting some specific code for spice in ps2 emulation. > IMO, the workaround should be moved to spice keyboard handling (ui/spice-input.c), > which needs to generate a qcode instead of a scancode. This isn't really a spice specific hack. QEMU internal code is *not* required to use qcodes - the KeyValue struct is a union that allows use of either qcodes or XT scancodes, and the latter is what all the frontends (SPICE, VNC, GTk, SDL) use. QCodes are really only input by the monitor (the sendkey command). > Here, you're making ps2 emulation work again with spice, but you're missing all > other emulations using qcodes. Do you want to also modify them? I checked the USB keyboard device and that works fine. AFAIK, the PS/2 code is the only code that takes XT scancodes, converts to qcode and then converts qcodes back to scancodes - most others avoid this kind of roundtripping via qcodes. > I understand that it may be the easiest way to fix the regression for 2.10, but > it needs at least a comment. Regards, Daniel
Hi, > > You're putting some specific code for spice in ps2 emulation. > > IMO, the workaround should be moved to spice keyboard handling > > (ui/spice-input.c), > > which needs to generate a qcode instead of a scancode. > > This isn't really a spice specific hack. QEMU internal code is *not* > required > to use qcodes qcodes are prefered in new code though. > - the KeyValue struct is a union that allows use of either qcodes > or XT scancodes, and the latter is what all the frontends (SPICE, > VNC, GTk, SDL) > use. QCodes are really only input by the monitor (the sendkey > command). Well, PAUSE is actually sent as qcode by sdl and gtk. This avoids special cases in the input layer (PAUSE is the only three scancodes key sequence). IMO spice should do the same. I want switch UIs to qcodes anyway. cheers, Gerd
On Tue, Jul 25, 2017 at 01:53:40PM +0200, Gerd Hoffmann wrote: > Hi, > > > > You're putting some specific code for spice in ps2 emulation. > > > IMO, the workaround should be moved to spice keyboard handling > > > (ui/spice-input.c), > > > which needs to generate a qcode instead of a scancode. > > > > This isn't really a spice specific hack. QEMU internal code is *not* > > required > > to use qcodes > > qcodes are prefered in new code though. > > > - the KeyValue struct is a union that allows use of either qcodes > > or XT scancodes, and the latter is what all the frontends (SPICE, > > VNC, GTk, SDL) > > use. QCodes are really only input by the monitor (the sendkey > > command). > > Well, PAUSE is actually sent as qcode by sdl and gtk. This avoids > special cases in the input layer (PAUSE is the only three scancodes key > sequence). IMO spice should do the same. I want switch UIs to qcodes > anyway. qcodes as currently defined cover only a subset of the AT set1 scancodes, so we need to define countless more qcodes before we consider converting UIs to use qcodes. Aside from the pause/break bug, the changes to ps2 driver to round trip via qcodes have now made it impossible to send a large number of key sequences to the guest OS :-( Admittedly the missing key codes are not so commonly used, but it is still a notable regression in functionality today Regards, Daniel
Hi, > qcodes as currently defined cover only a subset of the AT set1 > scancodes, > so we need to define countless more qcodes before we consider > converting > UIs to use qcodes. > > Aside from the pause/break bug, the changes to ps2 driver to round > trip > via qcodes have now made it impossible to send a large number of key > sequences to the guest OS :-( Admittedly the missing key codes are > not > so commonly used, but it is still a notable regression in > functionality > today My keymap branch carries fixes for that now: https://www.kraxel.org/cgit/qemu/log/?h=work/xkbcommon For your keycodemapdb patches I'd suggest to cherry-pick at least the "ui: move qemu_input_linux_to_qcode()" patch. Then have sdl + gtk generate linux evdev codes using keycodemapdb, map that to qcodes using qemu_input_linux_to_qcode(), submit qcodes to the qemu input layer. cheers, Gerd
On Wed, Jul 26, 2017 at 01:44:07PM +0200, Gerd Hoffmann wrote: > Hi, > > > qcodes as currently defined cover only a subset of the AT set1 > > scancodes, > > so we need to define countless more qcodes before we consider > > converting > > UIs to use qcodes. > > > > Aside from the pause/break bug, the changes to ps2 driver to round > > trip > > via qcodes have now made it impossible to send a large number of key > > sequences to the guest OS :-( Admittedly the missing key codes are > > not > > so commonly used, but it is still a notable regression in > > functionality > > today > > My keymap branch carries fixes for that now: > https://www.kraxel.org/cgit/qemu/log/?h=work/xkbcommon > > For your keycodemapdb patches I'd suggest to cherry-pick at least the > "ui: move qemu_input_linux_to_qcode()" patch. FYI, after adding qcodes to the keycodemapdb, (but not having picked your branch), I see 131 keys where there is a known AT set 1 scancode, but no corresponding QKeyCode. I'll happily add the rest of them if you want (attaching the list) > Then have sdl + gtk generate linux evdev codes using keycodemapdb, map > that to qcodes using qemu_input_linux_to_qcode(), submit qcodes to the > qemu input layer. I was actually just going add an entry for QKeyCode names to the keycodemapdb, so we can convert straight to that, and also let us auto-generate the qemu_input_linux_to_qcode() table. Strangely there is no Linux key code defined for AltGr, so if we map via Linux key codes, we would loose ability to send AltGr Regards, Daniel
Hi, > FYI, after adding qcodes to the keycodemapdb, (but not having picked > your branch), I don't think this is a good idea. qkeycode numbers are a qemu-private thing, they can (and did in the past) change. The names are api and must not change. > I was actually just going add an entry for QKeyCode names to the > keycodemapdb, so we can convert straight to that, and also let > us auto-generate the qemu_input_linux_to_qcode() table. Ok, with the names it should work. > Strangely there is no Linux key code defined for AltGr, so if we > map via Linux key codes, we would loose ability to send AltGr KEY_RIGHTALT? cheers, Gerd
On Wed, Jul 26, 2017 at 02:33:10PM +0200, Gerd Hoffmann wrote: > Hi, > > > FYI, after adding qcodes to the keycodemapdb, (but not having picked > > your branch), > > I don't think this is a good idea. qkeycode numbers are a qemu-private > thing, they can (and did in the past) change. The names are api and > must not change. Sorry, yes, I meant only adding the names - libvirt needs to know the qcode names and what they map to in other key maps, to use the send-key command. > > I was actually just going add an entry for QKeyCode names to the > > keycodemapdb, so we can convert straight to that, and also let > > us auto-generate the qemu_input_linux_to_qcode() table. > > Ok, with the names it should work. > > > Strangely there is no Linux key code defined for AltGr, so if we > > map via Linux key codes, we would loose ability to send AltGr > > KEY_RIGHTALT? It seems KEY_RIGHTALT is different from AltGr - it maps to AT set 1 extended scancode 0xe0 0x38, where as AltGr generates 0x64 on my keyboard Regards, Daniel
On Wed, Jul 26, 2017 at 01:45:51PM +0100, Daniel P. Berrange wrote: > On Wed, Jul 26, 2017 at 02:33:10PM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > FYI, after adding qcodes to the keycodemapdb, (but not having picked > > > your branch), > > > > I don't think this is a good idea. qkeycode numbers are a qemu-private > > thing, they can (and did in the past) change. The names are api and > > must not change. > > Sorry, yes, I meant only adding the names - libvirt needs to know the > qcode names and what they map to in other key maps, to use the send-key > command. > > > > I was actually just going add an entry for QKeyCode names to the > > > keycodemapdb, so we can convert straight to that, and also let > > > us auto-generate the qemu_input_linux_to_qcode() table. > > > > Ok, with the names it should work. > > > > > Strangely there is no Linux key code defined for AltGr, so if we > > > map via Linux key codes, we would loose ability to send AltGr > > > > KEY_RIGHTALT? > > It seems KEY_RIGHTALT is different from AltGr - it maps to AT set 1 > extended scancode 0xe0 0x38, where as AltGr generates 0x64 on my > keyboard Opps, no I was mis-interpreting the evdev output. Physical key AltGr *does* produce KEY_RIGHTALT, Linux keycode 100, AT set1 scan code 0xe0 0x38, QEMU qcode alt_r, QEMU scancode 0xb8 What is confusing is that QEMU defines qcodes for alt, alt_r *and* altgr and altgr_r. QEMU's altgr scancode is 0x64 (qcode_to_number). The only Linux keycode that produces that AT set1 scancode is KEY_OPEN. There is no mapping in qcode_to_keycode_set1 for this, which is odd given that the set1 mapping should be the same as qcode_to_number mapping except different high bit handling. There is a mapping in qcode_to_keycode_set2 which corresponds to 0x08 and afaik that should not exist as there's no set2 scancode 0x08 QEMU's altgr_r scan code is 0xe4 (qcode_to_number), which is equiv to at set1 0xe0 0x64, and that should not exist afaik. There is no mapping in qcode_to_keycode_set1, which is again odd given that this should be the same as qcode_to_number. There is a mapping in qcode_to_keycode_set2 which corresponds to 0xe0 0x08 and afaik that's not defined for set2 scancode. So afaict, the altgr & altgr_r qcodes are just broken and should not even exist. Regards, Daniel
diff --git a/hw/input/ps2.c b/hw/input/ps2.c index 3ba05efd06..a132d1ba72 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -607,6 +607,13 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src, assert(evt->type == INPUT_EVENT_KIND_KEY); qcode = qemu_input_key_value_to_qcode(key->key); + if (qcode == 0 && + key->key->type == KEY_VALUE_KIND_NUMBER && + key->key->u.number.data == 0x61) { + ps2_put_keycode(s, 0xe1); + return; + } + if (s->scancode_set == 1) { if (qcode == Q_KEY_CODE_PAUSE) { if (key->down) { diff --git a/ui/input-keymap.c b/ui/input-keymap.c index 8a1476fc48..9211f835be 100644 --- a/ui/input-keymap.c +++ b/ui/input-keymap.c @@ -98,6 +98,7 @@ static const int qcode_to_number[] = { [Q_KEY_CODE_KP_ENTER] = 0x9c, [Q_KEY_CODE_KP_DECIMAL] = 0x53, [Q_KEY_CODE_SYSRQ] = 0x54, + [Q_KEY_CODE_PAUSE] = 0xc6, [Q_KEY_CODE_KP_0] = 0x52, [Q_KEY_CODE_KP_1] = 0x4f,
The processing of the scancodes for PAUSE/BREAK has been broken since the conversion to qcodes in: commit 8c10e0baf0260b59a4e984744462a18016662e3e Author: Hervé Poussineau <hpoussin@reactos.org> Date: Thu Sep 15 22:06:26 2016 +0200 ps2: use QEMU qcodes instead of scancodes When using a VNC client, with the raw scancode extension, the client will send a scancode of 0xc6 for both PAUSE and BREAK. There is mistakenly no entry in the qcode_to_number table for this scancode, so ps2_keyboard_event() just generates a log message and discards the scancode When using a SPICE client, it will also send 0xc6 for BREAK, but will send 0xe1 0x1d 0x45 0xe1 0x9d 0xc5 for PAUSE. There is no entry in the qcode_to_number table for the scancode 0xe1 because it is a special XT keyboard prefix not mapping to any QKeyCode. Again ps2_keyboard_event() just generates a log message and discards the scancode. The following 0x1d, 0x45, 0x9d, 0xc5 scancodes get handled correctly. Fixing this just requires special casing 0xe1 so it is directly queued for sending to the guest, skipping any conversion to QKeyCode. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- hw/input/ps2.c | 7 +++++++ ui/input-keymap.c | 1 + 2 files changed, 8 insertions(+)