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

[Alistair Francis]

On Mon, Oct 31, 2022 at 1:49 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> 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.

Ah, I didn't realise this. Can you add this to the code comment and
maybe add this description to the commit message. Otherwise:

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

Alistair

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