OK, I will fix it in the v2 patchset.
Jim Shu
On Fri, Apr 4, 2025 at 2:03 PM Alistair Francis <alistai...@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim....@sifive.com> wrote:
> >
> > Updating STCE will enable/disable SSTC in S-mode or/and VS-mode, so we
> > also need to update S/VS-mode Timer and S/VSTIP bits in $mip CSR.
> >
> > Signed-off-by: Jim Shu <jim....@sifive.com>
> > ---
> > target/riscv/csr.c | 44 ++++++++++++++++++++++++++++++++
> > target/riscv/time_helper.c | 51 ++++++++++++++++++++++++++++++++++++++
> > target/riscv/time_helper.h | 1 +
> > 3 files changed, 96 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index ba026dfc8e..c954e49cae 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -3156,6 +3156,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;
> > + bool stce_changed = false;
> >
> > if (riscv_cpu_mxl(env) == MXL_RV64) {
> > mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
> > @@ -3181,10 +3182,19 @@ static RISCVException write_menvcfg(CPURISCVState
> > *env, int csrno,
> > if ((val & MENVCFG_DTE) == 0) {
> > env->mstatus &= ~MSTATUS_SDT;
> > }
> > +
> > + if (cfg->ext_sstc &&
> > + ((env->menvcfg & MENVCFG_STCE) != (val & MENVCFG_STCE))) {
> > + stce_changed = true;
> > + }
> > }
> > env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> > write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
> >
> > + if (stce_changed) {
> > + riscv_timer_stce_changed(env, true, !!(val & MENVCFG_STCE));
> > + }
> > +
> > return RISCV_EXCP_NONE;
> > }
> >
> > @@ -3207,6 +3217,12 @@ static RISCVException write_menvcfgh(CPURISCVState
> > *env, int csrno,
> > (cfg->ext_smcdeleg ? MENVCFG_CDE : 0) |
> > (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
> > uint64_t valh = (uint64_t)val << 32;
> > + bool stce_changed = false;
> > +
> > + if (cfg->ext_sstc &&
> > + ((env->menvcfg & MENVCFG_STCE) != (valh & MENVCFG_STCE))) {
> > + stce_changed = true;
> > + }
> >
> > if ((valh & MENVCFG_DTE) == 0) {
> > env->mstatus &= ~MSTATUS_SDT;
> > @@ -3215,6 +3231,10 @@ static RISCVException write_menvcfgh(CPURISCVState
> > *env, int csrno,
> > env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> > write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
> >
> > + if (stce_changed) {
> > + riscv_timer_stce_changed(env, true, !!(valh & MENVCFG_STCE));
> > + }
> > +
> > return RISCV_EXCP_NONE;
> > }
> >
> > @@ -3292,8 +3312,10 @@ static RISCVException read_henvcfg(CPURISCVState
> > *env, int csrno,
> > static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> > target_ulong val)
> > {
> > + const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> > uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE |
> > HENVCFG_CBZE;
> > RISCVException ret;
> > + bool stce_changed = false;
> >
> > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > if (ret != RISCV_EXCP_NONE) {
> > @@ -3319,6 +3341,11 @@ static RISCVException write_henvcfg(CPURISCVState
> > *env, int csrno,
> > get_field(val, HENVCFG_PMM) != PMM_FIELD_RESERVED) {
> > mask |= HENVCFG_PMM;
> > }
> > +
> > + if (cfg->ext_sstc &&
> > + ((env->henvcfg & HENVCFG_STCE) != (val & HENVCFG_STCE))) {
> > + stce_changed = true;
> > + }
> > }
> >
> > env->henvcfg = val & mask;
> > @@ -3326,6 +3353,10 @@ static RISCVException write_henvcfg(CPURISCVState
> > *env, int csrno,
> > env->vsstatus &= ~MSTATUS_SDT;
> > }
> >
> > + if (stce_changed) {
> > + riscv_timer_stce_changed(env, false, !!(val & HENVCFG_STCE));
> > + }
> > +
> > return RISCV_EXCP_NONE;
> > }
> >
> > @@ -3347,19 +3378,32 @@ static RISCVException read_henvcfgh(CPURISCVState
> > *env, int csrno,
> > static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> > target_ulong val)
> > {
> > + const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
> > uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
> > HENVCFG_ADUE | HENVCFG_DTE);
> > uint64_t valh = (uint64_t)val << 32;
> > RISCVException ret;
> > + bool stce_changed = false;
> >
> > ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> > if (ret != RISCV_EXCP_NONE) {
> > return ret;
> > }
> > +
> > + if (cfg->ext_sstc &&
> > + ((env->henvcfg & HENVCFG_STCE) != (valh & HENVCFG_STCE))) {
> > + stce_changed = true;
> > + }
> > +
> > env->henvcfg = (env->henvcfg & 0xFFFFFFFF) | (valh & mask);
> > if ((env->henvcfg & HENVCFG_DTE) == 0) {
> > env->vsstatus &= ~MSTATUS_SDT;
> > }
> > +
> > + if (stce_changed) {
> > + riscv_timer_stce_changed(env, false, !!(val & HENVCFG_STCE));
> > + }
> > +
> > return RISCV_EXCP_NONE;
> > }
> >
> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > index aebf0798d0..c648c9fac7 100644
> > --- a/target/riscv/time_helper.c
> > +++ b/target/riscv/time_helper.c
> > @@ -140,6 +140,57 @@ void riscv_timer_write_timecmp(CPURISCVState *env,
> > QEMUTimer *timer,
> > timer_mod(timer, next);
> > }
> >
> > +/*
> > + * When disabling xenvcfg.STCE, the S/VS Timer may be disabled at the same
> > time.
> > + * It is safe to call this function regardless of whether the timer has
> > been
> > + * deleted or not. timer_del() will do nothing if the timer has already
> > + * been deleted.
> > + */
> > +static void riscv_timer_disable_timecmp(CPURISCVState *env, QEMUTimer
> > *timer,
> > + uint32_t timer_irq)
> > +{
> > + /* Disable S-mode Timer IRQ and HW-based STIP */
> > + if ((timer_irq == MIP_STIP) && !get_field(env->menvcfg, MENVCFG_STCE))
> > {
> > + riscv_cpu_update_mip(env, timer_irq, BOOL_TO_MASK(0));
> > + timer_del(timer);
> > + return;
> > + }
> > +
> > + /* Disable VS-mode Timer IRQ and HW-based VSTIP */
> > + if ((timer_irq == MIP_VSTIP) &&
> > + (!get_field(env->menvcfg, MENVCFG_STCE) ||
> > + !get_field(env->henvcfg, HENVCFG_STCE))) {
> > + env->vstime_irq = 0;
> > + riscv_cpu_update_mip(env, 0, BOOL_TO_MASK(0));
> > + timer_del(timer);
> > + return;
> > + }
> > +}
> > +
> > +/* Enable or disable S/VS-mode Timer when xenvcfg.STCE is changed */
> > +void riscv_timer_stce_changed(CPURISCVState *env, bool is_m_mode, bool
> > enable)
> > +{
> > + if (is_m_mode) {
> > + /* menvcfg.STCE changes */
> > + if (enable) {
> > + riscv_timer_write_timecmp(env, env->stimer, env->stimecmp, 0,
> > MIP_STIP);
> > + riscv_timer_write_timecmp(env, env->vstimer, env->vstimecmp,
> > + env->htimedelta, MIP_VSTIP);
>
> This line and ...
>
> > + } else {
> > + riscv_timer_disable_timecmp(env, env->stimer, MIP_STIP);
> > + riscv_timer_disable_timecmp(env, env->vstimer, MIP_VSTIP);
>
> This line are duplicated below.
>
> > + }
> > + } else {
>
> We can remove the else
>
> > + /* henvcfg.STCE changes */
> > + if (enable) {
> > + riscv_timer_write_timecmp(env, env->vstimer, env->vstimecmp,
> > + env->htimedelta, MIP_VSTIP);
> > + } else {
> > + riscv_timer_disable_timecmp(env, env->vstimer, MIP_VSTIP);
> > + }
>
> and always run this branch to remove the duplicated code above
>
> Alistair
>
> > + }
> > +}
> > +
> > void riscv_timer_init(RISCVCPU *cpu)
> > {
> > CPURISCVState *env;
> > diff --git a/target/riscv/time_helper.h b/target/riscv/time_helper.h
> > index cacd79b80c..af1f634f89 100644
> > --- a/target/riscv/time_helper.h
> > +++ b/target/riscv/time_helper.h
> > @@ -25,6 +25,7 @@
> > void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
> > uint64_t timecmp, uint64_t delta,
> > uint32_t timer_irq);
> > +void riscv_timer_stce_changed(CPURISCVState *env, bool is_m_mode, bool
> > enable);
> > void riscv_timer_init(RISCVCPU *cpu);
> >
> > #endif
> > --
> > 2.17.1
> >
> >
