I have a few questions and comments below. Many of them may be due to
my lack of understanding about the internals of KProbes.
David Daney.
On 06/07/2010 09:34 AM, Himanshu Chauhan wrote:
[...]
diff --git a/arch/mips/include/asm/kdebug.h b/arch/mips/include/asm/kdebug.h
index 5bf62aa..52818ac 100644
--- a/arch/mips/include/asm/kdebug.h
+++ b/arch/mips/include/asm/kdebug.h
@@ -8,6 +8,11 @@ enum die_val {
DIE_FP,
DIE_TRAP,
DIE_RI,
+#ifdef CONFIG_KPROBES
+ DIE_PAGE_FAULT,
+ DIE_BREAK,
+ DIE_SSTEPBP,
+#endif
};
It might be cleaner without the #ifdef. These are enum value
definitions, so it doesn't affect code size.
Can you also explain how the die notifier chain interacts with KProbes
and why it cannot be a seperate notifier chain?
#endif /* _ASM_MIPS_KDEBUG_H */
diff --git a/arch/mips/include/asm/kprobes.h b/arch/mips/include/asm/kprobes.h
new file mode 100644
index 0000000..0f647bf
--- /dev/null
+++ b/arch/mips/include/asm/kprobes.h
@@ -0,0 +1,85 @@
[...]
+
+#define BREAKPOINT_INSTRUCTION 0x0000000d
+
+/*
+ * We do not have hardware single-stepping on MIPS.
+ * So we implement software single-stepping with breakpoint
+ * trap 'break 5'.
+ */
+#define BREAKPOINT_INSTRUCTION_2 0x0000014d
The BREAK codes are defined in asm/break.h This should be added there
instead.
Why do you use codes (0 and 5) that are already kind of reserved for
user space debuggers?
+#define MAX_INSN_SIZE 2
+
+#define flush_insn_slot(p) do { \
+ /* invalidate I-cache */ \
+ asm volatile("cache 0, 0($0)"); \
+ /* invalidate D-cache */ \
+ asm volatile("cache 9, 0($0)"); \
+ } while(0);
+
You have to call a function in arch/mips/mm/c-* to do this, you cannot
open code with CACHE instructions as you need to handle CPU quirks and
SMP. It is possible that flush_icache_range() or flush_cache_sigtramp()
would work. Or we might need something new.
I see you use flush_icache_range() below, why have this definition, it
looks unused?
[...]
diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 8882e57..e53ac80 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -450,7 +450,13 @@ NESTED(nmi_handler, PT_SIZE, sp)
BUILD_HANDLER ades ade ade silent /* #5 */
BUILD_HANDLER ibe be cli silent /* #6 */
BUILD_HANDLER dbe be cli silent /* #7 */
+#ifndef CONFIG_KPROBES
+ /* call do_bp if bp hit and kprobes not configured */
BUILD_HANDLER bp bp sti silent /* #9 */
+#else
+ /* call do_break if bp hit and kprobes are configured */
+ BUILD_HANDLER bp break sti silent /* #9 */
+#endif
Why this ugliness? Can't you handle it in do_bp() or do_trap_or_bp()?
BUILD_HANDLER ri ri sti silent /* #10 */
BUILD_HANDLER cpu cpu sti silent /* #11 */
BUILD_HANDLER ov ov sti silent /* #12 */
diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
new file mode 100644
index 0000000..0493791
--- /dev/null
+++ b/arch/mips/kernel/kprobes.c
@@ -0,0 +1,380 @@
[...]
+
+int __kprobes arch_prepare_kprobe(struct kprobe *p)
+{
+ union mips_instruction insn;
+ int ret = 0;
+
+ insn = *p->addr;
+
+ switch (insn.i_format.opcode) {
+ /*
+ * This group contains:
+ * jr and jalr are in r_format format.
+ */
+ case spec_op:
+
+ /*
+ * This group contains:
+ * bltz_op, bgez_op, bltzl_op, bgezl_op,
+ * bltzal_op, bgezal_op, bltzall_op, bgezall_op.
+ */
+ case bcond_op:
+
+ /*
+ * These are unconditional and in j_format.
+ */
+ case jal_op:
+ case j_op:
+
+ /*
+ * These are conditional and in i_format.
+ */
+ case beq_op:
+ case beql_op:
+ case bne_op:
+ case bnel_op:
+ case blez_op:
+ case blezl_op:
+ case bgtz_op:
+ case bgtzl_op:
Need to add or otherwise handle:
#ifdef CONFIG_CPU_CAVIUM_OCTEON
case lwc2_op: /* This is bbit0 on Octeon */
case ldc2_op: /* This is bbit032 on Octeon */
case swc2_op: /* This is bbit1 on Octeon */
case sdc2_op: /* This is bbit132 on Octeon */
#endif
[...]
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 8bdd6a6..f6b4b41 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -27,6 +27,7 @@
#include<linux/kdebug.h>
#include<linux/notifier.h>
#include<linux/kdb.h>
+#include<linux/kprobes.h>
#include<asm/bootinfo.h>
#include<asm/branch.h>
@@ -334,7 +335,7 @@ void show_regs(struct pt_regs *regs)
__show_regs((struct pt_regs *)regs);
}
-void show_registers(const struct pt_regs *regs)
+void show_registers(struct pt_regs *regs)
{
const int field = 2 * sizeof(unsigned long);
@@ -790,6 +791,43 @@ out_sigsegv:
force_sig(SIGSEGV, current);
}
+#ifdef CONFIG_KPROBES
+asmlinkage void __kprobes do_break (struct pt_regs *regs)
+{
+ unsigned int opcode, bcode;
+
+ opcode = *(unsigned long *)(regs->cp0_epc);
+
+ bcode = ((opcode>> 6)& ((1<< 20) - 1));
+ if (bcode< (1<< 10))
+ bcode<<= 10;
+
+ /*
+ * notify the kprobe handlers,if instruction is break 0 or break 5
+ */
+ switch (bcode) {
+ case BRK_USERBP<< 10:
+ if (notify_die(DIE_BREAK, "debug", regs, bcode, 0, 0) == NOTIFY_STOP)
+ return;
+ else
+ break;
+ case BRK_SSTEPBP<< 10:
+ if (notify_die(DIE_SSTEPBP, "single_step", regs, bcode, 0, 0) == NOTIFY_STOP)
+ return;
+ else
+ break;
+ default:
+ break;
+ }
+
This should be folded into do_bp().
+ /*
+ * If the bcode is other than 0 and 5, then call the normal
+ * break handler do_bp()
+ */
+ do_bp(regs);
+}
+#endif
+
asmlinkage void do_tr(struct pt_regs *regs)
{
unsigned int opcode, tcode = 0;
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index b78f7d9..86e2d27 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -18,6 +18,8 @@
#include<linux/smp.h>
#include<linux/vt_kern.h> /* For unblank_screen() */
#include<linux/module.h>
+#include<linux/kprobes.h>
+#include<linux/kdebug.h> /* notify_die and asm/kdebug.h */
#include<asm/branch.h>
#include<asm/mmu_context.h>
@@ -31,8 +33,8 @@
* and the problem, and then passes it off to one of the appropriate
* routines.
*/
-asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long write,
- unsigned long address)
+asmlinkage void __kprobes do_page_fault(struct pt_regs *regs, unsigned long write,
+ unsigned long address)
{
struct vm_area_struct * vma = NULL;
struct task_struct *tsk = current;
@@ -47,6 +49,11 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long write,
field, regs->cp0_epc);
#endif
+ /* Notify kprobes fault handler. */
+ if (notify_die(DIE_PAGE_FAULT, "page fault",
+ regs, -1, SEGV_MAPERR, SEGV_MAPERR) == NOTIFY_STOP)
+ return;
+
info.si_code = SEGV_MAPERR;
/*
