Re: [PATCH 15/20] arm64/fpsimd: ptrace/prctl: Ensure VL changes leave task in a valid state

[Will Deacon <will@kernel.org>]

On Wed, May 07, 2025 at 06:10:07PM +0100, Mark Rutland wrote:
> On Wed, May 07, 2025 at 05:12:47PM +0100, Will Deacon wrote:
> > On Tue, May 06, 2025 at 04:25:18PM +0100, Mark Rutland wrote:
> > > +	/*
> > > +	 * Always preserve PSTATE.SM and the effective FPSIMD state, zeroing
> > > +	 * other SVE state.
> > > +	 */
> > > +	fpsimd_sync_from_effective_state(task);
> > > +	task_set_vl(task, type, vl);
> > > +	kfree(task->thread.sve_state);
> > > +	task->thread.sve_state = sve_state;
> > > +	fpsimd_sync_to_effective_state_zeropad(task);
> > > +
> > > +	if (type == ARM64_VEC_SME) {
> > > +		task->thread.svcr &= ~SVCR_ZA_MASK;
> > > +		kfree(task->thread.sme_state);
> > > +		task->thread.sme_state = sme_state;
> > > +	}
> > 
> > I'm probably just missing something here, but how does this address
> > problem three from the commit message where exiting streaming mode
> > should zero the bottom bits of the vector registers?
> 
> The idea is that we no longer exit streaming mode, so we don't need to zero the
> bottom bits.

Argh, I mis-read the svcr manipulation above as clearing SM rather than
ZA. Now that makes a lot more sense, thanks!

> Instead, when either the SVE or SME vector length changes, we
> consitently truncate the (streaming or non-streaming) SVE registers to
> 128-bits, but preserve the existing value of PSTATE.SM and the saved
> fp_type.
> 
> That all happens in:
> 
> 	fpsimd_sync_from_effective_state(task);
> 	task_set_vl(task, type, vl);
> 	kfree(task->thread.sve_state);
> 	task->thread.sve_state = sve_state;
> 	fpsimd_sync_to_effective_state_zeropad(task);
> 
> ... where:
> 
> * If the task's state was initially stored in FPSIMD format, the
>   fpsimd_sync_from_effective_state() and
>   fpsimd_sync_to_effective_state_zeropad() functions do not touch the
>   saved FPSIMD state, leaving the low 128 bits intact.
> 
> * If the task's state was initially stored in SVE format, whether
>   streaming or non-streaming, the fpsimd_sync_from_effective_state() and
>   fpsimd_sync_to_effective_state_zeropad() functions copy the low 128
>   bits into the FPSIMD storage, then copy that back into the new zeroed
>   SVE storage.
> 
> That's what I was trying to describe in the commit message where I said:
> 
> | To fix the second issue, we either need to clear PSTATE.SM or not change
> | the saved fp_type. Given we're going to eagerly allocate sve_state and
> | sme_state, the simplest option is to preserve PSTATE.SM and the saves
> | fp_type, and consistently truncate the SVE state. This ensures that the
> | task always stays in a valid state.
> 
> ... but I appreciate that mentioned the second issue and not the third.
> 
> I can add a note there, if it'd help?

Nah, I just need to learn to read properly.

Will