[8/9] target/riscv: debug: Return 0 if previous value written to tselect >= number of triggers

Commit Message

Frank Chang June 10, 2022, 5:13 a.m. UTC

From: Frank Chang <[e-mail]>

If the value written to tselect is greater than or equal to the number
of supported triggers, then the following reads of tselect would return
value 0.

Signed-off-by: Frank Chang <[e-mail]>
---
 target/riscv/cpu.h   | 1 +
 target/riscv/debug.c | 6 ++++++
 2 files changed, 7 insertions(+)

Comments

Bin Meng June 15, 2022, 1:17 p.m. UTC | #1

On Fri, Jun 10, 2022 at 1:24 PM <[e-mail]> wrote:
>
> From: Frank Chang <[e-mail]>
>
> If the value written to tselect is greater than or equal to the number
> of supported triggers, then the following reads of tselect would return
> value 0.

Where is this behavior documented?

>
> Signed-off-by: Frank Chang <[e-mail]>
> ---
>  target/riscv/cpu.h   | 1 +
>  target/riscv/debug.c | 6 ++++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index bac5f00722..c7ee3f80e6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -289,6 +289,7 @@ struct CPUArchState {
>
>      /* trigger module */
>      target_ulong trigger_cur;
> +    target_ulong trigger_prev;
>      target_ulong tdata1[RV_MAX_TRIGGERS];
>      target_ulong tdata2[RV_MAX_TRIGGERS];
>      target_ulong tdata3[RV_MAX_TRIGGERS];
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index ce9ff15d75..83b72fa1b9 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -158,6 +158,10 @@ bool tdata_available(CPURISCVState *env, int tdata_index)
>
>  target_ulong tselect_csr_read(CPURISCVState *env)
>  {
> +    if (env->trigger_prev >= RV_MAX_TRIGGERS) {
> +        return 0;
> +    }
> +
>      return env->trigger_cur;
>  }
>
> @@ -166,6 +170,8 @@ void tselect_csr_write(CPURISCVState *env, target_ulong val)
>      if (val < RV_MAX_TRIGGERS) {
>          env->trigger_cur = val;
>      }
> +
> +    env->trigger_prev = val;
>  }
>
>  static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
> --

The spec mentions "implementations which have 2^n triggers only need
to implement n bits of tselect", so in QEMU we can always implement
2^n triggers and have tselect implement just n bit.

In such way, writing tselect can be: env->trigger_cur = val &
(RV_MAX_TRIGGERS - 1).

and I believe you can squash this patch into patch 4 "target/riscv:
debug: Restrict the range of tselect value can be written" because in
patch 4 you changed the actual tselect range while the original
implementation allowed all bits to be set.

Regards,
Bin

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bac5f00722..c7ee3f80e6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -289,6 +289,7 @@  struct CPUArchState {
 
     /* trigger module */
     target_ulong trigger_cur;
+    target_ulong trigger_prev;
     target_ulong tdata1[RV_MAX_TRIGGERS];
     target_ulong tdata2[RV_MAX_TRIGGERS];
     target_ulong tdata3[RV_MAX_TRIGGERS];
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index ce9ff15d75..83b72fa1b9 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -158,6 +158,10 @@  bool tdata_available(CPURISCVState *env, int tdata_index)
 
 target_ulong tselect_csr_read(CPURISCVState *env)
 {
+    if (env->trigger_prev >= RV_MAX_TRIGGERS) {
+        return 0;
+    }
+
     return env->trigger_cur;
 }
 
@@ -166,6 +170,8 @@  void tselect_csr_write(CPURISCVState *env, target_ulong val)
     if (val < RV_MAX_TRIGGERS) {
         env->trigger_cur = val;
     }
+
+    env->trigger_prev = val;
 }
 
 static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,