arm: Preserve TPIDRURW on context switch

From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <[email protected]>

There are more and more applications coming to WinRT, Wine could support them, but mostly they expect to have the thread environment block (TEB) in TPIDRURW. This register must be preserved per thread instead of being cleared.

Signed-off-by Andr? Hentschel <[email protected]>

Quoted 6 lines · Andr? Hentschel

From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <[email protected]>

There are more and more applications coming to WinRT, Wine could support them, but mostly they expect to have the thread environment block (TEB) in TPIDRURW. This register must be preserved per thread instead of being cleared.

Quoted 1 line

Signed-off-by Andr? Hentschel <[email protected]>

This actually makes things less efficient all round, because you now use the value immediately after loading, which means it will cause pipeline stalls, certainly on older CPUs.

Could you please rework the patch to try avoiding soo many modifications to the way things have been done here?

Quoted 13 lines · Andr? Hentschel

Quoted 7 lines

From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <[email protected]>

There are more and more applications coming to WinRT, Wine could support them, but mostly they expect to have the thread environment block (TEB) in TPIDRURW. This register must be preserved per thread instead of being cleared.

Signed-off-by Andr? Hentschel <[email protected]>

This actually makes things less efficient all round, because you now use the value immediately after loading, which means it will cause pipeline stalls, certainly on older CPUs.

Quoted 2 lines

Could you please rework the patch to try avoiding soo many modifications to the way things have been done here?

copy_thread also needs updating so that the *register* value for the parent is copied to the child, since the parent may have written the register after the last context-switch, meaning that tp_value is out-of-date.

Am 22.04.2013 17:18, schrieb Will Deacon:

Quoted 14 lines · Russell King - ARM Linux

Quoted 13 lines · André Hentschel

Quoted 7 lines

From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <[email protected]>

There are more and more applications coming to WinRT, Wine could support them, but mostly they expect to have the thread environment block (TEB) in TPIDRURW. This register must be preserved per thread instead of being cleared.

Signed-off-by André Hentschel <[email protected]>

This actually makes things less efficient all round, because you now use the value immediately after loading, which means it will cause pipeline stalls, certainly on older CPUs.

Quoted 6 lines

Quoted 2 lines

Could you please rework the patch to try avoiding soo many modifications to the way things have been done here?

copy_thread also needs updating so that the *register* value for the parent is copied to the child, since the parent may have written the register after the last context-switch, meaning that tp_value is out-of-date.

Thank you both for reviewing.

I guess you mostly mean "ldr r6, [r2, #TI_CPU_DOMAIN]".
I just thought about old CPUs and remembered again that we at Wine need that patch only on v7 (and later). So is it ok to introduce a set_tls_v7 in tls.h and make use of CONFIG_CPU_V7 compile-time check in
the changed files and in the copy_thread function?

Do i need any further flag checks in copy_thread or can i use the compile-time check to add unconditional code?

Quoted 24 lines

Am 22.04.2013 17:18, schrieb Will Deacon:

Quoted 14 lines · Russell King - ARM Linux

Quoted 13 lines · Andr? Hentschel

Quoted 7 lines

From: =?UTF-8?q?Andr=C3=A9=20Hentschel?= <[email protected]>

There are more and more applications coming to WinRT, Wine could support them, but mostly they expect to have the thread environment block (TEB) in TPIDRURW. This register must be preserved per thread instead of being cleared.

Signed-off-by Andr? Hentschel <[email protected]>

This actually makes things less efficient all round, because you now use the value immediately after loading, which means it will cause pipeline stalls, certainly on older CPUs.

Quoted 6 lines

Quoted 2 lines

Could you please rework the patch to try avoiding soo many modifications to the way things have been done here?

copy_thread also needs updating so that the *register* value for the parent is copied to the child, since the parent may have written the register after the last context-switch, meaning that tp_value is out-of-date.

Thank you both for reviewing.

Quoted 5 lines

I guess you mostly mean "ldr r6, [r2, #TI_CPU_DOMAIN]".
I just thought about old CPUs and remembered again that we at Wine need that patch only on v7 (and later). So is it ok to introduce a set_tls_v7 in tls.h and make use of CONFIG_CPU_V7 compile-time check in
the changed files and in the copy_thread function?

No, we should support this feature on any CPU with the TPIDRURW register, otherwise it's going to get really confusing for userspace.

Quoted 2 lines

