[PATCH RFC v2 3/3] riscv: hw-breakpoints: add more trigger controls

[Andrew Bresticker]

On Mon, Dec 5, 2022 at 3:25 PM Sergey Matyukevich <geomatsi at gmail.com> wrote:
>
> Hi Andrew,
>
>
> > Hi Sergey,
> >
> > On Sat, Dec 3, 2022 at 4:55 PM Sergey Matyukevich <geomatsi at gmail.com> wrote:
> > >
> > > From: Sergey Matyukevich <sergey.matyukevich at syntacore.com>
> > >
> > > RISC-V SBI Debug Trigger extension proposal defines multiple functions
> > > to control debug triggers. The pair of install/uninstall functions was
> > > enough to implement ptrace interface for user-space debug. This patch
> > > implements kernel wrappers for start/update/stop SBI functions. The
> > > intended users of these control wrappers are kgdb and kernel modules
> > > that make use of kernel-wide hardware breakpoints and watchpoints.
> > >
> > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich at syntacore.com>
> >
> > <snip>
> >
> > > diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> > > index 5bb3b55cd464..afc59f8e034e 100644
> > > --- a/arch/riscv/include/asm/hw_breakpoint.h
> > > +++ b/arch/riscv/include/asm/hw_breakpoint.h
> > > @@ -137,6 +137,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> > >  int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
> > >                                     unsigned long val, void *data);
> > >
> > > +void arch_enable_hw_breakpoint(struct perf_event *bp);
> > > +void arch_update_hw_breakpoint(struct perf_event *bp);
> > > +void arch_disable_hw_breakpoint(struct perf_event *bp);
> > >  int arch_install_hw_breakpoint(struct perf_event *bp);
> > >  void arch_uninstall_hw_breakpoint(struct perf_event *bp);
> > >  void hw_breakpoint_pmu_read(struct perf_event *bp);
> > > @@ -153,5 +156,17 @@ static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > >  {
> > >  }
> > >
> > > +void arch_enable_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> > > +
> > > +void arch_update_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> > > +
> > > +void arch_disable_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> >
> > I don't see any references to {enable,update,disable}_hw_breakpoint in
> > common kernel code, nor do any other architectures define these
> > functions. Who are the intended users? Do we even need them for kgdb
> > on RISC-V?
>
> SBI Debug Trigger extension proposal defines more functions than just
> install/uninstall required for ptrace hw-breakpoints API. So this patch
> is an attempt to expose some additional SBI features to the rest of the
> kernel.

Got it. IMHO it's generally best not to add dead code unless a user
for it is imminent. It's harder to review something if you don't know
exactly how it'll be used.

> For instance, I have been using these stop/update/start functions for
> managing kernel-wide hw-breakpoints when experimenting with a sample
> module from samples/hw_breakpoint. It can be convenient to modify a
> breakpoint or watchpoint when it fires, e.g. to perform single-step
> before proceeding execution. Full-fledged unregister/register cycle is
> not suitable for this task. And this is where disable/update/enable
> sequence can help.
>
> I haven't yet tried to implement hw-breakpoint support in RISC-V kgdb,
> so I don't know for sure if these custom calls are needed there.
>
> > > diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> > > index 8eddf512cd03..ca7df02830c2 100644
> > > --- a/arch/riscv/kernel/hw_breakpoint.c
> > > +++ b/arch/riscv/kernel/hw_breakpoint.c
> > > @@ -2,6 +2,7 @@
> > >
> > >  #include <linux/hw_breakpoint.h>
> > >  #include <linux/perf_event.h>
> > > +#include <linux/spinlock.h>
> > >  #include <linux/percpu.h>
> > >  #include <linux/kdebug.h>
> > >
> > > @@ -9,6 +10,8 @@
> > >
> > >  /* bps/wps currently set on each debug trigger for each cpu */
> > >  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
> > > +static DEFINE_PER_CPU(unsigned long, msg_lock_flags);
> > > +static DEFINE_PER_CPU(raw_spinlock_t, msg_lock);
> >
> > I'm not sure I understand the point of this per-CPU spinlock. Just
> > disable preemption (and interrupts, if necessary).
> >
> > Also, arch_{install,uninstall}_hw_breakpoint are already expected to
> > be called from atomic context; is the intention here that they be
> > callable from outside atomic context?
>
> These additional locsk are not needed for install/uninstall pair due to
> the reason that you mentioned: those calls are called by kernel event
> core in atomic context with ctx->lock held. These locks are needed only
> to make sure that new 'arch_update_hw_breakpoint' function can use the
> same message buffers as install/uninstall.

Sure, but you can achieve the same by disabling preemption. No need
for an actual spinlock.

-Andrew

>
> Regards,
> Sergey