[PATCH] x86: Introduce REX prefix helper for kprobes

[PATCH] x86: Introduce REX prefix helper for kprobes

[Harvey Harrison]

Fold some small ifdefs into a helper function.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Masami, Ingo, I had this left in some unsent kprobes unification
work.  Depends on your tastes, but does reduce ifdefs and is a bit
better about self-documenting the REX prefix on X86_64.

If I find places that could also use this I'll try to find a suitable
header any stick a static inline there instead.  Otherwise static to
kprobes.c is probably more appropriate for now.

 arch/x86/kernel/kprobes.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
index 4e33329..b1804e4 100644
--- a/arch/x86/kernel/kprobes.c
+++ b/arch/x86/kernel/kprobes.c
@@ -171,6 +171,19 @@ static void __kprobes set_jmp_op(void *from, void *to)
 }
 
 /*
+ * Check for the REX prefix which can only exist on X86_64
+ * X86_32 always returns 0
+ */
+static int __kprobes is_REX_prefix(kprobe_opcode_t *insn)
+{
+#ifdef CONFIG_X86_64
+	if ((*insn & 0xf0) == 0x40)
+		return 1;
+#endif
+	return 0;
+}
+
+/*
  * Returns non-zero if opcode is boostable.
  * RIP relative instructions are adjusted at copying time in 64 bits mode
  */
@@ -239,14 +252,14 @@ static int __kprobes is_IF_modifier(kprobe_opcode_t *insn)
 	case 0x9d:		/* popf/popfd */
 		return 1;
 	}
-#ifdef CONFIG_X86_64
+
 	/*
-	 * on 64 bit x86, 0x40-0x4f are prefixes so we need to look
+	 * on X86_64, 0x40-0x4f are REX prefixes so we need to look
 	 * at the next byte instead.. but of course not recurse infinitely
 	 */
-	if (*insn  >= 0x40 && *insn <= 0x4f)
+	if (is_REX_prefix(insn))
 		return is_IF_modifier(++insn);
-#endif
+
 	return 0;
 }
 
@@ -284,7 +297,7 @@ static void __kprobes fix_riprel(struct kprobe *p)
 	}
 
 	/* Skip REX instruction prefix.  */
