[Qemu-devel] [PATCH v3] hcd-ohci: add dma error handling

[Qemu-devel] [PATCH v3] hcd-ohci: add dma error handling

[Alexey Kardashevskiy]

Current hcd-ohci does not handle DMA errors. However they may happen
so here we introduce simple error handling.

On such errors, a typical OHCI will stop operating, signal the guest
about the error by sending "UnrecoverableError Event", set itself into
error state and set "Detected Parity Error" in its PCI config space
to signal that it got an error and so does the patch.

This also adds ohci_stop() call to ohci_bus_start() to handle possible
failure of qemu_new_timer_ns().

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

---
Changes:
2013/07/25:
* added ohci_stop to ohci_bus_start
* added ohci_bus_stop to ohci_stop
* added return to ohci_frame_boundary() if UnrecoverableError happened to
prevent ohci_sof from failing as the timer is freed at that point

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/usb/hcd-ohci.c | 173 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 124 insertions(+), 49 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 2bab8ff..346adde 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -22,7 +22,6 @@
  *  o Allocate bandwidth in frames properly
  *  o Disable timers when nothing needs to be done, or remove timer usage
  *    all together.
- *  o Handle unrecoverable errors properly
  *  o BIOS work to boot from USB storage
 */
 
@@ -308,6 +307,8 @@ struct ohci_iso_td {
 
 #define OHCI_HRESET_FSBIR       (1 << 0)
 
+static void ohci_stop(OHCIState *ohci);
+
 /* Update IRQ levels */
 static inline void ohci_intr_update(OHCIState *ohci)
 {
@@ -508,11 +509,13 @@ static inline int get_dwords(OHCIState *ohci,
     addr += ohci->localmem_base;
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ohci->as, addr, buf, sizeof(*buf));
+        if (dma_memory_read(ohci->as, addr, buf, sizeof(*buf))) {
+            return -1;
+        }
         *buf = le32_to_cpu(*buf);
     }
 
-    return 1;
+    return 0;
 }
 
 /* Put an array of dwords in to main memory */
@@ -525,10 +528,12 @@ static inline int put_dwords(OHCIState *ohci,
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint32_t tmp = cpu_to_le32(*buf);
-        dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp));
+        if (dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp))) {
+            return -1;
+        }
     }
 
-    return 1;
+    return 0;
 }
 
 /* Get an array of words from main memory */
@@ -540,11 +545,13 @@ static inline int get_words(OHCIState *ohci,
     addr += ohci->localmem_base;
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ohci->as, addr, buf, sizeof(*buf));
+        if (dma_memory_read(ohci->as, addr, buf, sizeof(*buf))) {
+            return -1;
+        }
         *buf = le16_to_cpu(*buf);
     }
 
-    return 1;
+    return 0;
 }
 
 /* Put an array of words in to main memory */
@@ -557,10 +564,12 @@ static inline int put_words(OHCIState *ohci,
 
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint16_t tmp = cpu_to_le16(*buf);
-        dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp));
+        if (dma_memory_write(ohci->as, addr, &tmp, sizeof(tmp))) {
+            return -1;
+        }
     }
 
