[PATCH v2 0/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[PATCH v2 0/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[Victor Kamensky]

This is second version of patch that fixes rt_sig* ltp failures
in case of big endian V7 kernel. It make sigreturn_codes snippets
endian neutral. In this version of the patch problem is fixed by
using separate .S file with snippets written with regular asm
mnemonic. With such change compiler/linker take care of all needed
byteswaps in case of BE8 mode.

This approach was suggested on the following thread:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/191543.html

Changes were tested on V7 in both BE and LE modes

Changes from v1:
  Use separate .S file rather than <asm/opcodes.h> instruction
  byteswaping macros

Victor Kamensky (1):
  ARM: signal: sigreturn_codes should be endian neutral to work in BE8

 arch/arm/kernel/Makefile          |  3 +-
 arch/arm/kernel/signal.c          | 29 +++---------------
 arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 26 deletions(-)
 create mode 100644 arch/arm/kernel/sigreturn_codes.S

-- 
1.8.1.4

[PATCH v2 1/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[Victor Kamensky]

In case of BE8 kernel data is in BE order whereas code stays in LE
order. Move sigreturn_codes to separate .S file and use proper
assembler mnemonics for these code snippets. In this case compiler
will take care of proper instructions byteswaps for BE8 case.
Change assumes that sufficiently Thumb-capable tools are used to
build kernel.

Problem was discovered during ltp testing of BE system: all rt_sig*
tests failed. Tested against the same tests in both BE and LE modes.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm/kernel/Makefile          |  3 +-
 arch/arm/kernel/signal.c          | 29 +++---------------
 arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 26 deletions(-)
 create mode 100644 arch/arm/kernel/sigreturn_codes.S

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 86d10dd..0722155 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -17,7 +17,8 @@ CFLAGS_REMOVE_return_address.o = -pg
 
 obj-y		:= elf.o entry-common.o irq.o opcodes.o \
 		   process.o ptrace.o return_address.o \
-		   setup.o signal.o stacktrace.o sys_arm.o time.o traps.o
+		   setup.o signal.o sigreturn_codes.o \
+		   stacktrace.o sys_arm.o time.o traps.o
 
 obj-$(CONFIG_ATAGS)		+= atags_parse.o
 obj-$(CONFIG_ATAGS_PROC)	+= atags_proc.o
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index ab33042..cdcbf04 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -21,29 +21,8 @@
 #include <asm/unistd.h>
 #include <asm/vfp.h>
 
-/*
- * For ARM syscalls, we encode the syscall number into the instruction.
- */
-#define SWI_SYS_SIGRETURN	(0xef000000|(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE))
-#define SWI_SYS_RT_SIGRETURN	(0xef000000|(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE))
-
-/*
- * With EABI, the syscall number has to be loaded into r7.
- */
-#define MOV_R7_NR_SIGRETURN	(0xe3a07000 | (__NR_sigreturn - __NR_SYSCALL_BASE))
-#define MOV_R7_NR_RT_SIGRETURN	(0xe3a07000 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
-
-/*
- * For Thumb syscalls, we pass the syscall number via r7.  We therefore
- * need two 16-bit instructions.
- */
-#define SWI_THUMB_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_sigreturn - __NR_SYSCALL_BASE))
-#define SWI_THUMB_RT_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
-
-static const unsigned long sigreturn_codes[7] = {
-	MOV_R7_NR_SIGRETURN,    SWI_SYS_SIGRETURN,    SWI_THUMB_SIGRETURN,
-	MOV_R7_NR_RT_SIGRETURN, SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN,
-};
+extern const unsigned long sigreturn_codes[];
+extern const int sigreturn_codes_size;
 
 static unsigned long signal_return_offset;
 
@@ -639,10 +618,10 @@ struct page *get_signal_page(void)
 	 * Copy signal return handlers into the vector page, and
 	 * set sigreturn to be a pointer to these.
 	 */
-	memcpy(addr + offset, sigreturn_codes, sizeof(sigreturn_codes));
+	memcpy(addr + offset, sigreturn_codes, sigreturn_codes_size);
 
 	ptr = (unsigned long)addr + offset;
-	flush_icache_range(ptr, ptr + sizeof(sigreturn_codes));
+	flush_icache_range(ptr, ptr + sigreturn_codes_size);
 
 	return page;
 }
diff --git a/arch/arm/kernel/sigreturn_codes.S b/arch/arm/kernel/sigreturn_codes.S
new file mode 100644
index 0000000..3ac1540
--- /dev/null
+++ b/arch/arm/kernel/sigreturn_codes.S
@@ -0,0 +1,64 @@
+/*
+ * sigreturn_codes.S - code sinpets for sigreturn syscalls
+ *
+ * Created by:	Victor Kamensky, 2013-08-13
+ * Copyright:	(C) 2013  Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/unistd.h>
+
+/*
+ * For ARM syscalls, we encode the syscall number into the instruction.
+ * With EABI, the syscall number has to be loaded into r7. As result
+ * ARM syscall sequence snippet will have move and svc in .arm encoding
+ *
+ * For Thumb syscalls, we pass the syscall number via r7.  We therefore
+ * need two 16-bit instructions in .thumb encoding
+ */
+
+	.text
+	.global sigreturn_codes
+	.type	sigreturn_codes, #object
+
+sigreturn_codes:
+	/* ARM sigreturn syscall code snippet */
+	.arm
+	mov	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
+	svc	#(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE)
+
+	/* Thumb sigreturn syscall code snippet */
+	.thumb
+	movs	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
+	svc	#0
+
+	/* ARM sigreturn_rt syscall code snippet */
+	.arm
+	mov	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
+	svc	#(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE)
+
+	/* Thumb sigreturn_rt syscall code snippet */
+	.thumb
+	movs	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
+	svc	#0
+
+	.arm
+	.space	4
+__sigreturn_codes_end:
+	.size	sigreturn_codes, . - sigreturn_codes
+
+	.section .rodata
+	.global	sigreturn_codes_size
+	.type	sigreturn_codes_size, #object
+	.size	sigreturn_codes_size, 4
+
+sigreturn_codes_size:
+	.long __sigreturn_codes_end - sigreturn_codes
-- 
1.8.1.4

[PATCH v2 1/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[Dave Martin]

On Wed, Aug 14, 2013 at 12:08:54AM -0700, Victor Kamensky wrote:
> In case of BE8 kernel data is in BE order whereas code stays in LE
> order. Move sigreturn_codes to separate .S file and use proper
> assembler mnemonics for these code snippets. In this case compiler
> will take care of proper instructions byteswaps for BE8 case.
> Change assumes that sufficiently Thumb-capable tools are used to
> build kernel.
> 
> Problem was discovered during ltp testing of BE system: all rt_sig*
> tests failed. Tested against the same tests in both BE and LE modes.

This looks sensible to me, but I have a few comments -- see below.

Cheers
---Dave

> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  arch/arm/kernel/Makefile          |  3 +-
>  arch/arm/kernel/signal.c          | 29 +++---------------
>  arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 70 insertions(+), 26 deletions(-)
>  create mode 100644 arch/arm/kernel/sigreturn_codes.S
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 86d10dd..0722155 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -17,7 +17,8 @@ CFLAGS_REMOVE_return_address.o = -pg
>  
>  obj-y		:= elf.o entry-common.o irq.o opcodes.o \
>  		   process.o ptrace.o return_address.o \
> -		   setup.o signal.o stacktrace.o sys_arm.o time.o traps.o
> +		   setup.o signal.o sigreturn_codes.o \
> +		   stacktrace.o sys_arm.o time.o traps.o
>  
>  obj-$(CONFIG_ATAGS)		+= atags_parse.o
>  obj-$(CONFIG_ATAGS_PROC)	+= atags_proc.o
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index ab33042..cdcbf04 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -21,29 +21,8 @@
>  #include <asm/unistd.h>
>  #include <asm/vfp.h>
>  
> -/*
> - * For ARM syscalls, we encode the syscall number into the instruction.
> - */
> -#define SWI_SYS_SIGRETURN	(0xef000000|(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> -#define SWI_SYS_RT_SIGRETURN	(0xef000000|(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> -
> -/*
> - * With EABI, the syscall number has to be loaded into r7.
> - */
> -#define MOV_R7_NR_SIGRETURN	(0xe3a07000 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> -#define MOV_R7_NR_RT_SIGRETURN	(0xe3a07000 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> -
> -/*
> - * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> - * need two 16-bit instructions.
> - */
> -#define SWI_THUMB_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> -#define SWI_THUMB_RT_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> -
> -static const unsigned long sigreturn_codes[7] = {
> -	MOV_R7_NR_SIGRETURN,    SWI_SYS_SIGRETURN,    SWI_THUMB_SIGRETURN,
> -	MOV_R7_NR_RT_SIGRETURN, SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN,
> -};
> +extern const unsigned long sigreturn_codes[];

Could be [_NR_SIGRETURN_CODES] -- see below.

Then you can revert to using sizeof(sigreturn_codes), as previously.

> +extern const int sigreturn_codes_size;

Might go away too

>  
>  static unsigned long signal_return_offset;
>  
> @@ -639,10 +618,10 @@ struct page *get_signal_page(void)
>  	 * Copy signal return handlers into the vector page, and
>  	 * set sigreturn to be a pointer to these.
>  	 */
> -	memcpy(addr + offset, sigreturn_codes, sizeof(sigreturn_codes));
> +	memcpy(addr + offset, sigreturn_codes, sigreturn_codes_size);
>  
>  	ptr = (unsigned long)addr + offset;
> -	flush_icache_range(ptr, ptr + sizeof(sigreturn_codes));
> +	flush_icache_range(ptr, ptr + sigreturn_codes_size);
>  
>  	return page;
>  }
> diff --git a/arch/arm/kernel/sigreturn_codes.S b/arch/arm/kernel/sigreturn_codes.S
> new file mode 100644
> index 0000000..3ac1540
> --- /dev/null
> +++ b/arch/arm/kernel/sigreturn_codes.S
> @@ -0,0 +1,64 @@
> +/*
> + * sigreturn_codes.S - code sinpets for sigreturn syscalls
> + *
> + * Created by:	Victor Kamensky, 2013-08-13
> + * Copyright:	(C) 2013  Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/unistd.h>
> +
> +/*
> + * For ARM syscalls, we encode the syscall number into the instruction.
> + * With EABI, the syscall number has to be loaded into r7. As result
> + * ARM syscall sequence snippet will have move and svc in .arm encoding
> + *
> + * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> + * need two 16-bit instructions in .thumb encoding
> + */
> +
> +	.text

Maybe this should be in .rodata instead.  We only copy the instructions
from here... if we ever execute them here, something has gone wrong
somewhere.

This won't affect link-time swabbing.

> +	.global sigreturn_codes
> +	.type	sigreturn_codes, #object
> +
> +sigreturn_codes:

The above 3 lines can be replaced with ENTRY()...ENDPROC().

(.type / ENDPROC() isn't really required here, though.  I would suggest
omitting them, since although we assemble this as code, we really access
it as data)

> +	/* ARM sigreturn syscall code snippet */
> +	.arm

.arm can insert padding for alignment purposes.  To be on the safe side,
I suggest moving the first .arm just before the sigreturn_codes label.
This will avoid strange surprises if extra code is inserted into this file
later.

> +	mov	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE)

The "svc" mnemonic was invented for ARMv6.

Since all platforms rely on this code, including v4/v5, we should use
the old "swi" mnemonic throughout this file instead.

> +
> +	/* Thumb sigreturn syscall code snippet */
> +	.thumb

We should add a comment warning the reader that these instructions are
not executed in-place, and are expected to have a precise, fixed layout.

> +	movs	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#0
> +
> +	/* ARM sigreturn_rt syscall code snippet */
> +	.arm
> +	mov	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE)
> +
> +	/* Thumb sigreturn_rt syscall code snippet */
> +	.thumb
> +	movs	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#0
> +
> +	.arm
> +	.space	4

