Re: [RFC][PATCH 1/5] hw-breakpoints: Make kernel breakpoints API truly generic

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Jul 20, 2009 at 01:27:48PM -0400, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@gmail.com) wrote:
> [...]
> > diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> > index c1f64e6..015fec6 100644
> > --- a/kernel/hw_breakpoint.c
> > +++ b/kernel/hw_breakpoint.c
> > @@ -297,15 +297,21 @@ EXPORT_SYMBOL_GPL(unregister_user_hw_breakpoint);
> >  /**
> >   * register_kernel_hw_breakpoint - register a hardware breakpoint for kernel space
> >   * @bp: the breakpoint structure to register
> > - *
> > - * @bp.info->name or @bp.info->address, @bp.info->len, @bp.info->type and
> > + * @addr: the address where we want to set the breakpoint
> > + * @len: length of the value in memory to break in
> > + * @type: the type of the breakpoint (read/write/execute)
> >   * @bp->triggered must be set properly before invocation
> 
> Hi Frederic,
> 
> I think one of the great addition in this patchset is to allow using
> breakpoints from arch-agnostic code.
> 
> It becomes important to document the error values which can be returned
> by register_kernel_hw_breakpoint, so this will serve as guidelines for
> architecture-specific arch_fill_hw_breakpoint() implementation. This
> will become increasingly important, as this abstraction layer will
> basically be responsible for either:
> 
> - Finding the best support the architecture can provide for a given hw
>   breakpoint.


Indeed, and that's the biggest problem it has to face because supported
hardware breakpoint features are very differents from one architecture
to another.


> - Failing with an explicit error value telling the in-kernel user why it
>   failed (e.g. if it must use a fallback, or return the error to the
>   user).


Yeah, nice point, I'll send another iteration which better documents the error
return values, at least once I get a mostly agreed core implementation :-)


> Maybe we should think of a more flexible breakpoint type mapping too,
> e.g.:
> 
>  monitor _strictly_ execute operation on address 0x...
>    -> would fail if the architecture does not support execution access
>       monitoring
>  monitor (at least) execute operations on address 0x...
>    -> would be allowed to use a more general monitor (e.g. RWX) if the
>       architecture does not support "execute only" monitor.
> 
> (same for read and write)
> 
> Mathieu


Well, I'm not sure the problem mostly resides in the hardware implementation
of strict exec breakpoint types. But I guess your point is not limiting to
that. Yeah for example, x86 doesn't support read-only breakpoints.
But I guess that can be simulated using software artifacts, for example using
READ-WRITE breakpoints + the x86 decoder API, recently submitted by Masami,
to find the nature of the current instruction.

Anyway, your point is indeed important: return common error values for unsupported
breakpoint operations.

Thanks.

> 
> >   *
> >   */
> > -int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > +int register_kernel_hw_breakpoint(struct hw_breakpoint *bp, unsigned long addr,
> > +				  int len, enum breakpoint_type type)
> >  {
> >  	int rc;
> >  
> > +	rc = arch_fill_hw_breakpoint(bp, addr, len, type);
> > +	if (rc)
> > +		return rc;
> > +
> >  	rc = arch_validate_hwbkpt_settings(bp, NULL);
> >  	if (rc)
> >  		return rc;
> -- 
> Mathieu Desnoyers
> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68