Message-ID: <20090528111020.GA5640@in.ibm.com>
Date:	Thu, 28 May 2009 16:40:20 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	David Gibson <dwg@....ibm.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin Herrenschmidt <benh@....ibm.com>,
	maneesh@...ux.vnet.ibm.com, Roland McGrath <roland@...hat.com>,
	Masami Hiramatsu <mhiramat@...hat.com>
Subject: Re: [Patch 01/12] Prepare the code for Hardware Breakpoint
	interfaces

On Thu, May 28, 2009 at 03:28:59PM +1000, David Gibson wrote:
> On Mon, May 11, 2009 at 05:22:13PM +0530, K.Prasad wrote:
> > This patch introduces header files containing constants, structure
> > definitions and declaration of functions used by the hardware
> > breakpoint interface code.
> 
> Hrm.  I don't really like splitting prototypes from their
> implementations as you have done.  Patches in a series should be split
> into logical, self-contained units, not just sliced up by file.
> 
> [snip]

The first patch is one that prepares the kernel tree for the other patches
on the stack and still allow compilation of the kernel. This patch could
have been dismantled entirely, while the prototypes and header files moved
along with the corresponding .c files but it would have made the resultant
patch much bigger.

> > Index: include/asm-generic/hw_breakpoint.h
> > ===================================================================
> > --- /dev/null
> > +++ include/asm-generic/hw_breakpoint.h
> > @@ -0,0 +1,139 @@
> > +#ifndef	_ASM_GENERIC_HW_BREAKPOINT_H
> > +#define	_ASM_GENERIC_HW_BREAKPOINT_H
> 
> Is there a reason this is asm-generic, rather than
> linux/hw_breakpoint.h?
> 

None that I know of! It's a 'legacy' that I carried from Alan Stern's
original patchset.

