[PATCH v4 0/3] Introduce sdtrig ISA extension

[PATCH v4 0/3] Introduce sdtrig ISA extension

[Himanshu Chauhan]

All the CPUs may or may not implement the debug triggers. Some CPUs
may implement only debug specification v0.13 and not sdtrig ISA
extension.

This patchset, adds sdtrig ISA as an extension which can be turned on or off by
sdtrig=<true/false> option. It is turned off by default.

When debug is true and sdtrig is false, the behaviour is as defined in debug
specification v0.13. If sdtrig is turned on, the behaviour is as defined
in the sdtrig ISA extension.

The "sdtrig" string is concatenated to ISA string when debug or sdtrig is enabled.

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

Changes from v3:
  - debug propery is not deprecated but it is superceded by sdtrig extension
  - Mcontrol6 support is not published when only debug property is turned
    on as debug spec v0.13 doesn't define mcontrol6 match triggers.
  - Enabling sdtrig extension turns of debug property and a warning is printed.
    This doesn't break debug specification implemenation since sdtrig is
    backward compatible with debug specification.
  - Disable debug property and enable sdtrig by default for Ventana's Veyron
    CPUs.

Himanshu Chauhan (3):
  target/riscv: Enable mcontrol6 triggers only when sdtrig is selected
  target/riscv: Expose sdtrig ISA extension
  target/riscv: Enable sdtrig for Ventana's Veyron CPUs

 target/riscv/cpu.c     | 14 ++++++-
 target/riscv/cpu_cfg.h |  1 +
 target/riscv/csr.c     |  2 +-
 target/riscv/debug.c   | 92 +++++++++++++++++++++++++-----------------
 target/riscv/machine.c |  2 +-
 5 files changed, 70 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] target/riscv: Enable mcontrol6 triggers only when sdtrig is selected

[Himanshu Chauhan]

The mcontrol6 triggers are not defined in debug specification v0.13
These triggers are defined in sdtrig ISA extension.

This patch:
   * Adds ext_sdtrig capability which is used to select mcontrol6 triggers
   * Keeps the debug property. All triggers that are defined in v0.13 are
     exposed.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 target/riscv/cpu.c     |  4 +-
 target/riscv/cpu_cfg.h |  1 +
 target/riscv/csr.c     |  2 +-
 target/riscv/debug.c   | 92 +++++++++++++++++++++++++-----------------
 target/riscv/machine.c |  2 +-
 5 files changed, 60 insertions(+), 41 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c160b9216b..2602aae9f5 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1008,7 +1008,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.debug || cpu->cfg.ext_sdtrig) {
         riscv_trigger_reset_hold(env);
     }
 
@@ -1168,7 +1168,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.debug || cpu->cfg.ext_sdtrig) {
         riscv_trigger_realize(&cpu->env);
     }
 #endif
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 2040b90da0..0c57e1acd4 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -114,6 +114,7 @@ struct RISCVCPUConfig {
     bool ext_zvfbfwma;
     bool ext_zvfh;
     bool ext_zvfhmin;
+    bool ext_sdtrig;
     bool ext_smaia;
     bool ext_ssaia;
     bool ext_sscofpmf;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 726096444f..26623d3640 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)->debug || riscv_cpu_cfg(env)->ext_sdtrig) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index e30d99cc2f..c6a92ba0f7 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -100,13 +100,16 @@ static trigger_action_t get_trigger_action(CPURISCVState *env,
     target_ulong tdata1 = env->tdata1[trigger_index];
     int trigger_type = get_trigger_type(env, trigger_index);
     trigger_action_t action = DBG_ACTION_NONE;
+    const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
 
     switch (trigger_type) {
     case TRIGGER_TYPE_AD_MATCH:
         action = (tdata1 & TYPE2_ACTION) >> 12;
         break;
     case TRIGGER_TYPE_AD_MATCH6:
-        action = (tdata1 & TYPE6_ACTION) >> 12;
+        /* Only sdtrig ISA extension supports type 6 match */
+        if (cfg->ext_sdtrig)
+            action = (tdata1 & TYPE6_ACTION) >> 12;
         break;
     case TRIGGER_TYPE_INST_CNT:
     case TRIGGER_TYPE_INT:
@@ -727,7 +730,12 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
         type2_reg_write(env, env->trigger_cur, tdata_index, val);
         break;
     case TRIGGER_TYPE_AD_MATCH6:
-        type6_reg_write(env, env->trigger_cur, tdata_index, val);
+        if (riscv_cpu_cfg(env)->ext_sdtrig) {
+            type6_reg_write(env, env->trigger_cur, tdata_index, val);
+        } else {
+            qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
+                          trigger_type);
+        }
         break;
     case TRIGGER_TYPE_INST_CNT:
         itrigger_reg_write(env, env->trigger_cur, tdata_index, val);
@@ -750,9 +758,15 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
 
 target_ulong tinfo_csr_read(CPURISCVState *env)
 {
-    /* assume all triggers support the same types of triggers */
-    return BIT(TRIGGER_TYPE_AD_MATCH) |
-           BIT(TRIGGER_TYPE_AD_MATCH6);
+    target_ulong ts = 0;
+
+    ts = BIT(TRIGGER_TYPE_AD_MATCH);
+
+    /* sdtrig ISA extension supports type 6 match */
+    if (riscv_cpu_cfg(env)->ext_sdtrig)
+        ts |= BIT(TRIGGER_TYPE_AD_MATCH6);
+
+    return ts;
 }
 
 void riscv_cpu_debug_excp_handler(CPUState *cs)
@@ -803,19 +817,21 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
                 }
                 break;
             case TRIGGER_TYPE_AD_MATCH6:
