Re: [PATCH] target/riscv: Stop timer with infinite timecmp

[Andrew Jones]

On Fri, Aug 30, 2024 at 02:05:05PM GMT, Vladimir Isaev wrote:
> Hi Andrew,
> 
> 29.08.2024 11:40, Andrew Jones wrote:
> > While the spec doesn't state it, setting timecmp to UINT64_MAX is
> > another way to stop a timer, as it's considered setting the next
> > timer event to occur at infinity. 
> 
> I think this should be explicitly stated in the spec, since some software
> may initially set time and timecmp to big values just to check how overflow
> is handled. And without it no chance that all HW implementations will 
> interpret
> UINT64_MAX as 'stop timer'.
> 
> Do we need github issue on SSTC/privileged?

Hi Vladimir,

I don't think we need to update the spec, so hardware is free to make the
interrupt pending if time ever reaches UINT64_MAX. However, the hardware
will then need to clear the pending interrupt on the very next increment
of time. This means even if the interrupt is raised it will most likely
look spurious to the handler. I think we can leave this to software. If
the software wants to use UINT64_MAX to stop timers, then it may also want
to add a check in its timer handlers for timecmp == UINT64_MAX, and, under
that condition, just ignore the interrupt.

Thanks,
drew

> 
> Thank you,
> Vladimir Isaev
> 
> > And, even if the time CSR does
> > eventually reach UINT64_MAX, the very next tick will bring it back to
> > zero, once again less than timecmp. For this reason
> > riscv_timer_write_timecmp() special cases UINT64_MAX. However, if a
> > previously set timecmp has not yet expired, then setting timecmp to
> > UINT64_MAX to disable / stop it would not work, as the special case
> > left the previous QEMU timer active, which would then still deliver
> > an interrupt at that previous timecmp time. Ensure the stopped timer
> > will not still deliver an interrupt by also deleting the QEMU timer
> > in the UINT64_MAX special case.
> > 
> > Fixes: ae0edf2188b3 ("target/riscv: No need to re-start QEMU timer when 
> > timecmp == UINT64_MAX")
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  target/riscv/time_helper.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > index 8d245bed3ae3..bc0d9a0c4c35 100644
> > --- a/target/riscv/time_helper.c
> > +++ b/target/riscv/time_helper.c
> > @@ -92,6 +92,7 @@ void riscv_timer_write_timecmp(CPURISCVState *env, 
> > QEMUTimer *timer,
> >       * equals UINT64_MAX.
> >       */
> >      if (timecmp == UINT64_MAX) {
> > +        timer_del(timer);
> >          return;
> >      }
> >  
> 
> 
>