[Qemu-devel] [PATCH 0/7] usb: host-libusb and xhci fixes

[Qemu-devel] [PATCH 0/7] usb: host-libusb and xhci fixes

[Hans de Goede]

Hi Gerd,

Here is a set of fixes for host-libusb and xhci, mostly the result of me
working on adding support for streams to qemu's host redirection code
(and libusb and usbfs in the kernel).

I've this almost all working now. The 2 xhci fixes fixup error handling
when the real uasp device I have errors out on some scsi command during
probing which it does not like, with this fixed I can boot up a vm
and use some data on the disk without any glitches.

Currently the handover from host -> guest does not work well though, I
need to blacklist the host drivers so that the guest gets a pristine state
device, but I believe I know how I can fix this now ...

Anyways these patches fix various generic issues, so please apply. I hope
to have a patchset actually adding stream redirection soon.

Regards,

Hans

[Qemu-devel] [PATCH 1/7] usb-host-libusb: Fix reset handling

[Hans de Goede]

The guest will issue an initial device reset when the device is attached, but
since the current usb-host-libusb code only actually does the reset when
udev->configuration != 0, and on attach the device is not yet configured,
the reset gets ignored. This means that the device gets passed to the guest
in an unknown state, which is not good.

The udev->configuration check is there because of the release / claim
interfaces done around the libusb_device_reset call, but these are not
necessary. If interfaces are claimed when libusb_device_reset gets called
libusb will release + reclaim them itself.

The usb_host_ep_update call also is not necessary. If the reset succeeds the
original config and interface alt settings will be restored.

Last if the reset fails, that means the device has either disconnected or
morphed into an another device and has been completely re-enumerated,
so it is treated by the host as a new device and our handle is invalid,
so on reset failure we need to call usb_host_nodev().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/host-libusb.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index b27f559..09a8a73 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1277,16 +1277,14 @@ static void usb_host_flush_ep_queue(USBDevice *dev, USBEndpoint *ep)
 static void usb_host_handle_reset(USBDevice *udev)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
+    int rc;
 
     trace_usb_host_reset(s->bus_num, s->addr);
 
-    if (udev->configuration == 0) {
-        return;
+    rc = libusb_reset_device(s->dh);
+    if (rc != 0) {
+        usb_host_nodev(s);
     }
-    usb_host_release_interfaces(s);
-    libusb_reset_device(s->dh);
-    usb_host_claim_interfaces(s, 0);
-    usb_host_ep_update(s);
 }
 
 static int usb_host_alloc_streams(USBDevice *udev, USBEndpoint *ep,
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] usb-host-libusb: Configuration 0 may be a valid configuration

[Hans de Goede]

Quoting from: linux/Documentation/ABI/stable/sysfs-bus-usb:

	Note that some devices, in violation of the USB spec, have a
	configuration with a value equal to 0. Writing 0 to
	bConfigurationValue for these devices will install that
	configuration, rather then unconfigure the device.

So don't compare the configuration value against 0 to check for unconfigured
devices, instead check for a LIBUSB_ERROR_NOT_FOUND return from
libusb_get_active_config_descriptor().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/host-libusb.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 09a8a73..f267fd4 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1004,15 +1004,14 @@ static int usb_host_claim_interfaces(USBHostDevice *s, int configuration)
     udev->ninterfaces   = 0;
     udev->configuration = 0;
 
-    if (configuration == 0) {
-        /* address state - ignore */
-        return USB_RET_SUCCESS;
-    }
-
     usb_host_detach_kernel(s);
 
     rc = libusb_get_active_config_descriptor(s->dev, &conf);
     if (rc != 0) {
+        if (rc == LIBUSB_ERROR_NOT_FOUND) {
+            /* address state - ignore */
+            return USB_RET_SUCCESS;
+        }            
         return USB_RET_STALL;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] usb-host-libusb: Detach kernel drivers earlier

[Hans de Goede]

If we detach the kernel drivers on the first set_config, then they will
be still attached when the device gets its initial reset. Causing the drivers
to re-initialize the device after the reset, dirtying the device state.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/host-libusb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index f267fd4..2e6d932 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -137,6 +137,7 @@ static QTAILQ_HEAD(, USBHostDevice) hostdevs =
 static void usb_host_auto_check(void *unused);
 static void usb_host_release_interfaces(USBHostDevice *s);
 static void usb_host_nodev(USBHostDevice *s);
