[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

This patch adds some endianness-agnostic helpers to convert machine
instructions between canonical integer form and in-memory
representation, and also provides a transparent way to read a
single Thumb instruction from memory, without the need to know the
size in advance or write explicit condition checks.

A canonical integer form for representing instructions is also
formalised here.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/include/asm/opcodes.h |  162 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 162 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/opcodes.h

diff --git a/arch/arm/include/asm/opcodes.h b/arch/arm/include/asm/opcodes.h
new file mode 100644
index 0000000..5d18f92
--- /dev/null
+++ b/arch/arm/include/asm/opcodes.h
@@ -0,0 +1,162 @@
+/*
+ * arch/arm/include/asm/opcodes.h
+ *
+ * Copyright (C) 2011 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __ARM_OPCODES_H
+#define __ARM_OPCODES_H
+
+#include <linux/types.h>
+#include <linux/swab.h>
+
+typedef u32 arm_opcode_t;
+
+/*
+ * Canonical instruction representation (arm_opcode_t):
+ *
+ *	ARM:		0xKKLLMMNN
+ *	Thumb 16-bit:	0x0000KKLL, where KK < 0xE8
+ *	Thumb 32-bit:	0xKKLLMMNN, where KK >= 0xE8
+ *
+ * There is no way to distinguish an ARM instruction in canonical representation
+ * from a Thumb instruction (just as these cannot be distinguished in memory).
+ * Where this distinction is important, it needs to be tracked separately.
+ *
+ * Note that values in the range 0x0000E800..0xE7FFFFFF intentionally do not
+ * represent any valid Thumb-2 instruction.  For this range,
+ * __opcode_is_thumb32() and __opcode_is_thumb16() will both be false.
+ */
+
+#ifdef CONFIG_CPU_ENDIAN_BE8
+#define __opcode_to_mem_arm(x) swab32(x)
+#define __opcode_to_mem_thumb16(x) swab16(x)
+#define __opcode_to_mem_thumb32(x) swahb32(x)
+#else
+#define __opcode_to_mem_arm(x) (x) ((u32)(x))
+#define __opcode_to_mem_thumb16(x) ((u16)(x))
+#define __opcode_to_mem_thumb32(x) swahw32(x)
+#endif
+
+#define __mem_to_opcode_arm(x) __opcode_to_mem_arm(x)
+#define __mem_to_opcode_thumb16(x) __opcode_to_mem_thumb16(x)
+#define __mem_to_opcode_thumb32(x) __opcode_to_mem_thumb32(x)
+
+/* Operations specific to Thumb opcodes */
+
+/* Instruction size checks: */
+#define __opcode_is_thumb32(x) ((u32)(x) >= 0xE8000000UL)
+#define __opcode_is_thumb16(x) ((u32)(x) < 0xE800UL)
+
+/* Operations to construct or split 32-bit Thumb instructions: */
+#define __opcode_thumb32_first(x) ((u16)((thumb_opcode) >> 16))
+#define __opcode_thumb32_second(x) ((u16)(thumb_opcode))
+#define __opcode_thumb32_compose(first, second) \
+	(((u32)(u16)(first) << 16) | (u32)(u16)(second))
+
+/*
+ * int __opcode_read_<isa>(
+ *	arm_opcode_t *outp,
+ *	void const **inpp,
+ *	int (*readfn)(void *dst, void const *src, size_t size)
+ * )
+ *
+ * This helper reads one complete Thumb instruction and stores the canonicalised
+ * opcode to *outp.
+ *
+ * For maximum flexibility, the mechanism for reading the instruction is
+ * specified as an argument: read16fn(dst, src, size) must attempt to copy
+ * <size> bytes from <src> to <dst>.  <readfn>() should return 0 if the copy
+ * was successful, or an error code otherwise.
+ *
+ * Return:
+ *	0	success;
+ *			*outp contains the instruction read
+ *			*inp points to the next instruction
+ *	!= 0	failure:
+ *			*outp is undefined
+ *			*inp contains the first address not successfully read
+ *
+ * Writing this is a macro means that <readfn> can also be implemented as a
+ * macro.  This permits the simple case where no error checking is required to
+ * be heavily optimised.
+ */
+#define __opcode_read_thumb(outp, inpp, readfn) ({			\
+	u16 __t;							\
+									\
+	BUILD_BUG_ON(sizeof(*(outp)) != sizeof(arm_opcode_t));		\
+									\
+	___read_advance(&__t, inpp, sizeof(__t), readfn)		\
+	|| __opcode_is_thumb16(*(outp) = __mem_to_opcode_thumb16(__t)) ? 0 : \
+		___read_advance(&__t, inpp, sizeof(__t), readfn)	\
+		|| (*(outp) = __opcode_thumb32_compose(			\
+					*(outp),			\
+					__mem_to_opcode_thumb16(__t)),	\
+		    0);							\
+})
+#define ___read_advance(outp, inpp, size, readfn) ({			\
+	int __status;							\
+									\
+	__status = readfn(outp, *(inpp), size);				\
+	if (!__status)							\
+		*(inpp) = (typeof(*(inpp)))((uintptr_t)*(inpp) + (size)); \
+									\
+	__status;							\
+})
+
+#define __opcode_read_arm(outp, inpp, readfn) ({			\
+	BUILD_BUG_ON(sizeof(*(outp)) != sizeof(arm_opcode_t));		\
+									\
+	___read_advance(outp, inpp, sizeof(arm_opcode_t), readfn)	\
+	|| (*(outp) = __mem_to_opcode_arm(*(outp)),			\
+	    0);								\
+})
+
+/* __opcode_read_<isa>_simple(
+ *	arm_opcode_t *outp,
+ *	void const **inpp
+ * )
+ *
+ * Reads n Thumb-2 instruction from memory, without error checks.
+ * This macro will always succeed and return 0.  Otherwise, it is similar
+ * to __opcode_read_thumb().
+ */
+#define __opcode_read_thumb_simple(outp, inp)				\
+	__opcode_read_thumb(outp, inp, ___read16_simple)
+#define __opcode_read_arm_simple(outp, inp)				\
+	__opcode_read_arm(outp, inp, ___read32_simple)
+	
+#define ___read16_simple(outp, inp, size) 				\
+	(*(outp) = *(u16 *)(inp), 0)
+#define ___read32_simple(outp, inp, size) 				\
+	(*(outp) = *(u32 *)(inp), 0)
+
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define __opcode_read(outp, inpp, readfn) \
+	__opcode_read_thumb(outp, inpp, readfn)
+#define __opcode_read_simple(outp, inpp) \
+	__opcode_read_thumb_simple(outp, inpp)
+#else
+#define __opcode_read(outp, inpp, readfn) \
+	__opcode_read_arm(outp, inpp, readfn)
+#define __opcode_read_simple(outp, inpp) \
+	__opcode_read_arm_simple(outp, inpp)
+#endif
+
+/* Maybe add some C static functions here, with proper type annotations */
+
+#endif /* ! __ARM_OPCODES_H */
-- 
1.7.4.1

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Fri, Nov 25, 2011 at 11:28:13AM +0000, Dave Martin wrote:
> This patch adds some endianness-agnostic helpers to convert machine
> instructions between canonical integer form and in-memory
> representation, and also provides a transparent way to read a
> single Thumb instruction from memory, without the need to know the
> size in advance or write explicit condition checks.
> 
> A canonical integer form for representing instructions is also
> formalised here.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---

