[Qemu-devel] [USB patches for 1.3]: Lower cpuload when using the usb-tablet

[Qemu-devel] [USB patches for 1.3]: Lower cpuload when using the usb-tablet

[Hans de Goede]

And here then is my "piece de resistance" (at least for the last few weeks),
my final version of the patchset to lower the cpuload when using the
usb-tablet device, or for that matter when using any usb devices with
interrupt eps connected to the ehci controller.

This final version is actually quite simple, and since the high cpuload
with the usb-tablet is an often heared complaint I would like to advocate
that this is a bugfix and thus belongs in 1.3, I hope you will agree and
send this to Anthony for inclusion into 1.3 :)

Regards,

Hans

[Qemu-devel] [PATCH 1/5] usb: Call wakeup when data becomes available for all devices with int eps

[Hans de Goede]

This is necessary for proper interaction with the xhci controller, and it
will allow other hcds to lower there frame timer while waiting for interrupt
data.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-hub.c     | 2 ++
 hw/usb/dev-network.c | 7 +++++++
 hw/usb/dev-wacom.c   | 4 ++++
 hw/usb/redirect.c    | 4 ++++
 4 files changed, 17 insertions(+)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 9ee60dd..470fbbb 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -184,6 +184,7 @@ static void usb_hub_detach(USBPort *port1)
         port->wPortStatus &= ~PORT_STAT_ENABLE;
         port->wPortChange |= PORT_STAT_C_ENABLE;
     }
+    usb_wakeup(s->intr);
 }
 
 static void usb_hub_child_detach(USBPort *port1, USBDevice *child)
@@ -363,6 +364,7 @@ static void usb_hub_handle_control(USBDevice *dev, USBPacket *p,
                     port->wPortChange |= PORT_STAT_C_RESET;
                     /* set enable bit */
                     port->wPortStatus |= PORT_STAT_ENABLE;
+                    usb_wakeup(s->intr);
                 }
                 break;
             case PORT_POWER:
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 14d9e5a..30cb033 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -639,6 +639,8 @@ typedef struct USBNetState {
     unsigned int in_ptr, in_len;
     uint8_t in_buf[2048];
 
+    USBEndpoint *intr;
+
     char usbstring_mac[13];
     NICState *nic;
     NICConf conf;
@@ -851,6 +853,10 @@ static void *rndis_queue_response(USBNetState *s, unsigned int length)
     struct rndis_response *r =
             g_malloc0(sizeof(struct rndis_response) + length);
 
+    if (QTAILQ_EMPTY(&s->rndis_resp)) {
+        usb_wakeup(s->intr);
+    }
+
     QTAILQ_INSERT_TAIL(&s->rndis_resp, r, entries);
     r->length = length;
 
@@ -1349,6 +1355,7 @@ static int usb_net_initfn(USBDevice *dev)
     s->media_state = 0;	/* NDIS_MEDIA_STATE_CONNECTED */;
     s->filter = 0;
     s->vendorid = 0x1234;
+    s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     s->nic = qemu_new_nic(&net_usbnet_info, &s->conf,
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index 08b416d..f7342b0 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -43,6 +43,7 @@
 
 typedef struct USBWacomState {
     USBDevice dev;
+    USBEndpoint *intr;
     QEMUPutMouseEntry *eh_entry;
     int dx, dy, dz, buttons_state;
     int x, y;
@@ -137,6 +138,7 @@ static void usb_mouse_event(void *opaque,
     s->dz += dz1;
     s->buttons_state = buttons_state;
     s->changed = 1;
+    usb_wakeup(s->intr);
 }
 
 static void usb_wacom_event(void *opaque,
@@ -150,6 +152,7 @@ static void usb_wacom_event(void *opaque,
     s->dz += dz;
     s->buttons_state = buttons_state;
     s->changed = 1;
+    usb_wakeup(s->intr);
 }
 
 static inline int int_clamp(int val, int vmin, int vmax)
@@ -337,6 +340,7 @@ static int usb_wacom_initfn(USBDevice *dev)
     USBWacomState *s = DO_UPCAST(USBWacomState, dev, dev);
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
+    s->intr = usb_ep_get(dev, USB_TOKEN_IN, 1);
     s->changed = 1;
     return 0;
 }
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 15f394c..a4d324f 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1667,6 +1667,10 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id,
             return;
         }
 
+        if (QTAILQ_EMPTY(&dev->endpoint[EP2I(ep)].bufpq)) {
+            usb_wakeup(usb_ep_get(&dev->dev, USB_TOKEN_IN, ep & 0x0f));
+        }
+
         /* bufp_alloc also adds the packet to the ep queue */
         bufp_alloc(dev, data, data_len, interrupt_packet->status, ep);
     } else {
-- 
1.8.0

[Qemu-devel] [PATCH 2/5] usb: Don't allow USB_RET_ASYNC for interrupt packets

[Hans de Goede]

It is tempting to use USB_RET_ASYNC for interrupt packets, rather then the
current NAK + polling approach, but this causes issues for migration, as
an async completed packet will not getting written back to guest memory until
the next poll time, and if a migration happens in between it will get lost!

Make an exception for host devices, because:
1) host-linux actually uses async completion for interrupt endpoints
2) host devices don't migrate anyways

