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

[Himanshu Chauhan]

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

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.

> 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