Notes:

 * We don't necessarily need everything that's in this example header

 * A generic instruction writing macro could be added, similar to the
   generic read macro, if this looks useful.

 * We could align the use of undefined instruction encodings across
   the kernel via this header: all instruction sets allow a
   guaranteed undefined instruction with up to 8 choosable bits,
   and we can also define additional generic and "NULL" encodings
   for internal use by kernel code which deals with instruction
   opcodes -- to signal special cases and error values etc.

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Rabin Vincent]

On Fri, Nov 25, 2011 at 16:58, Dave Martin <dave.martin@linaro.org> wrote:
> +#ifdef CONFIG_CPU_ENDIAN_BE8
> +#define __opcode_to_mem_arm(x) swab32(x)
> +#define __opcode_to_mem_thumb16(x) swab16(x)
> +#define __opcode_to_mem_thumb32(x) swahb32(x)
> +#else
> +#define __opcode_to_mem_arm(x) (x) ((u32)(x))
> +#define __opcode_to_mem_thumb16(x) ((u16)(x))
> +#define __opcode_to_mem_thumb32(x) swahw32(x)
> +#endif

The current kprobes code does:

#ifndef __ARMEB__ /* Swap halfwords for little-endian */
                bkp = (bkp >> 16) | (bkp << 16);
#endif

There seems to be a difference between your macros and that for the case
__ARMEB__ + !CONFIG_CPU_ENDIAN_BE8.  Or is that combination not
possible?

