[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