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

[Bin Meng]

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