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
> +#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.
> 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.
*/
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.
Something like this, untested:
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;
cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
