| Message ID | 20141126082548.34D5114017D@ozlabs.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 11/26/2014 01:55 PM, Michael Ellerman wrote: > On Tue, 2014-25-11 at 10:08:48 UTC, Anshuman Khandual wrote: >> This patch enables support for hardware instruction breakpoints >> on POWER8 with the help of a new register CIABR (Completed >> Instruction Address Breakpoint Register). With this patch, single >> hardware instruction breakpoint can be added and cleared during >> any active xmon debug session. This hardware based instruction >> breakpoint mechanism works correctly along with the existing TRAP >> based instruction breakpoints available on xmon. > > > Hi Anshuman, > >> diff --git a/arch/powerpc/include/asm/xmon.h b/arch/powerpc/include/asm/xmon.h >> index 5eb8e59..5d17aec 100644 >> --- a/arch/powerpc/include/asm/xmon.h >> +++ b/arch/powerpc/include/asm/xmon.h >> @@ -29,5 +29,11 @@ static inline void xmon_register_spus(struct list_head *list) { }; >> extern int cpus_are_in_xmon(void); >> #endif > > This file is the exported interface *of xmon*, it's not the place to put things > that xmon needs internally. > > For now just put it in xmon.c Okay. > >> +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_PPC_SPLPAR) >> +#include <asm/plpar_wrappers.h> >> +#else >> +static inline long plapr_set_ciabr(unsigned long ciabr) {return 0; }; >> +#endif > > Also the ifdef is overly verbose, CONFIG_PPC_SPLPAR essentially depends on > CONFIG_PPC_BOOK3S_64. So you can just use #ifdef CONFIG_PPC_SPLPAR. Yeah, thats correct. > >> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c >> index b988b5a..c2f601a 100644 >> --- a/arch/powerpc/xmon/xmon.c >> +++ b/arch/powerpc/xmon/xmon.c >> @@ -271,6 +273,55 @@ static inline void cinval(void *p) >> } >> >> /* >> + * write_ciabr >> + * >> + * This function writes a value to the >> + * CIARB register either directly through >> + * mtspr instruction if the kernel is in HV >> + * privilege mode or call a hypervisor function >> + * to achieve the same in case the kernel is in >> + * supervisor privilege mode. >> + */ > > I'm not really sure a function this small needs a documentation block. > > But if you're going to add one, PLEASE make sure it's an actual kernel-doc > style comment. > > You can check with: > > $ ./scripts/kernel-doc -text arch/powerpc/xmon/xmon.c > > Which you'll notice prints: > > Warning(arch/powerpc/xmon/xmon.c): no structured comments found > > You need something like: > > /** > * write_ciabr() - write the CIABR SPR > * @ciabr: The value to write. > * > * This function writes a value to the CIABR register either directly through > * mtspr instruction if the kernel is in HV privilege mode or calls a > * hypervisor function to achieve the same in case the kernel is in supervisor > * privilege mode. > */ Sure. > > > > The rest of the patch is OK. But I was hoping you'd notice that we no longer > support any cpus that implement CPU_FTR_IABR. And so you can just repurpose all > the iabr logic for ciabr. Okay. > > Something like this, untested: Yeah it is working on LPAR and also on bare metal platform as well. The new patch will use some of your suggested code, so can I add your signed-off-by to the patch as well ?
On Thu, 2014-11-27 at 13:46 +0530, Anshuman Khandual wrote: > On 11/26/2014 01:55 PM, Michael Ellerman wrote: > > Something like this, untested: > > Yeah it is working on LPAR and also on bare metal platform as well. The new patch > will use some of your suggested code, so can I add your signed-off-by to the patch > as well ? Thanks for testing. Yes please add my: Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> cheers
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index b988b5addf86..6894650bff7f 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -51,6 +51,12 @@ #include <asm/paca.h> #endif +#ifdef CONFIG_PPC_SPLPAR +#include <asm/plpar_wrappers.h> +#else +static inline long plapr_set_ciabr(unsigned long ciabr) { return 0; }; +#endif + #include "nonstdio.h" #include "dis-asm.h" @@ -270,6 +276,31 @@ static inline void cinval(void *p) asm volatile ("dcbi 0,%0; icbi 0,%0" : : "r" (p)); } +static void write_ciabr(unsigned long ciabr) +{ + if (!cpu_has_feature(CPU_FTR_ARCH_207S)) + return; + + if (cpu_has_feature(CPU_FTR_HVMODE)) { + mtspr(SPRN_CIABR, ciabr); + return; + } + + plapr_set_ciabr(ciabr); +} + +static void set_ciabr(unsigned long addr) +{ + addr &= ~CIABR_PRIV; + + if (cpu_has_feature(CPU_FTR_HVMODE)) + addr |= CIABR_PRIV_HYPER; + else + addr |= CIABR_PRIV_SUPER; + + write_ciabr(addr); +} + /* * Disable surveillance (the service processor watchdog function) * while we are in xmon. @@ -764,9 +795,9 @@ static void insert_cpu_bpts(void) brk.len = 8; __set_breakpoint(&brk); } - if (iabr && cpu_has_feature(CPU_FTR_IABR)) - mtspr(SPRN_IABR, iabr->address - | (iabr->enabled & (BP_IABR|BP_IABR_TE))); + + if (iabr) + set_ciabr(iabr->address); } static void remove_bpts(void) @@ -792,8 +823,7 @@ static void remove_bpts(void) static void remove_cpu_bpts(void) { hw_breakpoint_disable(); - if (cpu_has_feature(CPU_FTR_IABR)) - mtspr(SPRN_IABR, 0); + write_ciabr(0); } /* Command interpreting routine */ @@ -1127,7 +1157,7 @@ static char *breakpoint_help_string = "b <addr> [cnt] set breakpoint at given instr addr\n" "bc clear all breakpoints\n" "bc <n/addr> clear breakpoint number n or at addr\n" - "bi <addr> [cnt] set hardware instr breakpoint (POWER3/RS64 only)\n" + "bi <addr> [cnt] set hardware instr breakpoint (POWER8 only)\n" "bd <addr> [cnt] set hardware data breakpoint\n" ""; @@ -1166,7 +1196,7 @@ bpt_cmds(void) break; case 'i': /* bi - hardware instr breakpoint */ - if (!cpu_has_feature(CPU_FTR_IABR)) { + if (!cpu_has_feature(CPU_FTR_ARCH_207S)) { printf("Hardware instruction breakpoint " "not supported on this cpu\n"); break;