[PATCH] arm64/sve: Rework SVE access trap to convert state in registers

[Catalin Marinas]

On Fri, Mar 12, 2021 at 07:03:13PM +0000, Mark Brown wrote:
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index bec5f14b622a..ebb263b2d3b1 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -73,6 +73,7 @@ extern void sve_flush_live(void);
>  extern void sve_load_from_fpsimd_state(struct user_fpsimd_state const *state,
>  				       unsigned long vq_minus_1);
>  extern unsigned int sve_get_vl(void);
> +extern void sve_set_vq(unsigned long vq_minus_1);
>  
>  struct arm64_cpu_capabilities;
>  extern void sve_kernel_enable(const struct arm64_cpu_capabilities *__unused);
> diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
> index 2ca395c25448..3ecec60d3295 100644
> --- a/arch/arm64/kernel/entry-fpsimd.S
> +++ b/arch/arm64/kernel/entry-fpsimd.S
> @@ -48,6 +48,11 @@ SYM_FUNC_START(sve_get_vl)
>  	ret
>  SYM_FUNC_END(sve_get_vl)
>  
> +SYM_FUNC_START(sve_set_vq)
> +	sve_load_vq x0, x1, x2
> +	ret
> +SYM_FUNC_END(sve_set_vq)
> +
>  /*
>   * Load SVE state from FPSIMD state.
>   *
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 062b21f30f94..0f58e45bd3d1 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -926,9 +926,8 @@ void fpsimd_release_task(struct task_struct *dead_task)
>   * Trapped SVE access
>   *
>   * Storage is allocated for the full SVE state, the current FPSIMD
> - * register contents are migrated across, and TIF_SVE is set so that
> - * the SVE access trap will be disabled the next time this task
> - * reaches ret_to_user.
> + * register contents are migrated across, and the access trap is
> + * disabled.
>   *
>   * TIF_SVE should be clear on entry: otherwise, fpsimd_restore_current_state()
>   * would have disabled the SVE access trap for userspace during
> @@ -946,15 +945,24 @@ void do_sve_acc(unsigned int esr, struct pt_regs *regs)
>  
>  	get_cpu_fpsimd_context();
>  
> -	fpsimd_save();
> -
> -	/* Force ret_to_user to reload the registers: */
> -	fpsimd_flush_task_state(current);
> -
> -	fpsimd_to_sve(current);
>  	if (test_and_set_thread_flag(TIF_SVE))
>  		WARN_ON(1); /* SVE access shouldn't have trapped */
>  
> +	/*
> +	 * Convert the FPSIMD state to SVE, zeroing all the state that
> +	 * is not shared with FPSIMD. If (as is likely) the current
> +	 * state is live in the registers then do this there and
> +	 * update our metadata for the current task including
> +	 * disabling the trap, otherwise update our in-memory copy.
> +	 */
> +	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> +		sve_set_vq(sve_vq_from_vl(current->thread.sve_vl) - 1);
> +		sve_flush_live();
> +		fpsimd_bind_task_to_cpu();
> +	} else {
> +		fpsimd_to_sve(current);
> +	}
> +
>  	put_cpu_fpsimd_context();
>  }

Just to make sure I understand: in the current code, if
TIF_FOREIGN_FPSTATE is cleared and we trapped (TIF_SVE is also cleared),
the FPSIMD state is transferred to the SVE one in memory and both
TIF_SVE and TIF_FOREIGN_FPSTATE get set. The (potentially stale) SVE
state is subsequently restored via do_notify_resume(). I couldn't find
where we actually zero the in-memory SVE state or save the latest one
via do_el0_svc(). So it looks like we may restore some old SVE state
after a syscall (maybe I'm missing something but it would be nice to
follow zero or preserved approach).

With your patch in the above scenario, we no longer restore the SVE
state from memory. We just flush the live SVE registers and disable
trapping. The assumption here is that (1) when TIF_SVE is cleared, we no
longer care about any of the SVE state (memory or regs) and (2) we only
trap if TIF_SVE was cleared (i.e. we don't have a different scenario of
lazy restoring where TIF_SVE but we still trap). That's fine by me and
matches the code paths but it wasn't entirely clear in the TIF_SVE
description higher up in this file.

The other case is TIF_FOREIGN_FPSTATE being set in do_sve_acc(). Since
we never return to user with this flag set, when can we actually hit the
'else' path in your patch?

-- 
Catalin