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

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

[Alexey Kardashevskiy]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Current hcd-ohci does not handle DMA errors which can actually
happen.

However it is not clear what approach should be used here -
for example, get_dwords returns positive number saying that there
is no error as all the callers consider the return value as fail
if it is less than zero. Normally you would expect bool=true/int=0
as success and bool=false/int=-1 as fail.

Any suggestion?

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

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index a096ecf..48297da 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -508,7 +508,9 @@ 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);
     }
 
@@ -525,7 +527,9 @@ 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;
@@ -540,7 +544,9 @@ 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);
     }
 
@@ -557,7 +563,9 @@ 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;
@@ -585,8 +593,8 @@ static inline int ohci_read_iso_td(OHCIState *ohci,
 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)) ? -1 : 0;
 }
 
 static inline int ohci_put_ed(OHCIState *ohci,
@@ -617,16 +625,15 @@ static inline int ohci_put_iso_td(OHCIState *ohci,
 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) ? -1 : 0;
 }
 
 /* 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 +641,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 +668,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);
@@ -1236,7 +1259,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);
+        /* TODO: Add signaling of DMA errors to the guest */
+        return;
+    }
 
     /* Process all the lists at the end of the frame */
     if (ohci->ctl & OHCI_CTL_PLE) {
-- 
1.8.3.2

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

[Benjamin Herrenschmidt]

On Wed, 2013-07-17 at 19:46 +1000, Alexey Kardashevskiy wrote:
> Current hcd-ohci does not handle DMA errors which can actually
> happen.
> 
> However it is not clear what approach should be used here -
> for example, get_dwords returns positive number saying that there
> is no error as all the callers consider the return value as fail
> if it is less than zero. Normally you would expect bool=true/int=0
> as success and bool=false/int=-1 as fail.
> 
> Any suggestion?

The right thing to do is not only to bring the error up the stack, but
essentially to set the error bits in the PCI command status and put the
whole HCI in error state (and stop operating)

That how real HW reacts.

Cheers,
Ben.

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

[Alexander Graf]

On 17.07.2013, at 13:15, Benjamin Herrenschmidt wrote:

> On Wed, 2013-07-17 at 19:46 +1000, Alexey Kardashevskiy wrote:
>> Current hcd-ohci does not handle DMA errors which can actually
>> happen.
>> 
>> However it is not clear what approach should be used here -
>> for example, get_dwords returns positive number saying that there
>> is no error as all the callers consider the return value as fail
>> if it is less than zero. Normally you would expect bool=true/int=0
>> as success and bool=false/int=-1 as fail.
>> 
>> Any suggestion?
> 
> The right thing to do is not only to bring the error up the stack, but
> essentially to set the error bits in the PCI command status and put the
> whole HCI in error state (and stop operating)
> 
> That how real HW reacts.

Who does that? I always assumed it's the IOMMU that kills the device when it accesses regions it's not allowed to access. On real hardware, memory transfers don't have error return codes, do they?


Alex

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

[Benjamin Herrenschmidt]

On Wed, 2013-07-17 at 14:31 +0200, Alexander Graf wrote:
> On 17.07.2013, at 13:15, Benjamin Herrenschmidt wrote:
> 
> > On Wed, 2013-07-17 at 19:46 +1000, Alexey Kardashevskiy wrote:
> >> Current hcd-ohci does not handle DMA errors which can actually
> >> happen.
> >> 
> >> However it is not clear what approach should be used here -
> >> for example, get_dwords returns positive number saying that there
> >> is no error as all the callers consider the return value as fail
> >> if it is less than zero. Normally you would expect bool=true/int=0
> >> as success and bool=false/int=-1 as fail.
> >> 
> >> Any suggestion?
> > 
> > The right thing to do is not only to bring the error up the stack, but
> > essentially to set the error bits in the PCI command status and put the
> > whole HCI in error state (and stop operating)
> > 
> > That how real HW reacts.
> 
> Who does that? I always assumed it's the IOMMU that kills the device
> when it accesses regions it's not allowed to access.

Hah, no, iommu's only "kill devices" on fancy HW like powerpc :-)

On these, when any kind of error occur, we isolate the entire thing.

>  On real hardware, memory transfers don't have error return codes, do they?

No they sort-of do :-)

For example, on PCI, there are 3 common causes of errors: Parity, Target
Aborts and Master Aborts. The former is somewhat obvious, the second
means the target aborted the cycle before completion, the latter usually
means no target responded (timeout). There are two physical lines used
to convey error informations (and potentially abort cycles), PERR and
SERR.

Depending on the details of the bus protocol, the error causes can be a
bit different. On PCIe you can actually shoot error messages up the
link, transactions are packets and can result in an error response,
etc...

Since qemu mostly emulates PCI, let's stick to that. An iommu error will
typically be a target abort. So the device should react as such.

A typical O/EHCI will stop operating, set itself into error state (which
can be queried by MMIO) and will set something like PERR in its config
space to signal that it got an error.

Cheers,
Ben.