-	if ((*insn & 0xf0) == 0x40)
+	if (is_REX_prefix(insn))
 		++insn;
 
 	if (*insn == 0x0f) {
@@ -748,11 +761,9 @@ static void __kprobes resume_execution(struct kprobe *p,
 	unsigned long orig_ip = (unsigned long)p->addr;
 	kprobe_opcode_t *insn = p->ainsn.insn;
 
-#ifdef CONFIG_X86_64
 	/*skip the REX prefix*/
-	if (*insn >= 0x40 && *insn <= 0x4f)
+	if (is_REX_prefix(insn))
 		insn++;
-#endif
 
 	regs->flags &= ~X86_EFLAGS_TF;
 	switch (*insn) {
-- 
1.5.4.rc0.1143.g1a8a

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Masami Hiramatsu]

Hi Harvey,

Harvey Harrison wrote:
> Fold some small ifdefs into a helper function.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> Masami, Ingo, I had this left in some unsent kprobes unification
> work.  Depends on your tastes, but does reduce ifdefs and is a bit
> better about self-documenting the REX prefix on X86_64.

Basically, I think it is good idea.
Could you use a macro same as the stack_addr() macro, like as below?

#defile is_REX_prefix(insn) ((insn & 0xf0) == 0x40))

This is just a bit checker, so I think a macro is better to do that.

> If I find places that could also use this I'll try to find a suitable
> header any stick a static inline there instead.  Otherwise static to
> kprobes.c is probably more appropriate for now.
> 
>  arch/x86/kernel/kprobes.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
> index 4e33329..b1804e4 100644
> --- a/arch/x86/kernel/kprobes.c
> +++ b/arch/x86/kernel/kprobes.c
> @@ -171,6 +171,19 @@ static void __kprobes set_jmp_op(void *from, void *to)
>  }
>  
>  /*
> + * Check for the REX prefix which can only exist on X86_64
> + * X86_32 always returns 0
> + */
> +static int __kprobes is_REX_prefix(kprobe_opcode_t *insn)
> +{
> +#ifdef CONFIG_X86_64
> +	if ((*insn & 0xf0) == 0x40)
> +		return 1;
> +#endif
> +	return 0;
> +}
> +
> +/*
>   * Returns non-zero if opcode is boostable.
>   * RIP relative instructions are adjusted at copying time in 64 bits mode
>   */
> @@ -239,14 +252,14 @@ static int __kprobes is_IF_modifier(kprobe_opcode_t *insn)
>  	case 0x9d:		/* popf/popfd */
>  		return 1;
>  	}
> -#ifdef CONFIG_X86_64
> +
>  	/*
> -	 * on 64 bit x86, 0x40-0x4f are prefixes so we need to look
> +	 * on X86_64, 0x40-0x4f are REX prefixes so we need to look
>  	 * at the next byte instead.. but of course not recurse infinitely
>  	 */
> -	if (*insn  >= 0x40 && *insn <= 0x4f)
> +	if (is_REX_prefix(insn))
>  		return is_IF_modifier(++insn);
> -#endif
> +
>  	return 0;
>  }
>  
> @@ -284,7 +297,7 @@ static void __kprobes fix_riprel(struct kprobe *p)
>  	}
>  
>  	/* Skip REX instruction prefix.  */
> -	if ((*insn & 0xf0) == 0x40)
> +	if (is_REX_prefix(insn))
>  		++insn;
>  
>  	if (*insn == 0x0f) {
> @@ -748,11 +761,9 @@ static void __kprobes resume_execution(struct kprobe *p,
>  	unsigned long orig_ip = (unsigned long)p->addr;
>  	kprobe_opcode_t *insn = p->ainsn.insn;
>  
> -#ifdef CONFIG_X86_64
>  	/*skip the REX prefix*/
> -	if (*insn >= 0x40 && *insn <= 0x4f)
> +	if (is_REX_prefix(insn))
>  		insn++;
> -#endif
>  
>  	regs->flags &= ~X86_EFLAGS_TF;
>  	switch (*insn) {

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[H. Peter Anvin]

Masami Hiramatsu wrote:
> Hi Harvey,
> 
> Harvey Harrison wrote:
>> Fold some small ifdefs into a helper function.
>>
>> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>> ---
>> Masami, Ingo, I had this left in some unsent kprobes unification
>> work.  Depends on your tastes, but does reduce ifdefs and is a bit
>> better about self-documenting the REX prefix on X86_64.
> 
> Basically, I think it is good idea.
> Could you use a macro same as the stack_addr() macro, like as below?
> 
> #defile is_REX_prefix(insn) ((insn & 0xf0) == 0x40))
> 
> This is just a bit checker, so I think a macro is better to do that.
> 

Why is a macro better than an inline, and why the odd mIXed case?

	-hpa

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Masami Hiramatsu]

Hi,

H. Peter Anvin wrote:
>> Could you use a macro same as the stack_addr() macro, like as below?
>>
>> #defile is_REX_prefix(insn) ((insn & 0xf0) == 0x40))
>>
>> This is just a bit checker, so I think a macro is better to do that.
>>
> 
> Why is a macro better than an inline, and why the odd mIXed case?

I thought we can use macro because it just check a bit mask.
And if we use this as a macro, it will be defined in #ifdef block at
the top of kprobes.c. It is simple in this case.
I know the inline is better than the macro, it can check the type of
arguments.
If you would like to use inline, how about this?

---
(in case of CONFIG_X86_64)
static inline int is_rex_prefix(int op)
{
	return ((op & 0xf0) == 0x40);
}

(in case of CONFIG_X86_32)
#define is_rex_prefix(op) (0)
---

About the name, I just used the previous inline function name.

Thank you,

> 
> 	-hpa

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Masami Hiramatsu]

Masami Hiramatsu wrote:
> (in case of CONFIG_X86_64)
> static inline int is_rex_prefix(int op)

Oops, s/int op/kprobe_opcode_t opcode/

> About the name, I just used the previous inline function name.

I re-think "is_REX_prefix" is better, because it is an architecture
dependent notation(intel's software developers manual called it
"REX prefixes"), and there is already is_IF_modifier() function
in arch/x86/kernel/kprobes.c.

Thanks,

Best Regards,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com, masami.hiramatsu.pt@hitachi.com

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@redhat.com> wrote:

> > Why is a macro better than an inline, and why the odd mIXed case?
> 
> I thought we can use macro because it just check a bit mask. And if we 
> use this as a macro, it will be defined in #ifdef block at the top of 
> kprobes.c. It is simple in this case. I know the inline is better than 
> the macro, it can check the type of arguments. If you would like to 
> use inline, how about this?

yes, we prefer inlines for just about everything. We have reoccuring 
regressions due to macro side-effects, lack of type and argument 
checking - and the simplest maintenance policy is to just never 
introduce new macros. Macros also played an active role in creating our 
current include file dependencies spaghetti. So unless there are very, 
very strong reasons in favor of adding a macro, please always use 
inlines.

	Ingo

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Harvey Harrison]

On Sat, 2007-12-29 at 23:04 -0800, H. Peter Anvin wrote:
> Masami Hiramatsu wrote:
> > Hi Harvey,
> > 
> > Harvey Harrison wrote:
> >> Fold some small ifdefs into a helper function.
> >>
> >> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >> ---
> >> Masami, Ingo, I had this left in some unsent kprobes unification
> >> work.  Depends on your tastes, but does reduce ifdefs and is a bit
> >> better about self-documenting the REX prefix on X86_64.
> > 
> > Basically, I think it is good idea.
> > Could you use a macro same as the stack_addr() macro, like as below?
> > 
> > #defile is_REX_prefix(insn) ((insn & 0xf0) == 0x40))
> > 
> > This is just a bit checker, so I think a macro is better to do that.
> > 
> 
> Why is a macro better than an inline, and why the odd mIXed case?
> 

I was emulating existing practice I saw in kprobes, see is_IF_modifier.

Harvey

Re: [PATCH] x86: Introduce REX prefix helper for kprobes

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Fold some small ifdefs into a helper function.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>

thanks, applied.

	Ingo