[PATCH 0/4] clock: Get rid of clock_get_ns()

[PATCH 0/4] clock: Get rid of clock_get_ns()

[Peter Maydell]

This patchseries makes some changes to the clock API:
 * Remove clock_get_ns()
 * Add clock_ticks_to_ns() to return number of nanoseconds
   it will take the clock to tick N times
 * clock_display_freq() to return prettily-formatted string
   for showing humans the approximate clock frequency

This is based on discussions we had about these APIs a little while
back.  The core driver here is that the clock objects internally
store the period in units of 2^-32 ns, so both clock_get_ns() and
clock_get_hz() are inherently returning a rounded-off result, which
can be badly inaccurate for fast clocks or if you want to multiply it
by a large tick count.

Ideally I'd like to get rid of clock_get_hz() as well, but
that looks trickier than handling clock_get_ns().

Patch 4 borrows a lot of the concept from one of Philippe's that he
sent out previously.

NB: tested with 'make check' and 'make check-acceptance' only.

thanks
-- PMM

Peter Maydell (4):
  clock: Introduce clock_ticks_to_ns()
  target/mips: Don't use clock_get_ns() in clock period calculation
  clock: Remove clock_get_ns()
  clock: Define and use new clock_display_freq()

 docs/devel/clocks.rst  | 37 +++++++++++++++++++++++++++++++++----
 include/hw/clock.h     | 41 ++++++++++++++++++++++++++++++++++++++---
 hw/core/clock.c        |  6 ++++++
 softmmu/qdev-monitor.c |  6 +++---
 target/mips/cpu.c      |  4 ++--
 5 files changed, 82 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Peter Maydell]

The clock_get_ns() API claims to return the period of a clock in
nanoseconds. Unfortunately since it returns an integer and a
clock's period is represented in units of 2^-32 nanoseconds,
the result is often an approximation, and calculating a clock
expiry deadline by multiplying clock_get_ns() by a number-of-ticks
is unacceptably inaccurate.

Introduce a new API clock_ticks_to_ns() which returns the number
of nanoseconds it takes the clock to make a given number of ticks.
This function can do the complete calculation internally and
will thus give a more accurate result.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The 64x64->128 multiply is a bit painful for 32-bit and I
guess in theory since we know we only want bits [95:32]
of the result we could special-case it, but TBH I don't
think 32-bit hosts merit much optimization effort these days.
---
 docs/devel/clocks.rst | 15 +++++++++++++++
 include/hw/clock.h    | 29 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index e5da28e2111..aebeedbb95e 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -258,6 +258,21 @@ Here is an example:
                         clock_get_ns(dev->my_clk_input));
     }
 
+Calculating expiry deadlines
+----------------------------
+
+A commonly required operation for a clock is to calculate how long
+it will take for the clock to tick N times; this can then be used
+to set a timer expiry deadline. Use the function ``clock_ticks_to_ns()``,
+which takes an unsigned 64-bit count of ticks and returns the length
+of time in nanoseconds required for the clock to tick that many times.
+
+It is important not to try to calculate expiry deadlines using a
+shortcut like multiplying a "period of clock in nanoseconds" value
+by the tick count, because clocks can have periods which are not a
+whole number of nanoseconds, and the accumulated error in the
+multiplication can be significant.
+
 Changing a clock period
 -----------------------
 
diff --git a/include/hw/clock.h b/include/hw/clock.h
index 81bcf3e505a..a9425d9fb14 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -16,6 +16,7 @@
 
 #include "qom/object.h"
 #include "qemu/queue.h"
+#include "qemu/host-utils.h"
 
 #define TYPE_CLOCK "clock"
 OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
@@ -218,6 +219,34 @@ static inline unsigned clock_get_ns(Clock *clk)
     return CLOCK_PERIOD_TO_NS(clock_get(clk));
 }
 
