Re: [PATCH] tile: support kprobes on tilegx

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

Hello Tony,

(2013/08/10 4:08), Tony Lu wrote:
> This change includes support for Kprobes, Jprobes and Return Probes.

Thank you for the effort, this looks good for the first step.

However, it seems this only supports instructions which doesn't touch
the execution path. I don't know tile ISA, but it should have jump/branch/
call/return etc. Those should be fixed after singlestep out-of-line in
resume_execution(). Or, kprobes should reject to probe on such instructions
by arch_prepare_kprobe().

> +int __kprobes arch_prepare_kprobe(struct kprobe *p)
> +{
> +	unsigned long addr = (unsigned long)p->addr;
> +
> +	if (addr & (sizeof(kprobe_opcode_t) - 1))
> +		return -EINVAL;
> +
> +	/* insn: must be on special executable page on tile. */
> +	p->ainsn.insn = get_insn_slot();
> +	if (!p->ainsn.insn)
> +		return -ENOMEM;
> +
> +	/*
> +	 * In the kprobe->ainsn.insn[] array we store the original
> +	 * instruction at index zero and a break trap instruction at
> +	 * index one.
> +	 */
> +	memcpy(&p->ainsn.insn[0], p->addr, sizeof(kprobe_opcode_t));
> +	p->ainsn.insn[1] = breakpoint2_insn;
> +	p->opcode = *p->addr;
> +
> +	return 0;
> +}
[...]
> +/*
> + * Called after single-stepping.  p->addr is the address of the
> + * instruction whose first byte has been replaced by the "break 0"
> + * temporarily put back the original opcode to single-step, we
> + * single-stepped a copy of the instruction.  The address of this
> + * copy is p->ainsn.insn.
> + *
> + * This function prepares to return from the post-single-step
> + * breakpoint trap.
> + */
> +static void __kprobes resume_execution(struct kprobe *p,
> +				       struct pt_regs *regs,
> +				       struct kprobe_ctlblk *kcb)
> +{
> +	unsigned long orig_pc = kcb->kprobe_saved_pc;
> +	regs->pc = orig_pc + 8;
> +}
> +

I recommend you to fix that, at least filter out non-supported instructions
by arch_prepare_kprobe(), because no other users don't know which
instruction they can probe. :)

Thank you again!

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com