Messages in this thread | | | Subject | Re: [PATCH 5/5] uprobes/x86: Move default_xol_ops's data into arch_uprobe->def | From | Jim Keniston <> | Date | Thu, 24 Apr 2014 16:30:47 -0700 |
| |
On Tue, 2014-04-22 at 16:47 +0200, Oleg Nesterov wrote: > Finally we can move arch_uprobe->fixups/rip_rela_target_address > into the new "def" struct and place this struct in the union, they > are only used by default_xol_ops paths. > > The patch also renames rip_rela_target_address to riprel_target just > to make this name shorter. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
I see a couple of nits in this patch (see below), but the others look good.
Patches 1-5 of this set: Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>
> --- > arch/x86/include/asm/uprobes.h | 12 ++++++---- > arch/x86/kernel/uprobes.c | 41 +++++++++++++++++++-------------------- > 2 files changed, 27 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h > index 93bee7b..72caff7 100644 > --- a/arch/x86/include/asm/uprobes.h > +++ b/arch/x86/include/asm/uprobes.h > @@ -41,18 +41,20 @@ struct arch_uprobe { > u8 ixol[MAX_UINSN_BYTES]; > }; > > - u16 fixups; > const struct uprobe_xol_ops *ops; > > union { > -#ifdef CONFIG_X86_64 > - unsigned long rip_rela_target_address; > -#endif > struct { > s32 offs; > u8 ilen; > u8 opc1; > - } branch; > + } branch; > + struct { > +#ifdef CONFIG_X86_64 > + long riprel_target; > +#endif > + u16 fixups; > + } def;
"def" is kind of ambiguous. How about "dfault" or some such?
> }; > }; > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index e6314a2..69b2d61 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c ... > @@ -636,12 +635,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > > /* > * Figure out which fixups arch_uprobe_post_xol() will need to perform, > - * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups > - * is either zero or it reflects rip-related fixups. > + * and annotate def->fixups accordingly. To start with, ->fixups is > + * either zero or it reflects rip-related fixups.
That sentence stopped being true a couple of patch sets ago. handle_riprel_insn() is called later in this function now.
> */ > switch (OPCODE1(&insn)) { > case 0x9d: /* popf */ > - auprobe->fixups |= UPROBE_FIX_SETF; > + auprobe->def.fixups |= UPROBE_FIX_SETF; > break; > case 0xc3: /* ret or lret -- ip is correct */ > case 0xcb: > @@ -669,9 +668,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, > } > > if (fix_ip) > - auprobe->fixups |= UPROBE_FIX_IP; > + auprobe->def.fixups |= UPROBE_FIX_IP; > if (fix_call) > - auprobe->fixups |= UPROBE_FIX_CALL; > + auprobe->def.fixups |= UPROBE_FIX_CALL; > > auprobe->ops = &default_xol_ops; > return 0;
Jim
| |