Skip to contentSkip to conversation

PATCH: QEMU support for UHCI suspend / remote wake up

PATCH In review Marcelo Tosatti 4 participants 21 messages Nov 25, 2010 – Dec 1, 2010 KVM

lore
Share & tools
lore

Marcelo Tosatti

author
#0 lore raw

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

To enable autosuspend for the USB tablet on Linux guests:

echo auto > /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level

It reduces CPU consumption of an idle FC12 guest from 2.7% to 0.3%.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-hid.c +3 -0
index 882d933..b7a4dc1 100644
@@ -412,6 +412,9 @@ static void usb_hid_changed(USBHIDState *hs)
 
     if (hs->datain)
         hs->datain(hs->datain_opaque);
+
+    if (hs->dev.remote_wakeup)
+        usb_remote_wakeup(&hs->dev);
 }
 
 static void usb_mouse_event(void *opaque,
hw/usb-uhci.c +40 -0
index 1d83400..674cb0c 100644
@@ -59,6 +59,7 @@
 
 #define UHCI_PORT_RESET (1 << 9)
 #define UHCI_PORT_LSDA  (1 << 8)
+#define UHCI_PORT_RD    (1 << 6)
 #define UHCI_PORT_ENC   (1 << 3)
 #define UHCI_PORT_EN    (1 << 2)
 #define UHCI_PORT_CSC   (1 << 1)

@@ -501,6 +502,7 @@ static void uhci_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
             port->ctrl = (port->ctrl & 0x01fb) | (val & ~0x01fb);
             /* some bits are reset when a '1' is written to them */
             port->ctrl &= ~(val & 0x000a);
+            port->ctrl &= ~(port->ctrl & 0x0040); /* clear port resume detected */
         }
         break;
     }

@@ -593,6 +595,43 @@ static void uhci_resume (void *opaque)
     }
 }
 
+static UHCIPort *find_port(UHCIState *s, USBDevice *d)
+{
+    int i;
+
+    for (i = 0; i < NB_PORTS; i++) {
+        UHCIPort *port = &s->ports[i];
+
+        if (d == port->port.dev) {
+            return port;
+        }
+    }
+
+    return NULL;
+}
+
+static void uhci_event(USBDevice *dev)
+{
+    USBBus *bus = usb_bus_from_device(dev);
+    UHCIState *s = container_of(bus, UHCIState, bus);
+
+    if (s->cmd & UHCI_CMD_EGSM) {
+        UHCIPort *port = find_port(s, dev);
+
+        if (!port) {
+            return;
+        }
+
+        if (port->ctrl & UHCI_PORT_RD) {
+            return;
+        }
+
+        port->ctrl |= UHCI_PORT_RD;
+
+        uhci_resume(s);
+    }
+}
+
 static void uhci_attach(USBPort *port1, USBDevice *dev)
 {
     UHCIState *s = port1->opaque;

@@ -602,6 +641,7 @@ static void uhci_attach(USBPort *port1, USBDevice *dev)
         if (port->port.dev) {
             usb_attach(port1, NULL);
         }
+        dev->info->remote_wakeup_cb = uhci_event;
         /* set connect status */
         port->ctrl |= UHCI_PORT_CCS | UHCI_PORT_CSC;
hw/usb.c +6 -0
index a326bcf..9b24d49 100644
@@ -229,3 +229,9 @@ void usb_send_msg(USBDevice *dev, int msg)
 
     /* This _must_ be synchronous */
 }
