[Qemu-devel] [PATCH 0/4] usb/hid: bugfixes, more on usb and hid split

[Qemu-devel] [PATCH 0/4] usb/hid: bugfixes, more on usb and hid split

[Michael Walle]

This USB patchset moves the VM state stuff from usb-hid.c to hid.c, so it
can be reused by other devices.

There is one major drawback: i need to increase the vmstate version_id of
the usb-hid device. I don't know if you agree with this change.
Alternatively, we could add a load_old function which just skips old
versions. 


Michael Walle (4):
  hid: register kbd hander in init()
  hid: introduce hid vmstate macros
  usb-hid: use hid vmstate macro
  milkymist-softusb: use hid code directly

 hw/hid.c               |   63 ++++++++++++++++++++++++-
 hw/hw.h                |   20 ++++++++
 hw/milkymist-softusb.c |  122 +++++++++++++++---------------------------------
 hw/usb-hid.c           |   51 +++-----------------
 4 files changed, 127 insertions(+), 129 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH 1/4] hid: register kbd hander in init()

[Michael Walle]

Register the keyboard event handler in hid's init() instead of its reset()
function.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/hid.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/hid.c b/hw/hid.c
index 7b5ef5f..6934f05 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -359,7 +359,6 @@ void hid_reset(HIDState *hs)
 {
     switch (hs->kind) {
     case HID_KEYBOARD:
-        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
         memset(hs->kbd.keycodes, 0, sizeof(hs->kbd.keycodes));
         memset(hs->kbd.key, 0, sizeof(hs->kbd.key));
         hs->kbd.keys = 0;
@@ -393,7 +392,9 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event)
     hs->kind = kind;
     hs->event = event;
 
-    if (hs->kind == HID_MOUSE) {
+    if (hs->kind == HID_KEYBOARD) {
+        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
+    } else if (hs->kind == HID_MOUSE) {
         hs->ptr.eh_entry = qemu_add_mouse_event_handler(hid_pointer_event, hs,
                                                         0, "QEMU HID Mouse");
     } else if (hs->kind == HID_TABLET) {
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/4] hid: introduce hid vmstate macros

[Michael Walle]

Add VMSTATE macros to describe a HIDState. Based on usb-hid.c descriptions.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/hid.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/hw.h  |   20 ++++++++++++++++++++
 2 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/hw/hid.c b/hw/hid.c
index 6934f05..90a9b3d 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -402,3 +402,61 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event)
                                                         1, "QEMU HID Tablet");
     }
 }
+
+static int hid_post_load(void *opaque, int version_id)
+{
+    HIDState *s = opaque;
+
+    if (s->idle) {
+        hid_set_next_idle(s, qemu_get_clock_ns(vm_clock));
+    }
+    return 0;
+}
+
+static const VMStateDescription vmstate_hid_ptr_queue = {
+    .name = "HIDPointerEventQueue",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(xdx, HIDPointerEvent),
+        VMSTATE_INT32(ydy, HIDPointerEvent),
+        VMSTATE_INT32(dz, HIDPointerEvent),
+        VMSTATE_INT32(buttons_state, HIDPointerEvent),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+const VMStateDescription vmstate_hid_ptr_device = {
+    .name = "HIDPointerDevice",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = hid_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_ARRAY(ptr.queue, HIDState, QUEUE_LENGTH, 0,
+                             vmstate_hid_ptr_queue, HIDPointerEvent),
+        VMSTATE_UINT32(head, HIDState),
+        VMSTATE_UINT32(n, HIDState),
+        VMSTATE_INT32(protocol, HIDState),
+        VMSTATE_UINT8(idle, HIDState),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+const VMStateDescription vmstate_hid_keyboard_device = {
+    .name = "HIDKeyboardDevice",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = hid_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(kbd.keycodes, HIDState, QUEUE_LENGTH),
+        VMSTATE_UINT16(kbd.modifiers, HIDState),
+        VMSTATE_UINT8(kbd.leds, HIDState),
+        VMSTATE_UINT8_ARRAY(kbd.key, HIDState, 16),
+        VMSTATE_INT32(kbd.keys, HIDState),
+        VMSTATE_UINT32(head, HIDState),
+        VMSTATE_UINT32(n, HIDState),
+        VMSTATE_INT32(protocol, HIDState),
+        VMSTATE_UINT8(idle, HIDState),
+        VMSTATE_END_OF_LIST(),
+    }
+};
diff --git a/hw/hw.h b/hw/hw.h
index df6ca65..a124da9 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -701,6 +701,26 @@ extern const VMStateDescription vmstate_ptimer;
     .offset     = vmstate_offset_pointer(_state, _field, ptimer_state), \
 }
 