+/**
+ * clock_ticks_to_ns:
+ * @clk: the clock to query
+ * @ticks: number of ticks
+ *
+ * Returns the length of time in nanoseconds for this clock
+ * to tick @ticks times. Because a clock can have a period
+ * which is not a whole number of nanoseconds, it is important
+ * to use this function when calculating things like timer
+ * expiry deadlines, rather than attempting to obtain a "period
+ * in nanoseconds" value and then multiplying that by a number
+ * of ticks.
+ */
+static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
+{
+    uint64_t ns_low, ns_high;
+
+    /*
+     * clk->period is the period in units of 2^-32 ns, so
+     * (clk->period * ticks) is the required length of time in those
+     * units, and we can convert to nanoseconds by multiplying by
+     * 2^32, which is the same as shifting the 128-bit multiplication
+     * result right by 32.
+     */
+    mulu64(&ns_low, &ns_high, clk->period, ticks);
+    return ns_low >> 32 | ns_high << 32;
+}
+
 /**
  * clock_is_enabled:
  * @clk: a clock
-- 
2.20.1

[PATCH 2/4] target/mips: Don't use clock_get_ns() in clock period calculation

[Peter Maydell]

Currently the MIPS code uses the old clock_get_ns() API to
calculate a time length in nanoseconds:
 cpu->cp0_count_rate * clock_get_ns(MIPS_CPU(cpu)->clock)

This relies on the clock having a period which is an exact number
of nanoseconds.

Switch to the new clock_ticks_to_ns() function, which does the
multiplication internally at a higher precision.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/mips/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 76d50b00b42..de15ec6068a 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -147,8 +147,8 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
 
-    env->cp0_count_ns = cpu->cp0_count_rate
-                        * clock_get_ns(MIPS_CPU(cpu)->clock);
+    env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
+                                          cpu->cp0_count_rate);
     assert(env->cp0_count_ns);
 }
 
-- 
2.20.1

[PATCH 3/4] clock: Remove clock_get_ns()

[Peter Maydell]

Remove the now-unused clock_get_ns() API and the CLOCK_PERIOD_TO_NS()
macro that only it was using.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/clocks.rst | 17 +++++++++++++----
 include/hw/clock.h    |  6 ------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index aebeedbb95e..9a93d1361b4 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -238,8 +238,17 @@ object during device instance init. For example:
 Fetching clock frequency/period
 -------------------------------
 
-To get the current state of a clock, use the functions ``clock_get()``,
-``clock_get_ns()`` or ``clock_get_hz()``.
+To get the current state of a clock, use the functions ``clock_get()``
+or ``clock_get_hz()``.
+
+``clock_get()`` returns the period of the clock in its fully precise
+internal representation, as an unsigned 64-bit integer in units of
+2^-32 nanoseconds. (For many purposes ``clock_ticks_to_ns()`` will
+be more convenient; see the section below on expiry deadlines.)
+
+``clock_get_hz()`` returns the frequency of the clock, rounded to the
+next lowest integer. This implies some inaccuracy due to the rounding,
+so be cautious about using it in calculations.
 
 It is also possible to register a callback on clock frequency changes.
 Here is an example:
@@ -254,8 +263,8 @@ Here is an example:
          */
 
         /* do something with the new period */
-        fprintf(stdout, "device new period is %" PRIu64 "ns\n",
-                        clock_get_ns(dev->my_clk_input));
+        fprintf(stdout, "device new period is %" PRIu64 "* 2^-32 ns\n",
+                        clock_get(dev->my_clk_input));
     }
 
 Calculating expiry deadlines
diff --git a/include/hw/clock.h b/include/hw/clock.h
index a9425d9fb14..9c0b1eb4c3f 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -39,7 +39,6 @@ typedef void ClockCallback(void *opaque);
  * macro helpers to convert to hertz / nanosecond
  */
 #define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
-#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
 #define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
 #define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_PERIOD_1SEC / (per) : 0u)
 
@@ -214,11 +213,6 @@ static inline unsigned clock_get_hz(Clock *clk)
     return CLOCK_PERIOD_TO_HZ(clock_get(clk));
 }
 
-static inline unsigned clock_get_ns(Clock *clk)
-{
-    return CLOCK_PERIOD_TO_NS(clock_get(clk));
-}
-
 /**
  * clock_ticks_to_ns:
  * @clk: the clock to query
-- 
2.20.1

[PATCH 4/4] clock: Define and use new clock_display_freq()

[Peter Maydell]

It's common to want to print a human-readable indication of a clock's
frequency. Provide a utility function in the clock API to return a
string which is a displayable representation of the frequency,
and use it in qdev-monitor.c.

Before:

  (qemu) info qtree
  [...]
  dev: xilinx,zynq_slcr, id ""
    clock-in "ps_clk" freq_hz=3.333333e+07
    mmio 00000000f8000000/0000000000001000

After:

  dev: xilinx,zynq_slcr, id ""
    clock-in "ps_clk" freq_hz=33.3 MHz
    mmio 00000000f8000000/0000000000001000


Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is based on Philippe's patch
"qdev-monitor: Display frequencies scaled to SI unit"
but I have abstracted out the "prettified string" into the clock API.
---
 docs/devel/clocks.rst  |  5 +++++
 include/hw/clock.h     | 12 ++++++++++++
 hw/core/clock.c        |  6 ++++++
 softmmu/qdev-monitor.c |  6 +++---
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index 9a93d1361b4..cf8067542a1 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -267,6 +267,11 @@ Here is an example:
                         clock_get(dev->my_clk_input));
     }
 
+If you are only interested in the frequency for displaying it to
+humans (for instance in debugging), use ``clock_display_freq()``,
+which returns a prettified string-representation, e.g. "33.3 MHz".
+The caller must free the string with g_free() after use.
+
 Calculating expiry deadlines
 ----------------------------
 
diff --git a/include/hw/clock.h b/include/hw/clock.h
index 9c0b1eb4c3f..7bc9afb0800 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -252,4 +252,16 @@ static inline bool clock_is_enabled(const Clock *clk)
     return clock_get(clk) != 0;
 }
 
+/**
+ * clock_display_freq: return human-readable representation of clock frequency
+ * @clk: clock
+ *
+ * Return a string which has a human-readable representation of the
+ * clock's frequency, e.g. "33.3 MHz". This is intended for debug
+ * and display purposes.
+ *
+ * The caller is responsible for freeing the string with g_free().
+ */
+char *clock_display_freq(Clock *clk);
+
 #endif /* QEMU_HW_CLOCK_H */
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 8c6af223e7c..76b5f468b6e 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/clock.h"
 #include "trace.h"
 
