Re: [Qemu-devel] [PATCH] ehci: handle dma errors

[Hans de Goede <hdegoede@redhat.com>]

Looks good, ack.


On 11/15/2012 01:22 PM, Gerd Hoffmann wrote:
> Starting with commit 1c380f9460522f32c8dd2577b2a53d518ec91c6d dma
> transfers can actually fail.  This patch makes ehci keep track
> of the busmaster bit in pci config space, by setting/clearing the
> dma_context pointer.  Attempts to dma without context will result
> in raising HSE (Host System Error) interrupt and stopping the host
> controller.
>
> This patch fixes WinXP not booting with a usb stick attached to ehci.
> Root cause is seabios activating ehci so you can boot from the stick,
> and WinXP clearing the busmaster bit before resetting the host
> controller, leading to ehci actually trying dma while it is disabled.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/usb/hcd-ehci-pci.c |   17 +++++++++++++
>   hw/usb/hcd-ehci.c     |   63 ++++++++++++++++++++++++++++++++++--------------
>   trace-events          |    1 +
>   3 files changed, 62 insertions(+), 19 deletions(-)
>
> diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
> index fe45a1f..5887eab 100644
> --- a/hw/usb/hcd-ehci-pci.c
> +++ b/hw/usb/hcd-ehci-pci.c
> @@ -17,6 +17,7 @@
>
>   #include "hw/usb/hcd-ehci.h"
>   #include "hw/pci.h"
> +#include "range.h"
>
>   typedef struct EHCIPCIState {
>       PCIDevice pcidev;
> @@ -79,6 +80,21 @@ static int usb_ehci_pci_initfn(PCIDevice *dev)
>       return 0;
>   }
>
> +static void usb_ehci_pci_write_config(PCIDevice *dev, uint32_t addr,
> +                                      uint32_t val, int l)
> +{
> +    EHCIPCIState *i = DO_UPCAST(EHCIPCIState, pcidev, dev);
> +    bool busmaster;
> +
> +    pci_default_write_config(dev, addr, val, l);
> +
> +    if (!range_covers_byte(addr, l, PCI_COMMAND)) {
> +        return;
> +    }
> +    busmaster = pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER;
> +    i->ehci.dma = busmaster ? pci_dma_context(dev) : NULL;
> +}
> +
>   static Property ehci_pci_properties[] = {
>       DEFINE_PROP_UINT32("maxframes", EHCIPCIState, ehci.maxframes, 128),
>       DEFINE_PROP_END_OF_LIST(),
> @@ -106,6 +122,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
>       k->device_id = i->device_id;
>       k->revision = i->revision;
>       k->class_id = PCI_CLASS_SERIAL_USB;
> +    k->config_write = usb_ehci_pci_write_config;
>       dc->vmsd = &vmstate_ehci_pci;
>       dc->props = ehci_pci_properties;
>   }
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 57da76c..df3aaed 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1000,21 +1000,25 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
>                                   *mmio, old);
>   }
>
> -
> -// TODO : Put in common header file, duplication from usb-ohci.c
> -
>   /* Get an array of dwords from main memory */
>   static inline int get_dwords(EHCIState *ehci, uint32_t addr,
>                                uint32_t *buf, int num)
>   {
>       int i;
>
> +    if (!ehci->dma) {
> +        ehci_raise_irq(ehci, USBSTS_HSE);
> +        ehci->usbcmd &= ~USBCMD_RUNSTOP;
> +        trace_usb_ehci_dma_error();
> +        return -1;
> +    }
> +
>       for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
>           dma_memory_read(ehci->dma, addr, buf, sizeof(*buf));
>           *buf = le32_to_cpu(*buf);
>       }
>
> -    return 1;
> +    return num;
>   }
>
>   /* Put an array of dwords in to main memory */
> @@ -1023,12 +1027,19 @@ static inline int put_dwords(EHCIState *ehci, uint32_t addr,
>   {
>       int i;
>
> +    if (!ehci->dma) {
> +        ehci_raise_irq(ehci, USBSTS_HSE);
> +        ehci->usbcmd &= ~USBCMD_RUNSTOP;
> +        trace_usb_ehci_dma_error();
> +        return -1;
> +    }
> +
>       for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
>           uint32_t tmp = cpu_to_le32(*buf);
>           dma_memory_write(ehci->dma, addr, &tmp, sizeof(tmp));
>       }
>
> -    return 1;
> +    return num;
>   }
>
>   /*
> @@ -1440,8 +1451,10 @@ static int ehci_state_waitlisthead(EHCIState *ehci,  int async)
>
>       /*  Find the head of the list (4.9.1.1) */
>       for(i = 0; i < MAX_QH; i++) {
> -        get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &qh,
> -                   sizeof(EHCIqh) >> 2);
> +        if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &qh,
> +                       sizeof(EHCIqh) >> 2) < 0) {
> +            return 0;
> +        }
>           ehci_trace_qh(NULL, NLPTR_GET(entry), &qh);
>
>           if (qh.epchar & QH_EPCHAR_H) {
> @@ -1538,8 +1551,11 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>           goto out;
>       }
>
> -    get_dwords(ehci, NLPTR_GET(q->qhaddr),
> -               (uint32_t *) &qh, sizeof(EHCIqh) >> 2);
> +    if (get_dwords(ehci, NLPTR_GET(q->qhaddr),
> +                   (uint32_t *) &qh, sizeof(EHCIqh) >> 2) < 0) {
> +        q = NULL;
> +        goto out;
> +    }
>       ehci_trace_qh(q, NLPTR_GET(q->qhaddr), &qh);
>
>       /*
> @@ -1626,8 +1642,10 @@ static int ehci_state_fetchitd(EHCIState *ehci, int async)
>       assert(!async);
>       entry = ehci_get_fetch_addr(ehci, async);
>
> -    get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &itd,
> -               sizeof(EHCIitd) >> 2);
> +    if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &itd,
> +                   sizeof(EHCIitd) >> 2) < 0) {
> +        return -1;
> +    }
>       ehci_trace_itd(ehci, entry, &itd);
>
>       if (ehci_process_itd(ehci, &itd, entry) != 0) {
> @@ -1650,8 +1668,10 @@ static int ehci_state_fetchsitd(EHCIState *ehci, int async)
>       assert(!async);
>       entry = ehci_get_fetch_addr(ehci, async);
>
> -    get_dwords(ehci, NLPTR_GET(entry), (uint32_t *)&sitd,
> -               sizeof(EHCIsitd) >> 2);
> +    if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *)&sitd,
> +                   sizeof(EHCIsitd) >> 2) < 0) {
> +        return 0;
> +    }
>       ehci_trace_sitd(ehci, entry, &sitd);
>
>       if (!(sitd.results & SITD_RESULTS_ACTIVE)) {
> @@ -1712,8 +1732,10 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
>       EHCIPacket *p;
>       int again = 1;
>
> -    get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), (uint32_t *) &qtd,
> -               sizeof(EHCIqtd) >> 2);
> +    if (get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), (uint32_t *) &qtd,
> +                   sizeof(EHCIqtd) >> 2) < 0) {
> +        return 0;
> +    }
>       ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);
>
>       p = QTAILQ_FIRST(&q->packets);
> @@ -1806,8 +1828,10 @@ static int ehci_fill_queue(EHCIPacket *p)
>                   goto leave;
>               }
>           }
> -        get_dwords(q->ehci, NLPTR_GET(qtdaddr),
> -                   (uint32_t *) &qtd, sizeof(EHCIqtd) >> 2);
> +        if (get_dwords(q->ehci, NLPTR_GET(qtdaddr),
> +                       (uint32_t *) &qtd, sizeof(EHCIqtd) >> 2) < 0) {
> +            return -1;
> +        }
>           ehci_trace_qtd(q, NLPTR_GET(qtdaddr), &qtd);
>           if (!(qtd.token & QTD_TOKEN_ACTIVE)) {
>               break;
> @@ -2106,8 +2130,9 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
>           }
>           list |= ((ehci->frindex & 0x1ff8) >> 1);
>
> -        dma_memory_read(ehci->dma, list, &entry, sizeof entry);
> -        entry = le32_to_cpu(entry);
> +        if (get_dwords(ehci, list, &entry, 1) < 0) {
> +            break;
> +        }
>
>           DPRINTF("PERIODIC state adv fr=%d.  [%08X] -> %08X\n",
>                   ehci->frindex / 8, list, entry);
> diff --git a/trace-events b/trace-events
> index 10813d5..913e00b 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -286,6 +286,7 @@ usb_ehci_irq(uint32_t level, uint32_t frindex, uint32_t sts, uint32_t mask) "lev
>   usb_ehci_guest_bug(const char *reason) "%s"
>   usb_ehci_doorbell_ring(void) ""
>   usb_ehci_doorbell_ack(void) ""
> +usb_ehci_dma_error(void) ""
>
>   # hw/usb/hcd-uhci.c
>   usb_uhci_reset(void) "=== RESET ==="
>