+extern const VMStateDescription vmstate_hid_keyboard_device;
+
+#define VMSTATE_HID_KEYBOARD_DEVICE(_field, _state) {                \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(HIDState),                                  \
+    .vmsd       = &vmstate_hid_keyboard_device,                      \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, HIDState),    \
+}
+
+extern const VMStateDescription vmstate_hid_ptr_device;
+
+#define VMSTATE_HID_POINTER_DEVICE(_field, _state) {                 \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(HIDState),                                  \
+    .vmsd       = &vmstate_hid_ptr_device,                           \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, HIDState),    \
+}
+
 /* _f : field name
    _f_n : num of elements field_name
    _n : num of elements
-- 
1.7.2.5

[Qemu-devel] [PATCH 3/4] usb-hid: use hid vmstate macro

[Michael Walle]

Increase version id, this will make almost any saved vm incompatible :(

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/usb-hid.c |   51 ++++++++-------------------------------------------
 1 files changed, 8 insertions(+), 43 deletions(-)

diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index e5d57de..2efbc8d 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -538,61 +538,26 @@ void usb_hid_datain_cb(USBDevice *dev, void *opaque, void (*datain)(void *))
     s->datain = datain;
 }
 
-static int usb_hid_post_load(void *opaque, int version_id)
-{
-    USBHIDState *s = opaque;
-
-    if (s->hid.idle) {
-        hid_set_next_idle(&s->hid, qemu_get_clock_ns(vm_clock));
-    }
-    return 0;
-}
-
-static const VMStateDescription vmstate_usb_ptr_queue = {
-    .name = "usb-ptr-queue",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .fields = (VMStateField []) {
-        VMSTATE_INT32(xdx, HIDPointerEvent),
-        VMSTATE_INT32(ydy, HIDPointerEvent),
-        VMSTATE_INT32(dz, HIDPointerEvent),
-        VMSTATE_INT32(buttons_state, HIDPointerEvent),
-        VMSTATE_END_OF_LIST()
-    }
-};
 static const VMStateDescription vmstate_usb_ptr = {
     .name = "usb-ptr",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .post_load = usb_hid_post_load,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields = (VMStateField []) {
         VMSTATE_USB_DEVICE(dev, USBHIDState),
-        VMSTATE_STRUCT_ARRAY(hid.ptr.queue, USBHIDState, QUEUE_LENGTH, 0,
-                             vmstate_usb_ptr_queue, HIDPointerEvent),
-        VMSTATE_UINT32(hid.head, USBHIDState),
-        VMSTATE_UINT32(hid.n, USBHIDState),
-        VMSTATE_INT32(hid.protocol, USBHIDState),
-        VMSTATE_UINT8(hid.idle, USBHIDState),
+        VMSTATE_HID_POINTER_DEVICE(hid, USBHIDState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_usb_kbd = {
     .name = "usb-kbd",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .post_load = usb_hid_post_load,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields = (VMStateField []) {
         VMSTATE_USB_DEVICE(dev, USBHIDState),
-        VMSTATE_UINT32_ARRAY(hid.kbd.keycodes, USBHIDState, QUEUE_LENGTH),
-        VMSTATE_UINT32(hid.head, USBHIDState),
-        VMSTATE_UINT32(hid.n, USBHIDState),
-        VMSTATE_UINT16(hid.kbd.modifiers, USBHIDState),
-        VMSTATE_UINT8(hid.kbd.leds, USBHIDState),
-        VMSTATE_UINT8_ARRAY(hid.kbd.key, USBHIDState, 16),
-        VMSTATE_INT32(hid.kbd.keys, USBHIDState),
-        VMSTATE_INT32(hid.protocol, USBHIDState),
-        VMSTATE_UINT8(hid.idle, USBHIDState),
+        VMSTATE_HID_KEYBOARD_DEVICE(hid, USBHIDState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.2.5

[Qemu-devel] [PATCH 4/4] milkymist-softusb: use hid code directly

[Michael Walle]

Remove the dummy USB device and use the HID code directly. Use the HID code
for the mouse support, too.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/milkymist-softusb.c |  122 +++++++++++++++---------------------------------
 1 files changed, 38 insertions(+), 84 deletions(-)

diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
index 75c85ae..fe4eedb 100644
--- a/hw/milkymist-softusb.c
+++ b/hw/milkymist-softusb.c
@@ -25,7 +25,7 @@
 #include "sysbus.h"
 #include "trace.h"
 #include "console.h"
-#include "usb.h"
+#include "hid.h"
 #include "qemu-error.h"
 
 enum {
@@ -46,9 +46,8 @@ enum {
 
 struct MilkymistSoftUsbState {
     SysBusDevice busdev;
-    USBBus usbbus;
-    USBPort usbport[2];
-    USBDevice *usbdev;
+    HIDState hid_kbd;
+    HIDState hid_mouse;
 
     qemu_irq irq;
 
@@ -62,13 +61,10 @@ struct MilkymistSoftUsbState {
     uint32_t regs[R_MAX];
 
     /* mouse state */