> +
> +#define __mem_to_opcode_arm(x) __opcode_to_mem_arm(x)
> +#define __mem_to_opcode_thumb16(x) __opcode_to_mem_thumb16(x)
> +#define __mem_to_opcode_thumb32(x) __opcode_to_mem_thumb32(x)
> +
> +/* Operations specific to Thumb opcodes */
> +
> +/* Instruction size checks: */
> +#define __opcode_is_thumb32(x) ((u32)(x) >= 0xE8000000UL)
> +#define __opcode_is_thumb16(x) ((u32)(x) < 0xE800UL)
> +
> +/* Operations to construct or split 32-bit Thumb instructions: */
> +#define __opcode_thumb32_first(x) ((u16)((thumb_opcode) >> 16))
> +#define __opcode_thumb32_second(x) ((u16)(thumb_opcode))
> +#define __opcode_thumb32_compose(first, second) \
> + ? ? ? (((u32)(u16)(first) << 16) | (u32)(u16)(second))

All of these could certainly find use in the files being touched by
the jump label patchset:

diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index c46d18b..c437a9d 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -72,12 +72,13 @@ static int ftrace_modify_code(unsigned long pc,
unsigned long old,
 {
 	unsigned long replaced;

-#ifndef __ARMEB__
 	if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
-		old = (old >> 16) | (old << 16);
-		new = (new >> 16) | (new << 16);
+		old = __opcode_to_mem_thumb32(old);
+		new = __opcode_to_mem_thumb32(new);
+	} else {
+		old = __opcode_to_mem_arm(old);
+		new = __opcode_to_mem_arm(new);
 	}
-#endif

 	if (old) {
 		if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
diff --git a/arch/arm/kernel/insn.c b/arch/arm/kernel/insn.c
index f65b68c..9079997 100644
--- a/arch/arm/kernel/insn.c
+++ b/arch/arm/kernel/insn.c
@@ -27,7 +27,7 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned
long addr, bool link)
 	if (link)
 		second |= 1 << 14;

-	return (first << 16) | second;
+	return __opcode_thumb32_compose(first, second);
 }

 static unsigned long
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index 15df8a5..5398659 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -17,21 +17,21 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
 	bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
 	int size;

