[Qemu-devel] [PATCH 00/10] usb: a bunch of bugfixes.

[Qemu-devel] [PATCH 00/10] usb: a bunch of bugfixes.

[Gerd Hoffmann]

  Hi,

Result of the most recent usb debugging session.  Some tracing
improvements and bugfixes for xhci, uas and the usb hub.

please review,
  Gerd

Gerd Hoffmann (10):
  xhci: remove leftover debug printf
  xhci: add tracepoint for endpoint state changes
  xhci: add port to slot_address tracepoint
  xhci: fix endpoint interval calculation
  xhci: emulate intr endpoint intervals correctly
  xhci: reset port when disabling slot
  uas: add property for request logging
  usb: parallelize usb3 streams
  usb-hub: add tracepoint for status reports
  Revert "usb-hub: report status changes only once"

 hw/usb/core.c     |  7 +++---
 hw/usb/dev-hub.c  |  7 ++----
 hw/usb/dev-uas.c  | 15 +++++++++---
 hw/usb/hcd-xhci.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++---------
 trace-events      |  4 +++-
 5 files changed, 80 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/10] xhci: remove leftover debug printf

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index be6b86e..57c06e7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1164,8 +1164,6 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
 
     if (sctx->sct == -1) {
         xhci_dma_read_u32s(epctx->xhci, sctx->pctx, ctx, sizeof(ctx));
-        fprintf(stderr, "%s: init sctx #%d @ " DMA_ADDR_FMT ": %08x %08x\n",
-                __func__, streamid, sctx->pctx, ctx[0], ctx[1]);
         sct = (ctx[0] >> 1) & 0x07;
         if (epctx->lsa && sct != 1) {
             *cc_error = CC_INVALID_STREAM_TYPE_ERROR;
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/10] xhci: add tracepoint for endpoint state changes

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 19 +++++++++++++++++++
 trace-events      |  1 +
 2 files changed, 20 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 57c06e7..83161b9 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -586,6 +586,14 @@ static const char *TRBCCode_names[] = {
     [CC_SPLIT_TRANSACTION_ERROR]       = "CC_SPLIT_TRANSACTION_ERROR",
 };
 
+static const char *ep_state_names[] = {
+    [EP_DISABLED] = "disabled",
+    [EP_RUNNING]  = "running",
+    [EP_HALTED]   = "halted",
+    [EP_STOPPED]  = "stopped",
+    [EP_ERROR]    = "error",
+};
+
 static const char *lookup_name(uint32_t index, const char **list, uint32_t llen)
 {
     if (index >= llen || list[index] == NULL) {
@@ -606,6 +614,12 @@ static const char *event_name(XHCIEvent *event)
                        ARRAY_SIZE(TRBCCode_names));
 }
 
+static const char *ep_state_name(uint32_t state)
+{
+    return lookup_name(state, ep_state_names,
+                       ARRAY_SIZE(ep_state_names));
+}
+
 static uint64_t xhci_mfindex_get(XHCIState *xhci)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -1203,6 +1217,11 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
     }
 
     xhci_dma_write_u32s(xhci, epctx->pctx, ctx, sizeof(ctx));
+    if (epctx->state != state) {
+        trace_usb_xhci_ep_state(epctx->slotid, epctx->epid,
+                                ep_state_name(epctx->state),
+                                ep_state_name(state));
+    }
     epctx->state = state;
 }
 