-    int mouse_dx;
-    int mouse_dy;
-    int mouse_dz;
-    uint8_t mouse_buttons_state;
+    uint8_t mouse_hid_buffer[4];
 
     /* keyboard state */
-    uint8_t kbd_usb_buffer[8];
+    uint8_t kbd_hid_buffer[8];
 };
 typedef struct MilkymistSoftUsbState MilkymistSoftUsbState;
 
@@ -177,16 +173,10 @@ static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
 static void softusb_mouse_changed(MilkymistSoftUsbState *s)
 {
     uint8_t m;
-    uint8_t buf[4];
-
-    buf[0] = s->mouse_buttons_state;
-    buf[1] = s->mouse_dx;
-    buf[2] = s->mouse_dy;
-    buf[3] = s->mouse_dz;
 
     softusb_read_dmem(s, COMLOC_MEVT_PRODUCE, &m, 1);
     trace_milkymist_softusb_mevt(m);
-    softusb_write_dmem(s, COMLOC_MEVT_BASE + 4 * m, buf, 4);
+    softusb_write_dmem(s, COMLOC_MEVT_BASE + 4 * m, s->mouse_hid_buffer, 4);
     m = (m + 1) & 0xf;
     softusb_write_dmem(s, COMLOC_MEVT_PRODUCE, &m, 1);
 
@@ -200,7 +190,7 @@ static void softusb_kbd_changed(MilkymistSoftUsbState *s)
 
     softusb_read_dmem(s, COMLOC_KEVT_PRODUCE, &m, 1);
     trace_milkymist_softusb_kevt(m);
-    softusb_write_dmem(s, COMLOC_KEVT_BASE + 8 * m, s->kbd_usb_buffer, 8);
+    softusb_write_dmem(s, COMLOC_KEVT_BASE + 8 * m, s->kbd_hid_buffer, 8);
     m = (m + 1) & 0x7;
     softusb_write_dmem(s, COMLOC_KEVT_PRODUCE, &m, 1);
 
@@ -208,62 +198,42 @@ static void softusb_kbd_changed(MilkymistSoftUsbState *s)
     qemu_irq_pulse(s->irq);
 }
 
-static void softusb_mouse_event(void *opaque,
-       int dx, int dy, int dz, int buttons_state)
+static void softusb_kbd_hid_datain(HIDState *hs)
 {
-    MilkymistSoftUsbState *s = opaque;
+    MilkymistSoftUsbState *s = container_of(hs, MilkymistSoftUsbState, hid_kbd);
+    int len;
 
     /* if device is in reset, do nothing */
     if (s->regs[R_CTRL] & CTRL_RESET) {
         return;
     }
 
-    trace_milkymist_softusb_mouse_event(dx, dy, dz, buttons_state);
+    len = hid_keyboard_poll(hs, s->kbd_hid_buffer, sizeof(s->kbd_hid_buffer));
 
-    s->mouse_dx = dx;
-    s->mouse_dy = dy;
-    s->mouse_dz = dz;
-    s->mouse_buttons_state = buttons_state;
-
-    softusb_mouse_changed(s);
+    if (len == 8) {
+        softusb_kbd_changed(s);
+    }
 }
 
