Re: [RFC PATCH 4/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_NS()

From: Peter Maydell <peter.maydell@linaro.org>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Damien Hedde" <damien.hedde@greensocs.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Luc Michel" <luc.michel@greensocs.com>
Subject: Re: [RFC PATCH 4/4] hw/clock: Inline and remove CLOCK_PERIOD_TO_NS()
Date: Tue, 20 Oct 2020 21:25:54 +0100	[thread overview]
Message-ID: <CAFEAcA_SjX3OVof0A1fOWNkVT8wc9QEmA=3Wj2RgRKZNMFbQqg@mail.gmail.com> (raw)
In-Reply-To: <20201020182008.2240590-5-f4bug@amsat.org>

On Tue, 20 Oct 2020 at 19:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> This macro is only used once. Inline caring about 64-bit
> multiplication, and remove it.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/clock.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index b58038f1e7d..f329fcf0ea5 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)
> @@ -38,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)
>
>  /**
> @@ -210,9 +210,14 @@ static inline uint64_t clock_get_hz(Clock *clk)
>      return CLOCK_PERIOD_1SEC / clk->period;
>  }
>
> -static inline unsigned clock_get_ns(Clock *clk)
> +static inline uint64_t clock_get_ns(Clock *clk)
>  {
> -    return CLOCK_PERIOD_TO_NS(clock_get(clk));
> +    uint64_t lo, hi;
> +
> +    mulu64(&lo, &hi, clock_get(clk), 1000000000llu);
> +    divu128(&lo, &hi, CLOCK_PERIOD_1SEC);
> +
> +    return lo;
>  }

I think the clock_get_ns() function is still flawed
regardless of its return type or internal implementation.
If you have a 2GHz clock then the correct answer is
"0.5" and so an integer representation is going to be
wrong by 0.5ns. If the reason why you wanted the period
in nanoseconds was so you could multiply it by a number
of ticks in order to work out when to set a timer, you
cannot live with the error resulting from rounding it
either way. We need to replace this function with one
which does the whole job of "tell me how many nanoseconds
X ticks of this clock will take" and does the entire
calculation so it can do it without introducing rounding errors.

thanks
-- PMM