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

[PATCH v2 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 x-sdtrig=<true/false> option. It is
turned on by default.

"x-sdtrig" is concatenated to ISA string when it is enabled.
Like so:
rv64imafdch_zicbom_*_x-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

Himanshu Chauhan (2):
  target/riscv: Convert sdtrig functionality from property to an
    extension
  target/riscv: Export sdtrig in ISA string

 target/riscv/cpu.c        | 8 ++++----
 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, 8 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] target/riscv: Convert sdtrig functionality from property to an extension

[Himanshu Chauhan]

The debug trigger (sdtrig) capability is controlled using the debug property.
The sdtrig is an ISA extension and should be treated so. The sdtrig extension
may or may not be implemented in a system. Therefore, it must raise an illegal
instruction exception when it is disabled and its CSRs are accessed.

This patch removes the "debug" property and replaces it with ext_sdtrig extension.
It also raises an illegal instruction exception when the extension is disabled and
its CSRs are accessed.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/cpu.c        | 7 +++----
 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, 7 insertions(+), 8 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b07a76ef6b..c770a7e506 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -909,7 +909,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);
     }
 
@@ -1068,7 +1068,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
@@ -1393,6 +1393,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
 
 /* These are experimental so mark with 'x-' */
 const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
+    MULTI_EXT_CFG_BOOL("x-sdtrig", ext_sdtrig, true),
     MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
     MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
 
@@ -1480,8 +1481,6 @@ Property riscv_cpu_options[] = {
 };
 
 static Property riscv_cpu_properties[] = {
-    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
-
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index f4605fb190..341ebf726a 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -109,6 +109,7 @@ struct RISCVCPUConfig {
     bool ext_zvfbfwma;
     bool ext_zvfh;
     bool ext_zvfhmin;
+    bool ext_sdtrig;
     bool ext_smaia;
     bool ext_ssaia;
     bool ext_sscofpmf;
@@ -145,7 +146,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 e7e23b34f4..3f7c2f1315 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -126,7 +126,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 c50a33397c..8dbb49aa88 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -543,7 +543,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 72fe2374dc..8f9787a30f 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -231,7 +231,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 v2 2/2] target/riscv: Export sdtrig in ISA string

[Himanshu Chauhan]

This patch adds "x-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,x-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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c770a7e506..860e520730 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -153,6 +153,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
     ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
     ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
+    ISA_EXT_DATA_ENTRY(x-sdtrig, PRIV_VERSION_1_12_0, ext_sdtrig),
     ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
     ISA_EXT_DATA_ENTRY(xtheadbb, PRIV_VERSION_1_11_0, ext_xtheadbb),
     ISA_EXT_DATA_ENTRY(xtheadbs, PRIV_VERSION_1_11_0, ext_xtheadbs),
-- 
2.34.1

Re: [PATCH v2 1/2] target/riscv: Convert sdtrig functionality from property to an extension

[Daniel Henrique Barboza]

On 1/17/24 11:24, Himanshu Chauhan wrote:
> The debug trigger (sdtrig) capability is controlled using the debug property.
> The sdtrig is an ISA extension and should be treated so. The sdtrig extension
> may or may not be implemented in a system. Therefore, it must raise an illegal
> instruction exception when it is disabled and its CSRs are accessed.
> 
> This patch removes the "debug" property and replaces it with ext_sdtrig extension.
> It also raises an illegal instruction exception when the extension is disabled and
> its CSRs are accessed.
> 
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>   target/riscv/cpu.c        | 7 +++----
>   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, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..c770a7e506 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -909,7 +909,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);
>       }
>   
> @@ -1068,7 +1068,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
> @@ -1393,6 +1393,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>   
>   /* These are experimental so mark with 'x-' */
>   const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-sdtrig", ext_sdtrig, true),
>       MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
>       MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
>   
> @@ -1480,8 +1481,6 @@ Property riscv_cpu_options[] = {
>   };
>   
>   static Property riscv_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
> -

We can't just get rid of 'debug' out of nowhere. If any user has any scripts that happens
to be using the 'debug' flag QEMU will stop booting for them.

The policy for deprecation is to (1) warn the user that the option is deprecated and
inform about the right option to use and (2) document it in docs/about/deprecated.rst.

You can keep basically everything you did here but, in another patch, you'll need a special
setter() for the 'debug' property that warns about the deprecation and sets ext_sdtrig.
Here's a non-tested diff of what this would look like:

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 8d3ec74a1c..66b5033ce5 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1685,6 +1685,39 @@ static const PropertyInfo prop_pmp = {
      .set = prop_pmp_set,
  };
  
