[PATCH 0/4] Several sstc extension fixes

[PATCH 0/4] Several sstc extension fixes

[Jim Shu]

This patch series contains several sstc fixes:

(1) Writing to ACLINT mtime should also update the period of S/VS-mode
    timer, just like M-mode timer.
(2) VSTIP bit of $mip CSR should check both M-mode and H-mode STCE.
(3) Writing to STCE bit may enable/disable sstc extension in S/VS-mode,
    which should update the timer and IRQ pending bits.

Jim Shu (4):
  target/riscv: Add the checking into stimecmp write function.
  hw/intc: riscv_aclint: Fix mtime write for sstc extension
  target/riscv: Fix VSTIP bit in sstc extension.
  target/riscv: Enable/Disable S/VS-mode Timer when STCE bit is changed

 hw/intc/riscv_aclint.c     |  5 +++
 target/riscv/csr.c         | 53 ++++++++++++++++++++++++++++-
 target/riscv/time_helper.c | 70 ++++++++++++++++++++++++++++++++++++--
 target/riscv/time_helper.h |  1 +
 4 files changed, 126 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/4] target/riscv: Add the checking into stimecmp write function.

[Jim Shu]

Preparation commit to let aclint timer to use stimecmp write function.
Aclint timer doesn't call sstc() predicate so we need to check inside
the stimecmp write function.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/time_helper.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
index bc0d9a0c4c..aebf0798d0 100644
--- a/target/riscv/time_helper.c
+++ b/target/riscv/time_helper.c
@@ -46,8 +46,23 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
 {
     uint64_t diff, ns_diff, next;
     RISCVAclintMTimerState *mtimer = env->rdtime_fn_arg;
-    uint32_t timebase_freq = mtimer->timebase_freq;
-    uint64_t rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
+    uint32_t timebase_freq;
+    uint64_t rtc_r;
+
+    if (!riscv_cpu_cfg(env)->ext_sstc || !env->rdtime_fn ||
+        !env->rdtime_fn_arg || !get_field(env->menvcfg, MENVCFG_STCE)) {
+        /* S/VS Timer IRQ depends on sstc extension, rdtime_fn(), and STCE. */
+        return;
+    }
+
+    if (timer_irq == MIP_VSTIP &&
+        (!riscv_has_ext(env, RVH) || !get_field(env->henvcfg, HENVCFG_STCE))) {
+        /* VS Timer IRQ also depends on RVH and henvcfg.STCE. */
+        return;
+    }
+
+    timebase_freq = mtimer->timebase_freq;
+    rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
 
     if (timecmp <= rtc_r) {
         /*
-- 
2.17.1

[PATCH 2/4] hw/intc: riscv_aclint: Fix mtime write for sstc extension

[Jim Shu]

When changing the mtime value, the period of [s|vs]timecmp timers
should also be updated like the period of mtimecmp timer.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/intc/riscv_aclint.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index db374a7c2d..5f4a17e177 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -28,6 +28,7 @@
 #include "qemu/module.h"
 #include "hw/sysbus.h"
 #include "target/riscv/cpu.h"
+#include "target/riscv/time_helper.h"
 #include "hw/qdev-properties.h"
 #include "hw/intc/riscv_aclint.h"
 #include "qemu/timer.h"
@@ -240,6 +241,10 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
             riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
                                               mtimer->hartid_base + i,
                                               mtimer->timecmp[i]);
+            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);
+
         }
         return;
     }
-- 
2.17.1

[PATCH 3/4] target/riscv: Fix VSTIP bit in sstc extension.

[Jim Shu]

VSTIP is only writable when both [mh]envcfg.STCE is enabled, or it will
revert it's defined behavior as if sstc extension is not implemented.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 target/riscv/csr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 49566d3c08..ba026dfc8e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3630,7 +3630,14 @@ static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
     if (riscv_cpu_cfg(env)->ext_sstc && (env->priv == PRV_M) &&
         get_field(env->menvcfg, MENVCFG_STCE)) {
         /* sstc extension forbids STIP & VSTIP to be writeable in mip */
-        mask = mask & ~(MIP_STIP | MIP_VSTIP);
+
+        /* STIP is not writable when menvcfg.STCE is enabled. */
+        mask = mask & ~MIP_STIP;
+
+        /* VSTIP is not writable when both [mh]envcfg.STCE are enabled. */
+        if (get_field(env->henvcfg, HENVCFG_STCE)) {
+            mask = mask & ~MIP_VSTIP;
+        }
     }
 
     if (mask) {
-- 
2.17.1

[PATCH 4/4] target/riscv: Enable/Disable S/VS-mode Timer when STCE bit is changed

[Jim Shu]

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.shu@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);
+        } else {
+            riscv_timer_disable_timecmp(env, env->stimer, MIP_STIP);
+            riscv_timer_disable_timecmp(env, env->vstimer, MIP_VSTIP);
+        }
+    } 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);
+        }
+    }
+}
+
 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