@@ -111,6 +112,11 @@ static void clock_disconnect(Clock *clk)
     QLIST_REMOVE(clk, sibling);
 }
 
+char *clock_display_freq(Clock *clk)
+{
+    return freq_to_str(clock_get_hz(clk));
+}
+
 static void clock_initfn(Object *obj)
 {
     Clock *clk = CLOCK(obj);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index bf79d0bbcd9..6263d600026 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -747,11 +747,11 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
         }
     }
     QLIST_FOREACH(ncl, &dev->clocks, node) {
-        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
+        g_autofree char *freq_str = clock_display_freq(ncl->clock);
+        qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
                     ncl->output ? "out" : "in",
                     ncl->alias ? " (alias)" : "",
-                    ncl->name,
-                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
+                    ncl->name, freq_str);
     }
     class = object_get_class(OBJECT(dev));
     do {
-- 
2.20.1

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Richard Henderson]

On 12/8/20 12:15 PM, Peter Maydell wrote:
> The clock_get_ns() API claims to return the period of a clock in
> nanoseconds. Unfortunately since it returns an integer and a
> clock's period is represented in units of 2^-32 nanoseconds,
> the result is often an approximation, and calculating a clock
> expiry deadline by multiplying clock_get_ns() by a number-of-ticks
> is unacceptably inaccurate.
> 
> Introduce a new API clock_ticks_to_ns() which returns the number
> of nanoseconds it takes the clock to make a given number of ticks.
> This function can do the complete calculation internally and
> will thus give a more accurate result.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The 64x64->128 multiply is a bit painful for 32-bit and I
> guess in theory since we know we only want bits [95:32]
> of the result we could special-case it, but TBH I don't
> think 32-bit hosts merit much optimization effort these days.
> ---
>  docs/devel/clocks.rst | 15 +++++++++++++++
>  include/hw/clock.h    | 29 +++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index e5da28e2111..aebeedbb95e 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -258,6 +258,21 @@ Here is an example:
>                          clock_get_ns(dev->my_clk_input));
>      }
>  
> +Calculating expiry deadlines
> +----------------------------
> +
> +A commonly required operation for a clock is to calculate how long
> +it will take for the clock to tick N times; this can then be used
> +to set a timer expiry deadline. Use the function ``clock_ticks_to_ns()``,
> +which takes an unsigned 64-bit count of ticks and returns the length
> +of time in nanoseconds required for the clock to tick that many times.
> +
> +It is important not to try to calculate expiry deadlines using a
> +shortcut like multiplying a "period of clock in nanoseconds" value
> +by the tick count, because clocks can have periods which are not a
> +whole number of nanoseconds, and the accumulated error in the
> +multiplication can be significant.
> +
>  Changing a clock period
>  -----------------------
>  
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index 81bcf3e505a..a9425d9fb14 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -16,6 +16,7 @@
>  
>  #include "qom/object.h"
>  #include "qemu/queue.h"
> +#include "qemu/host-utils.h"
>  
>  #define TYPE_CLOCK "clock"
>  OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
> @@ -218,6 +219,34 @@ static inline unsigned clock_get_ns(Clock *clk)
>      return CLOCK_PERIOD_TO_NS(clock_get(clk));
>  }
>  
> +/**
> + * clock_ticks_to_ns:
> + * @clk: the clock to query
> + * @ticks: number of ticks
> + *
> + * Returns the length of time in nanoseconds for this clock
> + * to tick @ticks times. Because a clock can have a period
> + * which is not a whole number of nanoseconds, it is important
> + * to use this function when calculating things like timer
> + * expiry deadlines, rather than attempting to obtain a "period
> + * in nanoseconds" value and then multiplying that by a number
> + * of ticks.
> + */
> +static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
> +{
> +    uint64_t ns_low, ns_high;
> +
> +    /*
> +     * clk->period is the period in units of 2^-32 ns, so
> +     * (clk->period * ticks) is the required length of time in those
> +     * units, and we can convert to nanoseconds by multiplying by
> +     * 2^32, which is the same as shifting the 128-bit multiplication
> +     * result right by 32.
> +     */
> +    mulu64(&ns_low, &ns_high, clk->period, ticks);
> +    return ns_low >> 32 | ns_high << 32;

With the shift, you're discarding the high 32 bits of the result.  You'll lose
those same bits if you shift one of the inputs left by 32, and use only the
high part of the result, e.g.

    mulu(&discard, &ret, clk->period, ticks << 32);
    return ret;

Which on some hosts, e.g. aarch64, only requires umulh and not two multiply
instructions.

Either way, I wonder if you want to either use uint32_t ticks, or assert that
ticks <= UINT32_MAX?  Or if you don't shift ticks, assert that ns_high <=
UINT32_MAX, so that you don't lose output bits?


r~

Re: [PATCH 2/4] target/mips: Don't use clock_get_ns() in clock period calculation

[Richard Henderson]

On 12/8/20 12:15 PM, Peter Maydell wrote:
> Currently the MIPS code uses the old clock_get_ns() API to
> calculate a time length in nanoseconds:
>  cpu->cp0_count_rate * clock_get_ns(MIPS_CPU(cpu)->clock)
> 
> This relies on the clock having a period which is an exact number
> of nanoseconds.
> 
> Switch to the new clock_ticks_to_ns() function, which does the
> multiplication internally at a higher precision.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/mips/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/4] clock: Remove clock_get_ns()

