Re: [Qemu-devel] [PATCH v1 4/9] ps2: fix scancodes sent for Alt-Print key combination (aka SysRq)

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

On Thu, Oct 19, 2017 at 03:28:43PM +0100, Daniel P. Berrange wrote:
> 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>
> ---
>  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;

Presumably the addition of this field means I should do something with the
vmstate for migration. I'm unclear exactly how to modify that data in the right
way that correctly preserves back compatibility ?


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