+
+void usb_remote_wakeup(USBDevice *dev)
+{
+    if (dev->info->remote_wakeup_cb)
+        dev->info->remote_wakeup_cb(dev);
+}
hw/usb.h +6 -0
index 00d2802..16de1c9 100644
@@ -189,6 +189,11 @@ struct USBDeviceInfo {
      */
     int (*handle_data)(USBDevice *dev, USBPacket *p);
 
+    /*
+     * Process remote wakeup request.
+     */
+    void (*remote_wakeup_cb)(USBDevice *dev);
+
     const char *product_desc;
 
     /* handle legacy -usbdevice command line options */

@@ -317,6 +322,7 @@ void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
+void usb_remote_wakeup(USBDevice *dev);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
 {
#1 lore raw

Hi,

Quoted 7 lines
hw/usb-uhci.c +1 -0
+        dev->info->remote_wakeup_cb = uhci_event;
hw/usb.h +0 -0
index 00d2802..16de1c9 100644

hw/usb.h struct USBDeviceInfo { · line 196

Quoted 8 lines
hw/usb.h +5 -0
@@ -189,6 +189,11 @@ struct USBDeviceInfo {
       */
      int (*handle_data)(USBDevice *dev, USBPacket *p);
 
+    /*
+     * Process remote wakeup request.
+     */
+    void (*remote_wakeup_cb)(USBDevice *dev);
+

No way. DeviceInfo holds informations about the device *implementation*. Multiple instances will share that, placing runtime data there is just plain wrong.

Also this is a callback from the usb device to the usb host adapter, not the other way around. I'd suggest to create a USBBusOps for host adapter callbacks and and stick it into USBBus.

cheers,
Gerd

author
#2 lore raw

cover USB UHCI global suspend / remote wakeup

See patch 2 for details.

v2:
- Add remote wakeup callback in USBBus, as suggested by Gerd.

author
#3 lore raw

PATCH 1/2 add USBBusOps to USBBus

Needed for remote wakeup notification.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-bus.c +2 -1
Index: qemu-kvm/hw/usb-bus.c
===================================================================
@@ -14,11 +14,12 @@ static struct BusInfo usb_bus_info = {
 static int next_usb_bus = 0;
 static QTAILQ_HEAD(, USBBus) busses = QTAILQ_HEAD_INITIALIZER(busses);
 
-void usb_bus_new(USBBus *bus, DeviceState *host)
+void usb_bus_new(USBBus *bus, DeviceState *host, USBBusOps *ops)
 {
     qbus_create_inplace(&bus->qbus, &usb_bus_info, host, NULL);
     bus->busnr = next_usb_bus++;
     bus->qbus.allow_hotplug = 1; /* Yes, we can */
+    bus->ops = ops;
     QTAILQ_INIT(&bus->free);
     QTAILQ_INIT(&bus->used);
     QTAILQ_INSERT_TAIL(&busses, bus, next);
hw/usb.c +9 -0
Index: qemu-kvm/hw/usb.c
===================================================================
@@ -229,3 +229,12 @@ void usb_send_msg(USBDevice *dev, int ms
 
     /* This _must_ be synchronous */
 }
+
+void usb_remote_wakeup(USBDevice *dev)
+{
+    USBBus *bus = usb_bus_from_device(dev);
+
+    if (bus->ops && bus->ops->remote_wakeup_cb) {
+        bus->ops->remote_wakeup_cb(bus, dev);
+    }
+}
hw/usb.h +11 -1
Index: qemu-kvm/hw/usb.h
===================================================================
@@ -123,6 +123,7 @@
 #define USB_ENDPOINT_XFER_INT		3
 
 typedef struct USBBus USBBus;
+typedef struct USBBusOps USBBusOps;
 typedef struct USBPort USBPort;
 typedef struct USBDevice USBDevice;
 typedef struct USBDeviceInfo USBDeviceInfo;

@@ -294,8 +295,16 @@ void musb_set_size(MUSBState *s, int epn
 
 /* usb-bus.c */
 
+struct USBBusOps {
+    /*
+     * Process remote wakeup request.
+     */
+    void (*remote_wakeup_cb)(USBBus *bus, USBDevice *dev);
+};
+
 struct USBBus {
     BusState qbus;
+    USBBusOps *ops;
     int busnr;
     int nfree;
     int nused;

@@ -304,7 +313,7 @@ struct USBBus {
     QTAILQ_ENTRY(USBBus) next;
 };
 
-void usb_bus_new(USBBus *bus, DeviceState *host);
+void usb_bus_new(USBBus *bus, DeviceState *host, USBBusOps *ops);
 USBBus *usb_bus_find(int busnr);
 void usb_qdev_register(USBDeviceInfo *info);
 void usb_qdev_register_many(USBDeviceInfo *info);

@@ -317,6 +326,7 @@ void usb_unregister_port(USBBus *bus, US
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
+void usb_remote_wakeup(USBDevice *dev);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
 {
hw/usb-musb.c +1 -1
Index: qemu-kvm/hw/usb-musb.c
===================================================================
@@ -342,7 +342,7 @@ struct MUSBState {
         s->ep[i].epnum = i;
     }
 
-    usb_bus_new(&s->bus, NULL /* FIXME */);
+    usb_bus_new(&s->bus, NULL /* FIXME */, NULL);
     usb_register_port(&s->bus, &s->port, s, 0, musb_attach);
 
     return s;
hw/usb-ohci.c +1 -1
Index: qemu-kvm/hw/usb-ohci.c
===================================================================
@@ -1702,7 +1702,7 @@ static void usb_ohci_init(OHCIState *ohc
 
     ohci->name = dev->info->name;
 
-    usb_bus_new(&ohci->bus, dev);
+    usb_bus_new(&ohci->bus, dev, NULL);
     ohci->num_ports = num_ports;
     for (i = 0; i < num_ports; i++) {
         usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, ohci_attach);
hw/usb-uhci.c +1 -1
Index: qemu-kvm/hw/usb-uhci.c
===================================================================
@@ -1113,7 +1113,7 @@ static int usb_uhci_common_initfn(UHCISt
     pci_conf[PCI_INTERRUPT_PIN] = 4; // interrupt pin 3
     pci_conf[0x60] = 0x10; // release number
 
-    usb_bus_new(&s->bus, &s->dev.qdev);
+    usb_bus_new(&s->bus, &s->dev.qdev, NULL);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i].port, s, i, uhci_attach);
     }
author
#4 lore raw

PATCH 2/2 support for UHCI suspend / remote wake up

In reply to Marcelo Tosatti

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

To enable autosuspend for the USB tablet on Linux guests:

echo auto > /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level

It reduces CPU consumption of an idle FC12 guest from 2.7% to 0.3%.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-hid.c +3 -0
Index: qemu-kvm/hw/usb-hid.c
===================================================================
@@ -412,6 +412,9 @@ static void usb_hid_changed(USBHIDState 
 
     if (hs->datain)
         hs->datain(hs->datain_opaque);
+
+    if (hs->dev.remote_wakeup)
+        usb_remote_wakeup(&hs->dev);
 }
 
 static void usb_mouse_event(void *opaque,
hw/usb-uhci.c +42 -1

73 lines not on this page · raw message

#5 lore raw

Re: [patch 2/2] support for UHCI suspend / remote wake up

Quoted 7 lines · Marcelo Tosatti

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

To enable autosuspend for the USB tablet on Linux guests:

echo auto> /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level

Hmm, did you ever got this working sanely?

/me sees bus disconnects in the guest ...

Quoted 3 lines
              port->ctrl&= ~(val&  0x000a);
+            port->ctrl&= ~(port->ctrl&  0x0040); /* clear port resume detected */
          }

This chunk looks suspicious ...

I suspect the port suspend/resume emulation isn't complete.

/me goes debugging,
Gerd

author
#6 lore raw

Re: [patch 2/2] support for UHCI suspend / remote wake up

Quoted 10 lines · Gerd Hoffmann
Quoted 7 lines · Marcelo Tosatti

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

To enable autosuspend for the USB tablet on Linux guests:

echo auto> /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level

Hmm, did you ever got this working sanely?

Yes. Linux and Windows.

Quoted 1 line

/me sees bus disconnects in the guest ...

I was seeing bus disconnects when not clearing port resume bit properly.

Quoted 10 lines
Quoted 3 lines
             port->ctrl&= ~(val&  0x000a);
+            port->ctrl&= ~(port->ctrl&  0x0040); /* clear port resume detected */
         }

This chunk looks suspicious ...

I suspect the port suspend/resume emulation isn't complete.

/me goes debugging,
Gerd

CONFIG_USB_DEBUG helps.

#7 lore raw

Re: [patch 2/2] support for UHCI suspend / remote wake up

Quoted 9 lines

I was seeing bus disconnects when not clearing port resume bit properly.

Quoted 7 lines
Quoted 3 lines
              port->ctrl&= ~(val&   0x000a);
+            port->ctrl&= ~(port->ctrl&   0x0040); /* clear port resume detected */
          }

This chunk looks suspicious ...

I suspect the port suspend/resume emulation isn't complete.

The bug is that the port resume bit is masked out from guest writes, so the guest hasn't a chance to clear it ...

cheers,
Gerd

PS: http://cgit.freedesktop.org/spice/qemu/log/?h=usb.1

#8 lore raw

Re: [patch 0/2] USB UHCI global suspend / remote wakeup

In reply to Marcelo Tosatti
Quoted 3 lines

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

I'm pretty sure this is wrong. Suspend/resume works based on physical topology, i.e. the resume notification should go to the the port/hub to which the device is connected, not directly to the host controller.

Paul

author
#9 lore raw

Re: [Qemu-devel] [patch 0/2] USB UHCI global suspend / remote wakeup

Quoted 7 lines · Paul Brook
Quoted 3 lines

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

I'm pretty sure this is wrong. Suspend/resume works based on physical topology, i.e. the resume notification should go to the the port/hub to which the device is connected, not directly to the host controller.

If the host controller is in global suspend state, and resume is received on any of its root hub ports (given that remote wakeup is enabled for the given port), the system will be interrupted if interrupt enable bit is set.

2.1.2 USB STATUS REGISTER
I/O Address: Base + (02-03h)
Default Value: 0000h
Attribute: Read/Write Clear
size: 16 bits
This register indicates pending interrupts and various states of the Host Controller

Resume Detect. The Host Controller sets this bit to 1 when it receives a “RESUME” signal from a USB device. This is only valid if the Host Controller is in a global suspend state (bit 3 of Command register = 1).

2.1.7.1 Behavior Under Global or Selective Suspend Scenarios

Resume will be recognized in the USBSTS register (bit 2) if resume is received on a suspended or enabled port when the Host Controller is in the global suspend state (USBCMD register bit 3 is set).

4.2.1 RESUME RECEIVED
This event indicates that the HC received a RESUME signal from a device on the USB during a global suspend. If
this interrupt is enabled in the HC Interrupt Enable register, a hardware interrupt will be signaled to the system
allowing the USB to be brought out of the suspend state and returned to normal operation.

You are correct in that USB HUB emulation does not propagate resume, but this does not make this patch incorrect.

#10 lore raw

Re: [Qemu-devel] [patch 0/2] USB UHCI global suspend / remote wakeup

Quoted 11 lines · Marcelo Tosatti
Quoted 7 lines · Paul Brook
Quoted 3 lines

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

I'm pretty sure this is wrong. Suspend/resume works based on physical topology, i.e. the resume notification should go to the the port/hub to which the device is connected, not directly to the host controller.

You are correct in that USB HUB emulation does not propagate resume, but this does not make this patch incorrect.

Well, it does. When the notification is port based our software model should better reflect that, so we have the chance to add resume propagation to the hub emulation later on.

I guess the Ops should be moved from the USBBus to the USBPort to reflect that. This way the hub emulation and the uhci root hub can have different callbacks, which is needed to get this correct.

cheers,
Gerd

#11 lore raw

Re: [Qemu-devel] [patch 0/2] USB UHCI global suspend / remote wakeup

Quoted 10 lines · Marcelo Tosatti
Quoted 9 lines · Paul Brook
Quoted 7 lines
Quoted 3 lines

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

I'm pretty sure this is wrong. Suspend/resume works based on physical topology, i.e. the resume notification should go to the the port/hub to which the device is connected, not directly to the host controller.

Quoted 6 lines
Quoted 2 lines

You are correct in that USB HUB emulation does not propagate resume, but this does not make this patch incorrect.

Well, it does. When the notification is port based our software model should better reflect that, so we have the chance to add resume propagation to the hub emulation later on.

Exactly. The patch assumes the device is connected to a root hub port. This assumption is incorrect.

The device should be sending the resume signal to the port/hub to which it is connected. If that hub is still active it will reactivate the port, and flag a port change notification in the normal manner. If the hub is also suspended it will propagate the resume notification upstream (which may or may not be the root hub).

Paul

author
#13 lore raw

PATCH 1/3 add USBPortOps to USBPort

Needed for remote wakeup notification.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-bus.c +2 -2
Index: qemu-kvm/hw/usb-bus.c
===================================================================
@@ -110,11 +110,11 @@ USBDevice *usb_create_simple(USBBus *bus
 }
 
 void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
-                       usb_attachfn attach)
+                       USBPortOps *ops)
 {
     port->opaque = opaque;
     port->index = index;
-    port->attach = attach;
+    port->ops = ops;
     QTAILQ_INSERT_TAIL(&bus->free, port, next);
     bus->nfree++;
 }
hw/usb.c +1 -1
Index: qemu-kvm/hw/usb.c
===================================================================
@@ -28,7 +28,7 @@
 
 void usb_attach(USBPort *port, USBDevice *dev)
 {
-    port->attach(port, dev);
+    port->ops->attach(port, dev);
 }
 
 /**********************/
hw/usb.h +6 -3
Index: qemu-kvm/hw/usb.h
===================================================================
@@ -124,6 +124,7 @@
 
 typedef struct USBBus USBBus;
 typedef struct USBPort USBPort;
+typedef struct USBPortOps USBPortOps;
 typedef struct USBDevice USBDevice;
 typedef struct USBDeviceInfo USBDeviceInfo;
 typedef struct USBPacket USBPacket;

@@ -196,12 +197,14 @@ struct USBDeviceInfo {
     USBDevice *(*usbdevice_init)(const char *params);
 };
 
-typedef void (*usb_attachfn)(USBPort *port, USBDevice *dev);
+struct USBPortOps {
+    void (*attach)(USBPort *port, USBDevice *dev);
+};
 
 /* USB port on which a device can be connected */
 struct USBPort {
     USBDevice *dev;
-    usb_attachfn attach;
+    USBPortOps *ops;
     void *opaque;
     int index; /* internal port index, may be used with the opaque */
     QTAILQ_ENTRY(USBPort) next;

@@ -312,7 +315,7 @@ USBDevice *usb_create(USBBus *bus, const
 USBDevice *usb_create_simple(USBBus *bus, const char *name);
 USBDevice *usbdevice_create(const char *cmdline);
 void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
-                       usb_attachfn attach);
+                       USBPortOps *ops);
 void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
hw/usb-musb.c +5 -1
Index: qemu-kvm/hw/usb-musb.c
===================================================================
@@ -261,6 +261,10 @@
 
 static void musb_attach(USBPort *port, USBDevice *dev);
 
+static USBPortOps musb_port_ops = {
+    .attach = musb_attach,
+};
+
 typedef struct {
     uint16_t faddr[2];
     uint8_t haddr[2];

@@ -343,7 +347,7 @@ struct MUSBState {
     }
 
     usb_bus_new(&s->bus, NULL /* FIXME */);
-    usb_register_port(&s->bus, &s->port, s, 0, musb_attach);
+    usb_register_port(&s->bus, &s->port, s, 0, &musb_port_ops);
 
     return s;
 }
hw/usb-ohci.c +5 -1
Index: qemu-kvm/hw/usb-ohci.c
===================================================================
@@ -1676,6 +1676,10 @@ static CPUWriteMemoryFunc * const ohci_w
     ohci_mem_write
 };
 
+static USBPortOps ohci_port_ops = {
+    .attach = ohci_attach,
+};
+
 static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
                           int num_ports, uint32_t localmem_base)
 {

@@ -1705,7 +1709,7 @@ static void usb_ohci_init(OHCIState *ohc
     usb_bus_new(&ohci->bus, dev);
     ohci->num_ports = num_ports;
     for (i = 0; i < num_ports; i++) {
-        usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, ohci_attach);
+        usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, &ohci_port_ops);
     }
 
     ohci->async_td = 0;
hw/usb-uhci.c +5 -1
Index: qemu-kvm/hw/usb-uhci.c
===================================================================
@@ -1101,6 +1101,10 @@ static void uhci_map(PCIDevice *pci_dev,
     register_ioport_read(addr, 32, 1, uhci_ioport_readb, s);
 }
 
+static USBPortOps uhci_port_ops = {
+    .attach = uhci_attach,
+};
+
 static int usb_uhci_common_initfn(UHCIState *s)
 {
     uint8_t *pci_conf = s->dev.config;

@@ -1115,7 +1119,7 @@ static int usb_uhci_common_initfn(UHCISt
 
     usb_bus_new(&s->bus, &s->dev.qdev);
     for(i = 0; i < NB_PORTS; i++) {
-        usb_register_port(&s->bus, &s->ports[i].port, s, i, uhci_attach);
+        usb_register_port(&s->bus, &s->ports[i].port, s, i, &uhci_port_ops);
     }
     s->frame_timer = qemu_new_timer(vm_clock, uhci_frame_timer, s);
     s->expire_time = qemu_get_clock(vm_clock) +
hw/usb-hub.c +5 -1
Index: qemu-kvm/hw/usb-hub.c
===================================================================
@@ -524,6 +524,10 @@ static void usb_hub_handle_destroy(USBDe
     }
 }
 
+static USBPortOps hub_port_ops = {
+    .attach = usb_hub_attach,
+};
+
 static int usb_hub_initfn(USBDevice *dev)
 {
     USBHubState *s = DO_UPCAST(USBHubState, dev, dev);

@@ -535,7 +539,7 @@ static int usb_hub_initfn(USBDevice *dev
     for (i = 0; i < s->nb_ports; i++) {
         port = &s->ports[i];
         usb_register_port(usb_bus_from_device(dev),
-                          &port->port, s, i, usb_hub_attach);
+                          &port->port, s, i, &hub_port_ops);
         port->wPortStatus = PORT_STAT_POWER;
         port->wPortChange = 0;
     }
author
#15 lore raw

PATCH 2/3 support for UHCI suspend / remote wake up

In reply to Marcelo Tosatti

This patch enables USB UHCI global suspend/resume feature. The OS will stop the HC once all ports are suspended. If there is activity on the port(s), an interrupt signalling remote wakeup will be triggered.

To enable autosuspend for the USB tablet on Linux guests:

echo auto > /sys/devices/pci0000:00/0000:00:01.2/usb1/1-1/power/level

It reduces CPU consumption of an idle FC12 guest from 2.7% to 0.3%.

To enable autosuspend in Windows:

1) Enable "Allow the computer to turn off this
device to save power" checkbox in "Power Management" tab of USB Root Hub properties.

2) Create a REG_BINARY (XP) or REG_DWORD (Vista/2008) key named SelectiveSuspendEnabled under:

HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Enum\USB\VID_0627&PID_0001\1\Device Parameters

With a value of 1.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-hid.c +3 -0
Index: qemu-kvm/hw/usb-hid.c
===================================================================
@@ -412,6 +412,9 @@ static void usb_hid_changed(USBHIDState 
 
     if (hs->datain)
         hs->datain(hs->datain_opaque);
+
+    if (hs->dev.remote_wakeup)
+        usb_remote_wakeup(&hs->dev);
 }
 
 static void usb_mouse_event(void *opaque,
hw/usb-uhci.c +20 -0
Index: qemu-kvm/hw/usb-uhci.c
===================================================================
@@ -59,6 +59,7 @@
 
 #define UHCI_PORT_RESET (1 << 9)
 #define UHCI_PORT_LSDA  (1 << 8)
+#define UHCI_PORT_RD    (1 << 6)
 #define UHCI_PORT_ENC   (1 << 3)
 #define UHCI_PORT_EN    (1 << 2)
 #define UHCI_PORT_CSC   (1 << 1)

@@ -501,6 +502,7 @@ static void uhci_ioport_writew(void *opa
             port->ctrl = (port->ctrl & 0x01fb) | (val & ~0x01fb);
             /* some bits are reset when a '1' is written to them */
             port->ctrl &= ~(val & 0x000a);
+            port->ctrl &= ~(port->ctrl & 0x0040); /* clear port resume detected */
         }
         break;
     }

@@ -593,6 +595,23 @@ static void uhci_resume (void *opaque)
     }
 }
 
+static void uhci_event(USBPort *port, USBDevice *dev)
+{
+    USBBus *bus = usb_bus_from_device(dev);
+    UHCIState *s = container_of(bus, UHCIState, bus);
+
+    if (s->cmd & UHCI_CMD_EGSM) {
+        UHCIPort *uport = DO_UPCAST(UHCIPort, port, port);
+
+        if (uport->ctrl & UHCI_PORT_RD) {
+            return;
+        }
+
+        uport->ctrl |= UHCI_PORT_RD;
+        uhci_resume(s);
+    }
+}
+
 static void uhci_attach(USBPort *port1, USBDevice *dev)
 {
     UHCIState *s = port1->opaque;

@@ -1103,6 +1122,7 @@ static void uhci_map(PCIDevice *pci_dev,
 
 static USBPortOps uhci_port_ops = {
     .attach = uhci_attach,
+    .remote_wakeup = uhci_event,
 };
 
 static int usb_uhci_common_initfn(UHCIState *s)
hw/usb.h +2 -0
Index: qemu-kvm/hw/usb.h
===================================================================
@@ -199,6 +199,7 @@ struct USBDeviceInfo {
 
 struct USBPortOps {
     void (*attach)(USBPort *port, USBDevice *dev);
+    void (*remote_wakeup)(USBPort *port, USBDevice *dev);
 };
 
 /* USB port on which a device can be connected */

@@ -253,6 +254,7 @@ static inline void usb_cancel_packet(USB
 }
 
 void usb_attach(USBPort *port, USBDevice *dev);
+void usb_remote_wakeup(USBDevice *dev);
 int usb_generic_handle_packet(USBDevice *s, USBPacket *p);
 int set_usb_string(uint8_t *buf, const char *str);
 void usb_send_msg(USBDevice *dev, int msg);
hw/usb.c +15 -0
Index: qemu-kvm/hw/usb.c
===================================================================
@@ -31,6 +31,21 @@ void usb_attach(USBPort *port, USBDevice
     port->ops->attach(port, dev);
 }
 
+void usb_remote_wakeup(USBDevice *dev)
+{
+    USBBus *bus = usb_bus_from_device(dev);
+    USBPort *port;
+
+    QTAILQ_FOREACH(port, &bus->used, next) {
+        if (port->dev == dev)
+            break;
+    }
+    assert(port != NULL);
+
+    if (port->ops->remote_wakeup)
+        port->ops->remote_wakeup(port, dev);
+}
+
 /**********************/
 
 /* generic USB device helpers (you are not forced to use them when
#16 lore raw

[Qemu-devel] Re: [patch 2/3] support for UHCI suspend / remote wake up

Hi,

Quoted 4 lines
              port->ctrl = (port->ctrl&  0x01fb) | (val&  ~0x01fb);
              /* some bits are reset when a '1' is written to them */
              port->ctrl&= ~(val&  0x000a);

+ port->ctrl&= ~(port->ctrl& 0x0040); /* clear port resume detected */

Removing the port resume bit from the readonly mask works better than the clear-on-any-write hack ;)

cheers,
Gerd

author
#17 lore raw

PATCH 3/3 UHCI: Substate section for migration of remote wakeup feature

In reply to Marcelo Tosatti

Use a subsection to migrate remote wakeup feature only when used by the guest.

Signed-off-by Marcelo Tosatti <[email protected]>
hw/usb-uhci.c +41 -0
Index: qemu-kvm/hw/usb-uhci.c
===================================================================
@@ -363,6 +363,39 @@ static void uhci_pre_save(void *opaque)
     uhci_async_cancel_all(s);
 }
 
+static bool uhci_port_wakeup_state_needed(void *opaque)
+{
+    UHCIPort *port = opaque;
+
+    if (port->port.dev) {
+        return port->port.dev->remote_wakeup;
+    }
+
+    return false;
+}
+
+static int uhci_port_wakeup_post_load(void *opaque, int version_id)
+{
+    UHCIPort *port = opaque;
+
+    if (port->port.dev) {
+        port->port.dev->remote_wakeup = 1;
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_uhci_wakeup_state = {
+    .name = "uhci port/wakeup",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .post_load = uhci_port_wakeup_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_uhci_port = {
     .name = "uhci port",
     .version_id = 1,

@@ -371,6 +404,14 @@ static const VMStateDescription vmstate_
     .fields      = (VMStateField []) {
         VMSTATE_UINT16(ctrl, UHCIPort),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_uhci_wakeup_state,
+            .needed = uhci_port_wakeup_state_needed,
+        }, {
+            /* empty */
+        }
     }
 };
#18 lore raw

[Qemu-devel] Re: [patch 3/3] UHCI: Substate section for migration of remote wakeup feature

Quoted 1 line · Marcelo Tosatti

Use a subsection to migrate remote wakeup feature only when used by the guest.

Yea, right, this must be saved. But certainly not by the UHCI adapter, it belongs into the usb devices. Some of them don't save state at all today, which makes it a bit tricky to do this in a way that migration to older versions keeps working at least as long as remote wakeup isn't used ...

Juan, any idea?

cheers,
Gerd

#19 lore raw

[Qemu-devel] Re: [patch 3/3] UHCI: Substate section for migration of remote wakeup feature

Quoted 10 lines · Gerd Hoffmann
Quoted 1 line · Marcelo Tosatti

Use a subsection to migrate remote wakeup feature only when used by the guest.

Yea, right, this must be saved. But certainly not by the UHCI adapter, it belongs into the usb devices. Some of them don't save state at all today, which makes it a bit tricky to do this in a way that migration to older versions keeps working at least as long as remote wakeup isn't used ...

Juan, any idea?

Second thought.

Only to add sections for all devices that don't have one. But the problem is that we have optional subsections, not optional subsections (it is not a way to not send a section because no value there).

Later, Juan.

author
#20 lore raw

[Qemu-devel] Re: [patch 3/3] UHCI: Substate section for migration of remote wakeup feature

Quoted 12 lines · Juan Quintela
Quoted 10 lines · Gerd Hoffmann
Quoted 1 line · Marcelo Tosatti

Use a subsection to migrate remote wakeup feature only when used by the guest.

Yea, right, this must be saved. But certainly not by the UHCI adapter, it belongs into the usb devices. Some of them don't save state at all today, which makes it a bit tricky to do this in a way that migration to older versions keeps working at least as long as remote wakeup isn't used ...

Juan, any idea?

Quoted 7 lines

Second thought.

Only to add sections for all devices that don't have one. But the problem is that we have optional subsections, not optional subsections (it is not a way to not send a section because no value there).

Later, Juan.

Which breaks migration for older versions. Which lowers the applicability of s/r.

Yes, remote_wakeup does not belong to UHCI, but i dont see another solution. Or break migration upstream and use the hack for vendors.