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

[Richard W.M. Jones]

On Wed, Oct 29, 2014 at 02:42:51PM +0100, Adam Hoka wrote:
> 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>

I don't really have any opinion on this patch.  All I care is that it
doesn't break the Linux device driver (the Intel-supplied 32 bit
Windows device driver is unfortunately a lost cause).  Did you test it
against Linux?  I wrote a small test harness that makes testing the
qemu watchdog simple:

http://git.annexia.org/?p=watchdog-test-framework.git;a=summary

Rich.

>  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
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW