[Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

Originally, qemu_mod_timer() was using ticks to count time.
And i6300esb was converting internal clock ticks (33 MHz) to
QEMU timer ticks.

The timer has been changed by a script to use nanoseconds:

    7447545 change all other clock references to use
            nanosecond resolution accessors

As i6300esb takes nanoseconds, we don't need anymore to
multiply counter by get_ticks_per_sec()/33MHz, but instead
we must convert watchdog ticks into nanoseconds.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index cfa2b1b..21119ab 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -124,19 +124,24 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
     else
         timeout = d->timer2_preload;
 
-    if (d->clock_scale == CLOCK_SCALE_1KHZ)
+    /* convert timeout to 33Mhz clock ticks */
+    if (d->clock_scale == CLOCK_SCALE_1KHZ) {
+        /* The 20-bit Preload Value is loaded into bits 34:15 of the
+         * main down counter. [...] The approximate clock generated
+         * is 1 KHz, (Default)
+         */
         timeout <<= 15;
-    else
+    } else {
+        /* The 20-bit Preload Value is loaded into bits 24:5 of the
+         * main down counter. [...] The approximate clock generated
+         * is 1 MHz.
+         */
         timeout <<= 5;
-
-    /* Get the timeout in units of ticks_per_sec.
-     *
-     * ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with
-     * 20 bits of user supplied preload, and 15 bits of scale, the
-     * multiply here can exceed 64-bits, before we divide by 33MHz, so
-     * we use a higher-precision intermediate result.
+    }
+    /* A 33 Mhz clock gives a 30 ns tick,
+     * convert timeout from ticks to ns
      */
-    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
+    timeout *= 30;
 
     i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

On 03/08/2015 15:52, Laurent Vivier wrote:
> Originally, qemu_mod_timer() was using ticks to count time.
> And i6300esb was converting internal clock ticks (33 MHz) to
> QEMU timer ticks.
> 
> The timer has been changed by a script to use nanoseconds:
> 
>     7447545 change all other clock references to use
>             nanosecond resolution accessors
> 
> As i6300esb takes nanoseconds, we don't need anymore to
> multiply counter by get_ticks_per_sec()/33MHz, but instead
> we must convert watchdog ticks into nanoseconds.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index cfa2b1b..21119ab 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -124,19 +124,24 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
>      else
>          timeout = d->timer2_preload;
>  
> -    if (d->clock_scale == CLOCK_SCALE_1KHZ)
> +    /* convert timeout to 33Mhz clock ticks */
> +    if (d->clock_scale == CLOCK_SCALE_1KHZ) {
> +        /* The 20-bit Preload Value is loaded into bits 34:15 of the
> +         * main down counter. [...] The approximate clock generated
> +         * is 1 KHz, (Default)
> +         */
>          timeout <<= 15;
> -    else
> +    } else {
> +        /* The 20-bit Preload Value is loaded into bits 24:5 of the
> +         * main down counter. [...] The approximate clock generated
> +         * is 1 MHz.
> +         */
>          timeout <<= 5;
> -
> -    /* Get the timeout in units of ticks_per_sec.
> -     *
> -     * ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with
> -     * 20 bits of user supplied preload, and 15 bits of scale, the
> -     * multiply here can exceed 64-bits, before we divide by 33MHz, so
> -     * we use a higher-precision intermediate result.
> +    }
> +    /* A 33 Mhz clock gives a 30 ns tick,
> +     * convert timeout from ticks to ns
>       */
> -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> +    timeout *= 30;

I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?
if this is the former, I should use "timeout = timeout * 1000 / 33" instead.
(35 bit value * 10 bit value = 45 bit value, it fits in a 64 bit integer)

Any comment ?

>  
>      i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
>  
> 

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Richard W.M. Jones]

On Mon, Aug 03, 2015 at 04:28:14PM +0200, Laurent Vivier wrote:
> 
> 
> On 03/08/2015 15:52, Laurent Vivier wrote:
> > Originally, qemu_mod_timer() was using ticks to count time.
> > And i6300esb was converting internal clock ticks (33 MHz) to
> > QEMU timer ticks.
> > 
> > The timer has been changed by a script to use nanoseconds:
> > 
> >     7447545 change all other clock references to use
> >             nanosecond resolution accessors
> > 
> > As i6300esb takes nanoseconds, we don't need anymore to
> > multiply counter by get_ticks_per_sec()/33MHz, but instead
> > we must convert watchdog ticks into nanoseconds.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> > index cfa2b1b..21119ab 100644
> > --- a/hw/watchdog/wdt_i6300esb.c
> > +++ b/hw/watchdog/wdt_i6300esb.c
> > @@ -124,19 +124,24 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
> >      else
> >          timeout = d->timer2_preload;
> >  
> > -    if (d->clock_scale == CLOCK_SCALE_1KHZ)
> > +    /* convert timeout to 33Mhz clock ticks */
> > +    if (d->clock_scale == CLOCK_SCALE_1KHZ) {
> > +        /* The 20-bit Preload Value is loaded into bits 34:15 of the
> > +         * main down counter. [...] The approximate clock generated
> > +         * is 1 KHz, (Default)
> > +         */
> >          timeout <<= 15;
> > -    else
> > +    } else {
> > +        /* The 20-bit Preload Value is loaded into bits 24:5 of the
> > +         * main down counter. [...] The approximate clock generated
> > +         * is 1 MHz.
> > +         */
> >          timeout <<= 5;
> > -
> > -    /* Get the timeout in units of ticks_per_sec.
> > -     *
> > -     * ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with
> > -     * 20 bits of user supplied preload, and 15 bits of scale, the
> > -     * multiply here can exceed 64-bits, before we divide by 33MHz, so
> > -     * we use a higher-precision intermediate result.
> > +    }
> > +    /* A 33 Mhz clock gives a 30 ns tick,
> > +     * convert timeout from ticks to ns
> >       */
> > -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> > +    timeout *= 30;
> 
> I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?

>From the datasheet (chapter 16):

https://www-ssl.intel.com/content/www/us/en/chipsets/6300esb-io-controller-hub-datasheet.html

it says "33 MHz clock (30 ns clock ticks)" which is contradictory.

I suspect it does really mean 33,333,333 Mz (== 30 ns ticks) since
that is the base clock speed discussed elsewhere in that document.

> if this is the former, I should use "timeout = timeout * 1000 / 33" instead.
> (35 bit value * 10 bit value = 45 bit value, it fits in a 64 bit integer)

See also commit 4bc7b4d56657ebf75b986ad46e959cf7232ff26a and the
discussion here:

https://lists.gnu.org/archive/html/qemu-ppc/2015-03/threads.html#00448

The original code did `get_ticks_per_sec() * timeout / 33000000' which
definitely overflowed.

Rich.

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

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Paolo Bonzini]

On 03/08/2015 16:46, Richard W.M. Jones wrote:
>> > I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?
> From the datasheet (chapter 16):
> 
> https://www-ssl.intel.com/content/www/us/en/chipsets/6300esb-io-controller-hub-datasheet.html
> 
> it says "33 MHz clock (30 ns clock ticks)" which is contradictory.

I found that the spec allows for any speed up to 33333333 Hz (30 ns
cycle), so both are okay.  However, at least hw/net/rtl8139.c assumes
it's 33000000 Hz, so it's nice to be consistent.

Paolo

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

On 03/08/2015 16:46, Richard W.M. Jones wrote:
> On Mon, Aug 03, 2015 at 04:28:14PM +0200, Laurent Vivier wrote:
>>
>>
>> On 03/08/2015 15:52, Laurent Vivier wrote:
>>> Originally, qemu_mod_timer() was using ticks to count time.
>>> And i6300esb was converting internal clock ticks (33 MHz) to
>>> QEMU timer ticks.
>>>
>>> The timer has been changed by a script to use nanoseconds:
>>>
>>>     7447545 change all other clock references to use
>>>             nanosecond resolution accessors
>>>
>>> As i6300esb takes nanoseconds, we don't need anymore to
>>> multiply counter by get_ticks_per_sec()/33MHz, but instead
>>> we must convert watchdog ticks into nanoseconds.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>  hw/watchdog/wdt_i6300esb.c | 25 +++++++++++++++----------
>>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
>>> index cfa2b1b..21119ab 100644
>>> --- a/hw/watchdog/wdt_i6300esb.c
>>> +++ b/hw/watchdog/wdt_i6300esb.c
>>> @@ -124,19 +124,24 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
>>>      else
>>>          timeout = d->timer2_preload;
>>>  
>>> -    if (d->clock_scale == CLOCK_SCALE_1KHZ)
>>> +    /* convert timeout to 33Mhz clock ticks */
>>> +    if (d->clock_scale == CLOCK_SCALE_1KHZ) {
>>> +        /* The 20-bit Preload Value is loaded into bits 34:15 of the
>>> +         * main down counter. [...] The approximate clock generated
>>> +         * is 1 KHz, (Default)
>>> +         */
>>>          timeout <<= 15;
>>> -    else
>>> +    } else {
>>> +        /* The 20-bit Preload Value is loaded into bits 24:5 of the
>>> +         * main down counter. [...] The approximate clock generated
>>> +         * is 1 MHz.
>>> +         */
>>>          timeout <<= 5;
>>> -
>>> -    /* Get the timeout in units of ticks_per_sec.
>>> -     *
>>> -     * ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with
>>> -     * 20 bits of user supplied preload, and 15 bits of scale, the
>>> -     * multiply here can exceed 64-bits, before we divide by 33MHz, so
>>> -     * we use a higher-precision intermediate result.
>>> +    }
>>> +    /* A 33 Mhz clock gives a 30 ns tick,
>>> +     * convert timeout from ticks to ns
>>>       */
>>> -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
>>> +    timeout *= 30;
>>
>> I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?
> 
> From the datasheet (chapter 16):
> 
> https://www-ssl.intel.com/content/www/us/en/chipsets/6300esb-io-controller-hub-datasheet.html
> 
> it says "33 MHz clock (30 ns clock ticks)" which is contradictory.
> 
> I suspect it does really mean 33,333,333 Mz (== 30 ns ticks) since
> that is the base clock speed discussed elsewhere in that document.

So we can multiply by 30 :)

> 
>> if this is the former, I should use "timeout = timeout * 1000 / 33" instead.
>> (35 bit value * 10 bit value = 45 bit value, it fits in a 64 bit integer)
> 
> See also commit 4bc7b4d56657ebf75b986ad46e959cf7232ff26a and the
> discussion here:
> 
> https://lists.gnu.org/archive/html/qemu-ppc/2015-03/threads.html#00448
> 
> The original code did `get_ticks_per_sec() * timeout / 33000000' which
> definitely overflowed.

I've seen that, but it seems it always overflows and the maximum value
seems to be 256 seconds.

The second parameters of muldiv64() must be uint32 whereas "timeout" is
int64_t (real size is 35 bit long):

static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)

Laurent

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

On 03/08/2015 17:02, Paolo Bonzini wrote:
> 
> 
> On 03/08/2015 16:46, Richard W.M. Jones wrote:
>>>> I'm wondering if a 33 Mhz clock is 33000000 Hz or 33333333 Hz ?
>> From the datasheet (chapter 16):
>>
>> https://www-ssl.intel.com/content/www/us/en/chipsets/6300esb-io-controller-hub-datasheet.html
>>
>> it says "33 MHz clock (30 ns clock ticks)" which is contradictory.
> 
> I found that the spec allows for any speed up to 33333333 Hz (30 ns
> cycle), so both are okay.  However, at least hw/net/rtl8139.c assumes
> it's 33000000 Hz, so it's nice to be consistent.

So, do you want I resend a patch with "1000 / 33" instead ?

We can also update hw/net/rtl8139.c to replace "muldiv64(X,
get_ticks_per_sec(), 33000000)" by "X * 30" ?

Laurent

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Paolo Bonzini]

