Re: [Qemu-devel] [PATCH qemu 4/6] virtio-input: emulated devices

[Gerd Hoffmann]

On Do, 2014-04-10 at 13:55 +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 10, 2014 at 11:07:52AM +0200, Gerd Hoffmann wrote:
> > This patch adds the virtio-input-hid base class and
> > virtio-{keyboard,mouse,tablet} subclasses building on the base class.
> > They are hooked up to the qemu input core and deliver input events
> > to the guest like all other hid devices (ps/2 kbd, usb tablet, ...).
> > 
> > Using them is as simple as adding "-device virtio-tablet-pci" to your
> > command line.  If you want add multiple devices but don't want waste
> > a pci slot for each you can compose a multifunction device this way:
> > 
> > qemu -device virtio-keyboard-pci,addr=0d.0,multifunction=on \
> >      -device virtio-tablet-pci,addr=0d.1,multifunction=on
> > 
> > Signed-off-by: Gerd Hoffmann <address@hidden>
> 
> Hmm - that's interesting.
> I was under the impression that a single pci function can be
> a keyboard, mouse and tablet at the same time.

It is possible to create a device supporting both keyboard and
mouse/tablet events.  Which will also show up as single input device in
the guest then.  People and software tends to not expect that though, so
I think it is better to keep them separate.

> If they aren't why don't we assign distinct device IDs to them
> after all?

pci device ids I assume?  Sure, we can do that.  Will make lspci output
a bit more informative (no need to check /proc/bus/input/devices to
figure what kind of input device it is).

> > +    [Q_KEY_CODE_META_L]              = KEY_LEFTMETA,
> > +    [Q_KEY_CODE_META_R]              = KEY_RIGHTMETA,
> > +    [Q_KEY_CODE_MENU]                = KEY_MENU,
> > +};
> 
> OK these are values send to guest, right?

Yes.

> And they are from linux/input.h, right? But are these
> reasonable in a cross-platform device?

Can't see strong reasons speaking against it.  It's kernel/userspace
API, therefore stable.  There are keycodes defined for pretty much
anything you can think of.

linux guest code is dead simple.  For other guests supporting it
shouldn't be that hard too, they basically need a mapping table to map
the linux KEY_* codes into their internal representation.

> E.g. Linux is pretty good at backwards compatibility
> but less good at versioning.

--verbose please.

> That header says "Most of the keys/buttons are modeled after USB HUT
> 1.12" but as far as I could see the codes are not from HUT, correct?

No, the codes are different.

> Would it be a good idea to use codes from HUT directly?
> This way we could extend functionality without adding lots of
> text to the spec, simply by referring to HUT.

I want to simply refer to linux/input.h in the spec.

> Also what defines the subset selected?

All keys in linux/input.h are supported by the virtio input protocol.

The current qemu kbd emulation covers all keys qemu knows (see QKeyCode
in qapi-schema.json).

> > +static const unsigned int axismap_abs[INPUT_AXIS_MAX] = {
> > +    [INPUT_AXIS_X]                   = ABS_X,
> > +    [INPUT_AXIS_Y]                   = ABS_Y,
> > +};
> > +
> 
> In the future, it seems like a good idea to report raw
> multi-touch events to guests - this would need a different
> interface along the lines of
> Documentation/input/multi-touch-protocol.txt

Should be no big deal.  Not looked at that deeply yet due to lack of
test hardware, but I think all we need is mapping the info from
EVIOCGMTSLOTS into config space, simliar to how it is done for
EVIOCGABS.

> Do MT devices generate ST events as well so it's ok to just
> filter out everything we don't recognize?

Yes, as far I know both mt and st events are generated.

> > +static void virtio_input_hid_handle_status(VirtIOInput *vinput,
> > +                                           virtio_input_event *event)
> > +{
> > +    VirtIOInputHID *vhid = VIRTIO_INPUT_HID(vinput);
> > +    int ledbit = 0;
> > +
> > +    switch (le16_to_cpu(event->type)) {
> > +    case EV_LED:
> > +        if (event->code == LED_NUML) {
> > +            ledbit = QEMU_NUM_LOCK_LED;
> > +        } else if (event->code == LED_CAPSL) {
> > +            ledbit = QEMU_CAPS_LOCK_LED;
> > +        } else if (event->code == LED_SCROLLL) {
> > +            ledbit = QEMU_SCROLL_LOCK_LED;
> > +        }
> > +        if (event->value) {
> > +            vhid->ledstate |= ledbit;
> > +        } else {
> > +            vhid->ledstate &= ~ledbit;
> > +        }
> > +        kbd_put_ledstate(vhid->ledstate);
> 
> What does this do? notice led light up on one keyboard and propagate
> state to all keyboards?

Notify everybody interested in about kbd led changes.  ps/2+usb kbd
emulations do the same.

It is used by vnc for example, to make sure capslock/numlock state
between guest and host stay in sync.

cheers,
  Gerd