Hi Daniel: On 1/19/2026 9:19 PM, Daniel Henrique Barboza wrote: >On 1/17/2026 1:27 AM, Chao Liu wrote: >> Add cfg.ext_sdext and cfg.ext_sdtrig and expose them as ISA >> extensions. Keep the legacy 'debug' CPU property as a global kill >> switch and force-disable both when it is off.
>I would rather put the 'debug' flag in deprecation. It's a flag that at >this moment means 'enable sdtrig' and I'd rather get rid of it than >making it enable sdext too. >But deprecating 'debug' is out of scope for this work and we can handle >it later. Matter of fact I might drop patches deprecating it today. >About enabling sdext by default, the patch is breaking bios-table-test >because we're adding an extra string in riscv,isa ... Thanks for the review :) Enabling sdext by default is indeed not a good idea, especially when it is not fully implemented, so I agree to set it to disable by default until it is fully available before considering whether to enable it by default. >> >> Trigger CSRs (tselect/tdata*/tinfo/mcontext) and trigger setup now >> depend on ext_sdtrig instead of cfg.debug. >> >> Signed-off-by: Chao Liu <[email protected]> >> --- >> target/riscv/cpu.c | 18 +++++++++++++++--- >> target/riscv/cpu_cfg_fields.h.inc | 2 ++ >> target/riscv/csr.c | 16 ++++++++-------- >> target/riscv/machine.c | 4 ++-- >> target/riscv/tcg/tcg-cpu.c | 11 +---------- >> 5 files changed, 28 insertions(+), 23 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 73d4280d7c..bc0b385cc1 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -189,7 +189,8 @@ 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(sdext, PRIV_VERSION_1_12_0, ext_sdext), >> + ISA_EXT_DATA_ENTRY(sdtrig, PRIV_VERSION_1_12_0, ext_sdtrig), >> ISA_EXT_DATA_ENTRY(shcounterenw, PRIV_VERSION_1_12_0, has_priv_1_12), >> ISA_EXT_DATA_ENTRY(sha, PRIV_VERSION_1_12_0, ext_sha), >> ISA_EXT_DATA_ENTRY(shgatpa, PRIV_VERSION_1_12_0, has_priv_1_12), >> @@ -783,7 +784,7 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType >> type) >> env->vill = true; >> >> #ifndef CONFIG_USER_ONLY >> - if (cpu->cfg.debug) { >> + if (cpu->cfg.ext_sdtrig) { >> riscv_trigger_reset_hold(env); >> } >> >> @@ -922,6 +923,15 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error >> **errp) >> return; >> } >> } >> + >> + /* >> + * Keep the legacy 'debug' CPU property as a global kill switch. >> + * If it is off, force-disable Sdext/Sdtrig regardless of ISA strings. >> + */ >> + if (!cpu->cfg.debug) { >> + cpu->cfg.ext_sdext = false; >> + cpu->cfg.ext_sdtrig = false; >> + } >> } >> >> static void riscv_cpu_realize(DeviceState *dev, Error **errp) >> @@ -946,7 +956,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error >> **errp) >> riscv_cpu_register_gdb_regs_for_features(cs); >> >> #ifndef CONFIG_USER_ONLY >> - if (cpu->cfg.debug) { >> + if (cpu->cfg.ext_sdtrig) { >> riscv_trigger_realize(&cpu->env); >> } >> #endif >> @@ -1112,6 +1122,8 @@ static void riscv_cpu_init(Object *obj) >> */ >> RISCV_CPU(obj)->cfg.ext_zicntr = !mcc->def->bare; >> RISCV_CPU(obj)->cfg.ext_zihpm = !mcc->def->bare; >> + RISCV_CPU(obj)->cfg.ext_sdext = true; >> + RISCV_CPU(obj)->cfg.ext_sdtrig = true; > ^ here. Every time we change the defaults we have to dance around > bios-tables-test due to riscv,isa changes. > If we really want sdext to be enabled by default we should (1) only > enable it when the feature is already fully implemented and (2) changing > bios-tables-test according to avoid leaving broken tests. I agree, as stated above. > The remaining of the patch has the right idea, like here: >> >> /* Default values for non-bool cpu properties */ >> cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16); >> diff --git a/target/riscv/cpu_cfg_fields.h.inc >> b/target/riscv/cpu_cfg_fields.h.inc >> index a154ecdc79..9701319195 100644 >> --- a/target/riscv/cpu_cfg_fields.h.inc >> +++ b/target/riscv/cpu_cfg_fields.h.inc >> @@ -44,6 +44,8 @@ BOOL_FIELD(ext_zihpm) >> BOOL_FIELD(ext_zimop) >> BOOL_FIELD(ext_zcmop) >> BOOL_FIELD(ext_ztso) >> +BOOL_FIELD(ext_sdext) >> +BOOL_FIELD(ext_sdtrig) >> BOOL_FIELD(ext_smstateen) >> BOOL_FIELD(ext_sstc) >> BOOL_FIELD(ext_smcdeleg) >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index 5c91658c3d..4f071b1db2 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -775,9 +775,9 @@ static RISCVException have_mseccfg(CPURISCVState *env, >> int csrno) >> return RISCV_EXCP_ILLEGAL_INST; >> } >> >> -static RISCVException debug(CPURISCVState *env, int csrno) >> +static RISCVException sdtrig(CPURISCVState *env, int csrno) >> { >> - if (riscv_cpu_cfg(env)->debug) { >> + if (riscv_cpu_cfg(env)->ext_sdtrig) { >> return RISCV_EXCP_NONE; >> } > debug == sdtrig is what we want and we should use 'sdtrig' everywhere > 'debug' is appearing, but there's a particular way to go about it. We > need to add getters/setters for 'debug' that would enable/disable > sdtrig. We should avoid adding this kind of handling in init() or > finalize(). > I can do that in a deprecation patch. You can use your patches to handle > sdext only. Thanks! I agree with this plan. I will focus on the implementation of sdext in subsequent patches. If you have patches related to handling debug flags, I will rebase them to continue the work on sdext. Thanks, Chao
