Re: [PATCH 4/4] arm64/fpsimd: Document the use of TIF_FOREIGN_FPSTATE by KVM

[Marc Zyngier <maz@kernel.org>]

On Thu, 21 Oct 2021 16:57:15 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Thu, Oct 21, 2021 at 04:11:24PM +0100, Marc Zyngier wrote:
> > The bit of documentation that talks about TIF_FOREIGN_FPSTATE
> > does not mention the ungodly tricks that KVM plays with this flag.
> > 
> > Try and document this for the posterity.
> 
> Yes, more documentation here would definitely be helpful - it's pretty
> hard to follow what KVM is doing here.
> 
> >   * CPU currently contain the most recent userland FPSIMD state of the current
> > - * task.
> > + * task *or* the state of the corresponding KVM vcpu if userspace is behaving
> > + * as a VMM and that the vcpu has used FP during its last run. In the latter
> > + * case, KVM will set TIF_FOREIGN_FPSTATE on kvm_vcpu_put(). For all intents
> > + * and purposes, the vcpu FP state is treated identically to userspace's.
> 
> I'm not able to find a kvm_vcpu_put() function in upstream, just
> kvm_cpu_put_sysregs_vhe().  There's kvm_arch_vcpu_put() which is called
> from the vcpu_put() function in generic KVM code but they don't show up
> until you start mangling the name in that comment.

You, vcpu_put() is the one I had in mind.

> It'd be good to
> mention what vcpu_put() is actually doing and a bit more about the
> general model, KVM is behaving differently here AFAICT in that it flags
> the current state as invalid when it saves the context to memory rather
> than when an event happens that requires that the context be reloaded.
> There's no problem there but it's a bit surprising due the difference
> and worth highlighting.

There is a bit more to it: KVM flags the userspace state as invalid,
but also ties the guest state to the current task via
fpsimd_bind_state_to_cpu() so that the state can be saved on
vcpu_put() via fpsimd_save_and_flush_cpu_state(), or if we end-up
running kernel_neon_begin() because of some softirq handling.

> I think I'd also be inclined to restructure this to foreground the fact
> that it's the state of the current task but that task may be a VMM.  So
> something more like
> 
> 	...contain the most recent FPSIMD state of the current userspace
> 	task.  If the task is behaving as a VMM then this will be
> 	managed by KVM which will...
> 
> making it a bit easier to follow (assuming my understanding of what's
> going on is correct, if not then I guess something else needs
> clarifying!).

I'll have a go at rewriting this.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm