Re: [Patch 01/11] Introducing generic hardware breakpoint handler interfaces

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Sat, Mar 21, 2009 at 05:39:59PM -0400, Alan Stern wrote:
> On Sat, 21 Mar 2009, K.Prasad wrote:
> 
> > > > + * Kernel breakpoints grow downwards, starting from HB_NUM
> > > > + * 'hbkpt_kernel_pos' denotes lowest numbered breakpoint register occupied for
> > > > + * kernel-space request
> > > > + */
> > > > +unsigned int hbkpt_kernel_pos;
> > > 
> > > This doesn't make much sense.  All you need to know is which registers
> > > are in use; all others are available.
> > > 
> > 
> > As explained by Maneesh earlier, we compact the kernel-space requests
> > into registers (HB_NUM - 1) to hbkpt_kernel_pos. The kernel-space
> > requests aren't specific to any given register number too, and so
> > compaction would be suitable for this case (unlike when implemented for
> > user-space which might need virtualisation of registers).
> 
> Okay, that makes sense.  Perhaps you could add a short comment here
> explaining that the register allocations get compacted when a kernel
> breakpoint is unregistered, so they will always be contiguous.
> 
> > > It's also a poor choice of name.  Everywhere else (in my patches,
> > > anyway) the code refers to hardware breakpoints using the abbreviation
> > > "hwbp" or "hw_breakpoint".  There's no reason suddenly to start using
> > > "hbkpt".
> > > 
> > 
> > I began using 'hbkpt' as a shorter naming convention (the longer one
> > being hw_breakpoint) without being really conscious of the 'hwbkpt'
> > usage by you (even some of the previous iterations contained them in
> > samples/hw_breakpoint and ftrace-plugin).
> > 
> > Well, I will rename my shorter name to 'hwbkpt' for uniformity.
> 
> My patch never used "hwbkpt".  Besides "hw_breakpoint", it used only 
> "bp".  On going back and checking, I found that it didn't even use 
> "hwbp".  Some other abbreviations it did use were "kbp" for kernel 
> breakpoint, "chbi" for per-CPU hardware breakpoint info, and "thbi" for 
> per-thread hardware breakpoint info.
> 
> If you're looking for a good short name, and if you want to keep 
> hardware breakpoints distinct from software breakpoints, I suggest 
> "hbp" instead of "hbkpt".  But it's up to you, and it's worth noticing 
> that the code already contains lots of variables named just "bp".
> 

I am renaming all 'hbkpt' strings to 'hbp'.

> > > > +/* One higher than the highest counted user-space breakpoint register */
> > > > +unsigned int hbkpt_user_max;
> > > 
> > > Likewise, this variable isn't really needed.  It's just one more than
> > > the largest i such that hbkpt_user_max_refcount[i] > 0.
> > > 
> > 
> > It acts like a cache for determining the user-space breakpoint boundary.
> > It is used for sanity checks and in its absence we will have to compute from
> > hbkpt_user_max_refcount[] everytime.
> 
> That's right.  Isn't it simpler to check
> 
> 	kernel_pos > 0 && hbkpt_user_refcount[kernel_pos - 1] == 0
> 
> than to check
> 
> 	kernel_pos - 1 >= hbkpt_user_max
> 
> _and_ to keep hbkpt_user_max set to the correct value at all times?
>

Unfortunately the lines of code required to maintain the variable comes
close to the amount of lines it would potentially save. I will change to
code to compute it from hbkpt_user_refcount[] everytime.
 
> > > > +/*
> > > > + * Load the debug registers during startup of a CPU.
> > > > + */
> > > > +void load_debug_registers(void)
> > > > +{
> > > > +	int i;
> > > > +	unsigned long flags;
> > > > +
> > > > +	/* Prevent IPIs for new kernel breakpoint updates */
> > > > +	local_irq_save(flags);
> > > > +
> > > > +	for (i = hbkpt_kernel_pos; i < HB_NUM; i++)
> > > > +		if (hbkpt_kernel[i])
> > > > +			on_each_cpu(arch_install_kernel_hbkpt,
> > > > +				(void *)hbkpt_kernel[i], 0);
> > > 
> > > This is completely wrong.  First of all, it's dumb to send multiple
> > > IPIs (one for each iteration through the loop).  Second, this routine
> > > shouldn't send any IPIs at all!  It gets invoked when a CPU is
> > > starting up and wants to load its _own_ debug registers -- not tell
> > > another CPU to load anything.
> > > 
> > 
> > As I agreed before, it is an overkill (given the design of
> > arch_install_kernel_hbkpt()). I will create a new
> > arch_update_kernel_hbkpt(pos, bp) that will install breakpoints only
> > on the CPU starting up.
> 
> Doesn't arch_install_kernel_hbkpt() already install breakpoints
> on only the current CPU?  So why do you need a new function?
>

There will be a few more changes to arch_install_kernel_hbkpt() along
with this. Please find the changes in the ensuing patchset.
 
> > > > +	/* Check that the virtual address is in the proper range */
> > > > +	if (tsk) {
> > > > +		if (!arch_check_va_in_userspace(bp->info.address, tsk))
> > > > +			return -EFAULT;
> > > > +	} else {
> > > > +		if (!arch_check_va_in_kernelspace(bp->info.address))
> > > > +			return -EFAULT;
> > > > +	}
> > > 
> > > Roland pointed out that these checks need to take into account the
> > > length of the breakpoint.  For example, in
> > > arch_check_va_in_userspace() it isn't sufficient for the start of the
> > > breakpoint region to be a userspace address; the end of the
> > > breakpoint region must also be in userspace.
> > > 
> > 
> > Ok. Will do something like:
> > return (va <= (TASK_SIZE - (hw_breakpoint_length * word_size)));
> 
> What is the purpose of word_size here?  The breakpoint length should be 
> specified in bytes, not words.
> 
> Don't forget that that in arch_check_va_in_kernelspace() you need to 
> check both for values that are too low and values that are too high 
> (they overflow and wrap around back to a user address).
> 

While I understand the user-space checking using the length of the HW
Breakpoint, I don't really see how I can check for an upper-bound for
kernel-space virtual addresses. Most usage in the kernel only checks for
the address >= TASK_SIZE (while they check for add + len if the length
of the memory is known). I will be glad to have any suggestions in this
regard.

> > We don't keep track of the thread (in the sense the task_struct) but
> > 'hbkpt_user_max' is used for validating requests and book-keeping. As
> > Maneesh mentioned before flush_thread_hw_breakpoint() updates
> > 'hbkpt_user_max'.
> > 
> > I can change it to read like the one below if it sounds better to you.
> > 
> > /* 
> >  * 'tsk' uses more number of registers than 'hbkpt_user_max'. Update
> >  * the same.
> >  */
> 
> My preference is simply to eliminate hbkpt_user_max entirely.
> 
> Alan Stern
>

Done.

Thanks,
K.Prasad