> > +#ifndef __ARCH_HW_BREAKPOINT_H
> > +#error "Please don't include this file directly"
> > +#endif
> > +
> > +#ifdef	__KERNEL__
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +#include <linux/kallsyms.h>
> > +
> > +/**
> > + * struct hw_breakpoint - unified kernel/user-space hardware breakpoint
> > + * @triggered: callback invoked after target address access
> 
> "triggered" seems an odd name to me - it suggests a boolean flag, not
> a function pointer.
>

Re-reading the word 'triggered' and checking its usage in the existing
kernel I realise that it could have been named something better (handler?).
This certainly joins the list of enhancements I intend to make on the
patchset in the future (perhaps after it makes it to the mainline).
 
> > + * @info: arch-specific breakpoint info (address, length, and type)
> > + *
> > + * %hw_breakpoint structures are the kernel's way of representing
> > + * hardware breakpoints.  These are data breakpoints
> > + * (also known as "watchpoints", triggered on data access), and the breakpoint's
> > + * target address can be located in either kernel space or user space.
> 
> I thought this infrastructure could also handle instruction
> breakpoints.  The comment above seems to contradict that.
> 

Agreed, the comment reflects the old state of the patch when user-space
interfaces weren't exported, and HW_BREAKPOINT_EXECUTE was confined only
to ptrace. Now that they are exported, a kernel-module can request a
breakpoint register to watch for 'execute' operations on instructions. I
will update the comment.

> > + * The breakpoint's address, length, and type are highly
> > + * architecture-specific.  The values are encoded in the @info field; you
> > + * specify them when registering the breakpoint.  To examine the encoded
> > + * values use hw_breakpoint_get_{kaddress,uaddress,len,type}(), declared
> > + * below.
> > + *
> > + * The address is specified as a regular kernel pointer (for kernel-space
> > + * breakponts) or as an %__user pointer (for user-space breakpoints).
> > + * With register_user_hw_breakpoint(), the address must refer to a
> > + * location in user space.  The breakpoint will be active only while the
> > + * requested task is running.  Conversely with
> > + * register_kernel_hw_breakpoint(), the address must refer to a location
> > + * in kernel space, and the breakpoint will be active on all CPUs
> > + * regardless of the current task.
> > + *
> > + * The length is the breakpoint's extent in bytes, which is subject to
> > + * certain limitations.  include/asm/hw_breakpoint.h contains macros
> > + * defining the available lengths for a specific architecture.  Note that
> > + * the address's alignment must match the length.  The breakpoint will
> > + * catch accesses to any byte in the range from address to address +
> > + * (length - 1).
> > + *
> > + * The breakpoint's type indicates the sort of access that will cause it
> > + * to trigger.  Possible values may include:
> > + *
> > + * 	%HW_BREAKPOINT_RW (triggered on read or write access),
> > + * 	%HW_BREAKPOINT_WRITE (triggered on write access), and
> > + * 	%HW_BREAKPOINT_READ (triggered on read access).
> > + *
> > + * Appropriate macros are defined in include/asm/hw_breakpoint.h; not all
> > + * possibilities are available on all architectures.  Execute breakpoints
> > + * must have length equal to the special value %HW_BREAKPOINT_LEN_EXECUTE.
> 
> Why do instruction breakpoints have a special length, rather than a
> special type?  Or do they have both?
> 

Both. The len for a execute breakpoint is HW_BREAKPOINT_LEN_EXECUTE and
type HW_BREAKPOINT_EXECUTE.

> > + * When a breakpoint gets hit, the @triggered callback is
> > + * invoked in_interrupt with a pointer to the %hw_breakpoint structure and the
> > + * processor registers.
> > + * Data breakpoints occur after the memory access has taken place.
> > + * Breakpoints are disabled during execution @triggered, to avoid
> > + * recursive traps and allow unhindered access to breakpointed memory.
> > + *
> > + * This sample code sets a breakpoint on pid_max and registers a callback
> > + * function for writes to that variable.  Note that it is not portable
> > + * as written, because not all architectures support HW_BREAKPOINT_LEN_4.
> > + *
> > + * ----------------------------------------------------------------------
> > + *
> > + * #include <asm/hw_breakpoint.h>
> > + *
> > + * struct hw_breakpoint my_bp;
> > + *
> > + * static void my_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
> > + * {
> > + * 	printk(KERN_DEBUG "Inside triggered routine of breakpoint exception\n");
> > + * 	dump_stack();
> > + *  	.......<more debugging output>........
> > + * }
> > + *
> > + * static struct hw_breakpoint my_bp;
> > + *
> > + * static int init_module(void)
> > + * {
> > + *	..........<do anything>............
> > + *	my_bp.info.type = HW_BREAKPOINT_WRITE;
> > + *	my_bp.info.len = HW_BREAKPOINT_LEN_4;
> > + *
> > + *	my_bp.installed = (void *)my_bp_installed;
> > + *
> > + *	rc = register_kernel_hw_breakpoint(&my_bp);
> > + *	..........<do anything>............
> > + * }
> > + *
> > + * static void cleanup_module(void)
> > + * {
> > + *	..........<do anything>............
> > + *	unregister_kernel_hw_breakpoint(&my_bp);
> > + *	..........<do anything>............
> > + * }
> > + *
> > + * ----------------------------------------------------------------------
> > + */
> > +struct hw_breakpoint {
> > +	void (*triggered)(struct hw_breakpoint *, struct pt_regs *);
> > +	struct arch_hw_breakpoint info;
> > +};
> > +
> > +/*
> > + * len and type values are defined in include/asm/hw_breakpoint.h.
> > + * Available values vary according to the architecture.  On i386 the
> > + * possibilities are:
> > + *
> > + *	HW_BREAKPOINT_LEN_1
> > + *	HW_BREAKPOINT_LEN_2
> > + *	HW_BREAKPOINT_LEN_4
> > + *	HW_BREAKPOINT_RW
> > + *	HW_BREAKPOINT_READ
> > + *
> > + * On other architectures HW_BREAKPOINT_LEN_8 may be available, and the
> > + * 1-, 2-, and 4-byte lengths may be unavailable.  There also may be
> > + * HW_BREAKPOINT_WRITE.  You can use #ifdef to check at compile time.
> > + */
> > +
> > +int register_user_hw_breakpoint(struct task_struct *tsk,
> > +					struct hw_breakpoint *bp);
> > +void unregister_user_hw_breakpoint(struct task_struct *tsk,
> > +						struct hw_breakpoint *bp);
> > +/*
> > + * Kernel breakpoints are not associated with any particular thread.
> > + */
> > +int register_kernel_hw_breakpoint(struct hw_breakpoint *bp);
> > +void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
> > +void switch_to_none_hw_breakpoint(void);
> > +
> > +extern unsigned int hbp_kernel_pos;
> > +extern spinlock_t hw_breakpoint_lock;
> > +
> > +#endif	/* __KERNEL__ */
> > +#endif	/* _ASM_GENERIC_HW_BREAKPOINT_H */
> > Index: arch/x86/include/asm/hw_breakpoint.h
> > ===================================================================
> > --- /dev/null
> > +++ arch/x86/include/asm/hw_breakpoint.h
> > @@ -0,0 +1,66 @@
> > +#ifndef	_I386_HW_BREAKPOINT_H
> > +#define	_I386_HW_BREAKPOINT_H
> > +
> > +#ifdef	__KERNEL__
> > +#define	__ARCH_HW_BREAKPOINT_H
> > +
> > +struct arch_hw_breakpoint {
> > +	char		*name; /* Contains name of the symbol to set bkpt */
> > +	unsigned long	address;
> > +	u8		len;
> > +	u8		type;
> > +};
> > +
> > +#include <linux/kdebug.h>
> > +#include <asm-generic/hw_breakpoint.h>
> > +
> > +/* Available HW breakpoint length encodings */
> > +#define HW_BREAKPOINT_LEN_1		0x40
> > +#define HW_BREAKPOINT_LEN_2		0x44
> > +#define HW_BREAKPOINT_LEN_4		0x4c
> > +#define HW_BREAKPOINT_LEN_EXECUTE	0x40
> > +
> > +#ifdef CONFIG_X86_64
> > +#define HW_BREAKPOINT_LEN_8		0x48
> > +#endif
> > +
> > +/* Available HW breakpoint type encodings */
> > +
> > +/* trigger on instruction execute */
> > +#define HW_BREAKPOINT_EXECUTE	0x80
> > +/* trigger on memory write */
> > +#define HW_BREAKPOINT_WRITE	0x81
> > +/* trigger on memory read or write */
> > +#define HW_BREAKPOINT_RW	0x83
> 
> What is the reason for making the encoding of all these values
> arch-specific?  How would you set up the encoding for an arch where
> different CPU variants have different debug facilities (as we do on
> powerpc).
> 