On 03/08/2015 17:13, Laurent Vivier wrote:
>>> >> it says "33 MHz clock (30 ns clock ticks)" which is contradictory.
>> > 
>> > I found that the spec allows for any speed up to 33333333 Hz (30 ns
>> > cycle), so both are okay.  However, at least hw/net/rtl8139.c assumes
>> > it's 33000000 Hz, so it's nice to be consistent.
> So, do you want I resend a patch with "1000 / 33" instead ?
> 
> We can also update hw/net/rtl8139.c to replace "muldiv64(X,
> get_ticks_per_sec(), 33000000)" by "X * 30" ?

Either would do.  If you modify hw/net/rtl8139.c you have to modify CLK
in tests/test-rtl8139.c as well.

Paolo

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

On 03/08/2015 17:18, Paolo Bonzini wrote:
> 
> 
> On 03/08/2015 17:13, Laurent Vivier wrote:
>>>>>> it says "33 MHz clock (30 ns clock ticks)" which is contradictory.
>>>>
>>>> I found that the spec allows for any speed up to 33333333 Hz (30 ns
>>>> cycle), so both are okay.  However, at least hw/net/rtl8139.c assumes
>>>> it's 33000000 Hz, so it's nice to be consistent.
>> So, do you want I resend a patch with "1000 / 33" instead ?
>>
>> We can also update hw/net/rtl8139.c to replace "muldiv64(X,
>> get_ticks_per_sec(), 33000000)" by "X * 30" ?
> 
> Either would do.  If you modify hw/net/rtl8139.c you have to modify CLK
> in tests/test-rtl8139.c as well.

