Re: [PATCH] MIPS: KProbes support v0.1

[Himanshu Chauhan <hschauhan@nulltrace.org>]

Hi David,

Thanks for taking a look.

> >+#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.
>
Hmm.. I will remove this.
 
> 
> Can you also explain how the die notifier chain interacts with
> KProbes and why it cannot be a seperate notifier chain?

Actually, looking at x86 and other code, this is not the proper way
to do it. I will try to comeup with common approach.

> 
> >  #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?

As said ealier, this patch was based on some very older patch of 2.6.16 from
Sony Corp, I didn't make much changes like this. But anyways, I wan't aware of
this either. What would be the best code then?

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

Framework needs you to define this.

> 
> Why this ugliness?  Can't you handle it in do_bp() or  do_trap_or_bp()?

Let me see what I can do about this.

> 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

Though I have worked on octeon before but I don't remember nitty-gritties.
And I have no clue about these ops :) No way to test either!

With all that, I will soon send an updated patch.
Thanks for the review!

Regards
-Himanshu