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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Apr 10, 2014 at 01:47:08PM +0200, Gerd Hoffmann wrote:
> 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 <kraxel@redhat.com>
> > 
> > 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).

It's up to you I am just asking.

If drivers don't expect a mix of functionality, then separate IDs
seem better.
OTOH an advantage to using a custom identification scheme would be
if you want to allow a single device to change type
dynamically.

E.g. if guest knows about MT, act as an MT device otherwise
act as a mouse.

Or more interestingly, as we migrate from host with a mouse
to an MT one, switch from a mouse to a touch device?


> > > +    [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.

See below.


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


That's exactly the question.  Which version of linux/input.h?

> > Also what defines the subset selected?
> 
> All keys in linux/input.h are supported by the virtio input protocol.

I'm asking how is this versioned: assume that linux adds
a new keycode, existing guests don't expect it.
Don't we need any negotiation? Is it really always safe to
ignore keypresses?

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

Hmm SDL can give us a bunch of codes like volume up/down
that you don't seem to include.



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