* [PATCH] target/riscv: set tval for triggered watchpoints
@ 2023-01-30 10:07 Sergey Matyukevich
2023-01-30 19:10 ` Richard Henderson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sergey Matyukevich @ 2023-01-30 10:07 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Sergey Matyukevich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: set tval for triggered watchpoints
2023-01-30 10:07 [PATCH] target/riscv: set tval for triggered watchpoints Sergey Matyukevich
@ 2023-01-30 19:10 ` Richard Henderson
2023-01-31 0:19 ` Alistair Francis
2023-01-31 0:58 ` Alistair Francis
2 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2023-01-30 19:10 UTC (permalink / raw)
To: Sergey Matyukevich, qemu-riscv, qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Sergey Matyukevich
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~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: set tval for triggered watchpoints
2023-01-30 10:07 [PATCH] target/riscv: set tval for triggered watchpoints Sergey Matyukevich
2023-01-30 19:10 ` Richard Henderson
@ 2023-01-31 0:19 ` Alistair Francis
2023-01-31 0:58 ` Alistair Francis
2 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2023-01-31 0:19 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: qemu-riscv, qemu-devel, Palmer Dabbelt, Alistair Francis,
Bin Meng, Sergey Matyukevich
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: set tval for triggered watchpoints
2023-01-30 10:07 [PATCH] target/riscv: set tval for triggered watchpoints Sergey Matyukevich
2023-01-30 19:10 ` Richard Henderson
2023-01-31 0:19 ` Alistair Francis
@ 2023-01-31 0:58 ` Alistair Francis
2023-01-31 1:52 ` Bin Meng
2 siblings, 1 reply; 6+ messages in thread
From: Alistair Francis @ 2023-01-31 0:58 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: qemu-riscv, qemu-devel, Palmer Dabbelt, Alistair Francis,
Bin Meng, Sergey Matyukevich
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: set tval for triggered watchpoints
2023-01-31 0:58 ` Alistair Francis
@ 2023-01-31 1:52 ` Bin Meng
2023-01-31 7:58 ` Sergey Matyukevich
0 siblings, 1 reply; 6+ messages in thread
From: Bin Meng @ 2023-01-31 1:52 UTC (permalink / raw)
To: Alistair Francis
Cc: Sergey Matyukevich, qemu-riscv, qemu-devel, Palmer Dabbelt,
Alistair Francis, Bin Meng, Sergey Matyukevich,
Richard Henderson
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target/riscv: set tval for triggered watchpoints
2023-01-31 1:52 ` Bin Meng
@ 2023-01-31 7:58 ` Sergey Matyukevich
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Matyukevich @ 2023-01-31 7:58 UTC (permalink / raw)
To: Bin Meng
Cc: Alistair Francis, qemu-riscv, qemu-devel, Palmer Dabbelt,
Alistair Francis, Bin Meng, Sergey Matyukevich,
Richard Henderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-31 7:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-30 10:07 [PATCH] target/riscv: set tval for triggered watchpoints Sergey Matyukevich
2023-01-30 19:10 ` Richard Henderson
2023-01-31 0:19 ` Alistair Francis
2023-01-31 0:58 ` Alistair Francis
2023-01-31 1:52 ` Bin Meng
2023-01-31 7:58 ` Sergey Matyukevich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.