| Message ID | CAH+eYFBM36VqYAqZdiZnMP7AODh_SqYQWuoFDSyS3BDZDvHU5A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
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().
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
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. ;-)
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
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.
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
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)