Re: [Qemu-devel] [3/6] PS/2: Add KBD_CMD_SCANCODE command

[Aurelien Jarno]

On Mon, Mar 03, 2008 at 11:44:37AM +0100, Hervé Poussineau wrote:
> PS/2 controller emulation lacks the KBD_CMD_SCANCODE command, which  
> gets/sets the scancode set (1, 2 or 3).
> Scancode sets 1 and 2 are still not supported.

AFAIK, the default scancode set should be 2, and not 3. This is also the
scancode set used internally by QEMU. 

See my other comments inline.

> (This patch was already sent on 20080224)

> Index: hw/ps2.c
> ===================================================================
> RCS file: /sources/qemu/qemu/hw/ps2.c,v
> retrieving revision 1.10
> diff -u -r1.10 ps2.c
> --- hw/ps2.c  16 Dec 2007 23:41:11 -0000      1.10
> +++ hw/ps2.c  23 Feb 2008 21:05:05 -0000
> @@ -34,6 +34,7 @@
>  /* Keyboard Commands */
>  #define KBD_CMD_SET_LEDS     0xED    /* Set keyboard leds */
>  #define KBD_CMD_ECHO         0xEE
> +#define KBD_CMD_SCANCODE     0xF0    /* Get/set scancode set */
>  #define KBD_CMD_GET_ID               0xF2    /* get keyboard ID */
>  #define KBD_CMD_SET_RATE     0xF3    /* Set typematic rate */
>  #define KBD_CMD_ENABLE               0xF4    /* Enable scanning */
> @@ -89,6 +90,7 @@
>         conversions we do the translation (if any) in the PS/2 emulation
>         not the keyboard controller.  */
>      int translate;
> +    int scancode_set;
>  } PS2KbdState;
>  
>  typedef struct {
> @@ -134,7 +136,9 @@
>  static void ps2_put_keycode(void *opaque, int keycode)
>  {
>      PS2KbdState *s = opaque;
> -    if (!s->translate && keycode < 0xe0)
> +
> +    /* XXX: add support for scancode sets 1 and 2 */
> +    if (!s->translate && keycode < 0xe0 && s->scancode_set == 3)

should be scancode_set == 2.

>        {
>          if (keycode & 0x80)
>              ps2_queue(&s->common, 0xf0);
> @@ -202,6 +206,7 @@
>              s->scan_enabled = 1;
>              ps2_queue(&s->common, KBD_REPLY_ACK);
>              break;
> +        case KBD_CMD_SCANCODE:
>          case KBD_CMD_SET_LEDS:
>          case KBD_CMD_SET_RATE:
>              s->common.write_cmd = val;
> @@ -227,6 +232,22 @@
>              break;
>          }
>          break;
> +    case KBD_CMD_SCANCODE:
> +        if (val == 0) {
> +            if (s->scancode_set == 1)
> +                ps2_queue(&s->common, 0x43);
> +            else if (s->scancode_set == 2)
> +                ps2_queue(&s->common, 0x41);
> +            else if (s->scancode_set == 3)
> +                ps2_queue(&s->common, 0x3f);

Those values are the translated ones. I think you should call
ps2_put_keycode() instead of ps2_queue(), so that the correct code is
sent when translation is disabled.

OTOH, untranslated mode is almost never used by the OSes, and it looks
like KBD_CMD_GET_ID is also wrong.

> +            else
> +                ps2_queue(&s->common, KBD_REPLY_ACK);
> +        } else {
> +            s->scancode_set = val;
> +            ps2_queue(&s->common, KBD_REPLY_ACK);
> +        }
> +        s->common.write_cmd = -1;
> +        break;
>      case KBD_CMD_SET_LEDS:
>          ps2_queue(&s->common, KBD_REPLY_ACK);
>          s->common.write_cmd = -1;
> @@ -493,6 +514,7 @@
>      ps2_common_save (f, &s->common);
>      qemu_put_be32(f, s->scan_enabled);
>      qemu_put_be32(f, s->translate);
> +    qemu_put_be32(f, s->scancode_set);
>  }
>  
>  static void ps2_mouse_save(QEMUFile* f, void* opaque)
> @@ -516,12 +538,16 @@
>  {
>      PS2KbdState *s = (PS2KbdState*)opaque;
>  
> -    if (version_id != 2)
> +    if (version_id != 2 && version_id != 3)
>          return -EINVAL;
>  
>      ps2_common_load (f, &s->common);
>      s->scan_enabled=qemu_get_be32(f);
>      s->translate=qemu_get_be32(f);
> +    if (version_id == 3)
> +        s->scancode_set=qemu_get_be32(f);
> +    else
> +        s->scancode_set=3;

Should be 2.

>      return 0;
>  }
>  
> @@ -552,8 +578,9 @@
>  
>      s->common.update_irq = update_irq;
>      s->common.update_arg = update_arg;
> +    s->scancode_set = 3;

Should be 2.

>      ps2_reset(&s->common);
> -    register_savevm("ps2kbd", 0, 2, ps2_kbd_save, ps2_kbd_load, s);
> +    register_savevm("ps2kbd", 0, 3, ps2_kbd_save, ps2_kbd_load, s);
>      qemu_add_kbd_event_handler(ps2_put_keycode, s);
>      qemu_register_reset(ps2_reset, &s->common);
>      return s;


-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net