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

[Mark Brown]

On Tue, Nov 21, 2023 at 12:45:48PM -0500, Ard Biesheuvel wrote:
> On Tue, 21 Nov 2023 at 11:56, Mark Brown <broonie at kernel.org> wrote:
> > On Tue, Nov 21, 2023 at 02:52:15PM +0100, Ard Biesheuvel wrote:

> > > So introduce a thread_info flag TIF_USING_KMODE_NEON, and modify the
> > > context switch hook for FPSIMD to preserve resp. restore the kernel mode

> > Can we bikeshed this to TIF_USING_KMODE_FP please?

> TIF_USING_KMODE_FPSIMD perhaps?

Sure - my goal was to avoid wanting to rename the flag if we use the
vector extensions, but then we already use FPSIMD for all FP related
state so that's fine also.

> > This all works but I'm having a hard time loving the fact that it's
> > not integrated with the cleverness that we're doing with KVM to switch
> > out where a FP save gets written to, we've open coded a FPSIMD only
> > register save and we'll have to extend that to support SVE or SME (which
> > I'm pretty sure we'll want to do eventually).

> How so? KVM only deals with user mode (guest) FPSIMD state, and never
> uses the FPSIMD unit in kernel mode.

Given that we only have one copy of the FP registers which is shared
between all ELs I'm just thinking of this in terms of the FP registers,
what owns them and where we write their contents to rather than that
they're particularly user registers - the only thing that's specifically
user mode about the existing fpsimd_save() is that it's got the check
for TIF_FOREIGN_FPSTATE in there.  It is also called fpsimd_save() so it
is where I'd expect to find all the code that saves FP things.  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().  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.

> Also, I don't think it makes lots of sense to complicate things now in
> anticipation of kernel mode SVE/SME, which doesn't seem that
> compelling to me. (The primary use case for kernel mode NEON is the
> crypto extensions, which are only accessible via NEON registers to
> begin with, and which provide a 5x-15x performance boost compared to
> scalar code. SVE may support those crypto extensions too, but that
> doesn't necessarily imply they will actually be faster than plain
> NEON.)

The preparation for kernel mode SVE/SME is as much a nice fallout that I
noticed as it is a specific goal, my thinking here is more about
consistency in how we work with the FP state.

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.

> Modulo future support for kernel mode SVE/SME, I don't think there is
> a need for all of this. But of course, I could be the one missing
> something here.

I'm not sure it's a huge refactor, it's mostly just the rename?  It
seemed proportionate to the change anyway.  I've not actually *done* it
though so it might be much more involved than I'm imagining.  I might
have a look tomorrow if I get time, I need to poke at that code anyway
for some other things.

Oh, and BTW I should mention my series for the 2023 DPISA:

   https://lore.kernel.org/linux-arm-kernel/20231114-arm64-2023-dpisa-v2-0-47251894f6a8@kernel.org/

Once that's merged there'll need to be some consideration of FPMR,
though as far as I'm aware nothing in there impacts any crypto stuff so
it should't cause any immediate problems to ignore 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/20231121/b865e9fc/attachment.sig>