-    return 1;
+    return 0;
 }
 
 static inline int ohci_read_ed(OHCIState *ohci,
@@ -578,15 +587,15 @@ static inline int ohci_read_td(OHCIState *ohci,
 static inline int ohci_read_iso_td(OHCIState *ohci,
                                    dma_addr_t addr, struct ohci_iso_td *td)
 {
-    return (get_dwords(ohci, addr, (uint32_t *)td, 4) &&
+    return (get_dwords(ohci, addr, (uint32_t *)td, 4) ||
             get_words(ohci, addr + 16, td->offset, 8));
 }
 
 static inline int ohci_read_hcca(OHCIState *ohci,
                                  dma_addr_t addr, struct ohci_hcca *hcca)
 {
-    dma_memory_read(ohci->as, addr + ohci->localmem_base, hcca, sizeof(*hcca));
-    return 1;
+    return dma_memory_read(ohci->as, addr + ohci->localmem_base,
+                           hcca, sizeof(*hcca));
 }
 
 static inline int ohci_put_ed(OHCIState *ohci,
@@ -610,23 +619,22 @@ static inline int ohci_put_td(OHCIState *ohci,
 static inline int ohci_put_iso_td(OHCIState *ohci,
                                   dma_addr_t addr, struct ohci_iso_td *td)
 {
-    return (put_dwords(ohci, addr, (uint32_t *)td, 4) &&
+    return (put_dwords(ohci, addr, (uint32_t *)td, 4) ||
             put_words(ohci, addr + 16, td->offset, 8));
 }
 
 static inline int ohci_put_hcca(OHCIState *ohci,
                                 dma_addr_t addr, struct ohci_hcca *hcca)
 {
-    dma_memory_write(ohci->as,
-                     addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET,
-                     (char *)hcca + HCCA_WRITEBACK_OFFSET,
-                     HCCA_WRITEBACK_SIZE);
-    return 1;
+    return dma_memory_write(ohci->as,
+                            addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET,
+                            (char *)hcca + HCCA_WRITEBACK_OFFSET,
+                            HCCA_WRITEBACK_SIZE);
 }
 
 /* Read/Write the contents of a TD from/to main memory.  */
-static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
-                         uint8_t *buf, int len, DMADirection dir)
+static int ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
+                        uint8_t *buf, int len, DMADirection dir)
 {
     dma_addr_t ptr, n;
 
@@ -634,18 +642,26 @@ static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
     n = 0x1000 - (ptr & 0xfff);
     if (n > len)
         n = len;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir);
-    if (n == len)
-        return;
+
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir)) {
+        return -1;
+    }
+    if (n == len) {
+        return 0;
+    }
     ptr = td->be & ~0xfffu;
     buf += n;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, len - n, dir);
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf,
+                      len - n, dir)) {
+        return -1;
+    }
+    return 0;
 }
 
 /* Read/Write the contents of an ISO TD from/to main memory.  */
-static void ohci_copy_iso_td(OHCIState *ohci,
-                             uint32_t start_addr, uint32_t end_addr,
-                             uint8_t *buf, int len, DMADirection dir)
+static int ohci_copy_iso_td(OHCIState *ohci,
+                            uint32_t start_addr, uint32_t end_addr,
+                            uint8_t *buf, int len, DMADirection dir)
 {
     dma_addr_t ptr, n;
 
@@ -653,12 +669,20 @@ static void ohci_copy_iso_td(OHCIState *ohci,
     n = 0x1000 - (ptr & 0xfff);
     if (n > len)
         n = len;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir);
-    if (n == len)
-        return;
+
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, n, dir)) {
+        return -1;
+    }
+    if (n == len) {
+        return 0;
+    }
     ptr = end_addr & ~0xfffu;
     buf += n;
-    dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf, len - n, dir);
+    if (dma_memory_rw(ohci->as, ptr + ohci->localmem_base, buf,
+                      len - n, dir)) {
+        return -1;
+    }
+    return 0;
 }
 
 static void ohci_process_lists(OHCIState *ohci, int completion);
@@ -698,8 +722,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
 
     addr = ed->head & OHCI_DPTR_MASK;
 
-    if (!ohci_read_iso_td(ohci, addr, &iso_td)) {
+    if (ohci_read_iso_td(ohci, addr, &iso_td)) {
         printf("usb-ohci: ISO_TD read error at %x\n", addr);
+        ohci_stop(ohci);
         return 0;
     }
 
@@ -740,7 +765,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         i = OHCI_BM(iso_td.flags, TD_DI);
         if (i < ohci->done_count)
             ohci->done_count = i;
-        ohci_put_iso_td(ohci, addr, &iso_td);
+        if (ohci_put_iso_td(ohci, addr, &iso_td)) {
+            ohci_stop(ohci);
+            return 1;
+        }
         return 0;
     }
 
@@ -821,8 +849,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     }
 
     if (len && dir != OHCI_TD_DIR_IN) {
-        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
-                         DMA_DIRECTION_TO_DEVICE);
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
+                             DMA_DIRECTION_TO_DEVICE)) {
+            ohci_stop(ohci);
+            return 1;
+        }
     }
 
     if (!completion) {
@@ -852,8 +883,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     /* Writeback */
     if (dir == OHCI_TD_DIR_IN && ret >= 0 && ret <= len) {
         /* IN transfer succeeded */
-        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
-                         DMA_DIRECTION_FROM_DEVICE);
+        if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
+                             DMA_DIRECTION_FROM_DEVICE)) {
+            ohci_stop(ohci);
+            return 1;
+        }
         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC,
                     OHCI_CC_NOERROR);
         OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, ret);
@@ -910,7 +944,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         if (i < ohci->done_count)
             ohci->done_count = i;
     }
