Re: [PATCH v4 7/8] arm64/sve: Don't disable SVE on syscalls return

[Will Deacon <will@kernel.org>]

On Fri, Aug 28, 2020 at 07:11:54PM +0100, Mark Brown wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Per the syscalls ABI the state of the SVE registers is unknown after a
> syscall. In practice the kernel will disable SVE and zero all the
> registers but the first 128-bits of the vector on the next SVE
> instruction. In workloads mixing SVE and syscalls this will result in at
> least one extra entry/exit to the kernel per syscall when the SVE
> registers are accessed for the first time after the syscall.
> 
> To avoid the second entry/exit a new flag TIF_SVE_NEEDS_FLUSH is
> introduced to mark a task that needs to flush the SVE context on
> return to userspace.
> 
> On entry to a syscall the flag TIF_SVE will still be cleared, it will
> be restored on return to userspace once the SVE state has been flushed.
> This means that if a task requires to synchronize the FP state during a
> syscall (e.g context switch, signal) only the FPSIMD registers will be
> saved. When the task is rescheduled the SVE state will be loaded from
> FPSIMD state.
> 
> We could instead handle flushing the SVE state in do_el0_svc() however
> doing this reduces the potential for further optimisations such as
> initializing the SVE registers directly from the FPSIMD state when
> taking a SVE access trap and has some potential edge cases if we
> schedule before we return to userspace after do_el0_svc().
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/thread_info.h |  6 ++-
>  arch/arm64/kernel/fpsimd.c           | 61 +++++++++++++++++++++++++---
>  arch/arm64/kernel/process.c          |  1 +
>  arch/arm64/kernel/ptrace.c           | 11 +++++
>  arch/arm64/kernel/signal.c           | 16 +++++++-
>  arch/arm64/kernel/syscall.c          | 13 +++---
>  6 files changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 5e784e16ee89..dfaf872c0a07 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -67,6 +67,7 @@ void arch_release_task_struct(struct task_struct *tsk);
>  #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
>  #define TIF_UPROBE		4	/* uprobe breakpoint or singlestep */
>  #define TIF_FSCHECK		5	/* Check FS is USER_DS on return */
> +#define TIF_SVE_NEEDS_FLUSH	6	/* Flush SVE registers on return */
>  #define TIF_SYSCALL_TRACE	8	/* syscall trace active */
>  #define TIF_SYSCALL_AUDIT	9	/* syscall auditing */
>  #define TIF_SYSCALL_TRACEPOINT	10	/* syscall tracepoint for ftrace */
> @@ -97,9 +98,12 @@ void arch_release_task_struct(struct task_struct *tsk);
>  #define _TIF_32BIT		(1 << TIF_32BIT)
>  #define _TIF_SVE		(1 << TIF_SVE)
>  
> +#define _TIF_SVE_NEEDS_FLUSH	(1 << TIF_SVE_NEEDS_FLUSH)
> +
>  #define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>  				 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
> -				 _TIF_UPROBE | _TIF_FSCHECK)
> +				 _TIF_UPROBE | _TIF_FSCHECK | \
> +				 _TIF_SVE_NEEDS_FLUSH)
>  
>  #define _TIF_SYSCALL_WORK	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
>  				 _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 1c6a82083d5c..b0fc8823d731 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -213,6 +213,8 @@ static bool have_cpu_fpsimd_context(void)
>   */
>  static void __sve_free(struct task_struct *task)
>  {
> +	/* SVE context will be zeroed when allocated. */
> +	clear_tsk_thread_flag(task, TIF_SVE_NEEDS_FLUSH);
>  	kfree(task->thread.sve_state);
>  	task->thread.sve_state = NULL;
>  }
> @@ -269,6 +271,14 @@ static void sve_free(struct task_struct *task)
>   *  * FPSR and FPCR are always stored in task->thread.uw.fpsimd_state
>   *    irrespective of whether TIF_SVE is clear or set, since these are
>   *    not vector length dependent.
> + *
> + *  * When TIF_SVE_NEEDS_FLUSH is set, all the SVE registers but the first
> + *    128-bits of the Z-registers are logically zero but not stored anywhere.
> + *    Saving logically zero bits across context switches is therefore
> + *    pointless, although they must be zeroed before re-entering userspace.
> + *    This can be set at the same time as TIF_FPSIMD_FOREIGN_STATE, when it
> + *    is then the first 128 bits of the SVE registers will be restored from
> + *    the FPSIMD state.
>   */
>  
>  /*
> @@ -277,18 +287,38 @@ static void sve_free(struct task_struct *task)
>   * This function should be called only when the FPSIMD/SVE state in
>   * thread_struct is known to be up to date, when preparing to enter
>   * userspace.
> + *
> + * When TIF_SVE_NEEDS_FLUSH is set, the SVE state will be restored from the
> + * FPSIMD state.
> + *
> + * TIF_SVE_NEEDS_FLUSH and TIF_SVE set at the same time should never happen.
> + * In the unlikely case it happens, the code is able to cope with it. It will
> + * first restore the SVE registers and then flush them in
> + * fpsimd_restore_current_state.

I find this pretty confusing and, if anything, I'd have expected it to be
the other way around: TIF_SVE_NEEDS_FLUSH should only be checked if TIF_SVE
is set. Can we leave TIF_SVE set on syscall entry and just check whether
we need to flush on return?

Having said that, one overall concern I have with this patch is that there
is a lot of ad-hoc flag manipulation which feels like a disaster to
maintain. Do we really need all 8 states provided by FOREIGN_FPSTATE, SVE
and SVE_NEEDS_FLUSH?

>   */
>  static void task_fpsimd_load(void)
>  {
>  	WARN_ON(!system_supports_fpsimd());
>  	WARN_ON(!have_cpu_fpsimd_context());
>  
> -	if (system_supports_sve() && test_thread_flag(TIF_SVE))
> -		sve_load_state(sve_pffr(&current->thread),
> -			       &current->thread.uw.fpsimd_state.fpsr,
> -			       sve_vq_from_vl(current->thread.sve_vl) - 1);
> -	else
> -		fpsimd_load_state(&current->thread.uw.fpsimd_state);
> +	/* Ensure that we only evaluate system_supports_sve() once. */
> +	if (system_supports_sve()) {

I don't understand what the comment is getting at here, or how this code
ensure we only evaluate this once. What's the issue?

> +		if (test_thread_flag(TIF_SVE)) {
> +			WARN_ON_ONCE(test_thread_flag(TIF_SVE_NEEDS_FLUSH));
> +			sve_load_state(sve_pffr(&current->thread),
> +				       &current->thread.uw.fpsimd_state.fpsr,
> +				       sve_vq_from_vl(current->thread.sve_vl) - 1);
> +			return;
> +		} else if (test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) {
> +			WARN_ON_ONCE(test_thread_flag(TIF_SVE));

We already checked TIF_SVE and we know it's false (unless there was a
concurrent update, but then this would be racy anyway).

> +			sve_load_from_fpsimd_state(&current->thread.uw.fpsimd_state,
> +				   sve_vq_from_vl(current->thread.sve_vl) - 1);
> +			set_thread_flag(TIF_SVE);
> +			return;
> +		}
> +	}
> +
> +	fpsimd_load_state(&current->thread.uw.fpsimd_state);
>  }
>  
>  /*
> @@ -1159,10 +1189,29 @@ void fpsimd_restore_current_state(void)
>  	get_cpu_fpsimd_context();
>  
>  	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> +		/*
> +		 * If TIF_SVE_NEEDS_FLUSH is set this takes care of
> +		 * restoring the SVE state that is preserved over
> +		 * syscalls should we have context switched.
> +		 */
>  		task_fpsimd_load();
>  		fpsimd_bind_task_to_cpu();
>  	}
>  
> +	if (system_supports_sve() &&
> +	    test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) {

Why do we need to check system_supports_sve() here?

> +		/*
> +		 * The userspace had SVE enabled on entry to the kernel
> +		 * and requires the state to be flushed.
> +		 *
> +		 * We rely on the vector length to be set correctly beforehand
> +		 * when converting a loaded FPSIMD state to SVE state.
> +		 */
> +		sve_flush_live();
> +		sve_user_enable();
> +		set_thread_flag(TIF_SVE);
> +	}
> +
>  	put_cpu_fpsimd_context();
>  }
>  
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index b63ce4c54cfe..db951c63fc6a 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -369,6 +369,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>  	 */
>  	dst->thread.sve_state = NULL;
>  	clear_tsk_thread_flag(dst, TIF_SVE);
> +	clear_tsk_thread_flag(dst, TIF_SVE_NEEDS_FLUSH);
>  
>  	return 0;
>  }
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index d8ebfd813e28..2ab7102f5fd7 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -768,6 +768,10 @@ static int sve_get(struct task_struct *target,
>  
>  	/* Otherwise: full SVE case */
>  
> +	/* The flush should have happened when the thread was stopped */
> +	if (test_and_clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH))
> +		WARN(1, "TIF_SVE_NEEDS_FLUSH was set");

Given that this adds an atomic operation, I don't think we should be doing
this unless it's necessary and it looks like a debug check to me.

>  	BUILD_BUG_ON(SVE_PT_SVE_OFFSET != sizeof(header));
>  	start = SVE_PT_SVE_OFFSET;
>  	end = SVE_PT_SVE_FFR_OFFSET(vq) + SVE_PT_SVE_FFR_SIZE(vq);
> @@ -830,6 +834,11 @@ static int sve_set(struct task_struct *target,
>  		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
>  				SVE_PT_FPSIMD_OFFSET);
>  		clear_tsk_thread_flag(target, TIF_SVE);
> +		/*
> +		 * If ptrace requested to use FPSIMD, then don't try to
> +		 * re-enable SVE when the task is running again.
> +		 */

I think this comment needs some help. Is "ptrace" the tracer and "the task"
the tracee?

> +		clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH);
>  		goto out;
>  	}
>  
> @@ -854,6 +863,8 @@ static int sve_set(struct task_struct *target,
>  	 */
>  	fpsimd_sync_to_sve(target);
>  	set_tsk_thread_flag(target, TIF_SVE);
> +	/* Don't flush SVE registers on return as ptrace will update them. */

Same here.

Will

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