Ideally we would convert host-linux.c to handle (input) interrupt endpoints in
a buffered manner like it does for isoc endpoints, keeping multiple urbs
submitted to ensure the devices timing requirements are met, as well as making
its interrupt ep handling the same as other usb-devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h            | 1 +
 hw/usb/core.c       | 4 ++++
 hw/usb/host-bsd.c   | 1 +
 hw/usb/host-linux.c | 1 +
 4 files changed, 7 insertions(+)

diff --git a/hw/usb.h b/hw/usb.h
index 7d6de69..58f812f 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -197,6 +197,7 @@ struct USBEndpoint {
 
 enum USBDeviceFlags {
     USB_DEV_FLAG_FULL_PATH,
+    USB_DEV_FLAG_IS_HOST,
 };
 
 /* definition of a USB device */
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 52b5310..8e360d3 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -406,7 +406,11 @@ void usb_handle_packet(USBDevice *dev, USBPacket *p)
     if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
         usb_process_one(p);
         if (p->status == USB_RET_ASYNC) {
+            /* hcd drivers cannot handle async for isoc */
             assert(p->ep->type != USB_ENDPOINT_XFER_ISOC);
+            /* using async for interrupt packets breaks migration */
+            assert(p->ep->type != USB_ENDPOINT_XFER_INT ||
+                   (dev->flags & USB_DEV_FLAG_IS_HOST));
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
         } else if (p->status == USB_RET_ADD_TO_QUEUE) {
diff --git a/hw/usb/host-bsd.c b/hw/usb/host-bsd.c
index 6473e8b..dae0009 100644
--- a/hw/usb/host-bsd.c
+++ b/hw/usb/host-bsd.c
@@ -292,6 +292,7 @@ static void usb_host_handle_destroy(USBDevice *opaque)
 
 static int usb_host_initfn(USBDevice *dev)
 {
+    dev->flags |= (1 << USB_DEV_FLAG_IS_HOST);
     return 0;
 }
 
diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c
index aa77b77..bdafb6b 100644
--- a/hw/usb/host-linux.c
+++ b/hw/usb/host-linux.c
@@ -1476,6 +1476,7 @@ static int usb_host_initfn(USBDevice *dev)
 {
     USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
 
+    dev->flags |= (1 << USB_DEV_FLAG_IS_HOST);
     dev->auto_attach = 0;
     s->fd = -1;
     s->hub_fd = -1;
-- 
1.8.0

[Qemu-devel] [PATCH 3/5] usb: Allow overriding of usb_desc at the device level

[Hans de Goede]

This allows devices to present a different set of descriptors based on
device properties.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb.h     | 1 +
 hw/usb/bus.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/hw/usb.h b/hw/usb.h
index 58f812f..268e653 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -230,6 +230,7 @@ struct USBDevice {
     USBEndpoint ep_out[USB_MAX_ENDPOINTS];
 
     QLIST_HEAD(, USBDescString) strings;
+    const USBDesc *usb_desc; /* Overrides class usb_desc if not NULL */
     const USBDescDevice *device;
 
     int configuration;
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 99aac7a..fe26998 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -166,6 +166,9 @@ const char *usb_device_get_product_desc(USBDevice *dev)
 const USBDesc *usb_device_get_usb_desc(USBDevice *dev)
 {
     USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
+    if (dev->usb_desc) {
+        return dev->usb_desc;
+    }
     return klass->usb_desc;
 }
 
-- 
1.8.0

[Qemu-devel] [PATCH 4/5] ehci: Lower timer freq when the periodic schedule is idle

[Hans de Goede]

Lower the timer freq if no iso schedule packets complete for 64 frames in
a row.

We can safely do this, without adding latency, because:
1) If there is isoc traffic this will never trigger
2) For async handled interrupt packets (only usb-host), the completion handler
   will immediately schedule the frame_timer from a bh
3) All devices using NAK to signal no data for interrupt endpoints now use
   wakeup, which will immediately schedule the frame_timer from a bh

The advantage of this is that when we only have interrupt packets in the
periodic schedule, async_stepdown can do its work and significantly lower
the frequency at which the frame_timer runs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 39 +++++++++++++++++++++++++++++++++------
 hw/usb/hcd-ehci.h |  1 +
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 7df8e21..7536837 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -114,6 +114,7 @@
 #define BUFF_SIZE        5*4096   // Max bytes to transfer per transaction
 #define MAX_QH           100      // Max allowable queue heads in a chain
 #define MIN_FR_PER_TICK  3        // Min frames to process when catching up
+#define PERIODIC_ACTIVE  64
 
 /*  Internal periodic / asynchronous schedule state machine states
  */
@@ -738,6 +739,19 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
     return 0;
 }
 
