lkml.org 
[lkml]   [2009]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Patch 01/11] Introducing generic hardware breakpoint handler interfaces
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".

> > > +/* 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?

> > > +/*
> > > + * 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?

> > > + /* 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).

> 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



\
 
 \ /
  Last update: 2009-03-21 22:43    [W:0.061 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site