Re: [GIT PULL v6] hw-breakpoints: Rewrite on top of perf events v6

[K.Prasad]

On Thu, Nov 26, 2009 at 06:59:05AM +0100, Frederic Weisbecker wrote:
> On Tue, Nov 24, 2009 at 06:51:27PM +0530, K.Prasad wrote:
> > On Tue, Nov 24, 2009 at 11:13:42AM +0100, Ingo Molnar wrote:
> > > 
> > > * K.Prasad <prasad@xxxxxxxxxxxxxxxxxx> wrote:
> > > 
<snipped>
> > > > - Proposed migration of register allocation logic to arch-specific 
> > > > files from kernel/hw_breakpoint.c. This is best done early to help 
> > > > easy porting of code to other architectures (we have an active 
> > > > interest in bringing support for PPC64 and S390). If done later, it 
> > > > will entail additional effort in porting for each architecture.
> > > 
> > > I think the general direction should be towards librarized common 
> > > frameworks.
> > > 
> > > If an architecture wants to do something special it should either extend 
> > > the core code, or, if it's too weird to be added to the core, override 
> > > it via its own implementation.
> > > 
> > Given the feeling that the generic set of constraints in the re-written
> > kernel/hw_breakpoint.c cannot accommodate the needs of various
> > processors (LKML ref:20091117013959.GG5293@nowher) and that
> > the register allocation logic should move to arch-specific code, it is
> > best done early to help easy porting for other archs. For instance
> > there's already a port to PPC64 against the layered hw-breakpoint
> > (found here: 20090903183930.GA4590@xxxxxxxxxx) and one from the
> > community for SH (20091018062558.GA20535@xxxxxxxxxxxx).
> > 
> > If such code migration is done while porting of a new architecture, then
> > it involves making changes to every other arch on which it is previously
> > implemented (or workaround using #ifdef).
> 
> As I said, we can probably workaround it by keeping the most part
> in the generic code and delegate special arch things to arch
> constraints.
> 

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();

  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.

This implementation would be even superior (in terms of extensibility)
to even the older hw-breakpoint layer implementation (despite it providing
a working layer for x86 and PPC64).

> > > > - Fix ptrace bugs that potentially alter the semantics of ptrace.
> > > 
> > > Is there a specific list of these bugs?
> > > 
> > 
> > As pointed out in 20091111130207.GA5676@xxxxxxxxxx and
> > 20091112042502.GA3145@xxxxxxxxxx, ptrace requests can a) lose register
> > slots when modifying the breakpoint addresses and b) new implementation
> > assumes that every DR7 write to be preceded by a write on DR0-DR3 which
> > need not be true.
> 
> The a) case is going to be fixed.
> But the b) situation must be reported as a user mistake (which is what is
> done currently): -EINVAL, -EIO or whatever. Enabling a breakpoint without
> having given an address is a userland bug.
> 

b) need not be a user mistake always (except perhaps the first time). As I
mentioned here 20091112042502.GA3145@xxxxxxxxxx, DR7 enable/disable
without a DR0-DR3 write can be done by the user through ptrace for
optimising the number of write operations (and hence ptrace syscalls).

Consider the following steps which is entirely valid (in mainline ptrace)
but which would fail if assumed that a DR0-DR3 write precedes a DR7 write:
i) Set address on DR0
ii) Enable bits corresponding to DR0 in DR7
iii) Disable DR0 bits in DR7
iv) Re-enable DR0 bits in DR7

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/