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

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

[Himanshu Chauhan]

All the CPUs may or may not implement the debug triggers (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

Changes from v1:
   - Replaced the debug property with ext_sdtrig
   - Marked it experimenatal by naming it x-sdtrig
   - x-sdtrig is added to ISA string
   - Reversed the patch order

Changes from v2:
   - Mark debug property as deprecated and replace internally with sdtrig extension
   - setting/unsetting debug property shows warning and sets/unsets ext_sdtrig
   - sdtrig is added to ISA string as RISC-V debug specification is frozen

Himanshu Chauhan (2):
  target/riscv: Mark debug property as deprecated
  target/riscv: Export sdtrig in ISA string

 target/riscv/cpu.c        | 38 +++++++++++++++++++++++++++++++++++---
 target/riscv/cpu_cfg.h    |  2 +-
 target/riscv/cpu_helper.c |  2 +-
 target/riscv/csr.c        |  2 +-
 target/riscv/machine.c    |  2 +-
 5 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] target/riscv: Mark debug property as deprecated

[Himanshu Chauhan]

The debug property implements the ratified debug specification v0.13.
This specification is superseded by (now frozen) RISC-V debug specification v1.0
It defines sdtrig ISA extension which is forward and backward comptible with
the debug specification v0.13.

This patch deprecates the debug property and replaces with ext_sdtrig.
A deprecation warning is displayed if debug property is used.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/cpu.c        | 37 ++++++++++++++++++++++++++++++++++---
 target/riscv/cpu_cfg.h    |  2 +-
 target/riscv/cpu_helper.c |  2 +-
 target/riscv/csr.c        |  2 +-
 target/riscv/machine.c    |  2 +-
 5 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5ff0192c52..5d5d8f0375 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -996,7 +996,7 @@ static void riscv_cpu_reset_hold(Object *obj)
     set_default_nan_mode(1, &env->fp_status);
 
 #ifndef CONFIG_USER_ONLY
