On 11/26/2014 01:55 PM, Michael Ellerman wrote:

On Tue, 2014-25-11 at 10:08:48 UTC, Anshuman Khandual wrote:

quoted

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,

quoted

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.

quoted

+#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.

quoted

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 ?