-static void softusb_usbdev_datain(void *opaque)
+static void softusb_mouse_hid_datain(HIDState *hs)
 {
-    MilkymistSoftUsbState *s = opaque;
-
-    USBPacket p;
-
-    usb_packet_init(&p);
-    usb_packet_setup(&p, USB_TOKEN_IN, 0, 1);
-    usb_packet_addbuf(&p, s->kbd_usb_buffer, sizeof(s->kbd_usb_buffer));
-    s->usbdev->info->handle_data(s->usbdev, &p);
-    usb_packet_cleanup(&p);
-
-    softusb_kbd_changed(s);
-}
+    MilkymistSoftUsbState *s =
+            container_of(hs, MilkymistSoftUsbState, hid_mouse);
+    int len;
 
-static void softusb_attach(USBPort *port)
-{
-}
+    /* if device is in reset, do nothing */
+    if (s->regs[R_CTRL] & CTRL_RESET) {
+        return;
+    }
 
-static void softusb_detach(USBPort *port)
-{
-}
+    len = hid_pointer_poll(hs, s->mouse_hid_buffer,
+            sizeof(s->mouse_hid_buffer));
 
-static void softusb_child_detach(USBPort *port, USBDevice *child)
-{
+    if (len == 4) {
+        softusb_mouse_changed(s);
+    }
 }
 
