[Qemu-devel] [PATCH] ehci: Improve detection of invalid entries in the async schedule

[Qemu-devel] [PATCH] ehci: Improve detection of invalid entries in the async schedule

[David Ahern]

Hi Jan:

Another patch for the ehci branch - emptying out my patch queue as I need to set this aside for a while. 

I am testing qemu and qemu-kvm with 3 different USB keys -- all Kingstons of various sizes 1 GB, 4GB and 8GB. I am definitely seeing different behaviors with the 4GB and 8GB working really well. With the 4GB key I am seeing data rates of 5MB/sec with qemu and close to 8MB/sec with kvm. The 1 GB key hits hiccups from time to time. 

If you see the stall enable debugging in usb-linux:

--- a/usb-linux.c
+++ b/usb-linux.c
@@ -243,7 +243,7 @@ static void async_complete(void *opaque)
 
         p = aurb->packet;
 
-        DPRINTF("husb: async completed. aurb %p status %d alen %d\n",
+        printf("husb: async completed. aurb %p status %d alen %d\n",
                 aurb, aurb->urb.status, aurb->urb.actual_length);
 
         if (p) {

David

[Qemu-devel] [PATCH] Improve detection of invalid entries in the async schedule

[David Ahern]

Signed-off-by: David Ahern <daahern@cisco.com>
---
 hw/usb-ehci.c |   41 +++++++++++++++++++++++++++++------------
 1 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 29baf74..e2f8e54 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -144,7 +144,7 @@
 #define NB_MAXINTRATE    8        // Max rate at which controller issues ints
 #define NB_PORTS         4        // Number of downstream ports
 #define BUFF_SIZE        5*4096   // Max bytes to transfer per transaction
-#define MAX_ITERATIONS   50       // Max number of states before we abort
+#define MAX_ITERATIONS   20       // Max number of QH before we break the loop
 #define MAX_QH           100      // Max allowable queue heads in a chain
 
 /*  Internal periodic / asynchronous schedule state machine states
@@ -1319,6 +1319,11 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async, int *state)
         }
     }
 #endif
+    if (entry < 0x1000) {
+        DPRINTF("fetchentry: entry invalid (0x%08x)\n", entry);
+        *state = EST_ACTIVE;
+        goto out;
+    }
 
     /* section 4.8, only QH in async schedule */
     if (async && (NLPTR_TYPE_GET(entry) != NLPTR_TYPE_QH)) {
@@ -1348,6 +1353,7 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async, int *state)
         return -1;
     }
 
+out:
     return again;
 }
 
@@ -1394,7 +1400,7 @@ static int ehci_state_fetchqh(EHCIState *ehci, int async, int *state)
         *state = EST_HORIZONTALQH;
         again = 1;
 
