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

[Sean Christopherson <seanjc@google.com>]

On Fri, Feb 27, 2026, Yosry Ahmed wrote:
> On Fri, Feb 27, 2026 at 4:07 PM Sean Christopherson <seanjc@google.com> wrote:
> > 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.
> 
> For context, this patch (and others you quoted below) were a direct
> result of this discussion in v2:
> https://lore.kernel.org/kvm/aThIQzni6fC1qdgj@google.com/. I didn't
> look too closely into the SMM bug tbh I just copy/pasted that verbatim
> into the changelog.

Well fudge.  I'm sorry.  I obviously got a bit hasty with the whole svm_leave_smm()
thing.  *sigh*

> As for refactoring the code, I didn't really do it for SMM, but I
> think the code is generally cleaner with the single VMRUN failure
> path.

Except for the minor detail of being wrong :-)

And while I agree it's probably "cleaner", I think it's actually harder for
readers to understand, especially for readers that are familiar with nVMX.  E.g.
having a separate #VMEXIT path for consistency checks can actually be helpful,
because it highlights things that happen on #VMEXIT even when KVM hasn't started
loading L2 state.

> That being said..
> 
> > 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.
> 
> .. I am not sure if you mean dropping them completely, or dropping
> them from stable@.

My preference is to completely drop these:

  KVM: nSVM: Unify handling of VMRUN failures with proper cleanup
  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: Call enter_guest_mode() before switching to VMCB02

> I am fine with dropping the stable@ tag from everything from this
> point onward, or re-ordering the patches to keep it for the missing
> consistency checks.

And then moving these to the end of the series (or at least, beyond the stable@
patches):

  KVM: nSVM: Make nested_svm_merge_msrpm() return an errno
  KVM: nSVM: Drop nested_vmcb_check_{save/control}() wrappers

> If you mean drop them completely, it's a bit of a shame because I
> think the code ends up looking much better, but I also understand
> given all the back-and-forth, and the new problem I reported recently
> that will need further refactoring to address (see my other reply to
> the same patch).

After paging more of this stuff back in (it's been a while since I looked at the
equivalent nVMX flow in depth), I'm quite opposed to aiming for a unified #VMEXIT
path for VMRUN.  Although it might seem otherwise at times, nVMX and nSVM didn't
end up with nested_vmx_load_cr3() buried toward the end of their flows purely to
make the code harder to read, there are real dependencies that need to be taken
into account.

And there's also value in having similar flows for nVMX and nSVM, e.g. where most
consistency checks occur before KVM starts loading L2 state.  VMX just happens to
architecturally _require_ that, whereas with SVM it was a naturally consequence
of writing the code.

Without the unification, a minimal #VMEXIT helper doesn't make any sense, and
I don't see any strong justification for shuffling around the order.