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

[Jan Kiszka]

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 <address@hidden>
> ---
>  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);

signature.asc
Description: OpenPGP digital signature