diff --git a/trace-events b/trace-events
index 3856b5c..eb8eaef 100644
--- a/trace-events
+++ b/trace-events
@@ -381,6 +381,7 @@ usb_xhci_ep_set_dequeue(uint32_t slotid, uint32_t epid, uint32_t streamid, uint6
 usb_xhci_ep_kick(uint32_t slotid, uint32_t epid, uint32_t streamid) "slotid %d, epid %d, streamid %d"
 usb_xhci_ep_stop(uint32_t slotid, uint32_t epid) "slotid %d, epid %d"
 usb_xhci_ep_reset(uint32_t slotid, uint32_t epid) "slotid %d, epid %d"
+usb_xhci_ep_state(uint32_t slotid, uint32_t epid, const char *os, const char *ns) "slotid %d, epid %d, %s -> %s"
 usb_xhci_xfer_start(void *xfer, uint32_t slotid, uint32_t epid, uint32_t streamid) "%p: slotid %d, epid %d, streamid %d"
 usb_xhci_xfer_async(void *xfer) "%p"
 usb_xhci_xfer_nak(void *xfer) "%p"
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/10] xhci: add port to slot_address tracepoint

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 +-
 trace-events      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 83161b9..4d693bc 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2135,7 +2135,6 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
     int i;
     TRBCCode res;
 
-    trace_usb_xhci_slot_address(slotid);
     assert(slotid >= 1 && slotid <= xhci->numslots);
 
     dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
@@ -2168,6 +2167,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, unsigned int slotid,
         fprintf(stderr, "xhci: port not found\n");
         return CC_TRB_ERROR;
     }
+    trace_usb_xhci_slot_address(slotid, uport->path);
 
     dev = uport->dev;
     if (!dev) {
diff --git a/trace-events b/trace-events
index eb8eaef..f849807 100644
--- a/trace-events
+++ b/trace-events
@@ -371,7 +371,7 @@ usb_xhci_port_link(uint32_t port, uint32_t pls) "port %d, pls %d"
 usb_xhci_port_notify(uint32_t port, uint32_t pls) "port %d, bits %x"
 usb_xhci_slot_enable(uint32_t slotid) "slotid %d"
 usb_xhci_slot_disable(uint32_t slotid) "slotid %d"
-usb_xhci_slot_address(uint32_t slotid) "slotid %d"
+usb_xhci_slot_address(uint32_t slotid, const char *port) "slotid %d, port %s"
 usb_xhci_slot_configure(uint32_t slotid) "slotid %d"
 usb_xhci_slot_evaluate(uint32_t slotid) "slotid %d"
 usb_xhci_slot_reset(uint32_t slotid) "slotid %d"
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/10] xhci: fix endpoint interval calculation

[Gerd Hoffmann]

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4d693bc..3826979 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1274,7 +1274,7 @@ static void xhci_init_epctx(XHCIEPContext *epctx,
         epctx->ring.ccs = ctx[2] & 1;
     }
 
-    epctx->interval = 1 << (ctx[0] >> 16) & 0xff;
+    epctx->interval = 1 << ((ctx[0] >> 16) & 0xff);
 }
 
 static TRBCCode xhci_enable_ep(XHCIState *xhci, unsigned int slotid,
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/10] xhci: emulate intr endpoint intervals correctly

[Gerd Hoffmann]

Respect the interval for interrupt endpoints, so we don't finish
transfers as fast as possible but at the rate configured by the guest.

Fixes guest deadlocks triggered by interrupt storms.

Cc:
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 3826979..2e2eb55 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -355,6 +355,7 @@ typedef struct XHCITransfer {
     unsigned int streamid;
     bool in_xfer;
     bool iso_xfer;
+    bool timed_xfer;
 
     unsigned int trb_count;
     unsigned int trb_alloced;
@@ -1820,6 +1821,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer)
 
     xfer->in_xfer = bmRequestType & USB_DIR_IN;
     xfer->iso_xfer = false;
+    xfer->timed_xfer = false;
 
     if (xhci_setup_packet(xfer) < 0) {
         return -1;
@@ -1835,6 +1837,17 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer)
     return 0;
 }
 
+static void xhci_calc_intr_kick(XHCIState *xhci, XHCITransfer *xfer,
+                                XHCIEPContext *epctx, uint64_t mfindex)
+{
+    uint64_t asap = ((mfindex + epctx->interval - 1) &
+                     ~(epctx->interval-1));
+    uint64_t kick = epctx->mfindex_last + epctx->interval;
+
+    assert(epctx->interval != 0);
+    xfer->mfindex_kick = MAX(asap, kick);
+}
+
 static void xhci_calc_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
                                XHCIEPContext *epctx, uint64_t mfindex)
 {
@@ -1857,8 +1870,8 @@ static void xhci_calc_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
     }
 }
 
