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:
[...]
quoted text > 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?
quoted text > #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 @@
[...]
quoted text > +
> +#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?
quoted text > +#define MAX_INSN_SIZE 2
> +
> +#define flush_insn_slot(p) do { \
> + /* invalidate I-cache */ \
> + asm volatile("cache 0, 0(__PLACEHOLDER__3_)"); \
> + /* invalidate D-cache */ \
> + asm volatile("cache 9, 0(__PLACEHOLDER__3_)"); \
> + } 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?
[...]
quoted text > 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()?
quoted text > 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 @@
[...]
quoted text > +
> +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
[...]
quoted text > 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().
quoted text > + /*
> + * 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;
>
> /*
--
unsubscribe notice To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/ Messages in current thread:
Re: [PATCH] MIPS: KProbes support v0.1 , David Daney , (Mon Jun 7, 12:41 pm)