[Richard Henderson]

On 12/8/20 12:15 PM, Peter Maydell wrote:
> Remove the now-unused clock_get_ns() API and the CLOCK_PERIOD_TO_NS()
> macro that only it was using.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  docs/devel/clocks.rst | 17 +++++++++++++----
>  include/hw/clock.h    |  6 ------
>  2 files changed, 13 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/4] clock: Define and use new clock_display_freq()

[Richard Henderson]

On 12/8/20 12:15 PM, Peter Maydell wrote:
> It's common to want to print a human-readable indication of a clock's
> frequency. Provide a utility function in the clock API to return a
> string which is a displayable representation of the frequency,
> and use it in qdev-monitor.c.
> 
> Before:
> 
>   (qemu) info qtree
>   [...]
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=3.333333e+07
>     mmio 00000000f8000000/0000000000001000
> 
> After:
> 
>   dev: xilinx,zynq_slcr, id ""
>     clock-in "ps_clk" freq_hz=33.3 MHz
>     mmio 00000000f8000000/0000000000001000
> 
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is based on Philippe's patch
> "qdev-monitor: Display frequencies scaled to SI unit"
> but I have abstracted out the "prettified string" into the clock API.
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Luc Michel]

On 12/9/20 12:39 AM, Richard Henderson wrote:
> On 12/8/20 12:15 PM, Peter Maydell wrote:
>> The clock_get_ns() API claims to return the period of a clock in
>> nanoseconds. Unfortunately since it returns an integer and a
>> clock's period is represented in units of 2^-32 nanoseconds,
>> the result is often an approximation, and calculating a clock
>> expiry deadline by multiplying clock_get_ns() by a number-of-ticks
>> is unacceptably inaccurate.
>>
>> Introduce a new API clock_ticks_to_ns() which returns the number
>> of nanoseconds it takes the clock to make a given number of ticks.
>> This function can do the complete calculation internally and
>> will thus give a more accurate result.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> The 64x64->128 multiply is a bit painful for 32-bit and I
>> guess in theory since we know we only want bits [95:32]
>> of the result we could special-case it, but TBH I don't
>> think 32-bit hosts merit much optimization effort these days.
>> ---
>>   docs/devel/clocks.rst | 15 +++++++++++++++
>>   include/hw/clock.h    | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
>> index e5da28e2111..aebeedbb95e 100644
>> --- a/docs/devel/clocks.rst
>> +++ b/docs/devel/clocks.rst
>> @@ -258,6 +258,21 @@ Here is an example:
>>                           clock_get_ns(dev->my_clk_input));
>>       }
>>   
>> +Calculating expiry deadlines
>> +----------------------------
>> +
>> +A commonly required operation for a clock is to calculate how long
>> +it will take for the clock to tick N times; this can then be used
>> +to set a timer expiry deadline. Use the function ``clock_ticks_to_ns()``,
>> +which takes an unsigned 64-bit count of ticks and returns the length
>> +of time in nanoseconds required for the clock to tick that many times.
>> +
>> +It is important not to try to calculate expiry deadlines using a
>> +shortcut like multiplying a "period of clock in nanoseconds" value
>> +by the tick count, because clocks can have periods which are not a
>> +whole number of nanoseconds, and the accumulated error in the
>> +multiplication can be significant.
>> +
>>   Changing a clock period
>>   -----------------------
>>   
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> index 81bcf3e505a..a9425d9fb14 100644
>> --- a/include/hw/clock.h
>> +++ b/include/hw/clock.h
>> @@ -16,6 +16,7 @@
>>   
>>   #include "qom/object.h"
>>   #include "qemu/queue.h"
>> +#include "qemu/host-utils.h"
>>   
>>   #define TYPE_CLOCK "clock"
>>   OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
>> @@ -218,6 +219,34 @@ static inline unsigned clock_get_ns(Clock *clk)
>>       return CLOCK_PERIOD_TO_NS(clock_get(clk));
>>   }
>>   
>> +/**
>> + * clock_ticks_to_ns:
>> + * @clk: the clock to query
>> + * @ticks: number of ticks
>> + *
>> + * Returns the length of time in nanoseconds for this clock
>> + * to tick @ticks times. Because a clock can have a period
>> + * which is not a whole number of nanoseconds, it is important
>> + * to use this function when calculating things like timer
>> + * expiry deadlines, rather than attempting to obtain a "period
>> + * in nanoseconds" value and then multiplying that by a number
>> + * of ticks.
>> + */
>> +static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
>> +{
>> +    uint64_t ns_low, ns_high;
>> +
>> +    /*
>> +     * clk->period is the period in units of 2^-32 ns, so
>> +     * (clk->period * ticks) is the required length of time in those
>> +     * units, and we can convert to nanoseconds by multiplying by
>> +     * 2^32, which is the same as shifting the 128-bit multiplication
>> +     * result right by 32.
>> +     */
>> +    mulu64(&ns_low, &ns_high, clk->period, ticks);
>> +    return ns_low >> 32 | ns_high << 32;
> 
> With the shift, you're discarding the high 32 bits of the result.  You'll lose
> those same bits if you shift one of the inputs left by 32, and use only the
> high part of the result, e.g.
> 
>      mulu(&discard, &ret, clk->period, ticks << 32);
>      return ret;
> 
> Which on some hosts, e.g. aarch64, only requires umulh and not two multiply
> instructions.
> 
> Either way, I wonder if you want to either use uint32_t ticks, or assert that
> ticks <= UINT32_MAX?  Or if you don't shift ticks, assert that ns_high <=
> UINT32_MAX, so that you don't lose output bits?

If I'm not mistaken, loosing bits in the 32 bits upper part would mean 
that the number of ticks correspond to a period greater or equal to:
   2^96 ns ~= 251230855258 years.
So I guess this case is not that relevant anyways. Maybe asserting here 
would help the developer using this function to catch a bug in her/his code.

> 
> 
> r~
> 

Re: [PATCH 2/4] target/mips: Don't use clock_get_ns() in clock period calculation

[Luc Michel]

On 12/8/20 7:15 PM, Peter Maydell wrote:
> Currently the MIPS code uses the old clock_get_ns() API to
> calculate a time length in nanoseconds:
>   cpu->cp0_count_rate * clock_get_ns(MIPS_CPU(cpu)->clock)
> 
> This relies on the clock having a period which is an exact number
> of nanoseconds.
> 
> Switch to the new clock_ticks_to_ns() function, which does the
> multiplication internally at a higher precision.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
>   target/mips/cpu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 76d50b00b42..de15ec6068a 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -147,8 +147,8 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
>   {
>       CPUMIPSState *env = &cpu->env;
>   
> -    env->cp0_count_ns = cpu->cp0_count_rate
> -                        * clock_get_ns(MIPS_CPU(cpu)->clock);
> +    env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
> +                                          cpu->cp0_count_rate);
>       assert(env->cp0_count_ns);
>   }
>   
> 

