[Qemu-devel] [PATCH] usb: Fix host-side endian bugs in hcd-ehci

[David Gibson]

The EHCI device model is horribly broken for big-endian hosts.  It uses a
union of 'mmio' a byte array which is as-is as the device's MMIO space
with the various internal registers.  The IO routines assume that mmio is
laid out in little-endian order, but everything else in the code accesses
the register variables directly assuming (host) native endian.

This fix is fairly ugly - a nicer approach would involve converting
hcd-ehci to use the new-style memory region .read and .write functions -
but it's the most minimal fix I can see to apply for the qemu 1.2 release.

NB: It might look like this patch breaks support for unaligned accesses,
however these already didn't work as they would be rejected by the code in
memory.c before even reaching hcd-ehci.c.

Cc: Gerd Hoffman <address@hidden>

Signed-off-by: David Gibson <address@hidden>
---
 hw/usb/hcd-ehci.c |   94 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 36 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 104c21d..bf73a78 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -54,6 +54,7 @@
 #define HCSPPORTROUTE2   CAPREGBASE + 0x0010
 
 #define OPREGBASE        0x0020        // Operational Registers Base Address
+#define OPREGNUM         ((MMIO_SIZE - OPREGBASE) / sizeof(uint32_t))
 
 #define USBCMD           OPREGBASE + 0x0000
 #define USBCMD_RUNSTOP   (1 << 0)      // run / Stop
@@ -393,10 +394,10 @@ struct EHCIState {
      *  EHCI spec version 1.0 Section 2.3
      *  Host Controller Operational Registers
      */
+    uint8_t cap[OPREGBASE];
     union {
-        uint8_t mmio[MMIO_SIZE];
+        uint32_t opreg[OPREGNUM];
         struct {
-            uint8_t cap[OPREGBASE];
             uint32_t usbcmd;
             uint32_t usbsts;
             uint32_t usbintr;
@@ -962,7 +963,7 @@ static int ehci_register_companion(USBBus *bus, USBPort 
*ports[],
     }
 
     s->companion_count++;
-    s->mmio[0x05] = (s->companion_count << 4) | portcount;
+    s->cap[0x05] = (s->companion_count << 4) | portcount;
 
     return 0;
 }
@@ -1007,7 +1008,7 @@ static void ehci_reset(void *opaque)
         }
     }
 
-    memset(&s->mmio[OPREGBASE], 0x00, MMIO_SIZE - OPREGBASE);
+    memset(&s->opreg, 0, sizeof(s->opreg));
 
     s->usbcmd = NB_MAXINTRATE << USBCMD_ITC_SH;
     s->usbsts = USBSTS_HALT;
@@ -1034,22 +1035,37 @@ static void ehci_reset(void *opaque)
     qemu_bh_cancel(s->async_bh);
 }
 
+static uint32_t ehci_mem_readl(void *ptr, target_phys_addr_t addr);
+
 static uint32_t ehci_mem_readb(void *ptr, target_phys_addr_t addr)
 {
     EHCIState *s = ptr;
     uint32_t val;
 
-    val = s->mmio[addr];
+    if (addr < OPREGBASE) {
+        val = s->cap[addr];
+    } else {
+        /* Registers present as little-endian, regardless of host endianness */
+        val = ehci_mem_readl(ptr, addr & ~3);
+        val = (val >> (8*(addr & 3))) & 0xff;
+    }
 
     return val;
 }
 
 static uint32_t ehci_mem_readw(void *ptr, target_phys_addr_t addr)
 {
-    EHCIState *s = ptr;
     uint32_t val;
 
-    val = s->mmio[addr] | (s->mmio[addr+1] << 8);
+    /* Unaligned access not possible, since .valid.unaligned is not
+     * set in mem region ops */
+    if (addr < OPREGBASE) {
+        /* Registers present as little-endian, regardless of host endianness */
+        val = ehci_mem_readb(ptr, addr) | (ehci_mem_readb(ptr, addr + 1) << 8);
+    } else {
+        val = ehci_mem_readl(ptr, addr & ~3);
+        val = (val >> (8*(addr & 3))) & 0xffff;
+    }
 
     return val;
 }
@@ -1059,8 +1075,16 @@ static uint32_t ehci_mem_readl(void *ptr, 
target_phys_addr_t addr)
     EHCIState *s = ptr;
     uint32_t val;
 
