1. Patchew
  2. QEMU
  3. View series

[PATCH] target/riscv: set tval for triggered watchpoints

Sergey Matyukevich posted 1 patch 1 year, 2 months ago
There is a newer version of this series
target/riscv/cpu_helper.c | 3 +++
target/riscv/debug.c      | 1 +
2 files changed, 4 insertions(+)
[PATCH] target/riscv: set tval for triggered watchpoints
Posted by Sergey Matyukevich 1 year, 2 months ago 
From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

According to priviledged spec, if [sm]tval is written with a nonzero
value when a breakpoint exception occurs, then [sm]tval will contain
the faulting virtual address. Set tval to hit address when breakpoint
exception is triggered by hardware watchpoint.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 target/riscv/cpu_helper.c | 3 +++
 target/riscv/debug.c      | 1 +
 2 files changed, 4 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 9a28816521..d3be8c0511 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1641,6 +1641,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
             tval = env->bins;
             break;
+        case RISCV_EXCP_BREAKPOINT:
+            tval = env->badaddr;
+            break;
         default:
             break;
         }
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index bf4840a6a3..48ef3c59ea 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -761,6 +761,7 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
 
     if (cs->watchpoint_hit) {
         if (cs->watchpoint_hit->flags & BP_CPU) {
+            env->badaddr = cs->watchpoint_hit->hitaddr;
             cs->watchpoint_hit = NULL;
             do_trigger_action(env, DBG_ACTION_BP);
         }
-- 
2.39.0
Re: [PATCH] target/riscv: set tval for triggered watchpoints
Posted by Alistair Francis 1 year, 2 months ago 
On Mon, Jan 30, 2023 at 8:08 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
>
> According to priviledged spec, if [sm]tval is written with a nonzero
> value when a breakpoint exception occurs, then [sm]tval will contain
> the faulting virtual address. Set tval to hit address when breakpoint
> exception is triggered by hardware watchpoint.
>
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/cpu_helper.c | 3 +++
>  target/riscv/debug.c      | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 9a28816521..d3be8c0511 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1641,6 +1641,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
>              tval = env->bins;
>              break;
> +        case RISCV_EXCP_BREAKPOINT:
> +            tval = env->badaddr;
> +            break;
>          default:
>              break;
>          }
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index bf4840a6a3..48ef3c59ea 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -761,6 +761,7 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
>
>      if (cs->watchpoint_hit) {
>          if (cs->watchpoint_hit->flags & BP_CPU) {
> +            env->badaddr = cs->watchpoint_hit->hitaddr;
>              cs->watchpoint_hit = NULL;
>              do_trigger_action(env, DBG_ACTION_BP);
>          }
> --
> 2.39.0
>
>
Re: [PATCH] target/riscv: set tval for triggered watchpoints
Posted by Bin Meng 1 year, 2 months ago 
On Tue, Jan 31, 2023 at 8:59 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jan 30, 2023 at 8:08 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> >
> > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> >
> > According to priviledged spec, if [sm]tval is written with a nonzero
> > value when a breakpoint exception occurs, then [sm]tval will contain
> > the faulting virtual address. Set tval to hit address when breakpoint
> > exception is triggered by hardware watchpoint.
> >
> > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
>
> Thanks!
>
> Applied to riscv-to-apply.next

Oops, too quick, but I have one comment :)

>
> Alistair
>
> > ---
> >  target/riscv/cpu_helper.c | 3 +++
> >  target/riscv/debug.c      | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 9a28816521..d3be8c0511 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1641,6 +1641,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> >          case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
> >              tval = env->bins;
> >              break;
> > +        case RISCV_EXCP_BREAKPOINT:
> > +            tval = env->badaddr;

RISCV_EXCP_BREAKPOINT may come from 'ebreak' so we should test if this
exception comes from the debug module.

The spec also says about icount trigger that:

"If the trigger fires with action =0 then zero is written to the tval
CSR on the breakpoint trap."

So we can't blindly set tval for every breakpoint exception.

