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",
--
2.49.0
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.
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",
2025-06-23T18:39:02-03:00, Daniel Henrique Barboza <dbarboza@ventanamicro.com>:
> Hi Radim,
>
> It seems like this patch is breaking 'make check-functional':
That is a nice command to know of, thanks!
> 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.
Uff, mtimer is NULL on spike:
0x0000555555c46618 in riscv_timer_write_timecmp (env=env@entry=0x555556888270, timer=0x5555568a61e0, timecmp=0, delta=delta@entry=0, timer_irq=timer_irq@entry=32) at ../target/riscv/time_helper.c:49
49 uint32_t timebase_freq = mtimer->timebase_freq;
(gdb) bt
#0 0x0000555555c46618 in riscv_timer_write_timecmp (env=env@entry=0x555556888270, timer=0x5555568a61e0, timecmp=0, delta=delta@entry=0, timer_irq=timer_irq@entry=32) at ../target/riscv/time_helper.c:49
#1 0x0000555555c6eb9e in write_menvcfg (env=0x555556888270, csrno=<optimized out>, val=<optimized out>, ra=140736012591329) at ../target/riscv/csr.c:3214
#2 0x0000555555c6a181 in riscv_csrrw_do64 (env=env@entry=0x555556888270, csrno=<optimized out>, ret_value=ret_value@entry=0x0, new_value=<optimized out>, write_mask=<optimized out>, ra=140736012591329) at ../target/riscv/csr.c:5579
[...]
(gdb) p mtimer
$1 = (RISCVAclintMTimerState *) 0x0
(gdb) p timer
$2 = (QEMUTimer *) 0x5555568a61e0
(gdb) p *timer
$3 = {expire_time = -1, timer_list = 0x55555666b840, cb = 0x555555c465d0 <riscv_stimer_cb>, opaque = 0x5555568856b0, next = 0x0, attributes = 0, scale = 1}
I'll try to figure out is going on, but `make check-functional` passes
with this hack:
diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
index 81a6a6394502..a2092206cb20 100644
--- a/target/riscv/time_helper.c
+++ b/target/riscv/time_helper.c
@@ -44,10 +44,8 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
uint64_t timecmp, uint64_t delta,
uint32_t timer_irq)
{
- uint64_t diff, ns_diff, next;
+ uint64_t diff, ns_diff, next, timebase_freq, rtc_r;
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;
/*
* *envcfg.STCE disables *stimecmp interrupts, but still allows higher
@@ -59,6 +57,13 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
return;
}
+ if (!mtimer) {
+ return;
+ }
+
+ timebase_freq = mtimer->timebase_freq;
+ rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
+
if (timecmp <= rtc_r) {
/*
* If we're setting an stimecmp value in the "past",
Hi Radim,
Does your patchset want to resolve the same issue as my sstc patch [1]?
My sstc patchset has been merged to "riscv-to-apply.next".
Maybe you can review it or check if there is still any issue in the
"riscv-to-apply.next" branch, thanks!
[1] "[PATCH v3 4/4] target/riscv: Enable/Disable S/VS-mode Timer when
STCE bit is changed"
https://patchew.org/QEMU/20250519143518.11086-1-jim.shu@sifive.com/20250519143518.11086-5-jim.shu@sifive.com/
Thanks,
Jim
On Tue, Jun 24, 2025 at 9:49 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-06-23T18:39:02-03:00, Daniel Henrique Barboza <dbarboza@ventanamicro.com>:
> > Hi Radim,
> >
> > It seems like this patch is breaking 'make check-functional':
>
> That is a nice command to know of, thanks!
>
> > 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.
>
> Uff, mtimer is NULL on spike:
>
> 0x0000555555c46618 in riscv_timer_write_timecmp (env=env@entry=0x555556888270, timer=0x5555568a61e0, timecmp=0, delta=delta@entry=0, timer_irq=timer_irq@entry=32) at ../target/riscv/time_helper.c:49
> 49 uint32_t timebase_freq = mtimer->timebase_freq;
> (gdb) bt
> #0 0x0000555555c46618 in riscv_timer_write_timecmp (env=env@entry=0x555556888270, timer=0x5555568a61e0, timecmp=0, delta=delta@entry=0, timer_irq=timer_irq@entry=32) at ../target/riscv/time_helper.c:49
> #1 0x0000555555c6eb9e in write_menvcfg (env=0x555556888270, csrno=<optimized out>, val=<optimized out>, ra=140736012591329) at ../target/riscv/csr.c:3214
> #2 0x0000555555c6a181 in riscv_csrrw_do64 (env=env@entry=0x555556888270, csrno=<optimized out>, ret_value=ret_value@entry=0x0, new_value=<optimized out>, write_mask=<optimized out>, ra=140736012591329) at ../target/riscv/csr.c:5579
> [...]
> (gdb) p mtimer
> $1 = (RISCVAclintMTimerState *) 0x0
> (gdb) p timer
> $2 = (QEMUTimer *) 0x5555568a61e0
> (gdb) p *timer
> $3 = {expire_time = -1, timer_list = 0x55555666b840, cb = 0x555555c465d0 <riscv_stimer_cb>, opaque = 0x5555568856b0, next = 0x0, attributes = 0, scale = 1}
>
> I'll try to figure out is going on, but `make check-functional` passes
> with this hack:
>
> diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> index 81a6a6394502..a2092206cb20 100644
> --- a/target/riscv/time_helper.c
> +++ b/target/riscv/time_helper.c
> @@ -44,10 +44,8 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
> uint64_t timecmp, uint64_t delta,
> uint32_t timer_irq)
> {
> - uint64_t diff, ns_diff, next;
> + uint64_t diff, ns_diff, next, timebase_freq, rtc_r;
> 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;
>
> /*
> * *envcfg.STCE disables *stimecmp interrupts, but still allows higher
> @@ -59,6 +57,13 @@ void riscv_timer_write_timecmp(CPURISCVState *env, QEMUTimer *timer,
> return;
> }
>
> + if (!mtimer) {
> + return;
> + }
> +
> + timebase_freq = mtimer->timebase_freq;
> + rtc_r = env->rdtime_fn(env->rdtime_fn_arg) + delta;
> +
> if (timecmp <= rtc_r) {
> /*
> * If we're setting an stimecmp value in the "past",
>
2025-06-25T14:24:25+08:00, Jim Shu <jim.shu@sifive.com>: > Hi Radim, > > Does your patchset want to resolve the same issue as my sstc patch [1]? > My sstc patchset has been merged to "riscv-to-apply.next". Yeah, I wasn't aware where patches go in the QEMU land. > Maybe you can review it or check if there is still any issue in the > "riscv-to-apply.next" branch, thanks! My patch is doing exactly the same as [1/4] and [3/4] in your series. I see you also fixed the NULL dereference. :) I'm withdrawing this series, thanks.
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",
>
>
© 2016 - 2025 Red Hat, Inc.