Yonghong,
The patch looks good to me, but I'll try to read it carefully later.
Just a couple of cosmetic nits for now.
On 11/09, Yonghong Song wrote:
>
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -53,6 +53,10 @@ struct arch_uprobe {
> u8 fixups;
> u8 ilen;
> } defparam;
> + struct {
> + u8 src_offset; /* to the start of pt_regs */
> + u8 ilen;
> + } push;
> };
> };
I know it is very easy to blame the naming ;) but src_offset doesn't
look nice. How about reg_offset ?
> +static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs
> *regs)
> +{
> + void *src_ptr = (void *)regs + auprobe->push.src_offset;
> +
> + if (emulate_push_stack(regs, *(unsigned long *)src_ptr))
> + return false;
You can declare src_ptr as
unsigned long *src_ptr = ...;
and avoid the casting below.
> +static int uprobe_setup_xol_ops(struct arch_uprobe *auprobe, struct insn
> *insn)
> {
> u8 opc1 = OPCODE1(insn);
> int i;
>
> switch (opc1) {
> + case 0x50 ... 0x57:
> + return uprobe_setup_push_ops(auprobe, insn, opc1);
> +
> case 0xeb: /* jmp 8 */
> case 0xe9: /* jmp 32 */
> case 0x90: /* prefix* + nop; same as jmp with .offs = 0 */
> @@ -767,7 +863,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
> struct mm_struct *mm,
> if (ret)
> return ret;
>
> - ret = branch_setup_xol_ops(auprobe, &insn);
> + ret = uprobe_setup_xol_ops(auprobe, &insn);
> if (ret != -ENOSYS)
> return ret;
Well... again, this is cosmetic and I won't insist, but to me it would be
more clean to not change/rename branch_setup_xol_ops(). Instead, you can
just add
ret = uprobe_setup_push_ops(...);
if (ret != -ENOSYS)
return ret;
at the start of arch_uprobe_analyze_insn(), right after the similar
branch_setup_xol_ops() call.
Btw... please add v2 into the subject when you send the new version.
Oleg.