Do i need any further flag checks in copy_thread or can i use the compile-time check to add unconditional code?

You could introduce `get' tls functions, which don't do anything for CPUs without the relevant registers.

Am 23.04.2013 11:15, schrieb Will Deacon:

Quoted 2 lines

You could introduce `get' tls functions, which don't do anything for CPUs without the relevant registers.

Before i have another round of testing and patch formatting/sending, what about the untested patch below?

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

index cddda1f..bb5b48d 100644
@@ -58,7 +58,7 @@ struct thread_info {
 	struct cpu_context_save	cpu_context;	/* cpu context */
 	__u32			syscall;	/* syscall number */
 	__u8			used_cp[16];	/* thread used copro */
-	unsigned long		tp_value;
+	unsigned long		tp_value[2];
 #ifdef CONFIG_CRUNCH
 	struct crunch_state	crunchstate;
 #endif

arch/arm/include/asm/tls.h +0 -0

index 73409e6..1c10163 100644

arch/arm/include/asm/tls.h +6 -3

+	.endm
+
+
 	.macro set_tls_none, tp, tmp1, tmp2
 	.endm
 
 	.macro set_tls_v6k, tp, tmp1, tmp2
-	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
-	mov	\tmp1, #0
-	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
+	ldrd	\tmp1, \tmp2, [\tp]
+	mcr	p15, 0, \tmp1, c13, c0, 3	@ set user r/o TLS register
+	mcr	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register
 	.endm
 
 	.macro set_tls_v6, tp, tmp1, tmp2

arch/arm/include/asm/tls.h +11 -2

+	ldreq	\tmp1, [\tp]
+	mcrne	p15, 0, \tmp1, c13, c0, 3	@ yes, set user r/o TLS register
+	mcrne	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register
+	streq	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
 	.endm
 
 	.macro set_tls_software, tp, tmp1, tmp2
-	mov	\tmp1, #0xffff0fff
-	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0
+	ldr	\tmp1, [\tp]
+	mov	\tmp2, #0xffff0fff
+	str	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
 	.endm
 #endif
 
 #ifdef CONFIG_TLS_REG_EMUL
 #define tls_emu		1
 #define has_tls_reg		1
+#define get_tls2		get_tls2_none
 #define set_tls		set_tls_none
 #elif defined(CONFIG_CPU_V6)
 #define tls_emu		0
 #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
+#define get_tls2		get_tls2_v6
 #define set_tls		set_tls_v6
 #elif defined(CONFIG_CPU_32v6K)
 #define tls_emu		0
 #define has_tls_reg		1
+#define get_tls2		get_tls2_v6k
 #define set_tls		set_tls_v6k
 #else
 #define tls_emu		0
 #define has_tls_reg		0
+#define get_tls2		get_tls2_none
 #define set_tls		set_tls_software
 #endif
 

arch/arm/kernel/entry-armv.S +3 -1

index 0f82098..097686b 100644
@@ -728,7 +728,7 @@ ENTRY(__switch_to)
  UNWIND(.fnstart	)
  UNWIND(.cantunwind	)
 	add	ip, r1, #TI_CPU_SAVE
-	ldr	r3, [r2, #TI_TP_VALUE]
+	add	r3, r1, #TI_TP_VALUE
  ARM(	stmia	ip!, {r4 - sl, fp, sp, lr} )	@ Store most regs on stack
  THUMB(	stmia	ip!, {r4 - sl, fp}	   )	@ Store most regs on stack
  THUMB(	str	sp, [ip], #4		   )

@@ -736,6 +736,8 @@ ENTRY(__switch_to)
 #ifdef CONFIG_CPU_USE_DOMAINS
 	ldr	r6, [r2, #TI_CPU_DOMAIN]
 #endif
+	get_tls2	r3, r4
+	add	r3, r2, #TI_TP_VALUE
 	set_tls	r3, r4, r5
 #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
 	ldr	r7, [r2, #TI_TASK]

arch/arm/kernel/process.c +0 -0

index 047d3e4..6138eb1 100644

arch/arm/kernel/process.c +0 -0

 	thread_notify(THREAD_NOTIFY_COPY, thread);
 

arch/arm/kernel/ptrace.c +0 -0

index 03deeff..2bc1514 100644

Hi Andrew,

Quoted 6 lines · André Hentschel

Am 23.04.2013 11:15, schrieb Will Deacon:

Quoted 2 lines

You could introduce `get' tls functions, which don't do anything for CPUs without the relevant registers.

Before i have another round of testing and patch formatting/sending, what about the untested patch below?

Ok. Comments inline.

Review

Quoted 3 lines

#ifdef __ASSEMBLY__

(quoted diff) +2 -0

+	.macro get_tls2_none, tp, tmp1
+	.endm

Cosmetic, but these are really horrible macro names.

arch/arm/include/asm/tls.h line 18

Quoted 6 lines

arch/arm/include/asm/tls.h +6 -0

+	.macro get_tls2_v6, tp, tmp1
+	ldr	\tmp1, =elf_hwcap
+	ldr	\tmp1, [\tmp1, #0]
+	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
+	mrcne	p15, 0, \tmp1, c13, c0, 2		@ get user r/w TLS register
+	strne	\tmp1, [\tp, #4]

You could factor out some of this hwcap checking now that it's used by both set and get.

arch/arm/include/asm/tls.h line 36

Quoted 1 line

arch/arm/include/asm/tls.h +1 -0

+	ldrdne	\tmp1, \tmp2, [\tp]

Does this work for big-endian CPUs?

arch/arm/kernel/process.c copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 400

Quoted 12 lines

arch/arm/kernel/process.c +2 -1

index 047d3e4..6138eb1 100644
@@ -395,7 +395,8 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
 	clear_ptrace_hw_breakpoint(p);
 
 	if (clone_flags & CLONE_SETTLS)
-		thread->tp_value = childregs->ARM_r3;
+		thread->tp_value[0] = childregs->ARM_r3;
+	thread->tp_value[1] = current_thread_info()->tp_value[1];
 

This still isn't correct. Imagine the following sequence of events:

- Task foo writes its TPIDRURW register from userspace and then issues a fork() system call. No context switch occurs between these two events.

- We start creating the child task, bar, and end up in copy_thread with the `thread' pointing at foo's struct thread_info, which contains the *old* TPIDRURW value.

- We copy out the stale value into bar, which is then scheduled with an old TPIDRURW value.

The solution is to reload the value sitting in the register in copy_thread, rather than relying on the thread_info being up-to-date. That's why I previously suggested not using asm macros for the getters.

Quoted 7 lines

arch/arm/kernel/ptrace.c +0 -0

index 03deeff..2bc1514 100644
@@ -849,7 +849,7 @@ long arch_ptrace(struct task_struct *child, long request,
 #endif
 

arch/arm/kernel/ptrace.c long arch_ptrace(struct task_struct *child, long request, · line 854

Quoted 5 lines

arch/arm/kernel/ptrace.c +1 -1

 		case PTRACE_GET_THREAD_AREA:
-			ret = put_user(task_thread_info(child)->tp_value,
+			ret = put_user(task_thread_info(child)->tp_value[0],
 				       datap);
 			break;

I'm guessing debuggers don't care about the new TLS value, or do we need a new ptrace request?

Hi Andrew,

Quoted 6 lines · Andr? Hentschel

Am 23.04.2013 11:15, schrieb Will Deacon:

Quoted 2 lines

You could introduce `get' tls functions, which don't do anything for CPUs without the relevant registers.

Before i have another round of testing and patch formatting/sending, what about the untested patch below?

Ok. Comments inline.

Am 24.04.2013 11:42, schrieb Will Deacon:

Quoted 11 lines

Hi Andrew,

Quoted 6 lines · André Hentschel

Am 23.04.2013 11:15, schrieb Will Deacon:

Quoted 2 lines

You could introduce `get' tls functions, which don't do anything for CPUs without the relevant registers.

Before i have another round of testing and patch formatting/sending, what about the untested patch below?

Ok. Comments inline.

I answered to that seperatly.
Here is another try based on your comments:

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

index cddda1f..bb5b48d 100644
@@ -58,7 +58,7 @@ struct thread_info {
  	struct cpu_context_save	cpu_context;	/* cpu context */
  	__u32			syscall;	/* syscall number */
  	__u8			used_cp[16];	/* thread used copro */
-	unsigned long		tp_value;
+	unsigned long		tp_value[2];
  #ifdef CONFIG_CRUNCH
  	struct crunch_state	crunchstate;
  #endif

arch/arm/include/asm/tls.h +7 -0

index 73409e6..02f8674 100644
@@ -2,48 +2,87 @@
  #define __ASMARM_TLS_H
 
  #ifdef __ASSEMBLY__
+	.macro check_hwcap_tls, tmp1
+	ldr	\tmp1, =elf_hwcap
+	ldr	\tmp1, [\tmp1, #0]
+	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
+	.endm
+
+

arch/arm/include/asm/tls.h +14 -12

+	mrcne	p15, 0, \tmp1, c13, c0, 2		@ get user r/w TLS register
+	strne	\tmp1, [\tp, #4]
+	.endm
+
+
  	.macro set_tls_none, tp, tmp1, tmp2
  	.endm
 
  	.macro set_tls_v6k, tp, tmp1, tmp2
-	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
-	mov	\tmp1, #0
-	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
+	ldrd	\tmp1, \tmp2, [\tp]
+	mcr	p15, 0, \tmp1, c13, c0, 3	@ set user r/o TLS register
+	mcr	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register
  	.endm
 
  	.macro set_tls_v6, tp, tmp1, tmp2
-	ldr	\tmp1, =elf_hwcap
-	ldr	\tmp1, [\tmp1, #0]
  	mov	\tmp2, #0xffff0fff
-	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
-	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
-	movne	\tmp1, #0
-	mcrne	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
-	streq	\tp, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
+	check_hwcap_tls \tmp1
+	ldrdne	\tmp1, \tmp2, [\tp]
+	ldreq	\tmp1, [\tp]
+	mcrne	p15, 0, \tmp1, c13, c0, 3	@ yes, set user r/o TLS register
+	mcrne	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register
+	streq	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
  	.endm
 
  	.macro set_tls_software, tp, tmp1, tmp2
-	mov	\tmp1, #0xffff0fff
-	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0

arch/arm/include/asm/tls.h +2 -0

  #define set_tls		set_tls_none
  #elif defined(CONFIG_CPU_V6)
  #define tls_emu		0
  #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
+#define get_tls		get_tls_v6
  #define set_tls		set_tls_v6
  #elif defined(CONFIG_CPU_32v6K)
  #define tls_emu		0
  #define has_tls_reg		1
+#define get_tls		get_tls_v6k

arch/arm/include/asm/tls.h +11 -0

+{
+	unsigned long t;
+#ifdef CONFIG_TLS_REG_EMUL
+	return;
+#endif
+	if (!has_tls_reg) return;
+	__asm__("mcr p15, 0, %0, c13, c0, 2" : : "r" (t));
+	*tpidrurw = t;
+}
+#endif
+
  #endif	/* __ASMARM_TLS_H */

arch/arm/kernel/entry-armv.S +3 -1

index 0f82098..2c892b2 100644
@@ -728,7 +728,7 @@ ENTRY(__switch_to)
   UNWIND(.fnstart	)
   UNWIND(.cantunwind	)
  	add	ip, r1, #TI_CPU_SAVE
-	ldr	r3, [r2, #TI_TP_VALUE]
+	add	r3, r1, #TI_TP_VALUE
   ARM(	stmia	ip!, {r4 - sl, fp, sp, lr} )	@ Store most regs on stack
   THUMB(	stmia	ip!, {r4 - sl, fp}	   )	@ Store most regs on stack
   THUMB(	str	sp, [ip], #4		   )

@@ -736,6 +736,8 @@ ENTRY(__switch_to)
  #ifdef CONFIG_CPU_USE_DOMAINS
  	ldr	r6, [r2, #TI_CPU_DOMAIN]
  #endif
+	get_tls	r3, r4
+	add	r3, r2, #TI_TP_VALUE
  	set_tls	r3, r4, r5
  #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
  	ldr	r7, [r2, #TI_TASK]

arch/arm/kernel/process.c +1 -0

index 047d3e4..a13bbc8 100644
@@ -36,6 +36,7 @@
  #include <asm/cacheflush.h>
  #include <asm/idmap.h>
  #include <asm/processor.h>
+#include <asm/tls.h>
  #include <asm/thread_notify.h>
  #include <asm/stacktrace.h>
  #include <asm/mach/time.h>

(quoted diff) +6 -0

+	.endm
+
+	.macro save_tlsuser_v6k, tp, tmp1, tmp2
+	@ TPIDRURW can be updated from userspace, so we have to re-read it
+	mrc	p15, 0, \tmp2, c13, c0, 2	@ load user r/w TLS register
+	str	\tmp2, [\tp, #4]
 	.endm
 
 	.macro set_tls_v6, tp, tmp1, tmp2

@@ -16,15 +25,26 @@
 	ldr	\tmp1, [\tmp1, #0]
 	mov	\tmp2, #0xffff0fff

(quoted diff) +9 -2

+	mrcne	p15, 0, \tmp2, c13, c0, 2	@ read user r/w TLS register
+	strne	\tmp2, [\tp, #4]		@ save in to thread_info
 	.endm
 
 	.macro set_tls_software, tp, tmp1, tmp2
-	mov	\tmp1, #0xffff0fff
-	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0
+	ldr	\tmp1, [\tp]
+	mov	\tmp2, #0xffff0fff
+	str	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
 	.endm
 #endif
 

@@ -32,18 +52,31 @@
 #define tls_emu		1
 #define has_tls_reg		1
 #define set_tls		set_tls_none
+#define save_tlsuser	save_tlsuser_none
+#define get_tlsuser	get_tlsuser_none
 #elif defined(CONFIG_CPU_V6)
 #define tls_emu		0
 #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
 #define set_tls		set_tls_v6
+#define save_tlsuser	save_tlsuser_v6
+#define get_tlsuser	get_tlsuser_v6
 #elif defined(CONFIG_CPU_32v6K)
 #define tls_emu		0
 #define has_tls_reg		1

Review

Quoted 7 lines · Jonathan Austin

.macro set_tls_v6k, tp, tmp1, tmp2

(quoted diff) +3 -3

-	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
-	mov	\tmp1, #0
-	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
+	ldrd	\tmp1, \tmp2, [\tp]
+	mcr	p15, 0, \tmp1, c13, c0, 3	@ set user r/o TLS register
+	mcr	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register

So we're still back at stalling the pipeline with result delays on older CPUs?

Quoted 13 lines

(quoted diff) +6 -0

+	.endm
+
+	.macro save_tlsuser_v6k, tp, tmp1, tmp2
+	@ TPIDRURW can be updated from userspace, so we have to re-read it
+	mrc	p15, 0, \tmp2, c13, c0, 2	@ load user r/w TLS register
+	str	\tmp2, [\tp, #4]
  	.endm
 
  	.macro set_tls_v6, tp, tmp1, tmp2

@@ -16,15 +25,26 @@
  	ldr	\tmp1, [\tmp1, #0]
  	mov	\tmp2, #0xffff0fff

Review line 32

Quoted 10 lines

(quoted diff) +5 -4

  	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
-	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
-	movne	\tmp1, #0
-	mcrne	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
-	streq	\tp, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
+	ldrned	\tmp1, \tmp2, [\tp]
+	ldreq	\tmp1, [\tp]
+	mcrne	p15, 0, \tmp1, c13, c0, 3	@ yes, set user r/o TLS register
+	mcrne	p15, 0, \tmp2, c13, c0, 2	@ set user r/w TLS register
+	streq	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0

This at least is better.

Review line 39

Quoted 7 lines

(quoted diff) +7 -0

+	.endm
+
+	.macro save_tlsuser_v6, tp, tmp1, tmp2
+	@ TPIDRURW can be updated from userspace, so we have to re-read it
+	ldr	\tmp1, =elf_hwcap
+	ldr	\tmp1, [\tmp1, #0]
+	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?

But this isn't - this involves two delays.

Quoted 35 lines

(quoted diff) +11 -2

+	mrcne	p15, 0, \tmp2, c13, c0, 2	@ read user r/w TLS register
+	strne	\tmp2, [\tp, #4]		@ save in to thread_info
  	.endm
 
  	.macro set_tls_software, tp, tmp1, tmp2
-	mov	\tmp1, #0xffff0fff
-	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0
+	ldr	\tmp1, [\tp]
+	mov	\tmp2, #0xffff0fff
+	str	\tmp1, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
  	.endm
  #endif
 

@@ -32,18 +52,31 @@
  #define tls_emu		1
  #define has_tls_reg		1
  #define set_tls		set_tls_none
+#define save_tlsuser	save_tlsuser_none
+#define get_tlsuser	get_tlsuser_none
  #elif defined(CONFIG_CPU_V6)
  #define tls_emu		0
  #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
  #define set_tls		set_tls_v6
+#define save_tlsuser	save_tlsuser_v6
+#define get_tlsuser	get_tlsuser_v6
  #elif defined(CONFIG_CPU_32v6K)
  #define tls_emu		0
  #define has_tls_reg		1
  #define set_tls		set_tls_v6k
+#define save_tlsuser	save_tlsuser_v6k
+#define get_tlsuser	get_tlsuser_v6k
  #else
  #define tls_emu		0
  #define has_tls_reg		0

Review line 75

Quoted 4 lines

(quoted diff) +2 -0

  #define set_tls		set_tls_software
+#define save_tlsuser	save_tlsuser_none
+#define get_tlsuser	get_tlsuser_none
  #endif

This separation of setting and saving the TLS value is actually quite silly. They're called from the same place, so lets just call it "switch_tls" instead.

Here's just the assembly bits doing that - this is totally untested of course:

  arch/arm/include/asm/tls.h   |   28 +++++++++++++++-------------
  arch/arm/kernel/entry-armv.S |    4 ++--
  2 files changed, 17 insertions(+), 15 deletions(-)

arch/arm/include/asm/tls.h +0 -0

index 73409e6..9c377f1 100644

arch/arm/include/asm/tls.h +13 -11

+	mrc	p15, 0, \tmp2, c13, c0, 2	@ get the user r/w register
  	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
-	mov	\tmp1, #0
-	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
+	mcr	p15, 0, \trw, c13, c0, 2	@ and the user r/w register
+	str	\tmp2, [\base, #TI_TP_VALUE + 4]@ save it
  	.endm
 
-	.macro set_tls_v6, tp, tmp1, tmp2
+	.macro switch_tls_v6, base, tp, trw, tmp1, tmp2
  	ldr	\tmp1, =elf_hwcap
  	ldr	\tmp1, [\tmp1, #0]
  	mov	\tmp2, #0xffff0fff
  	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
-	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
-	movne	\tmp1, #0
-	mcrne	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
  	streq	\tp, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
+	mrcne	p15, 0, \tmp2, c13, c0, 2	@ get the user r/w register
+	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
+	mcrne	p15, 0, \trw, c13, c0, 2	@ set user r/w register
+	strne	\tmp2, [\base, #TI_TP_VALUE + 4]@ save it
  	.endm
 
-	.macro set_tls_software, tp, tmp1, tmp2
+	.macro switch_tls_software, base, tp, trw, tmp1, tmp2
  	mov	\tmp1, #0xffff0fff
  	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0
  	.endm

@@ -31,19 +33,19 @@
  #ifdef CONFIG_TLS_REG_EMUL
  #define tls_emu		1
  #define has_tls_reg		1
-#define set_tls		set_tls_none
+#define switch_tls	switch_tls_none
  #elif defined(CONFIG_CPU_V6)
  #define tls_emu		0
  #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
-#define set_tls		set_tls_v6
+#define switch_tls	switch_tls_v6
  #elif defined(CONFIG_CPU_32v6K)
  #define tls_emu		0
  #define has_tls_reg		1
-#define set_tls		set_tls_v6k
+#define switch_tls	switch_tls_v6k
  #else
  #define tls_emu		0
  #define has_tls_reg		0
-#define set_tls		set_tls_software
+#define switch_tls	switch_tls_software
  #endif
 
  #endif	/* __ASMARM_TLS_H */

arch/arm/kernel/entry-armv.S +0 -1

index 0f82098..81a08b1 100644
@@ -728,15 +728,15 @@ ENTRY(__switch_to)
   UNWIND(.fnstart	)
   UNWIND(.cantunwind	)
  	add	ip, r1, #TI_CPU_SAVE
-	ldr	r3, [r2, #TI_TP_VALUE]

Hi Russell,

Thanks for the comments - you're right about the 'switch_tls' being more appropriate - needed to take a step back to see that.

I've got a few questions, added inline.

Andr?, Assuming I've understood things okay, there's a patch that uses Russell's asm stuff (with minor modifications, see the questions) and includes the C-world changes too. Perhaps you could see that it solves your problem?

Quoted 4 lines

(quoted diff) +1 -1

-	.macro set_tls_none, tp, tmp1, tmp2
+	.macro switch_tls_none, base, tp, trw, tmp1, tmp2
  	.endm

Review

Quoted 2 lines

(quoted diff) +1 -1

-	.macro set_tls_v6k, tp, tmp1, tmp2
+	.macro switch_tls_v6k, base, tp, trw, tmp1, tmp2

How do you feel about calling tp and trw something different? tpidro and tpidrw, or tp and tpuser?

The naming threw me off slightly first time I read this new signature (tp=thread_pointer/tls_pointer/etc).

André, Assuming I've understood things okay, there's a patch that uses Russell's asm stuff (with minor modifications, see the questions) and includes the C-world changes too. Perhaps you could see that it solves your problem?

Am 03.05.2013 17:24, schrieb Jonathan Austin:

Quoted 4 lines

Quoted 4 lines

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

-	.macro set_tls_none, tp, tmp1, tmp2
+	.macro switch_tls_none, base, tp, trw, tmp1, tmp2
   	.endm

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

-	.macro set_tls_v6k, tp, tmp1, tmp2
+	.macro switch_tls_v6k, base, tp, trw, tmp1, tmp2

How do you feel about calling tp and trw something different? tpidro and tpidrw, or tp and tpuser?

The naming threw me off slightly first time I read this new signature (tp=thread_pointer/tls_pointer/etc).

FWIW i think tp&tpuser is more consistent.

Quoted 4 lines

André, Assuming I've understood things okay, there's a patch that uses Russell's asm stuff (with minor modifications, see the questions) and includes the C-world changes too. Perhaps you could see that it solves your problem?

It works, but for various reasons i would like to suggest the patch below.
Reasons include: My thoughts about tp&tpuser naming and the helper function for copy_thread, further i'd really like to get a bit credit for spending weeks on getting my second kernel patch in :) If that patch is fine for you and no one object, i'd be happy to test it, adapt the commit message and include:

Reported-by André Hentschel <[email protected]>

Signed-off-by André Hentschel <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Signed-off-by Russell King <[email protected]>

Signed-off-by Jonathan Austin <[email protected]>

Not totally sure about the Signed-off-bys. Can i add a Signed-off-by for Russell King? Is it the right mail address for him/you?

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

index cddda1f..d90be6d 100644
@@ -58,7 +58,7 @@ struct thread_info {
  	struct cpu_context_save	cpu_context;	/* cpu context */
  	__u32			syscall;	/* syscall number */
  	__u8			used_cp[16];	/* thread used copro */
-	unsigned long		tp_value;
+	unsigned long		tp_value[2];	/* TLS registers */
  #ifdef CONFIG_CRUNCH
  	struct crunch_state	crunchstate;
  #endif

arch/arm/include/asm/tls.h +28 -13

index 73409e6..d7d542b 100644
@@ -2,27 +2,30 @@
  #define __ASMARM_TLS_H
 
  #ifdef __ASSEMBLY__
-	.macro set_tls_none, tp, tmp1, tmp2
+#include <asm/asm-offsets.h>
+	.macro switch_tls_none, base, tp, tpuser, tmp1, tmp2
  	.endm
 
-	.macro set_tls_v6k, tp, tmp1, tmp2
+	.macro switch_tls_v6k, base, tp, tpuser, tmp1, tmp2
+	mrc	p15, 0, \tmp2, c13, c0, 2	@ get the user r/w register
  	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
-	mov	\tmp1, #0
-	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
+	mcr	p15, 0, \tpuser, c13, c0, 2	@ and the user r/w register
+	strne	\tmp2, [\base, #TI_TP_VALUE + 4] @ save it
  	.endm
 
-	.macro set_tls_v6, tp, tmp1, tmp2
+	.macro switch_tls_v6, base, tp, tpuser, tmp1, tmp2
  	ldr	\tmp1, =elf_hwcap
  	ldr	\tmp1, [\tmp1, #0]
  	mov	\tmp2, #0xffff0fff
  	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
-	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
-	movne	\tmp1, #0
-	mcrne	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
  	streq	\tp, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
+	mrcne	p15, 0, \tmp2, c13, c0, 2	@ get the user r/w register
+	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
+	mcrne	p15, 0, \tpuser, c13, c0, 2	@ set user r/w register
+	strne	\tmp2, [\base, #TI_TP_VALUE + 4] @ save it
  	.endm
 
-	.macro set_tls_software, tp, tmp1, tmp2
+	.macro switch_tls_software, base, tp, tpuser, tmp1, tmp2
  	mov	\tmp1, #0xffff0fff
  	str	\tp, [\tmp1, #-15]		@ set TLS value at 0xffff0ff0
  	.endm

@@ -31,19 +34,31 @@
  #ifdef CONFIG_TLS_REG_EMUL
  #define tls_emu		1
  #define has_tls_reg		1
-#define set_tls		set_tls_none
+#define switch_tls	switch_tls_none
  #elif defined(CONFIG_CPU_V6)
  #define tls_emu		0
  #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
-#define set_tls		set_tls_v6
+#define switch_tls	switch_tls_v6
  #elif defined(CONFIG_CPU_32v6K)
  #define tls_emu		0
  #define has_tls_reg		1
-#define set_tls		set_tls_v6k
+#define switch_tls	switch_tls_v6k
  #else
  #define tls_emu		0
  #define has_tls_reg		0
-#define set_tls		set_tls_software
+#define switch_tls	switch_tls_software
  #endif
 
+#ifndef __ASSEMBLY__
+static inline unsigned long get_tlsuser(void)
+{
+	if (has_tls_reg && !tls_emu)
+	{
+		unsigned long t;
+		__asm__("mcr p15, 0, %0, c13, c0, 2" : : "r" (t));
+		return t;
+	}
+	return 0;
+}
+#endif
  #endif	/* __ASMARM_TLS_H */

arch/arm/kernel/entry-armv.S +2 -2

index 0f82098..80f09fe 100644
@@ -728,15 +728,15 @@ ENTRY(__switch_to)
   UNWIND(.fnstart	)
   UNWIND(.cantunwind	)
  	add	ip, r1, #TI_CPU_SAVE
-	ldr	r3, [r2, #TI_TP_VALUE]
   ARM(	stmia	ip!, {r4 - sl, fp, sp, lr} )	@ Store most regs on stack
   THUMB(	stmia	ip!, {r4 - sl, fp}	   )	@ Store most regs on stack
   THUMB(	str	sp, [ip], #4		   )
   THUMB(	str	lr, [ip], #4		   )
+	ldrd	r4, r5, [r2, #TI_TP_VALUE]
  #ifdef CONFIG_CPU_USE_DOMAINS
  	ldr	r6, [r2, #TI_CPU_DOMAIN]
  #endif
-	set_tls	r3, r4, r5
+	switch_tls r1, r4, r5, r3, r7
  #if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
  	ldr	r7, [r2, #TI_TASK]
  	ldr	r8, =__stack_chk_guard

arch/arm/kernel/process.c +3 -1

index 047d3e4..24dbc72 100644
@@ -39,6 +39,7 @@
  #include <asm/thread_notify.h>
  #include <asm/stacktrace.h>
  #include <asm/mach/time.h>
+#include <asm/tls.h>
 
  #ifdef CONFIG_CC_STACKPROTECTOR
  #include <linux/stackprotector.h>

@@ -395,7 +396,8 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
  	clear_ptrace_hw_breakpoint(p);
 
  	if (clone_flags & CLONE_SETTLS)
-		thread->tp_value = childregs->ARM_r3;
+		thread->tp_value[0] = childregs->ARM_r3;
+	thread->tp_value[1] = get_tlsuser();
 
  	thread_notify(THREAD_NOTIFY_COPY, thread);
 

arch/arm/kernel/ptrace.c +1 -1

index 03deeff..2bc1514 100644
@@ -849,7 +849,7 @@ long arch_ptrace(struct task_struct *child, long request,
  #endif
 
  		case PTRACE_GET_THREAD_AREA:
-			ret = put_user(task_thread_info(child)->tp_value,
+			ret = put_user(task_thread_info(child)->tp_value[0],
  				       datap);
  			break;
 

arch/arm/kernel/traps.c +2 -2

index 1c08911..f9d6259 100644
@@ -588,7 +588,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
  		return regs->ARM_r0;
 
  	case NR(set_tls):
-		thread->tp_value = regs->ARM_r0;
+		thread->tp_value[0] = regs->ARM_r0;
  		if (tls_emu)
  			return 0;
  		if (has_tls_reg) {

@@ -706,7 +706,7 @@ static int get_tp_trap(struct pt_regs *regs, unsigned int instr)
  	int reg = (instr >> 12) & 15;
  	if (reg == 15)
  		return 1;
-	regs->uregs[reg] = current_thread_info()->tp_value;
+	regs->uregs[reg] = current_thread_info()->tp_value[0];
  	regs->ARM_pc += 4;
  	return 0;
  }