Re: [PATCH 4/7] KVM: nVMX: Process each host MSR entry at most once when unwinding failed VM-Entry

[Sean Christopherson <seanjc@google.com>]

On Mon, Nov 02, 2026, Jim Mattson wrote:
> On Mon, Nov 2, 2026 at 3:19 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > From: Sean Christopherson <seanjc@google.com>
> >
> > Add a missing "break" in the inner for-loop of the open coded mess that
> > unwinds MSRs back to the pre-VM-Enter state when VMLAUNCH/VMRESUME VM-Fails
> > due to a consistency check that is caught by hardware but missed by KVM.
> 
> That sentence does not accurately describe what the code does.

"attempts to unwind MSRs back to the pre-VM-Enter state"

> > I.e. move on to the next MSR in the VM-Entry load list after performing a
> > write using a value found in the VM-Exit load list.  Processing *every*
> > matching entry in the VM-Exit load list makes the processing quadratic,
> > which is problematic if the load lists are full of MSRs that are expensive
> > to emulate.  E.g. writing APIC_BASE triggers kvm_recalculate_apic_map() and
> > a whole pile of unnecessary work.
> >
> > Functionally, arbitrarily restoring the value of the first matching entry
> > is "fine", because none of this is architecturally correct: the load lists
> > are never supposed to have been used in the first place.  In practice, it's
> > likely a moot point as no well-behaved VMM will have multiple, diverging
> > entries in the load lists.
> 
> This isn't "fine." This is disgusting.

The "fine" here refers to what is being changed in *this* patch, which is to
load each MSR at most once, and to use the first value encountered, e.g. versus
the last value.  IMO, that claim stands: in the context of the existing code,
this change is "fine".

> The assumption that the VM-exit MSR-load list will "undo" every
> assignment made by the VM-entry MSR-load list is misguided and wrong.

No one is assuming that.  The comment even says *exactly* what you're saying:

	/*
	 * This nasty bit of open coding is a compromise between blindly
	 * loading L1's MSRs using the exit load lists (incorrect emulation
	 * of VMFail), leaving the nested VM's MSRs in the software model
	 * (incorrect behavior) and snapshotting the modified MSRs (too
	 * expensive since the lists are unbound by hardware).  For each
	 * MSR that was (prematurely) loaded from the nested VMEntry load
	 * list, reload it from the exit load list if it exists and differs
	 * from the guest value.  The intent is to stuff host state as
	 * silently as possible, not to fully process the exit load list.
	 */

> Xen, for instance, puts SPEC_CTRL on the VM-entry MSR-load list, but
> it does not put it on the VM-exit MSR-load list. Instead, after a
> normal VM-exit, it restores the host value in
> vmx_asm_vmexit_handler():

...

> KVM needs to do better.

Meh, KVM is doing pretty darn good, all things considered.  Yeah, this code sucks.
But I'm not convinced the alternatives suck less.  We've gotten the set of missed
consistency checks down to effectively zero, module the TPR mess that I do test
by running warn_on_missed_cc=true.

E.g. for Xen to run afoul of a misbehaving guest, Xen would have botch its
handling of vTPR.

I'm not opposed to trying to figure out a not-horrible way to snapshot a limited,
hardcoded number of MSRs to make KVM less wrong.  But I sure as hell ain't going
to spend cycles on it at this point, because potentially clobbering L1 MSRs when
L1 itself has already screwed up is firmly in "don't care" territory right now.