* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).