+static void ehci_wakeup_endpoint(USBBus *bus, USBEndpoint *ep)
+{
+    EHCIState *s = container_of(bus, EHCIState, bus);
+    uint32_t portsc = s->portsc[ep->dev->port->index];
+
+    if (portsc & PORTSC_POWNER) {
+        return;
+    }
+
+    s->periodic_sched_active = PERIODIC_ACTIVE;
+    qemu_bh_schedule(s->async_bh);
+}
+
 static USBDevice *ehci_find_device(EHCIState *ehci, uint8_t addr)
 {
     USBDevice *dev;
@@ -1188,9 +1202,10 @@ static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
     trace_usb_ehci_packet_action(p->queue, p, "wakeup");
     p->async = EHCI_ASYNC_FINISHED;
 
-    if (p->queue->async) {
-        qemu_bh_schedule(p->queue->ehci->async_bh);
+    if (!p->queue->async) {
+        s->periodic_sched_active = PERIODIC_ACTIVE;
     }
+    qemu_bh_schedule(s->async_bh);
 }
 
 static void ehci_execute_complete(EHCIQueue *q)
@@ -1344,6 +1359,8 @@ static int ehci_process_itd(EHCIState *ehci,
     uint32_t i, len, pid, dir, devaddr, endp;
     uint32_t pg, off, ptr1, ptr2, max, mult;
 
+    ehci->periodic_sched_active = PERIODIC_ACTIVE;
+
     dir =(itd->bufptr[1] & ITD_BUFPTR_DIRECTION);
     devaddr = get_field(itd->bufptr[0], ITD_BUFPTR_DEVADDR);
     endp = get_field(itd->bufptr[0], ITD_BUFPTR_EP);
@@ -2033,6 +2050,9 @@ static void ehci_advance_state(EHCIState *ehci, int async)
         case EST_WRITEBACK:
             assert(q != NULL);
             again = ehci_state_writeback(q);
+            if (!async) {
+                ehci->periodic_sched_active = PERIODIC_ACTIVE;
+            }
             break;
 
         default:
@@ -2198,7 +2218,6 @@ static void ehci_frame_timer(void *opaque)
 
     if (ehci_periodic_enabled(ehci) || ehci->pstate != EST_INACTIVE) {
         need_timer++;
-        ehci->async_stepdown = 0;
 
         if (frames > ehci->maxframes) {
             skipped_frames = frames - ehci->maxframes;
@@ -2222,18 +2241,25 @@ static void ehci_frame_timer(void *opaque)
                     break;
                 }
             }
+            if (ehci->periodic_sched_active) {
+                ehci->periodic_sched_active--;
+            }
             ehci_update_frindex(ehci, 1);
             ehci_advance_periodic_state(ehci);
             ehci->last_run_ns += FRAME_TIMER_NS;
         }
     } else {
-        if (ehci->async_stepdown < ehci->maxframes / 2) {
-            ehci->async_stepdown++;
-        }
+        ehci->periodic_sched_active = 0;
         ehci_update_frindex(ehci, frames);
         ehci->last_run_ns += FRAME_TIMER_NS * frames;
     }
 
