Re: [PATCH v3 5/7] arm64/fpsimd: Load FP state based on recorded data type

[Marc Zyngier <maz@kernel.org>]

On Mon, 15 Aug 2022 23:55:27 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> Now that we are recording the type of floating point register state we
> are saving when we save it we can use that information when we load to
> decide which register state is required and bring the TIF_SVE state into
> sync with the loaded register state.

Really, this sentence makes zero sense to me. Please at least add some
punctuation, because the only words that spring to mind here are "DOES
NOT COMPUTE".

> 
> The SME state detauls are already recorded directly in the saved
> SVCR and handled based on the information there.
> 
> Since we are not changing any of the save paths there should be no
> functional change from this patch, further patches will make use of this
> to optimise and clarify the code.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c | 39 ++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index aaea2dc02cbd..4096530dd4c6 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -392,11 +392,36 @@ static void task_fpsimd_load(void)
>  	WARN_ON(!system_supports_fpsimd());
>  	WARN_ON(!have_cpu_fpsimd_context());
>  
> -	/* Check if we should restore SVE first */
> -	if (IS_ENABLED(CONFIG_ARM64_SVE) && test_thread_flag(TIF_SVE)) {
> -		sve_set_vq(sve_vq_from_vl(task_get_sve_vl(current)) - 1);
> -		restore_sve_regs = true;
> -		restore_ffr = true;
> +	if (system_supports_sve()) {
> +		switch (current->thread.fp_type) {
> +		case FP_STATE_FPSIMD:
> +			/* Stop tracking SVE for this task until next use. */
> +			if (test_and_clear_thread_flag(TIF_SVE))
> +				sve_user_disable();
> +			break;
> +		case FP_STATE_SVE:
> +			if (!thread_sm_enabled(&current->thread) &&
> +			    !WARN_ON_ONCE(!test_and_set_thread_flag(TIF_SVE)))
> +				sve_user_enable();
> +
> +			if (test_thread_flag(TIF_SVE))
> +				sve_set_vq(sve_vq_from_vl(task_get_sve_vl(current)) - 1);
> +
> +			restore_sve_regs = true;
> +			restore_ffr = true;
> +			break;
> +		default:
> +			/*
> +			 * This should never happen, we should always
> +			 * record what we saved when we save. We
> +			 * always at least have the memory allocated
> +			 * for FPSMID registers so try that and hope
> +			 * for the best.
> +			 */
> +			WARN_ON_ONCE(1);
> +			clear_thread_flag(TIF_SVE);
> +			break;

What makes it impossible for FP_STATE_TASK to reach this point? If
that's indeed an impossible case, please document it.

> +		}
>  	}
>  
>  	/* Restore SME, override SVE register configuration if needed */
> @@ -412,10 +437,8 @@ static void task_fpsimd_load(void)
>  		if (thread_za_enabled(&current->thread))
>  			za_load_state(current->thread.za_state);
>  
> -		if (thread_sm_enabled(&current->thread)) {
> -			restore_sve_regs = true;
> +		if (thread_sm_enabled(&current->thread))
>  			restore_ffr = system_supports_fa64();
> -		}
>  	}
>  
>  	if (restore_sve_regs) {

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm