Re: [PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Anup Patel]

On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > The time CSR will wrap-around immediately after reaching UINT64_MAX
> > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
> > in riscv_timer_write_timecmp().
>
> I'm not clear what this is fixing?
>
> If the guest sets a timer for UINT64_MAX shouldn't that still trigger
> an event at some point?

Here's what Sstc says about timer interrupt using Sstc:
"A supervisor timer interrupt becomes pending - as reflected in the
STIP bit in the mip and sip registers - whenever time contains a
value greater than or equal to stimecmp, treating the values as
unsigned integers. Writes to stimecmp are guaranteed to be
reflected in STIP eventually, but not necessarily immediately.
The interrupt remains posted until stimecmp becomes greater
than time - typically as a result of writing stimecmp."

When timecmp = UINT64_MAX, the time CSR will eventually reach
timecmp value but on next timer tick the time CSR will wrap-around
and become zero which is less than UINT64_MAX. Now, the timer
interrupt behaves like a level triggered interrupt so it will become 1
when time = timecmp = UINT64_MAX and next timer tick it will
become 0 again because time = 0 < timecmp = UINT64_MAX.

This time CSR wrap-around comparison with timecmp is natural
to implement in HW but not straight forward in QEMU hence this
patch.

Software can potentially use timecmp = UINT64_MAX as a way
to clear the timer interrupt and keep timer disabled instead of
enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps:
1) Linux RISC-V timer driver keep timer interrupt enable/disable
    state in-sync with Linux interrupt subsystem.
2) Reduce number of traps taken when emulating Sstc for the
    "Nested Guest" (i.e. Guest running under some "Guest Hypervisor"
    which in-turn runs under a "Host Hypervisor").

In fact, the SBI set_timer() call also defines similar mechanism to
disable timer: "If the supervisor wishes to clear the timer interrupt
without scheduling the next timer event, it can either request a timer
interrupt infinitely far into the future (i.e., (uint64_t)-1), ...".

Regards,
Anup

>
> Alistair
>
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  target/riscv/time_helper.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > index 4fb2a471a9..1ee9f94813 100644
> > --- a/target/riscv/time_helper.c
> > +++ b/target/riscv/time_helper.c
> > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer 
> > *timer,
> >          riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
> >      }
> >
> > +    /*
> > +     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
> > +     * time CSR will wrap-around immediately after reaching UINT64_MAX.
> > +     */
> > +    if (timecmp == UINT64_MAX) {
> > +        return;
> > +    }
> > +
> >      /* otherwise, set up the future timer interrupt */
> >      diff = timecmp - rtc_r;
> >      /* back to ns (note args switched in muldiv64) */
> > --
> > 2.34.1
> >
> >