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

[Himanshu Chauhan]

> On 13-Mar-2024, at 6:19 PM, Andrew Jones <ajones@ventanamicro.com> wrote:
> 
> 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.

Got it. I will make the necessary changes.

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