On Thu, 21 Oct 2021 16:57:15 +0100, Mark Brown <broonie@xxxxxxxxxx> 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.
