[Qemu-devel] [PATCH] virtio-input: implement pass-through evdev writes

[Qemu-devel] [PATCH] virtio-input: implement pass-through evdev writes

[Ladi Prosek]

The write path for pass-through devices, commonly used for controlling
keyboard LEDs via EV_LED, was not implemented. This commit adds the
necessary plumbing to connect the status virtio queue to the host evdev
file descriptor.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
Most of the new code has to do with handling EAGAIN/EWOULDBLOCK. I don't
believe that this will be happening in practise for local evdev devices.
Of course, all bets are off if fd is an arbitrary file, but we would be
running into issues anyway (right now, if virtio_input_host_event doesn't
read all of sizeof(struct virtio_input_event) bytes, we get out of sync
with the event stream). So part of me would just do one best-effort
write and ignore all errors. I'm curious what you think. 

 hw/input/virtio-input-host.c     | 85 ++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-input.h |  8 ++++
 2 files changed, 93 insertions(+)

diff --git a/hw/input/virtio-input-host.c b/hw/input/virtio-input-host.c
index 36856d6..bc02d3b 100644
--- a/hw/input/virtio-input-host.c
+++ b/hw/input/virtio-input-host.c
@@ -42,6 +42,55 @@ static void virtio_input_host_event(void *opaque)
     }
 }
 