Re: [PATCH 0/4] Several sstc extension fixes

[Jim Shu]

Hi,

Gentle ping on this patch.

Thanks,
Jim Shu


On Thu, Mar 20, 2025 at 3:22 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> This patch series contains several sstc fixes:
>
> (1) Writing to ACLINT mtime should also update the period of S/VS-mode
>     timer, just like M-mode timer.
> (2) VSTIP bit of $mip CSR should check both M-mode and H-mode STCE.
> (3) Writing to STCE bit may enable/disable sstc extension in S/VS-mode,
>     which should update the timer and IRQ pending bits.
>
> Jim Shu (4):
>   target/riscv: Add the checking into stimecmp write function.
>   hw/intc: riscv_aclint: Fix mtime write for sstc extension
>   target/riscv: Fix VSTIP bit in sstc extension.
>   target/riscv: Enable/Disable S/VS-mode Timer when STCE bit is changed
>
>  hw/intc/riscv_aclint.c     |  5 +++
>  target/riscv/csr.c         | 53 ++++++++++++++++++++++++++++-
>  target/riscv/time_helper.c | 70 ++++++++++++++++++++++++++++++++++++--
>  target/riscv/time_helper.h |  1 +
>  4 files changed, 126 insertions(+), 3 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 1/4] target/riscv: Add the checking into stimecmp write function.

[Alistair Francis]

On Thu, Mar 20, 2025 at 5:22 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Preparation commit to let aclint timer to use stimecmp write function.
> Aclint timer doesn't call sstc() predicate so we need to check inside
> the stimecmp write function.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>

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

Alistair