-    if (cpu->cfg.debug) {
+    if (cpu->cfg.ext_sdtrig) {
         riscv_trigger_reset_hold(env);
     }
 
@@ -1156,7 +1156,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     riscv_cpu_register_gdb_regs_for_features(cs);
 
 #ifndef CONFIG_USER_ONLY
-    if (cpu->cfg.debug) {
+    if (cpu->cfg.ext_sdtrig) {
         riscv_trigger_realize(&cpu->env);
     }
 #endif
@@ -1718,6 +1718,37 @@ static const PropertyInfo prop_mmu = {
     .set = prop_mmu_set,
 };
 
+static void prop_debug_set(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value;
+
+    warn_report("\"debug\" property is being deprecated.");
+
+    visit_type_bool(v, name, &value, errp);
+
+    if (cpu->cfg.ext_sdtrig != value && !riscv_cpu_is_dynamic(obj)) {
+        return;
+    }
+
+    cpu->cfg.ext_sdtrig = value;
+}
+
+static void prop_debug_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.ext_sdtrig;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_debug = {
+    .name = "debug",
+    .get = prop_debug_get,
+    .set = prop_debug_set,
+};
+
 static void prop_pmp_set(Object *obj, Visitor *v, const char *name,
                          void *opaque, Error **errp)
 {
@@ -2229,7 +2260,7 @@ RISCVCPUProfile *riscv_profiles[] = {
 };
 
 static Property riscv_cpu_properties[] = {
-    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
+    {.name = "debug", .info = &prop_debug}, /* Deprecated */
 
     {.name = "pmu-mask", .info = &prop_pmu_mask},
     {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 833bf58217..9fb4ca577f 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -113,6 +113,7 @@ struct RISCVCPUConfig {
     bool ext_zvfbfwma;
     bool ext_zvfh;
     bool ext_zvfhmin;
+    bool ext_sdtrig;
     bool ext_smaia;
     bool ext_ssaia;
     bool ext_sscofpmf;
@@ -148,7 +149,6 @@ struct RISCVCPUConfig {
     uint16_t cboz_blocksize;
     bool mmu;
     bool pmp;
-    bool debug;
     bool misa_w;
 
     bool short_isa_string;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d462d95ee1..b2ad97d601 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -129,7 +129,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
              ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED;
     }
 
-    if (cpu->cfg.debug && !icount_enabled()) {
+    if (cpu->cfg.ext_sdtrig && !icount_enabled()) {
         flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
     }
 #endif
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d4e8ac13b9..e60599d74e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -546,7 +546,7 @@ static RISCVException have_mseccfg(CPURISCVState *env, int csrno)
 
 static RISCVException debug(CPURISCVState *env, int csrno)
 {
-    if (riscv_cpu_cfg(env)->debug) {
+    if (riscv_cpu_cfg(env)->ext_sdtrig) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 81cf22894e..1b775342d2 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -230,7 +230,7 @@ static bool debug_needed(void *opaque)
 {
     RISCVCPU *cpu = opaque;
 
-    return cpu->cfg.debug;
+    return cpu->cfg.ext_sdtrig;
 }
 
 static int debug_post_load(void *opaque, int version_id)
-- 
2.34.1

[PATCH v3 2/2] target/riscv: Export sdtrig in ISA string

[Himanshu Chauhan]

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.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5d5d8f0375..6f98c0195c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1461,6 +1461,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),
@@ -1724,7 +1725,7 @@ static void prop_debug_set(Object *obj, Visitor *v, const char *name,
     RISCVCPU *cpu = RISCV_CPU(obj);
     bool value;
 
-    warn_report("\"debug\" property is being deprecated.");
+    warn_report("\"debug\" property is deprecated; use \"sdtrig\"");
 
     visit_type_bool(v, name, &value, errp);
 
-- 
2.34.1

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

[Andrew Jones]

On Thu, Feb 29, 2024 at 07:07:43PM +0530, Himanshu Chauhan wrote:
> All the CPUs may or may not implement the debug triggers (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
> 
> Changes from v1:
>    - Replaced the debug property with ext_sdtrig
>    - Marked it experimenatal by naming it x-sdtrig
>    - x-sdtrig is added to ISA string
>    - Reversed the patch order
> 
> Changes from v2:
>    - Mark debug property as deprecated and replace internally with sdtrig extension

I'm getting lost in our discussions, but I thought we needed both in case
a machine only implements debug 0.13, since sdtrig is at least 'more than'
debug, even if backwards compatible (which I also wasn't sure was the
case). If, OTOH, QEMU's debug implementation exactly implements sdtrig's
specification, then I'm in favor of deprecating the 'debug' extension.

Thanks,
drew


>    - setting/unsetting debug property shows warning and sets/unsets ext_sdtrig
>    - sdtrig is added to ISA string as RISC-V debug specification is frozen
> 
> Himanshu Chauhan (2):
>   target/riscv: Mark debug property as deprecated
>   target/riscv: Export sdtrig in ISA string
> 
>  target/riscv/cpu.c        | 38 +++++++++++++++++++++++++++++++++++---
>  target/riscv/cpu_cfg.h    |  2 +-
>  target/riscv/cpu_helper.c |  2 +-
>  target/riscv/csr.c        |  2 +-
>  target/riscv/machine.c    |  2 +-
>  5 files changed, 39 insertions(+), 7 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

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

[Anup Patel]

On Thu, Feb 29, 2024 at 8:42 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Feb 29, 2024 at 07:07:43PM +0530, Himanshu Chauhan wrote:
> > All the CPUs may or may not implement the debug triggers (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
> >
> > Changes from v1:
> >    - Replaced the debug property with ext_sdtrig
> >    - Marked it experimenatal by naming it x-sdtrig
> >    - x-sdtrig is added to ISA string
> >    - Reversed the patch order
> >
> > Changes from v2:
> >    - Mark debug property as deprecated and replace internally with sdtrig extension
>
> I'm getting lost in our discussions, but I thought we needed both in case
> a machine only implements debug 0.13, since sdtrig is at least 'more than'
> debug, even if backwards compatible (which I also wasn't sure was the
> case). If, OTOH, QEMU's debug implementation exactly implements sdtrig's
> specification, then I'm in favor of deprecating the 'debug' extension.

The QEMU's debug implementation aligns more with Sdtrig v1.0 specification
because we have mcontrol6 which was not present in debug 0.13

IMO, we should definitely depricate debug 0.13

Regards,
Anup

>
> Thanks,
> drew
>
>
> >    - setting/unsetting debug property shows warning and sets/unsets ext_sdtrig
> >    - sdtrig is added to ISA string as RISC-V debug specification is frozen
> >
> > Himanshu Chauhan (2):
> >   target/riscv: Mark debug property as deprecated
> >   target/riscv: Export sdtrig in ISA string
> >
> >  target/riscv/cpu.c        | 38 +++++++++++++++++++++++++++++++++++---
> >  target/riscv/cpu_cfg.h    |  2 +-
> >  target/riscv/cpu_helper.c |  2 +-
> >  target/riscv/csr.c        |  2 +-
> >  target/riscv/machine.c    |  2 +-
> >  5 files changed, 39 insertions(+), 7 deletions(-)
> >
> > --
> > 2.34.1
> >
> >
>

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

[Alistair Francis]

On Fri, Mar 1, 2024 at 2:20 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Feb 29, 2024 at 8:42 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Thu, Feb 29, 2024 at 07:07:43PM +0530, Himanshu Chauhan wrote:
> > > All the CPUs may or may not implement the debug triggers (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
> > >
> > > Changes from v1:
> > >    - Replaced the debug property with ext_sdtrig
> > >    - Marked it experimenatal by naming it x-sdtrig
> > >    - x-sdtrig is added to ISA string
> > >    - Reversed the patch order
> > >
> > > Changes from v2:
> > >    - Mark debug property as deprecated and replace internally with sdtrig extension
> >
> > I'm getting lost in our discussions, but I thought we needed both in case
> > a machine only implements debug 0.13, since sdtrig is at least 'more than'
> > debug, even if backwards compatible (which I also wasn't sure was the
> > case). If, OTOH, QEMU's debug implementation exactly implements sdtrig's
> > specification, then I'm in favor of deprecating the 'debug' extension.
>
> The QEMU's debug implementation aligns more with Sdtrig v1.0 specification
> because we have mcontrol6 which was not present in debug 0.13

I'm not sure that's exactly the case. I think QEMU implements the
debug 0.13 specification and also the mcontrol6. That's really a bug
that we support mcontrol6, it snuck in.

We can just support both and wrap the mcontrol6 section behind a
sdtrig check. That seems to be the easiest option. That way we can
support the current ratified debug spec and the experimental soon to
be ratified sdtrig and friends spec.

Alistair

>
> IMO, we should definitely depricate debug 0.13
>
> Regards,
> Anup
>
> >
> > Thanks,
> > drew
> >
> >
> > >    - setting/unsetting debug property shows warning and sets/unsets ext_sdtrig
> > >    - sdtrig is added to ISA string as RISC-V debug specification is frozen
> > >
> > > Himanshu Chauhan (2):
> > >   target/riscv: Mark debug property as deprecated
> > >   target/riscv: Export sdtrig in ISA string
> > >
> > >  target/riscv/cpu.c        | 38 +++++++++++++++++++++++++++++++++++---
> > >  target/riscv/cpu_cfg.h    |  2 +-
> > >  target/riscv/cpu_helper.c |  2 +-
> > >  target/riscv/csr.c        |  2 +-
> > >  target/riscv/machine.c    |  2 +-
> > >  5 files changed, 39 insertions(+), 7 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >
> > >
> >
>