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

[Adam Hoka <adam.hoka@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8785 bytes --]

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 <adam.hoka@gmail.com>
> 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



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4249 bytes --]