From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TYzEZ-0004ys-7I for qemu-devel@nongnu.org; Thu, 15 Nov 2012 08:11:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TYzEW-0002E2-4j for qemu-devel@nongnu.org; Thu, 15 Nov 2012 08:11:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TYzEV-0002Dv-Sw for qemu-devel@nongnu.org; Thu, 15 Nov 2012 08:11:52 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAFDBpeg001623 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 15 Nov 2012 08:11:51 -0500 Message-ID: <50A4EA8C.4000507@redhat.com> Date: Thu, 15 Nov 2012 14:13:48 +0100 From: Hans de Goede MIME-Version: 1.0 References: <1352982136-30761-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1352982136-30761-1-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ehci: handle dma errors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org 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 > --- > 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 ===" >