I like the idea. I will.

I guess you mean tests/rtl8139-test.c

What is the "in_Timer()" function ?

Laurent

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Paolo Bonzini]

On 03/08/2015 17:35, Laurent Vivier wrote:
> 
> I guess you mean tests/rtl8139-test.c
> 
> What is the "in_Timer()" function ?

There's some awful macro magic:

#define PORT(name, len, val) \
static unsigned __attribute__((unused)) in_##name(void) \
{ \
    unsigned res = qpci_io_read##len(dev, dev_base+(val)); \
    g_test_message("*%s -> %x\n", #name, res); \
    return res; \
} \
static void out_##name(unsigned v) \
{ \
    g_test_message("%x -> *%s\n", v, #name); \
    qpci_io_write##len(dev, dev_base+(val), v); \
}

PORT(Timer, l, 0x48)
PORT(IntrMask, w, 0x3c)
PORT(IntrStatus, w, 0x3E)
PORT(TimerInt, l, 0x54)

Pa##olo

[Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[Laurent Vivier]

We use muldiv64() to compute the time to wait:

    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);

but get_ticks_per_sec() is 10^9 (30 bit value) and timeout
is a 35 bit value.

Whereas muldiv64 is:

    uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)

So we loose 3 bits of timeout.

Swapping get_ticks_per_sec() and timeout fixes it.

We can also replace it by a multiplication by 30 ns,
but this changes PCI clock frequency from 33MHz to 33.333333MHz
and we need to do this on all the QEMU PCI devices (later...)

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/watchdog/wdt_i6300esb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index cfa2b1b..3e07d44 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -136,7 +136,7 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
      * multiply here can exceed 64-bits, before we divide by 33MHz, so
      * we use a higher-precision intermediate result.
      */
-    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
+    timeout = muldiv64(timeout, get_ticks_per_sec(), 33000000);
 
     i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Richard W.M. Jones]

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

