Hi Daniel,
On Mon, Feb 02, 2026 at 02:48:11PM -0300, Daniel Henrique Barboza wrote:
>
>
> On 1/30/2026 3:00 AM, Chao Liu wrote:
> > RISC-V Debug Specification:
> > https://github.com/riscv/riscv-debug-spec/releases/tag/1.0
> >
> > Allow mcontrol/mcontrol6 action=1 when Sdext is enabled. When such a
> > trigger hits, enter Debug Mode with cause=trigger and stop with
> > EXCP_DEBUG.
> >
> > Also report inst-count triggers in tinfo and read their action field.
> >
> > Signed-off-by: Chao Liu <[email protected]>
> > ---
> > target/riscv/debug.c | 53 ++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> > index 5877a60c50..4e30d42905 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -110,6 +110,8 @@ static trigger_action_t
> > get_trigger_action(CPURISCVState *env,
> > action = (tdata1 & TYPE6_ACTION) >> 12;
> > break;
> > case TRIGGER_TYPE_INST_CNT:
> > + action = tdata1 & ITRIGGER_ACTION;
> > + break;
> > case TRIGGER_TYPE_INT:
> > case TRIGGER_TYPE_EXCP:
> > case TRIGGER_TYPE_EXT_SRC:
> > @@ -280,6 +282,7 @@ static target_ulong textra_validate(CPURISCVState *env,
> > target_ulong tdata3)
> > static void do_trigger_action(CPURISCVState *env, target_ulong
> > trigger_index)
> > {
> > + CPUState *cs = env_cpu(env);
> > trigger_action_t action = get_trigger_action(env, trigger_index);
> > switch (action) {
> > @@ -289,6 +292,21 @@ static void do_trigger_action(CPURISCVState *env,
> > target_ulong trigger_index)
> > riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
> > break;
> > case DBG_ACTION_DBG_MODE:
> > + if (!env_archcpu(env)->cfg.ext_sdext) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "trigger action=debug mode requires Sdext\n");
>
>
> I believe we want LOG_GUEST_ERROR for invalid actions like this one. You can
> check examples in trigger_priv_match and other places in debug.c
>
> LOG_UNIMP is reserved for cases where QEMU does not implement something at
> all and we want to warn the user about it. For instance, again in
> debug.c:trigger_priv_match():
>
>
> case TRIGGER_TYPE_INT:
> case TRIGGER_TYPE_EXCP:
> case TRIGGER_TYPE_EXT_SRC:
> qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> type);
> break;
>
>
> This is the intended use for LOG_UNIMP.
>
Thank you for the review and the clarification on the logging macros.
I will replace the sdext-related LOG_UNIMP macros with LOG_GUEST_ERROR for
this case. Will send v5 with this fix.
Thanks,
Chao
> Everything else LGTM.
>
> Thanks,
> Daniel
>
>
>
> > + riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
> > + }
> > + riscv_cpu_enter_debug_mode(env, env->pc, DCSR_CAUSE_TRIGGER);
> > + /*
> > + * If this came from the Trigger Module's CPU
> > breakpoint/watchpoint,
> > + * we're already returning via EXCP_DEBUG. Otherwise, stop now.
> > + */
> > + if (cs->exception_index != EXCP_DEBUG) {
> > + cs->exception_index = EXCP_DEBUG;
> > + cpu_loop_exit_restore(cs, GETPC());
> > + }
> > + break;
> > case DBG_ACTION_TRACE0:
> > case DBG_ACTION_TRACE1:
> > case DBG_ACTION_TRACE2:
> > @@ -441,6 +459,7 @@ static target_ulong
> > type2_mcontrol_validate(CPURISCVState *env,
> > {
> > target_ulong val;
> > uint32_t size;
> > + uint32_t action;
> > /* validate the generic part first */
> > val = tdata1_validate(env, ctrl, TRIGGER_TYPE_AD_MATCH);
> > @@ -448,11 +467,25 @@ static target_ulong
> > type2_mcontrol_validate(CPURISCVState *env,
> > /* validate unimplemented (always zero) bits */
> > warn_always_zero_bit(ctrl, TYPE2_MATCH, "match");
> > warn_always_zero_bit(ctrl, TYPE2_CHAIN, "chain");
> > - warn_always_zero_bit(ctrl, TYPE2_ACTION, "action");
> > warn_always_zero_bit(ctrl, TYPE2_TIMING, "timing");
> > warn_always_zero_bit(ctrl, TYPE2_SELECT, "select");
> > warn_always_zero_bit(ctrl, TYPE2_HIT, "hit");
> > + action = (ctrl & TYPE2_ACTION) >> 12;
> > + if (action == DBG_ACTION_BP) {
> > + val |= ctrl & TYPE2_ACTION;
> > + } else if (action == DBG_ACTION_DBG_MODE) {
> > + if (env_archcpu(env)->cfg.ext_sdext) {
> > + val |= ctrl & TYPE2_ACTION;
> > + } else {
> > + qemu_log_mask(LOG_UNIMP,
> > + "trigger action=debug mode requires Sdext\n");
> > + }
> > + } else {
> > + qemu_log_mask(LOG_UNIMP, "trigger action: %u is not supported\n",
> > + action);
> > + }
> > +
> > /* validate size encoding */
> > size = type2_breakpoint_size(env, ctrl);
> > if (access_size[size] == -1) {
> > @@ -569,6 +602,7 @@ static target_ulong
> > type6_mcontrol6_validate(CPURISCVState *env,
> > {
> > target_ulong val;
> > uint32_t size;
> > + uint32_t action;
> > /* validate the generic part first */
> > val = tdata1_validate(env, ctrl, TRIGGER_TYPE_AD_MATCH6);
> > @@ -576,11 +610,25 @@ static target_ulong
> > type6_mcontrol6_validate(CPURISCVState *env,
> > /* validate unimplemented (always zero) bits */
> > warn_always_zero_bit(ctrl, TYPE6_MATCH, "match");
> > warn_always_zero_bit(ctrl, TYPE6_CHAIN, "chain");
> > - warn_always_zero_bit(ctrl, TYPE6_ACTION, "action");
> > warn_always_zero_bit(ctrl, TYPE6_TIMING, "timing");
> > warn_always_zero_bit(ctrl, TYPE6_SELECT, "select");
> > warn_always_zero_bit(ctrl, TYPE6_HIT, "hit");
> > + action = (ctrl & TYPE6_ACTION) >> 12;
> > + if (action == DBG_ACTION_BP) {
> > + val |= ctrl & TYPE6_ACTION;
> > + } else if (action == DBG_ACTION_DBG_MODE) {
> > + if (env_archcpu(env)->cfg.ext_sdext) {
> > + val |= ctrl & TYPE6_ACTION;
> > + } else {
> > + qemu_log_mask(LOG_UNIMP,
> > + "trigger action=debug mode requires Sdext\n");
> > + }
> > + } else {
> > + qemu_log_mask(LOG_UNIMP, "trigger action: %u is not supported\n",
> > + action);
> > + }
> > +
> > /* validate size encoding */
> > size = extract32(ctrl, 16, 4);
> > if (access_size[size] == -1) {
> > @@ -919,6 +967,7 @@ target_ulong tinfo_csr_read(CPURISCVState *env)
> > {
> > /* assume all triggers support the same types of triggers */
> > return BIT(TRIGGER_TYPE_AD_MATCH) |
> > + BIT(TRIGGER_TYPE_INST_CNT) |
> > BIT(TRIGGER_TYPE_AD_MATCH6);
> > }
>
