Re: [PATCH 1/5] target/riscv: split 'debug' and 'sdtrig' flags

On Wed, 2026-08-05 at 15:25 -0300, Daniel Henrique Barboza wrote:
> We need to support modern debug extensions that happens to be
> dependencies of other things that we want to support (e.g. a riscv
> server spec board).  These extensions, namely sdext, depends on Debug
> 1.0.  At this moment we support Debug 0.13, and so it happens that
> 0.13
> and 1.0 aren't backwards compatible, i.e. we need design changes to
> support both.
> 
> The easier way is to deprecate the Debug 0.13 support, together with
> its
> legacy 'debug' flag and call it a day.  But there's a demand to keep
> the
> 0.13 support around and this idea got scrapped.
> 
> We're going to support both 0.13 and 1.0, where the 'debug' flag will
> refer to 0.13 and 'sdtrig' to 1.0.  We need to split both in distinct
> flags first to handle the backend changes, so:
> 
> - add a new cpu->cfg.ext_sdtrig flag;
> - all code that checks "if debug enabled" now checks for both debug
> and
>   ext_sdtrig;
> - amend riscv_isa_string_ext() to keep adding 'sdtrig' in riscv,isa
> if
>   we have just the 'debug' flag enabled.
> 
> Signed-off-by: Daniel Henrique Barboza
> <[email protected]>

Reviewed-by: Alistair Francis <[email protected]>

Alistair

> ---
>  target/riscv/cpu.c                | 21 ++++++++++++++++-----
>  target/riscv/cpu_cfg_fields.h.inc |  1 +
>  target/riscv/machine.c            |  2 +-
>  target/riscv/tcg/csr.c            |  2 +-
>  target/riscv/tcg/tcg-cpu.c        |  3 ++-
>  5 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5a82e6563b..db4f2b6459 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -247,7 +247,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(zvkt, PRIV_VERSION_1_12_0, ext_zvkt),
>      ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
>      ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
> -    ISA_EXT_DATA_ENTRY(sdtrig, PRIV_VERSION_1_12_0, debug),
> +    ISA_EXT_DATA_ENTRY(sdtrig, PRIV_VERSION_1_12_0, ext_sdtrig),
>      ISA_INTERNAL_EXT_DATA_ENTRY(shcounterenw, PRIV_VERSION_1_12_0,
>                                  has_priv_1_12),
>      ISA_INTERNAL_EXT_DATA_ENTRY(sha, PRIV_VERSION_1_12_0, ext_sha),
> @@ -1079,7 +1079,7 @@ static void riscv_cpu_reset_hold(Object *obj,
> ResetType type)
>  
>  #ifndef CONFIG_USER_ONLY
>  #ifdef CONFIG_TCG
> -    if (cpu->cfg.debug) {
> +    if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_reset_hold(env);
>      }
>  #endif
> @@ -1241,7 +1241,7 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
>      riscv_cpu_register_gdb_regs_for_features(cs);
>  
>  #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> -    if (cpu->cfg.debug) {
> +    if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_realize(&cpu->env);
>      }
>  #endif
> @@ -1258,7 +1258,7 @@ static void riscv_cpu_unrealize(DeviceState
> *dev)
>  #if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>      RISCVCPU *cpu = RISCV_CPU(dev);
>  
> -    if (cpu->cfg.debug) {
> +    if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_unrealize(&cpu->env);
>      }
>  #endif
> @@ -2834,6 +2834,11 @@ RISCVCPUImpliedExtsRule
> *riscv_multi_ext_implied_rules[] = {
>  };
>  
>  static const Property riscv_cpu_properties[] = {
> +    /*
> +     * The 'debug' flag enables support for the legacy Debug
> +     * 0.13 spec.  In case cpu->ext.ext_sdtrig is also enabled
> +     * the CPU will enable Debug 1.0 instead.
> +     */
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>      DEFINE_PROP_BOOL("big-endian", RISCVCPU, cfg.big_endian, false),
>  
> @@ -3055,7 +3060,13 @@ static void riscv_isa_string_ext(RISCVCPU
> *cpu, char **isa_str,
>      char *new = *isa_str;
>  
>      for (edata = isa_edata_arr; edata && edata->name; edata++) {
> -        if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)) {
> +        if (isa_ext_is_enabled(cpu, edata->ext_enable_offset)
> +            /*
> +             * We've been adding 'sdtrig' in riscv,isa for
> +             * Debug 0.13 for awhile.  Until we decide to
> +             * move away from it we'll keep doing it.
> +             */
> +            || (!g_strcmp0(edata->name, "sdtrig") && cpu-
> >cfg.debug)) {
>              new = g_strconcat(old, "_", edata->name, NULL);
>              g_free(old);
>              old = new;
> diff --git a/target/riscv/cpu_cfg_fields.h.inc
> b/target/riscv/cpu_cfg_fields.h.inc
> index 9eb47af0a7..e32af40c90 100644
> --- a/target/riscv/cpu_cfg_fields.h.inc
> +++ b/target/riscv/cpu_cfg_fields.h.inc
> @@ -105,6 +105,7 @@ BOOL_FIELD(ext_zvfbfmin)
>  BOOL_FIELD(ext_zvfbfwma)
>  BOOL_FIELD(ext_zvfh)
>  BOOL_FIELD(ext_zvfhmin)
> +BOOL_FIELD(ext_sdtrig)
>  BOOL_FIELD(ext_smaia)
>  BOOL_FIELD(ext_ssaia)
>  BOOL_FIELD(ext_smctr)
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 0ab613a298..a744462d48 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -228,7 +228,7 @@ static bool debug_needed(void *opaque)
>  {
>      RISCVCPU *cpu = opaque;
>  
> -    return cpu->cfg.debug;
> +    return cpu->cfg.debug || cpu->cfg.ext_sdtrig;
>  }
>  
>  static int debug_post_load(void *opaque, int version_id)
> diff --git a/target/riscv/tcg/csr.c b/target/riscv/tcg/csr.c
> index 36f2004bc5..850fbf32fc 100644
> --- a/target/riscv/tcg/csr.c
> +++ b/target/riscv/tcg/csr.c
> @@ -804,7 +804,7 @@ static RISCVException have_mseccfg(CPURISCVState
> *env, int csrno)
>  
>  static RISCVException debug(CPURISCVState *env, int csrno)
>  {
> -    if (riscv_cpu_cfg(env)->debug) {
> +    if (riscv_cpu_cfg(env)->debug || riscv_cpu_cfg(env)->ext_sdtrig)
> {
>          return RISCV_EXCP_NONE;
>      }
>  
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 4af5cd9c73..02bbdaef8e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -182,7 +182,8 @@ static TCGTBCPUState
> riscv_get_tb_cpu_state(CPUState *cs)
>               ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED;
>      }
>  
> -    if (cpu->cfg.debug && !icount_enabled()) {
> +    if ((cpu->cfg.debug || cpu->cfg.ext_sdtrig)
> +        && !icount_enabled()) {
>          flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env-
> >itrigger_enabled);
>      }
>  #endif