-	if (thumb2 && !is_wide_instruction(insn)) {
-		*(u16 *)addr = insn;
+	if (thumb2 && __opcode_is_thumb16(insn)) {
+		*(u16 *)addr = __opcode_to_mem_thumb16(insn);
 		size = sizeof(u16);
 	} else if (thumb2 && ((uintptr_t)addr & 2)) {
 		u16 *addrh = addr;

-		addrw[0] = insn >> 16;
-		addrw[1] = insn & 0xffff;
+		addrw[0] = __opcode_thumb32_first(insn);
+		addrw[1] = __opcode_thumb32_second(insn);

 		size = sizeof(u32);
 	} else {
-#ifndef __ARMEB__
 		if (thumb2)
-			insn = (insn >> 16) | (insn << 16);
-#endif
+			insn = __opcode_to_mem_thumb32(insn);
+		else
+			insn = __opcode_to_mem_arm(insn);

 		*(u32 *)addr = insn;
 		size = sizeof(u32);
@@ -61,7 +61,7 @@ void __kprobes patch_text(void *addr, unsigned int insn)
 		stop_machine(patch_text_stop_machine, &patch, cpu_online_mask);
 	} else {
 		bool straddles_word = IS_ENABLED(CONFIG_THUMB2_KERNEL)
-				      && is_wide_instruction(insn)
+				      && __opcode_is_thumb32(insn)
 				      && ((uintptr_t)addr & 2);

 		if (straddles_word)

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Mon, Nov 28, 2011 at 10:29:14PM +0530, Rabin Vincent wrote:
> On Fri, Nov 25, 2011 at 16:58, Dave Martin <dave.martin@linaro.org> wrote:
> > +#ifdef CONFIG_CPU_ENDIAN_BE8
> > +#define __opcode_to_mem_arm(x) swab32(x)
> > +#define __opcode_to_mem_thumb16(x) swab16(x)
> > +#define __opcode_to_mem_thumb32(x) swahb32(x)
> > +#else
> > +#define __opcode_to_mem_arm(x) (x) ((u32)(x))
> > +#define __opcode_to_mem_thumb16(x) ((u16)(x))
> > +#define __opcode_to_mem_thumb32(x) swahw32(x)
> > +#endif
> 
> The current kprobes code does:
> 
> #ifndef __ARMEB__ /* Swap halfwords for little-endian */
>                 bkp = (bkp >> 16) | (bkp << 16);
> #endif
> 
> There seems to be a difference between your macros and that for the case
> __ARMEB__ + !CONFIG_CPU_ENDIAN_BE8.  Or is that combination not
> possible?
> 

For building the kernel, it is effectively impossible, since you can't
have Thumb-2 code on BE32 platforms.  The opcode_to_mem_thumb*()
definitions are currently "don't cares" for this configuration in
my RFC, but we should probably clarify how things should behave in this
case.

The kprobes code does not look correct for the big-endian case, since
the bytes are not swapped -- this will result in big-endian words or
halfwords in memory, which would be correct for BE32 but not for BE8
(where instructions are always little-endian).

So they're both wrong, in different ways if I've understood correctly.


I'm not exactly sure how we handle BE32, or whether we need to.  I
believe that for BE32 it would be correct to leave the canonical
instruction completely un-swapped, because instructions are natively
big-endian on those platforms.  Note that BE32 is only applicable to
older versions of the architecture, and so Thumb-2 is not applicable to
any BE32 platform, so the only situation where we would care is if
kprobes, ftrace or similar allows breakpoints or tracepoints to be set
in userspace Thumb code on these platforms.

I think that __ARMEB__ encompasses any big-endian target including BE8
and BE32, so we might need to be a bit careful about how we use it.


Rabin, did the __opcode_read stuff look useful for ftrace?  My idea
was to factor out the logic of how to read/write a whole instruction,
but my proposal may be overkill...


Tixy, do you have a view on these issues?

> > +
> > +#define __mem_to_opcode_arm(x) __opcode_to_mem_arm(x)
> > +#define __mem_to_opcode_thumb16(x) __opcode_to_mem_thumb16(x)
> > +#define __mem_to_opcode_thumb32(x) __opcode_to_mem_thumb32(x)
> > +
> > +/* Operations specific to Thumb opcodes */
> > +
> > +/* Instruction size checks: */
> > +#define __opcode_is_thumb32(x) ((u32)(x) >= 0xE8000000UL)
> > +#define __opcode_is_thumb16(x) ((u32)(x) < 0xE800UL)
> > +
> > +/* Operations to construct or split 32-bit Thumb instructions: */
> > +#define __opcode_thumb32_first(x) ((u16)((thumb_opcode) >> 16))
> > +#define __opcode_thumb32_second(x) ((u16)(thumb_opcode))
> > +#define __opcode_thumb32_compose(first, second) \
> > + ? ? ? (((u32)(u16)(first) << 16) | (u32)(u16)(second))
> 
> All of these could certainly find use in the files being touched by
> the jump label patchset:

I haven't reviewed everything, but yes -- that looks like the kind
of usage I was trying to enable.

Cheers
---Dave

> 
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index c46d18b..c437a9d 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -72,12 +72,13 @@ static int ftrace_modify_code(unsigned long pc,
> unsigned long old,
>  {
>  	unsigned long replaced;
> 
> -#ifndef __ARMEB__
>  	if (IS_ENABLED(CONFIG_THUMB2_KERNEL)) {
> -		old = (old >> 16) | (old << 16);
> -		new = (new >> 16) | (new << 16);
> +		old = __opcode_to_mem_thumb32(old);
> +		new = __opcode_to_mem_thumb32(new);
> +	} else {
> +		old = __opcode_to_mem_arm(old);
> +		new = __opcode_to_mem_arm(new);
>  	}
> -#endif
> 
>  	if (old) {
>  		if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
> diff --git a/arch/arm/kernel/insn.c b/arch/arm/kernel/insn.c
> index f65b68c..9079997 100644
> --- a/arch/arm/kernel/insn.c
> +++ b/arch/arm/kernel/insn.c
> @@ -27,7 +27,7 @@ __arm_gen_branch_thumb2(unsigned long pc, unsigned
> long addr, bool link)
>  	if (link)
>  		second |= 1 << 14;
> 
> -	return (first << 16) | second;
> +	return __opcode_thumb32_compose(first, second);
>  }
> 
>  static unsigned long
> diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
> index 15df8a5..5398659 100644
> --- a/arch/arm/kernel/patch.c
> +++ b/arch/arm/kernel/patch.c
> @@ -17,21 +17,21 @@ void __kprobes __patch_text(void *addr, unsigned int insn)
>  	bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
>  	int size;
> 
> -	if (thumb2 && !is_wide_instruction(insn)) {
> -		*(u16 *)addr = insn;
> +	if (thumb2 && __opcode_is_thumb16(insn)) {
> +		*(u16 *)addr = __opcode_to_mem_thumb16(insn);
>  		size = sizeof(u16);
>  	} else if (thumb2 && ((uintptr_t)addr & 2)) {
>  		u16 *addrh = addr;
> 
> -		addrw[0] = insn >> 16;
> -		addrw[1] = insn & 0xffff;
> +		addrw[0] = __opcode_thumb32_first(insn);
> +		addrw[1] = __opcode_thumb32_second(insn);
> 
>  		size = sizeof(u32);
>  	} else {
> -#ifndef __ARMEB__
>  		if (thumb2)
> -			insn = (insn >> 16) | (insn << 16);
> -#endif
> +			insn = __opcode_to_mem_thumb32(insn);
> +		else
> +			insn = __opcode_to_mem_arm(insn);
> 
>  		*(u32 *)addr = insn;
>  		size = sizeof(u32);
> @@ -61,7 +61,7 @@ void __kprobes patch_text(void *addr, unsigned int insn)
>  		stop_machine(patch_text_stop_machine, &patch, cpu_online_mask);
>  	} else {
>  		bool straddles_word = IS_ENABLED(CONFIG_THUMB2_KERNEL)
> -				      && is_wide_instruction(insn)
> +				      && __opcode_is_thumb32(insn)
>  				      && ((uintptr_t)addr & 2);
> 
>  		if (straddles_word)

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Tixy]

On Mon, 2011-11-28 at 17:41 +0000, Dave Martin wrote:
> On Mon, Nov 28, 2011 at 10:29:14PM +0530, Rabin Vincent wrote:
> > On Fri, Nov 25, 2011 at 16:58, Dave Martin <dave.martin@linaro.org> wrote:
> > > +#ifdef CONFIG_CPU_ENDIAN_BE8
> > > +#define __opcode_to_mem_arm(x) swab32(x)
> > > +#define __opcode_to_mem_thumb16(x) swab16(x)
> > > +#define __opcode_to_mem_thumb32(x) swahb32(x)
> > > +#else
> > > +#define __opcode_to_mem_arm(x) (x) ((u32)(x))
> > > +#define __opcode_to_mem_thumb16(x) ((u16)(x))
> > > +#define __opcode_to_mem_thumb32(x) swahw32(x)
> > > +#endif
> > 
> > The current kprobes code does:
> > 
> > #ifndef __ARMEB__ /* Swap halfwords for little-endian */
> >                 bkp = (bkp >> 16) | (bkp << 16);
> > #endif
> > 
> > There seems to be a difference between your macros and that for the case
> > __ARMEB__ + !CONFIG_CPU_ENDIAN_BE8.  Or is that combination not
> > possible?
> > 
> 
> For building the kernel, it is effectively impossible, since you can't
> have Thumb-2 code on BE32 platforms.  The opcode_to_mem_thumb*()
> definitions are currently "don't cares" for this configuration in
> my RFC, but we should probably clarify how things should behave in this
> case.
> 
> The kprobes code does not look correct for the big-endian case, since
> the bytes are not swapped -- this will result in big-endian words or
> halfwords in memory, which would be correct for BE32 but not for BE8
> (where instructions are always little-endian).
> 
> So they're both wrong, in different ways if I've understood correctly.
> 
> 
> I'm not exactly sure how we handle BE32, or whether we need to.  I
> believe that for BE32 it would be correct to leave the canonical
> instruction completely un-swapped, because instructions are natively
> big-endian on those platforms.  Note that BE32 is only applicable to
> older versions of the architecture, and so Thumb-2 is not applicable to
> any BE32 platform, so the only situation where we would care is if
> kprobes, ftrace or similar allows breakpoints or tracepoints to be set
> in userspace Thumb code on these platforms.
> 
> I think that __ARMEB__ encompasses any big-endian target including BE8
> and BE32, so we might need to be a bit careful about how we use it.
> 
> 
> Rabin, did the __opcode_read stuff look useful for ftrace?  My idea
> was to factor out the logic of how to read/write a whole instruction,
> but my proposal may be overkill...
> 
> 
> Tixy, do you have a view on these issues?

I had to read the ARM ARM, I wasn't aware of BE8 :-)

My view is that the the current kprobes code just doesn't handle BE8.
Anywhere where it reads the original instruction, writes a breakpoint or
restores the instruction would need to swap the byte order. To do that,
the proposed mem_to_opcode/opcode_to_mem helpers would be useful.

However, the read/write a whole instruction functions do look a bit
overkill. Especially if the number of places using these is small, due
to factorisations like Rabin's __patch_text().

-- 
Tixy

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Mon, Nov 28, 2011 at 07:20:33PM +0000, Tixy wrote:
> On Mon, 2011-11-28 at 17:41 +0000, Dave Martin wrote:
> > On Mon, Nov 28, 2011 at 10:29:14PM +0530, Rabin Vincent wrote:
> > > On Fri, Nov 25, 2011 at 16:58, Dave Martin <dave.martin@linaro.org> wrote:
> > > > +#ifdef CONFIG_CPU_ENDIAN_BE8
> > > > +#define __opcode_to_mem_arm(x) swab32(x)
> > > > +#define __opcode_to_mem_thumb16(x) swab16(x)
> > > > +#define __opcode_to_mem_thumb32(x) swahb32(x)
> > > > +#else
> > > > +#define __opcode_to_mem_arm(x) (x) ((u32)(x))
> > > > +#define __opcode_to_mem_thumb16(x) ((u16)(x))
> > > > +#define __opcode_to_mem_thumb32(x) swahw32(x)
> > > > +#endif
> > > 
> > > The current kprobes code does:
> > > 
> > > #ifndef __ARMEB__ /* Swap halfwords for little-endian */
> > >                 bkp = (bkp >> 16) | (bkp << 16);
> > > #endif
> > > 
> > > There seems to be a difference between your macros and that for the case
> > > __ARMEB__ + !CONFIG_CPU_ENDIAN_BE8.  Or is that combination not
> > > possible?
> > > 
> > 
> > For building the kernel, it is effectively impossible, since you can't
> > have Thumb-2 code on BE32 platforms.  The opcode_to_mem_thumb*()
> > definitions are currently "don't cares" for this configuration in
> > my RFC, but we should probably clarify how things should behave in this
> > case.
> > 
> > The kprobes code does not look correct for the big-endian case, since
> > the bytes are not swapped -- this will result in big-endian words or
> > halfwords in memory, which would be correct for BE32 but not for BE8
> > (where instructions are always little-endian).
> > 
> > So they're both wrong, in different ways if I've understood correctly.
> > 
> > 
> > I'm not exactly sure how we handle BE32, or whether we need to.  I
> > believe that for BE32 it would be correct to leave the canonical
> > instruction completely un-swapped, because instructions are natively
> > big-endian on those platforms.  Note that BE32 is only applicable to
> > older versions of the architecture, and so Thumb-2 is not applicable to
> > any BE32 platform, so the only situation where we would care is if
> > kprobes, ftrace or similar allows breakpoints or tracepoints to be set
> > in userspace Thumb code on these platforms.
> > 
> > I think that __ARMEB__ encompasses any big-endian target including BE8
> > and BE32, so we might need to be a bit careful about how we use it.
> > 
> > 
> > Rabin, did the __opcode_read stuff look useful for ftrace?  My idea
> > was to factor out the logic of how to read/write a whole instruction,
> > but my proposal may be overkill...
> > 
> > 
> > Tixy, do you have a view on these issues?
> 
> I had to read the ARM ARM, I wasn't aware of BE8 :-)

BE8 is "the" big-endianness, at least since about ARMv6.  It's the only
form of big-endianness applicable to any CPU running Thumb-2.

Do you care about being to able to set probes in Thumb user code when
the kernel is not Thumb-2, or do you simply not support that scenario
at all?  (Thinking about, I'm guessing we don't currently support that?)


> My view is that the the current kprobes code just doesn't handle BE8.
> Anywhere where it reads the original instruction, writes a breakpoint or
> restores the instruction would need to swap the byte order. To do that,
> the proposed mem_to_opcode/opcode_to_mem helpers would be useful.
> 
> However, the read/write a whole instruction functions do look a bit
> overkill. Especially if the number of places using these is small, due
> to factorisations like Rabin's __patch_text().

I'm inclined to agree -- it seemed worthwhile to see how possible it was,
but while the idea being abstracted is straightforward enough, trying to
do this kind of thing using the C preprocessor is like trying to build
a stepladder out of custard.

Cheers
---Dave

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Jon Medhurst (Tixy)]

On Tue, 2011-11-29 at 10:42 +0000, Dave Martin wrote:
> Do you care about being to able to set probes in Thumb user code when
> the kernel is not Thumb-2, or do you simply not support that scenario
> at all?  (Thinking about, I'm guessing we don't currently support that?)

kprobes are only designed to probe kernel code, that's what the 'k'
stands for. ;-)

-- 
Tixy

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Tue, Nov 29, 2011 at 02:06:08PM +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2011-11-29 at 10:42 +0000, Dave Martin wrote:
> > Do you care about being to able to set probes in Thumb user code when
> > the kernel is not Thumb-2, or do you simply not support that scenario
> > at all?  (Thinking about, I'm guessing we don't currently support that?)
> 
> kprobes are only designed to probe kernel code, that's what the 'k'
> stands for. ;-)

Right, I think I may have been confusing that with some ftrace features.

Cheers
---Dave

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Rabin Vincent]

On Mon, Nov 28, 2011 at 05:41:22PM +0000, Dave Martin wrote:
> Rabin, did the __opcode_read stuff look useful for ftrace?  My idea
> was to factor out the logic of how to read/write a whole instruction,
> but my proposal may be overkill...

No.  ftrace just uses probe_kernel_read() and always reads a 32-bit
instruction.

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Thu, Dec 01, 2011 at 10:56:40PM +0530, Rabin Vincent wrote:
> On Mon, Nov 28, 2011 at 05:41:22PM +0000, Dave Martin wrote:
> > Rabin, did the __opcode_read stuff look useful for ftrace?  My idea
> > was to factor out the logic of how to read/write a whole instruction,
> > but my proposal may be overkill...
> 
> No.  ftrace just uses probe_kernel_read() and always reads a 32-bit
> instruction.

Right -- fair enough.

---Dave

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Will Deacon]

