[Qemu-devel] [PATCH 16/31] usb-ehci: itd handling fixes.

[Gerd Hoffmann <kraxel@redhat.com>]

This patch fixes a bunch of issues in the itd descriptor handling.
Most important fix is to handle transfers which cross page borders
correctly by looking up the address of the next page.  Luckily the
linux uses physically contigous memory so the data used to hits the
correct location even with this bug instead of corrupting guest
memory.  Also the transfer length updates for outgoing transfers wasn't
correct.

While being at it DPRINTFs have been replaced by tracepoints.

The isoch_pause logic has been disabled.  Not clear to me which propose
this serves and I think it is incorrect too as we just skip processing
itds.  Even when no xfer happens we have to clear the active bit.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 8b1ae3a..d807245 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -198,6 +198,7 @@ typedef struct EHCIitd {
 #define ITD_BUFPTR_MAXPKT_MASK   0x000007ff
 #define ITD_BUFPTR_MAXPKT_SH     0
 #define ITD_BUFPTR_MULT_MASK     0x00000003
+#define ITD_BUFPTR_MULT_SH       0
 } EHCIitd;
 
 /*  EHCI spec version 1.0 Section 3.4
@@ -628,7 +629,11 @@ static void ehci_trace_qtd(EHCIQueue *q, target_phys_addr_t addr, EHCIqtd *qtd)
 
 static void ehci_trace_itd(EHCIState *s, target_phys_addr_t addr, EHCIitd *itd)
 {
-    trace_usb_ehci_itd(addr, itd->next);
+    trace_usb_ehci_itd(addr, itd->next,
+                       get_field(itd->bufptr[1], ITD_BUFPTR_MAXPKT),
+                       get_field(itd->bufptr[2], ITD_BUFPTR_MULT),
+                       get_field(itd->bufptr[0], ITD_BUFPTR_EP),
+                       get_field(itd->bufptr[0], ITD_BUFPTR_DEVADDR));
 }
 
 /* queue management */
