[PATCH 0/2] Export debug triggers as an extension

[PATCH 0/2] Export debug triggers as an extension

[Himanshu Chauhan]

All the CPUs may or may not implement the debug trigger (sdtrig)
extension. The presence of it should be dynamically detectable.
This patch exports the debug triggers as an extension which
can be turned on or off by sdtrig=<true/false> option. It is
turned on by default.

"sdtrig" is concatenated to ISA string when it is enabled.
Like so:
rv64imafdch_zicbom_*_sdtrig_*_sstc_svadu


Himanshu Chauhan (2):
  target/riscv: Export sdtrig as an extension and ISA string
  target/riscv: Raise an exception when sdtrig is turned off

 target/riscv/cpu.c     |  2 ++
 target/riscv/cpu_cfg.h |  1 +
 target/riscv/csr.c     | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+)

-- 
2.34.1

[PATCH 1/2] target/riscv: Export sdtrig as an extension and ISA string

[Himanshu Chauhan]

This patch makes the debug trigger (sdtrig) capability
as an extension and exports it as an ISA string. The sdtrig
extension may or may not be implemented in a system. The
	-cpu rv64,sdtrig=<true/false>
option can be used to dynamicaly turn sdtrig extension
on or off.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/cpu.c     | 2 ++
 target/riscv/cpu_cfg.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b07a76ef6b..aaa2d4ff1d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -143,6 +143,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),
@@ -1306,6 +1307,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("zve64d", ext_zve64d, false),
     MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
 
