[PATCH] Use definition for cmpxchg swi

[PATCH] Use definition for cmpxchg swi

[Russell King - ARM Linux]

Use a definition for the cmpxchg SWI instead of hard-coding the number.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/unistd.h |    4 ++++
 arch/arm/kernel/entry-armv.S  |    7 ++++---
 arch/arm/kernel/traps.c       |    2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 7020217..a15e740 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -402,6 +402,10 @@
 #define __ARM_NR_usr32			(__ARM_NR_BASE+4)
 #define __ARM_NR_set_tls		(__ARM_NR_BASE+5)
 
+#ifdef __KERNEL__
+#define __ARM_NR_cmpxchg		(__ARM_NR_BASE+0x00fff0)
+#endif
+
 /*
  * The following syscalls are obsolete and no longer available for EABI.
  */
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 0022b4d..d2903e3 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -21,6 +21,7 @@
 #include <mach/entry-macro.S>
 #include <asm/thread_notify.h>
 #include <asm/unwind.h>
+#include <asm/unistd.h>
 
 #include "entry-header.S"
 
@@ -908,10 +909,10 @@ __kuser_cmpxchg:				@ 0xffff0fc0
 	 * A special ghost syscall is used for that (see traps.c).
 	 */
 	stmfd	sp!, {r7, lr}
-	mov	r7, #0xff00		@ 0xfff0 into r7 for EABI
-	orr	r7, r7, #0xf0
-	swi	#0x9ffff0
+	ldr	r7, =1f			@ it's 20 bits
+	swi	__ARM_NR_cmpxchg
 	ldmfd	sp!, {r7, pc}
+1:	.word	__ARM_NR_cmpxchg
 
 #elif __LINUX_ARM_ARCH__ < 6
 
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 95718a6..3f361a7 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -528,7 +528,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 	 * __kuser_cmpxchg code in entry-armv.S should be aware of its
 	 * existence.  Don't ever use this from user code.
 	 */
-	case 0xfff0:
+	case NR(cmpxchg):
 	for (;;) {
 		extern void do_DataAbort(unsigned long addr, unsigned int fsr,
 					 struct pt_regs *regs);

[PATCH] Use definition for cmpxchg swi

[Nicolas Pitre]

On Sun, 8 Nov 2009, Russell King - ARM Linux wrote:

> Use a definition for the cmpxchg SWI instead of hard-coding the number.

Well...  Initially I didn't use a definition in unistd.h because I 
didn't want user space to "see" it and assume this was part of the 
kernel ABI since this is actually just an implementation detail that 
depends on a particular kernel configuration.  If you want to define it 
with this patch then I'd suggest copying the related comment from 
traps.c near the definition as well:

         * *NOTE*: This is a ghost syscall private to the kernel.  Only the
         * __kuser_cmpxchg code in entry-armv.S should be aware of its
         * existence.  Don't ever use this from user code.


Nicolas

[PATCH] Use definition for cmpxchg swi

[Russell King - ARM Linux]

On Sun, Nov 08, 2009 at 08:42:51PM -0500, Nicolas Pitre wrote:
> On Sun, 8 Nov 2009, Russell King - ARM Linux wrote:
> 
> > Use a definition for the cmpxchg SWI instead of hard-coding the number.
> 
> Well...  Initially I didn't use a definition in unistd.h because I 
> didn't want user space to "see" it and assume this was part of the 
> kernel ABI since this is actually just an implementation detail that 
> depends on a particular kernel configuration.

The ifdef __KERNEL__ is sufficient for that - kernel headers are now
stripped of code within these defines during 'make installheaders'.

[PATCH] Use definition for cmpxchg swi

[Nicolas Pitre]

On Mon, 9 Nov 2009, Russell King - ARM Linux wrote:

> On Sun, Nov 08, 2009 at 08:42:51PM -0500, Nicolas Pitre wrote:
> > On Sun, 8 Nov 2009, Russell King - ARM Linux wrote:
> > 
> > > Use a definition for the cmpxchg SWI instead of hard-coding the number.
> > 
> > Well...  Initially I didn't use a definition in unistd.h because I 
> > didn't want user space to "see" it and assume this was part of the 
> > kernel ABI since this is actually just an implementation detail that 
> > depends on a particular kernel configuration.
> 
> The ifdef __KERNEL__ is sufficient for that - kernel headers are now
> stripped of code within these defines during 'make installheaders'.

Sure. But I wouldn't be surprised to see people redefining it manually 
just because they didn't understand why there was a #ifdef __KERNEL__ in 
the kernel file.  Adding a comment makes the intent clearer.


Nicolas

[PATCH] Use definition for cmpxchg swi

[Russell King - ARM Linux]

On Mon, Nov 09, 2009 at 09:56:26AM -0500, Nicolas Pitre wrote:
> On Mon, 9 Nov 2009, Russell King - ARM Linux wrote:
> 
> > On Sun, Nov 08, 2009 at 08:42:51PM -0500, Nicolas Pitre wrote:
> > > On Sun, 8 Nov 2009, Russell King - ARM Linux wrote:
> > > 
> > > > Use a definition for the cmpxchg SWI instead of hard-coding the number.
> > > 
> > > Well...  Initially I didn't use a definition in unistd.h because I 
> > > didn't want user space to "see" it and assume this was part of the 
> > > kernel ABI since this is actually just an implementation detail that 
> > > depends on a particular kernel configuration.
> > 
> > The ifdef __KERNEL__ is sufficient for that - kernel headers are now
> > stripped of code within these defines during 'make installheaders'.
> 
> Sure. But I wouldn't be surprised to see people redefining it manually 
> just because they didn't understand why there was a #ifdef __KERNEL__ in 
> the kernel file.  Adding a comment makes the intent clearer.

One solution to that would be to make it a random number within a
certain range picked at kernel boot time. ;)

