+Peter
On Wed, Jan 05, 2022, David Woodhouse wrote:
> On Thu, 2021-12-23 at 21:24 +0000, Sean Christopherson wrote:
> > Commit e880c6ea55b9 ("KVM: x86: hyper-v: Prevent using not-yet-updated TSC page
> > by secondary CPUs") is squarely to blame as it was added after dirty ring, though
> > in Vitaly's defense, David put it best: "That's a fairly awful bear trap".
>
> Even with the WARN to keep us honest, this is still awful.
>
> We have kvm_vcpu_write_guest()... but the vcpu we pass it is ignored
> and only vcpu->kvm is used. But you have to be running on a physical
> CPU which currently owns *a* vCPU of that KVM, or you might crash.
>
> There is also kvm_write_guest() which doesn't take a vCPU as an
> argument, and looks for all the world like it was designed for you not
> to need one... but which still needs there to be a vCPU or it might
> crash.
>
> I think I want to kill the latter, make the former use the vCPU it's
> given, add a spinlock to the dirty ring which will be uncontended
> anyway in the common case so it shouldn't hurt (?),
IIRC, Peter did a fair amount of performance testing and analysis that led to
the current behavior.
> and then let people use kvm->vcpu[0] when they really need to, with a
> documented caveat that when there are *no* vCPUs in a KVM, dirty tracking
> might not work. Which is fine, as migration of a KVM that hasn't been fully
> set up yet is silly.
"when they really need to" can be a slippery slope, using vcpu[0] is also quite
gross. Though I 100% agree that always using kvm_get_running_vcpu() is awful.