Hi Dave,

On Fri, Nov 25, 2011 at 11:28:13AM +0000, Dave Martin wrote:
> This patch adds some endianness-agnostic helpers to convert machine
> instructions between canonical integer form and in-memory
> representation, and also provides a transparent way to read a
> single Thumb instruction from memory, without the need to know the
> size in advance or write explicit condition checks.
> 
> A canonical integer form for representing instructions is also
> formalised here.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>  arch/arm/include/asm/opcodes.h |  162 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 162 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/include/asm/opcodes.h

It looks like I might need to implement a basic disassembler for the
hw_breakpoint code and I would certainly like to reuse as much code as I
can. This header could obviously provide the code to fetch and format the
instruction, but it would be nice to have some extra helpers to aid
decoding.

Tixy - how much work do you reckon it would be to rework your kprobes
decoding code into a generic `here are my callbacks, please decode this
instruction stream for me' type thing?

All I want for hw_breakpoint is to know whether an instruction is a load or
a store, but even for that it looks like I'll need to duplicate a lot of
stuff.

Will

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Tue, Dec 06, 2011 at 03:08:55PM +0000, Will Deacon wrote:
> Hi Dave,
> 
> On Fri, Nov 25, 2011 at 11:28:13AM +0000, Dave Martin wrote:
> > This patch adds some endianness-agnostic helpers to convert machine
> > instructions between canonical integer form and in-memory
> > representation, and also provides a transparent way to read a
> > single Thumb instruction from memory, without the need to know the
> > size in advance or write explicit condition checks.
> > 
> > A canonical integer form for representing instructions is also
> > formalised here.
> > 
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> > ---
> >  arch/arm/include/asm/opcodes.h |  162 ++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 162 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/include/asm/opcodes.h
> 
> It looks like I might need to implement a basic disassembler for the
> hw_breakpoint code and I would certainly like to reuse as much code as I
> can. This header could obviously provide the code to fetch and format the
> instruction, but it would be nice to have some extra helpers to aid
> decoding.
> 
> Tixy - how much work do you reckon it would be to rework your kprobes
> decoding code into a generic `here are my callbacks, please decode this
> instruction stream for me' type thing?
> 
> All I want for hw_breakpoint is to know whether an instruction is a load or
> a store, but even for that it looks like I'll need to duplicate a lot of
> stuff.