Nico, do you know what the spare word at the end of this array was for?
Is it ABI, or just an accidental off-by-one?

It was introduced in commit fcca538b83f2984095f15f0f90f6c7686e3a32d4
([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn), but I can't
see clearly why it's needed.

> +__sigreturn_codes_end:
> +	.size	sigreturn_codes, . - sigreturn_codes
> +
> +	.section .rodata
> +	.global	sigreturn_codes_size
> +	.type	sigreturn_codes_size, #object
> +	.size	sigreturn_codes_size, 4
> +
> +sigreturn_codes_size:
> +	.long __sigreturn_codes_end - sigreturn_codes


The correct size is statically fixed by hard-coded assumptions in
signal.c -- we don't want to hide mis-edits by pretending to adapt to
size changes.

So instead of the above lines you could do something like this:

arch/arm/include/asm/signal.h:
#define _NR_SIGRETURN_CODES 7 /* or mabye 6? */
#define _SIGRETURN_CODES_SIZE (4 * _NR_SIGRETURN_CODES)


sigreturn_codes.S:
#include <asm/signal.h>

/* ... */
	svc	#0

	.if	. - sigreturn_codes != _SIGRETURN_CODES_SIZE
	.error	"Incorrect size for sigreturn_codes"
	.endif

Cheers
---Dave

[PATCH v2 1/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[Victor Kamensky]

[adding Nico to this thread]

Nico, could you please look at Dave's comment inline
about sigreturn_codes size. It was long time ago, but
maybe you can recall why sigreturn_codes size is 7 words,
but not 6? It was set by your
fcca538b83f2984095f15f0f90f6c7686e3a32d4
([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn)
commit

Dave, thank you, good comments! I think I can accommodate
most of them. Please see few issues inline.

On 15 August 2013 03:57, Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Wed, Aug 14, 2013 at 12:08:54AM -0700, Victor Kamensky wrote:
> > In case of BE8 kernel data is in BE order whereas code stays in LE
> > order. Move sigreturn_codes to separate .S file and use proper
> > assembler mnemonics for these code snippets. In this case compiler
> > will take care of proper instructions byteswaps for BE8 case.
> > Change assumes that sufficiently Thumb-capable tools are used to
> > build kernel.
> >
> > Problem was discovered during ltp testing of BE system: all rt_sig*
> > tests failed. Tested against the same tests in both BE and LE modes.
>
> This looks sensible to me, but I have a few comments -- see below.
>
> Cheers
> ---Dave
>
> >
> > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > ---
> >  arch/arm/kernel/Makefile          |  3 +-
> >  arch/arm/kernel/signal.c          | 29 +++---------------
> >  arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 70 insertions(+), 26 deletions(-)
> >  create mode 100644 arch/arm/kernel/sigreturn_codes.S
> >
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index 86d10dd..0722155 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -17,7 +17,8 @@ CFLAGS_REMOVE_return_address.o = -pg
> >
> >  obj-y                := elf.o entry-common.o irq.o opcodes.o \
> >                  process.o ptrace.o return_address.o \
> > -                setup.o signal.o stacktrace.o sys_arm.o time.o traps.o
> > +                setup.o signal.o sigreturn_codes.o \
> > +                stacktrace.o sys_arm.o time.o traps.o
> >
> >  obj-$(CONFIG_ATAGS)          += atags_parse.o
> >  obj-$(CONFIG_ATAGS_PROC)     += atags_proc.o
> > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > index ab33042..cdcbf04 100644
> > --- a/arch/arm/kernel/signal.c
> > +++ b/arch/arm/kernel/signal.c
> > @@ -21,29 +21,8 @@
> >  #include <asm/unistd.h>
> >  #include <asm/vfp.h>
> >
> > -/*
> > - * For ARM syscalls, we encode the syscall number into the instruction.
> > - */
> > -#define SWI_SYS_SIGRETURN    (0xef000000|(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> > -#define SWI_SYS_RT_SIGRETURN (0xef000000|(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> > -
> > -/*
> > - * With EABI, the syscall number has to be loaded into r7.
> > - */
> > -#define MOV_R7_NR_SIGRETURN  (0xe3a07000 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> > -#define MOV_R7_NR_RT_SIGRETURN       (0xe3a07000 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> > -
> > -/*
> > - * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> > - * need two 16-bit instructions.
> > - */
> > -#define SWI_THUMB_SIGRETURN  (0xdf00 << 16 | 0x2700 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> > -#define SWI_THUMB_RT_SIGRETURN       (0xdf00 << 16 | 0x2700 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> > -
> > -static const unsigned long sigreturn_codes[7] = {
> > -     MOV_R7_NR_SIGRETURN,    SWI_SYS_SIGRETURN,    SWI_THUMB_SIGRETURN,
> > -     MOV_R7_NR_RT_SIGRETURN, SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN,
> > -};
> > +extern const unsigned long sigreturn_codes[];
>
> Could be [_NR_SIGRETURN_CODES] -- see below.
>
> Then you can revert to using sizeof(sigreturn_codes), as previously.
>
> > +extern const int sigreturn_codes_size;
>
> Might go away too
>
> >
> >  static unsigned long signal_return_offset;
> >
> > @@ -639,10 +618,10 @@ struct page *get_signal_page(void)
> >        * Copy signal return handlers into the vector page, and
> >        * set sigreturn to be a pointer to these.
> >        */
> > -     memcpy(addr + offset, sigreturn_codes, sizeof(sigreturn_codes));
> > +     memcpy(addr + offset, sigreturn_codes, sigreturn_codes_size);
> >
> >       ptr = (unsigned long)addr + offset;
> > -     flush_icache_range(ptr, ptr + sizeof(sigreturn_codes));
> > +     flush_icache_range(ptr, ptr + sigreturn_codes_size);
> >
> >       return page;
> >  }
> > diff --git a/arch/arm/kernel/sigreturn_codes.S b/arch/arm/kernel/sigreturn_codes.S
> > new file mode 100644
> > index 0000000..3ac1540
> > --- /dev/null
> > +++ b/arch/arm/kernel/sigreturn_codes.S
> > @@ -0,0 +1,64 @@
> > +/*
> > + * sigreturn_codes.S - code sinpets for sigreturn syscalls
> > + *
> > + * Created by:       Victor Kamensky, 2013-08-13
> > + * Copyright:        (C) 2013  Linaro Limited
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <asm/unistd.h>
> > +
> > +/*
> > + * For ARM syscalls, we encode the syscall number into the instruction.
> > + * With EABI, the syscall number has to be loaded into r7. As result
> > + * ARM syscall sequence snippet will have move and svc in .arm encoding
> > + *
> > + * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> > + * need two 16-bit instructions in .thumb encoding
> > + */
> > +
> > +     .text
>
> Maybe this should be in .rodata instead.  We only copy the instructions
> from here... if we ever execute them here, something has gone wrong
> somewhere.
>
> This won't affect link-time swabbing.
>
> > +     .global sigreturn_codes
> > +     .type   sigreturn_codes, #object
> > +
> > +sigreturn_codes:
>
> The above 3 lines can be replaced with ENTRY()...ENDPROC().
>
> (.type / ENDPROC() isn't really required here, though.  I would suggest
> omitting them, since although we assemble this as code, we really access
> it as data)
>
> > +     /* ARM sigreturn syscall code snippet */
> > +     .arm
>
> .arm can insert padding for alignment purposes.  To be on the safe side,
> I suggest moving the first .arm just before the sigreturn_codes label.
> This will avoid strange surprises if extra code is inserted into this file
> later.
>
> > +     mov     r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> > +     svc     #(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE)
>
> The "svc" mnemonic was invented for ARMv6.
>
> Since all platforms rely on this code, including v4/v5, we should use
> the old "swi" mnemonic throughout this file instead.
>
> > +
> > +     /* Thumb sigreturn syscall code snippet */
> > +     .thumb
>
> We should add a comment warning the reader that these instructions are
> not executed in-place, and are expected to have a precise, fixed layout.
>
> > +     movs    r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> > +     svc     #0
> > +
> > +     /* ARM sigreturn_rt syscall code snippet */
> > +     .arm
> > +     mov     r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> > +     svc     #(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE)
> > +
> > +     /* Thumb sigreturn_rt syscall code snippet */
> > +     .thumb
> > +     movs    r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> > +     svc     #0
> > +
> > +     .arm
> > +     .space  4
>
> Nico, do you know what the spare word at the end of this array was for?
> Is it ABI, or just an accidental off-by-one?
>
> It was introduced in commit fcca538b83f2984095f15f0f90f6c7686e3a32d4
> ([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn), but I can't
> see clearly why it's needed.
>
> > +__sigreturn_codes_end:
> > +     .size   sigreturn_codes, . - sigreturn_codes
> > +
> > +     .section .rodata
> > +     .global sigreturn_codes_size
> > +     .type   sigreturn_codes_size, #object
> > +     .size   sigreturn_codes_size, 4
> > +
> > +sigreturn_codes_size:
> > +     .long __sigreturn_codes_end - sigreturn_codes
>
>
> The correct size is statically fixed by hard-coded assumptions in
> signal.c -- we don't want to hide mis-edits by pretending to adapt to
> size changes.
>
> So instead of the above lines you could do something like this:
>
> arch/arm/include/asm/signal.h:
> #define _NR_SIGRETURN_CODES 7 /* or mabye 6? */
> #define _SIGRETURN_CODES_SIZE (4 * _NR_SIGRETURN_CODES)

Note since arch/arm/include/asm/signal.h is part of
public API, adding above defines in the file would require
'#ifdef __KERNEL__' guards. Also since the file was not
included in .S file before '#ifndef __ASSEMBLY__'
statements will have to be added.

>
> sigreturn_codes.S:
> #include <asm/signal.h>
>
> /* ... */
>         svc     #0
>
>         .if     . - sigreturn_codes != _SIGRETURN_CODES_SIZE
>         .error  "Incorrect size for sigreturn_codes"
>         .endif

Nice trick. I see few examples of it in existing code.
Unfortunately it does not work for me - it seems to me
that the reason is an assembler bug. It looks like when movs
mnemonics is used in thumb mode with 'unified' syntax it
causes gas to complain that size of address expression
over code is not constant. Here is tiny example that
illustrates the issue:

[kamensky at kamensky-w530 linux-linaro-tracking-be]$ cat
/home/kamensky/tmp/codesize.S
.syntax unified

test:
       .thumb
       /*
        * presence of movs instruction in thumb mode causes
        * non-constant expression in which 'test' and '.'
        * participate
        */
       movs      r7, #153
       swi #0

       .if (. - test) != 4
       .error "Incorrect size"
       .endif

[kamensky at kamensky-w530 linux-linaro-tracking-be]$
arm-linux-gnueabihf-as -o codesize.o /home/kamensky/tmp/codesize.S
/home/kamensky/tmp/codesize.S: Assembler messages:
/home/kamensky/tmp/codesize.S:13: Error: non-constant expression in
".if" statement
[kamensky at kamensky-w530 linux-linaro-tracking-be]$
arm-linux-gnueabihf-as --version
GNU assembler (crosstool-NG linaro-1.13.1-4.7-2013.01-20130125 -
Linaro GCC 2013.01) 2.22

Or am I missing something here?

I think in this situation since this code has
auxiliary/defensive meaning, maybe we can drop
it and rely on comment that would request size
of snippet in assembler to match size of array in
extern statement.

Also in this case if we cannot use _SIGRETURN_CODES_SIZE
in .S file, I don't think there is a need to modify
arch/arm/include/asm/signal.h. These defines may be
placed directly into arch/arm/kernel/signal.c, or even
omitted with sigreturn_codes using size directly as
it is now.

Thanks,
Victor

> Cheers
> ---Dave

[PATCH v2 1/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

[Nicolas Pitre]

On Mon, 19 Aug 2013, Victor Kamensky wrote:

> [adding Nico to this thread]
> 
> Nico, could you please look at Dave's comment inline
> about sigreturn_codes size. It was long time ago, but
> maybe you can recall why sigreturn_codes size is 7 words,
> but not 6? It was set by your
> fcca538b83f2984095f15f0f90f6c7686e3a32d4
> ([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn)
> commit

Hmmm... OK.  If you look at the rest of the patch, the retcode variable 
is changed into an array of 2 words.

Then we compute an index into sigreturn_codes.  It starts with

	idx = thumb << 1;

meaning 0 for ARM mode or 2 for Thumb mode.

Then we have this:

	if (ka->sa.sa_flags & SA_SIGINFO)
		idx += 3;

Finally we unconditionally copy 2 words:

	if (__put_user(sigreturn_codes[idx],   rc) ||
	    __put_user(sigreturn_codes[idx+1], rc+1))
		return 1;

So if the index was 2 + 3 = 5 and you copy 2 words from there, the array 
has to be 7 in size for the read from sigreturn_codes[] not to overflow.  
Given that happens only in the Thumb case and that actually requires 
only one word, the 7th word is not important.

Why always copy 2 words?  That's a micro-optimization.  Alternatively, 
the code could have used an additional conditional to copy only one word 
if thumb or two words if !thumb but that created slightly less efficient 
code.

Oviously, this should have been commented in the commit log if not in 
the code directly.


> Dave, thank you, good comments! I think I can accommodate
> most of them. Please see few issues inline.
> 
> On 15 August 2013 03:57, Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Wed, Aug 14, 2013 at 12:08:54AM -0700, Victor Kamensky wrote:
> > > In case of BE8 kernel data is in BE order whereas code stays in LE
> > > order. Move sigreturn_codes to separate .S file and use proper
> > > assembler mnemonics for these code snippets. In this case compiler
> > > will take care of proper instructions byteswaps for BE8 case.
> > > Change assumes that sufficiently Thumb-capable tools are used to
> > > build kernel.
> > >
> > > Problem was discovered during ltp testing of BE system: all rt_sig*
> > > tests failed. Tested against the same tests in both BE and LE modes.
> >
> > This looks sensible to me, but I have a few comments -- see below.
> >
> > Cheers
> > ---Dave
> >
> > >
> > > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > > ---
> > >  arch/arm/kernel/Makefile          |  3 +-
> > >  arch/arm/kernel/signal.c          | 29 +++---------------
> > >  arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 70 insertions(+), 26 deletions(-)
> > >  create mode 100644 arch/arm/kernel/sigreturn_codes.S
> > >
> > > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > > index 86d10dd..0722155 100644
> > > --- a/arch/arm/kernel/Makefile
> > > +++ b/arch/arm/kernel/Makefile
> > > @@ -17,7 +17,8 @@ CFLAGS_REMOVE_return_address.o = -pg
> > >
> > >  obj-y                := elf.o entry-common.o irq.o opcodes.o \
> > >                  process.o ptrace.o return_address.o \
> > > -                setup.o signal.o stacktrace.o sys_arm.o time.o traps.o
> > > +                setup.o signal.o sigreturn_codes.o \
> > > +                stacktrace.o sys_arm.o time.o traps.o
> > >
> > >  obj-$(CONFIG_ATAGS)          += atags_parse.o
> > >  obj-$(CONFIG_ATAGS_PROC)     += atags_proc.o
> > > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > > index ab33042..cdcbf04 100644
> > > --- a/arch/arm/kernel/signal.c
> > > +++ b/arch/arm/kernel/signal.c
> > > @@ -21,29 +21,8 @@
> > >  #include <asm/unistd.h>
> > >  #include <asm/vfp.h>
> > >
> > > -/*
> > > - * For ARM syscalls, we encode the syscall number into the instruction.
> > > - */
> > > -#define SWI_SYS_SIGRETURN    (0xef000000|(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> > > -#define SWI_SYS_RT_SIGRETURN (0xef000000|(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> > > -
> > > -/*
> > > - * With EABI, the syscall number has to be loaded into r7.
> > > - */
> > > -#define MOV_R7_NR_SIGRETURN  (0xe3a07000 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> > > -#define MOV_R7_NR_RT_SIGRETURN       (0xe3a07000 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> > > -
> > > -/*
> > > - * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> > > - * need two 16-bit instructions.
> > > - */
> > > -#define SWI_THUMB_SIGRETURN  (0xdf00 << 16 | 0x2700 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> > > -#define SWI_THUMB_RT_SIGRETURN       (0xdf00 << 16 | 0x2700 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> > > -
> > > -static const unsigned long sigreturn_codes[7] = {
> > > -     MOV_R7_NR_SIGRETURN,    SWI_SYS_SIGRETURN,    SWI_THUMB_SIGRETURN,
> > > -     MOV_R7_NR_RT_SIGRETURN, SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN,
> > > -};
> > > +extern const unsigned long sigreturn_codes[];
> >
> > Could be [_NR_SIGRETURN_CODES] -- see below.
> >
> > Then you can revert to using sizeof(sigreturn_codes), as previously.
> >
> > > +extern const int sigreturn_codes_size;
> >
> > Might go away too
> >
> > >
> > >  static unsigned long signal_return_offset;
> > >
> > > @@ -639,10 +618,10 @@ struct page *get_signal_page(void)
> > >        * Copy signal return handlers into the vector page, and
> > >        * set sigreturn to be a pointer to these.
> > >        */
> > > -     memcpy(addr + offset, sigreturn_codes, sizeof(sigreturn_codes));
> > > +     memcpy(addr + offset, sigreturn_codes, sigreturn_codes_size);
> > >
> > >       ptr = (unsigned long)addr + offset;
> > > -     flush_icache_range(ptr, ptr + sizeof(sigreturn_codes));
> > > +     flush_icache_range(ptr, ptr + sigreturn_codes_size);
> > >
> > >       return page;
> > >  }
> > > diff --git a/arch/arm/kernel/sigreturn_codes.S b/arch/arm/kernel/sigreturn_codes.S
> > > new file mode 100644
> > > index 0000000..3ac1540
> > > --- /dev/null
> > > +++ b/arch/arm/kernel/sigreturn_codes.S
> > > @@ -0,0 +1,64 @@
> > > +/*
> > > + * sigreturn_codes.S - code sinpets for sigreturn syscalls
> > > + *
> > > + * Created by:       Victor Kamensky, 2013-08-13
> > > + * Copyright:        (C) 2013  Linaro Limited
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <asm/unistd.h>
> > > +
> > > +/*
> > > + * For ARM syscalls, we encode the syscall number into the instruction.
> > > + * With EABI, the syscall number has to be loaded into r7. As result
> > > + * ARM syscall sequence snippet will have move and svc in .arm encoding
> > > + *
> > > + * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> > > + * need two 16-bit instructions in .thumb encoding
> > > + */
> > > +
> > > +     .text
> >
> > Maybe this should be in .rodata instead.  We only copy the instructions
> > from here... if we ever execute them here, something has gone wrong
> > somewhere.
> >
> > This won't affect link-time swabbing.
> >
> > > +     .global sigreturn_codes
> > > +     .type   sigreturn_codes, #object
> > > +
> > > +sigreturn_codes:
> >
> > The above 3 lines can be replaced with ENTRY()...ENDPROC().
> >
> > (.type / ENDPROC() isn't really required here, though.  I would suggest
> > omitting them, since although we assemble this as code, we really access
> > it as data)
> >
> > > +     /* ARM sigreturn syscall code snippet */
> > > +     .arm
> >
> > .arm can insert padding for alignment purposes.  To be on the safe side,
> > I suggest moving the first .arm just before the sigreturn_codes label.
> > This will avoid strange surprises if extra code is inserted into this file
> > later.
> >
> > > +     mov     r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> > > +     svc     #(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE)
> >
> > The "svc" mnemonic was invented for ARMv6.
> >
> > Since all platforms rely on this code, including v4/v5, we should use
> > the old "swi" mnemonic throughout this file instead.
> >
> > > +
> > > +     /* Thumb sigreturn syscall code snippet */
> > > +     .thumb
> >
> > We should add a comment warning the reader that these instructions are
> > not executed in-place, and are expected to have a precise, fixed layout.
> >
> > > +     movs    r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> > > +     svc     #0
> > > +
> > > +     /* ARM sigreturn_rt syscall code snippet */
> > > +     .arm
> > > +     mov     r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> > > +     svc     #(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE)
> > > +
> > > +     /* Thumb sigreturn_rt syscall code snippet */
> > > +     .thumb
> > > +     movs    r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> > > +     svc     #0
> > > +
> > > +     .arm
> > > +     .space  4
> >
> > Nico, do you know what the spare word at the end of this array was for?
> > Is it ABI, or just an accidental off-by-one?
> >
> > It was introduced in commit fcca538b83f2984095f15f0f90f6c7686e3a32d4
> > ([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn), but I can't
> > see clearly why it's needed.
> >
> > > +__sigreturn_codes_end:
> > > +     .size   sigreturn_codes, . - sigreturn_codes
> > > +
> > > +     .section .rodata
> > > +     .global sigreturn_codes_size
> > > +     .type   sigreturn_codes_size, #object
> > > +     .size   sigreturn_codes_size, 4
> > > +
> > > +sigreturn_codes_size:
> > > +     .long __sigreturn_codes_end - sigreturn_codes
> >
> >
> > The correct size is statically fixed by hard-coded assumptions in
> > signal.c -- we don't want to hide mis-edits by pretending to adapt to
> > size changes.
> >
> > So instead of the above lines you could do something like this:
> >
> > arch/arm/include/asm/signal.h:
> > #define _NR_SIGRETURN_CODES 7 /* or mabye 6? */
> > #define _SIGRETURN_CODES_SIZE (4 * _NR_SIGRETURN_CODES)
> 
> Note since arch/arm/include/asm/signal.h is part of
> public API, adding above defines in the file would require
> '#ifdef __KERNEL__' guards. Also since the file was not
> included in .S file before '#ifndef __ASSEMBLY__'
> statements will have to be added.
> 
> >
> > sigreturn_codes.S:
> > #include <asm/signal.h>
> >
> > /* ... */
> >         svc     #0
> >
> >         .if     . - sigreturn_codes != _SIGRETURN_CODES_SIZE
> >         .error  "Incorrect size for sigreturn_codes"
> >         .endif
> 
> Nice trick. I see few examples of it in existing code.
> Unfortunately it does not work for me - it seems to me
> that the reason is an assembler bug. It looks like when movs
> mnemonics is used in thumb mode with 'unified' syntax it
> causes gas to complain that size of address expression
> over code is not constant. Here is tiny example that
> illustrates the issue:
> 
> [kamensky at kamensky-w530 linux-linaro-tracking-be]$ cat
> /home/kamensky/tmp/codesize.S
> .syntax unified
> 
> test:
>        .thumb
>        /*
>         * presence of movs instruction in thumb mode causes
>         * non-constant expression in which 'test' and '.'
>         * participate
>         */
>        movs      r7, #153
>        swi #0
> 
>        .if (. - test) != 4
>        .error "Incorrect size"
>        .endif
> 
> [kamensky at kamensky-w530 linux-linaro-tracking-be]$
> arm-linux-gnueabihf-as -o codesize.o /home/kamensky/tmp/codesize.S
> /home/kamensky/tmp/codesize.S: Assembler messages:
> /home/kamensky/tmp/codesize.S:13: Error: non-constant expression in
> ".if" statement
> [kamensky at kamensky-w530 linux-linaro-tracking-be]$
> arm-linux-gnueabihf-as --version
> GNU assembler (crosstool-NG linaro-1.13.1-4.7-2013.01-20130125 -
> Linaro GCC 2013.01) 2.22
> 
> Or am I missing something here?
> 
> I think in this situation since this code has
> auxiliary/defensive meaning, maybe we can drop
> it and rely on comment that would request size
> of snippet in assembler to match size of array in
> extern statement.
> 
> Also in this case if we cannot use _SIGRETURN_CODES_SIZE
> in .S file, I don't think there is a need to modify
> arch/arm/include/asm/signal.h. These defines may be
> placed directly into arch/arm/kernel/signal.c, or even
> omitted with sigreturn_codes using size directly as
> it is now.
> 
> Thanks,
> Victor
> 
> > Cheers
> > ---Dave
>