-                ctrl = env->tdata1[i];
-                pc = env->tdata2[i];
-
-                if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
-                    if (env->virt_enabled) {
-                        /* check VU/VS bit against current privilege level */
-                        if ((ctrl >> 23) & BIT(env->priv)) {
-                            return true;
-                        }
-                    } else {
-                        /* check U/S/M bit against current privilege level */
-                        if ((ctrl >> 3) & BIT(env->priv)) {
-                            return true;
+                if (cpu->cfg.ext_sdtrig) {
+                   ctrl = env->tdata1[i];
+                    pc = env->tdata2[i];
+
+                    if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
+                        if (env->virt_enabled) {
+                            /* check VU/VS bit against current privilege level */
+                            if ((ctrl >> 23) & BIT(env->priv)) {
+                                return true;
+                            }
+                        } else {
+                            /* check U/S/M bit against current privilege level */
+                            if ((ctrl >> 3) & BIT(env->priv)) {
+                                return true;
+                            }
                         }
                     }
                 }
@@ -869,27 +885,29 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
             }
             break;
         case TRIGGER_TYPE_AD_MATCH6:
-            ctrl = env->tdata1[i];
-            addr = env->tdata2[i];
-            flags = 0;
+            if (cpu->cfg.ext_sdtrig) {
+                ctrl = env->tdata1[i];
+                addr = env->tdata2[i];
+                flags = 0;
 
-            if (ctrl & TYPE6_LOAD) {
-                flags |= BP_MEM_READ;
-            }
-            if (ctrl & TYPE6_STORE) {
-                flags |= BP_MEM_WRITE;
-            }
+                if (ctrl & TYPE6_LOAD) {
+                    flags |= BP_MEM_READ;
+                }
+                if (ctrl & TYPE6_STORE) {
+                    flags |= BP_MEM_WRITE;
+                }
 
-            if ((wp->flags & flags) && (wp->vaddr == addr)) {
-                if (env->virt_enabled) {
-                    /* check VU/VS bit against current privilege level */
-                    if ((ctrl >> 23) & BIT(env->priv)) {
-                        return true;
-                    }
-                } else {
-                    /* check U/S/M bit against current privilege level */
-                    if ((ctrl >> 3) & BIT(env->priv)) {
-                        return true;
+                if ((wp->flags & flags) && (wp->vaddr == addr)) {
+                    if (env->virt_enabled) {
+                        /* check VU/VS bit against current privilege level */
+                        if ((ctrl >> 23) & BIT(env->priv)) {
+                            return true;
+                        }
+                    } else {
+                        /* check U/S/M bit against current privilege level */
+                        if ((ctrl >> 3) & BIT(env->priv)) {
+                            return true;
+                        }
                     }
                 }
             }
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 76f2150f78..1cb8656191 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.debug || cpu->cfg.ext_sdtrig);
 }
 
 static int debug_post_load(void *opaque, int version_id)
-- 
2.34.1

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

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

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 (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
         riscv_trigger_reset_hold(env);
     }
@@ -1480,6 +1487,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
     MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
 
+    MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, false),
     MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
     MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