> > +            break;
> >          default:
> >              break;
> >          }
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> > index bf4840a6a3..48ef3c59ea 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -761,6 +761,7 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
> >
> >      if (cs->watchpoint_hit) {
> >          if (cs->watchpoint_hit->flags & BP_CPU) {
> > +            env->badaddr = cs->watchpoint_hit->hitaddr;
> >              cs->watchpoint_hit = NULL;
> >              do_trigger_action(env, DBG_ACTION_BP);
> >          }
> > --

Regards,
Bin
Re: [PATCH] target/riscv: set tval for triggered watchpoints
Posted by Sergey Matyukevich 1 year, 2 months ago 
Hi Bin,

> > > According to priviledged spec, if [sm]tval is written with a nonzero
> > > value when a breakpoint exception occurs, then [sm]tval will contain
> > > the faulting virtual address. Set tval to hit address when breakpoint
> > > exception is triggered by hardware watchpoint.
> > >
> > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> >
> > Thanks!
> >
> > Applied to riscv-to-apply.next
> 
> Oops, too quick, but I have one comment :)
> 
> >
> > Alistair
> >
> > > ---
> > >  target/riscv/cpu_helper.c | 3 +++
> > >  target/riscv/debug.c      | 1 +
> > >  2 files changed, 4 insertions(+)
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 9a28816521..d3be8c0511 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -1641,6 +1641,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > >          case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
> > >              tval = env->bins;
> > >              break;
> > > +        case RISCV_EXCP_BREAKPOINT:
> > > +            tval = env->badaddr;
> 
> RISCV_EXCP_BREAKPOINT may come from 'ebreak' so we should test if this
> exception comes from the debug module.
> 
> The spec also says about icount trigger that:
> 
> "If the trigger fires with action =0 then zero is written to the tval
> CSR on the breakpoint trap."
> 
> So we can't blindly set tval for every breakpoint exception.
> 

Thanks for catching ! Initial idea was to set badaddr value only when
it is needed in target/riscv/debug.c. For instance, icount code does
not set badaddr, so tval will remain zero. On the other hand, breakpoint
exception may come from ebreak and badaddr may keep non-zero value from
some previous unrelated exception.

Explicit check should be more safe, e.g. something like that:

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index d3be8c0511..f1a0c65ad3 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1642,7 +1642,10 @@ 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;
+            }
             break;
         default:
             break;
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 48ef3c59ea..b091293069 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -761,8 +761,6 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
 
     if (cs->watchpoint_hit) {
         if (cs->watchpoint_hit->flags & BP_CPU) {
-            env->badaddr = cs->watchpoint_hit->hitaddr;
-            cs->watchpoint_hit = NULL;
             do_trigger_action(env, DBG_ACTION_BP);
         }
     } else {


I will a fixup after testing.

Regards,
Sergey
Re: [PATCH] target/riscv: set tval for triggered watchpoints
Posted by Alistair Francis 1 year, 2 months ago 
On Mon, Jan 30, 2023 at 8:08 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
>
> According to priviledged spec, if [sm]tval is written with a nonzero
> value when a breakpoint exception occurs, then [sm]tval will contain
> the faulting virtual address. Set tval to hit address when breakpoint
> exception is triggered by hardware watchpoint.
>
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 3 +++
>  target/riscv/debug.c      | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 9a28816521..d3be8c0511 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1641,6 +1641,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
>              tval = env->bins;
>              break;
> +        case RISCV_EXCP_BREAKPOINT:
> +            tval = env->badaddr;
> +            break;
>          default:
>              break;
>          }
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index bf4840a6a3..48ef3c59ea 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -761,6 +761,7 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
>
>      if (cs->watchpoint_hit) {
>          if (cs->watchpoint_hit->flags & BP_CPU) {
> +            env->badaddr = cs->watchpoint_hit->hitaddr;
>              cs->watchpoint_hit = NULL;
>              do_trigger_action(env, DBG_ACTION_BP);
>          }
> --
> 2.39.0
>
>
Re: [PATCH] target/riscv: set tval for triggered watchpoints
Posted by Richard Henderson 1 year, 2 months ago 
On 1/30/23 00:07, Sergey Matyukevich wrote:
> From: Sergey Matyukevich<sergey.matyukevich@syntacore.com>
> 
> According to priviledged spec, if [sm]tval is written with a nonzero
> value when a breakpoint exception occurs, then [sm]tval will contain
> the faulting virtual address. Set tval to hit address when breakpoint
> exception is triggered by hardware watchpoint.
> 
> Signed-off-by: Sergey Matyukevich<sergey.matyukevich@syntacore.com>
> ---
>   target/riscv/cpu_helper.c | 3 +++
>   target/riscv/debug.c      | 1 +
>   2 files changed, 4 insertions(+)

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

r~

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.