Re: [RFC PATCH v2 17/25] KVM: nSVM: Service local TLB flushes before nested transitions

[Yosry Ahmed <yosry@kernel.org>]

On Thu, Jul 23, 2026 at 06:34:16AM -0700, Sean Christopherson wrote:
> On Thu, Jul 23, 2026, Yosry Ahmed wrote:
> > On Wed, Jul 22, 2026 at 05:46:31PM -0700, Sean Christopherson wrote:
> > > On Tue, Jun 16, 2026, Yosry Ahmed wrote:
> > > > On Tue, Jun 16, 2026 at 11:06 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > > > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > > > > index 234724d8b4c54..7b19191e0e43f 100644
> > > > > > > --- a/arch/x86/kvm/svm/nested.c
> > > > > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > > > > @@ -717,9 +717,16 @@ static void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_v
> > > > > > >  {
> > > > > > >       struct kvm_vcpu *vcpu = &svm->vcpu;
> > > > > > >
> > > > > > > +     /*
> > > > > > > +      * TLB flushes are applied to the VMCB, so apply any pending TLB flushes
> > > > > > > +      * on the current VMCB before switching to a new one..
> > > > > > > +      */
> > > > > > > +     kvm_service_local_tlb_flush_requests(vcpu);
> > > > > >
> > > > > > [Severity: High]
> > > > > > Does this clear global TLB flush requests (like KVM_REQ_TLB_FLUSH_GUEST)
> > > > > > while applying them only to the outgoing VMCB?
> > > > > >
> > > > > > If a host-initiated flush occurs (e.g., from page reclaim), the request
> > > > > > bit will be consumed and applied to the outgoing VMCB, leaving the incoming
> > > > > > VMCB to miss the flush and run with a stale TLB.
> > > > >
> > > > > Yeah it is intentional to apply the flushes to the
> > > > > current/old/outgoing context. If there are flushes needed in the new
> > > > > context, they are queued by nested_svm_{entry/exit}_tlb_flush().
> > > > > That's similar to what VMX does.
> > > > >
> > > > > >
> > > > > > Also, is there a context mismatch here during nested VM-Exit?
> > > > > >
> > > > > > In nested_svm_vmexit(), leave_guest_mode(vcpu) is called before
> > > > > > svm_switch_vmcb(svm, &svm->vmcb01).
> > > > > >
> > > > > > Because of this, kvm_service_local_tlb_flush_requests() will see
> > > > > > is_guest_mode(vcpu) as false. If Hyper-V is enabled, this means
> > > > > > kvm_hv_purge_tlb_flush_fifo() will incorrectly target L1's FIFO while the
> > > > > > hardware flushes are actually being applied to L2's vmcb02.
> > > > >
> > > > > Ugh.. yes. This is annoying. kvm_service_local_tlb_flush_requests()
> > > > > needs to be called on both the current/old/outgoing VMCB *and* guest
> > > > > mode. So we'll need to open-code the call in a bunch of places before
> > > > > svm_switch_vmcb() and {enter/leave}_guest_mode(). I really liked
> > > > > putting it in svm_switch_vmcb() together with
> > > > > nested_svm_{entry/exit}_tlb_flush() so that all the TLB flushing logic
> > > > > for nested transitions live in one place and the ordering needs to be
> > > > > handled in one place.
> > > > 
> > > > Maybe we can just re-order the code to always call svm_switch_vmcb()
> > > > before {enter/leave}_guest_mode(). We already do that on the entry
> > > > side, and seems to be straightforward on the exit side.
> > > 
> > > As stated earlier, I'd prefer to explicitly do flushing stuff where it fits from
> > > an architectural perspective.
> > 
> > I did it this way because (as you also stated) it's more robust, and it
> > also documents the ordering requirements:
> > - We need to service local flushes before switching to the new VMCB.
> 
> No, that's not the requirement.  The requirement is that KVM faithlyfully emulates
> the SVM architecture.  For KVM's implementation, that _mostly_ aligns with
> switching between vmcb01 and vmcb02, but that's not a hard guarantee.  Unlike VMX,
> SVM doesn't force KVM doesn't need to switch the active VMCB in order to make
> changes to a VMCB, so nSVM may never end up with as many switches as nVMX that
> don't need to trigger a TLB flush, but conceptually it's still inaccurate.
> 
> And regarding robustness, the flaw Sashiko pointed out regarding servicing pending
> flushes after leave_guest_mode(vcpu) highlights that burying architectural behaviors
> in what are effectively utility functions can be dangerous.  The counter-argument is
> that we ended up with a similar bug in nVMX where KVM straight up forgot to service
> the flushes, but my point is that handling this in svm_switch_vmcb() isn't a silver
> bullet.  And I really don't like adding an arbitrary constraint that the "new" VMCB
> needs to "match" the current L1 vs. L2 mode.
> 
> > - We need to queue new flushes after servicing local flushes.
> > 
> > I like that it's all in one place. All that being said, I did go
> > back-and-forth on this so I am not opposed to open-coding it. Let me know if
> > the above argument swayed you or if you still prefer open-coding.
> 
> I still prefer open-coding the calls to perform/request/serivce flushes.

For the record, I had locally added a warning to svm_switch_vmcb() to
make sure that guest_mode and the active VMCB are consistent when
kvm_service_local_tlb_flush_requests() is called. Given that we are
going with open coding the calls, I will add a wrapper for the
warn+call:

static void nested_svm_service_local_tlb_flushes(struct vcpu *vcpu)
{
        struct vcpu_svm *svm = to_svm(vcpu);

        /*
         * TLB flushes are applied to the VMCB, so apply any pending TLB flushes
         * on the outgoing VMCB before switching to a new one. A TLB flush could
         * purge the relevant HV TLB flush FIFO depending on guest_mode, so make
         * sure the VMCB and guest_mode context is consistent.
         */
        WARN_ON_ONCE(is_guest_mode(vcpu) != (svm->vmcb == svm->nested.vmcb02.ptr));
        kvm_service_local_tlb_flush_requests(vcpu);
}