-static void xhci_check_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
-                                XHCIEPContext *epctx, uint64_t mfindex)
+static void xhci_check_intr_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
+                                     XHCIEPContext *epctx, uint64_t mfindex)
 {
     if (xfer->mfindex_kick > mfindex) {
         timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
@@ -1883,18 +1896,30 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
     switch(epctx->type) {
     case ET_INTR_OUT:
     case ET_INTR_IN:
+        xfer->pkts = 0;
+        xfer->iso_xfer = false;
+        xfer->timed_xfer = true;
+        mfindex = xhci_mfindex_get(xhci);
+        xhci_calc_intr_kick(xhci, xfer, epctx, mfindex);
+        xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
+        if (xfer->running_retry) {
+            return -1;
+        }
+        break;
     case ET_BULK_OUT:
     case ET_BULK_IN:
         xfer->pkts = 0;
         xfer->iso_xfer = false;
+        xfer->timed_xfer = false;
         break;
     case ET_ISO_OUT:
     case ET_ISO_IN:
         xfer->pkts = 1;
         xfer->iso_xfer = true;
+        xfer->timed_xfer = true;
         mfindex = xhci_mfindex_get(xhci);
         xhci_calc_iso_kick(xhci, xfer, epctx, mfindex);
-        xhci_check_iso_kick(xhci, xfer, epctx, mfindex);
+        xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
         if (xfer->running_retry) {
             return -1;
         }
@@ -1955,13 +1980,18 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
 
         trace_usb_xhci_xfer_retry(xfer);
         assert(xfer->running_retry);
-        if (xfer->iso_xfer) {
-            /* retry delayed iso transfer */
+        if (xfer->timed_xfer) {
+            /* time to kick the transfer? */
             mfindex = xhci_mfindex_get(xhci);
-            xhci_check_iso_kick(xhci, xfer, epctx, mfindex);
+            xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
             if (xfer->running_retry) {
                 return;
             }
+            xfer->timed_xfer = 0;
+            xfer->running_retry = 1;
+        }
+        if (xfer->iso_xfer) {
+            /* retry iso transfer */
             if (xhci_setup_packet(xfer) < 0) {
                 return;
             }
@@ -2047,7 +2077,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
                 epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
                 ep = xfer->packet.ep;
             } else {
-                if (!xfer->iso_xfer) {
+                if (!xfer->timed_xfer) {
                     fprintf(stderr, "xhci: error firing data transfer\n");
                 }
             }
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/10] xhci: reset port when disabling slot

[Gerd Hoffmann]

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 2e2eb55..10c938a 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2123,6 +2123,7 @@ static TRBCCode xhci_disable_slot(XHCIState *xhci, unsigned int slotid)
 
     xhci->slots[slotid-1].enabled = 0;
     xhci->slots[slotid-1].addressed = 0;
+    xhci->slots[slotid-1].uport = NULL;
     return CC_SUCCESS;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/10] uas: add property for request logging

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-uas.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 63ad12e..8701292 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -113,6 +113,9 @@ struct UASDevice {
     QTAILQ_HEAD(, UASStatus)  results;
     QTAILQ_HEAD(, UASRequest) requests;
 
+    /* properties */
+    uint32_t                  requestlog;
+
     /* usb 2.0 only */
     USBPacket                 *status2;
     UASRequest                *datain2;
@@ -692,9 +695,9 @@ static void usb_uas_command(UASDevice *uas, uas_ui *ui)
     req->req = scsi_req_new(req->dev, req->tag,
                             usb_uas_get_lun(req->lun),
                             ui->command.cdb, req);
-#if 1
-    scsi_req_print(req->req);
-#endif
+    if (uas->requestlog) {
+        scsi_req_print(req->req);
+    }
     len = scsi_req_enqueue(req->req);
     if (len) {
         req->data_size = len;
@@ -902,6 +905,11 @@ static const VMStateDescription vmstate_usb_uas = {
     }
 };
 
+static Property uas_properties[] = {
+    DEFINE_PROP_UINT32("log-scsi-req", UASDevice, requestlog, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void usb_uas_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -919,6 +927,7 @@ static void usb_uas_class_initfn(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->fw_name = "storage";
     dc->vmsd = &vmstate_usb_uas;
+    dc->props = uas_properties;
 }
 
 static const TypeInfo uas_info = {
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/10] usb: parallelize usb3 streams

[Gerd Hoffmann]

usb3 bulk endpoints with streams are implicitly pipelined now,
so the requests will actually be processed in parallel.  Also
allow them to complete out-of-order.

Fixes stalls in the uas driver.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 05948ca..31960c2 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -403,7 +403,7 @@ void usb_handle_packet(USBDevice *dev, USBPacket *p)
         p->ep->halted = false;
     }
 
-    if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
+    if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline || p->stream) {
         usb_process_one(p);
         if (p->status == USB_RET_ASYNC) {
             /* hcd drivers cannot handle async for isoc */
@@ -420,7 +420,8 @@ void usb_handle_packet(USBDevice *dev, USBPacket *p)
              * When pipelining is enabled usb-devices must always return async,
              * otherwise packets can complete out of order!
              */
-            assert(!p->ep->pipeline || QTAILQ_EMPTY(&p->ep->queue));
+            assert(p->stream || !p->ep->pipeline ||
+                   QTAILQ_EMPTY(&p->ep->queue));
             if (p->status != USB_RET_NAK) {
                 usb_packet_set_state(p, USB_PACKET_COMPLETE);
             }
@@ -434,7 +435,7 @@ void usb_packet_complete_one(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
 
-    assert(QTAILQ_FIRST(&ep->queue) == p);
+    assert(p->stream || QTAILQ_FIRST(&ep->queue) == p);
     assert(p->status != USB_RET_ASYNC && p->status != USB_RET_NAK);
 
     if (p->status != USB_RET_SUCCESS ||
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/10] usb-hub: add tracepoint for status reports

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-hub.c | 1 +
 trace-events     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index e865a98..54f63c0 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -475,6 +475,7 @@ static void usb_hub_handle_data(USBDevice *dev, USBPacket *p)
                 port->wPortChange_reported = port->wPortChange;
             }
             if (status != 0) {
+                trace_usb_hub_status_report(s->dev.addr, status);
                 for(i = 0; i < n; i++) {
                     buf[i] = status >> (8 * i);
                 }
diff --git a/trace-events b/trace-events
index f849807..754c28c 100644
--- a/trace-events
+++ b/trace-events
@@ -411,6 +411,7 @@ usb_hub_set_port_feature(int addr, int nr, const char *f) "dev %d, port %d, feat
 usb_hub_clear_port_feature(int addr, int nr, const char *f) "dev %d, port %d, feature %s"
 usb_hub_attach(int addr, int nr) "dev %d, port %d"
 usb_hub_detach(int addr, int nr) "dev %d, port %d"
+usb_hub_status_report(int addr, int status) "dev %d, status 0x%x"
 
 # hw/usb/dev-uas.c
 usb_uas_reset(int addr) "dev %d"
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/10] Revert "usb-hub: report status changes only once"

[Gerd Hoffmann]

This reverts commit a309ee6e0a256f690760abfba44fceaa52a7c2f3.

This isn't in line with the usb specification and adds regressions,
win7 fails to drive the usb hub for example.

Was added because it "solved" the issue of hubs interacting badly
with the xhci host controller.  Now with the root cause being fixed
in xhci (commit <FIXME>) we can revert this one.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-hub.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 54f63c0..58647b4 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -33,7 +33,6 @@ typedef struct USBHubPort {
     USBPort port;
     uint16_t wPortStatus;
     uint16_t wPortChange;
-    uint16_t wPortChange_reported;
 } USBHubPort;
 
 typedef struct USBHubState {
@@ -468,11 +467,8 @@ static void usb_hub_handle_data(USBDevice *dev, USBPacket *p)
             status = 0;
             for(i = 0; i < NUM_PORTS; i++) {
                 port = &s->ports[i];
-                if (port->wPortChange &&
-                    port->wPortChange_reported != port->wPortChange) {
+                if (port->wPortChange)
                     status |= (1 << (i + 1));
-                }
-                port->wPortChange_reported = port->wPortChange;
             }
             if (status != 0) {
                 trace_usb_hub_status_report(s->dev.addr, status);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/10] Revert "usb-hub: report status changes only once"

[Hans de Goede]

Hi,

On 08/28/2013 02:43 PM, Gerd Hoffmann wrote:
> This reverts commit a309ee6e0a256f690760abfba44fceaa52a7c2f3.
>
> This isn't in line with the usb specification and adds regressions,
> win7 fails to drive the usb hub for example.
>
> Was added because it "solved" the issue of hubs interacting badly
> with the xhci host controller.  Now with the root cause being fixed
> in xhci (commit <FIXME>) we can revert this one.

Maybe the FIXME in the commit msg should be fixed before this goes to
master ?? Other then that this patch set looks good, great work!

Regards,

Hans

Re: [Qemu-devel] [PATCH 10/10] Revert "usb-hub: report status changes only once"

[Gerd Hoffmann]

On Mi, 2013-08-28 at 17:13 +0200, Hans de Goede wrote:
> Hi,
> 
> On 08/28/2013 02:43 PM, Gerd Hoffmann wrote:
> > This reverts commit a309ee6e0a256f690760abfba44fceaa52a7c2f3.
> >
> > This isn't in line with the usb specification and adds regressions,
> > win7 fails to drive the usb hub for example.
> >
> > Was added because it "solved" the issue of hubs interacting badly
> > with the xhci host controller.  Now with the root cause being fixed
> > in xhci (commit <FIXME>) we can revert this one.
> 
> Maybe the FIXME in the commit msg should be fixed before this goes to
> master ?? Other then that this patch set looks good, great work!

Sure, will do when sending the pull request.  But will probably rebase
before doing so, which would render the hash invalid, thats why it isn't
there yet.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 05/10] xhci: emulate intr endpoint intervals correctly

[Michael Roth]

Quoting Gerd Hoffmann (2013-08-28 07:43:01)
> Respect the interval for interrupt endpoints, so we don't finish
> transfers as fast as possible but at the rate configured by the guest.
> 
> Fixes guest deadlocks triggered by interrupt storms.
> 
> Cc:

Was this meant to go to qemu-stable?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 3826979..2e2eb55 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -355,6 +355,7 @@ typedef struct XHCITransfer {
>      unsigned int streamid;
>      bool in_xfer;
>      bool iso_xfer;
> +    bool timed_xfer;
> 
>      unsigned int trb_count;
>      unsigned int trb_alloced;
> @@ -1820,6 +1821,7 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer)
> 
>      xfer->in_xfer = bmRequestType & USB_DIR_IN;
>      xfer->iso_xfer = false;
> +    xfer->timed_xfer = false;
> 
>      if (xhci_setup_packet(xfer) < 0) {
>          return -1;
> @@ -1835,6 +1837,17 @@ static int xhci_fire_ctl_transfer(XHCIState *xhci, XHCITransfer *xfer)
>      return 0;
>  }
> 
> +static void xhci_calc_intr_kick(XHCIState *xhci, XHCITransfer *xfer,
> +                                XHCIEPContext *epctx, uint64_t mfindex)
> +{
> +    uint64_t asap = ((mfindex + epctx->interval - 1) &
> +                     ~(epctx->interval-1));
> +    uint64_t kick = epctx->mfindex_last + epctx->interval;
> +
> +    assert(epctx->interval != 0);
> +    xfer->mfindex_kick = MAX(asap, kick);
> +}
> +
>  static void xhci_calc_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
>                                 XHCIEPContext *epctx, uint64_t mfindex)
>  {
> @@ -1857,8 +1870,8 @@ static void xhci_calc_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
>      }
>  }
> 
> -static void xhci_check_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
> -                                XHCIEPContext *epctx, uint64_t mfindex)
> +static void xhci_check_intr_iso_kick(XHCIState *xhci, XHCITransfer *xfer,
> +                                     XHCIEPContext *epctx, uint64_t mfindex)
>  {
>      if (xfer->mfindex_kick > mfindex) {
>          timer_mod(epctx->kick_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> @@ -1883,18 +1896,30 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
>      switch(epctx->type) {
>      case ET_INTR_OUT:
>      case ET_INTR_IN:
> +        xfer->pkts = 0;
> +        xfer->iso_xfer = false;
> +        xfer->timed_xfer = true;
> +        mfindex = xhci_mfindex_get(xhci);
> +        xhci_calc_intr_kick(xhci, xfer, epctx, mfindex);
> +        xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
> +        if (xfer->running_retry) {
> +            return -1;
> +        }
> +        break;
>      case ET_BULK_OUT:
>      case ET_BULK_IN:
>          xfer->pkts = 0;
>          xfer->iso_xfer = false;
> +        xfer->timed_xfer = false;
>          break;
>      case ET_ISO_OUT:
>      case ET_ISO_IN:
>          xfer->pkts = 1;
>          xfer->iso_xfer = true;
> +        xfer->timed_xfer = true;
>          mfindex = xhci_mfindex_get(xhci);
>          xhci_calc_iso_kick(xhci, xfer, epctx, mfindex);
> -        xhci_check_iso_kick(xhci, xfer, epctx, mfindex);
> +        xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
>          if (xfer->running_retry) {
>              return -1;
>          }
> @@ -1955,13 +1980,18 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
> 
>          trace_usb_xhci_xfer_retry(xfer);
>          assert(xfer->running_retry);
> -        if (xfer->iso_xfer) {
> -            /* retry delayed iso transfer */
> +        if (xfer->timed_xfer) {
> +            /* time to kick the transfer? */
>              mfindex = xhci_mfindex_get(xhci);
> -            xhci_check_iso_kick(xhci, xfer, epctx, mfindex);
> +            xhci_check_intr_iso_kick(xhci, xfer, epctx, mfindex);
>              if (xfer->running_retry) {
>                  return;
>              }
> +            xfer->timed_xfer = 0;
> +            xfer->running_retry = 1;
> +        }
> +        if (xfer->iso_xfer) {
> +            /* retry iso transfer */
>              if (xhci_setup_packet(xfer) < 0) {
>                  return;
>              }
> @@ -2047,7 +2077,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
>                  epctx->next_xfer = (epctx->next_xfer + 1) % TD_QUEUE;
>                  ep = xfer->packet.ep;
>              } else {
> -                if (!xfer->iso_xfer) {
> +                if (!xfer->timed_xfer) {
>                      fprintf(stderr, "xhci: error firing data transfer\n");
>                  }
>              }
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 05/10] xhci: emulate intr endpoint intervals correctly

[Gerd Hoffmann]

On Di, 2013-09-24 at 19:55 -0500, Michael Roth wrote:
> Quoting Gerd Hoffmann (2013-08-28 07:43:01)
> > Respect the interval for interrupt endpoints, so we don't finish
> > transfers as fast as possible but at the rate configured by the guest.
> > 
> > Fixes guest deadlocks triggered by interrupt storms.
> > 
> > Cc:
> 
> Was this meant to go to qemu-stable?

Yes.

As you seem to be busy collecting stable patches, here is another one
without cc qemu-stable which should go into 1.6.1:
c58c7b959b93b864a27fd6b3646ee1465ab8832b

thanks,
  Gerd