On Mon, 21 Mar 2011, Tixy wrote:
On Mon, 2011-03-21 at 13:18 -0400, Nicolas Pitre wrote:
On Mon, 21 Mar 2011, Tixy wrote:
From: Jon Medhurst tixy@yxit.co.uk
Signed-off-by: Jon Medhurst tixy@yxit.co.uk
arch/arm/kernel/traps.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 21ac43f..6468100 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -356,7 +356,12 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs) pc = (void __user *)instruction_pointer(regs); if (processor_mode(regs) == SVC_MODE) {
instr = *(u32 *) pc;
+#ifdef CONFIG_THUMB2_KERNEL
if (thumb_mode(regs))
instr = *(u16 *) pc;
else
+#endif
instr = *(u32 *) pc;
Although I suggested always using a 16-bit instruction for kprobe breakpoint, the above looks to be broken for 32-bit Thumb instructions.
It follows the existing pattern for user-side breakpoints and ptrace. These only use the first halfword of the instruction to select the hook to call; this hook then reads the second half and decides if it wants to handle it.
Right... well the user space Thumb access was originally meant for Thumb1 which has next to no 32-bit instructions. If it is easy to determine if an insn is 16 or 32 bits (I've seen x >= 0xe800 somewhere in your code) then I'd go with that and fetch the second half right away instead of duplicating the fetching of the second half everywhere.
I initially thought of fixing do_undefinstr to use the full 32 bits of the instruction, which seems the right thing to do. But backed off because it would introduce compatibility problems with existing handlers like ptrace and possibly other out-of-tree code.
We don't care about out-of-tree code. There is a reason for merging code into mainline after all.
From a quick glance only one hook so far expects a 32-bit Thumb2
instruction i.e. for ptrace. So IMHO this is worth doing.
One of my biggest headaches with this is deciding the best way of storing a 32-bit thumb instruction in an integer. The human readable way [ 1st-half : 2nd-half ], or the way that a 32-bit value would be stored in memory, which varies between little/big-endian, or even always as little-endian.
I'd suggest:
insn = ((u16 *)pc)[0]; if (insn_is_wide(insn)) { insn <<= 16; insn |= ((u16 *)pc)[1]; }
effectively using a big endian representation, but which is similar to ARM mode instructions, and which would make sense regardless of the instruction mode/width when printed.
In the end I chickened out and avoided rewriting things I didn't need to. Perhaps I should have gone the whole hog and made all code dealing with ARM instructions use a union like:
struct arm_instruction { union { u32 arm; /* * thumb[0] = 16-bit thumb instruction or * 1st half-word of 32-bit thumb instruction * thumb[1] = 2nd half-word of 32-bit thumb instruction */ u16 thumb[2]; }; };
I don't think going that far is necessary. That won't gain much in clarity and this has the potential of making the resulting compiled code a bit less efficient.
Nicolas