Re: [RFC PATCH v1] riscv: support for hardware breakpoints/watchpoints

[Sergey Matyukevich <geomatsi@gmail.com>]

On Mon, Nov 07, 2022 at 11:19:59PM +0530, Anup Patel wrote:
> On Mon, Nov 7, 2022 at 10:55 PM Andrew Bresticker <abrestic@rivosinc.com> wrote:
> >
> > On Mon, Nov 7, 2022 at 11:06 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Mon, Nov 7, 2022 at 8:21 PM Andrew Bresticker <abrestic@rivosinc.com> wrote:
> > > >
> > > > On Sat, Nov 5, 2022 at 5:10 AM Anup Patel <anup@brainfault.org> wrote:
> > > > >
> > > > > On Sat, Nov 5, 2022 at 3:07 AM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> > > > > >
> > > > > > Hi Andrew,
> > > > > >
> > > > > > > > RISC-V Debug specification includes Sdtrig ISA extension. This extension
> > > > > > > > describes Trigger Module. Triggers can cause a breakpoint exception,
> > > > > > > > entry into Debug Mode, or a trace action without having to execute a
> > > > > > > > special instruction. For native debugging triggers can be used to
> > > > > > > > implement hardware breakpoints and watchpoints.
> > > > > >
> > > > > > ... [snip]
> > > > > >
> > > > > > > > Despite missing userspace debug, initial implementation can be tested
> > > > > > > > on QEMU using kernel breakpoints, e.g. see samples/hw_breakpoint and
> > > > > > > > register_wide_hw_breakpoint. Hardware breakpoints work on upstream QEMU.
> > > > > > >
> > > > > > > We should also be able to enable the use of HW breakpoints (and
> > > > > > > watchpoints, modulo the issue mentioned below) in kdb, right?
> > > > > >
> > > > > > Interesting. So far I didn't think about using hw breakpoints in kgdb.
> > > > > > I took a quick look at riscv and arm64 kgdb code. It looks like there
> > > > > > is nothing wrong in adding arch-specific implementation of the function
> > > > > > 'kgdb_arch_set_breakpoint' that will use hw breakpoints if possible.
> > > > > > Besides it looks like in this case it makes sense to handle KGDB earlier
> > > > > > than hw breakpoints in do_trap_break.
> > > > > >
> > > > > > > > However this is not the case for watchpoints since there is no way to
> > > > > > > > figure out which watchpoint is triggered. IIUC there are two possible
> > > > > > > > options for doing this: using 'hit' bit in tdata1 or reading faulting
> > > > > > > > virtual address from STVAL. QEMU implements neither of them. Current
> > > > > > > > implementation opts for STVAL. So the following experimental QEMU patch
> > > > > > > > is required to make watchpoints work:
> > > > > > > >
> > > > > > > > :  diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > > > > > :  index 278d163803..8858be7411 100644
> > > > > > > > :  --- a/target/riscv/cpu_helper.c
> > > > > > > > :  +++ b/target/riscv/cpu_helper.c
> > > > > > > > :  @@ -1639,6 +1639,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> > > > > > > > :           case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
> > > > > > > > :               tval = env->bins;
> > > > > > > > :               break;
> > > > > > > > :  +        case RISCV_EXCP_BREAKPOINT:
> > > > > > > > :  +            tval = env->badaddr;
> > > > > > > > :  +            env->badaddr = 0x0;
> > > > > > > > :  +            break;
> > > > > > > > :           default:
> > > > > > > > :               break;
> > > > > > > > :           }
> > > > > > > > :  diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> > > > > > > > :  index 26ea764407..b4d1d566ab 100644
> > > > > > > > :  --- a/target/riscv/debug.c
> > > > > > > > :  +++ b/target/riscv/debug.c
> > > > > > > > :  @@ -560,6 +560,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);
> > > > > > > > :           }
> > > > > >
> > > > > > ... [snip]
> > > > > >
> > > > > > > > +int arch_install_hw_breakpoint(struct perf_event *bp)
> > > > > > > > +{
> > > > > > > > +       struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> > > > > > > > +       struct sbi_dbtr_data_msg *xmit;
> > > > > > > > +       struct sbi_dbtr_id_msg *recv;
> > > > > > > > +       struct perf_event **slot;
> > > > > > > > +       struct sbiret ret;
> > > > > > > > +       int err = 0;
> > > > > > > > +
> > > > > > > > +       xmit = kzalloc(SBI_MSG_SZ_ALIGN(sizeof(*xmit)), GFP_ATOMIC);
> > > > > > > > +       if (!xmit) {
> > > > > > > > +               err = -ENOMEM;
> > > > > > > > +               goto out;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       recv = kzalloc(SBI_MSG_SZ_ALIGN(sizeof(*recv)), GFP_ATOMIC);
> > > > > > > > +       if (!recv) {
> > > > > > > > +               err = -ENOMEM;
> > > > > > > > +               goto out;
> > > > > > > > +       }
> > > > > > >
> > > > > > > Do these really need to be dynamically allocated?
> > > > > >
> > > > > > According to SBI extension proposal, base address of this memory chunk
> > > > > > must be 16-bytes aligned. To simplify things, buffer with 'power of two
> > > > > > bytes' size (and >= 16 bytes) is allocated. In this case alignment of
> > > > > > the kmalloc buffer is guaranteed to be at least this size. IIUC more
> > > > > > efforts are needed to guarantee such alignment for a buffer on stack.
> > > >
> > > > You should be able to declare the struct with __aligned(16) to get the
> > > > desired alignment on the stack.
> > > >
> > > > > Stack is not appropriate for this. Please use a per-CPU global
> > > > > data for this purpose which should be 16 byte aligned as well.
> > > >
> > > > Is the desire to not use the stack purely a defensive measure, i.e. to
> > > > defend against a buggy or malicious firmware/hypervisor? That's fine,
> > > > I'm just curious if there's rationale beyond that (though I'd argue
> > > > we're already implicitly trusting whatever software is sitting below
> > > > us).
> > >
> > > The kernel stack is fixed size so it is best to avoid large structures
> > > or arrays on kernel stack.
> >
> > Of course, though I'm not sure I'd consider 32 bytes "large" :)
> 
> The current patch only configures one trigger at a time but if
> we are configuring multiple triggers in one-go then the trigger
> array will grow.

Existing architecture-specific calls (e.g. arch_install_hw_breakpoint)
handle a single breakpoint. IIUC we may have to setup multiple triggers
at once for masked watchpoints. A masked watchpoint watches many
addresses simultanously, e.g. see https://sourceware.org/gdb/onlinedocs/gdb/Set-Watchpoints.html.
Masked watchpoints can be converted into chained triggers which would
require configuring multiple triggers in one-go.

Do you have any other use-cases in mind ?

Regards,
Sergey