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

[Mark Rutland <mark.rutland@arm.com>]

On Wed, May 07, 2025 at 01:59:16PM +0100, Will Deacon wrote:
> On Tue, May 06, 2025 at 04:25:07PM +0100, Mark Rutland wrote:
> > @@ -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?

Largely yes, but there's a subtlety.

We can call restore_fpsimd_context() directly for the block above, since
that's equivalent as of the prior patch.

The "fpsimd_only" label is misleading, and is used in two distinct
cases:

(1) When the SVE context has no register state, that's used to resrtore
    an FPSIMD-only context, and is (now) equivalent to
    restore_fpsimd_context().

(2) When the SVE context has register state, we restore the SVE state,
    then fall through to the "fpsimd_only" block to merge the the FPSIMD
    registers into the SVE registers, preserving the upper bits.

    The merging is hidden opaquely in fpsimd_update_current_state().

So we can call restore_fpsimd_context() directly for the !SVE case, and
we can factor out reading the user_fpsimd_state, but for case 2 we MUST
NOT mess with TIF_SVE, SVCR, or fp_type, and MUST merge the register
contents.

If we do that, I'd want to have restore_fpsimd_context() directly assign
to current->thread.uw.fpsimd_state, and replace
restore_sve_fpsimd_context() with a new fpsimd_merge_into_sve() helper
or similar.

If that makes sense, I can go prep tha as a follow-up?

Mark.