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 03:29:39PM +0100, Will Deacon wrote:
> On Wed, May 07, 2025 at 03:21:46PM +0100, Mark Rutland wrote:
> > 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.
> 
> Right, all I'm suggesting here is to remove the 'fpsimd_only:' label and
> call into a restore_fpsimd_context()-like helper function in the
> FPSIMD-only case. We can leave the SVE case as-is (and potentially
> de-duplicate the copying later on if we want to).

Yep, that makes sense to me.

I reckon that either makes sense to fold that into the prior patch
rather than this one, or to do that as a separate patch.

Any preference?

Mark.