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

[Anup Patel]

On Wed, Nov 2, 2022 at 5:40 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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>

Sure, I will add a detailed comment block in the code itself.

Thanks,
Anup

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