-    val = s->mmio[addr] | (s->mmio[addr+1] << 8) |
-          (s->mmio[addr+2] << 16) | (s->mmio[addr+3] << 24);
+    /* Unaligned access not possible, since .valid.unaligned is not
+     * set in mem region ops */
+    if (addr < OPREGBASE) {
+        /* Registers present as little-endian, regardless of host endianness */
+        val = ehci_mem_readb(ptr, addr) | (ehci_mem_readb(ptr, addr+1) << 8)
+            | (ehci_mem_readb(ptr, addr+2) << 16)
+            | (ehci_mem_readb(ptr, addr+3) << 24);
+    } else {
+        val = s->opreg[(addr - OPREGBASE) / sizeof(uint32_t)];
+    }
 
     trace_usb_ehci_mmio_readl(addr, addr2str(addr), val);
     return val;
@@ -1147,32 +1171,30 @@ static void handle_port_status_write(EHCIState *s, int 
port, uint32_t val)
 static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
 {
     EHCIState *s = ptr;
-    uint32_t *mmio = (uint32_t *)(&s->mmio[addr]);
-    uint32_t old = *mmio;
+    uint32_t *opreg = &s->opreg[(addr - OPREGBASE) / sizeof(uint32_t)];
+    uint32_t old;
     int i;
 
     trace_usb_ehci_mmio_writel(addr, addr2str(addr), val);
 
-    /* Only aligned reads are allowed on OHCI */
-    if (addr & 3) {
-        fprintf(stderr, "usb-ehci: Mis-aligned write to addr 0x"
+    /* Unaligned access not possible, since .valid.unaligned is not
+     * set in mem region ops */
+    assert(!(addr & 3));
+
+    if (addr < OPREGBASE) {
+        fprintf(stderr, "usb-ehci: write attempt to read-only register"
                 TARGET_FMT_plx "\n", addr);
         return;
     }
 
+    old = *opreg;
+
     if (addr >= PORTSC && addr < PORTSC + 4 * NB_PORTS) {
         handle_port_status_write(s, (addr-PORTSC)/4, val);
-        trace_usb_ehci_mmio_change(addr, addr2str(addr), *mmio, old);
+        trace_usb_ehci_mmio_change(addr, addr2str(addr), *opreg, old);
         return;
     }
 
-    if (addr < OPREGBASE) {
-        fprintf(stderr, "usb-ehci: write attempt to read-only register"
-                TARGET_FMT_plx "\n", addr);
-        return;
-    }
-
-
     /* Do any register specific pre-write processing here.  */
     switch(addr) {
     case USBCMD:
@@ -1240,8 +1262,8 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t 
addr, uint32_t val)
         break;
     }
 
-    *mmio = val;
-    trace_usb_ehci_mmio_change(addr, addr2str(addr), *mmio, old);
+    *opreg = val;
+    trace_usb_ehci_mmio_change(addr, addr2str(addr), *opreg, old);
 }
 
 
@@ -2581,18 +2603,18 @@ static int usb_ehci_initfn(PCIDevice *dev)
     pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
 
     // 2.2 host controller interface version
-    s->mmio[0x00] = (uint8_t) OPREGBASE;
-    s->mmio[0x01] = 0x00;
-    s->mmio[0x02] = 0x00;
-    s->mmio[0x03] = 0x01;        // HC version
-    s->mmio[0x04] = NB_PORTS;    // Number of downstream ports
-    s->mmio[0x05] = 0x00;        // No companion ports at present
-    s->mmio[0x06] = 0x00;
-    s->mmio[0x07] = 0x00;
-    s->mmio[0x08] = 0x80;        // We can cache whole frame, not 64-bit 
capable
-    s->mmio[0x09] = 0x68;        // EECP
-    s->mmio[0x0a] = 0x00;
-    s->mmio[0x0b] = 0x00;
+    s->cap[0x00] = (uint8_t) OPREGBASE;
+    s->cap[0x01] = 0x00;
+    s->cap[0x02] = 0x00;
+    s->cap[0x03] = 0x01;        // HC version
+    s->cap[0x04] = NB_PORTS;    // Number of downstream ports
+    s->cap[0x05] = 0x00;        // No companion ports at present
+    s->cap[0x06] = 0x00;
+    s->cap[0x07] = 0x00;
+    s->cap[0x08] = 0x80;        // We can cache whole frame, not 64-bit capable
+    s->cap[0x09] = 0x68;        // EECP
+    s->cap[0x0a] = 0x00;
+    s->cap[0x0b] = 0x00;
 
     s->irq = s->dev.irq[3];
 
-- 
1.7.10.4