Re: [PATCH] arm64: Preserve the user r/w register tpidr_el0 on context switch and fork in compat mode

Will Deacon
2015-05-06T17:05:03Z

On Sun, May 03, 2015 at 05:24:18PM +0100, André Hentschel wrote:
> From: André Hentschel <nerv@dawncrow.de>
>
> Since commit a4780adeefd042482f624f5e0d577bf9cdcbb760 the user writeable TLS
> register on ARM is preserved per thread.
>
> This patch does it analogous to the ARM patch, but for compat mode on ARM64.
>
> Signed-off-by: André Hentschel <nerv@dawncrow.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Jonathan Austin <jonathan.austin@arm.com>
>
> ---
> This patch is against Linux 4.1-rc1 (b787f68c36d49bb1d9236f403813641efa74a031)
>
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 20e9591..cd7b8c9 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -78,7 +78,7 @@ struct cpu_context {
>
> struct thread_struct {
> struct cpu_context cpu_context; /* cpu context */
> - unsigned long tp_value;
> + unsigned long tp_value[2]; /* TLS registers */

I think I'd rather have a separate field for the user-writable TLS,
predicated on #ifdef COMPAT. It also removes confusion between the two tls
registers, which are also present on arm64.

> struct fpsimd_state fpsimd_state;
> unsigned long fault_address; /* fault info */
> unsigned long fault_code; /* ESR_EL1 value */
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index c6b1f3b..fc7cc6bc 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -218,7 +218,8 @@ static void tls_thread_flush(void)
> asm ("msr tpidr_el0, xzr");
>
> if (is_compat_task()) {
> - current->thread.tp_value = 0;
> + current->thread.tp_value[0] = 0;
> + current->thread.tp_value[1] = 0;
>
> /*
> * We need to ensure ordering between the shadow state and the
> @@ -254,7 +255,7 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> unsigned long stk_sz, struct task_struct *p)
> {
> struct pt_regs *childregs = task_pt_regs(p);
> - unsigned long tls = p->thread.tp_value;
> + unsigned long tls = p->thread.tp_value[0];
>
> memset(&p->thread.cpu_context, 0, sizeof(struct cpu_context));
>
> @@ -283,6 +284,11 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> */
> if (clone_flags & CLONE_SETTLS)
> tls = childregs->regs[3];
> + if (is_compat_thread(task_thread_info(p))) {
> + unsigned long tpuser;
> + asm("mrs %0, tpidr_el0" : "=r" (tpuser));
> + p->thread.tp_value[1] = tpuser;
> + }

Can't this hunk be in the is_compat_thread(...) block slightly earlier
on in the function? In fact, we're reading tpidr_el0 for both the compat and
native cases, so really this all be unconditional.

> } else {
> memset(childregs, 0, sizeof(struct pt_regs));
> childregs->pstate = PSR_MODE_EL1h;
> @@ -291,7 +297,7 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> }
> p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
> p->thread.cpu_context.sp = (unsigned long)childregs;
> - p->thread.tp_value = tls;
> + p->thread.tp_value[0] = tls;
>
> ptrace_hw_copy_thread(p);
>
> @@ -302,16 +308,17 @@ static void tls_thread_switch(struct task_struct *next)
> {
> unsigned long tpidr, tpidrro;
>
> - if (!is_compat_task()) {
> - asm("mrs %0, tpidr_el0" : "=r" (tpidr));
> - current->thread.tp_value = tpidr;
> - }
> + asm("mrs %0, tpidr_el0" : "=r" (tpidr));
> + if (is_compat_task())
> + current->thread.tp_value[1] = tpidr;
> + else
> + current->thread.tp_value[0] = tpidr;

Maybe we could have a macro thread_user_tls(thread) so that we could just
do thread_user_tls(current->thread) = tpidr;

Will