[Qemu-devel] [USB fixes for 1.3] usb-redir: Fix migration race with output int eps

[Qemu-devel] [USB fixes for 1.3] usb-redir: Fix migration race with output int eps

[Hans de Goede]

This 2 patch set for 1.3 fixes a migration race for devices with output
interrupt endpoints.

Regards,

Hans

[Qemu-devel] [PATCH 1/2] usb-redir: Split usb_handle_interrupt_data into separate in/out functions

[Hans de Goede]

No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 136 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 71 insertions(+), 65 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index c8e2718..9658bda 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -620,80 +620,82 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
     p->status = USB_RET_ASYNC;
 }
 
-static void usbredir_handle_interrupt_data(USBRedirDevice *dev,
-                                           USBPacket *p, uint8_t ep)
+static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
+                                              USBPacket *p, uint8_t ep)
 {
-    if (ep & USB_DIR_IN) {
-        /* Input interrupt endpoint, buffered packet input */
-        struct buf_packet *intp;
-        int status, len;
-
-        if (!dev->endpoint[EP2I(ep)].interrupt_started &&
-                !dev->endpoint[EP2I(ep)].interrupt_error) {
-            struct usb_redir_start_interrupt_receiving_header start_int = {
-                .endpoint = ep,
-            };
-            /* No id, we look at the ep when receiving a status back */
-            usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
-                                                          &start_int);
-            usbredirparser_do_write(dev->parser);
-            DPRINTF("interrupt recv started ep %02X\n", ep);
-            dev->endpoint[EP2I(ep)].interrupt_started = 1;
-            /* We don't really want to drop interrupt packets ever, but
-               having some upper limit to how much we buffer is good. */
-            dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
-            dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
-        }
+    /* Input interrupt endpoint, buffered packet input */
+    struct buf_packet *intp;
+    int status, len;
 
-        intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
-        if (intp == NULL) {
-            DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
-            /* Check interrupt_error for stream errors */
-            status = dev->endpoint[EP2I(ep)].interrupt_error;
-            dev->endpoint[EP2I(ep)].interrupt_error = 0;
-            if (status) {
-                usbredir_handle_status(dev, p, status);
-            } else {
-                p->status = USB_RET_NAK;
-            }
-            return;
-        }
-        DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
-                intp->status, intp->len);
+    if (!dev->endpoint[EP2I(ep)].interrupt_started &&
+            !dev->endpoint[EP2I(ep)].interrupt_error) {
+        struct usb_redir_start_interrupt_receiving_header start_int = {
+            .endpoint = ep,
+        };
+        /* No id, we look at the ep when receiving a status back */
+        usbredirparser_send_start_interrupt_receiving(dev->parser, 0,
+                                                      &start_int);
+        usbredirparser_do_write(dev->parser);
+        DPRINTF("interrupt recv started ep %02X\n", ep);
+        dev->endpoint[EP2I(ep)].interrupt_started = 1;
+        /* We don't really want to drop interrupt packets ever, but
+           having some upper limit to how much we buffer is good. */
+        dev->endpoint[EP2I(ep)].bufpq_target_size = 1000;
+        dev->endpoint[EP2I(ep)].bufpq_dropping_packets = 0;
+    }
 
-        status = intp->status;
-        len = intp->len;
-        if (len > p->iov.size) {
-            ERROR("received int data is larger then packet ep %02X\n", ep);
-            len = p->iov.size;
-            status = usb_redir_babble;
+    intp = QTAILQ_FIRST(&dev->endpoint[EP2I(ep)].bufpq);
+    if (intp == NULL) {
+        DPRINTF2("interrupt-token-in ep %02X, no intp\n", ep);
+        /* Check interrupt_error for stream errors */
+        status = dev->endpoint[EP2I(ep)].interrupt_error;
+        dev->endpoint[EP2I(ep)].interrupt_error = 0;
+        if (status) {
+            usbredir_handle_status(dev, p, status);
+        } else {
+            p->status = USB_RET_NAK;
         }
-        usb_packet_copy(p, intp->data, len);
-        bufp_free(dev, intp, ep);
-        usbredir_handle_status(dev, p, status);
-    } else {
-        /* Output interrupt endpoint, normal async operation */
-        struct usb_redir_interrupt_packet_header interrupt_packet;
-        uint8_t buf[p->iov.size];
+        return;
+    }
+    DPRINTF("interrupt-token-in ep %02X status %d len %d\n", ep,
+            intp->status, intp->len);
 
-        DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
-                p->iov.size, p->id);
+    status = intp->status;
+    len = intp->len;
+    if (len > p->iov.size) {
+        ERROR("received int data is larger then packet ep %02X\n", ep);
+        len = p->iov.size;
+        status = usb_redir_babble;
+    }
+    usb_packet_copy(p, intp->data, len);
+    bufp_free(dev, intp, ep);
+    usbredir_handle_status(dev, p, status);
+}
 
-        if (usbredir_already_in_flight(dev, p->id)) {
-            p->status = USB_RET_ASYNC;
-            return;
-        }
+static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
+                                               USBPacket *p, uint8_t ep)
+{
+    /* Output interrupt endpoint, normal async operation */
+    struct usb_redir_interrupt_packet_header interrupt_packet;
+    uint8_t buf[p->iov.size];
 
-        interrupt_packet.endpoint  = ep;
-        interrupt_packet.length    = p->iov.size;
+    DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
+            p->iov.size, p->id);
 
