[PATCH] arm64/sve: Rework SVE access trap to convert state in registers

[Mark Brown]

On Wed, Apr 07, 2021 at 12:45:12PM +0100, Catalin Marinas wrote:
> On Fri, Mar 12, 2021 at 07:03:13PM +0000, Mark Brown wrote:

> > -	/* Force ret_to_user to reload the registers: */
> > -	fpsimd_flush_task_state(current);

> Just to make sure I understand: in the current code, if
> TIF_FOREIGN_FPSTATE is cleared and we trapped (TIF_SVE is also cleared),
> the FPSIMD state is transferred to the SVE one in memory and both
> TIF_SVE and TIF_FOREIGN_FPSTATE get set. The (potentially stale) SVE

Right, TIF_FOREIGN_FPSTATE is unconditionally set in the current code.

> state is subsequently restored via do_notify_resume(). I couldn't find
> where we actually zero the in-memory SVE state or save the latest one
> via do_el0_svc(). So it looks like we may restore some old SVE state
> after a syscall (maybe I'm missing something but it would be nice to
> follow zero or preserved approach).

The state is currently converted via the fpsimd_to_sve() call in the
trap handler, that *should* be dealing with anything required for
conversion.  You're right that this seems to miss zeroing anything
outside of the values it immediately writes - I might be missing
something though.  It's only going to leak information from the current
task and it is within the documented behaviour so it's not a bug but as
such but it does seem cleaner, ought to have a memset() in there
somewhere.

> With your patch in the above scenario, we no longer restore the SVE
> state from memory. We just flush the live SVE registers and disable
> trapping. The assumption here is that (1) when TIF_SVE is cleared, we no
> longer care about any of the SVE state (memory or regs) and (2) we only
> trap if TIF_SVE was cleared (i.e. we don't have a different scenario of
> lazy restoring where TIF_SVE but we still trap). That's fine by me and

Indeed.

> matches the code paths but it wasn't entirely clear in the TIF_SVE
> description higher up in this file.

I'm not seeing what's missing from the discussion there?  It explicitly
says that if TIF_SVE is set we don't trap and if it's clear then we do
trap and which registers are valid in each state which I think makes the
behaviour here clear but perhaps I've been looking at this too much, or
you were looking for this in a slightly different place.

> The other case is TIF_FOREIGN_FPSTATE being set in do_sve_acc(). Since
> we never return to user with this flag set, when can we actually hit the
> 'else' path in your patch?

We'd have to have been preempted between entering the kernel and
actually handling the access trap, I can't think of a scenario where
that would happen but it seemed easier to have the code to handle it
than to make absolutely sure that there was no possible case that I was
missing.  I think we should have the detection code either way in case
it does end up happening somehow but it could be changed to use
WARN_ON_ONCE() or something instead of silently handling it?
-------------- 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/20210407/3308e4f3/attachment.sig>