[Qemu-devel] [PATCH] ehci: check controller state when setting list registers

[Qemu-devel] [PATCH] ehci: check controller state when setting list registers

[David Ahern]

A prior change prohibited change the periodic and async list registers if
the respective list was enabled. This check should also have included
whether the controller is in the run state as well.

Additionally, added a check that the list registers have been set prior to
using it.

With this change I can boot a linux guest with device attached at boot, and I
can boot a linux guest from a physical USB key. e.g.,

qemu-system-x86_64 -m 1024 -smp 2 -enable-kvm -usb -usbdevice host:2.4

(A Fedora 13, 64-bit, minimal install boots in ~30 seconds from a USB key!)

Windows XP is still not happy at boot time with a device attached at boot
(spins on blank screen). The Vista beast is ok which suggests an issue with
WinXP. Both of these OS access the registers with the lists enabled AND
the controller in the run state.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index ab9a23e..6e455a2 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -778,19 +778,19 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
         break;
 
     case PERIODICLISTBASE:
-        if (s->usbcmd & USBCMD_PSE) {
-            fprintf(stderr, "Guest OS should not be setting the periodic"
-                   " list base register while periodic schedule is enabled\n");
-            return;
+        if ((s->usbcmd & USBCMD_PSE) && (s->usbcmd & USBCMD_RUNSTOP)) {
+            fprintf(stderr, 
+              "ehci: PERIODIC list base register set while periodic schedule\n"
+              "      is enabled and HC is enabled\n");
         }
-        DPRINTF("ehci_mem_writel: %s set to 0x%08X\n", str, val);
+        DPRINTF("ehci_mem_writel: P-LIST BASE set to 0x%08X\n", val);
         break;
 
     case ASYNCLISTADDR:
-        if (s->usbcmd & USBCMD_ASE) {
-            fprintf(stderr, "Guest OS should not be setting the async list"
-                   " address register while async schedule is enabled\n");
-            return;
+        if ((s->usbcmd & USBCMD_ASE) && (s->usbcmd & USBCMD_RUNSTOP)) {
+            fprintf(stderr, 
+              "ehci: ASYNC list address register set while async schedule\n"
+              "      is enabled and HC is enabled\n");
         }
         DPRINTF("ehci_mem_writel: A-LIST ADDR set to 0x%08X\n", val);
         break;
@@ -1810,6 +1810,11 @@ static void ehci_advance_async_state(EHCIState *ehci)
 
         DPRINTF_ST("ASYNC: waiting for listhead, starting at %08x\n",
                 ehci->asynclistaddr);
+        /* check that address register has been set */
+        if (ehci->asynclistaddr == 0) {
+            break;
+        }
+
         ehci->astate = ehci_advance_state(ehci, 1, EST_WAITLISTHEAD);
         break;
 
@@ -1853,6 +1858,10 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         }
 
         list = ehci->periodiclistbase & 0xfffff000;
+        /* check that register has been set */
+        if (list == 0) {
+            break;
+        }
         list |= ((ehci->frindex & 0x1ff8) >> 1);
 
         cpu_physical_memory_rw(list, (uint8_t *) &entry, sizeof entry, 0);
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH] ehci: check controller state when setting list registers

[Jan Kiszka]

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

David Ahern wrote:
> A prior change prohibited change the periodic and async list registers if
> the respective list was enabled. This check should also have included
> whether the controller is in the run state as well.
> 
> Additionally, added a check that the list registers have been set prior to
> using it.
> 
> With this change I can boot a linux guest with device attached at boot, and I
> can boot a linux guest from a physical USB key. e.g.,
> 
> qemu-system-x86_64 -m 1024 -smp 2 -enable-kvm -usb -usbdevice host:2.4
> 
> (A Fedora 13, 64-bit, minimal install boots in ~30 seconds from a USB key!)
> 
> Windows XP is still not happy at boot time with a device attached at boot
> (spins on blank screen). The Vista beast is ok which suggests an issue with
> WinXP. Both of these OS access the registers with the lists enabled AND
> the controller in the run state.
> 

Thanks, applied all three patch (with minor trailing whitespace fixup in
 1 and 3).

They improve disk pass-through for me as well. I'm now also able to boot
from some external USB disk. However, applying more heavy load, the disk
stops working at some point and even disconnects after a while.

The changes also modified the errors disk emulation give. I can attach
and write to such images now, but reading back seems to return different
data.

Jan

