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

[Adam Hoka]

Hi,

On 11/30/2014 11:31 PM, Richard W.M. Jones wrote:
> 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.

Thanks for the feedback. I guess I could test it on Linux. Should I just 
run this utility?
What's wrong with the Intel/Win driver? BTW, could you ever find some 
documentation on
where the hell is the IRQ line is connected on real HW?

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

Regards,
Adam

smime.p7s
Description: S/MIME Cryptographic Signature