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

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

[Andrew Jones]

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. 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;
     }
 
-- 
2.45.2

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

[Alistair Francis]

On Thu, Aug 29, 2024 at 6:40 PM Andrew Jones <ajones@ventanamicro.com> 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. 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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;
>      }
>
> --
> 2.45.2
>
>

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

[Vladimir Isaev]

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?

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;
>      }
>  

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;
> >      }
> >  
> 
> 
> 

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

[Vladimir Isaev]

30.08.2024 17:50, 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.
> 

oh, I got it, thank you for explanation!

> 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;
>>>      }
>>>  
>>
>>
>>

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

[Alistair Francis]

On Thu, Aug 29, 2024 at 6:40 PM Andrew Jones <ajones@ventanamicro.com> 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. 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>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  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;
>      }
>
> --
> 2.45.2
>
>