+static void usb_host_detach_kernel(USBHostDevice *s);
 static void usb_host_attach_kernel(USBHostDevice *s);
 
 /* ------------------------------------------------------------------------ */
@@ -799,10 +800,13 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev)
         goto fail;
     }
 
-    libusb_get_device_descriptor(dev, &s->ddesc);
     s->dev     = dev;
     s->bus_num = bus_num;
     s->addr    = addr;
+
+    usb_host_detach_kernel(s);
+
+    libusb_get_device_descriptor(dev, &s->ddesc);
     usb_host_get_port(s->dev, s->port, sizeof(s->port));
 
     usb_ep_init(udev);
@@ -1063,7 +1067,6 @@ static void usb_host_set_config(USBHostDevice *s, int config, USBPacket *p)
     trace_usb_host_set_config(s->bus_num, s->addr, config);
 
     usb_host_release_interfaces(s);
-    usb_host_detach_kernel(s);
     rc = libusb_set_configuration(s->dh, config);
     if (rc != 0) {
         usb_host_libusb_error("libusb_set_configuration", rc);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-xhci.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 286d272..b343235 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -374,7 +374,6 @@ struct XHCIStreamContext {
     dma_addr_t pctx;
     unsigned int sct;
     XHCIRing ring;
-    XHCIStreamContext *sstreams;
 };
 
 struct XHCIEPContext {
@@ -1117,7 +1116,6 @@ static void xhci_reset_streams(XHCIEPContext *epctx)
 
     for (i = 0; i < epctx->nr_pstreams; i++) {
         epctx->pstreams[i].sct = -1;
-        g_free(epctx->pstreams[i].sstreams);
     }
 }
 
@@ -1170,15 +1168,9 @@ static TRBCCode xhci_alloc_streams(XHCIEPContext *epctx, dma_addr_t base)
 static void xhci_free_streams(XHCIEPContext *epctx)
 {
     USBEndpoint *ep;
-    int i;
 
     assert(epctx->pstreams != NULL);
 
-    if (!epctx->lsa) {
-        for (i = 0; i < epctx->nr_pstreams; i++) {
-            g_free(epctx->pstreams[i].sstreams);
-        }
-    }
     g_free(epctx->pstreams);
     epctx->pstreams = NULL;
     epctx->nr_pstreams = 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] usb-hcd-xhci: Remove unused cancelled member from XHCITransfer

[Hans de Goede]

Since qemu's USB model is geared towards emulated devices cancellation
is instanteneous, so no need to wait for cancellation to complete, as
such there is no wait for cancellation code, and the cancelled bool
as well as the bogus comment about it can be removed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-xhci.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index b343235..9845554 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -346,7 +346,6 @@ typedef struct XHCITransfer {
     QEMUSGList sgl;
     bool running_async;
     bool running_retry;
-    bool cancelled;
     bool complete;
     bool int_req;
     unsigned int iso_pkts;
@@ -1347,8 +1346,6 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
     if (t->running_async) {
         usb_cancel_packet(&t->packet);
         t->running_async = 0;
-        t->cancelled = 1;
-        DPRINTF("xhci: cancelling transfer, waiting for it to complete\n");
         killed = 1;
     }
     if (t->running_retry) {
@@ -1765,14 +1762,12 @@ static int xhci_complete_packet(XHCITransfer *xfer)
         xfer->running_async = 1;
         xfer->running_retry = 0;
         xfer->complete = 0;
-        xfer->cancelled = 0;
         return 0;
     } else if (xfer->packet.status == USB_RET_NAK) {
         trace_usb_xhci_xfer_nak(xfer);
         xfer->running_async = 0;
         xfer->running_retry = 1;
         xfer->complete = 0;
-        xfer->cancelled = 0;
         return 0;
     } else {
         xfer->running_async = 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] usb-hcd-xhci: Report completion of active transfer with CC_STOPPED on ep stop

[Hans de Goede]

As we should per the XHCI spec "4.6.9 Stop Endpoint".

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-xhci.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 9845554..4890344 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -497,6 +497,7 @@ static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid,
                          unsigned int epid, unsigned int streamid);
 static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
                                 unsigned int epid);