+    MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, true),
     MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
     MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index f4605fb190..3d3acc7f90 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -113,6 +113,7 @@ struct RISCVCPUConfig {
     bool ext_ssaia;
     bool ext_sscofpmf;
     bool ext_smepmp;
+    bool ext_sdtrig;
     bool rvv_ta_all_1s;
     bool rvv_ma_all_1s;
 
-- 
2.34.1

[PATCH 2/2] target/riscv: Raise an exception when sdtrig is turned off

[Himanshu Chauhan]

When sdtrig is turned off by "sdtrig=false" option, raise
and illegal instruction exception on any read/write to
sdtrig CSRs.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/csr.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index c50a33397c..b9ca016ef2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3854,6 +3854,10 @@ static RISCVException write_pmpaddr(CPURISCVState *env, int csrno,
 static RISCVException read_tselect(CPURISCVState *env, int csrno,
                                    target_ulong *val)
 {
+    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     *val = tselect_csr_read(env);
     return RISCV_EXCP_NONE;
 }
@@ -3861,6 +3865,10 @@ static RISCVException read_tselect(CPURISCVState *env, int csrno,
 static RISCVException write_tselect(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
+    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     tselect_csr_write(env, val);
     return RISCV_EXCP_NONE;
 }
@@ -3868,6 +3876,10 @@ static RISCVException write_tselect(CPURISCVState *env, int csrno,
 static RISCVException read_tdata(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     /* return 0 in tdata1 to end the trigger enumeration */
     if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
         *val = 0;
@@ -3885,6 +3897,10 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
 static RISCVException write_tdata(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
+    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     if (!tdata_available(env, csrno - CSR_TDATA1)) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
@@ -3896,6 +3912,10 @@ static RISCVException write_tdata(CPURISCVState *env, int csrno,
 static RISCVException read_tinfo(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
     *val = tinfo_csr_read(env);
     return RISCV_EXCP_NONE;
 }
-- 
2.34.1

Re: [PATCH 0/2] Export debug triggers as an extension

[Daniel Henrique Barboza]

Himanshu,

We spoke offline but let's make everyone aware:

- 'sdtrig' should be marked with 'x-' and be an experimental extension since
the spec isn't yet frozen;

- Alvin sent a patch to the ML adding the 'mcontext' CSR for 'sdtrig' some time
ago:

"[PATCH v2] target/riscv: Implement optional CSR mcontext of debug Sdtrig extension​"

It would be good to put his patch on top of this series to ease the review for everyone.
The changes done in patch 2 would also be applicable to the mcontext CSR;


- last but probably the most important: the existing 'debug' flag seems to be acting as
the actual 'sdtrig' extension due to how the flag is gating trigger code, e.g.:

   if (cpu->cfg.debug) {
         riscv_trigger_realize(&cpu->env);
     }

and

     if (cpu->cfg.debug) {
         riscv_trigger_reset_hold(env);
     }


If that's really the case, all the checks with cpu->cfg.debug will need to also include
cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make an option: do we leave
the debug triggers (i.e. the 'debug' flag) as always enabled?

If it's up to me I would make 'debug' as default 'false' and deprecate it. Users will need
to enable the debug triggers via x-sdtrig=true from now on. This will break existing behavior,
but the way it is now we're always enabling an extension (via the debug flag) that isn't even
frozen, so we're already in the wrong.


Alistair, any thoughts?


Thanks,


Daniel


On 1/10/24 01:02, Himanshu Chauhan wrote:
> All the CPUs may or may not implement the debug trigger (sdtrig)
> extension. The presence of it should be dynamically detectable.
> This patch exports the debug triggers as an extension which
> can be turned on or off by sdtrig=<true/false> option. It is
> turned on by default.
> 
> "sdtrig" is concatenated to ISA string when it is enabled.
> Like so:
> rv64imafdch_zicbom_*_sdtrig_*_sstc_svadu
> 
> 
> Himanshu Chauhan (2):
>    target/riscv: Export sdtrig as an extension and ISA string
>    target/riscv: Raise an exception when sdtrig is turned off
> 
>   target/riscv/cpu.c     |  2 ++
>   target/riscv/cpu_cfg.h |  1 +
>   target/riscv/csr.c     | 20 ++++++++++++++++++++
>   3 files changed, 23 insertions(+)
> 

Re: [PATCH 2/2] target/riscv: Raise an exception when sdtrig is turned off

[Alistair Francis]

On Wed, Jan 10, 2024 at 2:03 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> When sdtrig is turned off by "sdtrig=false" option, raise
> and illegal instruction exception on any read/write to
> sdtrig CSRs.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/csr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index c50a33397c..b9ca016ef2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3854,6 +3854,10 @@ static RISCVException write_pmpaddr(CPURISCVState *env, int csrno,
>  static RISCVException read_tselect(CPURISCVState *env, int csrno,
>                                     target_ulong *val)
>  {
> +    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }

Thanks for the patch!

You should be able to add this check to the

static RISCVException debug(CPURISCVState *env, int csrno)

function instead

Alistair

> +
>      *val = tselect_csr_read(env);
>      return RISCV_EXCP_NONE;
>  }
> @@ -3861,6 +3865,10 @@ static RISCVException read_tselect(CPURISCVState *env, int csrno,
>  static RISCVException write_tselect(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
> +    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>      tselect_csr_write(env, val);
>      return RISCV_EXCP_NONE;
>  }
> @@ -3868,6 +3876,10 @@ static RISCVException write_tselect(CPURISCVState *env, int csrno,
>  static RISCVException read_tdata(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>      /* return 0 in tdata1 to end the trigger enumeration */
>      if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
>          *val = 0;
> @@ -3885,6 +3897,10 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
>  static RISCVException write_tdata(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>  {
> +    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>      if (!tdata_available(env, csrno - CSR_TDATA1)) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> @@ -3896,6 +3912,10 @@ static RISCVException write_tdata(CPURISCVState *env, int csrno,
>  static RISCVException read_tinfo(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
> +    if (!riscv_cpu_cfg(env)->ext_sdtrig) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
>      *val = tinfo_csr_read(env);
>      return RISCV_EXCP_NONE;
>  }
> --
> 2.34.1
>
>

Re: [PATCH 1/2] target/riscv: Export sdtrig as an extension and ISA string

[Alistair Francis]

On Wed, Jan 10, 2024 at 2:03 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> This patch makes the debug trigger (sdtrig) capability
> as an extension and exports it as an ISA string. The sdtrig
> extension may or may not be implemented in a system. The
>         -cpu rv64,sdtrig=<true/false>
> option can be used to dynamicaly turn sdtrig extension
> on or off.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c     | 2 ++
>  target/riscv/cpu_cfg.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..aaa2d4ff1d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -143,6 +143,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),
> @@ -1306,6 +1307,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("zve64d", ext_zve64d, false),
>      MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>
> +    MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, true),

This exposes the property, but doesn't wire it up. Can you swap the
order of these patches?

>      MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
>      MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..3d3acc7f90 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -113,6 +113,7 @@ struct RISCVCPUConfig {
>      bool ext_ssaia;
>      bool ext_sscofpmf;
>      bool ext_smepmp;
> +    bool ext_sdtrig;

and include this change in the other patch

Alistair

>      bool rvv_ta_all_1s;
>      bool rvv_ma_all_1s;
>
> --
> 2.34.1
>
>

Re: [PATCH 0/2] Export debug triggers as an extension

[Alistair Francis]

On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Himanshu,
>
> We spoke offline but let's make everyone aware:
>
> - 'sdtrig' should be marked with 'x-' and be an experimental extension since
> the spec isn't yet frozen;
>
> - Alvin sent a patch to the ML adding the 'mcontext' CSR for 'sdtrig' some time
> ago:
>
> "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug Sdtrig extension"
>
> It would be good to put his patch on top of this series to ease the review for everyone.
> The changes done in patch 2 would also be applicable to the mcontext CSR;
>
>
> - last but probably the most important: the existing 'debug' flag seems to be acting as
> the actual 'sdtrig' extension due to how the flag is gating trigger code, e.g.:
>
>    if (cpu->cfg.debug) {
>          riscv_trigger_realize(&cpu->env);
>      }
>
> and
>
>      if (cpu->cfg.debug) {
>          riscv_trigger_reset_hold(env);
>      }
>
>
> If that's really the case, all the checks with cpu->cfg.debug will need to also include
> cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make an option: do we leave
> the debug triggers (i.e. the 'debug' flag) as always enabled?

From memory the "debug" property is for the original debug spec:
https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote

That was ratified and is an official extension. AFAIK this is what is
in physical hardware as well.

The actual PDF says draft though, I'm not sure what's going on there.

The debug spec doesn't have a Z* name, so it's just "debug", at least AFAIK.

"sdtrig" seems to be a new backwards-incompatible extension doing
basically the same thing. What a mess

>
> If it's up to me I would make 'debug' as default 'false' and deprecate it. Users will need

I don't think that's the right approach. It's a ratified extension
that we are supporting and is in hardware. I think we are stuck
supporting it

> to enable the debug triggers via x-sdtrig=true from now on. This will break existing behavior,
> but the way it is now we're always enabling an extension (via the debug flag) that isn't even
> frozen, so we're already in the wrong.

Maybe the debug group can chime in and say what they expect users to
do? It seems like there are conflicting specs

Alistair

Re: [PATCH 0/2] Export debug triggers as an extension

[Rob Bradford]

On Fri, 2024-01-12 at 13:52 +1000, Alistair Francis wrote:
> On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > 
> > Himanshu,
> > 
> > We spoke offline but let's make everyone aware:
> > 
> > - 'sdtrig' should be marked with 'x-' and be an experimental
> > extension since
> > the spec isn't yet frozen;
> > 
> > - Alvin sent a patch to the ML adding the 'mcontext' CSR for
> > 'sdtrig' some time
> > ago:
> > 
> > "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug
> > Sdtrig extension"
> > 
> > It would be good to put his patch on top of this series to ease the
> > review for everyone.
> > The changes done in patch 2 would also be applicable to the
> > mcontext CSR;
> > 
> > 
> > - last but probably the most important: the existing 'debug' flag
> > seems to be acting as
> > the actual 'sdtrig' extension due to how the flag is gating trigger
> > code, e.g.:
> > 
> >    if (cpu->cfg.debug) {
> >          riscv_trigger_realize(&cpu->env);
> >      }
> > 
> > and
> > 
> >      if (cpu->cfg.debug) {
> >          riscv_trigger_reset_hold(env);
> >      }
> > 
> > 
> > If that's really the case, all the checks with cpu->cfg.debug will
> > need to also include
> > cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make
> > an option: do we leave
> > the debug triggers (i.e. the 'debug' flag) as always enabled?
> 
> From memory the "debug" property is for the original debug spec:
> https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> 
> That was ratified and is an official extension. AFAIK this is what is
> in physical hardware as well.
> 
> The actual PDF says draft though, I'm not sure what's going on there.
> 
> The debug spec doesn't have a Z* name, so it's just "debug", at least
> AFAIK.
> 
> "sdtrig" seems to be a new backwards-incompatible extension doing
> basically the same thing. What a mess
> 
> > 
> > If it's up to me I would make 'debug' as default 'false' and
> > deprecate it. Users will need
> 
> I don't think that's the right approach. It's a ratified extension
> that we are supporting and is in hardware. I think we are stuck
> supporting it
> 
> 

I've done a bit of digging and I agree things are quite messy. Here are
my discoveries:

The debug option and the code for triggers was added in these commits:

c9711bd778 target/riscv: cpu: Enable native debug feature
38b4e781a4 target/riscv: machine: Add debug state description
b6092544fc target/riscv: csr: Hook debug CSR read/write
1acdb3b013 target/riscv: cpu: Add a config option for native debug
95799e36c1 target/riscv: Add initial support for the Sdtrig extension

In March 2022 - since the commit refers to the Sdtrig extension name
and from the date this was an implementation not of the ratified 0.13
debug spec (which did not have Sdtrig as a separate extension) but
rather a version of the in development 1.0 debug spec.

It's not trivial to tell if it's closer to the ratified 0.13 version or
the (hopefully soon to be frozen) 1.0 version.

As the only part of the debug specification to be implemented is the
triggers then effectively the debug option is x-sdtrig.

I don't think there is any way for code running on the machine to
identify what version of the debug is implemented - the appropriate
register is only available for external debug. Once 1.0 is frozen then
the presence of Sdtrig isa string would indicate 1.0 trigger support is
available.

According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
specification should freeze this month.

How about considering this as a solution:

- Add a new x-sdtrig option that defaults to false
- Deprecate debug option - but retain it with default on
- Add warning if triggers are used and x-sdtrig is not enabled
- Update the trigger implementation to match frozen spec

There is potentially a chance that some use cases will be broken but I
don't think triggers are being widely use - the SBI support only just
got merged:
https://github.com/riscv-software-src/opensbi/commit/97f234f15c9657c6ec69fa6ed745be8107bf6ae2

Hope this is helpful,

Rob

Re: [PATCH 0/2] Export debug triggers as an extension

[Daniel Henrique Barboza]

On 1/12/24 10:34, Rob Bradford wrote:
> On Fri, 2024-01-12 at 13:52 +1000, Alistair Francis wrote:
>> On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> Himanshu,
>>>
>>> We spoke offline but let's make everyone aware:
>>>
>>> - 'sdtrig' should be marked with 'x-' and be an experimental
>>> extension since
>>> the spec isn't yet frozen;
>>>
>>> - Alvin sent a patch to the ML adding the 'mcontext' CSR for
>>> 'sdtrig' some time
>>> ago:
>>>
>>> "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug
>>> Sdtrig extension"
>>>
>>> It would be good to put his patch on top of this series to ease the
>>> review for everyone.
>>> The changes done in patch 2 would also be applicable to the
>>> mcontext CSR;
>>>
>>>
>>> - last but probably the most important: the existing 'debug' flag
>>> seems to be acting as
>>> the actual 'sdtrig' extension due to how the flag is gating trigger
>>> code, e.g.:
>>>
>>>     if (cpu->cfg.debug) {
>>>           riscv_trigger_realize(&cpu->env);
>>>       }
>>>
>>> and
>>>
>>>       if (cpu->cfg.debug) {
>>>           riscv_trigger_reset_hold(env);
>>>       }
>>>
>>>
>>> If that's really the case, all the checks with cpu->cfg.debug will
>>> need to also include
>>> cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make
>>> an option: do we leave
>>> the debug triggers (i.e. the 'debug' flag) as always enabled?
>>
>>  From memory the "debug" property is for the original debug spec:
>> https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
>>
>> That was ratified and is an official extension. AFAIK this is what is
>> in physical hardware as well.
>>
>> The actual PDF says draft though, I'm not sure what's going on there.
>>
>> The debug spec doesn't have a Z* name, so it's just "debug", at least
>> AFAIK.
>>
>> "sdtrig" seems to be a new backwards-incompatible extension doing
>> basically the same thing. What a mess
>>
>>>
>>> If it's up to me I would make 'debug' as default 'false' and
>>> deprecate it. Users will need
>>
>> I don't think that's the right approach. It's a ratified extension
>> that we are supporting and is in hardware. I think we are stuck
>> supporting it
>>
>>
> 
> I've done a bit of digging and I agree things are quite messy. Here are
> my discoveries:
> 
> The debug option and the code for triggers was added in these commits:
> 
> c9711bd778 target/riscv: cpu: Enable native debug feature
> 38b4e781a4 target/riscv: machine: Add debug state description
> b6092544fc target/riscv: csr: Hook debug CSR read/write
> 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
> 
> In March 2022 - since the commit refers to the Sdtrig extension name
> and from the date this was an implementation not of the ratified 0.13
> debug spec (which did not have Sdtrig as a separate extension) but
> rather a version of the in development 1.0 debug spec.
> 
> It's not trivial to tell if it's closer to the ratified 0.13 version or
> the (hopefully soon to be frozen) 1.0 version.
> 
> As the only part of the debug specification to be implemented is the
> triggers then effectively the debug option is x-sdtrig.
> 
> I don't think there is any way for code running on the machine to
> identify what version of the debug is implemented - the appropriate
> register is only available for external debug. Once 1.0 is frozen then
> the presence of Sdtrig isa string would indicate 1.0 trigger support is
> available.
> 
> According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> specification should freeze this month.
> 
> How about considering this as a solution:
> 
> - Add a new x-sdtrig option that defaults to false
> - Deprecate debug option - but retain it with default on
> - Add warning if triggers are used and x-sdtrig is not enabled
> - Update the trigger implementation to match frozen spec

If x-sdtrig is 'false' and 'debug' is on, and then we warn if debug=true and x-sdtrig
is false, we'll warn every time using the defaults.

Given what you said here:

> 
> There is potentially a chance that some use cases will be broken but I
> don't think triggers are being widely use - the SBI support only just
> got merged:
> https://github.com/riscv-software-src/opensbi/commit/97f234f15c9657c6ec69fa6ed745be8107bf6a> 

I believe we can deprecate 'debug' and simply ignore its existence. Do everything else with
x-sdtrig. So if an user sets any 'debug' value we'll:

- warn that the flag is deprecated
- set x-sdtrig to whatever value the user set to 'debug'

'debug' will become just an alternate way to set x-sdtrig. The logic should just check for
x-sdtrig.


Thanks,


Daniel
   

> Hope this is helpful,
> 
> Rob
> 

Re: [PATCH 0/2] Export debug triggers as an extension

[Himanshu Chauhan]

> On 17-Jan-2024, at 10:29 PM, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
> 
> 
> On 1/12/24 10:34, Rob Bradford wrote:
>> On Fri, 2024-01-12 at 13:52 +1000, Alistair Francis wrote:
>>> On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>> 
>>>> Himanshu,
>>>> 
>>>> We spoke offline but let's make everyone aware:
>>>> 
>>>> - 'sdtrig' should be marked with 'x-' and be an experimental
>>>> extension since
>>>> the spec isn't yet frozen;
>>>> 
>>>> - Alvin sent a patch to the ML adding the 'mcontext' CSR for
>>>> 'sdtrig' some time
>>>> ago:
>>>> 
>>>> "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug
>>>> Sdtrig extension"
>>>> 
>>>> It would be good to put his patch on top of this series to ease the
>>>> review for everyone.
>>>> The changes done in patch 2 would also be applicable to the
>>>> mcontext CSR;
>>>> 
>>>> 
>>>> - last but probably the most important: the existing 'debug' flag
>>>> seems to be acting as
>>>> the actual 'sdtrig' extension due to how the flag is gating trigger
>>>> code, e.g.:
>>>> 
>>>>    if (cpu->cfg.debug) {
>>>>          riscv_trigger_realize(&cpu->env);
>>>>      }
>>>> 
>>>> and
>>>> 
>>>>      if (cpu->cfg.debug) {
>>>>          riscv_trigger_reset_hold(env);
>>>>      }
>>>> 
>>>> 
>>>> If that's really the case, all the checks with cpu->cfg.debug will
>>>> need to also include
>>>> cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make
>>>> an option: do we leave
>>>> the debug triggers (i.e. the 'debug' flag) as always enabled?
>>> 
>>> From memory the "debug" property is for the original debug spec:
>>> https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
>>> 
>>> That was ratified and is an official extension. AFAIK this is what is
>>> in physical hardware as well.
>>> 
>>> The actual PDF says draft though, I'm not sure what's going on there.
>>> 
>>> The debug spec doesn't have a Z* name, so it's just "debug", at least
>>> AFAIK.
>>> 
>>> "sdtrig" seems to be a new backwards-incompatible extension doing
>>> basically the same thing. What a mess
>>> 
>>>> 
>>>> If it's up to me I would make 'debug' as default 'false' and
>>>> deprecate it. Users will need
>>> 
>>> I don't think that's the right approach. It's a ratified extension
>>> that we are supporting and is in hardware. I think we are stuck
>>> supporting it
>>> 
>>> 
>> I've done a bit of digging and I agree things are quite messy. Here are
>> my discoveries:
>> The debug option and the code for triggers was added in these commits:
>> c9711bd778 target/riscv: cpu: Enable native debug feature
>> 38b4e781a4 target/riscv: machine: Add debug state description
>> b6092544fc target/riscv: csr: Hook debug CSR read/write
>> 1acdb3b013 target/riscv: cpu: Add a config option for native debug
>> 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
>> In March 2022 - since the commit refers to the Sdtrig extension name
>> and from the date this was an implementation not of the ratified 0.13
>> debug spec (which did not have Sdtrig as a separate extension) but
>> rather a version of the in development 1.0 debug spec.
>> It's not trivial to tell if it's closer to the ratified 0.13 version or
>> the (hopefully soon to be frozen) 1.0 version.
>> As the only part of the debug specification to be implemented is the
>> triggers then effectively the debug option is x-sdtrig.
>> I don't think there is any way for code running on the machine to
>> identify what version of the debug is implemented - the appropriate
>> register is only available for external debug. Once 1.0 is frozen then
>> the presence of Sdtrig isa string would indicate 1.0 trigger support is
>> available.
>> According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
>> specification should freeze this month.
>> How about considering this as a solution:
>> - Add a new x-sdtrig option that defaults to false
>> - Deprecate debug option - but retain it with default on
>> - Add warning if triggers are used and x-sdtrig is not enabled
>> - Update the trigger implementation to match frozen spec
> 
> If x-sdtrig is 'false' and 'debug' is on, and then we warn if debug=true and x-sdtrig
> is false, we'll warn every time using the defaults.
> 
> Given what you said here:
> 
>> There is potentially a chance that some use cases will be broken but I
>> don't think triggers are being widely use - the SBI support only just
>> got merged:
>> https://github.com/riscv-software-src/opensbi/commit/97f234f15c9657c6ec69fa6ed745be8107bf6a> 
> 
> I believe we can deprecate 'debug' and simply ignore its existence. Do everything else with
> x-sdtrig. So if an user sets any 'debug' value we'll:
> 
> - warn that the flag is deprecated
> - set x-sdtrig to whatever value the user set to 'debug'
> 
> 'debug' will become just an alternate way to set x-sdtrig. The logic should just check for
> x-sdtrig.

My v2 patchset which I posted a few ours back, gets rid of ‘debug’. It is replaced with x-sdtrig. Keeping them both doesn’t make sense.

Here is v2:
https://mail.gnu.org/archive/html/qemu-riscv/2024-01/msg00570.html


Regards
Himanshu

> 
> 
> Thanks,
> 
> 
> Daniel
>  
>> Hope this is helpful,
>> Rob

Re: [PATCH 0/2] Export debug triggers as an extension

[Alistair Francis]

On Fri, Jan 12, 2024 at 11:34 PM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> On Fri, 2024-01-12 at 13:52 +1000, Alistair Francis wrote:
> > On Thu, Jan 11, 2024 at 5:20 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> > >
> > > Himanshu,
> > >
> > > We spoke offline but let's make everyone aware:
> > >
> > > - 'sdtrig' should be marked with 'x-' and be an experimental
> > > extension since
> > > the spec isn't yet frozen;
> > >
> > > - Alvin sent a patch to the ML adding the 'mcontext' CSR for
> > > 'sdtrig' some time
> > > ago:
> > >
> > > "[PATCH v2] target/riscv: Implement optional CSR mcontext of debug
> > > Sdtrig extension"
> > >
> > > It would be good to put his patch on top of this series to ease the
> > > review for everyone.
> > > The changes done in patch 2 would also be applicable to the
> > > mcontext CSR;
> > >
> > >
> > > - last but probably the most important: the existing 'debug' flag
> > > seems to be acting as
> > > the actual 'sdtrig' extension due to how the flag is gating trigger
> > > code, e.g.:
> > >
> > >    if (cpu->cfg.debug) {
> > >          riscv_trigger_realize(&cpu->env);
> > >      }
> > >
> > > and
> > >
> > >      if (cpu->cfg.debug) {
> > >          riscv_trigger_reset_hold(env);
> > >      }
> > >
> > >
> > > If that's really the case, all the checks with cpu->cfg.debug will
> > > need to also include
> > > cpu->cfg.ext_sdtrig (one or the other). And now we'll have to make
> > > an option: do we leave
> > > the debug triggers (i.e. the 'debug' flag) as always enabled?
> >
> > From memory the "debug" property is for the original debug spec:
> > https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> >
> > That was ratified and is an official extension. AFAIK this is what is
> > in physical hardware as well.
> >
> > The actual PDF says draft though, I'm not sure what's going on there.
> >
> > The debug spec doesn't have a Z* name, so it's just "debug", at least
> > AFAIK.
> >
> > "sdtrig" seems to be a new backwards-incompatible extension doing
> > basically the same thing. What a mess
> >
> > >
> > > If it's up to me I would make 'debug' as default 'false' and
> > > deprecate it. Users will need
> >
> > I don't think that's the right approach. It's a ratified extension
> > that we are supporting and is in hardware. I think we are stuck
> > supporting it
> >
> >
>
> I've done a bit of digging and I agree things are quite messy. Here are
> my discoveries:
>
> The debug option and the code for triggers was added in these commits:
>
> c9711bd778 target/riscv: cpu: Enable native debug feature
> 38b4e781a4 target/riscv: machine: Add debug state description
> b6092544fc target/riscv: csr: Hook debug CSR read/write
> 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
>
> In March 2022 - since the commit refers to the Sdtrig extension name
> and from the date this was an implementation not of the ratified 0.13
> debug spec (which did not have Sdtrig as a separate extension) but
> rather a version of the in development 1.0 debug spec.

Yeah... We used the "stable" from master. That is our mistake there.

I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
the v4 as the changelog says: "mention Sdtrig extension in the commit"

>
> It's not trivial to tell if it's closer to the ratified 0.13 version or
> the (hopefully soon to be frozen) 1.0 version.
>
> As the only part of the debug specification to be implemented is the
> triggers then effectively the debug option is x-sdtrig.
>
> I don't think there is any way for code running on the machine to
> identify what version of the debug is implemented - the appropriate
> register is only available for external debug. Once 1.0 is frozen then
> the presence of Sdtrig isa string would indicate 1.0 trigger support is
> available.
>
> According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> specification should freeze this month.
>
> How about considering this as a solution:
>
> - Add a new x-sdtrig option that defaults to false
> - Deprecate debug option - but retain it with default on

We can't deprecate a ratified spec. The 0.13 just seems to call it
"debug" so that's what we are stuck with

> - Add warning if triggers are used and x-sdtrig is not enabled
> - Update the trigger implementation to match frozen spec

We will need to support two versions, as there are two ratified specs.

Alistair

>
> There is potentially a chance that some use cases will be broken but I
> don't think triggers are being widely use - the SBI support only just
> got merged:
> https://github.com/riscv-software-src/opensbi/commit/97f234f15c9657c6ec69fa6ed745be8107bf6ae2
>
> Hope this is helpful,
>
> Rob
>

Re: Re: [PATCH 0/2] Export debug triggers as an extension

[Andrew Jones]

On Mon, Jan 22, 2024 at 03:42:10PM +1000, Alistair Francis wrote:
> > > From memory the "debug" property is for the original debug spec:
> > > https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> > >
> > > That was ratified and is an official extension. AFAIK this is what is
> > > in physical hardware as well.
> > >
> > > The actual PDF says draft though, I'm not sure what's going on there.
> > >
> > > The debug spec doesn't have a Z* name, so it's just "debug", at least
> > > AFAIK.
> > >
> > > "sdtrig" seems to be a new backwards-incompatible extension doing
> > > basically the same thing. What a mess
...
> >
> > I've done a bit of digging and I agree things are quite messy. Here are
> > my discoveries:
> >
> > The debug option and the code for triggers was added in these commits:
> >
> > c9711bd778 target/riscv: cpu: Enable native debug feature
> > 38b4e781a4 target/riscv: machine: Add debug state description
> > b6092544fc target/riscv: csr: Hook debug CSR read/write
> > 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> > 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
> >
> > In March 2022 - since the commit refers to the Sdtrig extension name
> > and from the date this was an implementation not of the ratified 0.13
> > debug spec (which did not have Sdtrig as a separate extension) but
> > rather a version of the in development 1.0 debug spec.
> 
> Yeah... We used the "stable" from master. That is our mistake there.
> 
> I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
> the v4 as the changelog says: "mention Sdtrig extension in the commit"
> 
> >
> > It's not trivial to tell if it's closer to the ratified 0.13 version or
> > the (hopefully soon to be frozen) 1.0 version.
> >
> > As the only part of the debug specification to be implemented is the
> > triggers then effectively the debug option is x-sdtrig.
> >
> > I don't think there is any way for code running on the machine to
> > identify what version of the debug is implemented - the appropriate
> > register is only available for external debug. Once 1.0 is frozen then
> > the presence of Sdtrig isa string would indicate 1.0 trigger support is
> > available.
> >
> > According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> > specification should freeze this month.
> >
> > How about considering this as a solution:
> >
> > - Add a new x-sdtrig option that defaults to false
> > - Deprecate debug option - but retain it with default on
> 
> We can't deprecate a ratified spec. The 0.13 just seems to call it
> "debug" so that's what we are stuck with
> 
> > - Add warning if triggers are used and x-sdtrig is not enabled
> > - Update the trigger implementation to match frozen spec
> 
> We will need to support two versions, as there are two ratified specs.
>

We'll likely want to be allowed to deprecate ratified extensions as riscv
evolves. Despite best intentions, extensions may be designed and ratified
which ultimately fail to be of much utility, and new extensions will
supersede old extensions. If QEMU keeps every extension it adds, then
we'll slow progress on new extensions by maintaining old extension code.
The old extensions will also bitrot or waste CI resources getting tested
for no reason.

I don't know the history of 'debug' and 'sdtrig', other than what I've
read above, but, to me, it looks like 'debug' might be one of the first
extensions which should be deprecated. Assuming we have a long enough
deprecation period, then I think it's always safe to attempt a
deprecation. If somebody shouts, then it can always be taken back off the
chopping block.

Thanks,
drew

Re: Re: [PATCH 0/2] Export debug triggers as an extension

[Alistair Francis]

On Mon, Jan 22, 2024 at 7:16 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 22, 2024 at 03:42:10PM +1000, Alistair Francis wrote:
> > > > From memory the "debug" property is for the original debug spec:
> > > > https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> > > >
> > > > That was ratified and is an official extension. AFAIK this is what is
> > > > in physical hardware as well.
> > > >
> > > > The actual PDF says draft though, I'm not sure what's going on there.
> > > >
> > > > The debug spec doesn't have a Z* name, so it's just "debug", at least
> > > > AFAIK.
> > > >
> > > > "sdtrig" seems to be a new backwards-incompatible extension doing
> > > > basically the same thing. What a mess
> ...
> > >
> > > I've done a bit of digging and I agree things are quite messy. Here are
> > > my discoveries:
> > >
> > > The debug option and the code for triggers was added in these commits:
> > >
> > > c9711bd778 target/riscv: cpu: Enable native debug feature
> > > 38b4e781a4 target/riscv: machine: Add debug state description
> > > b6092544fc target/riscv: csr: Hook debug CSR read/write
> > > 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> > > 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
> > >
> > > In March 2022 - since the commit refers to the Sdtrig extension name
> > > and from the date this was an implementation not of the ratified 0.13
> > > debug spec (which did not have Sdtrig as a separate extension) but
> > > rather a version of the in development 1.0 debug spec.
> >
> > Yeah... We used the "stable" from master. That is our mistake there.
> >
> > I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
> > the v4 as the changelog says: "mention Sdtrig extension in the commit"
> >
> > >
> > > It's not trivial to tell if it's closer to the ratified 0.13 version or
> > > the (hopefully soon to be frozen) 1.0 version.
> > >
> > > As the only part of the debug specification to be implemented is the
> > > triggers then effectively the debug option is x-sdtrig.
> > >
> > > I don't think there is any way for code running on the machine to
> > > identify what version of the debug is implemented - the appropriate
> > > register is only available for external debug. Once 1.0 is frozen then
> > > the presence of Sdtrig isa string would indicate 1.0 trigger support is
> > > available.
> > >
> > > According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> > > specification should freeze this month.
> > >
> > > How about considering this as a solution:
> > >
> > > - Add a new x-sdtrig option that defaults to false
> > > - Deprecate debug option - but retain it with default on
> >
> > We can't deprecate a ratified spec. The 0.13 just seems to call it
> > "debug" so that's what we are stuck with
> >
> > > - Add warning if triggers are used and x-sdtrig is not enabled
> > > - Update the trigger implementation to match frozen spec
> >
> > We will need to support two versions, as there are two ratified specs.
> >
>
> We'll likely want to be allowed to deprecate ratified extensions as riscv
> evolves. Despite best intentions, extensions may be designed and ratified
> which ultimately fail to be of much utility, and new extensions will
> supersede old extensions. If QEMU keeps every extension it adds, then
> we'll slow progress on new extensions by maintaining old extension code.
> The old extensions will also bitrot or waste CI resources getting tested
> for no reason.

I agree that we might need to deprecate extensions.

I'm not sure the debug extension is there though. The debug extension
is used in current shipping hardware and has been ratified. The Sdtrig
isn't even ratified yet
(https://lists.riscv.org/g/tech-announce/message/320)

Right now I feel that we should at least wait for hardware that
supports Sdtrig to start to come out. Then we can look at deprecating
debug. Deprecating it now seems a bit premature.

Alistair

>
> I don't know the history of 'debug' and 'sdtrig', other than what I've
> read above, but, to me, it looks like 'debug' might be one of the first
> extensions which should be deprecated. Assuming we have a long enough
> deprecation period, then I think it's always safe to attempt a
> deprecation. If somebody shouts, then it can always be taken back off the
> chopping block.
>
> Thanks,
> drew
>

Re: Re: [PATCH 0/2] Export debug triggers as an extension

[Anup Patel]

On Mon, Feb 5, 2024 at 9:36 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jan 22, 2024 at 7:16 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Jan 22, 2024 at 03:42:10PM +1000, Alistair Francis wrote:
> > > > > From memory the "debug" property is for the original debug spec:
> > > > > https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> > > > >
> > > > > That was ratified and is an official extension. AFAIK this is what is
> > > > > in physical hardware as well.
> > > > >
> > > > > The actual PDF says draft though, I'm not sure what's going on there.
> > > > >
> > > > > The debug spec doesn't have a Z* name, so it's just "debug", at least
> > > > > AFAIK.
> > > > >
> > > > > "sdtrig" seems to be a new backwards-incompatible extension doing
> > > > > basically the same thing. What a mess
> > ...
> > > >
> > > > I've done a bit of digging and I agree things are quite messy. Here are
> > > > my discoveries:
> > > >
> > > > The debug option and the code for triggers was added in these commits:
> > > >
> > > > c9711bd778 target/riscv: cpu: Enable native debug feature
> > > > 38b4e781a4 target/riscv: machine: Add debug state description
> > > > b6092544fc target/riscv: csr: Hook debug CSR read/write
> > > > 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> > > > 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
> > > >
> > > > In March 2022 - since the commit refers to the Sdtrig extension name
> > > > and from the date this was an implementation not of the ratified 0.13
> > > > debug spec (which did not have Sdtrig as a separate extension) but
> > > > rather a version of the in development 1.0 debug spec.
> > >
> > > Yeah... We used the "stable" from master. That is our mistake there.
> > >
> > > I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
> > > the v4 as the changelog says: "mention Sdtrig extension in the commit"
> > >
> > > >
> > > > It's not trivial to tell if it's closer to the ratified 0.13 version or
> > > > the (hopefully soon to be frozen) 1.0 version.
> > > >
> > > > As the only part of the debug specification to be implemented is the
> > > > triggers then effectively the debug option is x-sdtrig.
> > > >
> > > > I don't think there is any way for code running on the machine to
> > > > identify what version of the debug is implemented - the appropriate
> > > > register is only available for external debug. Once 1.0 is frozen then
> > > > the presence of Sdtrig isa string would indicate 1.0 trigger support is
> > > > available.
> > > >
> > > > According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> > > > specification should freeze this month.
> > > >
> > > > How about considering this as a solution:
> > > >
> > > > - Add a new x-sdtrig option that defaults to false
> > > > - Deprecate debug option - but retain it with default on
> > >
> > > We can't deprecate a ratified spec. The 0.13 just seems to call it
> > > "debug" so that's what we are stuck with
> > >
> > > > - Add warning if triggers are used and x-sdtrig is not enabled
> > > > - Update the trigger implementation to match frozen spec
> > >
> > > We will need to support two versions, as there are two ratified specs.
> > >
> >
> > We'll likely want to be allowed to deprecate ratified extensions as riscv
> > evolves. Despite best intentions, extensions may be designed and ratified
> > which ultimately fail to be of much utility, and new extensions will
> > supersede old extensions. If QEMU keeps every extension it adds, then
> > we'll slow progress on new extensions by maintaining old extension code.
> > The old extensions will also bitrot or waste CI resources getting tested
> > for no reason.
>
> I agree that we might need to deprecate extensions.
>
> I'm not sure the debug extension is there though. The debug extension
> is used in current shipping hardware and has been ratified. The Sdtrig
> isn't even ratified yet
> (https://lists.riscv.org/g/tech-announce/message/320)

Is shipping real hardware OR ratification a requirement of
QEMU patch acceptance ?

Regards,
Anup

>
> Right now I feel that we should at least wait for hardware that
> supports Sdtrig to start to come out. Then we can look at deprecating
> debug. Deprecating it now seems a bit premature.
>
> Alistair
>
> >
> > I don't know the history of 'debug' and 'sdtrig', other than what I've
> > read above, but, to me, it looks like 'debug' might be one of the first
> > extensions which should be deprecated. Assuming we have a long enough
> > deprecation period, then I think it's always safe to attempt a
> > deprecation. If somebody shouts, then it can always be taken back off the
> > chopping block.
> >
> > Thanks,
> > drew
> >
>

Re: Re: [PATCH 0/2] Export debug triggers as an extension

[Alistair Francis]

On Mon, Feb 5, 2024 at 2:50 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Mon, Feb 5, 2024 at 9:36 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Mon, Jan 22, 2024 at 7:16 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 03:42:10PM +1000, Alistair Francis wrote:
> > > > > > From memory the "debug" property is for the original debug spec:
> > > > > > https://github.com/riscv/riscv-debug-spec/releases/tag/task_group_vote
> > > > > >
> > > > > > That was ratified and is an official extension. AFAIK this is what is
> > > > > > in physical hardware as well.
> > > > > >
> > > > > > The actual PDF says draft though, I'm not sure what's going on there.
> > > > > >
> > > > > > The debug spec doesn't have a Z* name, so it's just "debug", at least
> > > > > > AFAIK.
> > > > > >
> > > > > > "sdtrig" seems to be a new backwards-incompatible extension doing
> > > > > > basically the same thing. What a mess
> > > ...
> > > > >
> > > > > I've done a bit of digging and I agree things are quite messy. Here are
> > > > > my discoveries:
> > > > >
> > > > > The debug option and the code for triggers was added in these commits:
> > > > >
> > > > > c9711bd778 target/riscv: cpu: Enable native debug feature
> > > > > 38b4e781a4 target/riscv: machine: Add debug state description
> > > > > b6092544fc target/riscv: csr: Hook debug CSR read/write
> > > > > 1acdb3b013 target/riscv: cpu: Add a config option for native debug
> > > > > 95799e36c1 target/riscv: Add initial support for the Sdtrig extension
> > > > >
> > > > > In March 2022 - since the commit refers to the Sdtrig extension name
> > > > > and from the date this was an implementation not of the ratified 0.13
> > > > > debug spec (which did not have Sdtrig as a separate extension) but
> > > > > rather a version of the in development 1.0 debug spec.
> > > >
> > > > Yeah... We used the "stable" from master. That is our mistake there.
> > > >
> > > > I'm pretty sure we targeted the 0.13. The "Sdtrig" was only added in
> > > > the v4 as the changelog says: "mention Sdtrig extension in the commit"
> > > >
> > > > >
> > > > > It's not trivial to tell if it's closer to the ratified 0.13 version or
> > > > > the (hopefully soon to be frozen) 1.0 version.
> > > > >
> > > > > As the only part of the debug specification to be implemented is the
> > > > > triggers then effectively the debug option is x-sdtrig.
> > > > >
> > > > > I don't think there is any way for code running on the machine to
> > > > > identify what version of the debug is implemented - the appropriate
> > > > > register is only available for external debug. Once 1.0 is frozen then
> > > > > the presence of Sdtrig isa string would indicate 1.0 trigger support is
> > > > > available.
> > > > >
> > > > > According to JIRA - https://jira.riscv.org/browse/RVS-981 the debug
> > > > > specification should freeze this month.
> > > > >
> > > > > How about considering this as a solution:
> > > > >
> > > > > - Add a new x-sdtrig option that defaults to false
> > > > > - Deprecate debug option - but retain it with default on
> > > >
> > > > We can't deprecate a ratified spec. The 0.13 just seems to call it
> > > > "debug" so that's what we are stuck with
> > > >
> > > > > - Add warning if triggers are used and x-sdtrig is not enabled
> > > > > - Update the trigger implementation to match frozen spec
> > > >
> > > > We will need to support two versions, as there are two ratified specs.
> > > >
> > >
> > > We'll likely want to be allowed to deprecate ratified extensions as riscv
> > > evolves. Despite best intentions, extensions may be designed and ratified
> > > which ultimately fail to be of much utility, and new extensions will
> > > supersede old extensions. If QEMU keeps every extension it adds, then
> > > we'll slow progress on new extensions by maintaining old extension code.
> > > The old extensions will also bitrot or waste CI resources getting tested
> > > for no reason.
> >
> > I agree that we might need to deprecate extensions.
> >
> > I'm not sure the debug extension is there though. The debug extension
> > is used in current shipping hardware and has been ratified. The Sdtrig
> > isn't even ratified yet
> > (https://lists.riscv.org/g/tech-announce/message/320)
>
> Is shipping real hardware OR ratification a requirement of
> QEMU patch acceptance ?

We will accept an extension when it is ratified. The question here is,
what if two ratified extensions conflict?

The answer to me seems that we need to support both them. *Maybe* at
some point in the future we can then drop the debug extension. That
would require the Sdtrig extension to be widely used and debug not
used (which is why I brought up shipping hardware).

Alistair

>
> Regards,
> Anup
>
> >
> > Right now I feel that we should at least wait for hardware that
> > supports Sdtrig to start to come out. Then we can look at deprecating
> > debug. Deprecating it now seems a bit premature.
> >
> > Alistair
> >
> > >
> > > I don't know the history of 'debug' and 'sdtrig', other than what I've
> > > read above, but, to me, it looks like 'debug' might be one of the first
> > > extensions which should be deprecated. Assuming we have a long enough
> > > deprecation period, then I think it's always safe to attempt a
> > > deprecation. If somebody shouts, then it can always be taken back off the
> > > chopping block.
> > >
> > > Thanks,
> > > drew
> > >
> >