-    ohci_put_iso_td(ohci, addr, &iso_td);
+    if (ohci_put_iso_td(ohci, addr, &iso_td)) {
+        ohci_stop(ohci);
+    }
     return 1;
 }
 
@@ -943,8 +979,9 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
 #endif
         return 1;
     }
-    if (!ohci_read_td(ohci, addr, &td)) {
+    if (ohci_read_td(ohci, addr, &td)) {
         fprintf(stderr, "usb-ohci: TD read error at %x\n", addr);
+        ohci_stop(ohci);
         return 0;
     }
 
@@ -997,8 +1034,10 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
                 pktlen = len;
             }
             if (!completion) {
-                ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
-                             DMA_DIRECTION_TO_DEVICE);
+                if (ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen,
+                                 DMA_DIRECTION_TO_DEVICE)) {
+                    ohci_stop(ohci);
+                }
             }
         }
     }
@@ -1055,8 +1094,10 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
 
     if (ret >= 0) {
         if (dir == OHCI_TD_DIR_IN) {
-            ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
-                         DMA_DIRECTION_FROM_DEVICE);
+            if (ohci_copy_td(ohci, &td, ohci->usb_buf, ret,
+                             DMA_DIRECTION_FROM_DEVICE)) {
+                ohci_stop(ohci);
+            }
 #ifdef DEBUG_PACKET
             DPRINTF("  data:");
             for (i = 0; i < ret; i++)
@@ -1133,7 +1174,10 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
     if (i < ohci->done_count)
         ohci->done_count = i;
 exit_no_retire:
-    ohci_put_td(ohci, addr, &td);
+    if (ohci_put_td(ohci, addr, &td)) {
+        ohci_stop(ohci);
+        return 1;
+    }
     return OHCI_BM(td.flags, TD_CC) != OHCI_CC_NOERROR;
 }
 
@@ -1151,8 +1195,9 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
         return 0;
 
     for (cur = head; cur; cur = next_ed) {
-        if (!ohci_read_ed(ohci, cur, &ed)) {
+        if (ohci_read_ed(ohci, cur, &ed)) {
             fprintf(stderr, "usb-ohci: ED read error at %x\n", cur);
+            ohci_stop(ohci);
             return 0;
         }
 
@@ -1194,7 +1239,10 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
             }
         }
 
-        ohci_put_ed(ohci, cur, &ed);
+        if (ohci_put_ed(ohci, cur, &ed)) {
+            ohci_stop(ohci);
+            return 0;
+        }
     }
 
     return active;
@@ -1236,7 +1284,11 @@ static void ohci_frame_boundary(void *opaque)
     OHCIState *ohci = opaque;
     struct ohci_hcca hcca;
 
-    ohci_read_hcca(ohci, ohci->hcca, &hcca);
+    if (ohci_read_hcca(ohci, ohci->hcca, &hcca)) {
+        fprintf(stderr, "usb-ohci: HCCA read error at %x\n", ohci->hcca);
+        ohci_stop(ohci);
+        return;
+    }
 
     /* Process all the lists at the end of the frame */
     if (ohci->ctl & OHCI_CTL_PLE) {
@@ -1257,6 +1309,11 @@ static void ohci_frame_boundary(void *opaque)
     ohci->old_ctl = ohci->ctl;
     ohci_process_lists(ohci, 0);
 
+    /* Stop if UnrecoverableError happened or ohci_sof will crash */
+    if (ohci->intr_status & OHCI_INTR_UE) {
+        return;
+    }
+
     /* Frame boundary, so do EOF stuf here */
     ohci->frt = ohci->fit;
 
@@ -1282,7 +1339,9 @@ static void ohci_frame_boundary(void *opaque)
     ohci_sof(ohci);
 
     /* Writeback HCCA */
-    ohci_put_hcca(ohci, ohci->hcca, &hcca);
+    if (ohci_put_hcca(ohci, ohci->hcca, &hcca)) {
+        ohci_stop(ohci);
+    }
 }
 
 /* Start sending SOF tokens across the USB bus, lists are processed in
@@ -1296,7 +1355,7 @@ static int ohci_bus_start(OHCIState *ohci)
 
     if (ohci->eof_timer == NULL) {
         fprintf(stderr, "usb-ohci: %s: qemu_new_timer_ns failed\n", ohci->name);
-        /* TODO: Signal unrecoverable error */
+        ohci_stop(ohci);
         return 0;
     }
 