I'm responding to this comment in a reply to this related mail of yours:
http://lkml.org/lkml/2009/5/28/43.

> > +/* Total number of available HW breakpoint registers */
> > +#define HB_NUM 4
> > +
> > +extern struct hw_breakpoint *hbp_kernel[HB_NUM];
> > +DECLARE_PER_CPU(struct hw_breakpoint*, this_hbp_kernel[HB_NUM]);
> > +extern unsigned int hbp_user_refcount[HB_NUM];
> > +
> > +/*
> > + * Ptrace support: breakpoint trigger routine.
> > + */
> > +int __register_user_hw_breakpoint(int pos, struct task_struct *tsk,
> > +			struct hw_breakpoint *bp);
> > +int __modify_user_hw_breakpoint(int pos, struct task_struct *tsk,
> > +			struct hw_breakpoint *bp);
> > +void __unregister_user_hw_breakpoint(int pos, struct task_struct *tsk);
> > +
> > +void arch_install_thread_hw_breakpoint(struct task_struct *tsk);
> > +void arch_uninstall_thread_hw_breakpoint(void);
> > +int arch_check_va_in_userspace(unsigned long va, u8 hbp_len);
> > +int arch_check_va_in_kernelspace(unsigned long va, u8 hbp_len);
> > +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> > +						struct task_struct *tsk);
> > +void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk);
> > +void arch_flush_thread_hw_breakpoint(struct task_struct *tsk);
> > +void arch_update_kernel_hw_breakpoint(void *);
> > +int hw_breakpoint_handler(struct die_args *args);
> > +int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
> > +				     unsigned long val, void *data);
> 
> It looks like most if not all of these functions should have the same
> prototype (though a different implementation) on all archs.  In which
> case the prototypes should go in a generic header.
> 