Re: [PATCH 3/4] clock: Remove clock_get_ns()

[Luc Michel]

On 12/8/20 7:15 PM, Peter Maydell wrote:
> Remove the now-unused clock_get_ns() API and the CLOCK_PERIOD_TO_NS()
> macro that only it was using.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
>   docs/devel/clocks.rst | 17 +++++++++++++----
>   include/hw/clock.h    |  6 ------
>   2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index aebeedbb95e..9a93d1361b4 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -238,8 +238,17 @@ object during device instance init. For example:
>   Fetching clock frequency/period
>   -------------------------------
>   
> -To get the current state of a clock, use the functions ``clock_get()``,
> -``clock_get_ns()`` or ``clock_get_hz()``.
> +To get the current state of a clock, use the functions ``clock_get()``
> +or ``clock_get_hz()``.
> +
> +``clock_get()`` returns the period of the clock in its fully precise
> +internal representation, as an unsigned 64-bit integer in units of
> +2^-32 nanoseconds. (For many purposes ``clock_ticks_to_ns()`` will
> +be more convenient; see the section below on expiry deadlines.)
> +
> +``clock_get_hz()`` returns the frequency of the clock, rounded to the
> +next lowest integer. This implies some inaccuracy due to the rounding,
> +so be cautious about using it in calculations.
>   
>   It is also possible to register a callback on clock frequency changes.
>   Here is an example:
> @@ -254,8 +263,8 @@ Here is an example:
>            */
>   
>           /* do something with the new period */
> -        fprintf(stdout, "device new period is %" PRIu64 "ns\n",
> -                        clock_get_ns(dev->my_clk_input));
> +        fprintf(stdout, "device new period is %" PRIu64 "* 2^-32 ns\n",
> +                        clock_get(dev->my_clk_input));
>       }
>   
>   Calculating expiry deadlines
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index a9425d9fb14..9c0b1eb4c3f 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -39,7 +39,6 @@ typedef void ClockCallback(void *opaque);
>    * macro helpers to convert to hertz / nanosecond
>    */
>   #define CLOCK_PERIOD_FROM_NS(ns) ((ns) * (CLOCK_PERIOD_1SEC / 1000000000llu))
> -#define CLOCK_PERIOD_TO_NS(per) ((per) / (CLOCK_PERIOD_1SEC / 1000000000llu))
>   #define CLOCK_PERIOD_FROM_HZ(hz) (((hz) != 0) ? CLOCK_PERIOD_1SEC / (hz) : 0u)
>   #define CLOCK_PERIOD_TO_HZ(per) (((per) != 0) ? CLOCK_PERIOD_1SEC / (per) : 0u)
>   
> @@ -214,11 +213,6 @@ static inline unsigned clock_get_hz(Clock *clk)
>       return CLOCK_PERIOD_TO_HZ(clock_get(clk));
>   }
>   
> -static inline unsigned clock_get_ns(Clock *clk)
> -{
> -    return CLOCK_PERIOD_TO_NS(clock_get(clk));
> -}
> -
>   /**
>    * clock_ticks_to_ns:
>    * @clk: the clock to query
> 

Re: [PATCH 4/4] clock: Define and use new clock_display_freq()

[Luc Michel]

On 12/8/20 7:15 PM, Peter Maydell wrote:
> It's common to want to print a human-readable indication of a clock's
> frequency. Provide a utility function in the clock API to return a
> string which is a displayable representation of the frequency,
> and use it in qdev-monitor.c.
> 
> Before:
> 
>    (qemu) info qtree
>    [...]
>    dev: xilinx,zynq_slcr, id ""
>      clock-in "ps_clk" freq_hz=3.333333e+07
>      mmio 00000000f8000000/0000000000001000
> 
> After:
> 
>    dev: xilinx,zynq_slcr, id ""
>      clock-in "ps_clk" freq_hz=33.3 MHz
>      mmio 00000000f8000000/0000000000001000
> 
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> This is based on Philippe's patch
> "qdev-monitor: Display frequencies scaled to SI unit"
> but I have abstracted out the "prettified string" into the clock API.
> ---
>   docs/devel/clocks.rst  |  5 +++++
>   include/hw/clock.h     | 12 ++++++++++++
>   hw/core/clock.c        |  6 ++++++
>   softmmu/qdev-monitor.c |  6 +++---
>   4 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index 9a93d1361b4..cf8067542a1 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -267,6 +267,11 @@ Here is an example:
>                           clock_get(dev->my_clk_input));
>       }
>   
> +If you are only interested in the frequency for displaying it to
> +humans (for instance in debugging), use ``clock_display_freq()``,
> +which returns a prettified string-representation, e.g. "33.3 MHz".
> +The caller must free the string with g_free() after use.
> +
>   Calculating expiry deadlines
>   ----------------------------
>   
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index 9c0b1eb4c3f..7bc9afb0800 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -252,4 +252,16 @@ static inline bool clock_is_enabled(const Clock *clk)
>       return clock_get(clk) != 0;
>   }
>   
> +/**
> + * clock_display_freq: return human-readable representation of clock frequency
> + * @clk: clock
> + *
> + * Return a string which has a human-readable representation of the
> + * clock's frequency, e.g. "33.3 MHz". This is intended for debug
> + * and display purposes.
> + *
> + * The caller is responsible for freeing the string with g_free().
> + */
> +char *clock_display_freq(Clock *clk);
> +
>   #endif /* QEMU_HW_CLOCK_H */
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> index 8c6af223e7c..76b5f468b6e 100644
> --- a/hw/core/clock.c
> +++ b/hw/core/clock.c
> @@ -12,6 +12,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include "hw/clock.h"
>   #include "trace.h"
>   
> @@ -111,6 +112,11 @@ static void clock_disconnect(Clock *clk)
>       QLIST_REMOVE(clk, sibling);
>   }
>   
> +char *clock_display_freq(Clock *clk)
> +{
> +    return freq_to_str(clock_get_hz(clk));
> +}
> +
>   static void clock_initfn(Object *obj)
>   {
>       Clock *clk = CLOCK(obj);
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index bf79d0bbcd9..6263d600026 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -747,11 +747,11 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>           }
>       }
>       QLIST_FOREACH(ncl, &dev->clocks, node) {
> -        qdev_printf("clock-%s%s \"%s\" freq_hz=%e\n",
> +        g_autofree char *freq_str = clock_display_freq(ncl->clock);
> +        qdev_printf("clock-%s%s \"%s\" freq_hz=%s\n",
>                       ncl->output ? "out" : "in",
>                       ncl->alias ? " (alias)" : "",
> -                    ncl->name,
> -                    CLOCK_PERIOD_TO_HZ(1.0 * clock_get(ncl->clock)));
> +                    ncl->name, freq_str);
>       }
>       class = object_get_class(OBJECT(dev));
>       do {
> 

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Richard Henderson]

