Message-ID: <20090129070527.GA24496@in.ibm.com>
Date:	Thu, 29 Jan 2009 12:35:27 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Roland McGrath <roland@...hat.com>, akpm@...ux-foundation.org,
	mingo@...e.hu, richardj_moore@...ibm.com, naren@...ux.vnet.ibm.com
Subject: Re: [RFC Patch 0/9] Hardware Breakpoint interfaces - v4

On Fri, Jan 23, 2009 at 04:37:20PM +0530, K.Prasad wrote:
> On Thu, Jan 22, 2009 at 10:42:48AM -0500, Alan Stern wrote:
> > On Thu, 22 Jan 2009, K.Prasad wrote:
> > 
> > > Hi All,
> > > 	Please find the new set of patches that introduce kernel
> > > interfaces to use Hardware Breakpoint registers and an implementation
> > > for x86 (and x86_64) architecture, now labelled as Version IV.
> > > 
> > > The patches are based on 2.6.29-rc2 and have been tested to work fine on
> > > x86 architecture. Some of the major changes over the previous version of
> > > patches include:
> > > i) A sample kernel module that uses Data Hardware breakpoint over a
> > > kernel variable to monitor write operations. See
> > > samples/hw_breakpoint/data_breakpoint.c.
> > > ii) Changes in the register_kernel_hw_breakpoint() interface to accept
> > > only one parameter - pointer to a 'hw_breakpoint' structure which will
> > > contain all relevant data members.
> > 
> > I see some problems with this immediately.  Firstly, as a result of
> > this change the sample code in include/asm-generic/hw_breakpoint.h is
> > now incorrect.  You need to change the comments when you change the
> > code.
> > 
> > Second, your arch_store_info() routine is broken.  It checks for a
> > kernel symbol name even if when a userspace breakpoint is being
> > registered.  You should move that check up into validate_settings().
> > 
> > As a related matter, why did you put the "name" field into the
> > arch-dependent part?  Clearly it should be arch-independent.
> > 
> > Alan Stern
> > 
> 
> The user-space breakpoint requests face an issue due to removal of
> HW_BREAKPOINT_EXECUTE support, since ptrace was made to service these
> requests through the __register_user_hw_breakpoint() interface.
> 
> Through offline discussions, the suggestion gathered was to let ptrace
> process these requests for instruction watchpoint while maintaining a
> coherent view on the number of available registers.
> 
> I will send out a patch that refactors ptrace to handle instruction
> watchpoint requests (until we get it to support through our interface),
> which should also contain changes to the corresponding comments and
> 'name' field.
> 
> Thanks,
> K.Prasad
>

I choose to retain the "name" field in the arch-dependent structure. For
one, it keeps all related things like address, type and symbol name
grouped together, and secondly it allows individual arch implementations
the choice to allow "name" based look-up when unsuitable (even for x86
it wouldn't be suitable for I/O breakpoints). Let me know if you still
find the placement of this member unpalatable!

And as said earlier, the user-space was broken not because of an issue
with atch_store_info() but due to an issue with 'ptrace' interfacing
with the proposed infrastructure. All user-space requests would have the
address field populated and hence the 'kallsyms_lookup_name()' branch in
arch_store_info() wouldn't be taken.

I would be sending the patches that have the user-space issue fixed
shortly.

Thanks,
K.Prasad

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

Powered by blists - more mailing lists