Re: [PATCH for-9.1 v3 1/2] target/riscv/debug: set tval=pc in breakpoint exceptions

[Alistair Francis]

On Wed, Apr 17, 2024 at 9:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We're not setting (s/m)tval when triggering breakpoints of type 2
> (mcontrol) and 6 (mcontrol6). According to the debug spec section
> 5.7.12, "Match Control Type 6":
>
> "The Privileged Spec says that breakpoint exceptions that occur on
> instruction fetches, loads, or stores update the tval CSR with either
> zero or the faulting virtual address. The faulting virtual address for
> an mcontrol6 trigger with action = 0 is the address being accessed and
> which caused that trigger to fire."
>
> A similar text is also found in the Debug spec section 5.7.11 w.r.t.
> mcontrol.
>
> Note that what we're doing ATM is not violating the spec, but it's
> simple enough to set mtval/stval and it makes life easier for any
> software that relies on this info.
>
> Given that we always use action = 0, save the faulting address for the
> mcontrol and mcontrol6 trigger breakpoints into env->badaddr, which is
> used as as scratch area for traps with address information. 'tval' is
> then set during riscv_cpu_do_interrupt().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 1 +
>  target/riscv/debug.c      | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index fc090d729a..f9c6d7053b 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1717,6 +1717,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>              tval = env->bins;
>              break;
>          case RISCV_EXCP_BREAKPOINT:
> +            tval = env->badaddr;
>              if (cs->watchpoint_hit) {
>                  tval = cs->watchpoint_hit->hitaddr;
>                  cs->watchpoint_hit = NULL;
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index e30d99cc2f..b110370ea6 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -798,6 +798,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>                  if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
>                      /* check U/S/M bit against current privilege level */
>                      if ((ctrl >> 3) & BIT(env->priv)) {
> +                        env->badaddr = pc;
>                          return true;
>                      }
>                  }
> @@ -810,11 +811,13 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>                      if (env->virt_enabled) {
>                          /* check VU/VS bit against current privilege level */
>                          if ((ctrl >> 23) & BIT(env->priv)) {
> +                            env->badaddr = pc;
>                              return true;
>                          }
>                      } else {
>                          /* check U/S/M bit against current privilege level */
>                          if ((ctrl >> 3) & BIT(env->priv)) {
> +                            env->badaddr = pc;
>                              return true;
>                          }
>                      }
> --
> 2.44.0
>
>