Re: [RFC PATCH 05/25] target/riscv/debug: Update itrigger_enabled after changing privilege

[Daniel Henrique Barboza]

On 1/14/2026 1:46 AM, Nicholas Piggin wrote:

The !icount_enabled() path should update itrigger_enabled after changing
privilege, which can change whether or not an icount trigger is enabled.

Signed-off-by: Nicholas Piggin <npig...@gmail.com>
---
  target/riscv/cpu_helper.c | 7 +++++++
  target/riscv/debug.c      | 7 +++++++
  target/riscv/debug.h      | 3 +++
  3 files changed, 17 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index dd6c861a90..e096da939b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1030,9 +1030,12 @@ void riscv_ctr_add_entry(CPURISCVState *env, target_long 
src, target_long dst,

void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv, bool virt_en)

  {
+    bool change = false;
+
      g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);

if (newpriv != env->priv || env->virt_enabled != virt_en) {

+        change = true;
          if (icount_enabled()) {
              riscv_itrigger_update_priv(env);
          }
@@ -1075,6 +1078,10 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong 
newpriv, bool virt_en)
              riscv_cpu_update_mip(env, 0, 0);
          }
      }
+
+    if (change) {
+        riscv_cpu_debug_change_priv(env);
+    }

Is there a particular reason to do this change at the end of cpu_set_mode()?
Because if there isn't, we can remove the 'change' flag, then just do a:

>       if (newpriv != env->priv || env->virt_enabled != virt_en) {
> +        if (!icount_enabled()) {
> +            env->itrigger_enabled = riscv_itrigger_enabled(env);
> +        }
>           if (icount_enabled()) {
>               riscv_itrigger_update_priv(env);
>           }



You'll remove the icount_enabled() branch in patch 16 anyway so I wouldn't 
bother
do a if/then/else with icount_enabled(), but can be done too.


This will also get rid of the riscv_cpu_debug_change_priv() function added here.
Which is fine: from what I can see in the patches this is the only place that
it would be used, so no harm done.


Thanks,
Daniel

  }

/*

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 2effbb49af..51c5934bfa 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -1090,6 +1090,13 @@ void riscv_trigger_realize(CPURISCVState *env)
      }
  }

+void riscv_cpu_debug_change_priv(CPURISCVState *env)

+{
+    if (!icount_enabled()) {
+        env->itrigger_enabled = riscv_itrigger_enabled(env);
+    }
+}
+
  void riscv_trigger_reset_hold(CPURISCVState *env)
  {
      target_ulong tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH, 0, 0);
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index f76b8f944a..5a14b7894e 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -153,4 +153,7 @@ void riscv_trigger_reset_hold(CPURISCVState *env);

bool riscv_itrigger_enabled(CPURISCVState *env);

  void riscv_itrigger_update_priv(CPURISCVState *env);
+
+void riscv_cpu_debug_change_priv(CPURISCVState *env);
+
  #endif /* RISCV_DEBUG_H */