Re: [1/2 PATCH] Kprobes: watchpoint probes debug registerallocation interface

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Wed, 2005-07-20 at 02:52, Prasanna S Panchamukhi wrote:
> Hi All,
> 
> As per our discussions, I am reposting the kernel watchpoint
> probes patches. It can be cleanly applied over 2.6.13-rc3-mm1.
> 
> 
> Your comments are welcome.
> 
> Thanks
> Prasanna
> 
> This patch provides debug register allocation mechanism.
> Useful for debuggers like IOW, kgdb, kdb, kernel watchpoint.

You should include this description in the help text for "config
DEBUGREG".

...
>  
> +config DEBUGREG
> +	bool "Global Debug Registers"
> +	depends on DEBUG_KERNEL
> +	default off
> +	help
> +	  Global debug register settings need to be enabled by the
> +	  debuggers using it.

This isn't a very helpful description.  See above.

...
> +struct debugreg dr_list[DR_MAX];

DR_MAX (= 4) is kind of a misnomer.  The max address-register number
is 3.

> +unsigned long dr7_global_mask = 0;
> +static spinlock_t dr_lock = SPIN_LOCK_UNLOCKED;
> +static DEFINE_IDR(debugreg_idr);
> +static spinlock_t debugreg_idr_lock = SPIN_LOCK_UNLOCKED;
> +
> +static inline void set_dr7_global_mask(int regnum)
> +{

There are several functions like this where you use a case statement
but an array reference seems more appropriate.  E.g.,

static unsigned long dr7_global_bits[] = {
	DR7_DR0_BITS, DR7_DR1_BITS, DR7_DR2_BITS, DR7_DR3_BITS
};
#define DR_IS_ADDR(regnum) (0xf & (1 << (regnum)))	// dr0-dr3

	if (DR_IS_ADDR(regnum))
		dr7_global_mask |= dr7_global_bits[regnum];

This type of generalization is even more worthy of consideration
when we consider x86_64, whose debug-register architecture is
upward-compatible with i386, but also defines address registers
dr8-dr15 in addition to dr0-dr3.  (However, I don't know of any
chips that actually implement dr8-dr15.)

> +	switch (regnum) {
> +	case 0:
> +		dr7_global_mask |= DR7_DR0_BITS;
> +		break;
> +	case 1:
> +		dr7_global_mask |= DR7_DR1_BITS;
> +		break;
> +	case 2:
> +		dr7_global_mask |= DR7_DR2_BITS;
> +		break;
> +	case 3:
> +		dr7_global_mask |= DR7_DR3_BITS;
> +		break;
> +	}
> +	return;

This return is superfluous.

> +}
> +
> +static inline void clear_dr7_global_mask(int regnum)
> +{

See above.  This could be
	if (DR_IS_ADDR(regnum))
		dr7_global_mask &= ~dr7_global_bits[regnum];

> +	switch (regnum) {
> +	case 0:
> +		dr7_global_mask &= ~DR7_DR0_BITS;
> +		break;
> +	case 1:
> +		dr7_global_mask &= ~DR7_DR1_BITS;
> +		break;
> +	case 2:
> +		dr7_global_mask &= ~DR7_DR2_BITS;
> +		break;
> +	case 3:
> +		dr7_global_mask &= ~DR7_DR3_BITS;
> +		break;
> +	}
> +	return;

This return is superfluous.

> +}
> +
> +/*
> + * See if specific debug register is free.
> + */
> +static int specific_debugreg(unsigned int regnum)
> +{
> +	int r, n;
> +
> +	if (regnum >= DR_MAX)
> +		return -EINVAL;
> +
> +	spin_lock(&debugreg_idr_lock);
> +

I know Andi Kleen suggested using lib/idr.c, but it's not clear to
me why it's needed here.

> +	if (idr_find(&debugreg_idr, regnum)) {
> +		r = -EBUSY;
> +		goto out;
> +	}
> +
> +	r = idr_pre_get(&debugreg_idr, GFP_KERNEL);

This doesn't look right.  idr_pre_get() could sleep, but you're
holding the debugreg_idr_lock.

...
> +void dr_inc_use_count(unsigned long mask)

"mask" is not a very helpful name.  This is the contents of dr7, right?
Ditto for dr_dec_use_count().

...
> @@ -713,14 +722,22 @@ struct task_struct fastcall * __switch_t
>  	/*
>  	 * Now maybe reload the debug registers
>  	 */
> -	if (unlikely(next->debugreg[7])) {
> -		set_debugreg(current->thread.debugreg[0], 0);
> -		set_debugreg(current->thread.debugreg[1], 1);
> -		set_debugreg(current->thread.debugreg[2], 2);
> -		set_debugreg(current->thread.debugreg[3], 3);
> -		/* no 4 and 5 */
> +	/*
> +	 * Don't reload global debug registers. Don't touch the global debug
> +	 * register settings in dr7.
> +	 */
> +	if (unlikely(next_dr7)) {
> +		if (prev->debugreg[0] != next->debugreg[0])
> +			set_debugreg(current->thread.debugreg[0], 0);
> +		if (prev->debugreg[1] != next->debugreg[1])
> +			set_debugreg(current->thread.debugreg[1], 1);
> +		if (prev->debugreg[2] != next->debugreg[2])
> +			set_debugreg(current->thread.debugreg[2], 2);
> +		if (prev->debugreg[3] != next->debugreg[3])
> +			set_debugreg(current->thread.debugreg[3], 3);

The above should be converted to a for(...) loop.

> +  		/* no 4 and 5 */
>  		set_debugreg(current->thread.debugreg[6], 6);
> -		set_debugreg(current->thread.debugreg[7], 7);
> +		load_process_dr7(next_dr7);
>  	}
...

A comment would be nice here.  For example, why say "num<<1" instead
of "num*2"?  The compiler will optimize the * into a <<.  What fields
of dr7 is this looking at?  Why isn't "dr" called "dr7"?  Why isn't
"num" called "regnum" as elsewhere?

> +#define DR_IS_LOCAL(dr, num) ((dr) & (1UL << (num <<1)))

...
> +
> +static inline unsigned long read_dr(int regnum)
> +{
> +	unsigned long val = 0;
> +	switch (regnum) {
> +	case 0:
> +		get_dr(0, val);
> +		break;
> +	case 1:
> +		get_dr(1, val);
> +		break;
> +	case 2:
> +		get_dr(2, val);
> +		break;
> +	case 3:
> +		get_dr(3, val);
> +		break;
> +	case 6:
> +		get_dr(6, val);
> +		break;
> +	case 7:
> +		get_dr(7, val);
> +		break;
> +	}
> +	return val;
> +}

The above is another situation where a case statement is inappropriate.
You could do

#define DR_IS_READABLE(regnum) (0xcf & (1 << (regnum))) // dr0-3, 6, 7
	if (DR_IS_READABLE(regnum))
		get_dr(regnum, val);

or at least
	case 0:
	case 1:
	...
	case 7:
		get_dr(regnum, val);
> +
> +#undef get_dr
> +
> +#define set_dr(regnum, val) set_debugreg(val, regnum)
> +
> +static inline void write_dr(int regnum, unsigned long val)
> +{
> +	switch (regnum) {
> +	case 0:
> +		set_dr(0, val);
> +		break;
> +	case 1:
> +		set_dr(1, val);
> +		break;
> +	case 2:
> +		set_dr(2, val);
> +		break;
> +	case 3:
> +		set_dr(3, val);
> +		break;
> +	case 7:
> +		set_dr(7, val);
> +		break;
> +	}
> +	return;

Superfluous return.

> +}

Similarly to previous function, you could do

#define DR_IS_WRITABLE(regnum) (0x8f & (1 << (regnum))) // dr0-3, 7
	if (DR_IS_WRITABLE(regnum))
		set_dr(regnum, val);

Also, I notice that in some functions you call printk() if the
regnum is out of range, and in others you don't.  Is there a reason
behind this?  (I can see you not doing it for the static functions,
but doing it for the extern/exported functions.)

Patch 2/2 looks fine, except change
"This enables kernel-space watchpoints using processor's debug..."
to
"This enables kernel-space watchpoints using the processor's debug..."

I look forward to seeing these features incorporated into the mainline.
Jim