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

[Ard Biesheuvel]

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?

> Also is "context switch hook ... preserve resp. restore" just "context
> switch hooks to preserve and restore" or am I missing something there?
>

It combines with the to/from that follows, but I'll reword this to
make it less confusing

> > -     /* Save unsaved fpsimd state, if any: */
> > -     fpsimd_save();
> > +     if (!test_thread_flag(TIF_USING_KMODE_NEON)) {
> > +             /* Save unsaved user mode fpsimd state, if any: */
> > +             fpsimd_save();
> > +     } else {
> > +             fpsimd_save_state(&current->thread.kmode_fpsimd_state);
> > +     }
>
> 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.

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.)

> Unless we're going to totally rework how KVM does things it seems like
> it's better to use fpsimd_bind_state_to_cpu() in kernel_neon_begin(),
> but that does raise the complexity with checking TIF_FOREIGN_FPSTATE in
> there.  I *think* what we want here is to do something like rename the
> existing fpsimd_save() to user_fpsimd_save() (or something), pull out
> the actual register->memory bit into a new fpsimd_save() that's only
> used there, in fpsimd_thread_switch() and in the newly added code.  That
> way there's only one place where we have code that knows how to write
> data out to memory and we always have whatever memory backs any live
> data in the registers bound for use there, that seems less likely to go
> wrong in future and easier to add asserts for.
>
> I don't *think* we need to worry about overwriting whatever KVM bound
> there since we're about to set TIF_FOREIGN_FPSTATE and start using the
> registers so we won't be saving again, I could be missing some case
> there though.

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.