kdgb fix

PATCH New Ben Dooks 3 participants 8 messages Jul 25, 2013 – Jul 26, 2013 Arm (32-bit)

lore

Share & tools

lore

Ben Dooks

author

2013-07-25 14:49 UTC #0 lore raw

Think this is correct, but do not have the time to test.

Ben Dooks

author

2013-07-25 14:49 UTC #1 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

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 <[email protected]>

---
 arch/arm/include/asm/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/include/asm/kgdb.h +1 -1

index 48066ce..76227c8 100644
@@ -41,7 +41,7 @@
 
 static inline void arch_kgdb_breakpoint(void)
 {
-	asm(".word 0xe7ffdeff");
+	asm(".inst 0xe7ffdeff");
 }
 
 extern void kgdb_handle_bus_error(void);

Signature · 1 line

1.7.10.4

Will Deacon

2013-07-25 16:02 UTC #2 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

Quoted 6 lines · Ben Dooks

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

Quoted 27 lines

Signed-off-by Ben Dooks <[email protected]>

---
 arch/arm/include/asm/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/include/asm/kgdb.h +1 -1

index 48066ce..76227c8 100644
@@ -41,7 +41,7 @@
 
 static inline void arch_kgdb_breakpoint(void)
 {
-	asm(".word 0xe7ffdeff");
+	asm(".inst 0xe7ffdeff");
 }
 
 extern void kgdb_handle_bus_error(void);

Signature · 7 lines

1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Dave Martin

2013-07-25 17:11 UTC #3 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

In reply to Ben Dooks

Quoted 17 lines · Ben Dooks

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 <[email protected]>
---
 arch/arm/include/asm/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/kgdb.h +0 -0
index 48066ce..76227c8 100644

arch/arm/include/asm/kgdb.h line 44

Quoted 5 lines

arch/arm/include/asm/kgdb.h +1 -1

@@ -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

Ben Dooks

author

2013-07-25 17:13 UTC #4 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

Quoted 26 lines · Dave Martin

Quoted 17 lines · Ben Dooks
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<[email protected]>
---
  arch/arm/include/asm/kgdb.h |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/kgdb.h +0 -0
index 48066ce..76227c8 100644
arch/arm/include/asm/kgdb.h line 44
Quoted 5 lines
arch/arm/include/asm/kgdb.h +1 -1
@@ -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.

Quoted 7 lines

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!

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Dave Martin

2013-07-25 18:09 UTC #5 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

Quoted 32 lines · Ben Dooks

Quoted 31 lines · Dave Martin
Quoted 17 lines · Ben Dooks
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<[email protected]>
---
 arch/arm/include/asm/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/kgdb.h +0 -0
index 48066ce..76227c8 100644
arch/arm/include/asm/kgdb.h line 44
Quoted 5 lines
arch/arm/include/asm/kgdb.h +1 -1
@@ -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.

Quoted 5 lines

Quoted 2 lines
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

Ben Dooks

author

2013-07-26 10:31 UTC #6 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

Quoted 59 lines · Dave Martin

Quoted 32 lines · Ben Dooks
Quoted 31 lines · Dave Martin
Quoted 17 lines · Ben Dooks
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<[email protected]>
---
  arch/arm/include/asm/kgdb.h |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/kgdb.h +0 -0
index 48066ce..76227c8 100644
arch/arm/include/asm/kgdb.h line 44
Quoted 5 lines
arch/arm/include/asm/kgdb.h +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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);
}

Quoted 5 lines

...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.

Signature · 2 lines

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Dave Martin

2013-07-26 14:59 UTC #7 lore raw

PATCH 1/1 ARM: kdgb: use .inst for data to be assembled as intruction

Quoted 60 lines · Ben Dooks

Quoted 59 lines · Dave Martin
Quoted 32 lines · Ben Dooks
Quoted 31 lines · Dave Martin
Quoted 17 lines · Ben Dooks
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<[email protected]>
---
 arch/arm/include/asm/kgdb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/kgdb.h +0 -0
index 48066ce..76227c8 100644
arch/arm/include/asm/kgdb.h line 44
Quoted 5 lines
arch/arm/include/asm/kgdb.h +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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);
}

Quoted 8 lines

Quoted 5 lines
...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