On 12/9/20 2:49 AM, Luc Michel wrote:
> On 12/9/20 12:39 AM, Richard Henderson wrote:
>> On 12/8/20 12:15 PM, Peter Maydell wrote:
>>> The clock_get_ns() API claims to return the period of a clock in
>>> nanoseconds. Unfortunately since it returns an integer and a
>>> clock's period is represented in units of 2^-32 nanoseconds,
>>> the result is often an approximation, and calculating a clock
>>> expiry deadline by multiplying clock_get_ns() by a number-of-ticks
>>> is unacceptably inaccurate.
>>>
>>> Introduce a new API clock_ticks_to_ns() which returns the number
>>> of nanoseconds it takes the clock to make a given number of ticks.
>>> This function can do the complete calculation internally and
>>> will thus give a more accurate result.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> The 64x64->128 multiply is a bit painful for 32-bit and I
>>> guess in theory since we know we only want bits [95:32]
>>> of the result we could special-case it, but TBH I don't
>>> think 32-bit hosts merit much optimization effort these days.
>>> ---
>>>   docs/devel/clocks.rst | 15 +++++++++++++++
>>>   include/hw/clock.h    | 29 +++++++++++++++++++++++++++++
>>>   2 files changed, 44 insertions(+)
>>>
>>> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
>>> index e5da28e2111..aebeedbb95e 100644
>>> --- a/docs/devel/clocks.rst
>>> +++ b/docs/devel/clocks.rst
>>> @@ -258,6 +258,21 @@ Here is an example:
>>>                           clock_get_ns(dev->my_clk_input));
>>>       }
>>>   +Calculating expiry deadlines
>>> +----------------------------
>>> +
>>> +A commonly required operation for a clock is to calculate how long
>>> +it will take for the clock to tick N times; this can then be used
>>> +to set a timer expiry deadline. Use the function ``clock_ticks_to_ns()``,
>>> +which takes an unsigned 64-bit count of ticks and returns the length
>>> +of time in nanoseconds required for the clock to tick that many times.
>>> +
>>> +It is important not to try to calculate expiry deadlines using a
>>> +shortcut like multiplying a "period of clock in nanoseconds" value
>>> +by the tick count, because clocks can have periods which are not a
>>> +whole number of nanoseconds, and the accumulated error in the
>>> +multiplication can be significant.
>>> +
>>>   Changing a clock period
>>>   -----------------------
>>>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>>> index 81bcf3e505a..a9425d9fb14 100644
>>> --- a/include/hw/clock.h
>>> +++ b/include/hw/clock.h
>>> @@ -16,6 +16,7 @@
>>>     #include "qom/object.h"
>>>   #include "qemu/queue.h"
>>> +#include "qemu/host-utils.h"
>>>     #define TYPE_CLOCK "clock"
>>>   OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
>>> @@ -218,6 +219,34 @@ static inline unsigned clock_get_ns(Clock *clk)
>>>       return CLOCK_PERIOD_TO_NS(clock_get(clk));
>>>   }
>>>   +/**
>>> + * clock_ticks_to_ns:
>>> + * @clk: the clock to query
>>> + * @ticks: number of ticks
>>> + *
>>> + * Returns the length of time in nanoseconds for this clock
>>> + * to tick @ticks times. Because a clock can have a period
>>> + * which is not a whole number of nanoseconds, it is important
>>> + * to use this function when calculating things like timer
>>> + * expiry deadlines, rather than attempting to obtain a "period
>>> + * in nanoseconds" value and then multiplying that by a number
>>> + * of ticks.
>>> + */
>>> +static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
>>> +{
>>> +    uint64_t ns_low, ns_high;
>>> +
>>> +    /*
>>> +     * clk->period is the period in units of 2^-32 ns, so
>>> +     * (clk->period * ticks) is the required length of time in those
>>> +     * units, and we can convert to nanoseconds by multiplying by
>>> +     * 2^32, which is the same as shifting the 128-bit multiplication
>>> +     * result right by 32.
>>> +     */
>>> +    mulu64(&ns_low, &ns_high, clk->period, ticks);
>>> +    return ns_low >> 32 | ns_high << 32;
>>
>> With the shift, you're discarding the high 32 bits of the result.  You'll lose
>> those same bits if you shift one of the inputs left by 32, and use only the
>> high part of the result, e.g.
>>
>>      mulu(&discard, &ret, clk->period, ticks << 32);
>>      return ret;
>>
>> Which on some hosts, e.g. aarch64, only requires umulh and not two multiply
>> instructions.
>>
>> Either way, I wonder if you want to either use uint32_t ticks, or assert that
>> ticks <= UINT32_MAX?  Or if you don't shift ticks, assert that ns_high <=
>> UINT32_MAX, so that you don't lose output bits?
> 
> If I'm not mistaken, loosing bits in the 32 bits upper part would mean that the
> number of ticks correspond to a period greater or equal to:
>   2^96 ns ~= 251230855258 years.

No, would be the bit above above ns_high (this integer 64x64->128
multiplication is computing fixed point 64.0 x 32.32 -> 96.32).

This function is truncating back to 64.0, dropping the 32 high bits and 32 low
bits.  We lose bits at 2^64 ns ~= 584 years.  Which is still unreasonably long,
but could still be had from a timer setting ~= never.

An alternate to an assert could be saturation.  Input "infinity", return
"infinity".  More or less.


r~

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Peter Maydell]

