[PATCH for-9.1 v3 0/2] target/riscv: set tval in breakpoints

[PATCH for-9.1 v3 0/2] target/riscv: set tval in breakpoints

[Daniel Henrique Barboza]

Hi,

This new version has a change suggested by Richard in v2. No other
changes made.

Changes from v2:
- patch 2:
  - use tcg_constant_tl() instead of loading a temp and doing a
    movi_tl()
- v2 link: https://lore.kernel.org/qemu-riscv/20240416194132.1843699-1-dbarboza@ventanamicro.com/


Daniel Henrique Barboza (2):
  target/riscv/debug: set tval=pc in breakpoint exceptions
  trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

 target/riscv/cpu_helper.c                      | 1 +
 target/riscv/debug.c                           | 3 +++
 target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
 3 files changed, 6 insertions(+)

-- 
2.44.0

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

[Daniel Henrique Barboza]

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

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

[LIU Zhiwei]

On 2024/4/17 7:04, Daniel Henrique Barboza 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>
> ---
>   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;
>                           }
>                       }

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

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

[PATCH for-9.1 v3 2/2] trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

[Daniel Henrique Barboza]

Privileged spec section 4.1.9 mentions:

"When a trap is taken into S-mode, stval is written with
exception-specific information to assist software in handling the trap.
(...)

If stval is written with a nonzero value when a breakpoint,
address-misaligned, access-fault, or page-fault exception occurs on an
instruction fetch, load, or store, then stval will contain the faulting
virtual address."

A similar text is found for mtval in section 3.1.16.

Setting mtval/stval in this scenario is optional, but some softwares read
these regs when handling ebreaks.

Write 'badaddr' in all ebreak breakpoints to write the appropriate
'tval' during riscv_do_cpu_interrrupt().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
index 620ab54eb0..bc5263a4e0 100644
--- a/target/riscv/insn_trans/trans_privileged.c.inc
+++ b/target/riscv/insn_trans/trans_privileged.c.inc
@@ -62,6 +62,8 @@ static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
     if (pre == 0x01f01013 && ebreak == 0x00100073 && post == 0x40705013) {
         generate_exception(ctx, RISCV_EXCP_SEMIHOST);
     } else {
+        tcg_gen_st_tl(tcg_constant_tl(ebreak_addr), tcg_env,
+                      offsetof(CPURISCVState, badaddr));
         generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
     }
     return true;
-- 
2.44.0

Re: [PATCH for-9.1 v3 2/2] trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

[Richard Henderson]

On 4/16/24 16:04, Daniel Henrique Barboza wrote:
> Privileged spec section 4.1.9 mentions:
> 
> "When a trap is taken into S-mode, stval is written with
> exception-specific information to assist software in handling the trap.
> (...)
> 
> If stval is written with a nonzero value when a breakpoint,
> address-misaligned, access-fault, or page-fault exception occurs on an
> instruction fetch, load, or store, then stval will contain the faulting
> virtual address."
> 
> A similar text is found for mtval in section 3.1.16.
> 
> Setting mtval/stval in this scenario is optional, but some softwares read
> these regs when handling ebreaks.
> 
> Write 'badaddr' in all ebreak breakpoints to write the appropriate
> 'tval' during riscv_do_cpu_interrrupt().
> 
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
>   target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-9.1 v3 2/2] trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

[LIU Zhiwei]

On 2024/4/17 7:04, Daniel Henrique Barboza wrote:
> Privileged spec section 4.1.9 mentions:
>
> "When a trap is taken into S-mode, stval is written with
> exception-specific information to assist software in handling the trap.
> (...)
>
> If stval is written with a nonzero value when a breakpoint,
> address-misaligned, access-fault, or page-fault exception occurs on an
> instruction fetch, load, or store, then stval will contain the faulting
> virtual address."
>
> A similar text is found for mtval in section 3.1.16.
>
> Setting mtval/stval in this scenario is optional, but some softwares read
> these regs when handling ebreaks.
>
> Write 'badaddr' in all ebreak breakpoints to write the appropriate
> 'tval' during riscv_do_cpu_interrrupt().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> index 620ab54eb0..bc5263a4e0 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -62,6 +62,8 @@ static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
>       if (pre == 0x01f01013 && ebreak == 0x00100073 && post == 0x40705013) {
>           generate_exception(ctx, RISCV_EXCP_SEMIHOST);
>       } else {
> +        tcg_gen_st_tl(tcg_constant_tl(ebreak_addr), tcg_env,
> +                      offsetof(CPURISCVState, badaddr));

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
>       }
>       return true;

Re: [PATCH for-9.1 v3 2/2] trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

[Alistair Francis]

On Wed, Apr 17, 2024 at 9:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Privileged spec section 4.1.9 mentions:
>
> "When a trap is taken into S-mode, stval is written with
> exception-specific information to assist software in handling the trap.
> (...)
>
> If stval is written with a nonzero value when a breakpoint,
> address-misaligned, access-fault, or page-fault exception occurs on an
> instruction fetch, load, or store, then stval will contain the faulting
> virtual address."
>
> A similar text is found for mtval in section 3.1.16.
>
> Setting mtval/stval in this scenario is optional, but some softwares read
> these regs when handling ebreaks.
>
> Write 'badaddr' in all ebreak breakpoints to write the appropriate
> 'tval' during riscv_do_cpu_interrrupt().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> index 620ab54eb0..bc5263a4e0 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -62,6 +62,8 @@ static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
>      if (pre == 0x01f01013 && ebreak == 0x00100073 && post == 0x40705013) {
>          generate_exception(ctx, RISCV_EXCP_SEMIHOST);
>      } else {
> +        tcg_gen_st_tl(tcg_constant_tl(ebreak_addr), tcg_env,
> +                      offsetof(CPURISCVState, badaddr));
>          generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
>      }
>      return true;
> --
> 2.44.0
>
>

Re: [PATCH for-9.1 v3 0/2] target/riscv: set tval in breakpoints

[Alistair Francis]

On Wed, Apr 17, 2024 at 9:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> This new version has a change suggested by Richard in v2. No other
> changes made.
>
> Changes from v2:
> - patch 2:
>   - use tcg_constant_tl() instead of loading a temp and doing a
>     movi_tl()
> - v2 link: https://lore.kernel.org/qemu-riscv/20240416194132.1843699-1-dbarboza@ventanamicro.com/
>
>
> Daniel Henrique Barboza (2):
>   target/riscv/debug: set tval=pc in breakpoint exceptions
>   trans_privileged.c.inc: set (m|s)tval on ebreak breakpoint

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu_helper.c                      | 1 +
>  target/riscv/debug.c                           | 3 +++
>  target/riscv/insn_trans/trans_privileged.c.inc | 2 ++
>  3 files changed, 6 insertions(+)
>
> --
> 2.44.0
>
>