diff mbox series

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

Message ID 20240314113510.477862-2-hchauhan@ventanamicro.com
State New
Headers show
Series Introduce sdtrig ISA extension | expand

Commit Message

Himanshu Chauhan March 14, 2024, 11:35 a.m. UTC 
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   | 90 +++++++++++++++++++++++++-----------------
 4 files changed, 57 insertions(+), 40 deletions(-)

Comments

Andrew Jones March 14, 2024, 5:07 p.m. UTC | #1 
On Thu, Mar 14, 2024 at 05:05:08PM +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   | 90 +++++++++++++++++++++++++-----------------
>  4 files changed, 57 insertions(+), 40 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) {

I still don't see the point of adding '|| cpu->cfg.ext_sdtrig'. debug must
be true when ext_sdtrig is true.

>          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..674223e966 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -100,13 +100,15 @@ 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;
> +        if (cfg->ext_sdtrig)
> +            action = (tdata1 & TYPE6_ACTION) >> 12;

QEMU requires {}, even for single line blocks. I'm not sure if QEMU's
checkpatch is smart enough to complain about that, but if you haven't
run checkpatch, then you probably should.

>          break;
>      case TRIGGER_TYPE_INST_CNT:
>      case TRIGGER_TYPE_INT:
> @@ -727,7 +729,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 +757,14 @@ 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;

Useless initialization to zero since it's assigned in the next line.
Actually, should just do

  target_ulong ts = BIT(TRIGGER_TYPE_AD_MATCH);

> +
> +    ts = BIT(TRIGGER_TYPE_AD_MATCH);
> +
> +    if (riscv_cpu_cfg(env)->ext_sdtrig)
> +        ts |= BIT(TRIGGER_TYPE_AD_MATCH6);

Need {}

> +
> +    return ts;
>  }
>  
>  void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -803,19 +815,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 +883,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;
> +                        }
>                      }
>                  }
>              }
> -- 
> 2.34.1
>

For the two TRIGGER_TYPE_AD_MATCH6 cases above in
riscv_cpu_debug_check_breakpoint() and riscv_cpu_debug_check_watchpoint()
I'd just put a

 if (!cpu->cfg.ext_sdtrig) {
     break;
 }

at the top of the code, rather than indenting everything. But either way
is fine.

Thanks,
drew
diff mbox series

Patch

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..674223e966 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -100,13 +100,15 @@  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;
+        if (cfg->ext_sdtrig)
+            action = (tdata1 & TYPE6_ACTION) >> 12;
         break;
     case TRIGGER_TYPE_INST_CNT:
     case TRIGGER_TYPE_INT:
@@ -727,7 +729,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 +757,14 @@  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);
+
+    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 +815,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 +883,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;
+                        }
                     }
                 }
             }