+static void xhci_xfer_report(XHCITransfer *xfer);
 static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v);
 static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v);
 static USBEndpoint *xhci_epid_to_usbep(XHCIState *xhci,
@@ -1339,10 +1340,15 @@ static TRBCCode xhci_enable_ep(XHCIState *xhci, unsigned int slotid,
     return CC_SUCCESS;
 }
 
-static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
+static int xhci_ep_nuke_one_xfer(XHCITransfer *t, TRBCCode report)
 {
     int killed = 0;
 
+    if (report && (t->running_async || t->running_retry)) {
+        t->status = report;
+        xhci_xfer_report(t);
+    }
+
     if (t->running_async) {
         usb_cancel_packet(&t->packet);
         t->running_async = 0;
@@ -1355,6 +1361,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
             qemu_del_timer(epctx->kick_timer);
         }
         t->running_retry = 0;
+        killed = 1;
     }
     if (t->trbs) {
         g_free(t->trbs);
@@ -1367,7 +1374,7 @@ static int xhci_ep_nuke_one_xfer(XHCITransfer *t)
 }
 
 static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
-                               unsigned int epid)
+                               unsigned int epid, TRBCCode report)
 {
     XHCISlot *slot;
     XHCIEPContext *epctx;
@@ -1388,7 +1395,9 @@ static int xhci_ep_nuke_xfers(XHCIState *xhci, unsigned int slotid,
 
     xferi = epctx->next_xfer;
     for (i = 0; i < TD_QUEUE; i++) {
-        killed += xhci_ep_nuke_one_xfer(&epctx->transfers[xferi]);
+        killed += xhci_ep_nuke_one_xfer(&epctx->transfers[xferi], report);
+        if (killed)
+            report = 0; /* Only report once */
         epctx->transfers[xferi].packet.ep = NULL;
         xferi = (xferi + 1) % TD_QUEUE;
     }
@@ -1418,7 +1427,7 @@ static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
         return CC_SUCCESS;
     }
 
-    xhci_ep_nuke_xfers(xhci, slotid, epid);
+    xhci_ep_nuke_xfers(xhci, slotid, epid, 0);
 
     epctx = slot->eps[epid-1];
 
@@ -1460,7 +1469,7 @@ static TRBCCode xhci_stop_ep(XHCIState *xhci, unsigned int slotid,
         return CC_EP_NOT_ENABLED_ERROR;
     }
 
-    if (xhci_ep_nuke_xfers(xhci, slotid, epid) > 0) {
+    if (xhci_ep_nuke_xfers(xhci, slotid, epid, CC_STOPPED) > 0) {
         fprintf(stderr, "xhci: FIXME: endpoint stopped w/ xfers running, "
                 "data might be lost\n");
     }
@@ -1505,7 +1514,7 @@ static TRBCCode xhci_reset_ep(XHCIState *xhci, unsigned int slotid,
         return CC_CONTEXT_STATE_ERROR;
     }
 
-    if (xhci_ep_nuke_xfers(xhci, slotid, epid) > 0) {
+    if (xhci_ep_nuke_xfers(xhci, slotid, epid, 0) > 0) {
         fprintf(stderr, "xhci: FIXME: endpoint reset w/ xfers running, "
                 "data might be lost\n");
     }
@@ -2498,7 +2507,7 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport)
 
     for (ep = 0; ep < 31; ep++) {
         if (xhci->slots[slot].eps[ep]) {
-            xhci_ep_nuke_xfers(xhci, slot+1, ep+1);
+            xhci_ep_nuke_xfers(xhci, slot + 1, ep + 1, 0);
         }
     }
     xhci->slots[slot].uport = NULL;
@@ -3311,7 +3320,7 @@ static void xhci_complete(USBPort *port, USBPacket *packet)
     XHCITransfer *xfer = container_of(packet, XHCITransfer, packet);
 
     if (packet->status == USB_RET_REMOVE_FROM_QUEUE) {
-        xhci_ep_nuke_one_xfer(xfer);
+        xhci_ep_nuke_one_xfer(xfer, 0);
         return;
     }
     xhci_complete_packet(xfer);
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] usb-hcd-xhci: Update endpoint context dequeue pointer for streams too

[Hans de Goede]

With streams the endpoint context dequeue pointer should point to the
dequeue value for the currently active stream.

At least Linux guests expect it to point to value set by an set_ep_dequeue
upon completion of the set_ep_dequeue (before kicking the ep).

Otherwise the Linux kernel will complain (and things won't work):

xhci_hcd 0000:00:05.0: Mismatch between completed Set TR Deq Ptr command & xHCI internal state.
xhci_hcd 0000:00:05.0: ep deq seg = ffff8800366f0880, deq ptr = ffff8800366ec010

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-xhci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4890344..0bbf296 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1216,6 +1216,7 @@ static XHCIStreamContext *xhci_find_stream(XHCIEPContext *epctx,
 static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
                               XHCIStreamContext *sctx, uint32_t state)
 {
+    XHCIRing *ring = NULL;
     uint32_t ctx[5];
     uint32_t ctx2[2];
 
@@ -1226,6 +1227,7 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
     /* update ring dequeue ptr */
     if (epctx->nr_pstreams) {
         if (sctx != NULL) {
+            ring = &sctx->ring;
             xhci_dma_read_u32s(xhci, sctx->pctx, ctx2, sizeof(ctx2));
             ctx2[0] &= 0xe;
             ctx2[0] |= sctx->ring.dequeue | sctx->ring.ccs;
@@ -1233,8 +1235,12 @@ static void xhci_set_ep_state(XHCIState *xhci, XHCIEPContext *epctx,
             xhci_dma_write_u32s(xhci, sctx->pctx, ctx2, sizeof(ctx2));
         }
     } else {
-        ctx[2] = epctx->ring.dequeue | epctx->ring.ccs;
-        ctx[3] = (epctx->ring.dequeue >> 16) >> 16;
+        ring = &epctx->ring;
+    }
+    if (ring) {
+        ctx[2] = ring->dequeue | ring->ccs;
+        ctx[3] = (ring->dequeue >> 16) >> 16;
+
         DPRINTF("xhci: set epctx: " DMA_ADDR_FMT " state=%d dequeue=%08x%08x\n",
                 epctx->pctx, state, ctx[3], ctx[2]);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/7] usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext

[Gerd Hoffmann]

On Mo, 2013-09-23 at 20:54 +0200, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Patch doesn't apply.

That are bits for the (not fully implemented yet) secondary stream
arrays btw.  We might complete the implementation instead of kicking
them out.  I have no idea whenever there is a reasonable way to test
that though ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/7] usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext

[Hans de Goede]

Hi,

On 09/24/2013 11:37 AM, Gerd Hoffmann wrote:
> On Mo, 2013-09-23 at 20:54 +0200, Hans de Goede wrote:
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Patch doesn't apply.

Sorry, my bad, I had some other changes in my local tree
which I was not yet ready to send and this depended on them.

I'm ready to send the whole bunch of patches in one go now,
which I'll do directly after this mail.

> That are bits for the (not fully implemented yet) secondary stream
> arrays btw.

I know, but ...

 > We might complete the implementation instead of kicking
> them out.

Looking at the spec, I don't think any guest drivers will implement
secondary streams, the lsa can handle any reasonable amount of streams
just fine. The whole secondary stream thing is only interesting
if you want to do insane amount streams, or have stream id ranges
with holes in them.

> I have no idea whenever there is a reasonable way to test
> that though ...

I agree, and I'm not sure there ever will be. So I vote for not worrying
about secondary streams until we actually encounter a guest which uses
them (at which point we should have a way to test through that guest).

So my vote goes to just removing this cruft for now.

Regards,

Hans

[Qemu-devel] [PATCH 4/7] usb-hcd-xhci: Remove unused sstreamsm member from XHCIStreamContext

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

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

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 469c24d..e078c50 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -374,7 +374,6 @@ struct XHCIStreamContext {
     dma_addr_t pctx;
     unsigned int sct;
     XHCIRing ring;
-    XHCIStreamContext *sstreams;
 };
 
 struct XHCIEPContext {
@@ -1133,7 +1132,6 @@ static void xhci_reset_streams(XHCIEPContext *epctx)
 
     for (i = 0; i < epctx->nr_pstreams; i++) {
         epctx->pstreams[i].sct = -1;
-        g_free(epctx->pstreams[i].sstreams);
     }
 }
 
@@ -1146,15 +1144,8 @@ static void xhci_alloc_streams(XHCIEPContext *epctx, dma_addr_t base)
 
 static void xhci_free_streams(XHCIEPContext *epctx)
 {
-    int i;
-
     assert(epctx->pstreams != NULL);
 
-    if (!epctx->lsa) {
-        for (i = 0; i < epctx->nr_pstreams; i++) {
-            g_free(epctx->pstreams[i].sstreams);
-        }
-    }
     g_free(epctx->pstreams);
     epctx->pstreams = NULL;
     epctx->nr_pstreams = 0;
-- 
1.8.3.1