On Mon, Aug 03, 2015 at 03:52:28PM +0200, Laurent Vivier wrote:
> +    /* A 33 Mhz clock gives a 30 ns tick,
> +     * convert timeout from ticks to ns
>       */
> -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> +    timeout *= 30;

I see that you've just posted a v2 of this patch.  However here are
the results of testing the above version.  I used the attached test
script which automates things, mostly.

All times are in seconds.

  Requested timeout       Observed timeout
         60                     58
        120                    120
        250                    249
        270                    271
        500                    501  [note 1]
        520                    522
       1010                   1016
       1030                   1035
       2046                   2058
       2500  ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument
                              [note 2]

[note 1] I'm not worried about the timeout being off by a few seconds,
as that could easily be caused by inaccuracies in the test framework.

[note 2] Maximum setting for i6300esb Linux driver is 2046, see
linux.git/drivers/watchdog/i6300esb.c but note that the printed
messages in the driver relating to the range of the timeout are not
accurate.

This patch looks good to me.  I will test the v2 patch shortly.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

[-- Attachment #2: i6300esb-test.sh --]
[-- Type: application/x-sh, Size: 1085 bytes --]

Re: [Qemu-devel] [PATCH] i6300esb: correctly convert watchdog clock ticks into nanoseconds

[Laurent Vivier]

On 04/08/2015 12:13, Richard W.M. Jones wrote:
> On Mon, Aug 03, 2015 at 03:52:28PM +0200, Laurent Vivier wrote:
>> +    /* A 33 Mhz clock gives a 30 ns tick,
>> +     * convert timeout from ticks to ns
>>       */
>> -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
>> +    timeout *= 30;
> 
> I see that you've just posted a v2 of this patch.  However here are
> the results of testing the above version.  I used the attached test
> script which automates things, mostly.
> 
> All times are in seconds.
> 
>   Requested timeout       Observed timeout
>          60                     58
>         120                    120
>         250                    249
>         270                    271
>         500                    501  [note 1]
>         520                    522
>        1010                   1016
>        1030                   1035
>        2046                   2058
>        2500  ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument
>                               [note 2]

Thank you Rich.

> [note 1] I'm not worried about the timeout being off by a few seconds,
> as that could easily be caused by inaccuracies in the test framework.

The driver put 1024 in the counter for 1 second.

So for 520 seconds, we have ((520 * 1024) << 15) * 30 ns = 523 seconds
      2036 seconds          ((2046 * 1024) << 15) * 30 ns = 2059 seconds

So the emulation seems correct.

> [note 2] Maximum setting for i6300esb Linux driver is 2046, see
> linux.git/drivers/watchdog/i6300esb.c but note that the printed
> messages in the driver relating to the range of the timeout are not
> accurate.
> This patch looks good to me.  I will test the v2 patch shortly.

I have no preference of which patch to include in QEMU.

This one is nicer but request the same kind of modification in other
devices, the V2 is a trivial bug fix (currently timer overflow at 256
seconds). I just want to have the bug fixed...

Laurent

Re: [Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[Richard W.M. Jones]

On Tue, Aug 04, 2015 at 10:27:31AM +0200, Laurent Vivier wrote:
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -136,7 +136,7 @@ static void i6300esb_restart_timer(I6300State *d, int stage)
>       * multiply here can exceed 64-bits, before we divide by 33MHz, so
>       * we use a higher-precision intermediate result.
>       */
> -    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> +    timeout = muldiv64(timeout, get_ticks_per_sec(), 33000000);
>  
>      i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);

Here are the test results for this (v2) patch:

  Requested timeout     Observed timeout
       60                    58
      120                   120
      250                   253
      270                   274
      500                   507
      520                   529
     1010                  1026
     1030                  1045
     2046                  2078
     2500    ioctl: WDIOC_SETTIMEOUT: error setting timeout: Invalid argument

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[David Gibson]

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

On Tue, Aug 04, 2015 at 10:27:31AM +0200, Laurent Vivier wrote:
> We use muldiv64() to compute the time to wait:
> 
>     timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> 
> but get_ticks_per_sec() is 10^9 (30 bit value) and timeout
> is a 35 bit value.
> 
> Whereas muldiv64 is:
> 
>     uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> 
> So we loose 3 bits of timeout.
> 
> Swapping get_ticks_per_sec() and timeout fixes it.
> 
> We can also replace it by a multiplication by 30 ns,
> but this changes PCI clock frequency from 33MHz to 33.333333MHz
> and we need to do this on all the QEMU PCI devices (later...)
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Hah.  32-bit second argument.  Totally missed that when I put the
muldiv64() in there.  So I didn't eliminate the overflow, just pushed
it out some bits.

Thanks for finding this.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[Michael Tokarev]

04.08.2015 11:27, Laurent Vivier wrote:
> We use muldiv64() to compute the time to wait:
> 
>     timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
...

Applied to -trivial, thank you!

/mjt

Re: [Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[Paolo Bonzini]

On 06/09/2015 12:29, Michael Tokarev wrote:
> 04.08.2015 11:27, Laurent Vivier wrote:
>> We use muldiv64() to compute the time to wait:
>>
>>     timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
> ...
> 
> Applied to -trivial, thank you!

I think this was superseded by a later patch.

Paolo

Re: [Qemu-devel] [PATCH][TRIVIAL] i6300esb: fix timer overflow

[Laurent Vivier]

Le 06/09/2015 16:35, Paolo Bonzini a écrit :
> 
> 
> On 06/09/2015 12:29, Michael Tokarev wrote:
>> 04.08.2015 11:27, Laurent Vivier wrote:
>>> We use muldiv64() to compute the time to wait:
>>>
>>>     timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
>> ...
>>
>> Applied to -trivial, thank you!
> 
> I think this was superseded by a later patch.

Yes, but as the later has not already be taken, I prefer this one taken
and I'll update the other.

Laurent