Re: [Qemu-devel] [PATCH for 2.10] ps2: fix sending of PAUSE/BREAK scancodes

["Daniel P. Berrange" <berrange@redhat.com>]

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
-- 
|: 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 :|