I'll add a comment then.

Note, however, that the original code was buggy - it resulted in
the '0xfff0' syscall being called instead of '0xffff0' - note the
extra 4 bits there.

[PATCH] Use definition for cmpxchg swi

[Jamie Lokier]

Russell King - ARM Linux wrote:
> I'll add a comment then.
> 
> Note, however, that the original code was buggy - it resulted in
> the '0xfff0' syscall being called instead of '0xffff0' - note the
> extra 4 bits there.

Meaning that nobody has ever actually used the original code? :-)

I notice that it loads the syscall number register for EABI
compatibility, but that instruction could be trivially omitted with
OABI-compatible kernels, couldn't it?

By the way, did you change it to LDR to make it faster?  (I.e. does
that make it faster?)

-- Jamie

[PATCH] Use definition for cmpxchg swi

[Russell King - ARM Linux]

On Mon, Nov 09, 2009 at 07:08:52PM +0000, Jamie Lokier wrote:
> Russell King - ARM Linux wrote:
> > I'll add a comment then.
> > 
> > Note, however, that the original code was buggy - it resulted in
> > the '0xfff0' syscall being called instead of '0xffff0' - note the
> > extra 4 bits there.
> 
> Meaning that nobody has ever actually used the original code? :-)

Correct, because SMP on ARM architectures prior to V6 is rather rare.
My only implementation of that is a pile of FPGAs implementing a 4-way
ARM926 SMP system.

> I notice that it loads the syscall number register for EABI
> compatibility, but that instruction could be trivially omitted with
> OABI-compatible kernels, couldn't it?

It could.

> By the way, did you change it to LDR to make it faster?  (I.e. does
> that make it faster?)

Well, we could use one mov and two orr instructions to load the minimum
20 bits of syscall number (which equates to a minimum of 3 cycles) or
one ldr, which can be 1 cycle if the cache is warm.  Since the result
is not used in the following two instructions, there is no impact from
result delays.

[PATCH] Use definition for cmpxchg swi

[Nicolas Pitre]

On Mon, 9 Nov 2009, Jamie Lokier wrote:

> Russell King - ARM Linux wrote:
> > I'll add a comment then.
> > 
> > Note, however, that the original code was buggy - it resulted in
> > the '0xfff0' syscall being called instead of '0xffff0' - note the
> > extra 4 bits there.
> 
> Meaning that nobody has ever actually used the original code? :-)

Hard to tell.  This is certainly the last of the last resorts.

> I notice that it loads the syscall number register for EABI
> compatibility, but that instruction could be trivially omitted with
> OABI-compatible kernels, couldn't it?

Maybe, but I don't think we should care that much.  This is only to 
support maybe one or two dev boards in existence on the planet that 
might even not be used anymore, and being the solution of last resort 
means it is already much much slower than the alternatives.  So this is 
not worth the bother.

> By the way, did you change it to LDR to make it faster?  (I.e. does
> that make it faster?)

It's most probably only a case of making the define usage simpler.


Nicolas

[PATCH] Use definition for cmpxchg swi

[Russell King - ARM Linux]

So, can I have an ack for the patch with your suggested additional
comment in?

[PATCH] Use definition for cmpxchg swi

[Nicolas Pitre]

On Mon, 9 Nov 2009, Russell King - ARM Linux wrote:

> So, can I have an ack for the patch with your suggested additional
> comment in?

Sure.

Acked-by: Nicolas Pitre <nico@fluxnic.net>


>