+    if (ehci->periodic_sched_active) {
+        ehci->async_stepdown = 0;
+    } else if (ehci->async_stepdown < ehci->maxframes / 2) {
+        ehci->async_stepdown++;
+    }
+
     /*  Async is not inside loop since it executes everything it can once
      *  called
      */
@@ -2301,6 +2327,7 @@ static USBPortOps ehci_port_ops = {
 
 static USBBusOps ehci_bus_ops = {
     .register_companion = ehci_register_companion,
+    .wakeup_endpoint = ehci_wakeup_endpoint,
 };
 
 static int usb_ehci_post_load(void *opaque, int version_id)
diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index d8078f4..772870b 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -311,6 +311,7 @@ struct EHCIState {
 
     uint64_t last_run_ns;
     uint32_t async_stepdown;
+    uint32_t periodic_sched_active;
     bool int_req_by_async;
 };
 
-- 
1.8.0

[Qemu-devel] [PATCH 5/5] usb-tablet: Allow connecting to ehci

[Hans de Goede]

Our ehci code has is capable of significantly lowering the wakeup rate
for the hcd emulation while the device is idle. It is possible to add
similar code ot the uhci emulation, but that simply is not there atm,
and there is no reason why a (virtual) usb-tablet can not be a USB-2 device.

Making usb-hid devices connect to the emulated ehci controller instead
of the emulated uhci controller on vms which have both lowers the cpuload
for a fully idle vm from 20% to 2-3% (on my laptop).

An alternative implementation to using a property to select the tablet
type, would be simply making it a new device type, ie usb-tablet2, but the
downside of that is that this will require libvirt changes to be available
through libvirt at all, and then management tools changes to become the
default for new vms, where as using a property will automatically get
any pc-1.3 type vms the lower cpuload.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/pc_piix.c     |  4 +++
 hw/usb/dev-hid.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index cfa839c..3754e21 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -386,6 +386,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver   = "usb-tablet",\
+            .property = "usb_version",\
+            .value    = stringify(1),\
         }
 
 static QEMUMachine pc_machine_v1_2 = {
diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 55266b1..8749128 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -46,6 +46,7 @@ typedef struct USBHIDState {
     USBDevice dev;
     USBEndpoint *intr;
     HIDState hid;
+    uint32_t usb_version;
 } USBHIDState;
 
 enum {
@@ -131,6 +132,36 @@ static const USBDescIface desc_iface_tablet = {
     },
 };
 
+static const USBDescIface desc_iface_tablet2 = {
+    .bInterfaceNumber              = 0,
+    .bNumEndpoints                 = 1,
+    .bInterfaceClass               = USB_CLASS_HID,
+    .bInterfaceProtocol            = 0x02,
+    .ndesc                         = 1,
+    .descs = (USBDescOther[]) {
+        {
+            /* HID descriptor */
+            .data = (uint8_t[]) {
+                0x09,          /*  u8  bLength */
+                USB_DT_HID,    /*  u8  bDescriptorType */
+                0x01, 0x00,    /*  u16 HID_class */
+                0x00,          /*  u8  country_code */
+                0x01,          /*  u8  num_descriptors */
+                USB_DT_REPORT, /*  u8  type: Report */
+                74, 0,         /*  u16 len */
+            },
+        },
+    },
+    .eps = (USBDescEndpoint[]) {
+        {
+            .bEndpointAddress      = USB_DIR_IN | 0x01,
+            .bmAttributes          = USB_ENDPOINT_XFER_INT,
+            .wMaxPacketSize        = 8,
+            .bInterval             = 4, /* 2 ^ (4-1) * 125 usecs = 1 ms */
+        },
+    },
+};
+
 static const USBDescIface desc_iface_keyboard = {
     .bInterfaceNumber              = 0,
     .bNumEndpoints                 = 1,
@@ -196,6 +227,23 @@ static const USBDescDevice desc_device_tablet = {
     },
 };
 
+static const USBDescDevice desc_device_tablet2 = {
+    .bcdUSB                        = 0x0200,
+    .bMaxPacketSize0               = 64,
+    .bNumConfigurations            = 1,
+    .confs = (USBDescConfig[]) {
+        {
+            .bNumInterfaces        = 1,
+            .bConfigurationValue   = 1,
+            .iConfiguration        = STR_CONFIG_TABLET,
+            .bmAttributes          = 0xa0,
+            .bMaxPower             = 50,
+            .nif = 1,
+            .ifs = &desc_iface_tablet2,
+        },
+    },
+};
+
 static const USBDescDevice desc_device_keyboard = {
     .bcdUSB                        = 0x0100,
     .bMaxPacketSize0               = 8,
@@ -239,6 +287,20 @@ static const USBDesc desc_tablet = {
     .str  = desc_strings,
 };
 
+static const USBDesc desc_tablet2 = {
+    .id = {
+        .idVendor          = 0x0627,
+        .idProduct         = 0x0001,
+        .bcdDevice         = 0,
+        .iManufacturer     = STR_MANUFACTURER,
+        .iProduct          = STR_PRODUCT_TABLET,
+        .iSerialNumber     = STR_SERIALNUMBER,
+    },
+    .full = &desc_device_tablet,
+    .high = &desc_device_tablet2,
+    .str  = desc_strings,
+};
+
 static const USBDesc desc_keyboard = {
     .id = {
         .idVendor          = 0x0627,
@@ -508,6 +570,21 @@ static int usb_hid_initfn(USBDevice *dev, int kind)
 
 static int usb_tablet_initfn(USBDevice *dev)
 {
+    USBHIDState *us = DO_UPCAST(USBHIDState, dev, dev);
+
+    switch (us->usb_version) {
+    case 1:
+        dev->usb_desc = &desc_tablet;
+        break;
+    case 2:
+        dev->usb_desc = &desc_tablet2;
+        break;
+    default:
+        error_report("Invalid usb version %d for usb-tabler (must be 1 or 2)",
+                     us->usb_version);
+        return -1;
+    }
+
     return usb_hid_initfn(dev, HID_TABLET);
 }
 
@@ -562,8 +639,14 @@ static void usb_hid_class_initfn(ObjectClass *klass, void *data)
     uc->handle_control = usb_hid_handle_control;
     uc->handle_data    = usb_hid_handle_data;
     uc->handle_destroy = usb_hid_handle_destroy;
+    uc->handle_attach  = usb_desc_attach;
 }
 
+static Property usb_tablet_properties[] = {
+        DEFINE_PROP_UINT32("usb_version", USBHIDState, usb_version, 2),
+        DEFINE_PROP_END_OF_LIST(),
+};
+
 static void usb_tablet_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -572,8 +655,8 @@ static void usb_tablet_class_initfn(ObjectClass *klass, void *data)
     usb_hid_class_initfn(klass, data);
     uc->init           = usb_tablet_initfn;
     uc->product_desc   = "QEMU USB Tablet";
-    uc->usb_desc       = &desc_tablet;
     dc->vmsd = &vmstate_usb_ptr;
+    dc->props = usb_tablet_properties;
 }
 
 static TypeInfo usb_tablet_info = {
-- 
1.8.0