Note, I'm currently waiting on Leif to repost his opcodes.h before I
repost my instration-swabbing additions on top of it, since the swabbing
stuff seems to be strictly non-urgent.

Cheers
---Dave

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Jon Medhurst (Tixy)]

I've cut down the cc list...

On Tue, 2011-12-06 at 15:08 +0000, Will Deacon wrote:
[...]
> Tixy - how much work do you reckon it would be to rework your kprobes
> decoding code into a generic `here are my callbacks, please decode this
> instruction stream for me' type thing?
>
> All I want for hw_breakpoint is to know whether an instruction is a load or
> a store, but even for that it looks like I'll need to duplicate a lot of
> stuff.

The kprobes instruction decoding is pretty specialised if you look at
it. Most of the complexity is in the data tables, (grep for "const union
decode_item") which are processed by kprobe_decode_insn(). For
documentation look for comments before "enum decode_type" and
kprobe_decode_insn(). I suspect that to try and create something generic
out of these would be major work.

An alternative would be to bend the data tables to your purpose by
squeezing in and 'instruction type' parameter into the table (ldr, str,
other) and then parsing the tables with a different function to get the
instruction type. You may find though that there are some classes of
instruction that the existing tables don't decode fully, because kprobes
isn't interested in distinguishing between them.

-- 
Tixy

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Bi Junxiao]

on 12/06/2011 11:20 PM Dave Martin wrote:
> On Tue, Dec 06, 2011 at 03:08:55PM +0000, Will Deacon wrote:
>    
>> Hi Dave,
>>
>> On Fri, Nov 25, 2011 at 11:28:13AM +0000, Dave Martin wrote:
>>      
>>> This patch adds some endianness-agnostic helpers to convert machine
>>> instructions between canonical integer form and in-memory
>>> representation, and also provides a transparent way to read a
>>> single Thumb instruction from memory, without the need to know the
>>> size in advance or write explicit condition checks.
>>>
>>> A canonical integer form for representing instructions is also
>>> formalised here.
>>>
>>> Signed-off-by: Dave Martin<dave.martin@linaro.org>
>>> ---
>>>   arch/arm/include/asm/opcodes.h |  162 ++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 162 insertions(+), 0 deletions(-)
>>>   create mode 100644 arch/arm/include/asm/opcodes.h
>>>        
>> It looks like I might need to implement a basic disassembler for the
>> hw_breakpoint code and I would certainly like to reuse as much code as I
>> can. This header could obviously provide the code to fetch and format the
>> instruction, but it would be nice to have some extra helpers to aid
>> decoding.
>>
>> Tixy - how much work do you reckon it would be to rework your kprobes
>> decoding code into a generic `here are my callbacks, please decode this
>> instruction stream for me' type thing?
>>
>> All I want for hw_breakpoint is to know whether an instruction is a load or
>> a store, but even for that it looks like I'll need to duplicate a lot of
>> stuff.
>>      
> Note, I'm currently waiting on Leif to repost his opcodes.h before I
> repost my instration-swabbing additions on top of it, since the swabbing
> stuff seems to be strictly non-urgent.
>    
I am also waiting for your patch to do my be8 fix.
> Cheers
> ---Dave
>
>    

