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

[Andrew Jones]

On Wed, Mar 13, 2024 at 05:48:16PM +0530, Himanshu Chauhan wrote:
...
> >>>> #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?
> 
> Got it. The warning can be removed.
> 
> > 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
> 
> I would disagree to set debug=true when sdtrig is selected. These are two 
> different specifications and should be treated so. Sdtrig is a superset now 
> but may have another revision not backward compatible. For two different 
> specifications and flags should mirror the same. On the same lines, this 
> patch (as it does) should keep (cfg->debug || cfg->sdtrig) which shows that 
> you are dealing with two different specifications. They behave same for some 
> triggers but are still different. Deprecation isn’t a problem now or later.

sdtrig is frozen, otherwise it would require the 'x-' prefix, so it can
no longer change in a substantial way and therefore if it's a superset of
debug now then it always will be. If a change is made to "debug
functionality" then it will get a new extension name which may not be
compatible with either 'debug' or 'sdtrig', however that's not the case
here. If a platform supports 'sdtrig', then it supports 'debug', as
'sdtrig' is a superset of 'debug'. Pretending like they're mutually
exclusive doesn't make sense when they completely overlap. Indeed
platform's will likely *want* to advertise that they are compatible with
both because software that is only compatible with 'debug' will need to
know if it will work or not. A platform that says it's not compatible
with 'debug', when it is, because it supports sdtrig, is limiting the
software that will run on it for no reason.

Thanks,
drew

> 
> > 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
> 
>