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

[Frederic Weisbecker]

On Tue, Jul 21, 2009 at 04:45:17PM +0530, K.Prasad wrote:
> On Mon, Jul 20, 2009 at 01:08:03PM -0400, Frederic Weisbecker wrote:
> > To define a kernel hardware breakpoint, one need to define the
> > address, type and length of the breakpoint using arch specific
> > operations and then register it using a core helper.
> > 
> > The first stage is truly not scalable with respect to the number of
> > archictures, because for each of them that support hardware
> > breakpoints, we would need a seperate specific field definition for
> > the breakpoint.
> > 
> > However, the supported breakpoint functionalities may be very different
> > between architectures.
> > Then this new API tries to compose with the following constraints:
> > 
> > - a given architecture may perhaps not support the triggering on one
> >   of the usual memory access (read-write/read/write/execute)
> > 
> > - a given architecture may perhaps not support the ability to trigger
> >   a breakpoint only on specific memory access size lower than the word
> >   size for this arch.
> > 
> > - a given architecture may perhaps not support breakpoints on addresses
> >   range.
> > 
> > The new API changes the following prototype for a kernel breakpoint
> > registration:
> > 
> >  int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > 
> > into:
> > 
> >  int register_kernel_hw_breakpoint(struct hw_breakpoint *bp,
> >                                    unsigned long addr,
> > 				   int len, enum breakpoint_type type)
> 
> It is not clear how adding these new parameters to the interface would
> help it become generic, as opposed to moving them to 'struct
> hw_breakpoint'.
> 
> It would make the usage cumbersome of some architectures - say for
> instance the PPC64 which always has a breakpoint length of 8 bytes. So
> the user needs to specify either '8' always or '0' to indicate variable
> length not supported (but it is counter-intuitive..may be interpreted as
> zero-length).


Hm, so may be we should only push the target and the type and set the most
intuitive access length automatically? ie: 1 for x86 so that it triggers
for every accesses, and 8 for ppc?

Also how is actually interpreted an 8 bytes access on ppc? Ie: is a single
byte access actually interpreted through an 8 byte access because of memory
access constraints?


 
> > 
> > The choice of passing the breakpoint settings as parameters of the
> > registration helper and not by adding generic fields into the breakpoint
> > structure is motivated by the need of a very specific per arch
> > representation of the breakpoint:
> > 
> > - the arch may only need an address, but could also need a couple for
> >   breakpoints in ranges.
> > - the type is subject to arch interpretation (values of debug registers)
> > - the length too.
> > 
> > Then, to get back these values from a generic breakpoint structure that have
> > specific encodings into the arch fields, this API comes along with abstract
> > accessors which implementation is arch specific:
> > 
> > - type hw_breakpoint_type(bp)
> > - addr hw_breakpoint_addr(bp)
> > 
> > However, open debates come along this RFC patch:
> > 
> > - the address could be a generic field in struct hw_breakpoint. If we
> >   are dealing with a range breakpoint, then we would just need to
> >   compute addr + length to get the end of the range.
> > 
> > - the length and type could also be generic fields of
> >   struct hw_breakpoint. It would then be up to the arch to get a
> >   translation between such generic values and per arch needs.
> >
> 
> While the issues have been enumerated above, the patchset only pushes
> the issue into a different domain i.e. make the user determine if a
> breakpoint type or len is supported in a given architecture vs the existing
> implementation in which the user determines if a constant pertaining to
> a given len/type is defined. But the accessor-routines
> hw_breakpoint_type() and hw_breakpoint_addr() make it much easier to use
> and is a good addition.
> 
> To make the usage much easier, I would see a combination of the
> following:
> 
> - Define constants/enums for length and type that are common to all
>   architectures.


Indeed, in the hope that most basic types of breakpoints are supported
by every architectures (x and rw).


> - Define accessor routines that help determine if a given type/len is
>   supported on the host processor.


Or may be we should forget about the length as stated above?

But indeed we can think about an arch routine that reports the supported
types, that would provide a better granularity of error message than getting
a "non implemented" error while registering a breakpoint.


> - Move fields such as address, len and type to generic breakpoint
>   structure (if it still matters despite the two changes above).


Ok, I guess that would indeed be better to do that than using abstract
accessors. Let the arch interpret these generic informations by itself.

Thanks!


> Let me know what you think.
> 
> Thanks,
> K.Prasad
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/