Re: [PATCH 04/20] arm64/fpsimd: signal: Mandate SVE payload for streaming-mode state

[Will Deacon <will@kernel.org>]

On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> Non-streaming SVE state may be preserved without an SVE payload, in
> which case the SVE context only has a header with VL==0, and all state
> can be restored from the FPSIMD context. Streaming SVE state is always
> preserved with an SVE payload, where the SVE context header has VL!=0,
> and the SVE_SIG_FLAG_SM flag is set.
> 
> The kernel never preserves an SVE context where SVE_SIG_FLAG_SM is set
> without an SVE payload. However, restore_sve_fpsimd_context() doesn't
> forbid restoring such a context, and will handle this case by clearing
> PSTATE.SM and restoring the FPSIMD context into non-streaming mode,
> which isn't consistent with the SVE_SIG_FLAG_SM flag.
> 
> Forbid this case, and mandate an SVE payload when the SVE_SIG_FLAG_SM
> flag is set. This avoids an awkward ABI quirk and reduces the risk that
> later rework to this code permits configuring a task with PSTATE.SM==1
> and fp_type==FP_STATE_FPSIMD.
> 
> I've marked this as a fix given that we never intended to support this
> case, and we don't want anyone to start relying upon the old behaviour
> once we re-enable SME.
> 
> Fixes: 85ed24dad290 ("arm64/sme: Implement streaming SVE signal handling")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/signal.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index fdce1b856f498..e898948a31b65 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -387,6 +387,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	unsigned int vl, vq;
>  	struct user_fpsimd_state fpsimd;
>  	u16 user_vl, flags;
> +	bool sm;
>  
>  	if (user->sve_size < sizeof(*user->sve))
>  		return -EINVAL;
> @@ -396,7 +397,8 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	if (err)
>  		return err;
>  
> -	if (flags & SVE_SIG_FLAG_SM) {
> +	sm = flags & SVE_SIG_FLAG_SM;
> +	if (sm) {
>  		if (!system_supports_sme())
>  			return -EINVAL;
>  
> @@ -416,7 +418,16 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user)
>  	if (user_vl != vl)
>  		return -EINVAL;
>  
> -	if (user->sve_size == sizeof(*user->sve)) {
> +	/*
> +	 * Non-streaming SVE state may be preserved without an SVE payload, in
> +	 * which case the SVE context only has a header with VL==0, and all
> +	 * state can be restored from the FPSIMD context.
> +	 *
> +	 * Streaming SVE state is always preserved with an SVE payload. For
> +	 * consistency and robustness, reject restoring streaming SVE state
> +	 * without an SVE payload.
> +	 */
> +	if (!sm && user->sve_size == sizeof(*user->sve)) {
>  		clear_thread_flag(TIF_SVE);
>  		current->thread.svcr &= ~SVCR_SM_MASK;
>  		current->thread.fp_type = FP_STATE_FPSIMD;

This (along with the 'fpsimd_only:' block) is now practically identical
to restore_fpsimd_context(). I think that's a sign that we're doing the
right thing, but can we move that into a shared helper function?

Will