-        usb_packet_copy(p, buf, p->iov.size);
-        usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
-        usbredirparser_send_interrupt_packet(dev->parser, p->id,
-                                        &interrupt_packet, buf, p->iov.size);
-        usbredirparser_do_write(dev->parser);
+    if (usbredir_already_in_flight(dev, p->id)) {
         p->status = USB_RET_ASYNC;
+        return;
     }
+
+    interrupt_packet.endpoint  = ep;
+    interrupt_packet.length    = p->iov.size;
+
+    usb_packet_copy(p, buf, p->iov.size);
+    usbredir_log_data(dev, "interrupt data out:", buf, p->iov.size);
+    usbredirparser_send_interrupt_packet(dev->parser, p->id,
+                                    &interrupt_packet, buf, p->iov.size);
+    usbredirparser_do_write(dev->parser);
+    p->status = USB_RET_ASYNC;
 }
 
 static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
@@ -739,7 +741,11 @@ static void usbredir_handle_data(USBDevice *udev, USBPacket *p)
         usbredir_handle_bulk_data(dev, p, ep);
         break;
     case USB_ENDPOINT_XFER_INT:
-        usbredir_handle_interrupt_data(dev, p, ep);
+        if (ep & USB_DIR_IN) {
+            usbredir_handle_interrupt_in_data(dev, p, ep);
+        } else {
+            usbredir_handle_interrupt_out_data(dev, p, ep);
+        }
         break;
     default:
         ERROR("handle_data ep %02X has unknown type %d\n", ep,
-- 
1.8.0

[Qemu-devel] [PATCH 2/2] usb-redir: Don't handle interrupt output packets async

[Hans de Goede]

Instead report them as successfully completed directly on submission, this
has 2 advantages:

1) This matches the timing of interrupt output packets on real hardware,
with the previous async handling, if an ep has an interval of say 500 ms,
then there would be 500+ ms between the submission and the guest seeing the
completion, as we wont do the write back until the qh gets polled again. And
in the mean time the guest may very well have timed out, as the guest can
reasonable expect a much quicker completion.

2) This fixes interrupt output packets potentially getting send twice
surrounding a migration. As we delay the writeback to guest memory until
the qh gets polled again, there is a window between completion and writeback
where migration can happen, in this case the destination will not know
about the completion, and it will execute the packet *again*

But it does also come with a disadvantage:

1) If the actual interrupt out to the real usb device fails, there is no
way to report this back to the guest.

This patch assumes however that interrupt outs in practice never fail, as
they are only used by specialized drivers, which are unlikely to issue illegal
requests (unlike general class drivers which often issue requests which some
devices don't implement). And that thus the advantages outway the disadvantage.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 9658bda..15f394c 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -672,21 +672,22 @@ static void usbredir_handle_interrupt_in_data(USBRedirDevice *dev,
     usbredir_handle_status(dev, p, status);
 }
 
+/*
+ * Handle interrupt out data, the usbredir protocol expects us to do this
+ * async, so that it can report back a completion status. But guests will
+ * expect immediate completion for an interrupt endpoint, and handling this
+ * async causes migration issues. So we report success directly, counting
+ * on the fact that output interrupt packets normally always succeed.
+ */
 static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
                                                USBPacket *p, uint8_t ep)
 {
-    /* Output interrupt endpoint, normal async operation */
     struct usb_redir_interrupt_packet_header interrupt_packet;
     uint8_t buf[p->iov.size];
 
     DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
             p->iov.size, p->id);
 
-    if (usbredir_already_in_flight(dev, p->id)) {
-        p->status = USB_RET_ASYNC;
-        return;
-    }
-
     interrupt_packet.endpoint  = ep;
     interrupt_packet.length    = p->iov.size;
 
@@ -695,7 +696,6 @@ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
     usbredirparser_send_interrupt_packet(dev->parser, p->id,
                                     &interrupt_packet, buf, p->iov.size);
     usbredirparser_do_write(dev->parser);
-    p->status = USB_RET_ASYNC;
 }
 
 static void usbredir_stop_interrupt_receiving(USBRedirDevice *dev,
@@ -1670,11 +1670,13 @@ static void usbredir_interrupt_packet(void *priv, uint64_t id,
         /* bufp_alloc also adds the packet to the ep queue */
         bufp_alloc(dev, data, data_len, interrupt_packet->status, ep);
     } else {
-        USBPacket *p = usbredir_find_packet_by_id(dev, ep, id);
-        if (p) {
-            usbredir_handle_status(dev, p, interrupt_packet->status);
-            p->actual_length = interrupt_packet->length;
-            usb_packet_complete(&dev->dev, p);
+        /*
+         * We report output interrupt packets as completed directly upon
+         * submission, so all we can do here if one failed is warn.
+         */
+        if (interrupt_packet->status) {
+            WARNING("interrupt output failed status %d ep %02X id %"PRIu64"\n",
+                    interrupt_packet->status, ep, id);
         }
     }
 }
-- 
1.8.0