Some of the functions have different signatures across archs too (like
arch_check_va_in_userspace and arch_check_va_in_kernelspace). While your
comments are based on an older version of the patchset, the recent one
(http://lkml.org/lkml/2009/5/21/143) has a more concise prototype declaration.

> > +#endif	/* __KERNEL__ */
> > +#endif	/* _I386_HW_BREAKPOINT_H */
> > +
> > Index: arch/x86/include/asm/debugreg.h
> > ===================================================================
> > --- arch/x86/include/asm/debugreg.h.orig
> > +++ arch/x86/include/asm/debugreg.h
> > @@ -18,6 +18,7 @@
> >  #define DR_TRAP1	(0x2)		/* db1 */
> >  #define DR_TRAP2	(0x4)		/* db2 */
> >  #define DR_TRAP3	(0x8)		/* db3 */
> > +#define DR_TRAP_BITS	(DR_TRAP0|DR_TRAP1|DR_TRAP2|DR_TRAP3)
> >  
> >  #define DR_STEP		(0x4000)	/* single-step */
> >  #define DR_SWITCH	(0x8000)	/* task switch */
> > @@ -49,6 +50,8 @@
> >  
> >  #define DR_LOCAL_ENABLE_SHIFT 0    /* Extra shift to the local enable bit */
> >  #define DR_GLOBAL_ENABLE_SHIFT 1   /* Extra shift to the global enable bit */
> > +#define DR_LOCAL_ENABLE (0x1)      /* Local enable for reg 0 */
> > +#define DR_GLOBAL_ENABLE (0x2)     /* Global enable for reg 0 */
> >  #define DR_ENABLE_SIZE 2           /* 2 enable bits per register */
> >  
> >  #define DR_LOCAL_ENABLE_MASK (0x55)  /* Set  local bits for all 4 regs */
> > @@ -67,4 +70,32 @@
> >  #define DR_LOCAL_SLOWDOWN (0x100)   /* Local slow the pipeline */
> >  #define DR_GLOBAL_SLOWDOWN (0x200)  /* Global slow the pipeline */
> >  
> > +/*
> > + * HW breakpoint additions
> > + */
> > +#ifdef __KERNEL__
> > +
> > +/* For process management */
> > +void flush_thread_hw_breakpoint(struct task_struct *tsk);
> > +int copy_thread_hw_breakpoint(struct task_struct *tsk,
> > +		struct task_struct *child, unsigned long clone_flags);
> > +void dump_thread_hw_breakpoint(struct task_struct *tsk, int u_debugreg[8]);
> > +void switch_to_thread_hw_breakpoint(struct task_struct *tsk);
> > +
> > +/* For CPU management */
> > +void load_debug_registers(void);
> > +static inline void hw_breakpoint_disable(void)
> > +{
> > +	/* Zero the control register for HW Breakpoint */
> > +	set_debugreg(0UL, 7);
> > +
> > +	/* Zero-out the individual HW breakpoint address registers */
> > +	set_debugreg(0UL, 0);
> > +	set_debugreg(0UL, 1);
> > +	set_debugreg(0UL, 2);
> > +	set_debugreg(0UL, 3);
> > +}
> > +
> > +#endif	/* __KERNEL__ */
> > +
> >  #endif /* _ASM_X86_DEBUGREG_H */
> > Index: arch/x86/include/asm/processor.h
> > ===================================================================
> > --- arch/x86/include/asm/processor.h.orig
> > +++ arch/x86/include/asm/processor.h
> > @@ -29,6 +29,7 @@ struct mm_struct;
> >  #include <linux/threads.h>
> >  #include <linux/init.h>
> >  
> > +#define HB_NUM 4
> 
> Uh.. this #define appears to be duplicated in asm-x86/hw_breakpoint.h
> and here.
> 

While it has been the thinking that there's no suitable header file that
is common to processor.h and other users of HBP_NUM in which this
definition can be made that caused this duplicity, I realise that
debugreg.h could have been used.

I will move the definition of HBP_NUM to arch/x86/include/asm/debugreg.h
and include the same in this file.

> >  /*
> >   * Default implementation of macro that returns current
> >   * instruction pointer ("program counter").
> > @@ -435,8 +436,11 @@ struct thread_struct {
> >  	unsigned long		debugreg1;
> >  	unsigned long		debugreg2;
> >  	unsigned long		debugreg3;
> > +	unsigned long		debugreg[HB_NUM];
> >  	unsigned long		debugreg6;
> >  	unsigned long		debugreg7;
> > +	/* Hardware breakpoint info */
> > +	struct hw_breakpoint	*hbp[HB_NUM];
> >  	/* Fault info: */
> >  	unsigned long		cr2;
> >  	unsigned long		trap_no;
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

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