[PATCH 2/3] arm64: fpsimd: preserve/restore kernel mode NEON at context switch

[Mark Brown]

On Wed, Nov 22, 2023 at 10:30:15AM +0100, Ard Biesheuvel wrote:
> On Tue, 21 Nov 2023 at 21:25, Mark Brown <broonie at kernel.org> wrote:
> > On Tue, Nov 21, 2023 at 12:45:48PM -0500, Ard Biesheuvel wrote:

> Kernel mode FPSIMD state is always preserved eagerly and always
> restored eagerly, and always involves the contents of the NEON
> registers only. So what do we gain by forcing ourselves to use the
> complicated logic for a very simple use case that is completely
> disjoint?

I think a key difference between us here is that I don't see the use
case as completely disjoint - I see the floating point state of the
hardware as being the primary point from which everything else flows.
TBH I wouldn't even think about what's going on in fpsimd_save() as
relevant complexity, it's all encapsulated in there anyway.

> >  It is also called fpsimd_save() so it
> > is where I'd expect to find all the code that saves FP things.

> I can rename fpsimd_save() to fpsimd_save_user_state() and add a
> fpsimd_save_kernel_state() that encapsulates the fpsimd_save_state()
> call if that makes things clearer.

I think that'd definitely help.

> >  I would
> > also expect that anything that takes ownership of the registers for use
> > in a preemptable context where we might need to save would be using
> > bind_state_to_cpu().

> Why? This is part of the lazy restore logic that we have no need for.

It's mainly a code smell thing with making sure that things can be
followed and nobody forgets that something needs updates in future -
it's not that there's any actual problem now but rather about trying to
reduce the chances of surprises for anyone looking at the code in the
future.

> >  Adding new, independent paths for managing the
> > live register state feels like it's making things harder to reason
> > about.

> > To me what this change does is add another potential owner of the live
> > FP state, I wouldn't expect the fact that this user is for kernel mode
> > to change how we keep track of who owns that state.

> The difference is that kernel mode FPSIMD state will always be
> reloaded eagerly when switching to the task. None of the load/save
> logic that reasons about who owns what, whether the current state is
> the most recent one or whether it is SVE or SME is needed here.

It's about reducing the number of cases that the rest of the code needs
to think about rather than about the patch not working.

> > I do suspect we might see some useful crypto stuff for them at some
> > point as the architecture and implementations advance, but I very much
> > agree that it's premature to actually do anything since we've not yet
> > seen any concrete examples.

> Exactly. And if there are any data points you can share, I'm happy to
> look into this from the crypto side.

This is very much forward looking stuff rather than concrete reality,
there's nothing right now that I'm aware of (though I'd be delighted to
discover I'm wrong!).
-------------- 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/20231122/d231ceb7/attachment.sig>