Re: [Qemu-devel] [PATCH 5/5] aspeed/timer: Use signed muldiv for timer resets

[Peter Maydell]

On Thu, 14 Mar 2019 at 08:43, Cédric Le Goater <address@hidden> wrote:
>
> From: Christian Svensson <address@hidden>
>
> If the host decrements the counter register that results in a negative
> delta. This is then passed to muldiv64 which only handles unsigned
> numbers resulting in bogus results.
>
> This fix ensures the data being operated on is signed before it is
> ultimately casted to the final unsigned value.
>
> Test case: kexec a kernel using aspeed_timer and it will freeze on the
> second bootup when the kernel initializes the timer. With this patch
> that no longer happens and the timer appears to run OK.
>
> Signed-off-by: Christian Svensson <address@hidden>
> [clg: - checkpatch fixes ]
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  hw/timer/aspeed_timer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 9988b8fbbf17..0b16eac8970c 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -275,7 +275,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState 
> *s, int timer, int reg,
>              int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, 
> now);
>              uint32_t rate = calculate_rate(t);
>
> -            t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
> +            t->start = (int64_t)t->start +
> +                ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate);
>              aspeed_timer_mod(t);

You can't guarantee that __int128_t exists. If you must
use it then you need an alternate code path for the ifndef
CONFIG_INT128 case (which is going to be complicated enough
that this should really be abstracted away into a
signed version of muldiv64().)

But overall I'm a little sceptical that the aspeed timer is
really a special case that needs a signed version of this
when no other timer in the system does...

thanks
-- PMM