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 <pra...@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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
