Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753059AbaFBP2H (ORCPT ); Mon, 2 Jun 2014 11:28:07 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:45206 "EHLO e37.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751493AbaFBP2F (ORCPT ); Mon, 2 Jun 2014 11:28:05 -0400 Subject: Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates From: Jim Keniston To: Oleg Nesterov Cc: Ingo Molnar , Denys Vlasenko , Masami Hiramatsu , Srikar Dronamraju , linux-kernel@vger.kernel.org In-Reply-To: <20140601192520.GA14539@redhat.com> References: <20140601192520.GA14539@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Mon, 02 Jun 2014 08:27:59 -0700 Message-ID: <1401722879.6113.0.camel@oc7886638347.ibm.com.usor.ibm.com> Mime-Version: 1.0 X-Mailer: Evolution 2.32.3 (2.32.3-30.el6) Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14060215-7164-0000-0000-000002299ECF Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2014-06-01 at 21:25 +0200, Oleg Nesterov wrote: > Purely cosmetic, no changes in .o, > > 1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to > ->dflt. > > 2. Add the comment into default_post_xol_op() to explain "regs->sp +=". > > 3. Remove the stale part of the comment in arch_uprobe_analyze_insn(). > > Suggested-by: Jim Keniston > Signed-off-by: Oleg Nesterov Looks good. Thanks. Reviewed-by: Jim Keniston > --- > arch/x86/include/asm/uprobes.h | 2 +- > arch/x86/kernel/uprobes.c | 37 ++++++++++++++++++------------------- > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h > index 7be3c07..b3d9442 100644 > --- a/arch/x86/include/asm/uprobes.h > +++ b/arch/x86/include/asm/uprobes.h > @@ -52,7 +52,7 @@ struct arch_uprobe { > struct { > u8 fixups; > u8 ilen; > - } def; > + } dflt; > }; > }; > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index fcf6279..33e239f 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm) > * If arch_uprobe->insn doesn't use rip-relative addressing, return > * immediately. Otherwise, rewrite the instruction so that it accesses > * its memory operand indirectly through a scratch register. Set > - * def->fixups accordingly. (The contents of the scratch register > + * dflt->fixups accordingly. (The contents of the scratch register > * will be saved before we single-step the modified instruction, > * and restored afterward). > * > @@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn) > */ > if (reg != 6 && reg2 != 6) { > reg2 = 6; > - auprobe->def.fixups |= UPROBE_FIX_RIP_SI; > + auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI; > } else if (reg != 7 && reg2 != 7) { > reg2 = 7; > - auprobe->def.fixups |= UPROBE_FIX_RIP_DI; > + auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI; > /* TODO (paranoia): force maskmovq to not use di */ > } else { > reg2 = 3; > - auprobe->def.fixups |= UPROBE_FIX_RIP_BX; > + auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX; > } > /* > * Point cursor at the modrm byte. The next 4 bytes are the > @@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn) > static inline unsigned long * > scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > - if (auprobe->def.fixups & UPROBE_FIX_RIP_SI) > + if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI) > return ®s->si; > - if (auprobe->def.fixups & UPROBE_FIX_RIP_DI) > + if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI) > return ®s->di; > return ®s->bx; > } > @@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs) > */ > static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > - if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) { > + if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) { > struct uprobe_task *utask = current->utask; > unsigned long *sr = scratch_reg(auprobe, regs); > > utask->autask.saved_scratch_register = *sr; > - *sr = utask->vaddr + auprobe->def.ilen; > + *sr = utask->vaddr + auprobe->dflt.ilen; > } > } > > static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) > { > - if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) { > + if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) { > struct uprobe_task *utask = current->utask; > unsigned long *sr = scratch_reg(auprobe, regs); > > @@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs > struct uprobe_task *utask = current->utask; > > riprel_post_xol(auprobe, regs); > - if (auprobe->def.fixups & UPROBE_FIX_IP) { > + if (auprobe->dflt.fixups & UPROBE_FIX_IP) { > long correction = utask->vaddr - utask->xol_vaddr; > regs->ip += correction; > - } else if (auprobe->def.fixups & UPROBE_FIX_CALL) { > - regs->sp += sizeof_long(); > - if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen)) > + } else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) { > + regs->sp += sizeof_long(); /* Pop incorrect return address */ > + if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen)) > return -ERESTART; > } > /* popf; tell the caller to not touch TF */ > - if (auprobe->def.fixups & UPROBE_FIX_SETF) > + if (auprobe->dflt.fixups & UPROBE_FIX_SETF) > utask->autask.saved_tf = true; > > return 0; > @@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > > /* > * Figure out which fixups default_post_xol_op() will need to perform, > - * and annotate def->fixups accordingly. To start with, ->fixups is > - * either zero or it reflects rip-related fixups. > + * and annotate dflt->fixups accordingly. > */ > switch (OPCODE1(&insn)) { > case 0x9d: /* popf */ > - auprobe->def.fixups |= UPROBE_FIX_SETF; > + auprobe->dflt.fixups |= UPROBE_FIX_SETF; > break; > case 0xc3: /* ret or lret -- ip is correct */ > case 0xcb: > @@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > riprel_analyze(auprobe, &insn); > } > > - auprobe->def.ilen = insn.length; > - auprobe->def.fixups |= fix_ip_or_call; > + auprobe->dflt.ilen = insn.length; > + auprobe->dflt.fixups |= fix_ip_or_call; > > auprobe->ops = &default_xol_ops; > return 0; -- 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/