Re: [RFC Patch 2/2] PPC64-HWBKPT: Implement hw-breakpoints for PPC64

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

On Fri, Mar 26, 2010 at 04:11:45PM -0500, Dave Kleikamp wrote:
> On Tue, 2010-03-23 at 19:37 +0530, K.Prasad wrote:
> > plain text document attachment (ppc64_hbkpt_02)
> > Implement perf-events based hw-breakpoint interfaces for PPC64 processors.
> > These interfaces help arbitrate requests from various users and schedules
> > them as appropriate.
> > 
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> 
> SNIP
> 
> > Index: linux-2.6.ppc64_test/arch/powerpc/include/asm/cputable.h
> > ===================================================================
> > --- linux-2.6.ppc64_test.orig/arch/powerpc/include/asm/cputable.h
> > +++ linux-2.6.ppc64_test/arch/powerpc/include/asm/cputable.h
> > @@ -511,6 +511,13 @@ static inline int cpu_has_feature(unsign
> >  		& feature);
> >  }
> > 
> > +#define CPU_FTR_HAS_DABR (defined(CONFIG_PPC64) && \
> > +			  !defined(CONFIG_PPC_ADV_DEBUG_REGS))
> > +#ifdef CPU_FTR_HAS_DABR
> > +/* Number of physical HW breakpoint registers */
> > +#define HBP_NUM 1
> > +#endif
> > +
> >  #endif /* !__ASSEMBLY__ */
> > 
> >  #endif /* __KERNEL__ */
> 
> These new defines don't really correlate to the cpu table.  One would
> expect cpu_has_feature(CPU_FTR_HAS_DABR) to have meaning, but it would
> have to be defined similar to the other CPU_FTR_ constants, and or-ed
> with CPU_FTRS_ALWAYS (when appropriate).
> 

The code can be changed as below:

#if (defined(CONFIG_PPC64) && !defined(CONFIG_PPC_ADV_DEBUG_REGS))
#define CPU_FTR_HAS_DABR 1
/* Number of physical HW breakpoint registers */
#define HBP_NUM 1
#endif

However, a config option CONFIG_PPC_BOOK3S_64 (I just found) whose scope
includes only 64-bit server processors (having one DABR) to be the most
suitable.

I think it must be used in lieu of introducing a new CPU_FTR_HAS_DABR
definition in cputable.h


> Similarly, I would expect the cpu_spec structure to have a new field,
> hbp_num, which is initialized in cputable.c.  Maybe a longer name would
> be better, num_hw_brkpts?
> 

There are a few issues with such an approach:
i) Two such fields would be required in 'struct cpu_spec' - one for
instruction breakpoints and other for data.
ii) As pointed out by you below, hbp_num or num_hw_brkpts would always
be assigned to the compile time constant HBP_NUM (hence a variable is not
required to store it).
iii) HBP_NUM still cannot be entirely removed as it is used by generic
kernel/hw_breakpoint.c code (and is used by x86 code as well).

I think the simplest approach would be to have the following entry in
cputable.h (and get away with the rest of the additions seen in patch
ver XV)

#ifdef CONFIG_PPC_BOOK3S_64
#define HBP_NUM 1
#endif

The next version of the patch should contain changes to that effect
(assuming I hear no objections).

> When I added the PPC_ADV_DEBUG config options for the bookE features, I
> didn't see an immediate need to clutter the cputable since their values
> are fixed at compile time.  We should be consistent with these, but

It is even more true with ppc64-server processors, where the number of
debug registers (denoted by HBP_NUM) is fixed to 1 (unlike BookE where the
DACs can be used in standalone or as a pair of registers).

> unless we are going to determine any of these at run-time, I don't know
> that they belong in the cpu table.
>
> Thanks,
> Shaggy
> -- 

Thanks,
K.Prasad