| Message ID | 20240829084002.1805006-2-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Stop timer with infinite timecmp | expand |
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 > >
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; > } >
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; > > } > > > > >
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; >>> } >>> >> >> >>
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 > >
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; }
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(+)