> Signed-off-by: David Ahern <daahern@cisco.com>
> ---
>  hw/usb-ehci.c |   27 ++++++++++++++++++---------
>  1 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
> index ab9a23e..6e455a2 100644
> --- a/hw/usb-ehci.c
> +++ b/hw/usb-ehci.c
> @@ -778,19 +778,19 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
>          break;
>  
>      case PERIODICLISTBASE:
> -        if (s->usbcmd & USBCMD_PSE) {
> -            fprintf(stderr, "Guest OS should not be setting the periodic"
> -                   " list base register while periodic schedule is enabled\n");
> -            return;
> +        if ((s->usbcmd & USBCMD_PSE) && (s->usbcmd & USBCMD_RUNSTOP)) {
> +            fprintf(stderr, 
> +              "ehci: PERIODIC list base register set while periodic schedule\n"
> +              "      is enabled and HC is enabled\n");
>          }
> -        DPRINTF("ehci_mem_writel: %s set to 0x%08X\n", str, val);
> +        DPRINTF("ehci_mem_writel: P-LIST BASE set to 0x%08X\n", val);
>          break;
>  
>      case ASYNCLISTADDR:
> -        if (s->usbcmd & USBCMD_ASE) {
> -            fprintf(stderr, "Guest OS should not be setting the async list"
> -                   " address register while async schedule is enabled\n");
> -            return;
> +        if ((s->usbcmd & USBCMD_ASE) && (s->usbcmd & USBCMD_RUNSTOP)) {
> +            fprintf(stderr, 
> +              "ehci: ASYNC list address register set while async schedule\n"
> +              "      is enabled and HC is enabled\n");
>          }
>          DPRINTF("ehci_mem_writel: A-LIST ADDR set to 0x%08X\n", val);
>          break;
> @@ -1810,6 +1810,11 @@ static void ehci_advance_async_state(EHCIState *ehci)
>  
>          DPRINTF_ST("ASYNC: waiting for listhead, starting at %08x\n",
>                  ehci->asynclistaddr);
> +        /* check that address register has been set */
> +        if (ehci->asynclistaddr == 0) {
> +            break;
> +        }
> +
>          ehci->astate = ehci_advance_state(ehci, 1, EST_WAITLISTHEAD);
>          break;
>  
> @@ -1853,6 +1858,10 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
>          }
>  
>          list = ehci->periodiclistbase & 0xfffff000;
> +        /* check that register has been set */
> +        if (list == 0) {
> +            break;
> +        }
>          list |= ((ehci->frindex & 0x1ff8) >> 1);
>  
>          cpu_physical_memory_rw(list, (uint8_t *) &entry, sizeof entry, 0);



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

[Qemu-devel] Re: [PATCH] ehci: check controller state when setting list registers

[David S. Ahern]

On 07/10/10 00:02, Jan Kiszka wrote:
> They improve disk pass-through for me as well. I'm now also able to boot
> from some external USB disk. However, applying more heavy load, the disk
> stops working at some point and even disconnects after a while.

I saw that as well. On the TO-DO list.

> The changes also modified the errors disk emulation give. I can attach
> and write to such images now, but reading back seems to return different
> data.

One change msd needs is the ep_wMaxPacketSize. Also I cannot get msd to
work with linux guests with either uhci or ehci. Some debugging is
needed. e.g., impacts of the hack comments usb-msd.c (case insensitive
search)?

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 09a6a33..87c306e 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -124,7 +124,7 @@ static const uint8_t qemu_msd_config_descriptor[] = {
        0x05,       /*  u8  ep_bDescriptorType; Endpoint */
        0x81,       /*  u8  ep_bEndpointAddress; IN Endpoint 1 */
        0x02,       /*  u8  ep_bmAttributes; Bulk */
-       0x40, 0x00, /*  u16 ep_wMaxPacketSize; */
+       0x00, 0x02, /*  u16 ep_wMaxPacketSize; */
        0x00,       /*  u8  ep_bInterval; */

        /* Bulk-Out endpoint */
@@ -132,7 +132,7 @@ static const uint8_t qemu_msd_config_descriptor[] = {
        0x05,       /*  u8  ep_bDescriptorType; Endpoint */
        0x02,       /*  u8  ep_bEndpointAddress; OUT Endpoint 2 */
        0x02,       /*  u8  ep_bmAttributes; Bulk */
-       0x40, 0x00, /*  u16 ep_wMaxPacketSize; */
+       0x00, 0x02, /*  u16 ep_wMaxPacketSize; */
        0x00        /*  u8  ep_bInterval; */
 };