@@ -1270,41 +1275,51 @@ static int ehci_process_itd(EHCIState *ehci,
     USBPort *port;
     USBDevice *dev;
     int ret;
-    int i, j;
-    int ptr;
-    int pid;
-    int pg;
-    int len;
-    int dir;
-    int devadr;
-    int endp;
+    uint32_t i, j, len, len1, len2, pid, dir, devaddr, endp;
+    uint32_t pg, off, ptr1, ptr2, max, mult;
 
     dir =(itd->bufptr[1] & ITD_BUFPTR_DIRECTION);
-    devadr = get_field(itd->bufptr[0], ITD_BUFPTR_DEVADDR);
+    devaddr = get_field(itd->bufptr[0], ITD_BUFPTR_DEVADDR);
     endp = get_field(itd->bufptr[0], ITD_BUFPTR_EP);
-    /* maxpkt = get_field(itd->bufptr[1], ITD_BUFPTR_MAXPKT); */
+    max = get_field(itd->bufptr[1], ITD_BUFPTR_MAXPKT);
+    mult = get_field(itd->bufptr[2], ITD_BUFPTR_MULT);
 
     for(i = 0; i < 8; i++) {
         if (itd->transact[i] & ITD_XACT_ACTIVE) {
-            DPRINTF("ISOCHRONOUS active for frame %d, interval %d\n",
-                    ehci->frindex >> 3, i);
-
-            pg = get_field(itd->transact[i], ITD_XACT_PGSEL);
-            ptr = (itd->bufptr[pg] & ITD_BUFPTR_MASK) |
-                (itd->transact[i] & ITD_XACT_OFFSET_MASK);
-            len = get_field(itd->transact[i], ITD_XACT_LENGTH);
+            pg   = get_field(itd->transact[i], ITD_XACT_PGSEL);
+            off  = itd->transact[i] & ITD_XACT_OFFSET_MASK;
+            ptr1 = (itd->bufptr[pg] & ITD_BUFPTR_MASK);
+            ptr2 = (itd->bufptr[pg+1] & ITD_BUFPTR_MASK);
+            len  = get_field(itd->transact[i], ITD_XACT_LENGTH);
+
+            if (len > max * mult) {
+                len = max * mult;
+            }
 
             if (len > BUFF_SIZE) {
                 return USB_RET_PROCERR;
             }
 
-            DPRINTF("ISOCH: buffer %08X len %d\n", ptr, len);
+            if (off + len > 4096) {
+                /* transfer crosses page border */
+                len2 = off + len - 4096;
+                len1 = len - len2;
+            } else {
+                len1 = len;
+                len2 = 0;
+            }
 
             if (!dir) {
-                cpu_physical_memory_rw(ptr, &ehci->ibuffer[0], len, 0);
                 pid = USB_TOKEN_OUT;
-            } else
+                trace_usb_ehci_data(0, pg, off, ptr1 + off, len1, 0);
+                cpu_physical_memory_rw(ptr1 + off, &ehci->ibuffer[0], len1, 0);
+                if (len2) {
+                    trace_usb_ehci_data(0, pg+1, 0, ptr2, len2, len1);
+                    cpu_physical_memory_rw(ptr2, &ehci->ibuffer[len1], len2, 0);
+                }
+            } else {
                 pid = USB_TOKEN_IN;
+            }
 
             ret = USB_RET_NODEV;
 
@@ -1315,18 +1330,15 @@ static int ehci_process_itd(EHCIState *ehci,
                 // TODO sometime we will also need to check if we are the port owner
 
                 if (!(ehci->portsc[j] &(PORTSC_CONNECT))) {
-                    DPRINTF("Port %d, no exec, not connected(%08X)\n",
-                            j, ehci->portsc[j]);
                     continue;
                 }
 
                 ehci->ipacket.pid = pid;
-                ehci->ipacket.devaddr = devadr;
+                ehci->ipacket.devaddr = devaddr;
                 ehci->ipacket.devep = endp;
                 ehci->ipacket.data = ehci->ibuffer;
                 ehci->ipacket.len = len;
 
-                DPRINTF("calling usb_handle_packet\n");
                 ret = usb_handle_packet(dev, &ehci->ipacket);
 
                 if (ret != USB_RET_NODEV) {
@@ -1334,6 +1346,7 @@ static int ehci_process_itd(EHCIState *ehci,
                 }
             }
 
+#if 0
             /*  In isoch, there is no facility to indicate a NAK so let's
              *  instead just complete a zero-byte transaction.  Setting
              *  DBERR seems too draconian.
@@ -1358,24 +1371,40 @@ static int ehci_process_itd(EHCIState *ehci,
                 DPRINTF("ISOCH: received ACK, clearing pause\n");
                 ehci->isoch_pause = -1;
             }
+#else
+            if (ret == USB_RET_NAK) {
+                ret = 0;
+            }
+#endif
 
             if (ret >= 0) {
-                itd->transact[i] &= ~ITD_XACT_ACTIVE;
+                if (!dir) {
+                    /* OUT */
+                    set_field(&itd->transact[i], len - ret, ITD_XACT_LENGTH);
+                } else {
+                    /* IN */
+                    if (len1 > ret) {
+                        len1 = ret;
+                    }
+                    if (len2 > ret - len1) {
+                        len2 = ret - len1;
+                    }
+                    if (len1) {
+                        trace_usb_ehci_data(1, pg, off, ptr1 + off, len1, 0);
+                        cpu_physical_memory_rw(ptr1 + off, &ehci->ibuffer[0], len1, 1);
+                    }
+                    if (len2) {
+                        trace_usb_ehci_data(1, pg+1, 0, ptr2, len2, len1);
+                        cpu_physical_memory_rw(ptr2, &ehci->ibuffer[len1], len2, 1);
+                    }
+                    set_field(&itd->transact[i], ret, ITD_XACT_LENGTH);
+                }
 
                 if (itd->transact[i] & ITD_XACT_IOC) {
                     ehci_record_interrupt(ehci, USBSTS_INT);
                 }
             }
-
-            if (ret >= 0 && dir) {
-                cpu_physical_memory_rw(ptr, &ehci->ibuffer[0], len, 1);
-
-                if (ret != len) {
-                    DPRINTF("ISOCH IN expected %d, got %d\n",
-                            len, ret);
-                    set_field(&itd->transact[i], ret, ITD_XACT_LENGTH);
-                }
-            }
+            itd->transact[i] &= ~ITD_XACT_ACTIVE;
         }
     }
     return 0;
diff --git a/trace-events b/trace-events
index 51e2e7c..dd69702 100644
--- a/trace-events
+++ b/trace-events
@@ -203,7 +203,7 @@ disable usb_ehci_usbsts(const char *sts, int state) "usbsts %s %d"
 disable usb_ehci_state(const char *schedule, const char *state) "%s schedule %s"
 disable usb_ehci_qh(void *q, uint32_t addr, uint32_t next, uint32_t c_qtd, uint32_t n_qtd, uint32_t a_qtd, int rl, int mplen, int eps, int ep, int devaddr, int c, int h, int dtc, int i) "q %p - QH @ %08x: next %08x qtds %08x,%08x,%08x - rl %d, mplen %d, eps %d, ep %d, dev %d, c %d, h %d, dtc %d, i %d"
 disable usb_ehci_qtd(void *q, uint32_t addr, uint32_t next, uint32_t altnext, int tbytes, int cpage, int cerr, int pid, int ioc, int active, int halt, int babble, int xacterr) "q %p - QTD @ %08x: next %08x altnext %08x - tbytes %d, cpage %d, cerr %d, pid %d, ioc %d, active %d, halt %d, babble %d, xacterr %d"
-disable usb_ehci_itd(uint32_t addr, uint32_t next) "ITD @ %08x: next %08x"
+disable usb_ehci_itd(uint32_t addr, uint32_t next, uint32_t mplen, uint32_t mult, uint32_t ep, uint32_t devaddr) "ITD @ %08x: next %08x - mplen %d, mult %d, ep %d, dev %d"
 disable usb_ehci_port_attach(uint32_t port, const char *device) "attach port #%d - %s"
 disable usb_ehci_port_detach(uint32_t port) "detach port #%d"
 disable usb_ehci_port_reset(uint32_t port, int enable) "reset port #%d - %d"
-- 
1.7.1