-- 
2.34.1

[PATCH v4 3/3] target/riscv: Enable sdtrig for Ventana's Veyron CPUs

[Himanshu Chauhan]

Ventana's Veyron CPUs support sdtrig ISA extension. By default, enable
the sdtrig extension and disable the debug property for these CPUs.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ab057a0926..9ddebe468b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -568,7 +568,9 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
     cpu->cfg.ext_zicbom = true;
     cpu->cfg.cbom_blocksize = 64;
     cpu->cfg.cboz_blocksize = 64;
+    cpu->cfg.debug=false;
     cpu->cfg.ext_zicboz = true;
+    cpu->cfg.ext_sdtrig = true;
     cpu->cfg.ext_smaia = true;
     cpu->cfg.ext_ssaia = true;
     cpu->cfg.ext_sscofpmf = true;
-- 
2.34.1

Re: [PATCH v4 1/3] target/riscv: Enable mcontrol6 triggers only when sdtrig is selected

[Andrew Jones]

On Wed, Mar 13, 2024 at 11:39:29AM +0530, Himanshu Chauhan wrote:
> The mcontrol6 triggers are not defined in debug specification v0.13
> These triggers are defined in sdtrig ISA extension.
> 
> This patch:
>    * Adds ext_sdtrig capability which is used to select mcontrol6 triggers
>    * Keeps the debug property. All triggers that are defined in v0.13 are
>      exposed.
> 
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c     |  4 +-
>  target/riscv/cpu_cfg.h |  1 +
>  target/riscv/csr.c     |  2 +-
>  target/riscv/debug.c   | 92 +++++++++++++++++++++++++-----------------
>  target/riscv/machine.c |  2 +-
>  5 files changed, 60 insertions(+), 41 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c160b9216b..2602aae9f5 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1008,7 +1008,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.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_reset_hold(env);
>      }
>  
> @@ -1168,7 +1168,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.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_realize(&cpu->env);
>      }
>  #endif
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 2040b90da0..0c57e1acd4 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -114,6 +114,7 @@ struct RISCVCPUConfig {
>      bool ext_zvfbfwma;
>      bool ext_zvfh;
>      bool ext_zvfhmin;
> +    bool ext_sdtrig;
>      bool ext_smaia;
>      bool ext_ssaia;
>      bool ext_sscofpmf;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..26623d3640 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)->debug || riscv_cpu_cfg(env)->ext_sdtrig) {
>          return RISCV_EXCP_NONE;
>      }
>  
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index e30d99cc2f..c6a92ba0f7 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -100,13 +100,16 @@ static trigger_action_t get_trigger_action(CPURISCVState *env,
>      target_ulong tdata1 = env->tdata1[trigger_index];
>      int trigger_type = get_trigger_type(env, trigger_index);
>      trigger_action_t action = DBG_ACTION_NONE;
> +    const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
>  
>      switch (trigger_type) {
>      case TRIGGER_TYPE_AD_MATCH:
>          action = (tdata1 & TYPE2_ACTION) >> 12;
>          break;
>      case TRIGGER_TYPE_AD_MATCH6:
> -        action = (tdata1 & TYPE6_ACTION) >> 12;
> +        /* Only sdtrig ISA extension supports type 6 match */

I'd drop the comment since the if-statement says the same thing.

> +        if (cfg->ext_sdtrig)
> +            action = (tdata1 & TYPE6_ACTION) >> 12;
>          break;
>      case TRIGGER_TYPE_INST_CNT:
>      case TRIGGER_TYPE_INT:
> @@ -727,7 +730,12 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>          type2_reg_write(env, env->trigger_cur, tdata_index, val);
>          break;
>      case TRIGGER_TYPE_AD_MATCH6:
> -        type6_reg_write(env, env->trigger_cur, tdata_index, val);
> +        if (riscv_cpu_cfg(env)->ext_sdtrig) {
> +            type6_reg_write(env, env->trigger_cur, tdata_index, val);
> +        } else {
> +            qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                          trigger_type);
> +        }
>          break;
>      case TRIGGER_TYPE_INST_CNT:
>          itrigger_reg_write(env, env->trigger_cur, tdata_index, val);
> @@ -750,9 +758,15 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>  
>  target_ulong tinfo_csr_read(CPURISCVState *env)
>  {
> -    /* assume all triggers support the same types of triggers */
> -    return BIT(TRIGGER_TYPE_AD_MATCH) |
> -           BIT(TRIGGER_TYPE_AD_MATCH6);
> +    target_ulong ts = 0;
> +
> +    ts = BIT(TRIGGER_TYPE_AD_MATCH);
> +
> +    /* sdtrig ISA extension supports type 6 match */

Also drop this comment.

> +    if (riscv_cpu_cfg(env)->ext_sdtrig)
> +        ts |= BIT(TRIGGER_TYPE_AD_MATCH6);
> +
> +    return ts;
>  }
>  
>  void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -803,19 +817,21 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>                  }
>                  break;
>              case TRIGGER_TYPE_AD_MATCH6:
> -                ctrl = env->tdata1[i];
> -                pc = env->tdata2[i];
> -
> -                if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
> -                    if (env->virt_enabled) {
> -                        /* check VU/VS bit against current privilege level */
> -                        if ((ctrl >> 23) & BIT(env->priv)) {
> -                            return true;
> -                        }
> -                    } else {
> -                        /* check U/S/M bit against current privilege level */
> -                        if ((ctrl >> 3) & BIT(env->priv)) {
> -                            return true;
> +                if (cpu->cfg.ext_sdtrig) {
> +                   ctrl = env->tdata1[i];

Check your whitespace. Missing one space of indent in the line above.

> +                    pc = env->tdata2[i];
> +
> +                    if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
> +                        if (env->virt_enabled) {
> +                            /* check VU/VS bit against current privilege level */
> +                            if ((ctrl >> 23) & BIT(env->priv)) {
> +                                return true;
> +                            }
> +                        } else {
> +                            /* check U/S/M bit against current privilege level */
> +                            if ((ctrl >> 3) & BIT(env->priv)) {
> +                                return true;
> +                            }
>                          }
>                      }
>                  }
> @@ -869,27 +885,29 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>              }
>              break;
>          case TRIGGER_TYPE_AD_MATCH6:
> -            ctrl = env->tdata1[i];
> -            addr = env->tdata2[i];
> -            flags = 0;
> +            if (cpu->cfg.ext_sdtrig) {
> +                ctrl = env->tdata1[i];
> +                addr = env->tdata2[i];
> +                flags = 0;
>  
> -            if (ctrl & TYPE6_LOAD) {
> -                flags |= BP_MEM_READ;
> -            }
> -            if (ctrl & TYPE6_STORE) {
> -                flags |= BP_MEM_WRITE;
> -            }
> +                if (ctrl & TYPE6_LOAD) {
> +                    flags |= BP_MEM_READ;
> +                }
> +                if (ctrl & TYPE6_STORE) {
> +                    flags |= BP_MEM_WRITE;
> +                }
>  
> -            if ((wp->flags & flags) && (wp->vaddr == addr)) {
> -                if (env->virt_enabled) {
> -                    /* check VU/VS bit against current privilege level */
> -                    if ((ctrl >> 23) & BIT(env->priv)) {
> -                        return true;
> -                    }
> -                } else {
> -                    /* check U/S/M bit against current privilege level */
> -                    if ((ctrl >> 3) & BIT(env->priv)) {
> -                        return true;
> +                if ((wp->flags & flags) && (wp->vaddr == addr)) {
> +                    if (env->virt_enabled) {
> +                        /* check VU/VS bit against current privilege level */
> +                        if ((ctrl >> 23) & BIT(env->priv)) {
> +                            return true;
> +                        }
> +                    } else {
> +                        /* check U/S/M bit against current privilege level */
> +                        if ((ctrl >> 3) & BIT(env->priv)) {
> +                            return true;
> +                        }
>                      }
>                  }
>              }
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 76f2150f78..1cb8656191 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.debug || cpu->cfg.ext_sdtrig);

nit: Unnecssary ()

>  }
>  
>  static int debug_post_load(void *opaque, int version_id)
> -- 
> 2.34.1
> 
>

Thanks,
drew

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

[Andrew Jones]

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?

Thanks,
drew

> +    }
> +
>      if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
>          riscv_trigger_reset_hold(env);
>      }
> @@ -1480,6 +1487,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
>      MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>  
> +    MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, false),
>      MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
>      MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> -- 
> 2.34.1
> 
> 