@@ -1857,6 +1916,22 @@ typedef struct {
     uint32_t firstport;
 } OHCIPCIState;
 
+/** A typical O/EHCI will stop operating, set itself into error state
+ * (which can be queried by MMIO) and will set PERR in its config
+ * space to signal that it got an error
+ */
+static void ohci_stop(OHCIState *ohci)
+{
+    OHCIPCIState *dev = container_of(ohci, OHCIPCIState, state);
+
+    fprintf(stderr, "%s: DMA error\n", __func__);
+
+    ohci_set_interrupt(ohci, OHCI_INTR_UE);
+    ohci_bus_stop(ohci);
+    pci_set_word(dev->parent_obj.config + PCI_STATUS,
+                 PCI_STATUS_DETECTED_PARITY);
+}
+
 static int usb_ohci_initfn_pci(PCIDevice *dev)
 {
     OHCIPCIState *ohci = PCI_OHCI(dev);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3] hcd-ohci: add dma error handling

[Gerd Hoffmann]

On 07/26/13 11:10, Alexey Kardashevskiy wrote:
> Current hcd-ohci does not handle DMA errors. However they may happen
> so here we introduce simple error handling.
> 
> On such errors, a typical OHCI will stop operating, signal the guest
> about the error by sending "UnrecoverableError Event", set itself into
> error state and set "Detected Parity Error" in its PCI config space
> to signal that it got an error and so does the patch.
> 
> This also adds ohci_stop() call to ohci_bus_start() to handle possible
> failure of qemu_new_timer_ns().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Looks sane to me now.  When I get an ack from Benjamin I'll go put it
into the usb patch queue.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v3] hcd-ohci: add dma error handling

[Benjamin Herrenschmidt]

On Fri, 2013-07-26 at 11:58 +0200, Gerd Hoffmann wrote:
> On 07/26/13 11:10, Alexey Kardashevskiy wrote:
> > Current hcd-ohci does not handle DMA errors. However they may happen
> > so here we introduce simple error handling.
> > 
> > On such errors, a typical OHCI will stop operating, signal the guest
> > about the error by sending "UnrecoverableError Event", set itself into
> > error state and set "Detected Parity Error" in its PCI config space
> > to signal that it got an error and so does the patch.
> > 
> > This also adds ohci_stop() call to ohci_bus_start() to handle possible
> > failure of qemu_new_timer_ns().
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Looks sane to me now.  When I get an ack from Benjamin I'll go put it
> into the usb patch queue.

>From me ? Heh, I barely remember what the code looks like in there, if
you are both happy with it I don't think you need me :-)

The only possibly comment is that I would have called ohci_stop() something
a bit clearer such as ohci_die() or ohci_buserror() ... IE. That function
does more than just "stop", it also signals an error, and that attribute
should probably be described by the function name.

Cheers,
Ben.

Re: [Qemu-devel] [PATCH v3] hcd-ohci: add dma error handling

[Gerd Hoffmann]

On 07/26/13 12:10, Benjamin Herrenschmidt wrote:
> On Fri, 2013-07-26 at 11:58 +0200, Gerd Hoffmann wrote:
>> On 07/26/13 11:10, Alexey Kardashevskiy wrote:
>>> Current hcd-ohci does not handle DMA errors. However they may happen
>>> so here we introduce simple error handling.
>>>
>>> On such errors, a typical OHCI will stop operating, signal the guest
>>> about the error by sending "UnrecoverableError Event", set itself into
>>> error state and set "Detected Parity Error" in its PCI config space
>>> to signal that it got an error and so does the patch.
>>>
>>> This also adds ohci_stop() call to ohci_bus_start() to handle possible
>>> failure of qemu_new_timer_ns().
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> Looks sane to me now.  When I get an ack from Benjamin I'll go put it
>> into the usb patch queue.
> 
> From me ? Heh, I barely remember what the code looks like in there, if
> you are both happy with it I don't think you need me :-)

Well, ppc emulation uses ohci so I through an ack from a ppc person
which also commented on previous revisions would be nice to have ...

> The only possibly comment is that I would have called ohci_stop() something
> a bit clearer such as ohci_die() or ohci_buserror() ... IE. That function
> does more than just "stop", it also signals an error, and that attribute
> should probably be described by the function name.

xhci has an xhci_die() function for simliar purposes so I think
ohci_die() would be a good pick.

cheers,
  Gerd