Re: [PATCH v4 2/3] target/riscv: Expose sdtrig ISA extension

[Andrew Jones]

On Wed, Mar 13, 2024 at 03:50:16PM +0530, Himanshu Chauhan wrote:
> On Wed, Mar 13, 2024 at 3:24 PM Andrew Jones <ajones@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 <hchauhan@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