Re: [PATCH v4 3/3] target/riscv: Enable sdtrig for Ventana's Veyron CPUs

[Andrew Jones]

On Wed, Mar 13, 2024 at 11:39:31AM +0530, Himanshu Chauhan wrote:
> Ventana's Veyron CPUs support sdtrig ISA extension. By default, enable
> the sdtrig extension and disable the debug property for these CPUs.
> 
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ab057a0926..9ddebe468b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -568,7 +568,9 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>      cpu->cfg.ext_zicbom = true;
>      cpu->cfg.cbom_blocksize = 64;
>      cpu->cfg.cboz_blocksize = 64;
> +    cpu->cfg.debug=false;

Need spaces around '='

>      cpu->cfg.ext_zicboz = true;
> +    cpu->cfg.ext_sdtrig = true;
>      cpu->cfg.ext_smaia = true;
>      cpu->cfg.ext_ssaia = true;
>      cpu->cfg.ext_sscofpmf = true;
> -- 
> 2.34.1
> 
>

Thanks,
drew

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

[Himanshu Chauhan]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

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.


>
> Thanks,
> drew
>
> > +    }
> > +
> >      if (cpu->cfg.debug || cpu->cfg.ext_sdtrig) {
> >          riscv_trigger_reset_hold(env);
> >      }
> > @@ -1480,6 +1487,7 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_extensions[] = {
> >      MULTI_EXT_CFG_BOOL("zvfhmin", ext_zvfhmin, false),
> >      MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
> >
> > +    MULTI_EXT_CFG_BOOL("sdtrig", ext_sdtrig, false),
> >      MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
> >      MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
> >      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> > --
> > 2.34.1
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 3920 bytes --]

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