-    } else if (qh->token & QTD_TOKEN_ACTIVE) {
+    } else if ((qh->token & QTD_TOKEN_ACTIVE) && (qh->current_qtd > 0x1000)) {
         DPRINTF_ST("FETCHQH: Active, !Halt, execute - fetch qTD\n");
         ehci->qtdaddr = qh->current_qtd;
         *state = EST_FETCHQTD;
@@ -1499,10 +1505,17 @@ static int ehci_state_fetchqtd(EHCIState *ehci, int async, int *state)
 
 static int ehci_state_horizqh(EHCIState *ehci, int async, int *state)
 {
-    ehci->fetch_addr = ehci->qh.next;
-    *state = EST_FETCHENTRY;
+    int again = 0;
 
-    return 1;
+    if (ehci->fetch_addr != ehci->qh.next) {
+        ehci->fetch_addr = ehci->qh.next;
+        *state = EST_FETCHENTRY;
+        again = 1;
+    } else {
+        *state = EST_ACTIVE;
+    }
+
+    return again;
 }
 
 static int ehci_state_execute(EHCIState *ehci, int async, int *state)
@@ -1682,11 +1695,17 @@ static int ehci_advance_state(EHCIState *ehci,
     int iter = 0;
 
     do {
-        iter++;
-        if (iter > MAX_ITERATIONS) {
-            DPRINTF("\n*** advance_state: bailing on MAX ITERATIONS***\n\n"); 
-            state = EST_ACTIVE;
-            break;
+        if (state == EST_FETCHQH) {
+            iter++;
+            /* if we are roaming a lot of QH without executing a qTD
+             * something is wrong with the linked list. TO-DO: why is
+             * this hack needed?
+             */
+            if (iter > MAX_ITERATIONS) {
+                DPRINTF("\n*** advance_state: bailing on MAX ITERATIONS***\n"); 
+                state = EST_ACTIVE;
+                break;
+            }
         }
         switch(state) {
         case EST_WAITLISTHEAD:
@@ -1723,12 +1742,10 @@ static int ehci_advance_state(EHCIState *ehci,
             break;
 
         case EST_EXECUTING:
-            iter = 0;
             again = ehci_state_executing(ehci, async, &state);
             break;
 
         case EST_WRITEBACK:
-            iter = 0;
             again = ehci_state_writeback(ehci, async, &state);
             break;
 
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] Improve detection of invalid entries in the async schedule

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

David Ahern wrote:
> Signed-off-by: David Ahern <daahern@cisco.com>

Thanks, merged (with a tiny cleanup).

Jan

> ---
>  hw/usb-ehci.c |   41 +++++++++++++++++++++++++++++------------
>  1 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
> index 29baf74..e2f8e54 100644
> --- a/hw/usb-ehci.c
> +++ b/hw/usb-ehci.c
> @@ -144,7 +144,7 @@
>  #define NB_MAXINTRATE    8        // Max rate at which controller issues ints
>  #define NB_PORTS         4        // Number of downstream ports
>  #define BUFF_SIZE        5*4096   // Max bytes to transfer per transaction
> -#define MAX_ITERATIONS   50       // Max number of states before we abort
> +#define MAX_ITERATIONS   20       // Max number of QH before we break the loop
>  #define MAX_QH           100      // Max allowable queue heads in a chain
>  
>  /*  Internal periodic / asynchronous schedule state machine states
> @@ -1319,6 +1319,11 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async, int *state)
>          }
>      }
>  #endif
> +    if (entry < 0x1000) {
> +        DPRINTF("fetchentry: entry invalid (0x%08x)\n", entry);
> +        *state = EST_ACTIVE;
> +        goto out;
> +    }
>  
>      /* section 4.8, only QH in async schedule */
>      if (async && (NLPTR_TYPE_GET(entry) != NLPTR_TYPE_QH)) {
> @@ -1348,6 +1353,7 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async, int *state)
>          return -1;
>      }
>  
> +out:
>      return again;
>  }
>  
> @@ -1394,7 +1400,7 @@ static int ehci_state_fetchqh(EHCIState *ehci, int async, int *state)
>          *state = EST_HORIZONTALQH;
>          again = 1;
>  
> -    } else if (qh->token & QTD_TOKEN_ACTIVE) {
> +    } else if ((qh->token & QTD_TOKEN_ACTIVE) && (qh->current_qtd > 0x1000)) {
>          DPRINTF_ST("FETCHQH: Active, !Halt, execute - fetch qTD\n");
>          ehci->qtdaddr = qh->current_qtd;
>          *state = EST_FETCHQTD;
> @@ -1499,10 +1505,17 @@ static int ehci_state_fetchqtd(EHCIState *ehci, int async, int *state)
>  
>  static int ehci_state_horizqh(EHCIState *ehci, int async, int *state)
>  {
> -    ehci->fetch_addr = ehci->qh.next;
> -    *state = EST_FETCHENTRY;
> +    int again = 0;
>  
> -    return 1;
> +    if (ehci->fetch_addr != ehci->qh.next) {
> +        ehci->fetch_addr = ehci->qh.next;
> +        *state = EST_FETCHENTRY;
> +        again = 1;
> +    } else {
> +        *state = EST_ACTIVE;
> +    }
> +
> +    return again;
>  }
>  
>  static int ehci_state_execute(EHCIState *ehci, int async, int *state)
> @@ -1682,11 +1695,17 @@ static int ehci_advance_state(EHCIState *ehci,
>      int iter = 0;
>  
>      do {
> -        iter++;
> -        if (iter > MAX_ITERATIONS) {
> -            DPRINTF("\n*** advance_state: bailing on MAX ITERATIONS***\n\n"); 
> -            state = EST_ACTIVE;
> -            break;
> +        if (state == EST_FETCHQH) {
> +            iter++;
> +            /* if we are roaming a lot of QH without executing a qTD
> +             * something is wrong with the linked list. TO-DO: why is
> +             * this hack needed?
> +             */
> +            if (iter > MAX_ITERATIONS) {
> +                DPRINTF("\n*** advance_state: bailing on MAX ITERATIONS***\n"); 
> +                state = EST_ACTIVE;
> +                break;
> +            }
>          }
>          switch(state) {
>          case EST_WAITLISTHEAD:
> @@ -1723,12 +1742,10 @@ static int ehci_advance_state(EHCIState *ehci,
>              break;
>  
>          case EST_EXECUTING:
> -            iter = 0;
>              again = ehci_state_executing(ehci, async, &state);
>              break;
>  
>          case EST_WRITEBACK:
> -            iter = 0;
>              again = ehci_state_writeback(ehci, async, &state);
>              break;
>  



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]