On Wed, 9 Dec 2020 at 14:11, Richard Henderson
<richard.henderson@linaro.org> wrote:
> This function is truncating back to 64.0, dropping the 32 high bits and 32 low
> bits.  We lose bits at 2^64 ns ~= 584 years.  Which is still unreasonably long,
> but could still be had from a timer setting ~= never.
>
> An alternate to an assert could be saturation.  Input "infinity", return
> "infinity".  More or less.

Might be an idea. We have never really properly nailed down what
QEMU's simulation of time does when it hits INT64_MAX nanoseconds
(which is the furthest forward absolute time you can set a QEMUTimer).

In particular if you use the icount sleep=on option it is actually
possible for the simulation to get there (set a far-future timer,
no other interrupts or simulation input, do a sleep-til-next-interrupt)
and I have no idea what QEMU should do at that point (print "Welcome
to the end of the universe" and exit?).

FWIW, the reason I made this API take a 64-bit tick count and return
a 64-bit nanosecond count is because we do have timer devices where
the tick count is 64 bits (eg the Arm Generic Timers in the CPU)
and the QEMUTimer APIs all want "expiry date in nanoseconds as a
signed 64-bit value".

thanks
-- PMM

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Richard Henderson]

On 12/9/20 8:26 AM, Peter Maydell wrote:
> (print "Welcome to the end of the universe" and exit?)

