| Message ID | 1374763778-5305-2-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: > The arch_kgdb_breakpoint() function uses an inline assembly directive > to assemble a specific instruction using .word. This means the linker > will not treat is as an instruction, and therefore incorrectly swap > the endian-ness if running BE8. > > Note, not tested, please comment if this is wrong. Can't you just look at the vmlinux disassembly to confirm this is working correctly? Will > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > arch/arm/include/asm/kgdb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h > index 48066ce..76227c8 100644 > --- a/arch/arm/include/asm/kgdb.h > +++ b/arch/arm/include/asm/kgdb.h > @@ -41,7 +41,7 @@ > > static inline void arch_kgdb_breakpoint(void) > { > - asm(".word 0xe7ffdeff"); > + asm(".inst 0xe7ffdeff"); > } > > extern void kgdb_handle_bus_error(void); > -- > 1.7.10.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: > The arch_kgdb_breakpoint() function uses an inline assembly directive > to assemble a specific instruction using .word. This means the linker > will not treat is as an instruction, and therefore incorrectly swap > the endian-ness if running BE8. > > Note, not tested, please comment if this is wrong. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > arch/arm/include/asm/kgdb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h > index 48066ce..76227c8 100644 > --- a/arch/arm/include/asm/kgdb.h > +++ b/arch/arm/include/asm/kgdb.h > @@ -41,7 +41,7 @@ > > static inline void arch_kgdb_breakpoint(void) > { > - asm(".word 0xe7ffdeff"); > + asm(".inst 0xe7ffdeff"); Yikes, this isn't going to work in a Thumb kernel. We should make HAVE_ARCH_KGDB depend on !THUMB2_KERNEL until/unless that gets fixed... It looks like the incompatibilities may be more extensive than just this one instruction. For the ARM case, similarly to the other patches, please use the __inst macros from <asm/opcodes.h> instead of emitting the opcode explicitly. Cheers ---Dave
On 25/07/13 18:11, Dave Martin wrote: > On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: >> The arch_kgdb_breakpoint() function uses an inline assembly directive >> to assemble a specific instruction using .word. This means the linker >> will not treat is as an instruction, and therefore incorrectly swap >> the endian-ness if running BE8. >> >> Note, not tested, please comment if this is wrong. >> >> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk> >> --- >> arch/arm/include/asm/kgdb.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h >> index 48066ce..76227c8 100644 >> --- a/arch/arm/include/asm/kgdb.h >> +++ b/arch/arm/include/asm/kgdb.h >> @@ -41,7 +41,7 @@ >> >> static inline void arch_kgdb_breakpoint(void) >> { >> - asm(".word 0xe7ffdeff"); >> + asm(".inst 0xe7ffdeff"); > > Yikes, this isn't going to work in a Thumb kernel. > > We should make HAVE_ARCH_KGDB depend on !THUMB2_KERNEL until/unless that > gets fixed... It looks like the incompatibilities may be more extensive > than just this one instruction. > > > For the ARM case, similarly to the other patches, please use the __inst > macros from<asm/opcodes.h> instead of emitting the opcode explicitly. See previous objections to that, plus they're marked for internal use only!
On Thu, Jul 25, 2013 at 06:13:18PM +0100, Ben Dooks wrote: > On 25/07/13 18:11, Dave Martin wrote: > >On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: > >>The arch_kgdb_breakpoint() function uses an inline assembly directive > >>to assemble a specific instruction using .word. This means the linker > >>will not treat is as an instruction, and therefore incorrectly swap > >>the endian-ness if running BE8. > >> > >>Note, not tested, please comment if this is wrong. > >> > >>Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk> > >>--- > >> arch/arm/include/asm/kgdb.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h > >>index 48066ce..76227c8 100644 > >>--- a/arch/arm/include/asm/kgdb.h > >>+++ b/arch/arm/include/asm/kgdb.h > >>@@ -41,7 +41,7 @@ > >> > >> static inline void arch_kgdb_breakpoint(void) > >> { > >>- asm(".word 0xe7ffdeff"); > >>+ asm(".inst 0xe7ffdeff"); > > > >Yikes, this isn't going to work in a Thumb kernel. > > > >We should make HAVE_ARCH_KGDB depend on !THUMB2_KERNEL until/unless that > >gets fixed... It looks like the incompatibilities may be more extensive > >than just this one instruction. > > > > > >For the ARM case, similarly to the other patches, please use the __inst > >macros from<asm/opcodes.h> instead of emitting the opcode explicitly. > > See previous objections to that, plus they're marked for internal use > only! Ditto my counterarguments. I'm not emotionally attached to __inst*(), but we should use one or the other: either .inst or __inst(), not a mixture. However, the __inst macros work for inline asm and .S, and do more than just emitting a single opcode; see opcodes-virt.h for example, so while removing them isn't rocket science, it would involve churn in a few places. Note, the "Don't use these directly" comment only applies to the triple- underscored helpers. The broader "using these macros directly is poor practice" comment was an attempt to engourage people to write the likes of #define __KGDB_BKPT_INSTR __inst_arm(0xxe7ffdeff) static inline void arch_kgdb_breakpoint(void) { asm(__KGDB_BKPT_INSTR); } ...on the basis that this ought to be more readable. But this is a bit moot in a situation like this where the opcode is only used in one place, by itself, in a wrapper whose name makes the intent clear anyway. Cheers ---Dave
On 25/07/13 19:09, Dave Martin wrote: > On Thu, Jul 25, 2013 at 06:13:18PM +0100, Ben Dooks wrote: >> On 25/07/13 18:11, Dave Martin wrote: >>> On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: >>>> The arch_kgdb_breakpoint() function uses an inline assembly directive >>>> to assemble a specific instruction using .word. This means the linker >>>> will not treat is as an instruction, and therefore incorrectly swap >>>> the endian-ness if running BE8. >>>> >>>> Note, not tested, please comment if this is wrong. >>>> >>>> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk> >>>> --- >>>> arch/arm/include/asm/kgdb.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h >>>> index 48066ce..76227c8 100644 >>>> --- a/arch/arm/include/asm/kgdb.h >>>> +++ b/arch/arm/include/asm/kgdb.h >>>> @@ -41,7 +41,7 @@ >>>> >>>> static inline void arch_kgdb_breakpoint(void) >>>> { >>>> - asm(".word 0xe7ffdeff"); >>>> + asm(".inst 0xe7ffdeff"); >>> >>> Yikes, this isn't going to work in a Thumb kernel. >>> >>> We should make HAVE_ARCH_KGDB depend on !THUMB2_KERNEL until/unless that >>> gets fixed... It looks like the incompatibilities may be more extensive >>> than just this one instruction. >>> >>> >>> For the ARM case, similarly to the other patches, please use the __inst >>> macros from<asm/opcodes.h> instead of emitting the opcode explicitly. >> >> See previous objections to that, plus they're marked for internal use >> only! > > Ditto my counterarguments. I'm not emotionally attached to __inst*(), but > we should use one or the other: either .inst or __inst(), not a mixture. > However, the __inst macros work for inline asm and .S, and do more than > just emitting a single opcode; see opcodes-virt.h for example, so while > removing them isn't rocket science, it would involve churn in a few places. > > > Note, the "Don't use these directly" comment only applies to the triple- > underscored helpers. > > The broader "using these macros directly is poor practice" comment was > an attempt to engourage people to write the likes of > > #define __KGDB_BKPT_INSTR __inst_arm(0xxe7ffdeff) > > static inline void arch_kgdb_breakpoint(void) > { > asm(__KGDB_BKPT_INSTR); > } > > ...on the basis that this ought to be more readable. > > But this is a bit moot in a situation like this where the opcode is > only used in one place, by itself, in a wrapper whose name makes the > intent clear anyway. Ok, I will look into changing the patches in the next day or so and do the same fixes for kprobes.
On Fri, Jul 26, 2013 at 11:31:42AM +0100, Ben Dooks wrote: > On 25/07/13 19:09, Dave Martin wrote: > >On Thu, Jul 25, 2013 at 06:13:18PM +0100, Ben Dooks wrote: > >>On 25/07/13 18:11, Dave Martin wrote: > >>>On Thu, Jul 25, 2013 at 03:49:38PM +0100, Ben Dooks wrote: > >>>>The arch_kgdb_breakpoint() function uses an inline assembly directive > >>>>to assemble a specific instruction using .word. This means the linker > >>>>will not treat is as an instruction, and therefore incorrectly swap > >>>>the endian-ness if running BE8. > >>>> > >>>>Note, not tested, please comment if this is wrong. > >>>> > >>>>Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk> > >>>>--- > >>>> arch/arm/include/asm/kgdb.h | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>>diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h > >>>>index 48066ce..76227c8 100644 > >>>>--- a/arch/arm/include/asm/kgdb.h > >>>>+++ b/arch/arm/include/asm/kgdb.h > >>>>@@ -41,7 +41,7 @@ > >>>> > >>>> static inline void arch_kgdb_breakpoint(void) > >>>> { > >>>>- asm(".word 0xe7ffdeff"); > >>>>+ asm(".inst 0xe7ffdeff"); > >>> > >>>Yikes, this isn't going to work in a Thumb kernel. > >>> > >>>We should make HAVE_ARCH_KGDB depend on !THUMB2_KERNEL until/unless that > >>>gets fixed... It looks like the incompatibilities may be more extensive > >>>than just this one instruction. > >>> > >>> > >>>For the ARM case, similarly to the other patches, please use the __inst > >>>macros from<asm/opcodes.h> instead of emitting the opcode explicitly. > >> > >>See previous objections to that, plus they're marked for internal use > >>only! > > > >Ditto my counterarguments. I'm not emotionally attached to __inst*(), but > >we should use one or the other: either .inst or __inst(), not a mixture. > >However, the __inst macros work for inline asm and .S, and do more than > >just emitting a single opcode; see opcodes-virt.h for example, so while > >removing them isn't rocket science, it would involve churn in a few places. > > > > > >Note, the "Don't use these directly" comment only applies to the triple- > >underscored helpers. > > > >The broader "using these macros directly is poor practice" comment was > >an attempt to engourage people to write the likes of > > > >#define __KGDB_BKPT_INSTR __inst_arm(0xxe7ffdeff) > > > >static inline void arch_kgdb_breakpoint(void) > >{ > > asm(__KGDB_BKPT_INSTR); > >} > > > >...on the basis that this ought to be more readable. > > > >But this is a bit moot in a situation like this where the opcode is > >only used in one place, by itself, in a wrapper whose name makes the > >intent clear anyway. > > Ok, I will look into changing the patches in the next day or so > and do the same fixes for kprobes. OK, thanks. Cheers ---Dave
diff --git a/arch/arm/include/asm/kgdb.h b/arch/arm/include/asm/kgdb.h index 48066ce..76227c8 100644 --- a/arch/arm/include/asm/kgdb.h +++ b/arch/arm/include/asm/kgdb.h @@ -41,7 +41,7 @@ static inline void arch_kgdb_breakpoint(void) { - asm(".word 0xe7ffdeff"); + asm(".inst 0xe7ffdeff"); } extern void kgdb_handle_bus_error(void);
The arch_kgdb_breakpoint() function uses an inline assembly directive to assemble a specific instruction using .word. This means the linker will not treat is as an instruction, and therefore incorrectly swap the endian-ness if running BE8. Note, not tested, please comment if this is wrong. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- arch/arm/include/asm/kgdb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)