[RFC PATCH] ARM: Add generic instruction opcode manipulation helpers

[Dave Martin]

On Wed, Dec 07, 2011 at 01:22:34PM +0800, Bi Junxiao wrote:
> on 12/06/2011 11:20 PM Dave Martin wrote:
> >On Tue, Dec 06, 2011 at 03:08:55PM +0000, Will Deacon wrote:
> >>Hi Dave,
> >>
> >>On Fri, Nov 25, 2011 at 11:28:13AM +0000, Dave Martin wrote:
> >>>This patch adds some endianness-agnostic helpers to convert machine
> >>>instructions between canonical integer form and in-memory
> >>>representation, and also provides a transparent way to read a
> >>>single Thumb instruction from memory, without the need to know the
> >>>size in advance or write explicit condition checks.
> >>>
> >>>A canonical integer form for representing instructions is also
> >>>formalised here.
> >>>
> >>>Signed-off-by: Dave Martin<dave.martin@linaro.org>
> >>>---
> >>>  arch/arm/include/asm/opcodes.h |  162 ++++++++++++++++++++++++++++++++++++++++
> >>>  1 files changed, 162 insertions(+), 0 deletions(-)
> >>>  create mode 100644 arch/arm/include/asm/opcodes.h
> >>It looks like I might need to implement a basic disassembler for the
> >>hw_breakpoint code and I would certainly like to reuse as much code as I
> >>can. This header could obviously provide the code to fetch and format the
> >>instruction, but it would be nice to have some extra helpers to aid
> >>decoding.
> >>
> >>Tixy - how much work do you reckon it would be to rework your kprobes
> >>decoding code into a generic `here are my callbacks, please decode this
> >>instruction stream for me' type thing?
> >>
> >>All I want for hw_breakpoint is to know whether an instruction is a load or
> >>a store, but even for that it looks like I'll need to duplicate a lot of
> >>stuff.
> >Note, I'm currently waiting on Leif to repost his opcodes.h before I
> >repost my instration-swabbing additions on top of it, since the swabbing
> >stuff seems to be strictly non-urgent.
> I am also waiting for your patch to do my be8 fix.

OK -- in that case I will clean up and repost my patch anyway.

The two proposed bits of functionality in that header are independent,
so the later merge shouldn't affect what you're doing.

Cheers
---Dave