Sounds appropriately Hitchhiker's Guide.  ;-)


r~

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Peter Maydell]

On Tue, 8 Dec 2020 at 23:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/8/20 12:15 PM, Peter Maydell wrote:
> > +static inline uint64_t clock_ticks_to_ns(const Clock *clk, uint64_t ticks)
> > +{
> > +    uint64_t ns_low, ns_high;
> > +
> > +    /*
> > +     * clk->period is the period in units of 2^-32 ns, so
> > +     * (clk->period * ticks) is the required length of time in those
> > +     * units, and we can convert to nanoseconds by multiplying by
> > +     * 2^32, which is the same as shifting the 128-bit multiplication
> > +     * result right by 32.
> > +     */
> > +    mulu64(&ns_low, &ns_high, clk->period, ticks);
> > +    return ns_low >> 32 | ns_high << 32;
>
> With the shift, you're discarding the high 32 bits of the result.  You'll lose
> those same bits if you shift one of the inputs left by 32, and use only the
> high part of the result, e.g.
>
>     mulu(&discard, &ret, clk->period, ticks << 32);
>     return ret;
>
> Which on some hosts, e.g. aarch64, only requires umulh and not two multiply
> instructions.

We can't do this if we want to allow a full 64-bit 'ticks' input, right?

> Either way, I wonder if you want to either use uint32_t ticks, or assert that
> ticks <= UINT32_MAX?  Or if you don't shift ticks, assert that ns_high <=
> UINT32_MAX, so that you don't lose output bits?

So I think my plan for v2 of this series is just to add in the
saturation-to-INT64_MAX logic.

thanks
-- PMM

Re: [PATCH 1/4] clock: Introduce clock_ticks_to_ns()

[Richard Henderson]

On 12/10/20 2:47 PM, Peter Maydell wrote:
>> With the shift, you're discarding the high 32 bits of the result.  You'll lose
>> those same bits if you shift one of the inputs left by 32, and use only the
>> high part of the result, e.g.
>>
>>     mulu(&discard, &ret, clk->period, ticks << 32);
>>     return ret;
>>
>> Which on some hosts, e.g. aarch64, only requires umulh and not two multiply
>> instructions.
> 
> We can't do this if we want to allow a full 64-bit 'ticks' input, right?

Correct.

> So I think my plan for v2 of this series is just to add in the
> saturation-to-INT64_MAX logic.

Sounds good.

r~

Re: [PATCH 0/4] clock: Get rid of clock_get_ns()

[Philippe Mathieu-Daudé]

On 12/8/20 7:15 PM, Peter Maydell wrote:
> This patchseries makes some changes to the clock API:
>  * Remove clock_get_ns()
>  * Add clock_ticks_to_ns() to return number of nanoseconds
>    it will take the clock to tick N times
>  * clock_display_freq() to return prettily-formatted string
>    for showing humans the approximate clock frequency
> 
> This is based on discussions we had about these APIs a little while
> back.  The core driver here is that the clock objects internally
> store the period in units of 2^-32 ns, so both clock_get_ns() and
> clock_get_hz() are inherently returning a rounded-off result, which
> can be badly inaccurate for fast clocks or if you want to multiply it
> by a large tick count.
> 
> Ideally I'd like to get rid of clock_get_hz() as well, but
> that looks trickier than handling clock_get_ns().
> 
> Patch 4 borrows a lot of the concept from one of Philippe's that he
> sent out previously.

Thanks for tackling the clock_get_ns() part. I had some work in
progress I was procrastinating for after the release, but your
patches are much better documented :)

(I also started to get rid of clock_get_hz() but, as you figured,
this is not a trivial task).

> NB: tested with 'make check' and 'make check-acceptance' only.

I hit this issue while testing Huacai's MIPS Loongson3 virt machine
which sets the core freq at 1GHz IIRC. I still have the branch
so I'll test your series (or v2) during the week-end.

Regards,

Phil.

Re: [PATCH 0/4] clock: Get rid of clock_get_ns()

[Peter Maydell]

On Fri, 11 Dec 2020 at 13:50, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> (I also started to get rid of clock_get_hz() but, as you figured,
> this is not a trivial task).


Yeah; I haven't really looked at the users of clock_get_hz()
in detail to know whether it's really possible to remove it.
For the serial devices to some extent they really do want a
frequency to feed to the host serial baud-rate stuff...

Also, I have a timer device I'm working on which has a register
for "give number of ticks since simulation start" which should
thus read a value something like
 qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)  / clock_ticks_to_ns(clk, 1);

and this also suffers from possible rounding issues (though not
to the same extent as ticks-to-ns since it's a division rather
than a multiplication). I'm wondering if we should have a clock API
for "convert a duration in nanoseconds to a tick count" directly
as well. Dunno whether that helps with the clock_get_hz() use
cases, or if it's orthogonal to that.

thanks
-- PMM