-static USBPortOps softusb_ops = {
-    .attach = softusb_attach,
-    .detach = softusb_detach,
-    .child_detach = softusb_child_detach,
-};
-
-static USBBusOps softusb_bus_ops = {
-};
-
 static void milkymist_softusb_reset(DeviceState *d)
 {
     MilkymistSoftUsbState *s =
@@ -273,11 +243,11 @@ static void milkymist_softusb_reset(DeviceState *d)
     for (i = 0; i < R_MAX; i++) {
         s->regs[i] = 0;
     }
-    s->mouse_dx = 0;
-    s->mouse_dy = 0;
-    s->mouse_dz = 0;
-    s->mouse_buttons_state = 0;
-    memset(s->kbd_usb_buffer, 0, sizeof(s->kbd_usb_buffer));
+    memset(s->kbd_hid_buffer, 0, sizeof(s->kbd_hid_buffer));
+    memset(s->mouse_hid_buffer, 0, sizeof(s->mouse_hid_buffer));
+
+    hid_reset(&s->hid_kbd);
+    hid_reset(&s->hid_mouse);
 
     /* defaults */
     s->regs[R_CTRL] = CTRL_RESET;
@@ -304,23 +274,8 @@ static int milkymist_softusb_init(SysBusDevice *dev)
     cpu_register_physical_memory(s->dmem_base, s->dmem_size,
             dmem_ram | IO_MEM_RAM);
 
-    qemu_add_mouse_event_handler(softusb_mouse_event, s, 0, "Milkymist Mouse");
-
-    /* create our usb bus */
-    usb_bus_new(&s->usbbus, &softusb_bus_ops, NULL);
-
-    /* our two ports */
-    /* FIXME: claim to support full speed devices. qemu mouse and keyboard
-     * report themselves as full speed devices. */
-    usb_register_port(&s->usbbus, &s->usbport[0], NULL, 0, &softusb_ops,
-            USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
-    usb_register_port(&s->usbbus, &s->usbport[1], NULL, 1, &softusb_ops,
-            USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
-
-    /* and finally create an usb keyboard */
-    s->usbdev = usb_create_simple(&s->usbbus, "usb-kbd");
-    usb_hid_datain_cb(s->usbdev, s, softusb_usbdev_datain);
-    s->usbdev->info->handle_reset(s->usbdev);
+    hid_init(&s->hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
+    hid_init(&s->hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
 
     return 0;
 }
@@ -332,11 +287,10 @@ static const VMStateDescription vmstate_milkymist_softusb = {
     .minimum_version_id_old = 1,
     .fields      = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, MilkymistSoftUsbState, R_MAX),
-        VMSTATE_INT32(mouse_dx, MilkymistSoftUsbState),
-        VMSTATE_INT32(mouse_dy, MilkymistSoftUsbState),
-        VMSTATE_INT32(mouse_dz, MilkymistSoftUsbState),
-        VMSTATE_UINT8(mouse_buttons_state, MilkymistSoftUsbState),
-        VMSTATE_BUFFER(kbd_usb_buffer, MilkymistSoftUsbState),
+        VMSTATE_HID_KEYBOARD_DEVICE(hid_kbd, MilkymistSoftUsbState),
+        VMSTATE_HID_POINTER_DEVICE(hid_mouse, MilkymistSoftUsbState),
+        VMSTATE_BUFFER(kbd_hid_buffer, MilkymistSoftUsbState),
+        VMSTATE_BUFFER(mouse_hid_buffer, MilkymistSoftUsbState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 0/4] usb/hid: bugfixes, more on usb and hid split

[Gerd Hoffmann]

On 08/07/11 19:29, Michael Walle wrote:
> This USB patchset moves the VM state stuff from usb-hid.c to hid.c, so it
> can be reused by other devices.

Nice.

> There is one major drawback: i need to increase the vmstate version_id of
> the usb-hid device. I don't know if you agree with this change.
> Alternatively, we could add a load_old function which just skips old
> versions.

You don't have to change the version.  The binary representation in the 
savevm format doesn't change if you move the fields from vmstate_usb_ptr 
to vmstate_hid_ptr_device.

For anything referenced via VMS_STRUCT the vmstate->name is just there 
to beautify the source code, it isn't used at all when reading/writing 
the state.  No new section, no nothing.  The savevm code only looks at 
->fields and writes them out (and executes the hooks).

Reordering the fields (like you did with the keyboard) isn't going to 
work though.

cheers,
   Gerd

[Qemu-devel] [PATCH 1/4] hid: register kbd hander in init()

[Michael Walle]

Register the keyboard event handler in hid's init() instead of its reset()
function.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/hid.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/hid.c b/hw/hid.c
index 7b5ef5f..6934f05 100644
--- a/hw/hid.c
+++ b/hw/hid.c
@@ -359,7 +359,6 @@ void hid_reset(HIDState *hs)
 {
     switch (hs->kind) {
     case HID_KEYBOARD:
-        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
         memset(hs->kbd.keycodes, 0, sizeof(hs->kbd.keycodes));
         memset(hs->kbd.key, 0, sizeof(hs->kbd.key));
         hs->kbd.keys = 0;
@@ -393,7 +392,9 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event)
     hs->kind = kind;
     hs->event = event;
 
-    if (hs->kind == HID_MOUSE) {
+    if (hs->kind == HID_KEYBOARD) {
+        qemu_add_kbd_event_handler(hid_keyboard_event, hs);
+    } else if (hs->kind == HID_MOUSE) {
         hs->ptr.eh_entry = qemu_add_mouse_event_handler(hid_pointer_event, hs,
                                                         0, "QEMU HID Mouse");
     } else if (hs->kind == HID_TABLET) {
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 1/4] hid: register kbd hander in init()

[Peter Maydell]

On 9 August 2011 22:54, Michael Walle <michael@walle.cc> wrote:
> Register the keyboard event handler in hid's init() instead of its reset()
> function.
>
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

There's actually an equivalent patch to this lurking in the
qemu-linaro patch stack (inherited from the meego tree).

If we fix qemu_add_kbd_event_handler() so that it actually
lets you register more than one keyboard at once (as you
can with mice) then this is definitely required, otherwise
the USB keyboard reregisters itself every time there's a
USB reset, and you get key duplication.
(There's a meego patch to add the multiple-keyboard support:
http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=41fc7075cfd74524acc2db73a48534e6bfbea8c4
but that's still in my "to clean up" list so it contains some
changes which should be in a separate patch.)

-- PMM