[PATCH v7 2/2] arm64/sve: Rework SVE trap access to minimise memory access

[Mark Brown]

On Wed, Feb 10, 2021 at 11:09:56AM +0000, Dave Martin wrote:
> On Mon, Feb 01, 2021 at 12:29:01PM +0000, Mark Brown wrote:

> > +	/*
> > +	 * When the FPSIMD state is loaded:
> > +	 *      - The return path (see fpsimd_restore_current_state) requires
> > +	 *        the vector length to be loaded beforehand.
> > +	 *      - We need to rebind the task to the CPU so the newly allocated
> > +	 *        SVE state is used when the task is saved.
> > +	 */
> > +	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> > +		sve_set_vq(sve_vq_from_vl(current->thread.sve_vl) - 1);

> Hmmm, I can see why we need this here, but it feels slightly odd.
> Still, I don't have a better idea.

> Logically, this is all part of a single state change, where we
> transition from live FPSIMD-only state in the registers to live SVE
> state with a pending flush.  Although we could wrap that up in a helper,
> we only do this particular transition here so I guess factoring it out
> may not be worth it.

Yes, such a helper would have exactly one user so it's a bit unclear if
it's sensible to factor it out.

> > +		fpsimd_bind_task_to_cpu();
> > +	}
> >  
> >  	put_cpu_fpsimd_context();

> From here, can things go wrong if we get preempted and scheduled out?

> I think fpsimd_save() would just set TIF_SVE_FULL_REGS and save out the
> full register data, which may contain stale data in the non-FPSIMD bits
> because we haven't flushed them yet.

> Assuming I've not confused myself here, the same think could probably
> happen with Ard's changes if a softirq uses kernel_neon_begin(), causing
> fpsimd_save() to get called.

I think the issue here is actually in fpsimd_save() which as you said in
one of your other mails shouldn't be forcing on TIF_SVE_FULL_REGS, it
should just be saving whatever is specified by TIF_SVE_FULL_REGS.  That
way if we get scheduled between do_sve_acc() and returning to userspace
only the FPSIMD registers will be saved and we'll set TIF_SVE_FULL_REGS
and do the flush when reloading the registers prior to returning to
userspace.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20210210/498b2d76/attachment.sig>