+static void prop_debug_set(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value;
+
+    visit_type_bool(v, name, &value, errp);
+
+    if (cpu->cfg.debug != value && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        return;
+    }
+
+    warn_report("\"debug\" property is deprecated; use \"x-sdtrig\"");
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.debug = value;
+    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.pmp;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_debug = {
+    .name = "debug",
+    .get = prop_debug_get,
+    .set = prop_debug_set,
+};
+
  static int priv_spec_from_str(const char *priv_spec_str)
  {
      int priv_version = -1;
@@ -1942,7 +1975,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 */


An example of a docs/about/deprecated.rst entry would be:

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2e15040246..b7608030a9 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -439,6 +439,10 @@ by a ``pmu-mask`` property. If set of counters is continuous then the mask can
  be calculated with ``((2 ^ n) - 1) << 3``. The least significant three bits
  must be left clear.
  
+``debug=true|false`` on RISC-V CPUs (since 9.0)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The ``debug`` flag was superseded by the ``sdtrig`` extension.
  
  Backwards compatibility
  -----------------------



Thanks,


Daniel






>   #ifndef CONFIG_USER_ONLY
>       DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>   #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..341ebf726a 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -109,6 +109,7 @@ struct RISCVCPUConfig {
>       bool ext_zvfbfwma;
>       bool ext_zvfh;
>       bool ext_zvfhmin;
> +    bool ext_sdtrig;
>       bool ext_smaia;
>       bool ext_ssaia;
>       bool ext_sscofpmf;
> @@ -145,7 +146,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 e7e23b34f4..3f7c2f1315 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -126,7 +126,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 c50a33397c..8dbb49aa88 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -543,7 +543,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 72fe2374dc..8f9787a30f 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -231,7 +231,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)

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

[Andrew Jones]

On Wed, Jan 17, 2024 at 07:54:12PM +0530, Himanshu Chauhan wrote:
> This patch adds "x-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,x-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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c770a7e506..860e520730 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -153,6 +153,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
>      ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
>      ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
> +    ISA_EXT_DATA_ENTRY(x-sdtrig, PRIV_VERSION_1_12_0, ext_sdtrig),
>      ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
>      ISA_EXT_DATA_ENTRY(xtheadbb, PRIV_VERSION_1_11_0, ext_xtheadbb),
>      ISA_EXT_DATA_ENTRY(xtheadbs, PRIV_VERSION_1_11_0, ext_xtheadbs),
> -- 
> 2.34.1
> 
>

We don't want the 'x-' part to show up in the ISA string. isa_edata_arr[]
should get an entry without x-, and the x- property should be added to
riscv_cpu_experimental_exts[].

Thanks,
drew

Re: [PATCH v2 1/2] target/riscv: Convert sdtrig functionality from property to an extension

[Anup Patel]

On Wed, Jan 17, 2024 at 7:54 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> The debug trigger (sdtrig) capability is controlled using the debug property.
> The sdtrig is an ISA extension and should be treated so. The sdtrig extension
> may or may not be implemented in a system. Therefore, it must raise an illegal
> instruction exception when it is disabled and its CSRs are accessed.
>
> This patch removes the "debug" property and replaces it with ext_sdtrig extension.
> It also raises an illegal instruction exception when the extension is disabled and
> its CSRs are accessed.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c        | 7 +++----
>  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, 7 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..c770a7e506 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -909,7 +909,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);
>      }
>
> @@ -1068,7 +1068,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
> @@ -1393,6 +1393,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>
>  /* These are experimental so mark with 'x-' */
>  const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-sdtrig", ext_sdtrig, true),

Drop the "x-" because Sdtrig is already frozen and public review has started.

>      MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
>      MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
>
> @@ -1480,8 +1481,6 @@ Property riscv_cpu_options[] = {
>  };
>
>  static Property riscv_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
> -
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..341ebf726a 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -109,6 +109,7 @@ struct RISCVCPUConfig {
>      bool ext_zvfbfwma;
>      bool ext_zvfh;
>      bool ext_zvfhmin;
> +    bool ext_sdtrig;
>      bool ext_smaia;
>      bool ext_ssaia;
>      bool ext_sscofpmf;
> @@ -145,7 +146,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 e7e23b34f4..3f7c2f1315 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -126,7 +126,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 c50a33397c..8dbb49aa88 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -543,7 +543,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 72fe2374dc..8f9787a30f 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -231,7 +231,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
>
>

Regards,
Anup

Re: [PATCH v2 1/2] target/riscv: Convert sdtrig functionality from property to an extension

[Alistair Francis]

On Thu, Jan 18, 2024 at 12:25 AM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> The debug trigger (sdtrig) capability is controlled using the debug property.
> The sdtrig is an ISA extension and should be treated so. The sdtrig extension
> may or may not be implemented in a system. Therefore, it must raise an illegal
> instruction exception when it is disabled and its CSRs are accessed.
>
> This patch removes the "debug" property and replaces it with ext_sdtrig extension.
> It also raises an illegal instruction exception when the extension is disabled and
> its CSRs are accessed.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c        | 7 +++----
>  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, 7 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..c770a7e506 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -909,7 +909,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);
>      }
>
> @@ -1068,7 +1068,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
> @@ -1393,6 +1393,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
>
>  /* These are experimental so mark with 'x-' */
>  const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-sdtrig", ext_sdtrig, true),
>      MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
>      MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
>
> @@ -1480,8 +1481,6 @@ Property riscv_cpu_options[] = {
>  };
>
>  static Property riscv_cpu_properties[] = {
> -    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),

We can't do this. We have users of the debug spec already. It is a
ratified spec, we can't drop support for it either.

The debug working group has really backed us into a corner here. The
simplest approach is probably to just implement sdtrig (debug 1.0) and
the original debug (0.13) separately and support both. Then users can
enable which one they prefer

Alistair

> -
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..341ebf726a 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -109,6 +109,7 @@ struct RISCVCPUConfig {
>      bool ext_zvfbfwma;
>      bool ext_zvfh;
>      bool ext_zvfhmin;
> +    bool ext_sdtrig;
>      bool ext_smaia;
>      bool ext_ssaia;
>      bool ext_sscofpmf;
> @@ -145,7 +146,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 e7e23b34f4..3f7c2f1315 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -126,7 +126,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 c50a33397c..8dbb49aa88 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -543,7 +543,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 72fe2374dc..8f9787a30f 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -231,7 +231,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
>
>