[Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Alexander Graf]

When driving QEMU from the outside, we have basically no chance to
determine how quickly the guest OS picks up key events, so we usually
have to limit ourselves to very slow keyboard presses to make sure
the guest always has enough chance to pick them up.

This patch adds trace events for when the guest polls for HID keyboard
events. That way we can be reasonably safely assume that the guest
handled that one key event and can type the next.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/input/hid.c         | 26 +++++++++++++++++++++++++-
 hw/input/trace-events  |  2 ++
 include/hw/input/hid.h |  6 ++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/input/hid.c b/hw/input/hid.c
index fa9cc4c..b68f597 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -228,7 +228,7 @@ static void hid_keyboard_event(DeviceState *dev, QemuConsole *src,
 {
     HIDState *hs = (HIDState *)dev;
     int scancodes[3], i, count;
-    int slot;
+    int slot = -1;
     InputKeyEvent *key = evt->u.key.data;
 
     count = qemu_input_key_value_to_scancode(key->key,
@@ -241,6 +241,13 @@ static void hid_keyboard_event(DeviceState *dev, QemuConsole *src,
     for (i = 0; i < count; i++) {
         slot = (hs->head + hs->n) & QUEUE_MASK; hs->n++;
         hs->kbd.keycodes[slot] = scancodes[i];
+        hs->kbd.poll_trigger[slot] = NULL;
+    }
+
+    if (slot != -1) {
+        hs->kbd.poll_trigger[slot] = g_new(HIDPollTrigger, 1);
+        hs->kbd.poll_trigger[slot]->key = *key->key;
+        hs->kbd.poll_trigger[slot]->down = key->down;
     }
     hs->event(hs);
 }
@@ -256,6 +263,23 @@ static void hid_keyboard_process_keycode(HIDState *hs)
     slot = hs->head & QUEUE_MASK; QUEUE_INCR(hs->head); hs->n--;
     keycode = hs->kbd.keycodes[slot];
 
+    /* Trace polled key events */
+    if (hs->kbd.poll_trigger[slot]) {
+        HIDPollTrigger *t = hs->kbd.poll_trigger[slot];
+        switch (t->key.type) {
+        case KEY_VALUE_KIND_NUMBER:
+            trace_hid_kbd_key_nr_polled(t->key.u.number.data, t->down);
+            break;
+        case KEY_VALUE_KIND_QCODE:
+            trace_hid_kbd_key_qt_polled(t->key.u.qcode.data, t->down);
+            break;
+        default:
+            break;
+        }
+        g_free(t);
+        hs->kbd.poll_trigger[slot] = NULL;
+    }
+
     key = keycode & 0x7f;
     index = key | ((hs->kbd.modifiers & (1 << 8)) >> 1);
     hid_code = hid_usage_keys[index];
diff --git a/hw/input/trace-events b/hw/input/trace-events
index f3bfbed..7a6b5ff 100644
--- a/hw/input/trace-events
+++ b/hw/input/trace-events
@@ -24,6 +24,8 @@ milkymist_softusb_pulse_irq(void) "Pulse IRQ"
 
 # hw/input/hid.c
 hid_kbd_queue_full(void) "queue full"
+hid_kbd_key_nr_polled(uint64_t value, bool down) "val %#"PRIx64" down %d"
+hid_kbd_key_qt_polled(uint64_t value, bool down) "val %#"PRIx64" down %d"
 
 # hw/input/virtio
 virtio_input_queue_full(void) "queue full"
diff --git a/include/hw/input/hid.h b/include/hw/input/hid.h
index 2127c7c..ff9541e 100644
--- a/include/hw/input/hid.h
+++ b/include/hw/input/hid.h
@@ -25,8 +25,14 @@ typedef struct HIDMouseState {
     int mouse_grabbed;
 } HIDMouseState;
 
+typedef struct HIDPollTrigger {
+    KeyValue key;
+    bool down;
+} HIDPollTrigger;
+
 typedef struct HIDKeyboardState {
     uint32_t keycodes[QUEUE_LENGTH];
+    HIDPollTrigger *poll_trigger[QUEUE_LENGTH];
     uint16_t modifiers;
     uint8_t leds;
     uint8_t key[16];
-- 
1.8.5.6

Re: [Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Gerd Hoffmann]

On Do, 2017-03-30 at 15:35 +0200, Alexander Graf wrote:
> When driving QEMU from the outside, we have basically no chance to
> determine how quickly the guest OS picks up key events, so we usually
> have to limit ourselves to very slow keyboard presses to make sure
> the guest always has enough chance to pick them up.
> 
> This patch adds trace events for when the guest polls for HID keyboard
> events. That way we can be reasonably safely assume that the guest
> handled that one key event and can type the next.

Hmm, I'm wondering why you are tracing the event details?  Input layer
has tracepoints for this already, so why duplicate this here?
Especially as you have to do quite some effort to store the data for
tracing?

For queue management (in automated testing I assume?) a simple "queue
empty" trace point should do the trick, no?  Or maybe a "queue has $i of
$n slots filled atm" trace points, if you wanna know how much you can
pass in without loosing events due to the queue being full.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Peter Maydell]

On 30 March 2017 at 14:35, Alexander Graf <agraf@suse.de> wrote:
> When driving QEMU from the outside, we have basically no chance to
> determine how quickly the guest OS picks up key events, so we usually
> have to limit ourselves to very slow keyboard presses to make sure
> the guest always has enough chance to pick them up.
>
> This patch adds trace events for when the guest polls for HID keyboard
> events. That way we can be reasonably safely assume that the guest
> handled that one key event and can type the next.

This feels like the wrong way to solve the problem to me.
Maybe we should have something like the network and
UART device models do where they can say "my queue is
full, I cannot accept any more data from the outside
world" ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Alexander Graf]

On 03/30/2017 04:00 PM, Gerd Hoffmann wrote:
> On Do, 2017-03-30 at 15:35 +0200, Alexander Graf wrote:
>> When driving QEMU from the outside, we have basically no chance to
>> determine how quickly the guest OS picks up key events, so we usually
>> have to limit ourselves to very slow keyboard presses to make sure
>> the guest always has enough chance to pick them up.
>>
>> This patch adds trace events for when the guest polls for HID keyboard
>> events. That way we can be reasonably safely assume that the guest
>> handled that one key event and can type the next.
> Hmm, I'm wondering why you are tracing the event details?  Input layer
> has tracepoints for this already, so why duplicate this here?
> Especially as you have to do quite some effort to store the data for
> tracing?

Because we need to know when the events are off the queue, not pushed 
onto the queue.

> For queue management (in automated testing I assume?) a simple "queue
> empty" trace point should do the trick, no?  Or maybe a "queue has $i of
> $n slots filled atm" trace points, if you wanna know how much you can
> pass in without loosing events due to the queue being full.

That's a very good point. In fact, that would make things much much 
easier for the other side. If we just know "queue is drained", we know 
we can send new events.


Alex

Re: [Qemu-devel] [PATCH] input: Add trace events for polled keyboard input

[Alexander Graf]

On 03/30/2017 04:10 PM, Peter Maydell wrote:
> On 30 March 2017 at 14:35, Alexander Graf <agraf@suse.de> wrote:
>> When driving QEMU from the outside, we have basically no chance to
>> determine how quickly the guest OS picks up key events, so we usually
>> have to limit ourselves to very slow keyboard presses to make sure
>> the guest always has enough chance to pick them up.
>>
>> This patch adds trace events for when the guest polls for HID keyboard
>> events. That way we can be reasonably safely assume that the guest
>> handled that one key event and can type the next.
> This feels like the wrong way to solve the problem to me.
> Maybe we should have something like the network and
> UART device models do where they can say "my queue is
> full, I cannot accept any more data from the outside
> world" ?

That's really hard to realize unfortunately. If events come in from VNC, 
how do you stall new events from arriving? Would you stop reading the 
VNC stream? Would you allocate random buffers until you manage to 
finally push data onto the HID queue again?

Keep in mind that HID events might deliberately not be interpreted. OVMF 
doesn't have to have a tablet driver usually for example, so your tablet 
queue will fill up as soon as you push any mouse movement information in.


Alex