On Wed, Mar 13, 2024 at 03:50:16PM +0530, Himanshu Chauhan wrote: > On Wed, Mar 13, 2024 at 3:24 PM Andrew Jones <ajo...@ventanamicro.com> > wrote: > > > On Wed, Mar 13, 2024 at 11:39:30AM +0530, Himanshu Chauhan wrote: > > > This patch adds "sdtrig" in the ISA string when sdtrig extension is > > enabled. > > > The sdtrig extension may or may not be implemented in a system. > > Therefore, the > > > -cpu rv64,sdtrig=<true/false> > > > option can be used to dynamically turn sdtrig extension on or off. > > > > > > Since, the sdtrig ISA extension is a superset of debug specification, > > disable > > > the debug property when sdtrig is enabled. A warning is printed when > > this is > > > done. > > > > > > By default, the sdtrig extension is disabled and debug property enabled > > as usual. > > > > > > Signed-off-by: Himanshu Chauhan <hchau...@ventanamicro.com> > > > --- > > > target/riscv/cpu.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > index 2602aae9f5..ab057a0926 100644 > > > --- a/target/riscv/cpu.c > > > +++ b/target/riscv/cpu.c > > > @@ -175,6 +175,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, ext_sdtrig), > > > ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia), > > > ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp), > > > ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen), > > > @@ -1008,6 +1009,12 @@ static void riscv_cpu_reset_hold(Object *obj) > > > set_default_nan_mode(1, &env->fp_status); > > > > > > #ifndef CONFIG_USER_ONLY > > > + if (cpu->cfg.debug && cpu->cfg.ext_sdtrig) { > > > + warn_report("Disabling debug property since sdtrig ISA > > extension " > > > + "is enabled"); > > > + cpu->cfg.debug = 0; > > > > If sdtrig is a superset of debug, then why do we need to disable debug? > > > > "debug" is not disabled. The flag is turned off. This is for unambiguity > between which spec is in force currently. > May come handy (not now but later) in if conditions.
I know sdtrig/debug functionality remains enabled, but why state that the 'debug' functionality is no longer enabled? If sdtrig is a superset, then, when it's enabled, both the debug functionality and the sdtrig functionality are enabled. Actually, sdtrig should do the opposite, it should ensure debug=true. The warning would look odd to users that know this and the debug=off would also look odd in the results of cpu model expansion. Keeping debug=true would also avoid needing to change all the if cpu->cfg.debug conditions to if cpu->cfg.debug || cpu->cfg.ext_sdtrig. If we deprecate 'debug' someday, then we'll add a warning for when there is 'debug' explicitly enabled by a user and also for 'debug' configs which lack 'sdtrig', but we don't need to worry about that now. Thanks, drew