Re: [PATCH 1/2] target/riscv: disable *stimecmp interrupts without *envcfg.STCE

[Andrew Jones <ajones@ventanamicro.com>]

On Mon, Jun 23, 2025 at 06:39:02PM -0300, Daniel Henrique Barboza wrote:
> Hi Radim,
> 
> It seems like this patch is breaking 'make check-functional':
> 
> 12/12 qemu:func-quick+func-riscv64 / func-riscv64-riscv_opensbi   TIMEOUT         90.06s   killed by signal 15 SIGTERM
> 
> Checking the logs I verified that the problem can be reproduced by running the
> 'spike' machine as follows:
> 
> $ ./build/qemu-system-riscv64 -M spike   --nographic
> Segmentation fault (core dumped)
> 
> The expected result is to boot opensbi. The problem can't be reproduced with
> the 'virt' board, so something that you did here impacted 'spike' in particular
> for some reason.

FWIW, spike uses the masking approach for henvcfg.STCE. See [1]

[1] https://lists.riscv.org/g/tech-privileged/message/2385

Thanks,
drew

> 
> 
> Thanks,
> 
> Daniel
> 
> On 6/23/25 1:53 PM, Radim Krčmář wrote:
> > The specification states that menvcfg.STCE=0 prevents both *stimecmp
> > CSRs from having an effect on the pending interrupts.
> > henvcfg.STCE=0 disables only vstimecmp.
> > 
> > Make sure that when *envcfg.STCE is not set:
> > * writing the *stimecmp CSRs doesn't modify the *ip CSRs,
> > * and that the interrupt timer is disarmed.
> > 
> > Call the *stimecmp CSR update functions when *envcfg.STCE is toggled,
> > because the *ip CSRs need to immediately reflect the new behavior.
> > 
> > Fixes: 43888c2f1823 ("target/riscv: Add stimecmp support")
> > Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> > ---
> >   target/riscv/csr.c         | 12 ++++++++++++
> >   target/riscv/time_helper.c | 10 ++++++++++
> >   2 files changed, 22 insertions(+)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index fb149721691d..43eae9bcf153 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -3181,6 +3181,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >       const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> >       uint64_t mask = MENVCFG_FIOM | MENVCFG_CBIE | MENVCFG_CBCFE |
> >                       MENVCFG_CBZE | MENVCFG_CDE;
> > +    typeof(env->menvcfg) old = env->menvcfg;
> >       if (riscv_cpu_mxl(env) == MXL_RV64) {
> >           mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> > @@ -3208,6 +3209,11 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >           }
> >       }
> >       env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> > +
> > +    if ((old ^ env->menvcfg) & MENVCFG_STCE) {
> > +        riscv_timer_write_timecmp(env, env->stimer, env->stimecmp, 0, MIP_STIP);
> > +    }
> > +
> >       return write_henvcfg(env, CSR_HENVCFG, env->henvcfg, ra);
> >   }
> > @@ -3314,6 +3320,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >                                       target_ulong val, uintptr_t ra)
> >   {
> >       uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> > +    typeof(env->henvcfg) old = env->henvcfg;
> >       RISCVException ret;
> >       ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > @@ -3347,6 +3354,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >           env->vsstatus &= ~MSTATUS_SDT;
> >       }
> > +    if ((old ^ env->henvcfg) & HENVCFG_STCE) {
> > +        riscv_timer_write_timecmp(env, env->vstimer, env->vstimecmp,
> > +                                  env->htimedelta, MIP_VSTIP);
> > +    }
> > +
> >       return RISCV_EXCP_NONE;
> >   }
> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > index bc0d9a0c4c35..8198a2d8d92d 100644
> > --- a/target/riscv/time_helper.c
> > +++ b/target/riscv/time_helper.c
> > @@ -49,6 +49,16 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
> >       uint32_t timebase_freq = mtimer->timebase_freq;
> >       uint64_t rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
> > +    /*
> > +     * *envcfg.STCE disables *stimecmp interrupts, but still allows higher
> > +     * privileges to write the *stimecmp CSRs.
> > +     */
> > +    if (!get_field(env->menvcfg, MENVCFG_STCE) ||
> > +        (timer_irq == MIP_VSTIP && !get_field(env->henvcfg, HENVCFG_STCE))) {
> > +        timer_del(timer);
> > +        return;
> > +    }
> > +
> >       if (timecmp <= rtc_r) {
> >           /*
> >            * If we're setting an stimecmp value in the "past",
> 
>