[Qemu-devel] [PATCH] i6300esb: fix reading config registers and accept writes of all length

[Adam Hoka]

Don't require configuration register write to be off a certain length,
as some PCI implementations always access them in 32bit only. This is
because it's in fact the only kind of access supported by the standard,
anything else is implementation dependent.

Add support for reading back the configuration register values.

Unify the MMIO register implementation into a common read and write
function. This makes driver testing in QEMU less surprising.

Missing: interrupt register is still not implemented as interrupting
itself is absent. It's unclear from the 6300ESB ICH specs where
the IRQ line is connected in real hardware.

Signed-off-by: Adam Hoka <address@hidden>
---
 hw/watchdog/wdt_i6300esb.c | 134 ++++++++++++++++++---------------------------
 1 file changed, 53 insertions(+), 81 deletions(-)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 687c8b1..8512a91 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -212,12 +212,12 @@ static void i6300esb_config_write(PCIDevice *dev, 
uint32_t addr,
 
     i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len);
 
-    if (addr == ESB_CONFIG_REG && len == 2) {
+    if (addr == ESB_CONFIG_REG) {
         d->reboot_enabled = (data & ESB_WDT_REBOOT) == 0;
         d->clock_scale =
             (data & ESB_WDT_FREQ) != 0 ? CLOCK_SCALE_1MHZ : CLOCK_SCALE_1KHZ;
         d->int_type = (data & ESB_WDT_INTTYPE);
-    } else if (addr == ESB_LOCK_REG && len == 1) {
+    } else if (addr == ESB_LOCK_REG) {
         if (!d->locked) {
             d->locked = (data & ESB_WDT_LOCK) != 0;
             d->free_run = (data & ESB_WDT_FUNC) != 0;
@@ -240,13 +240,13 @@ static uint32_t i6300esb_config_read(PCIDevice *dev, 
uint32_t addr, int len)
 
     i6300esb_debug ("addr = %x, len = %d\n", addr, len);
 
-    if (addr == ESB_CONFIG_REG && len == 2) {
+    if (addr == ESB_CONFIG_REG) {
         data =
             (d->reboot_enabled ? 0 : ESB_WDT_REBOOT) |
             (d->clock_scale == CLOCK_SCALE_1MHZ ? ESB_WDT_FREQ : 0) |
             d->int_type;
         return data;
-    } else if (addr == ESB_LOCK_REG && len == 1) {
+    } else if (addr == ESB_LOCK_REG) {
         data =
             (d->free_run ? ESB_WDT_FUNC : 0) |
             (d->locked ? ESB_WDT_LOCK : 0) |
@@ -257,116 +257,88 @@ static uint32_t i6300esb_config_read(PCIDevice *dev, 
uint32_t addr, int len)
     }
 }
 
-static uint32_t i6300esb_mem_readb(void *vp, hwaddr addr)
+static uint32_t i6300esb_mem_read(void *vp, hwaddr addr)
 {
-    i6300esb_debug ("addr = %x\n", (int) addr);
-
-    return 0;
-}
-
-static uint32_t i6300esb_mem_readw(void *vp, hwaddr addr)
-{
-    uint32_t data = 0;
     I6300State *d = vp;
 
-    i6300esb_debug("addr = %x\n", (int) addr);
+    i6300esb_debug("addr = %p\n", (void *)addr);
 
-    if (addr == 0xc) {
+    switch (addr) {
+    case 0x00:
+        return d->timer1_preload;
+    case 0x04:
+        return d->timer2_preload;
+    case 0x0c:
         /* The previous reboot flag is really bit 9, but there is
          * a bug in the Linux driver where it thinks it's bit 12.
          * Set both.
          */
-        data = d->previous_reboot_flag ? 0x1200 : 0;
+        return d->previous_reboot_flag ? 0x1200 : 0;
     }
 
-    return data;
-}
-
-static uint32_t i6300esb_mem_readl(void *vp, hwaddr addr)
-{
-    i6300esb_debug("addr = %x\n", (int) addr);
-
     return 0;
 }
 
-static void i6300esb_mem_writeb(void *vp, hwaddr addr, uint32_t val)
+static void i6300esb_mem_write(void *vp, hwaddr addr, uint32_t val)
 {
     I6300State *d = vp;
 
-    i6300esb_debug("addr = %x, val = %x\n", (int) addr, val);
+    i6300esb_debug("addr = %p, val = 0x%x\n", (void *)addr, val);
 
-    if (addr == 0xc && val == 0x80)
+    /* register lock */
+    if (addr == 0xc && val == 0x80) {
         d->unlock_state = 1;
-    else if (addr == 0xc && val == 0x86 && d->unlock_state == 1)
+        return;
+    } else if (addr == 0xc && val == 0x86 && d->unlock_state == 1) {
         d->unlock_state = 2;
-}
+        return;
+    } else if (d->unlock_state == 0) {
+        return;
+    }
 
-static void i6300esb_mem_writew(void *vp, hwaddr addr, uint32_t val)
-{
-    I6300State *d = vp;
+    switch (addr) {
+    case 0x00:
+        d->timer1_preload = val & 0xfffff;
+        break;
 
-    i6300esb_debug("addr = %x, val = %x\n", (int) addr, val);
+    case 0x04:
+        d->timer2_preload = val & 0xfffff;
+        break;
 
-    if (addr == 0xc && val == 0x80)
-        d->unlock_state = 1;
-    else if (addr == 0xc && val == 0x86 && d->unlock_state == 1)
-        d->unlock_state = 2;
-    else {
-        if (d->unlock_state == 2) {
-            if (addr == 0xc) {
-                if ((val & 0x100) != 0)
-                    /* This is the "ping" from the userspace watchdog in
-                     * the guest ...
-                     */
-                    i6300esb_restart_timer(d, 1);
-
-                /* Setting bit 9 resets the previous reboot flag.
-                 * There's a bug in the Linux driver where it sets
-                 * bit 12 instead.
-                 */
-                if ((val & 0x200) != 0 || (val & 0x1000) != 0) {
-                    d->previous_reboot_flag = 0;
-                }
-            }
-
-            d->unlock_state = 0;
+    case 0x0c:
+        if ((val & 0x100) != 0) {
+            /* This is the "ping" from the userspace watchdog in
+             * the guest ...
+             */
+            i6300esb_restart_timer(d, 1);
         }
-    }
-}
-
-static void i6300esb_mem_writel(void *vp, hwaddr addr, uint32_t val)
-{
-    I6300State *d = vp;
 
-    i6300esb_debug ("addr = %x, val = %x\n", (int) addr, val);
-
-    if (addr == 0xc && val == 0x80)
-        d->unlock_state = 1;
-    else if (addr == 0xc && val == 0x86 && d->unlock_state == 1)
-        d->unlock_state = 2;
-    else {
-        if (d->unlock_state == 2) {
-            if (addr == 0)
-                d->timer1_preload = val & 0xfffff;
-            else if (addr == 4)
-                d->timer2_preload = val & 0xfffff;
-
-            d->unlock_state = 0;
+        /* Setting bit 9 resets the previous reboot flag.
+         * There's a bug in the Linux driver where it sets
+         * bit 12 instead.
+         */
+        if ((val & 0x200) != 0 || (val & 0x1000) != 0) {
+            d->previous_reboot_flag = 0;
         }
+
+        break;
     }
+
+    /* re-lock registers */
+    d->unlock_state = 0;
 }
 
 static const MemoryRegionOps i6300esb_ops = {
     .old_mmio = {
         .read = {
-            i6300esb_mem_readb,
-            i6300esb_mem_readw,
-            i6300esb_mem_readl,
+            i6300esb_mem_read,
+            i6300esb_mem_read,
+            i6300esb_mem_read,
         },
         .write = {
-            i6300esb_mem_writeb,
-            i6300esb_mem_writew,
-            i6300esb_mem_writel,
+            i6300esb_mem_write,
+            i6300esb_mem_write,
+            i6300esb_mem_write,
         },
     },
     .endianness = DEVICE_NATIVE_ENDIAN,
-- 
2.1.1