Re: [Patch 1/1] PPC64-HWBKPT: Implement hw-breakpoints for PPC64

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Feb 23, 2010 at 04:27:15PM +0530, K.Prasad wrote:
> On Mon, Feb 22, 2010 at 06:47:46PM +0530, K.Prasad wrote:
> > On Sun, Feb 21, 2010 at 02:01:37AM +0100, Frederic Weisbecker wrote:
> > > On Mon, Feb 15, 2010 at 11:29:14AM +0530, K.Prasad wrote:
> [snipped]
> > > Also, do you think addr/len/type is enough to abstract out
> > > any ppc breakpoints?
> > > 
> > > This looks enough to me to express range breakpoints and
> > > simple breakpoints. But what about value comparison?
> > > (And still, there may be other trickier implementations
> > > I don't know in ppc).
> > > 
> > 
> > The above implementation is for PPC64 architecture that supports only
> > 'simple' breakpoints of fixed length (no range breakpoints, no value
> > comparison). More on that below.
> >
> 
> Looks like I forgot the 'more on that below' part :-)....here are some
> thoughts...
> 
> Architectures like PPC Book-E have support for a variety of
> sophisticated debug features and our generic framework, in its present
> form, cannot easily port itself to these processors. In order to extend
> the framework for PPC Book-E, I intend the following to begin with:
> 
> - Implement support for data breakpoints through DAC registers with all
>   the 'bells and whistles'...support for instruction breakpoints through
>   IAC can come in later (without precluding its use through ptrace).
> 
> - Embed the flags/variables to store DVC, masked address mode, etc. in
>   'struct arch_hw_breakpoint', which will be populated by the user of
>   register_breakpoint interface.



Agreed.



> 
> Apart from the above extensions to the framework, changes in the generic
> code would be required as described in an earlier LKML mail (ref:
> message-id: 20091127190705.GB18408@in.ibm.com)....relevant contents
> pasted below:
> 
> "I think the register_<> interfaces can become wrappers around functions
> that do the following:
> 
> - arch_validate(): Validate request by invoking an arch-dependant
>   routine. Proceed if returned valid.
> - arch-specific debugreg availability: Do something like
>   if (arch_hw_breakpoint_availabile())
>         bp = perf_event_create_kernel_counter();



This is already what does register_hw_break....(), it fails
if a slot is not available:

perf_event_create_kernel_counter -> perf_bp_init() -> reserve_bp_slot()

Having a:

if (arch_hw_breakpoint_availabile())
         bp = perf_event_create_kernel_counter();

would be racy.



> 
>   perf_event_create_kernel_counter()--->arch_install_hw_breakpoint();
> 
> This way, all book-keeping related work (no. of pinned/flexible/per-cpu)
> will be moved to arch-specific files (will be helpful for PPC Book-E
> implementation having two types of debug registers). Every new
> architecture that intends to port to the new hw-breakpoint
> implementation must define their arch_validate(),
> arch_hw_breakpoint_available() and an arch_install_hw_breakpoint(),
> while the hw-breakpoint code will be flexible enough to extend itself to
> each of these archs."
> 
> Let me know what you think of the above.



We certainly need the slot reservation in arch (a part of it at least).
But we also need a kind of new interface for arch predefined attributes,
instead of generic attributes.

Probably we need a kind of perf_event_create_kernel_counter() that
can accept either a perf_event_attr (for perf syscall or ftrace)
and an arch structure that can be passed to the breakpoint API,
so that we don't need the generic translation.


> 
> Thanks,
> K.Prasad
>