Re: [PATCH v6 16/31] KVM: nSVM: Unify handling of VMRUN failures with proper cleanup

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 24, 2026, Yosry Ahmed wrote:
> There are currently two possible causes of VMRUN failures emulated by
> KVM:
> 
> 1) Consistency checks failures. In this case, KVM updates the exit code
>    in the mapped VMCB12 and exits early in nested_svm_vmrun(). This
>    causes a few problems:
> 
>   A) KVM does not clear the GIF if the early consistency checks fail
>      (because nested_svm_vmexit() is not called). Nothing requires
>      GIF=0 before a VMRUN, from the APM:
> 
> 	It is assumed that VMM software cleared GIF some time before
> 	executing the VMRUN instruction, to ensure an atomic state
> 	switch.
> 
>      So an early #VMEXIT from early consistency checks could leave the
>      GIF set.
> 
>   B) svm_leave_smm() is missing consistency checks on the newly loaded
>      guest state, because the checks aren't performed by
>      enter_svm_guest_mode().

This is flat out wrong.  RSM isn't missing any consistency checks that are
provided by nested_vmcb_check_save().

	if (CC(!(save->efer & EFER_SVME)))                                    <=== irrelevant given KVM's implementation
		return false;

	if (CC((save->cr0 & X86_CR0_CD) == 0 && (save->cr0 & X86_CR0_NW)) ||  <== kvm_set_cr0() in rsm_enter_protected_mode()
	    CC(save->cr0 & ~0xffffffffULL))
		return false;

	if (CC(!kvm_dr6_valid(save->dr6)) || CC(!kvm_dr7_valid(save->dr7)))   <== kvm_set_dr() in rsm_load_state_{32,64}
		return false;

	/*
	 * These checks are also performed by KVM_SET_SREGS,
	 * except that EFER.LMA is not checked by SVM against
	 * CR0.PG && EFER.LME.
	 */
	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) { 
		if (CC(!(save->cr4 & X86_CR4_PAE)) ||                         <== kvm_set_cr4() in rsm_enter_protected_mode()
		    CC(!(save->cr0 & X86_CR0_PE)) ||                          <== kvm_set_cr0() in rsm_enter_protected_mode()
		    CC(!kvm_vcpu_is_legal_cr3(vcpu, save->cr3)))              <== kvm_set_cr3() in rsm_enter_protected_mode()
			return false;
	}

	/* Note, SVM doesn't have any additional restrictions on CR4. */
	if (CC(!__kvm_is_valid_cr4(vcpu, save->cr4)))                         <== kvm_set_cr4() in rsm_enter_protected_mode()
		return false;

	if (CC(!kvm_valid_efer(vcpu, save->efer)))                            <== __kvm_emulate_msr_write() in rsm_load_state_64()
		return false;

Even if RSM were missing checks on the L2 state being loaded, I'm not willing to
take on _any_ complexity in nested VMRUN to make RSM suck a little less.  KVM's
L2 => SMM => RSM => L2 is fundamentally broken.  Anyone that argues otherwise is
ignoring architecturally defined behavior in the SDM and APM.

If someone wants to actually put in the effort to properly emulating SMI => RSM
from L2, then I'd be happy to take on some complexity, but even then it's not at
all clear that it would be necessary.

> 2) Failure to load L2's CR3 or merge the MSR bitmaps. In this case, a
>    fully-fledged #VMEXIT injection is performed as VMCB02 is already
>    prepared.
> 
> Arguably all VMRUN failures should be handled before the VMCB02 is
> prepared, but with proper cleanup (e.g. clear the GIF).

Eh, so long as KVM cleans up after itself, I don't see anything wrong with
preparing some of vmcb02.

So after staring at this for some time, us having gone through multiple attempts
to get things right, and this being tagged for stable@, unless I'm missing some
massive simplification this provides down the road, I am strongly against refactoring
this code, and 100% against reworking things to "fix" SMM.

And so for the stable@ patches, I'm also opposed to all of these:

  KVM: nSVM: Refactor minimal #VMEXIT handling out of nested_svm_vmexit()
  KVM: nSVM: Call nested_svm_init_mmu_context() before switching to VMCB02
  KVM: nSVM: Call nested_svm_merge_msrpm() from enter_svm_guest_mode()
  KVM: nSVM: Make nested_svm_merge_msrpm() return an errno
  KVM: nSVM: Call enter_guest_mode() before switching to VMCB02
  KVM: nSVM: Drop nested_vmcb_check_{save/control}() wrappers

unless they're *needed* by some later commit (I didn't look super closely).

For stable@, just fix the GIF case and move on.

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index d734cd5eef5e..d9790e37d4e8 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1036,6 +1036,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
                vmcb12->control.exit_code    = SVM_EXIT_ERR;
                vmcb12->control.exit_info_1  = 0;
                vmcb12->control.exit_info_2  = 0;
+               svm_set_gif(svm, false);
                goto out;
        }

Sorry for not catching this earlier, I didn't actually read the changelog until
this version.  /facepalm