> ---
>  target/riscv/time_helper.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> index bc0d9a0c4c..aebf0798d0 100644
> --- a/target/riscv/time_helper.c
> +++ b/target/riscv/time_helper.c
> @@ -46,8 +46,23 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
>  {
>      uint64_t diff, ns_diff, next;
>      RISCVAclintMTimerState *mtimer = env->rdtime_fn_arg;
> -    uint32_t timebase_freq = mtimer->timebase_freq;
> -    uint64_t rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
> +    uint32_t timebase_freq;
> +    uint64_t rtc_r;
> +
> +    if (!riscv_cpu_cfg(env)->ext_sstc || !env->rdtime_fn ||
> +        !env->rdtime_fn_arg || !get_field(env->menvcfg, MENVCFG_STCE)) {
> +        /* S/VS Timer IRQ depends on sstc extension, rdtime_fn(), and STCE. */
> +        return;
> +    }
> +
> +    if (timer_irq == MIP_VSTIP &&
> +        (!riscv_has_ext(env, RVH) || !get_field(env->henvcfg, HENVCFG_STCE))) {
> +        /* VS Timer IRQ also depends on RVH and henvcfg.STCE. */
> +        return;
> +    }
> +
> +    timebase_freq = mtimer->timebase_freq;
> +    rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
>
>      if (timecmp <= rtc_r) {
>          /*
> --
> 2.17.1
>
>

Re: [PATCH 2/4] hw/intc: riscv_aclint: Fix mtime write for sstc extension

[Alistair Francis]

On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> When changing the mtime value, the period of [s|vs]timecmp timers
> should also be updated like the period of mtimecmp timer.

Why should they be updated?

Alistair

>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/intc/riscv_aclint.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index db374a7c2d..5f4a17e177 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -28,6 +28,7 @@
>  #include "qemu/module.h"
>  #include "hw/sysbus.h"
>  #include "target/riscv/cpu.h"
> +#include "target/riscv/time_helper.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/intc/riscv_aclint.h"
>  #include "qemu/timer.h"
> @@ -240,6 +241,10 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>              riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
>                                                mtimer->hartid_base + i,
>                                                mtimer->timecmp[i]);
> +            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);
> +
>          }
>          return;
>      }
> --
> 2.17.1
>
>

Re: [PATCH 3/4] target/riscv: Fix VSTIP bit in sstc extension.

[Alistair Francis]

On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> VSTIP is only writable when both [mh]envcfg.STCE is enabled, or it will
> revert it's defined behavior as if sstc extension is not implemented.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>

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

Alistair

> ---
>  target/riscv/csr.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 49566d3c08..ba026dfc8e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3630,7 +3630,14 @@ static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
>      if (riscv_cpu_cfg(env)->ext_sstc && (env->priv == PRV_M) &&
>          get_field(env->menvcfg, MENVCFG_STCE)) {
>          /* sstc extension forbids STIP & VSTIP to be writeable in mip */
> -        mask = mask & ~(MIP_STIP | MIP_VSTIP);
> +
> +        /* STIP is not writable when menvcfg.STCE is enabled. */
> +        mask = mask & ~MIP_STIP;
> +
> +        /* VSTIP is not writable when both [mh]envcfg.STCE are enabled. */
> +        if (get_field(env->henvcfg, HENVCFG_STCE)) {
> +            mask = mask & ~MIP_VSTIP;
> +        }
>      }
>
>      if (mask) {
> --
> 2.17.1
>
>

Re: [PATCH 4/4] target/riscv: Enable/Disable S/VS-mode Timer when STCE bit is changed

[Alistair Francis]

On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim.shu@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.shu@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
>
>

Re: [PATCH 4/4] target/riscv: Enable/Disable S/VS-mode Timer when STCE bit is changed

[Jim Shu]

OK, I will fix it in the v2 patchset.


Jim Shu

On Fri, Apr 4, 2025 at 2:03 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim.shu@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.shu@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
> >
> >

Re: [PATCH 2/4] hw/intc: riscv_aclint: Fix mtime write for sstc extension

[Jim Shu]

The period of the stimecmp timer is the time until the next S-mode
timer IRQ. The value is calculated as "stimecmp - time". [1]
It is equal to "stimecmp - mtime" since the time CSR is a read-only
shadow of the memory-mapped mtime register.
Thus, changing mtime value will update the period of stimecmp timer.

Similarly, the period of vstimecmp timer is calculated as "vstimecmp -
(mtime + htimedelta)" [2], so changing mtime value will update the
period of vstimecmp timer.

[1] RISC-V Priv spec ch 9.1.1. Supervisor Timer (stimecmp) Register
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.
[2] RISC-V Priv spec ch19.2.1. Virtual Supervisor Timer (vstimecmp) Register
A virtual supervisor timer interrupt becomes pending, as reflected in
the VSTIP bit in the hip register, whenever (time + htimedelta),
truncated to 64 bits, contains a value greater than or equal to
vstimecmp

I will add the explaination in the commit log in the v2 patchset, thanks.


Jim Shu

On Fri, Apr 4, 2025 at 11:12 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 5:24 AM Jim Shu <jim.shu@sifive.com> wrote:
> >
> > When changing the mtime value, the period of [s|vs]timecmp timers
> > should also be updated like the period of mtimecmp timer.
>
> Why should they be updated?
>
> Alistair
>
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  hw/intc/riscv_aclint.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> > index db374a7c2d..5f4a17e177 100644
> > --- a/hw/intc/riscv_aclint.c
> > +++ b/hw/intc/riscv_aclint.c
> > @@ -28,6 +28,7 @@
> >  #include "qemu/module.h"
> >  #include "hw/sysbus.h"
> >  #include "target/riscv/cpu.h"
> > +#include "target/riscv/time_helper.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/intc/riscv_aclint.h"
> >  #include "qemu/timer.h"
> > @@ -240,6 +241,10 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
> >              riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
> >                                                mtimer->hartid_base + i,
> >                                                mtimer->timecmp[i]);
> > +            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);
> > +
> >          }
> >          return;
> >      }
> > --
> > 2.17.1
> >
> >