[Andrew Jones]

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

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

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

[Andrew Jones]

On Wed, Mar 13, 2024 at 05:48:16PM +0530, Himanshu Chauhan wrote:
...
> >>>> #ifndef CONFIG_USER_ONLY
> >>>> +    if (cpu->cfg.debug && cpu->cfg.ext_sdtrig) {
> >>>> +         warn_report("Disabling debug property since sdtrig ISA
> >>> extension "
> >>>> +                     "is enabled");
> >>>> +         cpu->cfg.debug = 0;
> >>> 
> >>> If sdtrig is a superset of debug, then why do we need to disable debug?
> >>> 
> >> 
> >> "debug" is not disabled. The flag is turned off. This is for unambiguity
> >> between which spec is in force currently.
> >> May come handy (not now but later) in if conditions.
> > 
> > I know sdtrig/debug functionality remains enabled, but why state that the
> > 'debug' functionality is no longer enabled?
> 
> Got it. The warning can be removed.
> 
> > If sdtrig is a superset, then,
> > when it's enabled, both the debug functionality and the sdtrig
> > functionality are enabled. Actually, sdtrig should do the opposite, it
> > should ensure debug=true. The warning would look odd to users that know
> 
> I would disagree to set debug=true when sdtrig is selected. These are two different specifications and should be treated so. Sdtrig is a superset now but may have another revision not backward compatible. For two different specifications and flags should mirror the same. On the same lines, this patch (as it does) should keep (cfg->debug || cfg->sdtrig) which shows that you are dealing with two different specifications. They behave same for some triggers but are still different. Deprecation isn’t a problem now or later.

sdtrig is frozen, otherwise it would require the 'x-' prefix, so it can
no longer change in a substantial way and therefore if it's a superset of
debug now then it always will be. If a change is made to "debug
functionality" then it will get a new extension name which may not be
compatible with either 'debug' or 'sdtrig', however that's not the case
here. If a platform supports 'sdtrig', then it supports 'debug', as
'sdtrig' is a superset of 'debug'. Pretending like they're mutually
exclusive doesn't make sense when they completely overlap. Indeed
platform's will likely *want* to advertise that they are compatible with
both because software that is only compatible with 'debug' will need to
know if it will work or not. A platform that says it's not compatible
with 'debug', when it is, because it supports sdtrig, is limiting the
software that will run on it for no reason.

Thanks,
drew

> 
> > this and the debug=off would also look odd in the results of cpu model
> > expansion. Keeping debug=true would also avoid needing to change all the
> > if cpu->cfg.debug conditions to if cpu->cfg.debug || cpu->cfg.ext_sdtrig.
> > 
> > If we deprecate 'debug' someday, then we'll add a warning for when
> > there is 'debug' explicitly enabled by a user and also for 'debug'
> > configs which lack 'sdtrig', but we don't need to worry about that now.
> > 
> > Thanks,
> > drew
> 
> 

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

[Himanshu Chauhan]

> On 13-Mar-2024, at 6:19 PM, Andrew Jones <ajones@ventanamicro.com> wrote:
> 
> On Wed, Mar 13, 2024 at 05:48:16PM +0530, Himanshu Chauhan wrote:
> ...
>>>>>> #ifndef CONFIG_USER_ONLY
>>>>>> +    if (cpu->cfg.debug && cpu->cfg.ext_sdtrig) {
>>>>>> +         warn_report("Disabling debug property since sdtrig ISA
>>>>> extension "
>>>>>> +                     "is enabled");
>>>>>> +         cpu->cfg.debug = 0;
>>>>> 
>>>>> If sdtrig is a superset of debug, then why do we need to disable debug?
>>>>> 
>>>> 
>>>> "debug" is not disabled. The flag is turned off. This is for unambiguity
>>>> between which spec is in force currently.
>>>> May come handy (not now but later) in if conditions.
>>> 
>>> I know sdtrig/debug functionality remains enabled, but why state that the
>>> 'debug' functionality is no longer enabled?
>> 
>> Got it. The warning can be removed.
>> 
>>> If sdtrig is a superset, then,
>>> when it's enabled, both the debug functionality and the sdtrig
>>> functionality are enabled. Actually, sdtrig should do the opposite, it
>>> should ensure debug=true. The warning would look odd to users that know
>> 
>> I would disagree to set debug=true when sdtrig is selected. These are two different specifications and should be treated so. Sdtrig is a superset now but may have another revision not backward compatible. For two different specifications and flags should mirror the same. On the same lines, this patch (as it does) should keep (cfg->debug || cfg->sdtrig) which shows that you are dealing with two different specifications. They behave same for some triggers but are still different. Deprecation isn’t a problem now or later.
> 
> sdtrig is frozen, otherwise it would require the 'x-' prefix, so it can
> no longer change in a substantial way and therefore if it's a superset of
> debug now then it always will be. If a change is made to "debug
> functionality" then it will get a new extension name which may not be
> compatible with either 'debug' or 'sdtrig', however that's not the case
> here. If a platform supports 'sdtrig', then it supports 'debug', as
> 'sdtrig' is a superset of 'debug'. Pretending like they're mutually
> exclusive doesn't make sense when they completely overlap. Indeed
> platform's will likely *want* to advertise that they are compatible with
> both because software that is only compatible with 'debug' will need to
> know if it will work or not. A platform that says it's not compatible
> with 'debug', when it is, because it supports sdtrig, is limiting the
> software that will run on it for no reason.

Got it. I will make the necessary changes.

> 
> Thanks,
> drew
> 
>> 
>>> this and the debug=off would also look odd in the results of cpu model
>>> expansion. Keeping debug=true would also avoid needing to change all the
>>> if cpu->cfg.debug conditions to if cpu->cfg.debug || cpu->cfg.ext_sdtrig.
>>> 
>>> If we deprecate 'debug' someday, then we'll add a warning for when
>>> there is 'debug' explicitly enabled by a user and also for 'debug'
>>> configs which lack 'sdtrig', but we don't need to worry about that now.
>>> 
>>> Thanks,
>>> drew