Re: [PATCH v2] hw/intc/sifive_clint: Fix expiration time logic

[Alistair Francis]

On Sun, Aug 29, 2021 at 12:27 PM s101062801 <s101062801@m101.nthu.edu.tw> wrote:
>
> After timecmp is modified, the value is converted into nanosecond,
> and pass to timer_mod.  However, timer_mod perceives the value as
> a signed integer.  An example that goes wrong is as follows:
>
> OpenSBI v0.9 initializes the cold boot hart's timecmp to
> 0xffffffff_ffffffff.  timer_mod then interprets the product of the
> following calculation as a negative value.  As a result, the clint
> timer is pulled out of timerlist because it looks like it has
> expired, which cause the MIP.MTIP is set before any real timer
> interrupt.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
> Signed-off-by: Quey-Liang Kao <s101062801@m101.nthu.edu.tw>
> ---
> v2:
>   - Fix the calculation for next.
>   - Link to issue 493.
>   - I saw David's after I made this patch.  Yet I want to correct the
> error
>     in v1.

Hello,

Thanks for the patch!

As David's was sent first I am going to apply that instead of this
one. Sorry about that.

Please feel free to send more QEMU patches in the future or to review
other people's patches.

Hopefully we will see you again :)

Alistair

> ---
>   hw/intc/sifive_clint.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> index 0f41e5ea1c..78f01d17d3 100644
> --- a/hw/intc/sifive_clint.c
> +++ b/hw/intc/sifive_clint.c
> @@ -44,6 +44,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>   {
>       uint64_t next;
>       uint64_t diff;
> +    uint64_t now;
>
>       uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
>
> @@ -59,9 +60,11 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>       riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
>       diff = cpu->env.timecmp - rtc_r;
>       /* back to ns (note args switched in muldiv64) */
> -    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> -    timer_mod(cpu->env.timer, next);
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    next = now + muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> +    timer_mod(cpu->env.timer, (next <= now) ?
> +                              (int64_t)((1ULL << 63) - 1) :
> +                              next);
>   }
>
>   /*
> --
> 2.32.0
>