+static int virtio_input_host_write_event(VirtIOInputHost *vih,
+                                         struct timeval *time,
+                                         struct virtio_input_event *virtio)
+{
+    struct input_event evdev;
+    int rc;
+
+    evdev.time = *time;
+    evdev.type = le16_to_cpu(virtio->type);
+    evdev.code = le16_to_cpu(virtio->code);
+    evdev.value = le32_to_cpu(virtio->value);
+
+    rc = write(vih->fd, &evdev, sizeof(evdev));
+    if (rc == -1) {
+        rc = errno;
+        if (rc == EAGAIN || rc == EWOULDBLOCK) {
+            return EAGAIN;
+        }
+        perror("virtio_input_host_write_event: write");
+        return rc;
+    }
+    if (rc != sizeof(evdev)) {
+        return EINVAL;
+    }
+    return 0;
+}
+
+static void virtio_input_writable(void *opaque)
+{
+    VirtIOInputHost *vih = opaque;
+    VirtIOInputEvent *vie;
+    int rc;
+
+    while (!QSIMPLEQ_EMPTY(&vih->pending_writes)) {
+        vie = QSIMPLEQ_FIRST(&vih->pending_writes);
+
+        rc = virtio_input_host_write_event(vih, &vie->time, &vie->virtio);
+        if (rc == EAGAIN) {
+            break;
+        }
+        QSIMPLEQ_REMOVE_HEAD(&vih->pending_writes, next);
+        g_free(vie);
+    }
+
+    if (QSIMPLEQ_EMPTY(&vih->pending_writes)) {
+        qemu_set_fd_handler(vih->fd, virtio_input_host_event, NULL, vih);
+    }
+}
+
 static void virtio_input_bits_config(VirtIOInputHost *vih,
                                      int type, int count)
 {
@@ -127,6 +176,8 @@ static void virtio_input_host_realize(DeviceState *dev, Error **errp)
     virtio_input_bits_config(vih, EV_LED, LED_CNT);
 
     qemu_set_fd_handler(vih->fd, virtio_input_host_event, NULL, vih);
+
+    QSIMPLEQ_INIT(&vih->pending_writes);
     return;
 
 err_close:
@@ -138,11 +189,44 @@ err_close:
 static void virtio_input_host_unrealize(DeviceState *dev, Error **errp)
 {
     VirtIOInputHost *vih = VIRTIO_INPUT_HOST(dev);
+    VirtIOInputEvent *vie;
 
     if (vih->fd > 0) {
         qemu_set_fd_handler(vih->fd, NULL, NULL, NULL);
         close(vih->fd);
     }
+
+    while (!QSIMPLEQ_EMPTY(&vih->pending_writes)) {
+        vie = QSIMPLEQ_FIRST(&vih->pending_writes);
+        QSIMPLEQ_REMOVE_HEAD(&vih->pending_writes, next);
+        g_free(vie);
+    }
+}
+
+static void virtio_input_host_handle_status(VirtIOInput *vinput,
+                                            virtio_input_event *event)
+{
+    VirtIOInputHost *vih = VIRTIO_INPUT_HOST(vinput);
+    struct timeval time;
+    int rc;
+
+    if (gettimeofday(&time, NULL)) {
+        perror("virtio_input_host_handle_status: gettimeofday");
+        return;
+    }
+
+    rc = virtio_input_host_write_event(vih, &time, event);
+    if (rc == EAGAIN) {
+        VirtIOInputEvent *vie = g_malloc(sizeof(*vie));
+        vie->time = time;
+        vie->virtio = *event;
+
+        if (QSIMPLEQ_EMPTY(&vih->pending_writes)) {
+            qemu_set_fd_handler(vih->fd, virtio_input_host_event,
+                                virtio_input_writable, vih);
+        }
+        QSIMPLEQ_INSERT_TAIL(&vih->pending_writes, vie, next);
+    }
 }
 
 static const VMStateDescription vmstate_virtio_input_host = {
@@ -164,6 +248,7 @@ static void virtio_input_host_class_init(ObjectClass *klass, void *data)
     dc->props          = virtio_input_host_properties;
     vic->realize       = virtio_input_host_realize;
     vic->unrealize     = virtio_input_host_unrealize;
+    vic->handle_status = virtio_input_host_handle_status;
 }
 
 static void virtio_input_host_init(Object *obj)
diff --git a/include/hw/virtio/virtio-input.h b/include/hw/virtio/virtio-input.h
index af1c207..d41d650 100644
--- a/include/hw/virtio/virtio-input.h
+++ b/include/hw/virtio/virtio-input.h
@@ -61,6 +61,7 @@ typedef struct VirtIOInputClass VirtIOInputClass;
 typedef struct VirtIOInputConfig VirtIOInputConfig;
 typedef struct VirtIOInputHID VirtIOInputHID;
 typedef struct VirtIOInputHost VirtIOInputHost;
+typedef struct VirtIOInputEvent VirtIOInputEvent;
 
 struct VirtIOInputConfig {
     virtio_input_config               config;
@@ -93,6 +94,12 @@ struct VirtIOInputClass {
     void (*handle_status)(VirtIOInput *vinput, virtio_input_event *event);
 };
 
+struct VirtIOInputEvent {
+    struct timeval                    time;
+    virtio_input_event                virtio;
+    QSIMPLEQ_ENTRY(VirtIOInputEvent)  next;
+};
+
 struct VirtIOInputHID {
     VirtIOInput                       parent_obj;
     char                              *display;
@@ -106,6 +113,7 @@ struct VirtIOInputHost {
     VirtIOInput                       parent_obj;
     char                              *evdev;
     int                               fd;
+    QSIMPLEQ_HEAD(pending_writes, VirtIOInputEvent) pending_writes;
 };
 
 void virtio_input_send(VirtIOInput *vinput, virtio_input_event *event);
-- 
2.5.5

Re: [Qemu-devel] [PATCH] virtio-input: implement pass-through evdev writes

[Gerd Hoffmann]

On Fr, 2016-04-01 at 11:31 +0200, Ladi Prosek wrote:
> The write path for pass-through devices, commonly used for controlling
> keyboard LEDs via EV_LED, was not implemented. This commit adds the
> necessary plumbing to connect the status virtio queue to the host evdev
> file descriptor.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
> Most of the new code has to do with handling EAGAIN/EWOULDBLOCK. I don't
> believe that this will be happening in practise for local evdev devices.

Agree.

> Of course, all bets are off if fd is an arbitrary file,

It can't be, it would fail the evdev ioctls.

> with the event stream). So part of me would just do one best-effort
> write and ignore all errors. I'm curious what you think. 

Just do best-effort sounds reasonable to me.

The code to handle EGAIN will just bitrot b/c it will never be used (and
I guess it is untested too, or did you manage to provoke EGAIN somehow?)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] virtio-input: implement pass-through evdev writes

[Ladi Prosek]

On Fri, Apr 1, 2016 at 12:20 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Fr, 2016-04-01 at 11:31 +0200, Ladi Prosek wrote:
>> The write path for pass-through devices, commonly used for controlling
>> keyboard LEDs via EV_LED, was not implemented. This commit adds the
>> necessary plumbing to connect the status virtio queue to the host evdev
>> file descriptor.
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>> Most of the new code has to do with handling EAGAIN/EWOULDBLOCK. I don't
>> believe that this will be happening in practise for local evdev devices.
>
> Agree.
>
>> Of course, all bets are off if fd is an arbitrary file,
>
> It can't be, it would fail the evdev ioctls.

Ah!

>> with the event stream). So part of me would just do one best-effort
>> write and ignore all errors. I'm curious what you think.
>
> Just do best-effort sounds reasonable to me.

Will do, thanks.

> The code to handle EGAIN will just bitrot b/c it will never be used (and
> I guess it is untested too, or did you manage to provoke EGAIN somehow?)

if (rand() & 1) {
    return EAGAIN;
}

> cheers,
>   Gerd
>