1. Patchew
  2. QEMU
  3. View series

[PATCH for-9.0] target/riscv/debug: set tval=pc in breakpoint exceptions

Daniel Henrique Barboza posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240320093221.220854-1-dbarboza@ventanamicro.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/cpu_helper.c | 1 +
target/riscv/debug.c      | 3 +++
2 files changed, 4 insertions(+)
[PATCH for-9.0] target/riscv/debug: set tval=pc in breakpoint exceptions
Posted by Daniel Henrique Barboza 1 month, 1 week ago 
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.

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().

Reported-by: Joseph Chan <jchan@ventanamicro.com>
Fixes: b5f6379d13 ("target/riscv: debug: Implement debug related TCGCPUOps")
Fixes: c472c142a7 ("target/riscv: debug: Add initial support of type 6 trigger")
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 ce7322011d..492ca63b1a 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.0] target/riscv/debug: set tval=pc in breakpoint exceptions
Posted by Alistair Francis 1 month, 1 week ago 
On Wed, Mar 20, 2024 at 7:33 PM 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."

Setting zero is valid though. I agree it's probably worth setting a
value, but I don't think we need this for 9.0

Alistair

>
> A similar text is also found in the Debug spec section 5.7.11 w.r.t.
> mcontrol.
>
> 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().
>
> Reported-by: Joseph Chan <jchan@ventanamicro.com>
> Fixes: b5f6379d13 ("target/riscv: debug: Implement debug related TCGCPUOps")
> Fixes: c472c142a7 ("target/riscv: debug: Add initial support of type 6 trigger")
> 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 ce7322011d..492ca63b1a 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.0] target/riscv/debug: set tval=pc in breakpoint exceptions
Posted by Daniel Henrique Barboza 2 weeks, 4 days ago 

On 3/22/24 00:59, Alistair Francis wrote:
> On Wed, Mar 20, 2024 at 7:33 PM 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."
> 
> Setting zero is valid though. I agree it's probably worth setting a
> value, but I don't think we need this for 9.0

In fact, when fixing another related bug I learned that this except I mentioned
from the debug spec doesn't tell the whole story. The section for mtval, priv
spec 3.1.16, says:

"When a trap is taken into M-mode, mtval is either set to zero or written with
exception-specific information to assist software in handling the trap. Otherwise,
mtval is never written by the implementation (...)"


So "mtval" zero is valid. However the section for stval, priv spec 4.1.9, says:

"When a trap is taken into S-mode, stval is written with exception-specific information
to assist software in handling the trap. Otherwise, stval is never written by the
implementation, though it may be explicitly written by software. The hardware platform
will specify which exceptions must set stval informatively and which may unconditionally
set it to zero."

This a bit more cryptic to me. It doesn't mention "it's either zero or written", giving
the impression that we must set it to a valid addr. But then in the end it follows up
with  "The hardware platform will specify which exceptions must set stval informatively
and which may unconditionally set it to zero.". So I'm not sure whether stval can be
zero or not. I *think* zero is kind of valid ...

Due to the apparent ambiguity I can't claim that this patch, and another patch I'm about
to send that kind of does the same thing, is 9.0 material. I'll send both as 9.1.


Thanks,

Daniel




> 
> Alistair
> 
>>
>> A similar text is also found in the Debug spec section 5.7.11 w.r.t.
>> mcontrol.
>>
>> 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().
>>
>> Reported-by: Joseph Chan <jchan@ventanamicro.com>
>> Fixes: b5f6379d13 ("target/riscv: debug: Implement debug related TCGCPUOps")
>> Fixes: c472c142a7 ("target/riscv: debug: Add initial support of type 6 trigger")
>> 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 ce7322011d..492ca63b1a 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
>>
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.