Re: [Patch 02/12] Introducing generic hardware breakpoint handler interfaces

[David Gibson <dwg@au1.ibm.com>]

On Thu, May 28, 2009 at 05:25:46PM +0530, K.Prasad wrote:
> On Thu, May 28, 2009 at 04:15:18PM +1000, David Gibson wrote:
> > On Mon, May 11, 2009 at 05:22:40PM +0530, K.Prasad wrote:
[snip]
> > > +/*
> > > + * Per-processor copy of hbp_kernel[]. Used only when hbp_kernel is being
> > > + * modified but we need the older copy to handle any hbp exceptions. It will
> > > + * sync with hbp_kernel[] value after updation is done through IPIs.
> > > + */
> > > +DEFINE_PER_CPU(struct hw_breakpoint*, this_hbp_kernel[HB_NUM]);
> > 
> > Ok.. this seems to assume that all breakpoints are equal as far as
> > allocation goes - i.e. that the total number of breakpoints is the
> > only allocation limit.  That's apparently true for x86, but on
> > embedded powerpc (4xx), for example, we can have up to 2 data
> > breakpoints and up to 4 instruction breakpoints independent of each
> > other.
> 
> True. But I hope that you see that the infrastructure is extensible.
> While the generic infrastructure has served the purposes of x86 and
> PPC64 well, one may want to 'grow' it when implementing breakpoints for
> 4xx processors. Say, make hbp_kernel[] a 2-D array, one for each
> HBP_TYPE (say DAC and IAC) and hbp_kernel_pos into an array (of size
> HBP_TYPE).

Hrm, I guess.  I suppose since there's no userspace exposed interface
to this new infrastructure (other than the backwards compatible ptrace
one), then this is indeed a deficiency which could be fixed up later.

[snip]
> > > +void switch_to_thread_hw_breakpoint(struct task_struct *tsk)
> > > +{
> > > +	/* Set the debug register */
> > > +	arch_install_thread_hw_breakpoint(tsk);
> > 
> > Um.. and the point of this wrapper which does nothing extra around the
> > arch-specific function would be...?
> > 
> > > +}
> > > +
> > > +/*
> > > + * Install the debug register values for just the kernel, no thread.
> > > + */
> > > +void switch_to_none_hw_breakpoint(void)
> > > +{
> > > +	arch_uninstall_thread_hw_breakpoint();
> > 
> > And again.
> > 
> 
> These functions have hence been removed. You can find the latest
> patchset here: http://lkml.org/lkml/2009/5/21/143.

Oh, sorry.  I thought I'd grabbed the latest patchset, but apparently
not.

[snip]
> > > +/*
> > > + * Load the debug registers during startup of a CPU.
> > > + */
> > > +void load_debug_registers(void)
> > 
> > This name does not suggest a boot and/or cpu hotplug only function.
> 
> I think the name aptly describes what the function achieves
> (irrespective of where it is employed). Let us know if you have better
> suggestions.

Hrm, not really.  To me, "load_debug_registers" suggests the operation
that happens in during context switch as we load the new threads debug
setup into the hardware registers.  AFAICT that's only part of what
this function does, and it's *not* typically called during context
switch.

How about "init_debug_registers()"?

-- 
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